Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Passkey support #13127

Closed
wants to merge 11 commits into from
Closed

feat: Passkey support #13127

wants to merge 11 commits into from

Conversation

merlindru
Copy link

@merlindru merlindru commented Jan 9, 2024

What does this PR do?

Fixes #13342
Fixes CAL-2984

This PR adds passkey support for logging in through Hanko Passkeys.

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update (probably)

How should this be tested?

  • Go to cloud.hanko.io and create a "Passkey API" project
    • Then create a new API key
      set it to HANKO_PASSKEYS_API_KEY="<api key>"
      (quotes important)
    • Copy the tenant ID to NEXT_PUBLIC_HANKO_PASSKEYS_TENANT_ID=<tenant id>
  • Log in as normal (with password/google/...)
  • Go to settings > passkeys > create a new passkey
  • Log out
  • Go to login > sign in with a passkey

Mandatory Tasks

  • Make sure you have self-reviewed the code. A decent size PR without self-review might be rejected.

next-auth-options.ts

Please review this file, there are substantial changes that might be unsafe. I am not sure if the file in it's current form is okay:

This PR changes the very important authorize function. I'm not sure if we want to keep it like this, but the authorize function itself does a lot of checks, only some relevant to credentials.

Instead of duplicating it, I added a second parameter, unsafe, intended to only be used from server code like in the PasskeyProvider. Using it is very explicit, you have to pass undefined as a first parameter, so it's clear that you shouldn't be using this functionality unless you're sure what you're doing:

// Example usage of authorize function
const sessionData = authorize(undefined, { unsafe: { userId: 123 }})

When unsafe is used, all the checks dependent on credentials are not being run anymore.

I am not sure how safe this is, or if a larger refactor is needed that properly separates checks dependent on credentials from all other checks. (That would certainly be cleaner, though)

Checklist

  • I haven't checked if my PR needs changes to the documentation
  • I haven't added tests that prove my fix is effective or that my feature works

Adds passkey button to login page:

CleanShot 2024-01-09 at 17 36 45@2x

Adds "Passkeys" settings section to sidebar:

CleanShot 2024-01-09 at 17 44 38@2x

Adds passkeys settings page

