Implementing passkey authentication#106
Conversation
Summary of ChangesHello @ilblackdragon, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's authentication system by integrating passkey (WebAuthn) support. This allows users to log in using modern, secure, and passwordless methods, improving both security and user experience. The changes span across the API, service, and database layers, introducing new data models, business logic, and API endpoints to handle the full lifecycle of passkey authentication. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces passkey (WebAuthn) authentication, a significant security and usability enhancement. The implementation is comprehensive, covering database schema changes, a new repository, a passkey service for the core logic, and API route handlers. The code is well-structured, particularly the separation of concerns between the service, repository, and WebAuthn utility modules. My review includes a critical fix for user metrics, suggestions for code deduplication, and improvements for maintainability by removing dead code and magic numbers.
| let is_new_user = passkey.last_used_at.is_none(); | ||
| let auth_method = AuthMethod::Passkey; | ||
| let metric_name = if is_new_user { | ||
| METRIC_USER_SIGNUP | ||
| } else { | ||
| METRIC_USER_LOGIN | ||
| }; |
There was a problem hiding this comment.
The current logic for determining is_new_user is flawed. It checks if passkey.last_used_at.is_none(), which is true the first time any passkey is used, not just for a new user. This will lead to existing users being marked as new when they use a newly added passkey, skewing your signup metrics.
Since this endpoint is for logging in with an existing passkey and does not handle user creation, is_new_user should always be false. The user registration flow should be responsible for setting this flag correctly upon initial signup.
// This endpoint is for passkey login, not registration.
// The `is_new_user` flag should be determined by the registration flow.
// For login, it's always false.
let is_new_user = false;
let auth_method = AuthMethod::Passkey;
let metric_name = METRIC_USER_LOGIN;| let session = app_state | ||
| .session_repository | ||
| .create_session(passkey.user_id) | ||
| .await | ||
| .map_err(|e| { | ||
| tracing::error!( | ||
| "Failed to create session for passkey user {}: {}", | ||
| passkey.user_id, | ||
| e | ||
| ); | ||
| ApiError::internal_server_error("Failed to create session") | ||
| })?; | ||
|
|
||
| let token = session.token.ok_or_else(|| { | ||
| tracing::error!( | ||
| "Passkey session token missing for session_id={}", | ||
| session.session_id | ||
| ); | ||
| ApiError::internal_server_error("Failed to create session") | ||
| })?; | ||
|
|
||
| let is_new_user = passkey.last_used_at.is_none(); | ||
| let auth_method = AuthMethod::Passkey; | ||
| let metric_name = if is_new_user { | ||
| METRIC_USER_SIGNUP | ||
| } else { | ||
| METRIC_USER_LOGIN | ||
| }; | ||
| let tags = [ | ||
| format!("{}:{}", TAG_AUTH_METHOD, auth_method.as_str()), | ||
| format!("{}:{}", TAG_IS_NEW_USER, is_new_user), | ||
| ]; | ||
| let tag_refs: Vec<&str> = tags.iter().map(|s| s.as_str()).collect(); | ||
| app_state | ||
| .metrics_service | ||
| .record_count(metric_name, 1, &tag_refs); | ||
|
|
||
| let activity_type = if is_new_user { | ||
| ActivityType::Signup | ||
| } else { | ||
| ActivityType::Login | ||
| }; | ||
| if let Err(e) = app_state | ||
| .analytics_service | ||
| .record_activity(RecordActivityRequest { | ||
| user_id: passkey.user_id, | ||
| activity_type, | ||
| auth_method: Some(auth_method), | ||
| metadata: None, | ||
| }) | ||
| .await | ||
| { | ||
| tracing::warn!("Failed to record passkey analytics event: {}", e); | ||
| } |
There was a problem hiding this comment.
There's significant code duplication between this passkey_verify function and the near_auth function for handling session creation, metrics, and analytics. This repeated logic makes the code harder to maintain.
Consider extracting this common logic into a helper function. For example, a function with a signature like async fn finalize_login(app_state: &AppState, user_id: UserId, is_new_user: bool, auth_method: AuthMethod) -> Result<Json<NearAuthResponse>, ApiError> could encapsulate these steps, improving code reuse and clarity.
| pub fn generate_challenge() -> String { | ||
| let mut rng = rand::rng(); | ||
| std::iter::repeat_with(|| rng.sample(Alphanumeric)) | ||
| .take(64) | ||
| .map(char::from) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
This generate_challenge function appears to be unused. The actual challenge generation is handled in passkey_service.rs which correctly generates random bytes for the challenge.
This function is also misleading as it generates an alphanumeric string, which is not the format used for WebAuthn challenges. To avoid confusion and remove dead code, this function should be deleted.
| fn map_algorithm(&self, alg: i32) -> Result<WebAuthnAlgorithm> { | ||
| match alg { | ||
| -7 => Ok(WebAuthnAlgorithm::Es256), | ||
| -8 => Ok(WebAuthnAlgorithm::EdDsa), | ||
| other => Err(anyhow!("Unsupported algorithm {other}")), | ||
| } | ||
| } |
There was a problem hiding this comment.
This function uses magic numbers (-7, -8) to identify COSE algorithms. To improve readability and maintainability, you should use the constants (COSE_ALG_ES256, COSE_ALG_EDDSA) defined in crates/services/src/auth/webauthn.rs.
fn map_algorithm(&self, alg: i32) -> Result<WebAuthnAlgorithm> {
use super::webauthn::{COSE_ALG_EDDSA, COSE_ALG_ES256};
match alg as i8 {
COSE_ALG_ES256 => Ok(WebAuthnAlgorithm::Es256),
COSE_ALG_EDDSA => Ok(WebAuthnAlgorithm::EdDsa),
other => Err(anyhow!("Unsupported algorithm {other}")),
}
}
|
See #131 |
2 participants
Ref nearai/private-chat#244