diff --git a/libs/SalesforceSDK/src/com/salesforce/androidsdk/auth/dpop/DPoPKeyManager.kt b/libs/SalesforceSDK/src/com/salesforce/androidsdk/auth/dpop/DPoPKeyManager.kt index 6a1898dd11..7265573609 100644 --- a/libs/SalesforceSDK/src/com/salesforce/androidsdk/auth/dpop/DPoPKeyManager.kt +++ b/libs/SalesforceSDK/src/com/salesforce/androidsdk/auth/dpop/DPoPKeyManager.kt @@ -55,6 +55,17 @@ object DPoPKeyManager { fun isDPoPTokenType(tokenType: String?): Boolean = tokenType?.equals(DPOP_TOKEN_TYPE, ignoreCase = true) == true + /** + * Returns whether the persisted fields required to use [tokenType] are internally + * consistent. A DPoP-bound credential must identify the key pair used for its proof; + * Bearer and transitional credentials do not require that identifier. + */ + @JvmStatic + fun hasCompleteDPoPCredentials( + credentialsIdentifier: String?, + tokenType: String? + ): Boolean = !isDPoPTokenType(tokenType) || !credentialsIdentifier.isNullOrBlank() + private const val TAG = "DPoPKeyManager" private val keyPairCache = ConcurrentHashMap() private val keyStoreLock = Any() diff --git a/libs/SalesforceSDK/src/com/salesforce/androidsdk/auth/dpop/DPoPRequestDecorator.kt b/libs/SalesforceSDK/src/com/salesforce/androidsdk/auth/dpop/DPoPRequestDecorator.kt index 4d97ffaea1..4eeab5006b 100644 --- a/libs/SalesforceSDK/src/com/salesforce/androidsdk/auth/dpop/DPoPRequestDecorator.kt +++ b/libs/SalesforceSDK/src/com/salesforce/androidsdk/auth/dpop/DPoPRequestDecorator.kt @@ -32,6 +32,7 @@ import com.salesforce.androidsdk.auth.OAuth2 import com.salesforce.androidsdk.util.SalesforceSDKLogger import okhttp3.Request import okhttp3.Response +import java.io.IOException /** * Public convenience API for app developers stamping DPoP/Bearer authorization @@ -51,17 +52,22 @@ object DPoPRequestDecorator { private const val NONCE_ERROR_VALUE = "use_dpop_nonce" /** - * Stamps the complete [Authorization] and optional [DPoP] proof header set on [builder]. - * No-op when [UserAccount.getAuthToken] is null or empty. + * Stamps [Authorization] and, if the account is DPoP-bound, a [DPoP] proof header + * on [builder]. No-op when [UserAccount.getAuthToken] is null or empty. Rejects an + * incomplete DPoP credential with [IOException] before mutating [builder]. */ fun applyAuthHeaders(builder: Request.Builder, userAccount: UserAccount) { + val tokenType = userAccount.tokenType + val credentialsIdentifier = userAccount.credentialsIdentifier + requireCompleteDPoPCredentials(credentialsIdentifier, tokenType) + val authToken = userAccount.authToken ?: return if (authToken.isEmpty()) return applyAuthHeaders( builder, - userAccount.credentialsIdentifier, - userAccount.tokenType, + credentialsIdentifier, + tokenType, authToken, userAccount.uiSid, ) @@ -80,6 +86,7 @@ object DPoPRequestDecorator { authToken: String?, uiSid: String?, ) { + requireCompleteDPoPCredentials(credentialsIdentifier, tokenType) if (authToken.isNullOrEmpty()) return val requestPath = builder.build().url.encodedPath @@ -139,7 +146,8 @@ object DPoPRequestDecorator { /** * Package-private helper used by RestClient.OAuthRefreshInterceptor to delegate - * the proof-building step without constructing a UserAccount. + * the proof-building step without constructing a UserAccount. An incomplete DPoP + * credential throws [IOException] so OkHttp reports it through normal request failure. */ @JvmName("attachProof") internal fun attachProof( @@ -148,6 +156,7 @@ object DPoPRequestDecorator { tokenType: String?, authToken: String? ) { + requireCompleteDPoPCredentials(credentialsIdentifier, tokenType) if (!DPoPKeyManager.shouldAttachDPoP(credentialsIdentifier, tokenType)) return try { val request = builder.build() @@ -164,4 +173,17 @@ object DPoPRequestDecorator { SalesforceSDKLogger.e(TAG, "Failed to attach DPoP proof", e) } } + + private fun requireCompleteDPoPCredentials( + credentialsIdentifier: String?, + tokenType: String? + ) { + if (!DPoPKeyManager.hasCompleteDPoPCredentials(credentialsIdentifier, tokenType)) { + throw IncompleteDPoPCredentialsException() + } + } + + private class IncompleteDPoPCredentialsException : IOException( + "DPoP credentials require a non-blank credentials identifier" + ) } diff --git a/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManager.java b/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManager.java index 62e14aec9a..daad89b2d1 100644 --- a/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManager.java +++ b/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManager.java @@ -54,6 +54,7 @@ import com.salesforce.androidsdk.auth.OAuth2.OAuthFailedException; import com.salesforce.androidsdk.auth.OAuth2.TokenEndpointResponse; import com.salesforce.androidsdk.auth.OAuth2.TokenErrorResponse; +import com.salesforce.androidsdk.auth.dpop.DPoPKeyManager; import com.salesforce.androidsdk.rest.RestClient.ClientInfo; import com.salesforce.androidsdk.util.SalesforceSDKLogger; @@ -242,7 +243,9 @@ private UserAccount validateUser(boolean requireRefreshFields, @Nullable UserAcc } if (user == null || isMissing(user.getUserId()) - || isMissing(user.getOrgId())) { + || isMissing(user.getOrgId()) + || !DPoPKeyManager.hasCompleteDPoPCredentials( + user.getCredentialsIdentifier(), user.getTokenType())) { return null; } if (requireRefreshFields && (isMissing(user.getRefreshTokenForPersistence()) diff --git a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/app/SalesforceSDKManagerClientManagerTest.kt b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/app/SalesforceSDKManagerClientManagerTest.kt index 30f467fb94..3dde07355b 100644 --- a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/app/SalesforceSDKManagerClientManagerTest.kt +++ b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/app/SalesforceSDKManagerClientManagerTest.kt @@ -597,6 +597,28 @@ class SalesforceSDKManagerClientManagerTest { } } + @Test + fun getRestClient_withDPoPAccountMissingCredentialsIdentifier_removesItWithoutCallback() { + val corruptUser = UserAccountBuilder.getInstance() + .populateFromUserAccount(buildUser("corrupt-dpop")) + .tokenType("dPoP") + .allowUnset(true) + .credentialsIdentifier(null) + .build() + userAccountManager.createAccount(corruptUser) + val account = requireNotNull(userAccountManager.buildAccount(corruptUser)) + val activity = mockk(relaxed = true) + var callbackCount = 0 + + sdkManager.getRestClient(activity) { callbackCount++ } + + assertEquals(0, callbackCount) + assertFalse(accountManager.getAccountsByType(account.type).contains(account)) + verify(exactly = 0) { + activity.startActivityForResult(any(), any()) + } + } + @Test fun getRestClient_resolvesCurrentAccountExactlyOnce() { /* diff --git a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/auth/dpop/DPoPKeyManagerTest.kt b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/auth/dpop/DPoPKeyManagerTest.kt index e90ac321a0..4bc2f40685 100644 --- a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/auth/dpop/DPoPKeyManagerTest.kt +++ b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/auth/dpop/DPoPKeyManagerTest.kt @@ -272,6 +272,18 @@ class DPoPKeyManagerTest { assertFalse(DPoPKeyManager.isDPoPTokenType("bearer")) } + @Test + fun test_hasCompleteDPoPCredentials_requiresNonBlankIdentifierOnlyForDPoP() { + assertTrue(DPoPKeyManager.hasCompleteDPoPCredentials("credential-id", "DPoP")) + assertTrue(DPoPKeyManager.hasCompleteDPoPCredentials("credential-id", "dPoP")) + assertFalse(DPoPKeyManager.hasCompleteDPoPCredentials(null, "DPoP")) + assertFalse(DPoPKeyManager.hasCompleteDPoPCredentials("", "dpop")) + assertFalse(DPoPKeyManager.hasCompleteDPoPCredentials(" ", "DPOP")) + + assertTrue(DPoPKeyManager.hasCompleteDPoPCredentials(null, "Bearer")) + assertTrue(DPoPKeyManager.hasCompleteDPoPCredentials("", null)) + } + @Test fun test_shouldAttachDPoP_lowercaseDPoPTokenType_returnsTrue() { // Regression for W-24027018: server returns lowercase "dpop" in refresh responses. diff --git a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/auth/dpop/DPoPRequestDecoratorTest.kt b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/auth/dpop/DPoPRequestDecoratorTest.kt index c2c584b378..97eea51d5b 100644 --- a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/auth/dpop/DPoPRequestDecoratorTest.kt +++ b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/auth/dpop/DPoPRequestDecoratorTest.kt @@ -39,9 +39,11 @@ import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertNotNull import org.junit.Assert.assertNull +import org.junit.Assert.assertThrows import org.junit.Assert.assertTrue import org.junit.Test import org.junit.runner.RunWith +import java.io.IOException /** * Unit tests for [DPoPRequestDecorator], the public convenience API for stamping @@ -139,6 +141,44 @@ class DPoPRequestDecoratorTest { assertNull(request.header(DPoPRequestDecorator.DPOP_HEADER)) } + @Test + fun applyAuthHeaders_dpopAccountWithMissingIdentifier_failsBeforeAddingHeaders() { + listOf(null, "", " ").forEach { identifier -> + val builder = requestBuilder() + + assertThrows(IOException::class.java) { + DPoPRequestDecorator.applyAuthHeaders( + builder, + userAccount( + tokenType = "dPoP", + credentialsIdentifier = identifier, + ), + ) + } + + assertNull(builder.build().header("Authorization")) + assertNull(builder.build().header(DPoPRequestDecorator.DPOP_HEADER)) + } + } + + @Test + fun applyAuthHeaders_dpopAccountWithMissingToken_failsClosed() { + val builder = requestBuilder() + + assertThrows(IOException::class.java) { + DPoPRequestDecorator.applyAuthHeaders( + builder, + userAccount( + authToken = null, + tokenType = "DPoP", + credentialsIdentifier = null, + ), + ) + } + + assertNull(builder.build().header("Authorization")) + } + @Test fun applyAuthHeaders_dpopAccount_isUseDPoPFalse_proofStillAttached() { val sdkManager = com.salesforce.androidsdk.app.SalesforceSDKManager.getInstance() diff --git a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/ClientManagerTest.java b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/ClientManagerTest.java index c9df2c0341..88a7a869d2 100644 --- a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/ClientManagerTest.java +++ b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/ClientManagerTest.java @@ -364,6 +364,50 @@ public void testPeekRestClientWithUserFailsClosedWhenSuppliedUserMissingUserId() Assert.assertNull(clientManager.peekRestClient(incompleteUser)); } + /** Persisted DPoP credentials without a key identifier are rejected as corrupt. */ + @Test + public void testPeekRestClientWithDPoPAndNullCredentialsIdentifierReturnsNull() { + final UserAccount corruptUser = UserAccountBuilder.getInstance() + .populateFromUserAccount(UserAccountTest.createTestAccount()) + .tokenType("DPoP") + .allowUnset(true) + .credentialsIdentifier(null) + .build(); + userAccountManager.createAccount(corruptUser); + clientManager = new ClientManager(targetContext, corruptUser); + + Assert.assertNull(clientManager.peekRestClient()); + } + + /** DPoP matching is case-insensitive and whitespace is not a usable key identifier. */ + @Test + public void testPeekRestClientWithMixedCaseDPoPAndBlankCredentialsIdentifierReturnsNull() { + final UserAccount corruptUser = UserAccountBuilder.getInstance() + .populateFromUserAccount(UserAccountTest.createTestAccount()) + .tokenType("dPoP") + .credentialsIdentifier(" ") + .build(); + userAccountManager.createAccount(corruptUser); + clientManager = new ClientManager(targetContext, corruptUser); + + Assert.assertNull(clientManager.peekRestClient()); + } + + /** Bearer accounts do not require a DPoP credentials identifier. */ + @Test + public void testPeekRestClientWithBearerAndNullCredentialsIdentifierReturnsClient() { + final UserAccount bearerUser = UserAccountBuilder.getInstance() + .populateFromUserAccount(UserAccountTest.createTestAccount()) + .tokenType("Bearer") + .allowUnset(true) + .credentialsIdentifier(null) + .build(); + userAccountManager.createAccount(bearerUser); + clientManager = new ClientManager(targetContext, bearerUser); + + Assert.assertNotNull(clientManager.peekRestClient()); + } + /** * Checks there are no test accounts */ diff --git a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/RestClientDPoPGateTests.kt b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/RestClientDPoPGateTests.kt index 2592e3f9bc..4ea89a5a53 100644 --- a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/RestClientDPoPGateTests.kt +++ b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/RestClientDPoPGateTests.kt @@ -33,6 +33,7 @@ import com.salesforce.androidsdk.auth.dpop.DPoPKeyManager import io.mockk.every import io.mockk.mockk import io.mockk.slot +import io.mockk.verify import okhttp3.Interceptor import okhttp3.Protocol import okhttp3.Request @@ -41,9 +42,11 @@ import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertNotNull import org.junit.Assert.assertNull +import org.junit.Assert.assertThrows import org.junit.Assert.assertTrue import org.junit.Test import org.junit.runner.RunWith +import java.io.IOException import java.net.URI import java.util.UUID @@ -172,23 +175,129 @@ class RestClientDPoPGateTests { ) } - // Null credentialsIdentifier short-circuits — no DPoP header regardless of tokenType. + // An explicit DPoP token type without its key identifier is corrupt. Fail before the + // OkHttp chain can transmit an Authorization header without a proof. @Test - fun test_givenNullCredentialsIdentifier_whenIntercept_thenNoDPoPHeader() { + fun test_givenDPoPWithNullCredentialsIdentifier_whenIntercept_thenFailsBeforeNetwork() { val interceptor = RestClient.OAuthRefreshInterceptor( null, "__ACCESS_TOKEN__", - "DPoP", + "dPoP", null, null, ) + val outbound = Request.Builder() + .url("https://instance.example.com/services/data/v65.0/query") + .get() + .build() + val chain = mockk(relaxed = true) { + every { request() } returns outbound + } - val captured = captureAuthenticatedRequest(interceptor) + assertThrows(IOException::class.java) { + interceptor.intercept(chain) + } - assertNull( - "Did not expect DPoP header when credentialsIdentifier is null", - captured.header("DPoP"), + verify(exactly = 0) { chain.proceed(any()) } + } + + @Test + fun test_givenDPoPWithBlankCredentialsIdentifier_whenIntercept_thenFailsBeforeNetwork() { + val interceptor = RestClient.OAuthRefreshInterceptor( + null, + "__ACCESS_TOKEN__", + "DPoP", + " ", + null, ) + val outbound = Request.Builder() + .url("https://instance.example.com/services/data/v65.0/query") + .get() + .build() + val chain = mockk(relaxed = true) { + every { request() } returns outbound + } + + assertThrows(IOException::class.java) { + interceptor.intercept(chain) + } + + verify(exactly = 0) { chain.proceed(any()) } + } + + @Test + fun test_givenIncompleteDPoPAndUiSidPolicy_whenIntercept_thenFailsBeforeNetwork() { + val sdkManager = SalesforceSDKManager.getInstance() + val originalPolicy = sdkManager.shouldUseUiSidBearerForPath + try { + sdkManager.shouldUseUiSidBearerForPath = { true } + val interceptor = RestClient.OAuthRefreshInterceptor( + null, + "__ACCESS_TOKEN__", + "DPoP", + null, + "__UI_SID__", + null, + ) + val outbound = Request.Builder() + .url("https://instance.example.com/services/session/bootstrap") + .get() + .build() + val chain = mockk(relaxed = true) { + every { request() } returns outbound + } + + assertThrows(IOException::class.java) { + interceptor.intercept(chain) + } + + verify(exactly = 0) { chain.proceed(any()) } + } finally { + sdkManager.shouldUseUiSidBearerForPath = originalPolicy + } + } + + @Test + fun test_givenCachedBearerInterceptorMigratesToIncompleteDPoP_whenReplay_thenReportsIoFailure() { + val provider = object : RestClient.AuthTokenProvider { + override fun getInstanceUrl() = "https://instance.example.com" + override fun getNewAuthToken() = "__DPOP_ACCESS_TOKEN__" + override fun getRefreshToken() = "__REFRESH_TOKEN__" + override fun getLastRefreshTime() = 0L + override fun getTokenType() = "DPoP" + } + val interceptor = RestClient.OAuthRefreshInterceptor( + clientInfo(), + "__BEARER_ACCESS_TOKEN__", + "Bearer", + null, + provider, + ) + val outbound = Request.Builder() + .url("https://instance.example.com/services/data/v65.0/query") + .get() + .build() + val attempts = mutableListOf() + val chain = mockk { + every { request() } returns outbound + every { proceed(any()) } answers { + val attemptedRequest = firstArg() + attempts += attemptedRequest + Response.Builder() + .request(attemptedRequest) + .protocol(Protocol.HTTP_1_1) + .code(401) + .message("Unauthorized") + .build() + } + } + + assertThrows(IOException::class.java) { + interceptor.intercept(chain) + } + + assertTrue(attempts.single().header("Authorization")!!.startsWith("Bearer ")) + verify(exactly = 1) { chain.proceed(any()) } } @Test