Empty:
CleanShot 2024-01-09 at 17 45 45@2x

  • Is the icon alright here? it looks somewhat more chunky than the "link" icon used e.g. for event types and api keys
  • On the other hand, it's the same icon we're using for passkeys everywhere (including the login page "Sign in with a passkey" button

Clicking on the "add" button and "sign in with a passkey" buttons opens a browser dialog. This looks different for every browser, phone, etc.

For example:
CleanShot 2024-01-09 at 17 47 48@2x

With passkeys added:
CleanShot 2024-01-09 at 17 49 38@2x

  • The name is set by the Hanko Passkeys API (our backend). We'll improve the name soon so it's not just a semi-random string and more user friendly
  • If the passkey was used, the description says e.g. "Created 3 days ago • Last used 5 minutes ago"

Dropdown stolen straight from API settings page:
CleanShot 2024-01-09 at 17 53 13@2x

This refactors the very important `authorize` function. I'm not sure if we want to keep it like this, but the authorize function itself does a lot of checks, only some relevant to `credentials`.

Instead of duplicating it, I added a second parameter, `unsafe`, intended to only be used from server code like in the PasskeyProvider.

When `unsafe` is used, all the checks dependent on `credentials` are not being run anymore. However, I am not sure how safe this is, or if a larger refactor is needed that properly separates checks dependent on `credentials` from all other checks.
Copy link

vercel bot commented Jan 9, 2024

Someone is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@CLAassistant
Copy link

CLAassistant commented Jan 9, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

github-actions bot commented Jan 9, 2024

Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link.

@github-actions github-actions bot added the ❗️ .env changes contains changes to env variables label Jan 9, 2024
@merlindru merlindru changed the title Passkey support feat: Passkey support Jan 9, 2024
Copy link
Member

@PeerRich PeerRich left a comment

Choose a reason for hiding this comment

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

hell yeah passkeys babyyyy

excited for this, will review

@keithwillcode keithwillcode added Medium priority Created by Linear-GitHub Sync authentication area: authentication, auth, google sign in, password, SAML, password reset, can't log in labels Jan 10, 2024
}).then((res) => res.json()),
onSuccess: props.onRemoved,
onError: (e: any) => {
showToast(e.message || t("something_went_wrong"), "error");
Copy link
Author

@merlindru merlindru Jan 10, 2024

Choose a reason for hiding this comment

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

Could also replace with

onError: (e: unknown) => {
    showToast(typeof e === "object" && e != null && "message" in e && typeof e.message === "string" ? e.message : t("something_went_wrong"), "error");

to get rid of eslint warning

@@ -55,6 +55,7 @@ const tabs: VerticalTabItemProps[] = [
{ name: "password", href: "/settings/security/password" },
{ name: "impersonation", href: "/settings/security/impersonation" },
{ name: "2fa_auth", href: "/settings/security/two-factor-auth" },
{ name: "passkeys", href: "/settings/security/passkeys" },
Copy link
Author

Choose a reason for hiding this comment

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

This still makes it appear above SSO. Is this position fine?

If we want it to be the last entry in the list, will have to add it below

Copy link
Member

Choose a reason for hiding this comment

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

positioning is fine

Copy link
Author

Choose a reason for hiding this comment

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

Could also move this to an .svg file if preferred

Copy link
Member

Choose a reason for hiding this comment

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

this is fine, however, ideally you move this into calcom/components/icon

@keithwillcode keithwillcode added this to the v3.9 milestone Jan 10, 2024
@keithwillcode keithwillcode added the community Created by Linear-GitHub Sync label Jan 11, 2024
@PeerRich
Copy link
Member

with this PR, can people also make an entire new account with passkeys? or just existing users?

@PeerRich
Copy link
Member

added HANKO_PASSKEYS_API_KEY and NEXT_PUBLIC_HANKO_PASSKEYS_TENANT_ID to vercel

Copy link
Contributor

Hey there, there is a merge conflict, can you take a look?

@github-actions github-actions bot added the 🚨 merge conflict This PR has a merge conflict that has to be addressed label Jan 23, 2024
Copy link
Contributor

github-actions bot commented Feb 7, 2024

This PR is being marked as stale due to inactivity.

@github-actions github-actions bot added the Stale label Feb 7, 2024
@keithwillcode keithwillcode requested a review from a team February 13, 2024 05:21
@PeerRich PeerRich removed the Stale label Feb 13, 2024
@keithwillcode keithwillcode added foundation and removed authentication area: authentication, auth, google sign in, password, SAML, password reset, can't log in labels Feb 16, 2024
Copy link
Contributor

github-actions bot commented Mar 2, 2024

This PR is being marked as stale due to inactivity.

@github-actions github-actions bot added the Stale label Mar 2, 2024
@keithwillcode keithwillcode modified the milestones: v3.9, v4.0 Mar 5, 2024
@keithwillcode
Copy link
Contributor

Moving to 4.0 based on current priorities.

@keithwillcode keithwillcode marked this pull request as draft March 5, 2024 13:17
@keithwillcode
Copy link
Contributor

Converted to draft until the conflicts can be resolved.

@github-actions github-actions bot removed the Stale label Mar 6, 2024
Copy link
Contributor

This PR is being marked as stale due to inactivity.

@github-actions github-actions bot added Stale and removed Stale labels Mar 21, 2024
@alishaz-polymath
Copy link
Member

alishaz-polymath commented Apr 6, 2024

Thank you for the PR 🙏
However, upon further investigation, if we roll out we are likely to go with nextauth passkeys implementation for now: https://authjs.dev/reference/core/providers/passkey#resources

This PR touches too many core auth files and we are not feeling 100% comfortable merging something that can potentially affect all users auth negatively

@dosubot dosubot bot modified the milestones: v4.0, v4.1 Apr 15, 2024
@zomars
Copy link
Member

zomars commented Apr 24, 2024

We had a call between Felix and @merlindru from the Hanko team and @keithwillcode and myself. We're expecting a new PR with a much simpler approach in the upcoming weeks.

@merlindru merlindru mentioned this pull request Apr 25, 2024
1 task
@dosubot dosubot bot added this to the v4.2 milestone May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Created by Linear-GitHub Sync ❗️ .env changes contains changes to env variables foundation Medium priority Created by Linear-GitHub Sync 🚨 merge conflict This PR has a merge conflict that has to be addressed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CAL-2984] Add Passkey support
6 participants