feat(elements): Add support for sign in with passkey - #3472

Merged
panteliselef merged 24 commits into
mainfrom
elef/passkey-elements
Jun 11, 2024
Merged

feat(elements): Add support for sign in with passkey#3472
panteliselef merged 24 commits into
mainfrom
elef/passkey-elements

Conversation

@panteliselef

@panteliselefpanteliselef commented May 30, 2024

Copy link
Copy Markdown
Contributor

Description

This PR add support for passkey usage within a SignIn flow

APIs introduced:

  • <SignIn.Passkey />
  • <SignIn.SupportedStrategy name='passkey'>
  • <SignIn.Strategy name='passkey'>
  • <Clerk.Input type='text' passkeyAutofill>

Usage Examples:

  • <SignIn.Passkey />
<SignIn.Stepname='start'><SignIn.Passkey><Clerk.Loading>{isLoading=>(isLoading ? <Spinner/> : 'Use passkey instead')}. </Clerk.Loading></SignIn.Passkey></SignIn.Step>
  • <SignIn.SupportedStrategy name='passkey'>
<SignIn.SupportedStrategyasChildname='passkey'><Button>use passkey</Button></SignIn.SupportedStrategy>
  • <SignIn.Strategy name='passkey'>
<SignIn.Strategyname='passkey'><pclassName='text-sm'>
Welcome back <SignIn.Salutation/>!
</p><CustomSubmit>Continue with Passkey</CustomSubmit></SignIn.Strategy>
  • <Clerk.Input type='text' passkeyAutofill>
<SignIn.Stepname='start'><Clerk.Fieldname='identifier'><Clerk.LabelclassName='sr-only'>Email</Clerk.Label><Clerk.InputpasskeyAutofillplaceholder='Enter your email address'/><Clerk.FieldError/></Clerk.Field>
</SignIn.Step/>

Docs PR: clerk/clerk-docs#1132

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

@changeset-bot

changeset-botBot commented May 30, 2024

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: d16809f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 14 packages
NameType
@clerk/elementsMinor
@clerk/clerk-jsMinor
@clerk/sharedMinor
@clerk/chrome-extensionPatch
@clerk/clerk-expoPatch
@clerk/backendPatch
@clerk/expressPatch
@clerk/fastifyPatch
@clerk/nextjsPatch
@clerk/clerk-reactPatch
@clerk/remixPatch
@clerk/clerk-sdk-nodePatch
@clerk/testingPatch
@clerk/uiPatch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Comment threadpackages/elements/src/internals/machines/sign-in/utils/starting-factors.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/verification.machine.ts Outdated
Comment on lines +372 to +381
case 'passkey': {
return await parent.getSnapshot().context.clerk.client.signIn.authenticateWithPasskey();
}

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When "Submit" is hit we don't want to call attempt, instead call authenticateWithPasskey

Comment on lines +276 to +308
const [isSupported, setIsSupported] = React.useState(false);
React.useEffect(() => {
async function runAutofillPasskey() {
const _isSupported = await isWebAuthnAutofillSupported().catch(() => false);
setIsSupported(_isSupported);
}

// @ts-expect-error - Depending on type the props can be different
if (passthroughProps?.passkeyAutofill) {
runAutofillPasskey();
}

// @ts-expect-error - Depending on type the props can be different
}, [passthroughProps?.passkeyAutofill]);

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be handled inside an actor ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which part are you referring to?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, passkey specific logic shouldn't live in the common input element like this. Let's try to move it to an actor.

Why does passkeyAutofill need to be on the input?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only necessary if we want to support passkey autofill.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we rely on "the platform" here? It doesn't feel necessary to create a separate abstraction if all it takes is specifying a native property on the element, unless we need to execute some specific logic to enable autofill.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to call authenticateWithPasskey internally

Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
@panteliselefpanteliselef changed the title Elef/passkey elementsfeat(elements): Add support for sign in with passkeyMay 30, 2024
Comment on lines +138 to +149
on: {
'AUTHENTICATE.PASSKEY': {
guard: not('isExampleMode'),
target: 'AttemptingPasskey',
reenter: true,
},
SUBMIT: {
guard: not('isExampleMode'),
target: 'Attempting',
reenter: true,
},
},

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enabling "autofill" is kind of fire and forget action, so we should be able to recover from this state and not get stuck. Promise will await until user interact with the field and select a passkey and that may never happen.

Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
@panteliselefpanteliselef self-assigned this May 31, 2024
@panteliselef
panteliselef marked this pull request as ready for review May 31, 2024 09:48
Comment threadpackages/elements/src/internals/machines/sign-in/start.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.types.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/start.types.ts Outdated
Comment thread.changeset/fuzzy-bees-doubt.md Outdated
Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
Comment threadpackages/elements/src/react/sign-in/action/passkey.tsx

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. I'd like to see the passkey specific logic moved out of the input logic.

Comment on lines +276 to +308
const [isSupported, setIsSupported] = React.useState(false);
React.useEffect(() => {
async function runAutofillPasskey() {
const _isSupported = await isWebAuthnAutofillSupported().catch(() => false);
setIsSupported(_isSupported);
}

// @ts-expect-error - Depending on type the props can be different
if (passthroughProps?.passkeyAutofill) {
runAutofillPasskey();
}

// @ts-expect-error - Depending on type the props can be different
}, [passthroughProps?.passkeyAutofill]);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, passkey specific logic shouldn't live in the common input element like this. Let's try to move it to an actor.

Why does passkeyAutofill need to be on the input?

Comment threadpackages/elements/src/internals/machines/sign-in/verification.machine.ts Outdated
Comment thread.changeset/fuzzy-bees-doubt.md Outdated
Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
Comment threadpackages/elements/src/react/sign-in/action/action.tsx Outdated
Comment threadpackages/elements/src/react/sign-in/passkey.tsx Outdated

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Let's move the logic from the component into useInput() to keep the component layer thin.

Any ideas for not referencing the sign in flow actor directly in the common input components?

Comment on lines +612 to +616
React.useEffect(() => {
if (passkeyAutofillSupported) {
signInRouterRef?.send({ type: 'AUTHENTICATE.PASSKEY.AUTOFILL' });
}
}, [passkeyAutofillSupported, signInRouterRef]);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be in the useInput() hook? Ideally the components here are just thin wrappers around the hook, which contains the business logic

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably yes, but if felt like something only this component should care about.

because we are using refs here and not the context, it will not error, but i thought it would ok to not litter useInput with sign in specific logic.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out with the current setup this is a bit hard. useSignInPasskeyAutofill will use the useSelector underneath which would error when the SignInRouter context is not available.

