Port session reverification support to Android#698
Conversation
|
Complex PR? Review this PR in Change Stack to move by importance, not file order. Warning Review limit reached
More reviews will be available in 14 minutes and 28 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds session in-session re-verification (step-up authentication) support to the Clerk Android SDK. It introduces the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
source/api/src/main/kotlin/com/clerk/api/passkeys/GoogleCredentialAuthenticationService.kt (2)
55-55: 💤 Low valueConsider refactoring instead of suppressing Detekt rules.
The
@Suppress("TooManyFunctions")annotation bypasses Detekt static analysis. As per coding guidelines, Detekt should be used with all rules enabled. Consider extracting helper objects or splitting responsibilities to reduce the function count.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/api/src/main/kotlin/com/clerk/api/passkeys/GoogleCredentialAuthenticationService.kt` at line 55, The class-level suppression `@Suppress`("TooManyFunctions") on GoogleCredentialAuthenticationService hides a Detekt warning; remove the suppression and split responsibilities by extracting cohesive helper classes or objects (e.g., GoogleOAuthClient for external API calls, TokenValidator for validation logic, CredentialMapper for mapping/transformations, and possibly a GoogleCredentialRepository for persistence-related methods), move the corresponding methods from GoogleCredentialAuthenticationService into these new classes, update constructor/Dagger/Koin injection of the new collaborators into GoogleCredentialAuthenticationService, and adjust unit tests; after refactoring, re-run Detekt and ensure the original class no longer exceeds the function limit so the suppression can be removed.
139-144: ⚡ Quick winConsider adding null check for sign-in nonce.
Similar to the session verification flow,
signIn.firstFactorVerification?.nonce(line 141) is nullable. If null,createWebAuthnRequestat line 373 will throw a genericrequireNotNullerror. Consider adding an explicit null check here as well for consistency and better error reporting.📋 Suggested null check
else -> { val activity = Clerk.credentialActivity()!! when (val createResult = createSignIn()) { is ClerkResult.Success -> { val signIn = createResult.value + val nonce = signIn.firstFactorVerification?.nonce + if (nonce == null) { + ClerkLog.e("Missing nonce in first factor verification for passkey sign-in") + return ClerkResult.unknownFailure( + IllegalStateException("First factor verification nonce is required for passkey authentication") + ) + } try { val credential = getCredentialFromManager( activity = activity, - nonce = signIn.firstFactorVerification?.nonce, + nonce = nonce, allowedCredentialIds = allowedCredentialIds, credentialRequestTypes = credentialTypes, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/api/src/main/kotlin/com/clerk/api/passkeys/GoogleCredentialAuthenticationService.kt` around lines 139 - 144, The call to getCredentialFromManager passes signIn.firstFactorVerification?.nonce which is nullable and can cause a generic requireNotNull failure later in createWebAuthnRequest; add an explicit null check before calling getCredentialFromManager (e.g., validate signIn.firstFactorVerification and its nonce) and return or throw a descriptive error (with context like "missing first-factor verification nonce for sign-in") so getCredentialFromManager / createWebAuthnRequest receive a non-null nonce; update the code paths around the getCredentialFromManager invocation and any callers that rely on the nonce to use this explicit validation.source/api/src/test/java/com/clerk/api/session/SessionVerificationTest.kt (1)
70-170: ⚡ Quick winConsider adding failure case tests.
The current test suite thoroughly validates the success paths (
ClerkResult.Success), but does not cover failure scenarios (ClerkResult.Failure). For authentication flows, it's valuable to verify that errors are properly propagated and handled.Consider adding tests that mock
ClerkResult.Failureresponses from theSessionApimethods to ensure the extension functions correctly forward error states. As per coding guidelines, regression tests should be added when altering auth flows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/api/src/test/java/com/clerk/api/session/SessionVerificationTest.kt` around lines 70 - 170, Add tests that cover ClerkResult.Failure paths by mocking ClerkApi.session to return ClerkResult.Failure from the same SessionApi methods already used in success tests (startVerification, prepareFirstFactorVerification, attemptFirstFactorVerification, attemptSecondFactorVerification) and asserting the session extension methods (Session.startVerification, Session.prepareFirstFactorVerification, Session.verifyWithPassword, Session.verifyWithTOTP, Session.verifyWithBackupCode) return ClerkResult.Failure and that the corresponding SessionApi method was invoked (use coEvery to return ClerkResult.failure(...) and coVerify to check call count); mirror the existing success-test structure (mockkObject(ClerkApi), every { ClerkApi.session } returns sessionApi, coEvery { sessionApi.<method>("sess_123", params) } returns ClerkResult.failure(...), call the session method, assert result is ClerkResult.Failure, and coVerify(exactly = 1) the API call).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@source/api/src/main/kotlin/com/clerk/api/passkeys/GoogleCredentialAuthenticationService.kt`:
- Around line 186-232: In verifySessionWithPasskey, check
preparedVerification.firstFactorVerification?.nonce for null before calling
getCredentialFromManager; if it's null, log a clear error like "Missing nonce in
preparedVerification.firstFactorVerification" and return a ClerkResult failure
(e.g., ClerkResult.unknownFailure(IllegalStateException("Missing nonce in
prepared verification"))) instead of proceeding to getCredentialFromManager so
the later requireNotNull crash in createWebAuthnRequest is avoided and the error
is reported clearly.
In
`@source/api/src/main/kotlin/com/clerk/api/session/SessionVerificationExtensions.kt`:
- Around line 51-64: The attemptFirstFactorVerification function currently
prefers password > publicKeyCredential > code and throws on invalid input;
instead validate inputs so exactly one of code, password, or publicKeyCredential
is provided and ensure a code must be paired with a non-empty strategy while
password/passkey must not be combined with a strategy mismatch; when validation
fails return a ClerkResult failure (ClerkResult.Failure/ClerkResult.Error
variant using ClerkErrorResponse) rather than calling error(), and construct
Session.AttemptFirstFactorParams only after validation (refer to
Session.attemptFirstFactorVerification and
Session.AttemptFirstFactorParams.Password/Passkey/Code to locate the code).
Ensure the function uses pattern matching on ClerkResult sealed types for the
result path and does not throw exceptions for input validation errors.
In `@source/api/src/test/java/com/clerk/api/session/SessionVerificationTest.kt`:
- Around line 19-190: Add a unit test in SessionVerificationTest.kt that covers
Session.verifyWithPasskey by mocking PasskeyService.verifySessionWithPasskey:
create a session via testSession(), mock ClerkApi.session if needed and
mockkObject(PasskeyService) or the relevant service object, set coEvery {
PasskeyService.verifySessionWithPasskey("sess_123", any(), any()) } to return
ClerkResult.success(testVerification(...)) and an error case, call
session.verifyWithPasskey(allowedCredentialIds = listOf("cred_1","cred_2")),
assert the result is ClerkResult.Success and the returned SessionVerification is
the same instance, and coVerify that verifySessionWithPasskey was invoked with
"sess_123" and the expected allowedCredentialIds to ensure parameters are
forwarded correctly.
---
Nitpick comments:
In
`@source/api/src/main/kotlin/com/clerk/api/passkeys/GoogleCredentialAuthenticationService.kt`:
- Line 55: The class-level suppression `@Suppress`("TooManyFunctions") on
GoogleCredentialAuthenticationService hides a Detekt warning; remove the
suppression and split responsibilities by extracting cohesive helper classes or
objects (e.g., GoogleOAuthClient for external API calls, TokenValidator for
validation logic, CredentialMapper for mapping/transformations, and possibly a
GoogleCredentialRepository for persistence-related methods), move the
corresponding methods from GoogleCredentialAuthenticationService into these new
classes, update constructor/Dagger/Koin injection of the new collaborators into
GoogleCredentialAuthenticationService, and adjust unit tests; after refactoring,
re-run Detekt and ensure the original class no longer exceeds the function limit
so the suppression can be removed.
- Around line 139-144: The call to getCredentialFromManager passes
signIn.firstFactorVerification?.nonce which is nullable and can cause a generic
requireNotNull failure later in createWebAuthnRequest; add an explicit null
check before calling getCredentialFromManager (e.g., validate
signIn.firstFactorVerification and its nonce) and return or throw a descriptive
error (with context like "missing first-factor verification nonce for sign-in")
so getCredentialFromManager / createWebAuthnRequest receive a non-null nonce;
update the code paths around the getCredentialFromManager invocation and any
callers that rely on the nonce to use this explicit validation.
In `@source/api/src/test/java/com/clerk/api/session/SessionVerificationTest.kt`:
- Around line 70-170: Add tests that cover ClerkResult.Failure paths by mocking
ClerkApi.session to return ClerkResult.Failure from the same SessionApi methods
already used in success tests (startVerification,
prepareFirstFactorVerification, attemptFirstFactorVerification,
attemptSecondFactorVerification) and asserting the session extension methods
(Session.startVerification, Session.prepareFirstFactorVerification,
Session.verifyWithPassword, Session.verifyWithTOTP,
Session.verifyWithBackupCode) return ClerkResult.Failure and that the
corresponding SessionApi method was invoked (use coEvery to return
ClerkResult.failure(...) and coVerify to check call count); mirror the existing
success-test structure (mockkObject(ClerkApi), every { ClerkApi.session }
returns sessionApi, coEvery { sessionApi.<method>("sess_123", params) } returns
ClerkResult.failure(...), call the session method, assert result is
ClerkResult.Failure, and coVerify(exactly = 1) the API call).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5da85b88-cb45-4b2c-9d9c-e615318504ef
📒 Files selected for processing (10)
source/api/src/main/kotlin/com/clerk/api/network/ApiPaths.ktsource/api/src/main/kotlin/com/clerk/api/network/api/SessionApi.ktsource/api/src/main/kotlin/com/clerk/api/network/model/factor/Factor.ktsource/api/src/main/kotlin/com/clerk/api/passkeys/GoogleCredentialAuthenticationService.ktsource/api/src/main/kotlin/com/clerk/api/passkeys/PasskeyService.ktsource/api/src/main/kotlin/com/clerk/api/session/Session.ktsource/api/src/main/kotlin/com/clerk/api/session/SessionVerification.ktsource/api/src/main/kotlin/com/clerk/api/session/SessionVerificationConvenienceExtensions.ktsource/api/src/main/kotlin/com/clerk/api/session/SessionVerificationExtensions.ktsource/api/src/test/java/com/clerk/api/session/SessionVerificationTest.kt
There was a problem hiding this comment.
🧹 Nitpick comments (1)
source/api/src/test/java/com/clerk/api/session/SessionVerificationTest.kt (1)
191-223: 💤 Low valueConsider isolating global state mutations in tests.
The test mutates
Clerk.applicationId(line 197) and restores it in a finally block (line 221). While the cleanup is handled correctly, mutating global state can lead to test fragility or race conditions if tests ever run in parallel.Consider mocking
Clerkas an object or using dependency injection to make the test more isolated and robust.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@source/api/src/test/java/com/clerk/api/session/SessionVerificationTest.kt` around lines 191 - 223, The test mutates global Clerk.applicationId; instead avoid changing global state by mocking Clerk as an object and stubbing its applicationId for the test (or inject applicationId into the Session under test). Replace the direct assignment to Clerk.applicationId with mockkObject(Clerk) and stub every { Clerk.applicationId } returns "com.clerk.test" before calling session.startEnterpriseSso, and then unmockkObject(Clerk) in finally (or prefer constructor/DI to pass the app id into Session and update SessionVerificationTest to supply it), ensuring you still verify sessionApi.prepareFirstFactorVerification with the expected params.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@source/api/src/test/java/com/clerk/api/session/SessionVerificationTest.kt`:
- Around line 191-223: The test mutates global Clerk.applicationId; instead
avoid changing global state by mocking Clerk as an object and stubbing its
applicationId for the test (or inject applicationId into the Session under
test). Replace the direct assignment to Clerk.applicationId with
mockkObject(Clerk) and stub every { Clerk.applicationId } returns
"com.clerk.test" before calling session.startEnterpriseSso, and then
unmockkObject(Clerk) in finally (or prefer constructor/DI to pass the app id
into Session and update SessionVerificationTest to supply it), ensuring you
still verify sessionApi.prepareFirstFactorVerification with the expected params.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a650c5a-9453-4f79-b33a-7646efa37287
📒 Files selected for processing (4)
source/api/src/main/kotlin/com/clerk/api/session/Session.ktsource/api/src/main/kotlin/com/clerk/api/session/SessionVerificationConvenienceExtensions.ktsource/api/src/main/kotlin/com/clerk/api/session/SessionVerificationExtensions.ktsource/api/src/test/java/com/clerk/api/session/SessionVerificationTest.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- source/api/src/main/kotlin/com/clerk/api/session/Session.kt
- source/api/src/main/kotlin/com/clerk/api/session/SessionVerificationExtensions.kt
2 participants
Summary
SessionVerificationplus session reverification APIs for start, prepare, and attempt flowsSessionwith reverification convenience methods, including password, TOTP, backup code, and passkey paths/client/sessions/{id}/verify...endpoints and factor metadata needed by the flowTesting
:source:api:testpassed:source:api:spotlessCheckpassed:source:api:detektpassed