feat(javascript-sdk): register-device-name #340

ryanbas21 · 2023-04-24T19:33:00Z

JIRA Ticket

https://bugster.forgerock.org/jira/browse/SDKS-2298

Description

make it so register accepts a parameter for registering a device name. An optional generic parameter that adds a device name to the registration step. This makes it so that ::${devicename} is concated to the device string that is made from the outcome.

Type of Change

Please Delete options that are not relevant

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

See the e2e example, which uses stoyans tenant

  const url = new URL(window.location.href);
  const amUrl =
    url.searchParams.get('amUrl') || 'https://openam-forgerrock-sdksteanant.forgeblocks.com/am/';
  const realmPath = url.searchParams.get('realmPath') || 'alpha';
  const un = url.searchParams.get('un') || 'ryan';
  const pw = url.searchParams.get('pw') || 'Password1!';
  const tree = url.searchParams.get('tree') || 'WebAuthn-Registration';

Click the device registration button and then go through the flow there. You should see the hidden callback have the outcome value with DeviceName (as passed in) concated to the end.

The name can be very long and cut off

Definition of Done

Check all that apply

Documentation

Acceptance criteria met
Spell-check run
Peer reviewed
Proofread

github-actions · 2023-04-24T19:42:28Z

Your preview environment pr-340-ryanbas21 has been deployed.

Preview environment endpoints are available at:

ryanbas21 · 2023-04-24T19:48:54Z

packages/javascript-sdk/project.json

      "executor": "@nrwl/rollup:rollup",
      "options": {
-        "compiler": "babel",
+        "compiler": "tsc",


so typechecking happens on build, was an oversight by me.

nx-cloud · 2023-04-24T20:17:54Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit f845505. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 5 targets

Sent with 💌 from NxCloud.

ryanbas21 · 2023-04-24T21:18:57Z

packages/javascript-sdk/src/fr-webauthn/index.ts

 import { parseWebAuthnAuthenticateText, parseWebAuthnRegisterText } from './script-parser';

+// <clientdata>::<attestation>::<publickeyCredential>::<DeviceName>
+type DeviceName<


This is a type that can create a Device Name if given a name or not

I like the strictness of this type, but the name feels a bit misleading.

ryanbas21 · 2023-04-24T21:19:23Z

packages/javascript-sdk/src/fr-webauthn/index.ts

+  PubKeyCred extends PublicKeyCredential,
+  Name = '',
+> = Name extends infer P extends string
+  ? `${ClientId}::${Attestation}::${PubKeyCred['id']}${P extends '' ? '' : `::${P}`}`


realistically, i thought a simpler type could be used, i have no idea why i couldn't use a simpler type, it only worked when I went this route.

ryanbas21 · 2023-04-24T21:20:15Z

packages/javascript-sdk/src/fr-webauthn/index.ts

      }
-
-      hiddenCallback.setInputValue(outcome);
+      hiddenCallback.setInputValue(deviceName ? `${outcome}::${deviceName}` : outcome);


hypothetically, we could give hiddenCallback a generic that defaults to unknown and then we can provide that here for more type inference and passing.

ryanbas21 · 2023-04-24T21:21:19Z

packages/javascript-sdk/src/fr-webauthn/index.ts


-      let stringOutput = `${clientDataJSON}::${authenticatorData}::${signature}::${credential.id}`;
+      let stringOutput =
+        `${clientDataJSON}::${authenticatorData}::${signature}::${credential.id}` as DeviceName<


Had to force this type, theres probably a way to create a string builder that outputs as the type though that may fit better.

ryanbas21 · 2023-04-24T21:22:10Z

packages/javascript-sdk/src/fr-webauthn/index.ts

+          string,
+          AttestationType,
+          PublicKeyCredential,
+          typeof userHandle


I suppose this will always be string, but if userHandle is ever a more strict type then we could leverage that here.

