breaking : Moved the useDPoP method in the WebAuthProvider class to the login builder class
#914
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,10 +27,9 @@ import kotlin.coroutines.resumeWithException | |
| * | ||
| * It uses an external browser by sending the [android.content.Intent.ACTION_VIEW] intent. | ||
| */ | ||
| public object WebAuthProvider { | ||
| private val TAG: String? = WebAuthProvider::class.simpleName | ||
| private const val KEY_BUNDLE_OAUTH_MANAGER_STATE = "oauth_manager_state" | ||
|
|
||
| private val callbacks = CopyOnWriteArraySet<Callback<Credentials, AuthenticationException>>() | ||
|
|
||
|
|
@@ -49,12 +48,6 @@ public object WebAuthProvider : SenderConstraining<WebAuthProvider> { | |
| callbacks -= callback | ||
| } | ||
|
|
||
| /** | ||
| * Initialize the WebAuthProvider instance for logging out the user using an account. Additional settings can be configured | ||
| * in the LogoutBuilder, like changing the scheme of the return to URL. | ||
|
|
@@ -305,14 +298,15 @@ public object WebAuthProvider : SenderConstraining<WebAuthProvider> { | |
| } | ||
| } | ||
|
|
||
| public class Builder internal constructor(private val account: Auth0) : SenderConstraining<Builder> { | ||
| private val values: MutableMap<String, String> = mutableMapOf() | ||
| private val headers: MutableMap<String, String> = mutableMapOf() | ||
| private var pkce: PKCE? = null | ||
| private var issuer: String? = null | ||
| private var scheme: String = "https" | ||
| private var redirectUri: String? = null | ||
| private var invitationUrl: String? = null | ||
| private var dPoP: DPoP? = null | ||
| private var ctOptions: CustomTabsOptions = CustomTabsOptions.newBuilder().build() | ||
| private var leeway: Int? = null | ||
| private var launchAsTwa: Boolean = false | ||
|
|
@@ -548,6 +542,18 @@ public object WebAuthProvider : SenderConstraining<WebAuthProvider> { | |
| return this | ||
| } | ||
|
|
||
| /** | ||
| * Enable DPoP (Demonstrating Proof-of-Possession) for this authentication request. | ||
| * DPoP binds access tokens to the client's cryptographic key, providing enhanced security. | ||
| * | ||
| * @param context the Android context used to access the keystore for DPoP key management | ||
| * @return the current builder instance | ||
| */ | ||
| public override fun useDPoP(context: Context): Builder { | ||
|
Contributor
There was a problem hiding this comment. Now that DPoP is scoped to the Builder (no longer on the singleton), the DPoP state won't survive process death. OAuthManager.toState() doesn't persist DPoP, and fromState() restores without it — so a DPoP-enabled login will silently resume without DPoP proofs if the OS kills the activity mid-redirect. Please add a dpoPEnabled: Boolean flag to OAuthManagerState, persist it in toState(), and reconstruct DPoP(context) in fromState() when the flag is true. No need to serialize the DPoP instance itself (it holds a Context). Please: 1>Add dpoPEnabled: Boolean = false to OAuthManagerState |
||
| dPoP = DPoP(context) | ||
| return this | ||
| } | ||
|
Comment on lines
+553
to
+556
There was a problem hiding this comment. Because DPoP is now configured per login Builder instance, make sure the authentication flow still survives process death/restoration: WebAuthProvider saves/restores OAuthManager via OAuthManager.toState()/fromState(), but OAuthManager.toState() does not currently persist DPoP and OAuthManagerState/PKCE reconstruction doesn’t re-enable DPoP on the restored AuthenticationAPIClient. This can cause a DPoP-enabled login to resume without DPoP proofs after restore. Consider persisting only a lightweight “DPoP enabled” flag in state and reconstructing/configuring DPoP (without serializing a Context-bearing DPoP instance) when restoring. |
||
|
|
||
| /** | ||
| * Request user Authentication. The result will be received in the callback. | ||
| * An error is raised if there are no browser applications installed in the device, or if | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -331,8 +331,8 @@ public class WebAuthProviderTest { | |
| @Test | ||
| public fun enablingDPoPWillGenerateNewKeyPairIfOneDoesNotExist() { | ||
| `when`(mockKeyStore.hasKeyPair()).thenReturn(false) | ||
| login(account) | ||
| .useDPoP(mockContext) | ||
| .start(activity, callback) | ||
|
Comment on lines
332
to
336
There was a problem hiding this comment. There’s no test asserting the new per-request behavior (i.e., enabling DPoP on one login builder does not affect a subsequent login call that doesn’t call useDPoP). Adding a regression test for the non-sticky behavior would better cover the API change described in the PR motivation. |
||
| verify(mockKeyStore).generateKeyPair(any(), any()) | ||
| } | ||
|
|
@@ -358,8 +358,8 @@ public class WebAuthProviderTest { | |
| `when`(mockKeyStore.hasKeyPair()).thenReturn(true) | ||
| `when`(mockKeyStore.getKeyPair()).thenReturn(Pair(mock(), FakeECPublicKey())) | ||
|
|
||
| login(account) | ||
| .useDPoP(mockContext) | ||
| .start(activity, callback) | ||
|
|
||
| verify(activity).startActivity(intentCaptor.capture()) | ||
|
|
@@ -2741,21 +2741,13 @@ public class WebAuthProviderTest { | |
|
|
||
| //DPoP | ||
|
|
||
| @Test | ||
| public fun shouldPassDPoPInstanceToOAuthManagerWhenDPoPIsEnabled() { | ||
| `when`(mockKeyStore.hasKeyPair()).thenReturn(true) | ||
| `when`(mockKeyStore.getKeyPair()).thenReturn(Pair(mock(), FakeECPublicKey())) | ||
|
|
||
| login(account) | ||
| .useDPoP(mockContext) | ||
| .start(activity, callback) | ||
|
|
||
| val managerInstance = WebAuthProvider.managerInstance as OAuthManager | ||
|
|
@@ -2775,8 +2767,8 @@ public class WebAuthProviderTest { | |
| public fun shouldGenerateKeyPairWhenDPoPIsEnabledAndNoKeyPairExists() { | ||
| `when`(mockKeyStore.hasKeyPair()).thenReturn(false) | ||
|
|
||
| login(account) | ||
| .useDPoP(mockContext) | ||
| .start(activity, callback) | ||
|
|
||
| verify(mockKeyStore).generateKeyPair(any(), any()) | ||
|
|
@@ -2787,8 +2779,8 @@ public class WebAuthProviderTest { | |
| `when`(mockKeyStore.hasKeyPair()).thenReturn(true) | ||
| `when`(mockKeyStore.getKeyPair()).thenReturn(Pair(mock(), FakeECPublicKey())) | ||
|
|
||
| login(account) | ||
| .useDPoP(mockContext) | ||
| .start(activity, callback) | ||
|
|
||
| verify(mockKeyStore, never()).generateKeyPair(any(), any()) | ||
|
|
@@ -2809,8 +2801,8 @@ public class WebAuthProviderTest { | |
| `when`(mockKeyStore.hasKeyPair()).thenReturn(true) | ||
| `when`(mockKeyStore.getKeyPair()).thenReturn(Pair(mock(), FakeECPublicKey())) | ||
|
|
||
| login(account) | ||
| .useDPoP(mockContext) | ||
| .start(activity, callback) | ||
|
|
||
| verify(activity).startActivity(intentCaptor.capture()) | ||
|
|
@@ -2829,8 +2821,8 @@ public class WebAuthProviderTest { | |
| `when`(mockKeyStore.hasKeyPair()).thenReturn(true) | ||
| `when`(mockKeyStore.getKeyPair()).thenReturn(null) | ||
|
|
||
| login(account) | ||
| .useDPoP(mockContext) | ||
| .start(activity, callback) | ||
|
|
||
| verify(activity).startActivity(intentCaptor.capture()) | ||
|
|
@@ -2845,8 +2837,8 @@ public class WebAuthProviderTest { | |
| `when`(mockKeyStore.hasKeyPair()).thenReturn(true) | ||
| `when`(mockKeyStore.getKeyPair()).thenReturn(Pair(mock(), FakeECPublicKey())) | ||
|
|
||
| val builder = login(account) | ||
| .useDPoP(mockContext) | ||
| .withConnection("test-connection") | ||
|
|
||
| builder.start(activity, callback) | ||
|
|
@@ -2861,8 +2853,7 @@ public class WebAuthProviderTest { | |
|
|
||
| @Test | ||
| public fun shouldNotAffectLogoutWhenDPoPIsEnabled() { | ||
|
Contributor
There was a problem hiding this comment. This test name is now misleading — it no longer enables DPoP at all, it just tests default logout. Consider renaming to shouldNotIncludeDPoPParametersInLogoutURI.
Contributor
Author
There was a problem hiding this comment. Good catch. This test is no longer required. Removed it |
||
| logout(account) | ||
| .start(activity, voidCallback) | ||
|
|
||
| verify(activity).startActivity(intentCaptor.capture()) | ||
|
|
@@ -2879,8 +2870,8 @@ public class WebAuthProviderTest { | |
| doThrow(DPoPException.KEY_GENERATION_ERROR) | ||
| .`when`(mockKeyStore).generateKeyPair(any(), any()) | ||
|
|
||
| login(account) | ||
| .useDPoP(mockContext) | ||
| .start(activity, callback) | ||
|
|
||
| // Verify that the authentication fails when DPoP key generation fails | ||
|
|
@@ -2909,8 +2900,8 @@ public class WebAuthProviderTest { | |
| val proxyAccount: Auth0 = Auth0.getInstance(JwtTestUtils.EXPECTED_AUDIENCE, mockAPI.domain) | ||
| proxyAccount.networkingClient = SSLTestUtils.testClient | ||
|
|
||
| login(proxyAccount) | ||
| .useDPoP(mockContext) | ||
| .withPKCE(pkce) | ||
| .start(activity, authCallback) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The text says the
useDPoP()method moved, but the API requires a Context parameter (useDPoP(context)/useDPoP(context: Context)). Updating the inline code reference would avoid implying a zero-arg overload still exists.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: should be
useDPoP(context: Context)to match the actual signature and avoid implying a zero-arg overload exists.