Conversation
…CredentialsManager class
…/Auth0.Android into dpop_thumbprint_validation
There was a problem hiding this comment.
Pull request overview
Adds a DPoP state validation gate to credentials refresh flows to prevent calling /oauth/token with stale/misaligned DPoP key material.
Changes:
- Introduces
validateDPoPState(tokenType)inBaseCredentialsManagerto verify DPoP key presence/configuration and thumbprint alignment (with migration/backfill behavior). - Calls DPoP validation before
ssoExchange(...)andrenewAuth(...)in bothCredentialsManagerandSecureCredentialsManager. - Captures
APICredentials.typewhen reading cached API credentials to drive validation during API-credentials refresh.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| auth0/src/main/java/com/auth0/android/authentication/storage/BaseCredentialsManager.kt | Adds centralized DPoP validation (key exists, client configured, thumbprint matches/backfills). |
| auth0/src/main/java/com/auth0/android/authentication/storage/CredentialsManager.kt | Runs DPoP validation before token refresh/SSO exchange; threads API-credential token type into validation. |
| auth0/src/main/java/com/auth0/android/authentication/storage/SecureCredentialsManager.kt | Runs DPoP validation before token refresh/SSO exchange; threads decrypted API-credential token type into validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| val currentThumbprint = try { | ||
| DPoPUtil.getPublicKeyJWK() | ||
| } catch (e: DPoPException) { | ||
| Log.e(this::class.java.simpleName, "Failed to read DPoP key thumbprint", e) | ||
| null | ||
| } |
There was a problem hiding this comment.
validateDPoPState only catches DPoPException around DPoPUtil.getPublicKeyJWK(), but KeyStore.getKey(...) can also throw other runtime/checked exceptions (e.g., UnrecoverableKeyException, NoSuchAlgorithmException, ClassCastException) via DPoPKeyStore.getKeyPair(). As written, those would bubble up during token refresh and can crash the app. Consider broadening the catch to Exception (or Throwable if you want to be extra defensive) and returning a CredentialsManagerException instead of throwing.
There was a problem hiding this comment.
This will be fixed as a separate PR at the root level
| if (storedThumbprint != null) { | ||
| if (currentThumbprint != storedThumbprint) { | ||
| Log.w(this::class.java.simpleName, "DPoP key thumbprint mismatch. Clearing stale credentials.") | ||
| clearCredentials() | ||
| return CredentialsManagerException(CredentialsManagerException.Code.DPOP_KEY_MISSING) | ||
| } |
There was a problem hiding this comment.
When storedThumbprint is set, a failure to read currentThumbprint (resulting in null) will be treated as a thumbprint mismatch, which clears credentials and returns DPOP_KEY_MISSING. This can log users out on transient/unknown KeyStore read issues even though the key alias exists (you already checked hasKeyPair()). Consider handling currentThumbprint == null as an indeterminate state (e.g., return an error without clearing, or retry) and only clearing when you have a non-null thumbprint that actually differs.
There was a problem hiding this comment.
This is intentional. hasKeyPair only checks if the KeyAlias exists or not. getPublicKeyJWK() actually reads the key and extracts the public key. If the alias exists but the key can't be read, the key is effectively unusable — any DPoP proof generation will also fail. So throwing error at this point is the correct approach
| protected fun validateDPoPState(tokenType: String?): CredentialsManagerException? { | ||
| val storedThumbprint = storage.retrieveString(KEY_DPOP_THUMBPRINT) | ||
| val isDPoPBound = (tokenType?.equals("DPoP", ignoreCase = true) == true) | ||
| || (storedThumbprint != null) | ||
| if (!isDPoPBound) return null |
There was a problem hiding this comment.
This change introduces new DPoP validation behavior that can fail early (and in some cases clear stored credentials) during renewAuth(...) / ssoExchange(...). There are existing unit tests for both CredentialsManager and SecureCredentialsManager, but none appear to cover DPoP scenarios; please add tests that exercise the new paths (key missing, thumbprint mismatch, and client not configured) and assert both the returned CredentialsManagerException code and whether credentials are cleared/backfilled as intended.
There was a problem hiding this comment.
Tests will be added later
Changes
This PR adds the DPoP validation check before making call to the
oauth/tokenendpointChecklist
I have read the Auth0 general contribution guidelines
I have read the Auth0 Code of Conduct
All existing and new tests complete without errors