fix : Handling DPoP enabled WebAuth flow after process death#977
Open
pmathew92 wants to merge 5 commits into
Open
fix : Handling DPoP enabled WebAuth flow after process death#977pmathew92 wants to merge 5 commits into
pmathew92 wants to merge 5 commits into
Conversation
📝 WalkthroughWalkthroughThis PR persists DPoP enablement as a boolean, updates serialization/deserialization, threads an Android Context through the restore path so OAuthManager.fromState can re-enable DPoP, and adds tests plus a CI action pin update. ChangesDPoP State Persistence Across Process Death
Sequence DiagramsequenceDiagram
participant Activity as AuthenticationActivity
participant WebAuth as WebAuthProvider
participant OAuthMgr as OAuthManager
participant APIClient as AuthenticationAPIClient
participant DPoP as DPoP
Activity->>WebAuth: onRestoreInstanceState(bundle, context)
WebAuth->>OAuthMgr: fromState(state, callback, context)
alt state.dPoPEnabled == true
OAuthMgr->>APIClient: enableDPoP()
OAuthMgr->>DPoP: construct DPoP(context)
end
OAuthMgr->>OAuthMgr: restore PKCE, headers, id-token verification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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.
🧹 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
Contributor
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
utkrishtsahu
reviewed
Jun 8, 2026
utkrishtsahu
reviewed
Jun 8, 2026
| callback = callback, | ||
| customAuthorizeUrl = state.customAuthorizeUrl | ||
| customAuthorizeUrl = state.customAuthorizeUrl, | ||
| dPoP = if (state.dPoPEnabled ) DPoP(context) else null |
Contributor
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.
Contributor
Author
There was a problem hiding this comment.
Yes, the dPoP arg is intentional — for state-consistency on toState(), round-trip + the test assertion.
utkrishtsahu
reviewed
Jun 8, 2026
utkrishtsahu
reviewed
Jun 8, 2026
Contributor
|
@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
requested changes
Jun 8, 2026
Contributor
Author
|
@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
utkrishtsahu
approved these changes
Jun 9, 2026
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
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