(props: FormInputProps, forwardedRef) => {
const clerk = useClerk();
const passkeyAutofillProp = (props as PasskeyInputProps).passkeyAutofill;
const signInRouterRef = SignInRouterCtx.useActorRef(true);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not crazy about referencing the sign in actor directly in these components, but not sure if there's another way. 🤔

Maybe @tmilewski has an idea?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this better ?

<SignIn.Passkey><Clerk.Input/></SignIn.Passkey>

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see issues with that as well, because SignIn.Passkey wouldn't know whether it needs to pass the autofill prop or not. To avoid leaking the prop to the dom for everything else, but only pass it to Clerk.Input.

Another thought is to have a SignIn.PasskeyInput, but this does not feel right.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are now simply detecting autoComplete="webauthn"

const field = useInput(props);

const hasPasskeyAutofillProp = Boolean(field.props.autoComplete?.includes('webauthn'));
const allowedTypeForPasskey = (['text', 'email'] as FormInputProps['type'][]).includes(field.props.type);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider tel here for phone numbers?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are correct

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@panteliselef
panteliselef enabled auto-merge (squash) June 11, 2024 08:02
@panteliselef
panteliselef merged commit 4ec3f63 into mainJun 11, 2024
@panteliselef
panteliselef deleted the elef/passkey-elements branch June 11, 2024 08:16
This was referenced Jun 11, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@panteliselef@tmilewski@brkalow@LekoArts@clerk-cookie
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all \u003cpre\u003e\u003ccode\u003e blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks"); } } catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); } })(); (function(){ try { var __m = "github.com"; var __re = new RegExp('^' + "github\\.com" + '
Skip to content

feat(elements): Add support for sign in with passkey - #3472

Merged
panteliselef merged 24 commits into
mainfrom
elef/passkey-elements
Jun 11, 2024
Merged

feat(elements): Add support for sign in with passkey#3472
panteliselef merged 24 commits into
mainfrom
elef/passkey-elements

Conversation

@panteliselef

@panteliselefpanteliselef commented May 30, 2024

Copy link
Copy Markdown
Contributor

Description

This PR add support for passkey usage within a SignIn flow

APIs introduced:

  • <SignIn.Passkey />
  • <SignIn.SupportedStrategy name='passkey'>
  • <SignIn.Strategy name='passkey'>
  • <Clerk.Input type='text' passkeyAutofill>

Usage Examples:

  • <SignIn.Passkey />
<SignIn.Stepname='start'><SignIn.Passkey><Clerk.Loading>{isLoading=>(isLoading ? <Spinner/> : 'Use passkey instead')}. </Clerk.Loading></SignIn.Passkey></SignIn.Step>
  • <SignIn.SupportedStrategy name='passkey'>
<SignIn.SupportedStrategyasChildname='passkey'><Button>use passkey</Button></SignIn.SupportedStrategy>
  • <SignIn.Strategy name='passkey'>
<SignIn.Strategyname='passkey'><pclassName='text-sm'>
Welcome back <SignIn.Salutation/>!
</p><CustomSubmit>Continue with Passkey</CustomSubmit></SignIn.Strategy>
  • <Clerk.Input type='text' passkeyAutofill>
<SignIn.Stepname='start'><Clerk.Fieldname='identifier'><Clerk.LabelclassName='sr-only'>Email</Clerk.Label><Clerk.InputpasskeyAutofillplaceholder='Enter your email address'/><Clerk.FieldError/></Clerk.Field>
</SignIn.Step/>

Docs PR: clerk/clerk-docs#1132

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

@changeset-bot

changeset-botBot commented May 30, 2024

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: d16809f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 14 packages
NameType
@clerk/elementsMinor
@clerk/clerk-jsMinor
@clerk/sharedMinor
@clerk/chrome-extensionPatch
@clerk/clerk-expoPatch
@clerk/backendPatch
@clerk/expressPatch
@clerk/fastifyPatch
@clerk/nextjsPatch
@clerk/clerk-reactPatch
@clerk/remixPatch
@clerk/clerk-sdk-nodePatch
@clerk/testingPatch
@clerk/uiPatch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Comment threadpackages/elements/src/internals/machines/sign-in/utils/starting-factors.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/verification.machine.ts Outdated
Comment on lines +372 to +381
case 'passkey': {
return await parent.getSnapshot().context.clerk.client.signIn.authenticateWithPasskey();
}

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When "Submit" is hit we don't want to call attempt, instead call authenticateWithPasskey

Comment on lines +276 to +308
const [isSupported, setIsSupported] = React.useState(false);
React.useEffect(() => {
async function runAutofillPasskey() {
const _isSupported = await isWebAuthnAutofillSupported().catch(() => false);
setIsSupported(_isSupported);
}

// @ts-expect-error - Depending on type the props can be different
if (passthroughProps?.passkeyAutofill) {
runAutofillPasskey();
}

// @ts-expect-error - Depending on type the props can be different
}, [passthroughProps?.passkeyAutofill]);

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be handled inside an actor ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which part are you referring to?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, passkey specific logic shouldn't live in the common input element like this. Let's try to move it to an actor.

Why does passkeyAutofill need to be on the input?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only necessary if we want to support passkey autofill.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we rely on "the platform" here? It doesn't feel necessary to create a separate abstraction if all it takes is specifying a native property on the element, unless we need to execute some specific logic to enable autofill.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to call authenticateWithPasskey internally

Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
@panteliselefpanteliselef changed the title Elef/passkey elementsfeat(elements): Add support for sign in with passkeyMay 30, 2024
Comment on lines +138 to +149
on: {
'AUTHENTICATE.PASSKEY': {
guard: not('isExampleMode'),
target: 'AttemptingPasskey',
reenter: true,
},
SUBMIT: {
guard: not('isExampleMode'),
target: 'Attempting',
reenter: true,
},
},

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enabling "autofill" is kind of fire and forget action, so we should be able to recover from this state and not get stuck. Promise will await until user interact with the field and select a passkey and that may never happen.

Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
@panteliselefpanteliselef self-assigned this May 31, 2024
@panteliselef
panteliselef marked this pull request as ready for review May 31, 2024 09:48
Comment threadpackages/elements/src/internals/machines/sign-in/start.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.types.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/start.types.ts Outdated
Comment thread.changeset/fuzzy-bees-doubt.md Outdated
Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
Comment threadpackages/elements/src/react/sign-in/action/passkey.tsx

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. I'd like to see the passkey specific logic moved out of the input logic.

Comment on lines +276 to +308
const [isSupported, setIsSupported] = React.useState(false);
React.useEffect(() => {
async function runAutofillPasskey() {
const _isSupported = await isWebAuthnAutofillSupported().catch(() => false);
setIsSupported(_isSupported);
}

// @ts-expect-error - Depending on type the props can be different
if (passthroughProps?.passkeyAutofill) {
runAutofillPasskey();
}

// @ts-expect-error - Depending on type the props can be different
}, [passthroughProps?.passkeyAutofill]);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, passkey specific logic shouldn't live in the common input element like this. Let's try to move it to an actor.

Why does passkeyAutofill need to be on the input?

Comment threadpackages/elements/src/internals/machines/sign-in/verification.machine.ts Outdated
Comment thread.changeset/fuzzy-bees-doubt.md Outdated
Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
Comment threadpackages/elements/src/react/sign-in/action/action.tsx Outdated
Comment threadpackages/elements/src/react/sign-in/passkey.tsx Outdated

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Let's move the logic from the component into useInput() to keep the component layer thin.

Any ideas for not referencing the sign in flow actor directly in the common input components?

Comment on lines +612 to +616
React.useEffect(() => {
if (passkeyAutofillSupported) {
signInRouterRef?.send({ type: 'AUTHENTICATE.PASSKEY.AUTOFILL' });
}
}, [passkeyAutofillSupported, signInRouterRef]);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be in the useInput() hook? Ideally the components here are just thin wrappers around the hook, which contains the business logic

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably yes, but if felt like something only this component should care about.

because we are using refs here and not the context, it will not error, but i thought it would ok to not litter useInput with sign in specific logic.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out with the current setup this is a bit hard. useSignInPasskeyAutofill will use the useSelector underneath which would error when the SignInRouter context is not available.

(props: FormInputProps, forwardedRef) => {
const clerk = useClerk();
const passkeyAutofillProp = (props as PasskeyInputProps).passkeyAutofill;
const signInRouterRef = SignInRouterCtx.useActorRef(true);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not crazy about referencing the sign in actor directly in these components, but not sure if there's another way. 🤔

Maybe @tmilewski has an idea?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this better ?

<SignIn.Passkey><Clerk.Input/></SignIn.Passkey>

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see issues with that as well, because SignIn.Passkey wouldn't know whether it needs to pass the autofill prop or not. To avoid leaking the prop to the dom for everything else, but only pass it to Clerk.Input.

Another thought is to have a SignIn.PasskeyInput, but this does not feel right.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are now simply detecting autoComplete="webauthn"

const field = useInput(props);

const hasPasskeyAutofillProp = Boolean(field.props.autoComplete?.includes('webauthn'));
const allowedTypeForPasskey = (['text', 'email'] as FormInputProps['type'][]).includes(field.props.type);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider tel here for phone numbers?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are correct

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@panteliselef
panteliselef enabled auto-merge (squash) June 11, 2024 08:02
@panteliselef
panteliselef merged commit 4ec3f63 into mainJun 11, 2024
@panteliselef
panteliselef deleted the elef/passkey-elements branch June 11, 2024 08:16
This was referenced Jun 11, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@panteliselef@tmilewski@brkalow@LekoArts@clerk-cookie
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat(elements): Add support for sign in with passkey - #3472

Merged
panteliselef merged 24 commits into
mainfrom
elef/passkey-elements
Jun 11, 2024
Merged

feat(elements): Add support for sign in with passkey#3472
panteliselef merged 24 commits into
mainfrom
elef/passkey-elements

Conversation

@panteliselef

@panteliselefpanteliselef commented May 30, 2024

Copy link
Copy Markdown
Contributor

Description

This PR add support for passkey usage within a SignIn flow

APIs introduced:

  • <SignIn.Passkey />
  • <SignIn.SupportedStrategy name='passkey'>
  • <SignIn.Strategy name='passkey'>
  • <Clerk.Input type='text' passkeyAutofill>

Usage Examples:

  • <SignIn.Passkey />
<SignIn.Stepname='start'><SignIn.Passkey><Clerk.Loading>{isLoading=>(isLoading ? <Spinner/> : 'Use passkey instead')}. </Clerk.Loading></SignIn.Passkey></SignIn.Step>
  • <SignIn.SupportedStrategy name='passkey'>
<SignIn.SupportedStrategyasChildname='passkey'><Button>use passkey</Button></SignIn.SupportedStrategy>
  • <SignIn.Strategy name='passkey'>
<SignIn.Strategyname='passkey'><pclassName='text-sm'>
Welcome back <SignIn.Salutation/>!
</p><CustomSubmit>Continue with Passkey</CustomSubmit></SignIn.Strategy>
  • <Clerk.Input type='text' passkeyAutofill>
<SignIn.Stepname='start'><Clerk.Fieldname='identifier'><Clerk.LabelclassName='sr-only'>Email</Clerk.Label><Clerk.InputpasskeyAutofillplaceholder='Enter your email address'/><Clerk.FieldError/></Clerk.Field>
</SignIn.Step/>

Docs PR: clerk/clerk-docs#1132

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

@changeset-bot

changeset-botBot commented May 30, 2024

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: d16809f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 14 packages
NameType
@clerk/elementsMinor
@clerk/clerk-jsMinor
@clerk/sharedMinor
@clerk/chrome-extensionPatch
@clerk/clerk-expoPatch
@clerk/backendPatch
@clerk/expressPatch
@clerk/fastifyPatch
@clerk/nextjsPatch
@clerk/clerk-reactPatch
@clerk/remixPatch
@clerk/clerk-sdk-nodePatch
@clerk/testingPatch
@clerk/uiPatch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Comment threadpackages/elements/src/internals/machines/sign-in/utils/starting-factors.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/verification.machine.ts Outdated
Comment on lines +372 to +381
case 'passkey': {
return await parent.getSnapshot().context.clerk.client.signIn.authenticateWithPasskey();
}

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When "Submit" is hit we don't want to call attempt, instead call authenticateWithPasskey

Comment on lines +276 to +308
const [isSupported, setIsSupported] = React.useState(false);
React.useEffect(() => {
async function runAutofillPasskey() {
const _isSupported = await isWebAuthnAutofillSupported().catch(() => false);
setIsSupported(_isSupported);
}

// @ts-expect-error - Depending on type the props can be different
if (passthroughProps?.passkeyAutofill) {
runAutofillPasskey();
}

// @ts-expect-error - Depending on type the props can be different
}, [passthroughProps?.passkeyAutofill]);

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be handled inside an actor ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which part are you referring to?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, passkey specific logic shouldn't live in the common input element like this. Let's try to move it to an actor.

Why does passkeyAutofill need to be on the input?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only necessary if we want to support passkey autofill.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we rely on "the platform" here? It doesn't feel necessary to create a separate abstraction if all it takes is specifying a native property on the element, unless we need to execute some specific logic to enable autofill.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to call authenticateWithPasskey internally

Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
@panteliselefpanteliselef changed the title Elef/passkey elementsfeat(elements): Add support for sign in with passkeyMay 30, 2024
Comment on lines +138 to +149
on: {
'AUTHENTICATE.PASSKEY': {
guard: not('isExampleMode'),
target: 'AttemptingPasskey',
reenter: true,
},
SUBMIT: {
guard: not('isExampleMode'),
target: 'Attempting',
reenter: true,
},
},

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enabling "autofill" is kind of fire and forget action, so we should be able to recover from this state and not get stuck. Promise will await until user interact with the field and select a passkey and that may never happen.

Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
@panteliselefpanteliselef self-assigned this May 31, 2024
@panteliselef
panteliselef marked this pull request as ready for review May 31, 2024 09:48
Comment threadpackages/elements/src/internals/machines/sign-in/start.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.types.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/start.types.ts Outdated
Comment thread.changeset/fuzzy-bees-doubt.md Outdated
Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
Comment threadpackages/elements/src/react/sign-in/action/passkey.tsx

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. I'd like to see the passkey specific logic moved out of the input logic.

Comment on lines +276 to +308
const [isSupported, setIsSupported] = React.useState(false);
React.useEffect(() => {
async function runAutofillPasskey() {
const _isSupported = await isWebAuthnAutofillSupported().catch(() => false);
setIsSupported(_isSupported);
}

// @ts-expect-error - Depending on type the props can be different
if (passthroughProps?.passkeyAutofill) {
runAutofillPasskey();
}

// @ts-expect-error - Depending on type the props can be different
}, [passthroughProps?.passkeyAutofill]);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, passkey specific logic shouldn't live in the common input element like this. Let's try to move it to an actor.

Why does passkeyAutofill need to be on the input?

Comment threadpackages/elements/src/internals/machines/sign-in/verification.machine.ts Outdated
Comment thread.changeset/fuzzy-bees-doubt.md Outdated
Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
Comment threadpackages/elements/src/react/sign-in/action/action.tsx Outdated
Comment threadpackages/elements/src/react/sign-in/passkey.tsx Outdated

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Let's move the logic from the component into useInput() to keep the component layer thin.

Any ideas for not referencing the sign in flow actor directly in the common input components?

Comment on lines +612 to +616
React.useEffect(() => {
if (passkeyAutofillSupported) {
signInRouterRef?.send({ type: 'AUTHENTICATE.PASSKEY.AUTOFILL' });
}
}, [passkeyAutofillSupported, signInRouterRef]);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be in the useInput() hook? Ideally the components here are just thin wrappers around the hook, which contains the business logic

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably yes, but if felt like something only this component should care about.

because we are using refs here and not the context, it will not error, but i thought it would ok to not litter useInput with sign in specific logic.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out with the current setup this is a bit hard. useSignInPasskeyAutofill will use the useSelector underneath which would error when the SignInRouter context is not available.

(props: FormInputProps, forwardedRef) => {
const clerk = useClerk();
const passkeyAutofillProp = (props as PasskeyInputProps).passkeyAutofill;
const signInRouterRef = SignInRouterCtx.useActorRef(true);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not crazy about referencing the sign in actor directly in these components, but not sure if there's another way. 🤔

Maybe @tmilewski has an idea?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this better ?

<SignIn.Passkey><Clerk.Input/></SignIn.Passkey>

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see issues with that as well, because SignIn.Passkey wouldn't know whether it needs to pass the autofill prop or not. To avoid leaking the prop to the dom for everything else, but only pass it to Clerk.Input.

Another thought is to have a SignIn.PasskeyInput, but this does not feel right.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are now simply detecting autoComplete="webauthn"

const field = useInput(props);

const hasPasskeyAutofillProp = Boolean(field.props.autoComplete?.includes('webauthn'));
const allowedTypeForPasskey = (['text', 'email'] as FormInputProps['type'][]).includes(field.props.type);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider tel here for phone numbers?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are correct

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@panteliselef
panteliselef enabled auto-merge (squash) June 11, 2024 08:02
@panteliselef
panteliselef merged commit 4ec3f63 into mainJun 11, 2024
@panteliselef
panteliselef deleted the elef/passkey-elements branch June 11, 2024 08:16
This was referenced Jun 11, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@panteliselef@tmilewski@brkalow@LekoArts@clerk-cookie
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length \u003e 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat(elements): Add support for sign in with passkey - #3472

Merged
panteliselef merged 24 commits into
mainfrom
elef/passkey-elements
Jun 11, 2024
Merged

feat(elements): Add support for sign in with passkey#3472
panteliselef merged 24 commits into
mainfrom
elef/passkey-elements

Conversation

@panteliselef

@panteliselefpanteliselef commented May 30, 2024

Copy link
Copy Markdown
Contributor

Description

This PR add support for passkey usage within a SignIn flow

APIs introduced:

  • <SignIn.Passkey />
  • <SignIn.SupportedStrategy name='passkey'>
  • <SignIn.Strategy name='passkey'>
  • <Clerk.Input type='text' passkeyAutofill>

Usage Examples:

  • <SignIn.Passkey />
<SignIn.Stepname='start'><SignIn.Passkey><Clerk.Loading>{isLoading=>(isLoading ? <Spinner/> : 'Use passkey instead')}. </Clerk.Loading></SignIn.Passkey></SignIn.Step>
  • <SignIn.SupportedStrategy name='passkey'>
<SignIn.SupportedStrategyasChildname='passkey'><Button>use passkey</Button></SignIn.SupportedStrategy>
  • <SignIn.Strategy name='passkey'>
<SignIn.Strategyname='passkey'><pclassName='text-sm'>
Welcome back <SignIn.Salutation/>!
</p><CustomSubmit>Continue with Passkey</CustomSubmit></SignIn.Strategy>
  • <Clerk.Input type='text' passkeyAutofill>
<SignIn.Stepname='start'><Clerk.Fieldname='identifier'><Clerk.LabelclassName='sr-only'>Email</Clerk.Label><Clerk.InputpasskeyAutofillplaceholder='Enter your email address'/><Clerk.FieldError/></Clerk.Field>
</SignIn.Step/>

Docs PR: clerk/clerk-docs#1132

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

@changeset-bot

changeset-botBot commented May 30, 2024

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: d16809f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 14 packages
NameType
@clerk/elementsMinor
@clerk/clerk-jsMinor
@clerk/sharedMinor
@clerk/chrome-extensionPatch
@clerk/clerk-expoPatch
@clerk/backendPatch
@clerk/expressPatch
@clerk/fastifyPatch
@clerk/nextjsPatch
@clerk/clerk-reactPatch
@clerk/remixPatch
@clerk/clerk-sdk-nodePatch
@clerk/testingPatch
@clerk/uiPatch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Comment threadpackages/elements/src/internals/machines/sign-in/utils/starting-factors.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/verification.machine.ts Outdated
Comment on lines +372 to +381
case 'passkey': {
return await parent.getSnapshot().context.clerk.client.signIn.authenticateWithPasskey();
}

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When "Submit" is hit we don't want to call attempt, instead call authenticateWithPasskey

Comment on lines +276 to +308
const [isSupported, setIsSupported] = React.useState(false);
React.useEffect(() => {
async function runAutofillPasskey() {
const _isSupported = await isWebAuthnAutofillSupported().catch(() => false);
setIsSupported(_isSupported);
}

// @ts-expect-error - Depending on type the props can be different
if (passthroughProps?.passkeyAutofill) {
runAutofillPasskey();
}

// @ts-expect-error - Depending on type the props can be different
}, [passthroughProps?.passkeyAutofill]);

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be handled inside an actor ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which part are you referring to?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, passkey specific logic shouldn't live in the common input element like this. Let's try to move it to an actor.

Why does passkeyAutofill need to be on the input?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only necessary if we want to support passkey autofill.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we rely on "the platform" here? It doesn't feel necessary to create a separate abstraction if all it takes is specifying a native property on the element, unless we need to execute some specific logic to enable autofill.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to call authenticateWithPasskey internally

Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
@panteliselefpanteliselef changed the title Elef/passkey elementsfeat(elements): Add support for sign in with passkeyMay 30, 2024
Comment on lines +138 to +149
on: {
'AUTHENTICATE.PASSKEY': {
guard: not('isExampleMode'),
target: 'AttemptingPasskey',
reenter: true,
},
SUBMIT: {
guard: not('isExampleMode'),
target: 'Attempting',
reenter: true,
},
},

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enabling "autofill" is kind of fire and forget action, so we should be able to recover from this state and not get stuck. Promise will await until user interact with the field and select a passkey and that may never happen.

Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
@panteliselefpanteliselef self-assigned this May 31, 2024
@panteliselef
panteliselef marked this pull request as ready for review May 31, 2024 09:48
Comment threadpackages/elements/src/internals/machines/sign-in/start.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.types.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/start.types.ts Outdated
Comment thread.changeset/fuzzy-bees-doubt.md Outdated
Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
Comment threadpackages/elements/src/react/sign-in/action/passkey.tsx

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. I'd like to see the passkey specific logic moved out of the input logic.

Comment on lines +276 to +308
const [isSupported, setIsSupported] = React.useState(false);
React.useEffect(() => {
async function runAutofillPasskey() {
const _isSupported = await isWebAuthnAutofillSupported().catch(() => false);
setIsSupported(_isSupported);
}

// @ts-expect-error - Depending on type the props can be different
if (passthroughProps?.passkeyAutofill) {
runAutofillPasskey();
}

// @ts-expect-error - Depending on type the props can be different
}, [passthroughProps?.passkeyAutofill]);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, passkey specific logic shouldn't live in the common input element like this. Let's try to move it to an actor.

Why does passkeyAutofill need to be on the input?

Comment threadpackages/elements/src/internals/machines/sign-in/verification.machine.ts Outdated
Comment thread.changeset/fuzzy-bees-doubt.md Outdated
Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
Comment threadpackages/elements/src/react/sign-in/action/action.tsx Outdated
Comment threadpackages/elements/src/react/sign-in/passkey.tsx Outdated

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Let's move the logic from the component into useInput() to keep the component layer thin.

Any ideas for not referencing the sign in flow actor directly in the common input components?

Comment on lines +612 to +616
React.useEffect(() => {
if (passkeyAutofillSupported) {
signInRouterRef?.send({ type: 'AUTHENTICATE.PASSKEY.AUTOFILL' });
}
}, [passkeyAutofillSupported, signInRouterRef]);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be in the useInput() hook? Ideally the components here are just thin wrappers around the hook, which contains the business logic

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably yes, but if felt like something only this component should care about.

because we are using refs here and not the context, it will not error, but i thought it would ok to not litter useInput with sign in specific logic.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out with the current setup this is a bit hard. useSignInPasskeyAutofill will use the useSelector underneath which would error when the SignInRouter context is not available.

(props: FormInputProps, forwardedRef) => {
const clerk = useClerk();
const passkeyAutofillProp = (props as PasskeyInputProps).passkeyAutofill;
const signInRouterRef = SignInRouterCtx.useActorRef(true);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not crazy about referencing the sign in actor directly in these components, but not sure if there's another way. 🤔

Maybe @tmilewski has an idea?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this better ?

<SignIn.Passkey><Clerk.Input/></SignIn.Passkey>

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see issues with that as well, because SignIn.Passkey wouldn't know whether it needs to pass the autofill prop or not. To avoid leaking the prop to the dom for everything else, but only pass it to Clerk.Input.

Another thought is to have a SignIn.PasskeyInput, but this does not feel right.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are now simply detecting autoComplete="webauthn"

const field = useInput(props);

const hasPasskeyAutofillProp = Boolean(field.props.autoComplete?.includes('webauthn'));
const allowedTypeForPasskey = (['text', 'email'] as FormInputProps['type'][]).includes(field.props.type);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider tel here for phone numbers?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are correct

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@panteliselef
panteliselef enabled auto-merge (squash) June 11, 2024 08:02
@panteliselef
panteliselef merged commit 4ec3f63 into mainJun 11, 2024
@panteliselef
panteliselef deleted the elef/passkey-elements branch June 11, 2024 08:16
This was referenced Jun 11, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@panteliselef@tmilewski@brkalow@LekoArts@clerk-cookie
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

feat(elements): Add support for sign in with passkey - #3472

Merged
panteliselef merged 24 commits into
mainfrom
elef/passkey-elements
Jun 11, 2024
Merged

feat(elements): Add support for sign in with passkey#3472
panteliselef merged 24 commits into
mainfrom
elef/passkey-elements

Conversation

@panteliselef

@panteliselefpanteliselef commented May 30, 2024

Copy link
Copy Markdown
Contributor

Description

This PR add support for passkey usage within a SignIn flow

APIs introduced:

  • <SignIn.Passkey />
  • <SignIn.SupportedStrategy name='passkey'>
  • <SignIn.Strategy name='passkey'>
  • <Clerk.Input type='text' passkeyAutofill>

Usage Examples:

  • <SignIn.Passkey />
<SignIn.Stepname='start'><SignIn.Passkey><Clerk.Loading>{isLoading=>(isLoading ? <Spinner/> : 'Use passkey instead')}. </Clerk.Loading></SignIn.Passkey></SignIn.Step>
  • <SignIn.SupportedStrategy name='passkey'>
<SignIn.SupportedStrategyasChildname='passkey'><Button>use passkey</Button></SignIn.SupportedStrategy>
  • <SignIn.Strategy name='passkey'>
<SignIn.Strategyname='passkey'><pclassName='text-sm'>
Welcome back <SignIn.Salutation/>!
</p><CustomSubmit>Continue with Passkey</CustomSubmit></SignIn.Strategy>
  • <Clerk.Input type='text' passkeyAutofill>
<SignIn.Stepname='start'><Clerk.Fieldname='identifier'><Clerk.LabelclassName='sr-only'>Email</Clerk.Label><Clerk.InputpasskeyAutofillplaceholder='Enter your email address'/><Clerk.FieldError/></Clerk.Field>
</SignIn.Step/>

Docs PR: clerk/clerk-docs#1132

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

@changeset-bot

changeset-botBot commented May 30, 2024

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: d16809f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 14 packages
NameType
@clerk/elementsMinor
@clerk/clerk-jsMinor
@clerk/sharedMinor
@clerk/chrome-extensionPatch
@clerk/clerk-expoPatch
@clerk/backendPatch
@clerk/expressPatch
@clerk/fastifyPatch
@clerk/nextjsPatch
@clerk/clerk-reactPatch
@clerk/remixPatch
@clerk/clerk-sdk-nodePatch
@clerk/testingPatch
@clerk/uiPatch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Comment threadpackages/elements/src/internals/machines/sign-in/utils/starting-factors.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/verification.machine.ts Outdated
Comment on lines +372 to +381
case 'passkey': {
return await parent.getSnapshot().context.clerk.client.signIn.authenticateWithPasskey();
}

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When "Submit" is hit we don't want to call attempt, instead call authenticateWithPasskey

Comment on lines +276 to +308
const [isSupported, setIsSupported] = React.useState(false);
React.useEffect(() => {
async function runAutofillPasskey() {
const _isSupported = await isWebAuthnAutofillSupported().catch(() => false);
setIsSupported(_isSupported);
}

// @ts-expect-error - Depending on type the props can be different
if (passthroughProps?.passkeyAutofill) {
runAutofillPasskey();
}

// @ts-expect-error - Depending on type the props can be different
}, [passthroughProps?.passkeyAutofill]);

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be handled inside an actor ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which part are you referring to?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, passkey specific logic shouldn't live in the common input element like this. Let's try to move it to an actor.

Why does passkeyAutofill need to be on the input?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only necessary if we want to support passkey autofill.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we rely on "the platform" here? It doesn't feel necessary to create a separate abstraction if all it takes is specifying a native property on the element, unless we need to execute some specific logic to enable autofill.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to call authenticateWithPasskey internally

Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
@panteliselefpanteliselef changed the title Elef/passkey elementsfeat(elements): Add support for sign in with passkeyMay 30, 2024
Comment on lines +138 to +149
on: {
'AUTHENTICATE.PASSKEY': {
guard: not('isExampleMode'),
target: 'AttemptingPasskey',
reenter: true,
},
SUBMIT: {
guard: not('isExampleMode'),
target: 'Attempting',
reenter: true,
},
},

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enabling "autofill" is kind of fire and forget action, so we should be able to recover from this state and not get stuck. Promise will await until user interact with the field and select a passkey and that may never happen.

Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
@panteliselefpanteliselef self-assigned this May 31, 2024
@panteliselef
panteliselef marked this pull request as ready for review May 31, 2024 09:48
Comment threadpackages/elements/src/internals/machines/sign-in/start.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.types.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/start.types.ts Outdated
Comment thread.changeset/fuzzy-bees-doubt.md Outdated
Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
Comment threadpackages/elements/src/react/sign-in/action/passkey.tsx

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. I'd like to see the passkey specific logic moved out of the input logic.

Comment on lines +276 to +308
const [isSupported, setIsSupported] = React.useState(false);
React.useEffect(() => {
async function runAutofillPasskey() {
const _isSupported = await isWebAuthnAutofillSupported().catch(() => false);
setIsSupported(_isSupported);
}

// @ts-expect-error - Depending on type the props can be different
if (passthroughProps?.passkeyAutofill) {
runAutofillPasskey();
}

// @ts-expect-error - Depending on type the props can be different
}, [passthroughProps?.passkeyAutofill]);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, passkey specific logic shouldn't live in the common input element like this. Let's try to move it to an actor.

Why does passkeyAutofill need to be on the input?

Comment threadpackages/elements/src/internals/machines/sign-in/verification.machine.ts Outdated
Comment thread.changeset/fuzzy-bees-doubt.md Outdated
Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
Comment threadpackages/elements/src/react/sign-in/action/action.tsx Outdated
Comment threadpackages/elements/src/react/sign-in/passkey.tsx Outdated

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Let's move the logic from the component into useInput() to keep the component layer thin.

Any ideas for not referencing the sign in flow actor directly in the common input components?

Comment on lines +612 to +616
React.useEffect(() => {
if (passkeyAutofillSupported) {
signInRouterRef?.send({ type: 'AUTHENTICATE.PASSKEY.AUTOFILL' });
}
}, [passkeyAutofillSupported, signInRouterRef]);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be in the useInput() hook? Ideally the components here are just thin wrappers around the hook, which contains the business logic

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably yes, but if felt like something only this component should care about.

because we are using refs here and not the context, it will not error, but i thought it would ok to not litter useInput with sign in specific logic.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out with the current setup this is a bit hard. useSignInPasskeyAutofill will use the useSelector underneath which would error when the SignInRouter context is not available.

(props: FormInputProps, forwardedRef) => {
const clerk = useClerk();
const passkeyAutofillProp = (props as PasskeyInputProps).passkeyAutofill;
const signInRouterRef = SignInRouterCtx.useActorRef(true);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not crazy about referencing the sign in actor directly in these components, but not sure if there's another way. 🤔

Maybe @tmilewski has an idea?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this better ?

<SignIn.Passkey><Clerk.Input/></SignIn.Passkey>

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see issues with that as well, because SignIn.Passkey wouldn't know whether it needs to pass the autofill prop or not. To avoid leaking the prop to the dom for everything else, but only pass it to Clerk.Input.

Another thought is to have a SignIn.PasskeyInput, but this does not feel right.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are now simply detecting autoComplete="webauthn"

const field = useInput(props);

const hasPasskeyAutofillProp = Boolean(field.props.autoComplete?.includes('webauthn'));
const allowedTypeForPasskey = (['text', 'email'] as FormInputProps['type'][]).includes(field.props.type);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider tel here for phone numbers?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are correct

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@panteliselef
panteliselef enabled auto-merge (squash) June 11, 2024 08:02
@panteliselef
panteliselef merged commit 4ec3f63 into mainJun 11, 2024
@panteliselef
panteliselef deleted the elef/passkey-elements branch June 11, 2024 08:16
This was referenced Jun 11, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@panteliselef@tmilewski@brkalow@LekoArts@clerk-cookie
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat(elements): Add support for sign in with passkey - #3472

Merged
panteliselef merged 24 commits into
mainfrom
elef/passkey-elements
Jun 11, 2024
Merged

feat(elements): Add support for sign in with passkey#3472
panteliselef merged 24 commits into
mainfrom
elef/passkey-elements

Conversation

@panteliselef

@panteliselefpanteliselef commented May 30, 2024

Copy link
Copy Markdown
Contributor

Description

This PR add support for passkey usage within a SignIn flow

APIs introduced:

  • <SignIn.Passkey />
  • <SignIn.SupportedStrategy name='passkey'>
  • <SignIn.Strategy name='passkey'>
  • <Clerk.Input type='text' passkeyAutofill>

Usage Examples:

  • <SignIn.Passkey />
<SignIn.Stepname='start'><SignIn.Passkey><Clerk.Loading>{isLoading=>(isLoading ? <Spinner/> : 'Use passkey instead')}. </Clerk.Loading></SignIn.Passkey></SignIn.Step>
  • <SignIn.SupportedStrategy name='passkey'>
<SignIn.SupportedStrategyasChildname='passkey'><Button>use passkey</Button></SignIn.SupportedStrategy>
  • <SignIn.Strategy name='passkey'>
<SignIn.Strategyname='passkey'><pclassName='text-sm'>
Welcome back <SignIn.Salutation/>!
</p><CustomSubmit>Continue with Passkey</CustomSubmit></SignIn.Strategy>
  • <Clerk.Input type='text' passkeyAutofill>
<SignIn.Stepname='start'><Clerk.Fieldname='identifier'><Clerk.LabelclassName='sr-only'>Email</Clerk.Label><Clerk.InputpasskeyAutofillplaceholder='Enter your email address'/><Clerk.FieldError/></Clerk.Field>
</SignIn.Step/>

Docs PR: clerk/clerk-docs#1132

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

@changeset-bot

changeset-botBot commented May 30, 2024

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: d16809f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 14 packages
NameType
@clerk/elementsMinor
@clerk/clerk-jsMinor
@clerk/sharedMinor
@clerk/chrome-extensionPatch
@clerk/clerk-expoPatch
@clerk/backendPatch
@clerk/expressPatch
@clerk/fastifyPatch
@clerk/nextjsPatch
@clerk/clerk-reactPatch
@clerk/remixPatch
@clerk/clerk-sdk-nodePatch
@clerk/testingPatch
@clerk/uiPatch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Comment threadpackages/elements/src/internals/machines/sign-in/utils/starting-factors.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/verification.machine.ts Outdated
Comment on lines +372 to +381
case 'passkey': {
return await parent.getSnapshot().context.clerk.client.signIn.authenticateWithPasskey();
}

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When "Submit" is hit we don't want to call attempt, instead call authenticateWithPasskey

Comment on lines +276 to +308
const [isSupported, setIsSupported] = React.useState(false);
React.useEffect(() => {
async function runAutofillPasskey() {
const _isSupported = await isWebAuthnAutofillSupported().catch(() => false);
setIsSupported(_isSupported);
}

// @ts-expect-error - Depending on type the props can be different
if (passthroughProps?.passkeyAutofill) {
runAutofillPasskey();
}

// @ts-expect-error - Depending on type the props can be different
}, [passthroughProps?.passkeyAutofill]);

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be handled inside an actor ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which part are you referring to?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, passkey specific logic shouldn't live in the common input element like this. Let's try to move it to an actor.

Why does passkeyAutofill need to be on the input?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only necessary if we want to support passkey autofill.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we rely on "the platform" here? It doesn't feel necessary to create a separate abstraction if all it takes is specifying a native property on the element, unless we need to execute some specific logic to enable autofill.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to call authenticateWithPasskey internally

Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
@panteliselefpanteliselef changed the title Elef/passkey elementsfeat(elements): Add support for sign in with passkeyMay 30, 2024
Comment on lines +138 to +149
on: {
'AUTHENTICATE.PASSKEY': {
guard: not('isExampleMode'),
target: 'AttemptingPasskey',
reenter: true,
},
SUBMIT: {
guard: not('isExampleMode'),
target: 'Attempting',
reenter: true,
},
},

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enabling "autofill" is kind of fire and forget action, so we should be able to recover from this state and not get stuck. Promise will await until user interact with the field and select a passkey and that may never happen.

Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
@panteliselefpanteliselef self-assigned this May 31, 2024
@panteliselef
panteliselef marked this pull request as ready for review May 31, 2024 09:48
Comment threadpackages/elements/src/internals/machines/sign-in/start.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.types.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/start.types.ts Outdated
Comment thread.changeset/fuzzy-bees-doubt.md Outdated
Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
Comment threadpackages/elements/src/react/sign-in/action/passkey.tsx

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. I'd like to see the passkey specific logic moved out of the input logic.

Comment on lines +276 to +308
const [isSupported, setIsSupported] = React.useState(false);
React.useEffect(() => {
async function runAutofillPasskey() {
const _isSupported = await isWebAuthnAutofillSupported().catch(() => false);
setIsSupported(_isSupported);
}

// @ts-expect-error - Depending on type the props can be different
if (passthroughProps?.passkeyAutofill) {
runAutofillPasskey();
}

// @ts-expect-error - Depending on type the props can be different
}, [passthroughProps?.passkeyAutofill]);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, passkey specific logic shouldn't live in the common input element like this. Let's try to move it to an actor.

Why does passkeyAutofill need to be on the input?

Comment threadpackages/elements/src/internals/machines/sign-in/verification.machine.ts Outdated
Comment thread.changeset/fuzzy-bees-doubt.md Outdated
Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
Comment threadpackages/elements/src/react/sign-in/action/action.tsx Outdated
Comment threadpackages/elements/src/react/sign-in/passkey.tsx Outdated

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Let's move the logic from the component into useInput() to keep the component layer thin.

Any ideas for not referencing the sign in flow actor directly in the common input components?

Comment on lines +612 to +616
React.useEffect(() => {
if (passkeyAutofillSupported) {
signInRouterRef?.send({ type: 'AUTHENTICATE.PASSKEY.AUTOFILL' });
}
}, [passkeyAutofillSupported, signInRouterRef]);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be in the useInput() hook? Ideally the components here are just thin wrappers around the hook, which contains the business logic

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably yes, but if felt like something only this component should care about.

because we are using refs here and not the context, it will not error, but i thought it would ok to not litter useInput with sign in specific logic.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out with the current setup this is a bit hard. useSignInPasskeyAutofill will use the useSelector underneath which would error when the SignInRouter context is not available.

(props: FormInputProps, forwardedRef) => {
const clerk = useClerk();
const passkeyAutofillProp = (props as PasskeyInputProps).passkeyAutofill;
const signInRouterRef = SignInRouterCtx.useActorRef(true);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not crazy about referencing the sign in actor directly in these components, but not sure if there's another way. 🤔

Maybe @tmilewski has an idea?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this better ?

<SignIn.Passkey><Clerk.Input/></SignIn.Passkey>

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see issues with that as well, because SignIn.Passkey wouldn't know whether it needs to pass the autofill prop or not. To avoid leaking the prop to the dom for everything else, but only pass it to Clerk.Input.

Another thought is to have a SignIn.PasskeyInput, but this does not feel right.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are now simply detecting autoComplete="webauthn"

const field = useInput(props);

const hasPasskeyAutofillProp = Boolean(field.props.autoComplete?.includes('webauthn'));
const allowedTypeForPasskey = (['text', 'email'] as FormInputProps['type'][]).includes(field.props.type);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider tel here for phone numbers?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are correct

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@panteliselef
panteliselef enabled auto-merge (squash) June 11, 2024 08:02
@panteliselef
panteliselef merged commit 4ec3f63 into mainJun 11, 2024
@panteliselef
panteliselef deleted the elef/passkey-elements branch June 11, 2024 08:16
This was referenced Jun 11, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@panteliselef@tmilewski@brkalow@LekoArts@clerk-cookie
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat(elements): Add support for sign in with passkey - #3472

Merged
panteliselef merged 24 commits into
mainfrom
elef/passkey-elements
Jun 11, 2024
Merged

feat(elements): Add support for sign in with passkey#3472
panteliselef merged 24 commits into
mainfrom
elef/passkey-elements

Conversation

@panteliselef

@panteliselefpanteliselef commented May 30, 2024

Copy link
Copy Markdown
Contributor

Description

This PR add support for passkey usage within a SignIn flow

APIs introduced:

  • <SignIn.Passkey />
  • <SignIn.SupportedStrategy name='passkey'>
  • <SignIn.Strategy name='passkey'>
  • <Clerk.Input type='text' passkeyAutofill>

Usage Examples:

  • <SignIn.Passkey />
<SignIn.Stepname='start'><SignIn.Passkey><Clerk.Loading>{isLoading=>(isLoading ? <Spinner/> : 'Use passkey instead')}. </Clerk.Loading></SignIn.Passkey></SignIn.Step>
  • <SignIn.SupportedStrategy name='passkey'>
<SignIn.SupportedStrategyasChildname='passkey'><Button>use passkey</Button></SignIn.SupportedStrategy>
  • <SignIn.Strategy name='passkey'>
<SignIn.Strategyname='passkey'><pclassName='text-sm'>
Welcome back <SignIn.Salutation/>!
</p><CustomSubmit>Continue with Passkey</CustomSubmit></SignIn.Strategy>
  • <Clerk.Input type='text' passkeyAutofill>
<SignIn.Stepname='start'><Clerk.Fieldname='identifier'><Clerk.LabelclassName='sr-only'>Email</Clerk.Label><Clerk.InputpasskeyAutofillplaceholder='Enter your email address'/><Clerk.FieldError/></Clerk.Field>
</SignIn.Step/>

Docs PR: clerk/clerk-docs#1132

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

@changeset-bot

changeset-botBot commented May 30, 2024

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: d16809f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 14 packages
NameType
@clerk/elementsMinor
@clerk/clerk-jsMinor
@clerk/sharedMinor
@clerk/chrome-extensionPatch
@clerk/clerk-expoPatch
@clerk/backendPatch
@clerk/expressPatch
@clerk/fastifyPatch
@clerk/nextjsPatch
@clerk/clerk-reactPatch
@clerk/remixPatch
@clerk/clerk-sdk-nodePatch
@clerk/testingPatch
@clerk/uiPatch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Comment threadpackages/elements/src/internals/machines/sign-in/utils/starting-factors.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/verification.machine.ts Outdated
Comment on lines +372 to +381
case 'passkey': {
return await parent.getSnapshot().context.clerk.client.signIn.authenticateWithPasskey();
}

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When "Submit" is hit we don't want to call attempt, instead call authenticateWithPasskey

Comment on lines +276 to +308
const [isSupported, setIsSupported] = React.useState(false);
React.useEffect(() => {
async function runAutofillPasskey() {
const _isSupported = await isWebAuthnAutofillSupported().catch(() => false);
setIsSupported(_isSupported);
}

// @ts-expect-error - Depending on type the props can be different
if (passthroughProps?.passkeyAutofill) {
runAutofillPasskey();
}

// @ts-expect-error - Depending on type the props can be different
}, [passthroughProps?.passkeyAutofill]);

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be handled inside an actor ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which part are you referring to?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, passkey specific logic shouldn't live in the common input element like this. Let's try to move it to an actor.

Why does passkeyAutofill need to be on the input?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only necessary if we want to support passkey autofill.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we rely on "the platform" here? It doesn't feel necessary to create a separate abstraction if all it takes is specifying a native property on the element, unless we need to execute some specific logic to enable autofill.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to call authenticateWithPasskey internally

Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
@panteliselefpanteliselef changed the title Elef/passkey elementsfeat(elements): Add support for sign in with passkeyMay 30, 2024
Comment on lines +138 to +149
on: {
'AUTHENTICATE.PASSKEY': {
guard: not('isExampleMode'),
target: 'AttemptingPasskey',
reenter: true,
},
SUBMIT: {
guard: not('isExampleMode'),
target: 'Attempting',
reenter: true,
},
},

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enabling "autofill" is kind of fire and forget action, so we should be able to recover from this state and not get stuck. Promise will await until user interact with the field and select a passkey and that may never happen.

Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
@panteliselefpanteliselef self-assigned this May 31, 2024
@panteliselef
panteliselef marked this pull request as ready for review May 31, 2024 09:48
Comment threadpackages/elements/src/internals/machines/sign-in/start.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.types.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/start.types.ts Outdated
Comment thread.changeset/fuzzy-bees-doubt.md Outdated
Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
Comment threadpackages/elements/src/react/sign-in/action/passkey.tsx

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. I'd like to see the passkey specific logic moved out of the input logic.

Comment on lines +276 to +308
const [isSupported, setIsSupported] = React.useState(false);
React.useEffect(() => {
async function runAutofillPasskey() {
const _isSupported = await isWebAuthnAutofillSupported().catch(() => false);
setIsSupported(_isSupported);
}

// @ts-expect-error - Depending on type the props can be different
if (passthroughProps?.passkeyAutofill) {
runAutofillPasskey();
}

// @ts-expect-error - Depending on type the props can be different
}, [passthroughProps?.passkeyAutofill]);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, passkey specific logic shouldn't live in the common input element like this. Let's try to move it to an actor.

Why does passkeyAutofill need to be on the input?

Comment threadpackages/elements/src/internals/machines/sign-in/verification.machine.ts Outdated
Comment thread.changeset/fuzzy-bees-doubt.md Outdated
Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
Comment threadpackages/elements/src/react/sign-in/action/action.tsx Outdated
Comment threadpackages/elements/src/react/sign-in/passkey.tsx Outdated

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Let's move the logic from the component into useInput() to keep the component layer thin.

Any ideas for not referencing the sign in flow actor directly in the common input components?

Comment on lines +612 to +616
React.useEffect(() => {
if (passkeyAutofillSupported) {
signInRouterRef?.send({ type: 'AUTHENTICATE.PASSKEY.AUTOFILL' });
}
}, [passkeyAutofillSupported, signInRouterRef]);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be in the useInput() hook? Ideally the components here are just thin wrappers around the hook, which contains the business logic

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably yes, but if felt like something only this component should care about.

because we are using refs here and not the context, it will not error, but i thought it would ok to not litter useInput with sign in specific logic.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out with the current setup this is a bit hard. useSignInPasskeyAutofill will use the useSelector underneath which would error when the SignInRouter context is not available.

(props: FormInputProps, forwardedRef) => {
const clerk = useClerk();
const passkeyAutofillProp = (props as PasskeyInputProps).passkeyAutofill;
const signInRouterRef = SignInRouterCtx.useActorRef(true);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not crazy about referencing the sign in actor directly in these components, but not sure if there's another way. 🤔

Maybe @tmilewski has an idea?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this better ?

<SignIn.Passkey><Clerk.Input/></SignIn.Passkey>

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see issues with that as well, because SignIn.Passkey wouldn't know whether it needs to pass the autofill prop or not. To avoid leaking the prop to the dom for everything else, but only pass it to Clerk.Input.

Another thought is to have a SignIn.PasskeyInput, but this does not feel right.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are now simply detecting autoComplete="webauthn"

const field = useInput(props);

const hasPasskeyAutofillProp = Boolean(field.props.autoComplete?.includes('webauthn'));
const allowedTypeForPasskey = (['text', 'email'] as FormInputProps['type'][]).includes(field.props.type);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider tel here for phone numbers?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are correct

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@panteliselef
panteliselef enabled auto-merge (squash) June 11, 2024 08:02
@panteliselef
panteliselef merged commit 4ec3f63 into mainJun 11, 2024
@panteliselef
panteliselef deleted the elef/passkey-elements branch June 11, 2024 08:16
This was referenced Jun 11, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@panteliselef@tmilewski@brkalow@LekoArts@clerk-cookie
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

feat(elements): Add support for sign in with passkey - #3472

Merged
panteliselef merged 24 commits into
mainfrom
elef/passkey-elements
Jun 11, 2024
Merged

feat(elements): Add support for sign in with passkey#3472
panteliselef merged 24 commits into
mainfrom
elef/passkey-elements

Conversation

@panteliselef

@panteliselefpanteliselef commented May 30, 2024

Copy link
Copy Markdown
Contributor

Description

This PR add support for passkey usage within a SignIn flow

APIs introduced:

  • <SignIn.Passkey />
  • <SignIn.SupportedStrategy name='passkey'>
  • <SignIn.Strategy name='passkey'>
  • <Clerk.Input type='text' passkeyAutofill>

Usage Examples:

  • <SignIn.Passkey />
<SignIn.Stepname='start'><SignIn.Passkey><Clerk.Loading>{isLoading=>(isLoading ? <Spinner/> : 'Use passkey instead')}. </Clerk.Loading></SignIn.Passkey></SignIn.Step>
  • <SignIn.SupportedStrategy name='passkey'>
<SignIn.SupportedStrategyasChildname='passkey'><Button>use passkey</Button></SignIn.SupportedStrategy>
  • <SignIn.Strategy name='passkey'>
<SignIn.Strategyname='passkey'><pclassName='text-sm'>
Welcome back <SignIn.Salutation/>!
</p><CustomSubmit>Continue with Passkey</CustomSubmit></SignIn.Strategy>
  • <Clerk.Input type='text' passkeyAutofill>
<SignIn.Stepname='start'><Clerk.Fieldname='identifier'><Clerk.LabelclassName='sr-only'>Email</Clerk.Label><Clerk.InputpasskeyAutofillplaceholder='Enter your email address'/><Clerk.FieldError/></Clerk.Field>
</SignIn.Step/>

Docs PR: clerk/clerk-docs#1132

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

@changeset-bot

changeset-botBot commented May 30, 2024

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: d16809f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 14 packages
NameType
@clerk/elementsMinor
@clerk/clerk-jsMinor
@clerk/sharedMinor
@clerk/chrome-extensionPatch
@clerk/clerk-expoPatch
@clerk/backendPatch
@clerk/expressPatch
@clerk/fastifyPatch
@clerk/nextjsPatch
@clerk/clerk-reactPatch
@clerk/remixPatch
@clerk/clerk-sdk-nodePatch
@clerk/testingPatch
@clerk/uiPatch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Comment threadpackages/elements/src/internals/machines/sign-in/utils/starting-factors.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/verification.machine.ts Outdated
Comment on lines +372 to +381
case 'passkey': {
return await parent.getSnapshot().context.clerk.client.signIn.authenticateWithPasskey();
}

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When "Submit" is hit we don't want to call attempt, instead call authenticateWithPasskey

Comment on lines +276 to +308
const [isSupported, setIsSupported] = React.useState(false);
React.useEffect(() => {
async function runAutofillPasskey() {
const _isSupported = await isWebAuthnAutofillSupported().catch(() => false);
setIsSupported(_isSupported);
}

// @ts-expect-error - Depending on type the props can be different
if (passthroughProps?.passkeyAutofill) {
runAutofillPasskey();
}

// @ts-expect-error - Depending on type the props can be different
}, [passthroughProps?.passkeyAutofill]);

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be handled inside an actor ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which part are you referring to?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, passkey specific logic shouldn't live in the common input element like this. Let's try to move it to an actor.

Why does passkeyAutofill need to be on the input?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only necessary if we want to support passkey autofill.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we rely on "the platform" here? It doesn't feel necessary to create a separate abstraction if all it takes is specifying a native property on the element, unless we need to execute some specific logic to enable autofill.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to call authenticateWithPasskey internally

Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
@panteliselefpanteliselef changed the title Elef/passkey elementsfeat(elements): Add support for sign in with passkeyMay 30, 2024
Comment on lines +138 to +149
on: {
'AUTHENTICATE.PASSKEY': {
guard: not('isExampleMode'),
target: 'AttemptingPasskey',
reenter: true,
},
SUBMIT: {
guard: not('isExampleMode'),
target: 'Attempting',
reenter: true,
},
},

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enabling "autofill" is kind of fire and forget action, so we should be able to recover from this state and not get stuck. Promise will await until user interact with the field and select a passkey and that may never happen.

Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
@panteliselefpanteliselef self-assigned this May 31, 2024
@panteliselef
panteliselef marked this pull request as ready for review May 31, 2024 09:48
Comment threadpackages/elements/src/internals/machines/sign-in/start.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.types.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/router.machine.ts Outdated
Comment threadpackages/elements/src/internals/machines/sign-in/start.types.ts Outdated
Comment thread.changeset/fuzzy-bees-doubt.md Outdated
Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
Comment threadpackages/elements/src/react/sign-in/action/passkey.tsx

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. I'd like to see the passkey specific logic moved out of the input logic.

Comment on lines +276 to +308
const [isSupported, setIsSupported] = React.useState(false);
React.useEffect(() => {
async function runAutofillPasskey() {
const _isSupported = await isWebAuthnAutofillSupported().catch(() => false);
setIsSupported(_isSupported);
}

// @ts-expect-error - Depending on type the props can be different
if (passthroughProps?.passkeyAutofill) {
runAutofillPasskey();
}

// @ts-expect-error - Depending on type the props can be different
}, [passthroughProps?.passkeyAutofill]);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, passkey specific logic shouldn't live in the common input element like this. Let's try to move it to an actor.

Why does passkeyAutofill need to be on the input?

Comment threadpackages/elements/src/internals/machines/sign-in/verification.machine.ts Outdated
Comment thread.changeset/fuzzy-bees-doubt.md Outdated
Comment threadpackages/elements/src/react/common/form/index.tsx Outdated
Comment threadpackages/elements/src/react/sign-in/action/action.tsx Outdated
Comment threadpackages/elements/src/react/sign-in/passkey.tsx Outdated

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. Let's move the logic from the component into useInput() to keep the component layer thin.

Any ideas for not referencing the sign in flow actor directly in the common input components?

Comment on lines +612 to +616
React.useEffect(() => {
if (passkeyAutofillSupported) {
signInRouterRef?.send({ type: 'AUTHENTICATE.PASSKEY.AUTOFILL' });
}
}, [passkeyAutofillSupported, signInRouterRef]);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be in the useInput() hook? Ideally the components here are just thin wrappers around the hook, which contains the business logic

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably yes, but if felt like something only this component should care about.

because we are using refs here and not the context, it will not error, but i thought it would ok to not litter useInput with sign in specific logic.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out with the current setup this is a bit hard. useSignInPasskeyAutofill will use the useSelector underneath which would error when the SignInRouter context is not available.

(props: FormInputProps, forwardedRef) => {
const clerk = useClerk();
const passkeyAutofillProp = (props as PasskeyInputProps).passkeyAutofill;
const signInRouterRef = SignInRouterCtx.useActorRef(true);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not crazy about referencing the sign in actor directly in these components, but not sure if there's another way. 🤔

Maybe @tmilewski has an idea?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this better ?

<SignIn.Passkey><Clerk.Input/></SignIn.Passkey>

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see issues with that as well, because SignIn.Passkey wouldn't know whether it needs to pass the autofill prop or not. To avoid leaking the prop to the dom for everything else, but only pass it to Clerk.Input.

Another thought is to have a SignIn.PasskeyInput, but this does not feel right.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are now simply detecting autoComplete="webauthn"

const field = useInput(props);

const hasPasskeyAutofillProp = Boolean(field.props.autoComplete?.includes('webauthn'));
const allowedTypeForPasskey = (['text', 'email'] as FormInputProps['type'][]).includes(field.props.type);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider tel here for phone numbers?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are correct

@brkalowbrkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@panteliselef
panteliselef enabled auto-merge (squash) June 11, 2024 08:02
@panteliselef
panteliselef merged commit 4ec3f63 into mainJun 11, 2024
@panteliselef
panteliselef deleted the elef/passkey-elements branch June 11, 2024 08:16
This was referenced Jun 11, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@panteliselef@tmilewski@brkalow@LekoArts@clerk-cookie