fix : Handling DPoP enabled WebAuth flow after process death#977
fix : Handling DPoP enabled WebAuth flow after process death#977pmathew92 wants to merge 13 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPersist DPoP enablement as a boolean, thread Android Context through the OAuth restoration path, buffer restored OAuth results until callbacks attach, re-enable DPoP on OAuthManager reconstruction, and add tests covering serialization, deserialization, and process-death restore. ChangesDPoP State Persistence Across Process Death
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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. ✨ Finishing Touches🧪 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.
🧹 Nitpick comments (2)
auth0/src/test/java/com/auth0/android/provider/WebAuthProviderTest.kt (1)
2979-2980: 🏗️ Heavy liftValidate restored behavior at the token exchange boundary, not only object presence.
A non-null
restoredManager.dPoPdoesn’t prove/oauth/tokenuses DPoP proof. Add an assertion that the resumed token request carries the DPoP header to directly cover the original regression.Suggested test direction
- val restoredManager = WebAuthProvider.managerInstance as OAuthManager - assertThat(restoredManager.dPoP, `is`(notNullValue())) + val restoredManager = WebAuthProvider.managerInstance as OAuthManager + assertThat(restoredManager.dPoP, `is`(notNullValue())) + // Then resume the flow and assert the outgoing /oauth/token request includes a DPoP header. + // (Use existing request-capture patterns in this test suite to verify headers.)🤖 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/provider/WebAuthProviderTest.kt` around lines 2979 - 2980, The test currently only checks restoredManager.dPoP is non-null; update it to assert the resumed token-exchange request actually includes the DPoP header so the /oauth/token boundary is covered. After obtaining restoredManager from WebAuthProvider.managerInstance (cast to OAuthManager), simulate or capture the resumed token request the manager sends during resume (the same place the original regression occurred) and assert that the HTTP request contains the DPoP header (e.g., "DPoP" or the library's header constant) and/or the expected proof value; ensure you reference restoredManager.dPoP and the resumed token request object when adding the assertion.auth0/src/test/java/com/auth0/android/provider/OAuthManagerStateTest.kt (1)
124-126: ⚡ Quick winUse JSON field removal instead of raw string replacement for legacy payload simulation.
The current replacement is formatting/order-sensitive and can become flaky. Remove
dPoPEnabledvia a parsed JSON object instead.Suggested diff
+import com.google.gson.JsonObject ... - // Remove the dPoPEnabled field to simulate legacy JSON - val legacyJson = json.replace(",\"dPoPEnabled\":false", "") + // Remove the dPoPEnabled field to simulate legacy JSON + val legacyJsonObject = GsonProvider.gson.fromJson(json, JsonObject::class.java) + legacyJsonObject.remove("dPoPEnabled") + val legacyJson = legacyJsonObject.toString()🤖 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/provider/OAuthManagerStateTest.kt` around lines 124 - 126, The test currently generates legacyJson by doing a fragile string replacement on json; instead parse the JSON into a JSON object, remove the "dPoPEnabled" field programmatically, then serialize back to a string so ordering/formatting won't break the test; locate the variables json and legacyJson in OAuthManagerStateTest.kt and replace the raw string replace with logic that parses json (e.g., using org.json.JSONObject or your project’s JSON library), calls remove("dPoPEnabled") on the parsed object, and assigns legacyJson to the serialized result.
🤖 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 `@auth0/src/test/java/com/auth0/android/provider/OAuthManagerStateTest.kt`:
- Around line 124-126: The test currently generates legacyJson by doing a
fragile string replacement on json; instead parse the JSON into a JSON object,
remove the "dPoPEnabled" field programmatically, then serialize back to a string
so ordering/formatting won't break the test; locate the variables json and
legacyJson in OAuthManagerStateTest.kt and replace the raw string replace with
logic that parses json (e.g., using org.json.JSONObject or your project’s JSON
library), calls remove("dPoPEnabled") on the parsed object, and assigns
legacyJson to the serialized result.
In `@auth0/src/test/java/com/auth0/android/provider/WebAuthProviderTest.kt`:
- Around line 2979-2980: The test currently only checks restoredManager.dPoP is
non-null; update it to assert the resumed token-exchange request actually
includes the DPoP header so the /oauth/token boundary is covered. After
obtaining restoredManager from WebAuthProvider.managerInstance (cast to
OAuthManager), simulate or capture the resumed token request the manager sends
during resume (the same place the original regression occurred) and assert that
the HTTP request contains the DPoP header (e.g., "DPoP" or the library's header
constant) and/or the expected proof value; ensure you reference
restoredManager.dPoP and the resumed token request object when adding the
assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 90c807f2-792c-4cc3-953f-8b8741d2cd1d
📒 Files selected for processing (6)
auth0/src/main/java/com/auth0/android/provider/AuthenticationActivity.ktauth0/src/main/java/com/auth0/android/provider/OAuthManager.ktauth0/src/main/java/com/auth0/android/provider/OAuthManagerState.ktauth0/src/main/java/com/auth0/android/provider/WebAuthProvider.ktauth0/src/test/java/com/auth0/android/provider/OAuthManagerStateTest.ktauth0/src/test/java/com/auth0/android/provider/WebAuthProviderTest.kt
|
@CodeRabbit review |
✅ Action performedReview finished.
|
| callback = callback, | ||
| customAuthorizeUrl = state.customAuthorizeUrl | ||
| customAuthorizeUrl = state.customAuthorizeUrl, | ||
| dPoP = if (state.dPoPEnabled ) DPoP(context) else null |
There was a problem hiding this comment.
The real fix is the useDPoP(context) call above — this dPoP arg only gets read in startAuthentication(), which doesn't run on resume. Is it set here just to keep state consistent / for the test to assert on? Want to confirm it's intentional.
There was a problem hiding this comment.
Yes, the dPoP arg is intentional — for state-consistency on toState(), round-trip + the test assertion.
|
@pmathew92 The PR notes the PAR restore path (PARCodeManager.fromState) is left unchanged. If a DPoP + PAR Universal Login flow is supported in this SDK, wouldn't it hit the same process-death bug and not be covered by this fix? Could you confirm whether that combination is possible? |
|
@utkrishtsahu PAR flow doesn't support DPoP since in PAR the jwk exchange is done in the client backend. So the flow is not applicable |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
auth0/src/test/java/com/auth0/android/provider/OAuthManagerStateTest.kt (1)
87-109:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the serialized field explicitly in the default-false test.
Line 87 currently validates only round-trip behavior; it can still pass if
serializeToJson()stops emittingdPoPEnabledand deserialization falls back tofalse.Suggested patch
val json = state.serializeToJson() + Assert.assertTrue(json.contains("\"dPoPEnabled\":false")) val deserializedState = OAuthManagerState.deserializeState(json) Assert.assertFalse(deserializedState.dPoPEnabled)🤖 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/provider/OAuthManagerStateTest.kt` around lines 87 - 109, The test currently only round-trips state via OAuthManagerState.serializeToJson() and deserializeState(), which can hide missing serialization of dPoPEnabled; update the test to explicitly assert the serialized JSON contains the dPoPEnabled field set to false (e.g. check the JSON string includes the "dPoPEnabled":false key/value) before calling OAuthManagerState.deserializeState(json), referencing serializeToJson() and OAuthManagerState.deserializeState so the test fails if the serializer stops emitting dPoPEnabled.
🧹 Nitpick comments (1)
auth0/src/test/java/com/auth0/android/provider/OAuthManagerStateTest.kt (1)
131-134: ⚡ Quick winUse structured JSON mutation for the legacy-payload test.
Line 133 relies on exact JSON text formatting/order. Parsing to a JSON object and removing the key makes this test resilient.
Suggested patch
+import com.google.gson.JsonObject ... - val legacyJson = json.replace(",\"dPoPEnabled\":false", "") + val jsonObject = GsonProvider.gson.fromJson(json, JsonObject::class.java) + jsonObject.remove("dPoPEnabled") + val legacyJson = jsonObject.toString()🤖 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/provider/OAuthManagerStateTest.kt` around lines 131 - 134, The test in OAuthManagerStateTest currently mutates the serialized JSON string using exact text replacement (the json and legacyJson variables); instead, parse the serialized string returned by serializeToJson() into a JSON object/node, remove the "dPoPEnabled" key from that object, then re-serialize to a string to produce legacyJson so the test no longer depends on property ordering/formatting; locate the serializeToJson() usage in the test and replace the string replace call with JSON parsing, key removal, and toString/serialize back to JSON.
🤖 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.
Outside diff comments:
In `@auth0/src/test/java/com/auth0/android/provider/OAuthManagerStateTest.kt`:
- Around line 87-109: The test currently only round-trips state via
OAuthManagerState.serializeToJson() and deserializeState(), which can hide
missing serialization of dPoPEnabled; update the test to explicitly assert the
serialized JSON contains the dPoPEnabled field set to false (e.g. check the JSON
string includes the "dPoPEnabled":false key/value) before calling
OAuthManagerState.deserializeState(json), referencing serializeToJson() and
OAuthManagerState.deserializeState so the test fails if the serializer stops
emitting dPoPEnabled.
---
Nitpick comments:
In `@auth0/src/test/java/com/auth0/android/provider/OAuthManagerStateTest.kt`:
- Around line 131-134: The test in OAuthManagerStateTest currently mutates the
serialized JSON string using exact text replacement (the json and legacyJson
variables); instead, parse the serialized string returned by serializeToJson()
into a JSON object/node, remove the "dPoPEnabled" key from that object, then
re-serialize to a string to produce legacyJson so the test no longer depends on
property ordering/formatting; locate the serializeToJson() usage in the test and
replace the string replace call with JSON parsing, key removal, and
toString/serialize back to JSON.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d5f7c3b-06f0-4fe2-af2d-1ce2d7876bba
📒 Files selected for processing (5)
.github/workflows/test.ymlauth0/src/main/java/com/auth0/android/provider/OAuthManager.ktauth0/src/main/java/com/auth0/android/provider/OAuthManagerState.ktauth0/src/test/java/com/auth0/android/provider/OAuthManagerStateTest.ktauth0/src/test/java/com/auth0/android/provider/WebAuthProviderTest.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- auth0/src/test/java/com/auth0/android/provider/WebAuthProviderTest.kt
- auth0/src/main/java/com/auth0/android/provider/OAuthManager.kt
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/main/java/com/auth0/android/provider/WebAuthProvider.kt`:
- Around line 184-203: The restored callback dispatch is racy: while you check
callbacks.isEmpty() under recoveryLock in onRestoreInstanceState's
Callback.onSuccess/onFailure, you iterate callbacks outside the lock and
removeCallback mutates callbacks without locking, so recovered results may be
lost; fix by acquiring recoveryLock, check for empty and set pendingRecovered if
empty, otherwise make a shallow copy of callbacks (e.g., new list) while still
inside synchronized(recoveryLock), release the lock and then iterate that copied
list to call callback.onSuccess/onFailure; reference symbols: recoveryLock,
callbacks, pendingRecovered, RecoveredResult.Success/Failure,
onRestoreInstanceState and removeCallback.
🪄 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: 04df376a-a753-4bd2-85af-a411cc821911
📒 Files selected for processing (1)
auth0/src/main/java/com/auth0/android/provider/WebAuthProvider.kt
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
auth0/src/main/java/com/auth0/android/provider/WebAuthProvider.kt (1)
145-167:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winInconsistency: AI summary claims buffering was added, but code shows buffering mechanism was removed.
The AI-generated summary states "added buffering for recovered results until callbacks register," but the code shows the
RecoveredResultsealed class,pendingRecoveredfield, andrecoveryLocksynchronization were all removed (confirmed by the library context summary). Lines 154–157 and 160–163 directly iteratecallbackswithout checking for emptiness or storing pending results.
Critical: Restored OAuth results are silently dropped if
callbacksis empty.When
onRestoreInstanceStatereconstructs theOAuthManagerafter process death and the token exchange completes, the inline callback (lines 153–165) directly iterates thecallbacksset. If no caller has invokedaddCallbackyet—common if the host activity/fragment is still initializing—the iteration produces zero deliveries and theCredentialsorAuthenticationExceptionresult is permanently lost. Users will never receive their authentication outcome after process death.The past review comment on this file flagged a similar race with
recoveryLock/pendingRecoveredand was marked addressed in commit 29ce30e, but the PR objectives note that subsequent revert commits removed the recovery mechanism, leaving the current code without any buffering.🛡️ Proposed fix: restore buffering or enforce callback registration before restore
Option 1 (recommended): Restore the buffering mechanism
Re-introduce a pending-result holder checked in
addCallback:+ private sealed class RecoveredResult { + data class Success(val credentials: Credentials) : RecoveredResult() + data class Failure(val error: AuthenticationException) : RecoveredResult() + } + private val recoveryLock = Any() + `@Volatile` private var pendingRecovered: RecoveredResult? = null + `@JvmStatic` public fun addCallback(callback: Callback<Credentials, AuthenticationException>) { callbacks += callback + val pending = synchronized(recoveryLock) { + pendingRecovered.also { pendingRecovered = null } + } + when (pending) { + is RecoveredResult.Success -> callback.onSuccess(pending.credentials) + is RecoveredResult.Failure -> callback.onFailure(pending.error) + null -> {} + } } override fun onSuccess(result: Credentials) { + val snapshot = synchronized(recoveryLock) { + if (callbacks.isEmpty()) { + pendingRecovered = RecoveredResult.Success(result) + return + } + callbacks.toList() + } - for (callback in callbacks) { + for (callback in snapshot) { callback.onSuccess(result) } } override fun onFailure(error: AuthenticationException) { + val snapshot = synchronized(recoveryLock) { + if (callbacks.isEmpty()) { + pendingRecovered = RecoveredResult.Failure(error) + return + } + callbacks.toList() + } - for (callback in callbacks) { + for (callback in snapshot) { callback.onFailure(error) } }Option 2: Document that callers must register callbacks before calling
onRestoreInstanceStateThis shifts the burden to
AuthenticationActivityand requires architectural changes; not recommended for an internal recovery path.🤖 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/provider/WebAuthProvider.kt` around lines 145 - 167, The restored OAuth flow can drop results if no callbacks are registered; reinstate the buffering mechanism: add back the RecoveredResult sealed class (or equivalent), a nullable pendingRecovered field and a recoveryLock object, then in onRestoreInstanceState where OAuthManager.fromState is created store the Credentials or AuthenticationException into pendingRecovered (synchronized on recoveryLock) when callbacks.isEmpty(), otherwise deliver to callbacks immediately; finally update addCallback to, under recoveryLock, check pendingRecovered and deliver and clear it after registering the new callback so late-registered callbacks receive the buffered result.
🤖 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.
Outside diff comments:
In `@auth0/src/main/java/com/auth0/android/provider/WebAuthProvider.kt`:
- Around line 145-167: The restored OAuth flow can drop results if no callbacks
are registered; reinstate the buffering mechanism: add back the RecoveredResult
sealed class (or equivalent), a nullable pendingRecovered field and a
recoveryLock object, then in onRestoreInstanceState where OAuthManager.fromState
is created store the Credentials or AuthenticationException into
pendingRecovered (synchronized on recoveryLock) when callbacks.isEmpty(),
otherwise deliver to callbacks immediately; finally update addCallback to, under
recoveryLock, check pendingRecovered and deliver and clear it after registering
the new callback so late-registered callbacks receive the buffered result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b9967ab-3cfb-4b0f-835e-6eb62f0425ff
📒 Files selected for processing (1)
auth0/src/main/java/com/auth0/android/provider/WebAuthProvider.kt
3 participants
Changes
Fixes a bug where the OAuth token exchange fails after process death during a
DPoP-enabled Universal Login flow.
When the Android host process is killed while Chrome Custom Tabs is open, the SDK
persists
OAuthManagerStatetosavedInstanceStateand rebuilds the manager viaOAuthManager.fromStateon restart. Previously, the DPoP configuration was lost onthis restore path:
OAuthManager.toState()did not persist the DPoP-enabled flag.OAuthManager.fromState()reconstructed the manager without DPoP, and the restoredPKCE'sAuthenticationAPIClientwas never DPoP-enabled.As a result, the
/oauth/tokenexchange after process restart was sent without aDPoP proof header, and the server rejected it with HTTP 400
invalid_request("Authorization Code is bound to a DPoP key, but DPoP header is missing or invalid").
This change persists a
dPoPEnabledboolean inOAuthManagerState(theDPoPobject holds a
Contextand cannot be serialized directly), and on restore re-createsthe
DPoPinstance and re-enables DPoP on the restored API client so the tokenexchange carries the proof header.
References
react-native-auth0#1549Files changed
OAuthManager.kt—toStatepersistsdPoPEnabled;fromStatetakes aContext,re-enables DPoP on the restored API client, and rebuilds the
DPoPinstance.OAuthManagerState.kt— replaced the non-serializableDPoP?field withdPoPEnabled: Boolean.WebAuthProvider.kt—onRestoreInstanceStateaccepts aContextand forwards itto
fromState. PAR restore path is unchanged.AuthenticationActivity.kt— passes the activityContextintoonRestoreInstanceState.Tests
OAuthManagerStateTest— serialization ofdPoPEnabled(true / default false /legacy JSON missing the field defaults to false, preserving back-compat with
previously persisted state).
WebAuthProviderTest— added a behavioral test that simulates the processdeath → restore path and asserts DPoP is re-enabled on the restored manager.
Testing
Please describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. Since this library has unit testing, tests should be added for new functionality and existing tests should complete without errors.
This change adds unit test coverage
This change adds integration test coverage
This change has been tested on the latest version of the platform/language or why not
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
Bug Fixes
Tests