feat: add passkeys #989
Merged
feat: add passkeys #989
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Important Auto Review SkippedAuto reviews are limited to the following labels: coderabbit. Please add one of these labels to enable auto reviews. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the WalkthroughThis update introduces comprehensive support for passkey authentication across the application. Modifications include the addition of passkey management functionality (creation, update, deletion, and listing of user passkeys), integration of server-side logic for passkey authentication, and UI updates to accommodate these changes. Additionally, the update includes enhancements to existing components for better usability and the introduction of new feature flags to control the availability of passkey authentication. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Thank you for following the naming conventions for pull request titles! 💚🚀 |
github-actions
bot
added
apps: web
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
apps/web/package.jsonis excluded by:!**/*.jsonpackage-lock.jsonis excluded by:!**/*.json
Files selected for processing (24)
- apps/web/src/app/(dashboard)/settings/security/activity/page.tsx (1 hunks)
- apps/web/src/app/(dashboard)/settings/security/page.tsx (4 hunks)
- apps/web/src/app/(dashboard)/settings/security/passkeys/create-passkey-dialog.tsx (1 hunks)
- apps/web/src/app/(dashboard)/settings/security/passkeys/page.tsx (1 hunks)
- apps/web/src/app/(dashboard)/settings/security/passkeys/user-passkeys-data-table-actions.tsx (1 hunks)
- apps/web/src/app/(dashboard)/settings/security/passkeys/user-passkeys-data-table.tsx (1 hunks)
- apps/web/src/components/(dashboard)/settings/layout/header.tsx (2 hunks)
- apps/web/src/components/forms/2fa/recovery-codes.tsx (1 hunks)
- apps/web/src/components/forms/signin.tsx (4 hunks)
- packages/lib/constants/auth.ts (1 hunks)
- packages/lib/constants/feature-flags.ts (2 hunks)
- packages/lib/next-auth/auth-options.ts (3 hunks)
- packages/lib/server-only/auth/create-passkey-registration-options.ts (1 hunks)
- packages/lib/server-only/auth/create-passkey-signin-options.ts (1 hunks)
- packages/lib/server-only/auth/create-passkey.ts (1 hunks)
- packages/lib/server-only/auth/delete-passkey.ts (1 hunks)
- packages/lib/server-only/auth/find-passkeys.ts (1 hunks)
- packages/lib/server-only/auth/update-passkey.ts (1 hunks)
- packages/lib/types/webauthn.ts (1 hunks)
- packages/lib/utils/authenticator.ts (1 hunks)
- packages/prisma/migrations/20240306060259_add_passkeys/migration.sql (1 hunks)
- packages/prisma/schema.prisma (3 hunks)
- packages/trpc/server/auth-router/router.ts (2 hunks)
- packages/trpc/server/auth-router/schema.ts (2 hunks)
Additional comments: 46
packages/lib/utils/authenticator.ts (1)
- 7-16: The implementation of
getAuthenticatorRegistrationOptionsis correct and follows best practices for extracting the RP details from the application's base URL. Good use of constants for defining the RP name and timeout.apps/web/src/components/(dashboard)/settings/layout/header.tsx (2)
- 8-8: The addition of the
hideDividerprop toSettingsHeaderPropsis a good enhancement for conditional rendering of UI elements based on the prop's value.- 32-32: Correct implementation of conditional rendering for the divider based on the
hideDividerprop. This enhances the component's flexibility.apps/web/src/components/forms/2fa/recovery-codes.tsx (1)
- 19-20: The styling changes to the
Buttoncomponent introducevariant="outline"and a new classbg-background. Please verify these changes align with the application's design guidelines.apps/web/src/app/(dashboard)/settings/security/activity/page.tsx (2)
- 18-18: Correct use of the
hideDividerprop inSettingsHeaderto enhance the page's visual layout by conditionally rendering the divider.- 23-25: Adding a
divwrapper withclassName="mt-4"aroundUserSecurityActivityDataTableis a good practice for improving visual spacing and layout consistency.apps/web/src/app/(dashboard)/settings/security/passkeys/page.tsx (2)
- 16-20: The feature flag check and conditional redirection logic are correctly implemented to ensure the passkey management page is only accessible when the feature is enabled.
- 24-26: Correct use of the
hideDividerprop inSettingsHeaderfor the passkey management page, consistent with the component's flexible design.packages/lib/types/webauthn.ts (1)
- 3-41: The Zod schemas for authentication and registration response validation are correctly defined, ensuring data integrity and providing clear error messages for invalid data. Good use of Zod for schema validation.
packages/lib/constants/feature-flags.ts (1)
- 26-26: The introduction of the
app_passkeyfeature flag based on theWEBAPP_BASE_URLvalue is a practical approach for conditional feature enablement. Ensure this aligns with the overall strategy for feature flag management and that the condition is appropriate for the intended environments.packages/lib/constants/auth.ts (3)
- 19-21: The addition of passkey-related audit log types (
PASSKEY_CREATED,PASSKEY_DELETED,PASSKEY_UPDATED) is a good practice for tracking significant events related to passkeys. This will be beneficial for auditing and monitoring purposes.- 27-27: Adding a specific audit log type for failed passkey sign-in attempts (
SIGN_IN_PASSKEY_FAIL) is crucial for security monitoring. It allows for the identification of potential unauthorized access attempts using passkeys.- 34-34: The
PASSKEY_TIMEOUTconstant defines a timeout duration for passkey verification. It's set to 60000 milliseconds (1 minute), which seems reasonable for user interaction. However, consider if this duration aligns with user experience expectations and security requirements.packages/lib/server-only/auth/create-passkey-registration-options.ts (1)
- 14-58: The
createPasskeyRegistrationOptionsfunction correctly retrieves user information and generates registration options for passkeys. It properly handles the exclusion of existing credentials and sets a timeout based on thePASSKEY_TIMEOUTconstant. Additionally, it creates a verification token with a 2-minute expiration, which is a good practice for security.However, ensure that the
expiresfield in the verification token creation (line 52) aligns with the expected challenge response time from the user. The 2-minute duration seems reasonable, but it should be consistent with the user experience and security policies.packages/lib/server-only/auth/find-passkeys.ts (1)
- 17-70: The
findPasskeysfunction is well-structured and provides a flexible way to query passkeys based on user ID, search term, pagination, and ordering. It uses Prisma'sfindManyandcountmethods to fetch data and calculate the total number of records, which is efficient for handling large datasets.One thing to note is the default ordering by
namein descending order (lines 24-25). Ensure this default behavior aligns with the user interface and user expectations. It might be more intuitive to order passkeys bycreatedAtorlastUsedAtby default for better usability.packages/prisma/migrations/20240306060259_add_passkeys/migration.sql (4)
- 9-12: The migration script correctly adds new values to the
UserSecurityAuditLogTypeenum to support passkey-related audit logs. This is essential for tracking passkey events in the system.- 15-30: The creation of the
Passkeytable with appropriate fields and constraints is well-defined. The use ofBYTEAfor storing credential IDs and public keys is appropriate for security. Ensure that thecredentialDeviceTypeandcredentialBackedUpfields are adequately documented to clarify their usage and expected values.- 33-40: The
AnonymousVerificationTokentable is correctly defined to support the management of verification tokens. The unique indexes onidandtokenensure data integrity and prevent duplicates.- 49-49: Adding a foreign key constraint (
Passkey_userId_fkey) on thePasskeytable to reference theUsertable with cascade options for delete and update actions is a good practice. It ensures data integrity and simplifies the management of user-related data.packages/trpc/server/auth-router/schema.ts (4)
- 38-41: The
ZCreatePasskeyMutationSchemais correctly defined to validate the input for creating a passkey. It ensures that thepasskeyNameis a non-empty string and that theverificationResponseadheres to theZRegistrationResponseJSONSchema. This validation is crucial for maintaining data integrity and preventing invalid inputs.- 43-45: The
ZDeletePasskeyMutationSchemais well-defined, requiring only thepasskeyIdas a non-empty string. This minimalistic approach is appropriate for delete operations, focusing on the essential identifier.- 47-50: The
ZUpdatePasskeyMutationSchemaincludes validation for both thepasskeyIdand the newnamefor the passkey. This schema ensures that both fields are provided and are non-empty strings, which is necessary for accurately identifying the passkey to update and applying the new name.- 52-59: The
ZFindPasskeysQuerySchemaextends theZBaseTableSearchParamsSchemaand adds an optionalorderByobject for specifying the ordering criteria. This flexibility in querying and ordering passkeys enhances the user experience by allowing customized views based on user preferences.packages/lib/server-only/auth/create-passkey.ts (1)
- 18-94: The
createPasskeyfunction is comprehensive, covering the verification of the registration response, deletion of the verification token, and creation of the passkey along with an audit log entry. It properly handles errors and ensures that the passkey is only created if the verification succeeds.A few points to consider:
- Ensure that the deletion of verification tokens (lines 44-49) before checking their expiration (lines 51-53) does not introduce race conditions or logic errors. It might be safer to check the expiration before deletion.
- The transaction (lines 71-93) ensures atomicity when creating the passkey and logging the event, which is a good practice.
Overall, the function is well-implemented, but pay attention to the order of operations regarding verification token handling.
apps/web/src/app/(dashboard)/settings/security/passkeys/user-passkeys-data-table.tsx (1)
- 17-119: The
UserPasskeysDataTablecomponent is well-structured, utilizing TRPC to fetch passkey data and handling pagination effectively. The use ofDataTableandDataTablePaginationcomponents from@documenso/uiensures a consistent UI experience.A few points to consider:
- Ensure that the
usePathnameanduseRouterhooks are used appropriately for navigation and URL management, especially in theonClearFiltersfunction (line 89).- The skeleton loading state (lines 93-115) provides a good user experience during data fetching. Verify that the number of skeleton rows and their appearance align with the actual data table layout.
Overall, the component is implemented correctly and should provide a smooth user experience for managing passkeys.
apps/web/src/app/(dashboard)/settings/security/page.tsx (1)
- 50-74: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [22-70]
The conditional rendering of the Passkeys section based on the
isPasskeyEnabledfeature flag (lines 53-70) is a good practice for controlled feature rollout. This approach allows for testing and gradual introduction of new features without impacting all users immediately.Ensure that the feature flag is correctly configured and tested across different environments to prevent unintended access to the passkey management functionality before it's fully ready for production use.
Verification successful
The verification process confirms that the
isPasskeyEnabledfeature flag is correctly implemented across different parts of the application, including the security settings page, to control access to the passkey functionality. This supports the original review comment about the good practice of using feature flags for controlled feature rollout and the conditional rendering of the Passkeys section based on theisPasskeyEnabledfeature flag.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the correct configuration and testing of the `isPasskeyEnabled` feature flag across different environments. # Manual verification recommended: Review feature flag configurations and conduct environment-specific testing. echo "Review feature flag configurations and conduct environment-specific testing for the `isPasskeyEnabled` flag."Length of output: 294
Script:
#!/bin/bash # Search the codebase for references to the `isPasskeyEnabled` feature flag to verify its configuration and usage. rg "isPasskeyEnabled" --context 5Length of output: 5495
apps/web/src/app/(dashboard)/settings/security/passkeys/user-passkeys-data-table-actions.tsx (1)
- 43-201: The
UserPasskeysDataTableActionscomponent provides a user-friendly interface for editing and deleting passkeys. The use of dialogs for confirmation and form submission is a good UX practice. The component correctly handles form submission and uses TRPC mutations for updating and deleting passkeys, with appropriate success and error handling.A few points to consider:
- Ensure that form validation for the
namefield in the update dialog is sufficient and aligns with any backend constraints on passkey names.- The loading states for the buttons during mutation operations (lines 148 and 192) enhance the user experience by providing feedback during asynchronous actions.
Overall, the component is well-implemented and should facilitate easy management of passkeys by the user.
apps/web/src/app/(dashboard)/settings/security/passkeys/create-passkey-dialog.tsx (5)
- 1-1: Using
'use client';at the top of the file is a good practice for Next.js applications to ensure that this component is only run on the client side, which is appropriate for a component that interacts with Web APIs like WebAuthn.- 42-44: The form validation schema defined with Zod is concise and ensures that the
passkeyNamefield is a string with a minimum length of 3 characters. This is a good practice for validating user input on the client side before sending it to the server.- 63-66: The use of TRPC's
useMutationhooks for creating passkey registration options and creating a passkey is a clean and efficient way to interact with the server-side logic. It keeps the component code focused on the UI logic while delegating data fetching and mutation to the TRPC framework.- 98-115: The
extractDefaultPasskeyNamefunction cleverly uses the user agent string to generate a default name for the passkey based on the user's browser and operating system. This is a thoughtful touch that enhances the user experience by providing a meaningful default value.- 118-128: The
useEffecthook is used to reset the form and set a default passkey name when the dialog is opened. This ensures that the form is in a clean state each time the user interacts with it, which is a good practice for form handling in React components.packages/trpc/server/auth-router/router.ts (6)
- 1-17: The imports are well-organized and relevant to the functionality being implemented. However, ensure that all imported modules are used within the file to avoid unnecessary imports that could lead to confusion or bloat.
- 120-133: The
createPasskeyRegistrationOptionsfunction is implemented correctly. It's good practice to log errors for monitoring and debugging purposes, as done here. Ensure that sensitive information is not logged to avoid potential security risks.- 136-157: The
createPasskeySigninOptionsfunction correctly extracts the CSRF token from the cookie and throws an error if it's missing. This is a crucial security measure. However, ensure that the CSRF token validation is robust and aligns with security best practices.- 159-178: The
deletePasskeyfunction includes proper error handling and uses request metadata, which is a good practice for auditing and security purposes. Ensure that the deletion operation is securely authenticated and authorized to prevent unauthorized access.- 180-200: The
findPasskeysfunction is implemented with pagination and ordering, which is excellent for performance and usability. Ensure that the data retrieval is optimized for large datasets to prevent potential performance issues.- 202-222: The
updatePasskeyfunction includes proper error handling and request metadata usage. Consider validating the input data thoroughly to prevent potential security vulnerabilities or data integrity issues.packages/lib/next-auth/auth-options.ts (2)
- 13-28: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [3-25]
The imports are correctly organized and relevant to the functionality being implemented. Ensure that all imported modules are used within the file to avoid unnecessary imports.
- 139-246: The new
CredentialsProviderfor 'webauthn' is well-implemented, with detailed error handling and validation logic. Ensure that the authentication logic aligns with security best practices, especially regarding the handling of CSRF tokens and the verification of authentication responses.apps/web/src/components/forms/signin.tsx (3)
- 9-20: The imports are correctly organized and relevant to the functionality being implemented. Ensure that all imported modules are used within the file to avoid unnecessary imports.
- 124-174: The
onSignInWithPasskeyfunction is well-implemented, with comprehensive error handling and user feedback. Ensure that the error handling covers all potential failure scenarios and that user feedback is clear and actionable.- 332-343: The UI update for the passkey sign-in button is correctly implemented. Ensure that the button is accessible, with appropriate aria-labels and keyboard navigation support, to enhance accessibility for all users.
packages/prisma/schema.prisma (3)
- 55-55: The
passkeysfield has been added to theUsermodel to establish a one-to-many relationship with thePasskeymodel. This is a crucial addition for linking users to their respective passkeys.- 72-81: New enum values (
PASSKEY_CREATED,PASSKEY_DELETED,PASSKEY_UPDATED,SIGN_IN_PASSKEY_FAIL) have been added toUserSecurityAuditLogType. These additions are essential for tracking passkey-related activities in the audit logs, enhancing security and auditability.- 121-126: The
AnonymousVerificationTokenmodel has been added, which seems to be used for anonymous verification processes. This model includes fields forid,token,expiresAt, andcreatedAt. The structure is appropriate for its intended use. Ensure that the token generation and expiration handling are securely implemented in the application logic.
apps/web/src/app/(dashboard)/settings/security/passkeys/create-passkey-dialog.tsx
Show resolved
Hide resolved
apps/web/src/app/(dashboard)/settings/security/passkeys/create-passkey-dialog.tsx
Outdated
Show resolved
Hide resolved
Mythie
reviewed
Mar 21, 2024
apps/web/src/app/(dashboard)/settings/security/passkeys/user-passkeys-data-table-actions.tsx
Outdated
Show resolved
Hide resolved
apps/web/src/app/(dashboard)/settings/security/passkeys/user-passkeys-data-table.tsx
Show resolved
Hide resolved
Mythie
reviewed
Mar 25, 2024
|
Added the changes, and did some retesting |
Mythie
approved these changes
Mar 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Add support to login with passkeys.
Passkeys can be added via the user security settings page.
Note: Currently left out adding the type of authentication method for the 'user security audit logs' because we're using the
signInnext-auth event which doesn't appear to provide the context. Will look into it at another time.Changes Made
Testing Performed
To be done.
MacOS:
Windows:
Linux:
iOS:
Checklist
Summary by CodeRabbit