Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesDPoP MFA integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
auth0/src/main/java/com/auth0/android/authentication/AuthenticationAPIClient.kt (1)
115-119: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument inherited DPoP behavior.
Line 119 makes
mfaClient()inherit this client’s DPoP configuration, but the public KDoc does not mention that behavior.📝 Proposed KDoc update
- * `@return` A new [MfaApiClient] instance configured for the transaction. + * `@return` A new [MfaApiClient] instance configured for the transaction. If this client + * has DPoP enabled via [useDPoP], the MFA client inherits that configuration.As per coding guidelines, “Update relevant documentation (README.md, EXAMPLES.md, KDoc comments) when changing public APIs”.
🤖 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 `@auth0/src/main/java/com/auth0/android/authentication/AuthenticationAPIClient.kt` around lines 115 - 119, The public KDoc for AuthenticationAPIClient.mfaClient currently omits that the returned MfaApiClient inherits this client’s DPoP configuration. Update the KDoc on mfaClient(mfaToken: String) to explicitly document this inherited DPoP behavior so the public API contract matches the implementation.Source: Coding guidelines
auth0/src/test/java/com/auth0/android/authentication/MfaApiClientTest.kt (1)
60-75: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRestore the global DPoP key store after each test.
Line 75 replaces
DPoPUtil.keyStoreglobally, buttearDown()does not restore it, which can leak the mock into other test classes.🧪 Proposed test isolation fix
private lateinit var gson: Gson private lateinit var mockKeyStore: DPoPKeyStore private lateinit var mockContext: Context + private lateinit var originalKeyStore: DPoPKeyStore `@Before` public fun setUp(): Unit { mockServer = SSLTestUtils.createMockWebServer() mockServer.start() @@ auth0.networkingClient = SSLTestUtils.testClient mfaClient = MfaApiClient(auth0, MFA_TOKEN) gson = GsonBuilder().serializeNulls().create() + originalKeyStore = DPoPUtil.keyStore mockKeyStore = mock() mockContext = mock() whenever(mockContext.applicationContext).thenReturn(mockContext) DPoPUtil.keyStore = mockKeyStore } `@After` public fun tearDown(): Unit { + DPoPUtil.keyStore = originalKeyStore mockServer.shutdown() }Also applies to: 78-81
🤖 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 `@auth0/src/test/java/com/auth0/android/authentication/MfaApiClientTest.kt` around lines 60 - 75, The test setup in MfaApiClientTest is replacing the global DPoPUtil.keyStore with a mock, but the teardown path does not restore the original store, which can leak state into other tests. Update the class-level test lifecycle around setUp/tearDown to save the previous DPoPUtil.keyStore before assigning mockKeyStore, then restore it after each test; use the existing MfaApiClientTest, setUp(), tearDown(), and DPoPUtil.keyStore symbols to locate the change.
🤖 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 `@auth0/src/test/java/com/auth0/android/authentication/MfaApiClientTest.kt`:
- Around line 113-214: Add callback-path coverage for DPoP verify in
MfaApiClientTest by exercising the verify flow through start(callback) instead
of only await(), using the existing MfaApiClient.useDPoP and
MfaVerificationType.Otp setup. Reuse the DPoP-enabled test pattern to assert the
request still hits /oauth/token with the DPoP header, and add a callback-based
case that verifies callback/error adaptation behavior matches the coroutine
path.
---
Outside diff comments:
In
`@auth0/src/main/java/com/auth0/android/authentication/AuthenticationAPIClient.kt`:
- Around line 115-119: The public KDoc for AuthenticationAPIClient.mfaClient
currently omits that the returned MfaApiClient inherits this client’s DPoP
configuration. Update the KDoc on mfaClient(mfaToken: String) to explicitly
document this inherited DPoP behavior so the public API contract matches the
implementation.
In `@auth0/src/test/java/com/auth0/android/authentication/MfaApiClientTest.kt`:
- Around line 60-75: The test setup in MfaApiClientTest is replacing the global
DPoPUtil.keyStore with a mock, but the teardown path does not restore the
original store, which can leak state into other tests. Update the class-level
test lifecycle around setUp/tearDown to save the previous DPoPUtil.keyStore
before assigning mockKeyStore, then restore it after each test; use the existing
MfaApiClientTest, setUp(), tearDown(), and DPoPUtil.keyStore symbols to locate
the change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: badf1c3d-67ea-41da-9151-0d7b9d5c0986
📒 Files selected for processing (3)
auth0/src/main/java/com/auth0/android/authentication/AuthenticationAPIClient.ktauth0/src/main/java/com/auth0/android/authentication/mfa/MfaApiClient.ktauth0/src/test/java/com/auth0/android/authentication/MfaApiClientTest.kt
| mockKeyStore = mock() | ||
| mockContext = mock() | ||
| whenever(mockContext.applicationContext).thenReturn(mockContext) | ||
| DPoPUtil.keyStore = mockKeyStore |
There was a problem hiding this comment.
Small thing — since setUp() swaps DPoPUtil.keyStore for a mock and it's a static, mind resetting it back to DPoPKeyStore() here? Otherwise the mock can bleed into other test classes. Not a big deal (the other client tests don't do it either and setUp reassigns each run).
| } | ||
|
|
||
| @Test | ||
| public fun shouldWrapDPoPExceptionAsMfaVerifyException(): Unit { |
There was a problem hiding this comment.
using getKeyPair() returning null to force the DPoPException works, but it leans on an internal detail of DPoPUtil. Might read cleaner to stub it to throw the DPoPException directly so the test says what it means and won't break if that null-handling ever changes. Totally optional.
2 participants
Changes
This PR adds DPoP support to
MFAApiClientclass.Checklist
I have read the Auth0 general contribution guidelines
I have read the Auth0 Code of Conduct
All existing and new tests complete without errors
Summary by CodeRabbit
verify, including inheritance from the main authentication client.verify.