userHandle is just a username, so it can be any kind of string. This is used for supporting the "usernameless" flow where the username is stored on the WebAuthn device.

Right but one could pass “username” and in typescript 5 we could leverage a const generic which would make the type “username” and not string so it could be used more strictly

ryanbas21 · 2023-04-24T21:26:01Z

packages/javascript-sdk/tsconfig.lib.json

    "target": "ES2020"
  },
-  "include": ["**/*.ts"],
+  "include": ["**/*.ts", "jest.setup.js"],


I think I addedt his when I was trying to find ways to test this with jest, but I can remove it.

Probably should remove it then, if it's not needed.

ryanbas21 · 2023-04-24T21:26:23Z

e2e/autoscript-apps/src/authn-webauthn/index.html

  <body>
    <!-- script src="/_polyfills/fast-text-encoder.js"></script -->
    <button class="login-btn">Login</button>
+    <button class="device-registration">Device Registration</button>


This is just for that manual test I wrote, has no affect on the automation

Oh, I was wondering why the additional flow was added. There's currently no automation around WebAuthn; everything is manually tested. So, nothing should prevent you from adding your feature to the existing flow, rather than create an entirely new flow.

Yeah I only added it because the only difference is that the previous flow didn't add a device name, but i could just add a device name and remvoe this i think if preferred.

The main reason for this flow, is that the tenant i used had a different tree, so I used stoyans tenant and the flow itself requires certs that run without errors.

So my testing was done in a dfiferent env and couldn't use the mock server itself.

cerebrl

This looks good overall. A few additional comments:

Can you fill out the PR description template?
Provide screenshots of the results from testing this new feature from Platform Admin (showing full end-to-end testing)
Rework the name of the new type
I don't think you need to create a whole new flow in the autoscript.ts, but let me know if I'm missing something

cerebrl · 2023-04-25T15:20:01Z

e2e/autoscript-apps/src/authn-webauthn/autoscript.ts

