Merge pull request #259 from MasterKale/fix/improve-resident-key-regi…

…stration-options Fix/improve resident key registration options
MasterKale · Aug 19, 2022 · 9e68a48 · 9e68a48
2 parents 24d1442 + ff0e52c
commit 9e68a48
Show file tree

Hide file tree

Showing 2 changed files with 114 additions and 7 deletions.
diff --git a/packages/server/src/registration/generateRegistrationOptions.test.ts b/packages/server/src/registration/generateRegistrationOptions.test.ts
@@ -166,3 +166,90 @@ test('should use custom supported algorithm IDs as-is when provided', () => {
     { alg: -65535, type: 'public-key' },
   ]);
 });
+
+test('should require resident key if residentKey option is absent but requireResidentKey is set to true', () => {
+  const options = generateRegistrationOptions({
+    rpID: 'not.real',
+    rpName: 'SimpleWebAuthn',
+    userID: '1234',
+    userName: 'usernameHere',
+    authenticatorSelection: {
+      requireResidentKey: true,
+    }
+  });
+
+  expect(options.authenticatorSelection?.requireResidentKey).toEqual(true);
+  expect(options.authenticatorSelection?.residentKey).toEqual('required');
+});
+
+test('should discourage resident key if residentKey option is absent but requireResidentKey is set to false', () => {
+  const options = generateRegistrationOptions({
+    rpID: 'not.real',
+    rpName: 'SimpleWebAuthn',
+    userID: '1234',
+    userName: 'usernameHere',
+    authenticatorSelection: {
+      requireResidentKey: false,
+    }
+  });
+
+  expect(options.authenticatorSelection?.requireResidentKey).toEqual(false);
+  expect(options.authenticatorSelection?.residentKey).toBeUndefined();
+});
+
+test('should not set resident key if both residentKey and requireResidentKey options are absent', () => {
+  const options = generateRegistrationOptions({
+    rpID: 'not.real',
+    rpName: 'SimpleWebAuthn',
+    userID: '1234',
+    userName: 'usernameHere',
+  });
+
+  expect(options.authenticatorSelection?.requireResidentKey).toEqual(false);
+  expect(options.authenticatorSelection?.residentKey).toBeUndefined();
+});
+
+test('should set requireResidentKey to true if residentKey if set to required', () => {
+  const options = generateRegistrationOptions({
+    rpID: 'not.real',
+    rpName: 'SimpleWebAuthn',
+    userID: '1234',
+    userName: 'usernameHere',
+    authenticatorSelection: {
+      residentKey: 'required',
+    },
+  });
+
+  expect(options.authenticatorSelection?.requireResidentKey).toEqual(true);
+  expect(options.authenticatorSelection?.residentKey).toEqual('required');
+});
+
+test('should set requireResidentKey to false if residentKey if set to preferred', () => {
+  const options = generateRegistrationOptions({
+    rpID: 'not.real',
+    rpName: 'SimpleWebAuthn',
+    userID: '1234',
+    userName: 'usernameHere',
+    authenticatorSelection: {
+      residentKey: 'preferred',
+    },
+  });
+
+  expect(options.authenticatorSelection?.requireResidentKey).toEqual(false);
+  expect(options.authenticatorSelection?.residentKey).toEqual('preferred');
+});
+
+test('should set requireResidentKey to false if residentKey if set to discouraged', () => {
+  const options = generateRegistrationOptions({
+    rpID: 'not.real',
+    rpName: 'SimpleWebAuthn',
+    userID: '1234',
+    userName: 'usernameHere',
+    authenticatorSelection: {
+      residentKey: 'discouraged',
+    },
+  });
+
+  expect(options.authenticatorSelection?.requireResidentKey).toEqual(false);
+  expect(options.authenticatorSelection?.residentKey).toEqual('discouraged');
+});
diff --git a/packages/server/src/registration/generateRegistrationOptions.ts b/packages/server/src/registration/generateRegistrationOptions.ts
@@ -120,15 +120,35 @@ export function generateRegistrationOptions(
   }));
 
   /**
-   * "Relying Parties SHOULD set [requireResidentKey] to true if, and only if, residentKey is set
-   * to "required""
-   *
-   * See https://www.w3.org/TR/webauthn-2/#dom-authenticatorselectioncriteria-requireresidentkey
+   * Capture some of the nuances of how `residentKey` and `requireResidentKey` how either is set
+   * depending on when either is defined in the options
    */
-  if (authenticatorSelection.residentKey === 'required') {
-    authenticatorSelection.requireResidentKey = true;
+  if (authenticatorSelection.residentKey === undefined) {
+    /**
+     * `residentKey`: "If no value is given then the effective value is `required` if
+     * requireResidentKey is true or `discouraged` if it is false or absent."
+     *
+     * See https://www.w3.org/TR/webauthn-2/#dom-authenticatorselectioncriteria-residentkey
+     */
+    if (authenticatorSelection.requireResidentKey) {
+      authenticatorSelection.residentKey = 'required';
+    } else {
+      /**
+       * FIDO Conformance v1.7.2 fails the first test if we do this, even though this is
+       * technically compatible with the WebAuthn L2 spec...
+       */
+      // authenticatorSelection.residentKey = 'discouraged';
+    }
   } else {
-    authenticatorSelection.requireResidentKey = false;
+    /**
+     * `requireResidentKey`: "Relying Parties SHOULD set it to true if, and only if, residentKey is
+     * set to "required""
+     *
+     * Spec says this property defaults to `false` so we should still be okay to assign `false` too
+     *
+     * See https://www.w3.org/TR/webauthn-2/#dom-authenticatorselectioncriteria-requireresidentkey
+     */
+    authenticatorSelection.requireResidentKey = authenticatorSelection.residentKey === 'required';
   }
 
   return {