+        console.log('Handle WebAuthn Registration');
+        try {
+          step = await forgerock.FRWebAuthn.register<'mydevice'>(step, 'mydevice');
+          // ensure the step here has the 'mydevice' name at the end of the value. (outcome)


Is this a TODO comment?

No, this was instructions for when you get to this step, you can ensure the callback has the value with mydevice at the end since it concates with a stringifed object its messy.

see here:

Oh, okay. Rather than this comment instructing the manual tester to inspect the step in the console, can we just have a condition in the code to check this for us, similar to the other conditions that are ensuring things are working properly? If mydevice is not present, let's just throw an error like how we handle other issues.

cerebrl · 2023-04-25T15:23:33Z

e2e/autoscript-apps/src/authn-webauthn/index.html

  <body>
    <!-- script src="/_polyfills/fast-text-encoder.js"></script -->
    <button class="login-btn">Login</button>
+    <button class="device-registration">Device Registration</button>


Oh, I was wondering why the additional flow was added. There's currently no automation around WebAuthn; everything is manually tested. So, nothing should prevent you from adding your feature to the existing flow, rather than create an entirely new flow.

cerebrl · 2023-04-25T16:24:16Z

packages/javascript-sdk/src/fr-webauthn/index.ts

    const { hiddenCallback, metadataCallback, textOutputCallback } = this.getCallbacks(step);
    if (hiddenCallback && (metadataCallback || textOutputCallback)) {
-      let outcome: string;
+      let outcome: DeviceName<string, AttestationType, PublicKeyCredential>;


This type reads a bit odd for me. Should the type name be something more like OutcomeWithName, rather than DeviceName?

cerebrl · 2023-04-25T16:25:44Z

packages/javascript-sdk/src/fr-webauthn/index.ts

 import { parseWebAuthnAuthenticateText, parseWebAuthnRegisterText } from './script-parser';

+// <clientdata>::<attestation>::<publickeyCredential>::<DeviceName>
+type DeviceName<


I like the strictness of this type, but the name feels a bit misleading.

cerebrl · 2023-04-25T17:52:33Z

packages/javascript-sdk/src/fr-webauthn/index.ts

+          string,
+          AttestationType,
+          PublicKeyCredential,
+          typeof userHandle


userHandle is just a username, so it can be any kind of string. This is used for supporting the "usernameless" flow where the username is stored on the WebAuthn device.

cerebrl · 2023-04-25T17:53:10Z

packages/javascript-sdk/tsconfig.lib.json

    "target": "ES2020"
  },
-  "include": ["**/*.ts"],
+  "include": ["**/*.ts", "jest.setup.js"],


Probably should remove it then, if it's not needed.

make it so `register` accepts a parameter for registering a device name

cerebrl

LGTM!

ryanbas21 temporarily deployed to Preview April 24, 2023 19:33 — with GitHub Actions Inactive

github-actions bot added NX root sdk test labels Apr 24, 2023

ryanbas21 commented Apr 24, 2023

View reviewed changes

ryanbas21 force-pushed the webauthn branch from ad0c0b0 to 49215b6 Compare April 24, 2023 19:51

ryanbas21 temporarily deployed to Preview April 24, 2023 19:51 — with GitHub Actions Inactive

ryanbas21 commented Apr 24, 2023

View reviewed changes

cerebrl requested changes Apr 25, 2023

View reviewed changes

ryanbas21 force-pushed the webauthn branch from 49215b6 to f2abaea Compare April 25, 2023 19:36

ryanbas21 temporarily deployed to Preview April 25, 2023 19:36 — with GitHub Actions Inactive

ryanbas21 force-pushed the webauthn branch from f2abaea to e5ec474 Compare April 25, 2023 21:26

ryanbas21 temporarily deployed to Preview April 25, 2023 21:26 — with GitHub Actions Inactive

ryanbas21 force-pushed the webauthn branch from e5ec474 to 3444f8e Compare April 25, 2023 21:57

ryanbas21 temporarily deployed to Preview April 25, 2023 21:57 — with GitHub Actions Inactive

ryanbas21 force-pushed the webauthn branch from 3444f8e to 215f0bb Compare April 26, 2023 21:39

ryanbas21 temporarily deployed to Preview April 26, 2023 21:39 — with GitHub Actions Inactive

ryanbas21 force-pushed the webauthn branch from 215f0bb to 908d105 Compare April 26, 2023 21:43

ryanbas21 temporarily deployed to Preview April 26, 2023 21:43 — with GitHub Actions Inactive

ryanbas21 force-pushed the webauthn branch from 908d105 to 177d3c9 Compare April 26, 2023 21:44

ryanbas21 temporarily deployed to Preview April 26, 2023 21:44 — with GitHub Actions Inactive

feat(javascript-sdk): register-device-name

f845505

make it so `register` accepts a parameter for registering a device name

ryanbas21 force-pushed the webauthn branch from 177d3c9 to f845505 Compare April 26, 2023 22:16

ryanbas21 temporarily deployed to Preview April 26, 2023 22:16 — with GitHub Actions Inactive

cerebrl approved these changes Apr 26, 2023

View reviewed changes

ryanbas21 merged commit b99946a into develop Apr 26, 2023

ryanbas21 deleted the webauthn branch August 2, 2024 14:51

feat(javascript-sdk): register-device-name #340

feat(javascript-sdk): register-device-name #340

Uh oh!

Conversation

ryanbas21 commented Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

JIRA Ticket

Description

Type of Change

How Has This Been Tested?

Definition of Done

Documentation

Uh oh!

github-actions bot commented Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nx-cloud bot commented Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

☁️ Nx Cloud Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cerebrl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cerebrl left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

ryanbas21 commented Apr 24, 2023 •

edited

Loading

github-actions bot commented Apr 24, 2023 •

edited

Loading

nx-cloud bot commented Apr 24, 2023 •

edited

Loading