From 3b2139e7544ebe9382b50c6c53e273f2695eab81 Mon Sep 17 00:00:00 2001 From: p3dr0rv Date: Mon, 28 Apr 2025 12:53:27 -0700 Subject: [PATCH 1/4] DUNA add sate validation --- .../internal/AuthenticationConstants.java | 4 + .../oauth2/WebViewAuthorizationFragment.java | 1 + .../AzureActiveDirectoryWebViewClient.java | 2 +- .../SwitchBrowserChallenge.kt | 10 +- .../SwitchBrowserRequestHandler.kt | 6 +- .../SwitchBrowserProtocolCoordinator.kt | 21 +++- .../switchbrowser/SwitchBrowserUriHelper.kt | 103 +++++++++++++----- .../SwitchBrowserRequestHandlerTest.kt | 30 ++++- .../SwitchBrowserUriHelperTest.kt | 35 +++++- .../SwitchBrowserProtocolCoordinatorTest.kt | 8 +- 10 files changed, 172 insertions(+), 48 deletions(-) diff --git a/common/src/main/java/com/microsoft/identity/common/adal/internal/AuthenticationConstants.java b/common/src/main/java/com/microsoft/identity/common/adal/internal/AuthenticationConstants.java index 9c3ba74a8e..c96b0d5e39 100644 --- a/common/src/main/java/com/microsoft/identity/common/adal/internal/AuthenticationConstants.java +++ b/common/src/main/java/com/microsoft/identity/common/adal/internal/AuthenticationConstants.java @@ -560,6 +560,10 @@ public static final class SWITCH_BROWSER { */ public static final String ACTION_URI = "action_uri"; + /** + * String Query parameter key for the state blob. + */ + public static final String STATE = "state"; } /** diff --git a/common/src/main/java/com/microsoft/identity/common/internal/providers/oauth2/WebViewAuthorizationFragment.java b/common/src/main/java/com/microsoft/identity/common/internal/providers/oauth2/WebViewAuthorizationFragment.java index 6589917b89..ee44184134 100644 --- a/common/src/main/java/com/microsoft/identity/common/internal/providers/oauth2/WebViewAuthorizationFragment.java +++ b/common/src/main/java/com/microsoft/identity/common/internal/providers/oauth2/WebViewAuthorizationFragment.java @@ -184,6 +184,7 @@ private void resumeSwitchBrowser(@NonNull final Bundle extras) { try { Logger.info(methodTag, "Resuming switch browser flow"); getSwitchBrowserCoordinator().processSwitchBrowserResume( + mAuthorizationRequestUrl, extras, (switchBrowserResumeUri, switchBrowserResumeHeaders) -> { launchWebView(switchBrowserResumeUri.toString(), switchBrowserResumeHeaders); diff --git a/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/AzureActiveDirectoryWebViewClient.java b/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/AzureActiveDirectoryWebViewClient.java index d21b37d84c..2db5879bcc 100644 --- a/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/AzureActiveDirectoryWebViewClient.java +++ b/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/AzureActiveDirectoryWebViewClient.java @@ -390,7 +390,7 @@ private void processSwitchBrowserRequest(@NonNull final String url) { final String methodTag = TAG + ":processSwitchBrowserRequest"; try { mSwitchBrowserRequestHandler.processChallenge( - SwitchBrowserChallenge.constructFromRedirectUrl(url) + SwitchBrowserChallenge.constructFromRedirectUrl(url, mRequestUrl) ); } catch (final Throwable throwable) { Logger.error(methodTag, "Switch browser challenge could not be processed.", throwable); diff --git a/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/challengehandlers/SwitchBrowserChallenge.kt b/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/challengehandlers/SwitchBrowserChallenge.kt index 02fe763533..1342bf49c1 100644 --- a/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/challengehandlers/SwitchBrowserChallenge.kt +++ b/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/challengehandlers/SwitchBrowserChallenge.kt @@ -30,7 +30,8 @@ import com.microsoft.identity.common.internal.ui.webview.switchbrowser.SwitchBro * It contains the URI to be opened in the new browser. */ data class SwitchBrowserChallenge( - val uri: Uri, + val processUri: Uri, + val authorizationUrl: String ) { companion object { @@ -46,10 +47,13 @@ data class SwitchBrowserChallenge( */ @JvmStatic @Throws(Exception::class) - fun constructFromRedirectUrl(redirectUrl: String): SwitchBrowserChallenge { + fun constructFromRedirectUrl(redirectUrl: String, authorizationUrl: String): SwitchBrowserChallenge { Uri.parse(redirectUrl).let { redirectUri -> SwitchBrowserUriHelper.buildProcessUri(redirectUri).let { processUri -> - return SwitchBrowserChallenge(uri = processUri) + return SwitchBrowserChallenge( + processUri = processUri, + authorizationUrl = authorizationUrl + ) } } } diff --git a/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/challengehandlers/SwitchBrowserRequestHandler.kt b/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/challengehandlers/SwitchBrowserRequestHandler.kt index 8fc7cbc4a4..df6f78268a 100644 --- a/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/challengehandlers/SwitchBrowserRequestHandler.kt +++ b/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/challengehandlers/SwitchBrowserRequestHandler.kt @@ -28,6 +28,7 @@ import android.content.Intent import com.microsoft.identity.common.adal.internal.AuthenticationConstants.SWITCH_BROWSER import com.microsoft.identity.common.internal.ui.browser.AndroidBrowserSelector import com.microsoft.identity.common.internal.ui.browser.CustomTabsManager +import com.microsoft.identity.common.internal.ui.webview.switchbrowser.SwitchBrowserUriHelper import com.microsoft.identity.common.internal.ui.webview.switchbrowser.SwitchBrowserUriHelper.isSwitchBrowserRedirectUrl import com.microsoft.identity.common.java.browser.IBrowserSelector import com.microsoft.identity.common.java.exception.ClientException @@ -83,6 +84,9 @@ class SwitchBrowserRequestHandler( override fun processChallenge(switchBrowserChallenge: SwitchBrowserChallenge) { SpanExtension.makeCurrentSpan(span).use { val methodTag = "$TAG:processChallenge" + + val state = switchBrowserChallenge.processUri.getQueryParameter(SWITCH_BROWSER.STATE) + SwitchBrowserUriHelper.statesMatch(switchBrowserChallenge.authorizationUrl, state) // Select a browser to handle the switch browser challenge val browser = browserSelector.selectBrowser( @@ -125,7 +129,7 @@ class SwitchBrowserRequestHandler( "Launching switch browser request on browser: ${browser.packageName}" ) browserIntent.setPackage(browser.packageName) - browserIntent.setData(switchBrowserChallenge.uri) + browserIntent.setData(switchBrowserChallenge.processUri) activity.startActivity(browserIntent) isChallengeHandled = true span.setAttribute( diff --git a/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserProtocolCoordinator.kt b/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserProtocolCoordinator.kt index ca825012a3..3372909102 100644 --- a/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserProtocolCoordinator.kt +++ b/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserProtocolCoordinator.kt @@ -31,14 +31,13 @@ import com.microsoft.identity.common.adal.internal.AuthenticationConstants.Autho import com.microsoft.identity.common.adal.internal.AuthenticationConstants.SWITCH_BROWSER import com.microsoft.identity.common.internal.providers.oauth2.BrokerAuthorizationActivity import com.microsoft.identity.common.internal.ui.webview.challengehandlers.SwitchBrowserRequestHandler -import com.microsoft.identity.common.internal.ui.webview.switchbrowser.SwitchBrowserUriHelper.buildResumeUri -import com.microsoft.identity.common.internal.ui.webview.switchbrowser.SwitchBrowserUriHelper.isSwitchBrowserRedirectUrl import com.microsoft.identity.common.java.AuthenticationConstants.AAD.AUTHORIZATION import com.microsoft.identity.common.java.exception.ClientException import com.microsoft.identity.common.java.opentelemetry.AttributeName import com.microsoft.identity.common.java.opentelemetry.OTelUtility import com.microsoft.identity.common.java.opentelemetry.SpanExtension import com.microsoft.identity.common.java.opentelemetry.SpanName +import com.microsoft.identity.common.java.providers.oauth2.AuthorizationRequest import com.microsoft.identity.common.java.ui.AuthorizationAgent import com.microsoft.identity.common.logging.Logger import io.opentelemetry.api.trace.Span @@ -69,7 +68,7 @@ class SwitchBrowserProtocolCoordinator( * Returns `true` if the URL matches the expected pattern, `false` otherwise. */ fun isSwitchBrowserResume(url: String?, redirectUrl: String): Boolean { - return isSwitchBrowserRedirectUrl(url, redirectUrl, SWITCH_BROWSER.RESUME_PATH) + return SwitchBrowserUriHelper.isSwitchBrowserRedirectUrl(url, redirectUrl, SWITCH_BROWSER.RESUME_PATH) } /** @@ -94,6 +93,10 @@ class SwitchBrowserProtocolCoordinator( SWITCH_BROWSER.CODE, uri.getQueryParameter(SWITCH_BROWSER.CODE) ) + intent.putExtra( + SWITCH_BROWSER.STATE, + uri.getQueryParameter(SWITCH_BROWSER.STATE) + ) return intent } } @@ -109,6 +112,7 @@ class SwitchBrowserProtocolCoordinator( */ @Throws(ClientException::class) fun processSwitchBrowserResume( + authorizationRequest: String, extras: Bundle, onSuccessAction: (Uri, HashMap) -> Unit ) { @@ -116,17 +120,22 @@ class SwitchBrowserProtocolCoordinator( val methodTag = "$TAG:processSwitchBrowserResume" val actionUri = extras.getString(SWITCH_BROWSER.ACTION_URI) val code = extras.getString(SWITCH_BROWSER.CODE) - if (actionUri.isNullOrEmpty() || code.isNullOrEmpty()) { + val state = extras.getString(SWITCH_BROWSER.STATE) + if (actionUri.isNullOrEmpty() || code.isNullOrEmpty() || state.isNullOrEmpty()) { val clientException = ClientException( ClientException.MISSING_PARAMETER, - "Action URI is null/empty: ${actionUri == null}, code is null/empty: ${code == null}" + "Action URI is null/empty: ${actionUri.isNullOrEmpty()}," + + " code is null/empty: ${code.isNullOrEmpty()}," + + " state is null/empty: ${state.isNullOrEmpty()}" ) span.setStatus(StatusCode.ERROR) span.recordException(clientException) span.end() throw clientException } - val resumeUri = buildResumeUri(actionUri) + SwitchBrowserUriHelper.statesMatch(authorizationRequest, state) + // Validate the state from auth request and redirect URL is the same + val resumeUri = SwitchBrowserUriHelper.buildResumeUri(actionUri, state) val headers = hashMapOf(AUTHORIZATION to code) onSuccessAction(resumeUri, headers) // Reset the challenge state after processing the resume action diff --git a/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserUriHelper.kt b/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserUriHelper.kt index 6084669b9f..11b2bb91a7 100644 --- a/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserUriHelper.kt +++ b/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserUriHelper.kt @@ -24,9 +24,10 @@ package com.microsoft.identity.common.internal.ui.webview.switchbrowser import android.net.Uri import com.microsoft.identity.common.adal.internal.AuthenticationConstants.SWITCH_BROWSER -import com.microsoft.identity.common.java.AuthenticationConstants.OAuth2 import com.microsoft.identity.common.java.exception.ClientException +import com.microsoft.identity.common.java.opentelemetry.SpanExtension import com.microsoft.identity.common.logging.Logger +import io.opentelemetry.api.trace.StatusCode /** * SwitchBrowserUriHelper is a helper class to build URIs for the switch browser challenge. @@ -69,51 +70,37 @@ object SwitchBrowserUriHelper { Logger.error(methodTag, errorMessage, exception) throw exception } + val state = uri.getQueryParameter( + SWITCH_BROWSER.STATE + ) + if (state.isNullOrEmpty()) { + // This should never happen, but if it does, we should log it and throw. + val errorMessage = "switch browser action state is null or empty" + val exception = ClientException(ClientException.MALFORMED_URL, errorMessage) + Logger.error(methodTag, errorMessage, exception) + throw exception + } // Query parameters for the process uri. val queryParams = hashMapOf() queryParams[SWITCH_BROWSER.CODE] = code + queryParams[SWITCH_BROWSER.STATE] = state // Construct the uri to the process endpoint. return buildSwitchBrowserUri(actionUri, queryParams) } - /** - * Build a generic switch browser uri. - * - * @param actionUri The action uri to be opened. - * @param queryParams The query parameters to be included in the switch browser uri. - * - * @return The switch browser uri constructed from the action uri and query parameters. - */ - @Throws(IllegalArgumentException::class, NullPointerException::class, UnsupportedOperationException::class) - private fun buildSwitchBrowserUri( - actionUri: String, - queryParams: HashMap = hashMapOf() - ): Uri { - val paths = actionUri.split("/") - val authority = paths[0] - val uriBuilder = Uri.Builder() - .scheme("https") - .encodedAuthority(authority) - for (i in 1 until paths.size) { - uriBuilder.appendPath(paths[i]) - } - for ((key, value) in queryParams.entries) { - uriBuilder.appendQueryParameter(key, value) - } - return uriBuilder.build() - } - /** * Build the resume uri for the switch browser challenge. * * @param actionUri The action uri to be opened. + * @param state The state to be included in the switch browser uri. * * @return The resume uri constructed from the bundle. * e.g. actionUri */ - fun buildResumeUri(actionUri: String): Uri { + fun buildResumeUri(actionUri: String, state: String): Uri { // Construct the uri to the resume endpoint. - return buildSwitchBrowserUri(actionUri) + val queryParams = hashMapOf(SWITCH_BROWSER.STATE to state) + return buildSwitchBrowserUri(actionUri, queryParams) } /** @@ -134,4 +121,60 @@ object SwitchBrowserUriHelper { val expectedUrl = "$redirectUrl/$switchBrowserPath" return url.startsWith(expectedUrl, ignoreCase = true) } + + /** + * Check if state in the auth request matches the state provided. + */ + fun statesMatch(authorizationUrl: String, state: String?) { + val span = SpanExtension.current() + // Validate the state from auth request and redirect URL is the same + if (state.isNullOrEmpty()) { + val clientException = ClientException( + ClientException.STATE_MISMATCH, + "State is null." + ) + span.setStatus(StatusCode.ERROR) + span.recordException(clientException) + span.end() + throw clientException + } + val authRequestState = Uri.parse(authorizationUrl).getQueryParameter(SWITCH_BROWSER.STATE) + if (state != authRequestState) { + val clientException = ClientException( + ClientException.STATE_MISMATCH, + "State does not match with the auth request state." + ) + span.setStatus(StatusCode.ERROR) + span.recordException(clientException) + span.end() + throw clientException + } + } + + /** + * Build a generic switch browser uri. + * + * @param actionUri The action uri to be opened. + * @param queryParams The query parameters to be included in the switch browser uri. + * + * @return The switch browser uri constructed from the action uri and query parameters. + */ + @Throws(IllegalArgumentException::class, NullPointerException::class, UnsupportedOperationException::class) + private fun buildSwitchBrowserUri( + actionUri: String, + queryParams: HashMap = hashMapOf() + ): Uri { + val paths = actionUri.split("/") + val authority = paths[0] + val uriBuilder = Uri.Builder() + .scheme("https") + .encodedAuthority(authority) + for (i in 1 until paths.size) { + uriBuilder.appendPath(paths[i]) + } + for ((key, value) in queryParams.entries) { + uriBuilder.appendQueryParameter(key, value) + } + return uriBuilder.build() + } } diff --git a/common/src/test/java/com/microsoft/identity/common/internal/ui/webview/challengehandlers/SwitchBrowserRequestHandlerTest.kt b/common/src/test/java/com/microsoft/identity/common/internal/ui/webview/challengehandlers/SwitchBrowserRequestHandlerTest.kt index 5c147a71ac..59b45a5103 100644 --- a/common/src/test/java/com/microsoft/identity/common/internal/ui/webview/challengehandlers/SwitchBrowserRequestHandlerTest.kt +++ b/common/src/test/java/com/microsoft/identity/common/internal/ui/webview/challengehandlers/SwitchBrowserRequestHandlerTest.kt @@ -57,7 +57,8 @@ class SwitchBrowserRequestHandlerTest { val context = mock(Context::class.java) val customTabsManager = mock(CustomTabsManager::class.java) val challenge = mock(SwitchBrowserChallenge::class.java) - `when`(challenge.uri).thenReturn(Uri.parse("https://example.com")) + `when`(challenge.processUri).thenReturn(Uri.parse("https://example.com?state=123")) + `when`(challenge.authorizationUrl).thenReturn("https://auth.com?state=123") val browserSelector = // Browser available IBrowserSelector { _, _ -> Browser("fakeBrowser", emptySet(), "browser", false) } val handler = SwitchBrowserRequestHandler(mockActivity, context, customTabsManager, browserSelector, null) @@ -73,7 +74,8 @@ class SwitchBrowserRequestHandlerTest { val context = mock(Context::class.java) val customTabsManager = mock(CustomTabsManager::class.java) val challenge = mock(SwitchBrowserChallenge::class.java) - `when`(challenge.uri).thenReturn(Uri.parse("https://example.com")) + `when`(challenge.processUri).thenReturn(Uri.parse("https://example.com?state=123")) + `when`(challenge.authorizationUrl).thenReturn("https://auth.com?state=123") val browserSelector = IBrowserSelector { _, _ -> null } // No browser available val handler = SwitchBrowserRequestHandler(activity, context, customTabsManager, browserSelector, null) val exception = Assert.assertThrows(ClientException::class.java) { @@ -82,4 +84,28 @@ class SwitchBrowserRequestHandlerTest { Assert.assertEquals(ClientException.NO_BROWSERS_AVAILABLE, exception.errorCode) Assert.assertEquals("No browser found for SwitchBrowserChallenge.", exception.message) } + + @Test + fun `test processChallenge states mismatch`() { + // Mock parameters + val mockActivity = mock() + var activityExecuted = false + doAnswer { + activityExecuted = true + null + }.whenever(mockActivity).startActivity(any()) + val context = mock(Context::class.java) + val customTabsManager = mock(CustomTabsManager::class.java) + val challenge = mock(SwitchBrowserChallenge::class.java) + `when`(challenge.processUri).thenReturn(Uri.parse("https://example.com?state=123")) + `when`(challenge.authorizationUrl).thenReturn("https://auth.com?state=456") + val browserSelector = // Browser available + IBrowserSelector { _, _ -> Browser("fakeBrowser", emptySet(), "browser", false) } + val handler = SwitchBrowserRequestHandler(mockActivity, context, customTabsManager, browserSelector, null) + val exception = Assert.assertThrows(ClientException::class.java) { + handler.processChallenge(challenge) + } + Assert.assertEquals(ClientException.STATE_MISMATCH, exception.errorCode) + Assert.assertEquals("State does not match with the auth request state.", exception.message) + } } diff --git a/common/src/test/java/com/microsoft/identity/common/internal/ui/webview/challengehandlers/SwitchBrowserUriHelperTest.kt b/common/src/test/java/com/microsoft/identity/common/internal/ui/webview/challengehandlers/SwitchBrowserUriHelperTest.kt index 7c2ff89c02..c65aba2abc 100644 --- a/common/src/test/java/com/microsoft/identity/common/internal/ui/webview/challengehandlers/SwitchBrowserUriHelperTest.kt +++ b/common/src/test/java/com/microsoft/identity/common/internal/ui/webview/challengehandlers/SwitchBrowserUriHelperTest.kt @@ -38,13 +38,15 @@ class SwitchBrowserUriHelperTest { companion object { private const val CODE = "your-switch-browser-code" private const val ACTION_URI = "login.microsoftonline.com/switchbrowser/process" + private const val STATE = "123" } @Test fun `test constructFromRedirectUri with valid redirect uri`() { val redirectString = "${Broker.NEW_BROKER_REDIRECT_URI}?" + "${SWITCH_BROWSER.CODE}=$CODE&" + - "${SWITCH_BROWSER.ACTION_URI}=$ACTION_URI" + "${SWITCH_BROWSER.ACTION_URI}=$ACTION_URI&" + + "${SWITCH_BROWSER.STATE}=$STATE" val redirectUri = Uri.parse(redirectString) val switchBrowserProcessUri = SwitchBrowserUriHelper.buildProcessUri(redirectUri) @@ -95,12 +97,41 @@ class SwitchBrowserUriHelperTest { @Test fun `test buildResumeUri valid params`() { val uri = SwitchBrowserUriHelper.buildResumeUri( - ACTION_URI + ACTION_URI, STATE ) Assert.assertNotNull(uri) Assert.assertEquals( ACTION_URI, uri.host + uri.path ) + Assert.assertEquals( + STATE, + uri.getQueryParameter(SWITCH_BROWSER.STATE) + ) + } + + @Test + fun `test states match`() { + SwitchBrowserUriHelper.statesMatch( + "https://example.auth.com/path?${SWITCH_BROWSER.STATE}=$STATE", STATE + ) + } + + @Test + fun `test states don't match`() { + val exception = Assert.assertThrows( + ClientException::class.java) { + SwitchBrowserUriHelper.statesMatch( + "https://example.auth.com/path?${SWITCH_BROWSER.STATE}=$STATE", "error" + ) + } + Assert.assertEquals( + ClientException.STATE_MISMATCH, + exception.errorCode + ) + Assert.assertEquals( + "State does not match with the auth request state.", + exception.message + ) } } diff --git a/common/src/test/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserProtocolCoordinatorTest.kt b/common/src/test/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserProtocolCoordinatorTest.kt index 306f006ba5..031a0ae6c8 100644 --- a/common/src/test/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserProtocolCoordinatorTest.kt +++ b/common/src/test/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserProtocolCoordinatorTest.kt @@ -50,15 +50,17 @@ class SwitchBrowserProtocolCoordinatorTest { doNothing().`when`(mockSwitchBrowserRequestHandler).resetChallengeState() val code = "switch_browser_code" val actionUrl = "test.example.com/switchbrowser/path" + val state = "123" val extras = Bundle().apply { putString(SWITCH_BROWSER.CODE, code) putString(SWITCH_BROWSER.ACTION_URI, actionUrl) + putString(SWITCH_BROWSER.STATE, state) } // Create an instance of SwitchBrowserProtocolCoordinator val coordinator = SwitchBrowserProtocolCoordinator(mockSwitchBrowserRequestHandler) // Call the method to be tested - coordinator.processSwitchBrowserResume(extras) { uri, headers -> + coordinator.processSwitchBrowserResume("https://auth.com?state=$state",extras) { uri, headers -> // Verify the resume URI Assert.assertEquals(actionUrl, uri.host + uri.path) Assert.assertEquals(code, headers[AUTHORIZATION]) @@ -78,13 +80,13 @@ class SwitchBrowserProtocolCoordinatorTest { val exception = Assert.assertThrows(ClientException::class.java) { // Call the method to be tested - coordinator.processSwitchBrowserResume(extras) { _, _ -> + coordinator.processSwitchBrowserResume("",extras) { _, _ -> // This block should not be executed Assert.fail() } } Assert.assertEquals(ClientException.MISSING_PARAMETER, exception.errorCode) - Assert.assertEquals("Action URI is null/empty: true, code is null/empty: true", exception.message) + Assert.assertEquals("Action URI is null/empty: true, code is null/empty: true, state is null/empty: true", exception.message) } @Test From 0a5f8155de662ea2cb4c51d68f9566e814116105 Mon Sep 17 00:00:00 2001 From: p3dr0rv Date: Mon, 28 Apr 2025 13:18:29 -0700 Subject: [PATCH 2/4] update changelog --- changelog.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/changelog.txt b/changelog.txt index 6e9a99be5d..41583994d8 100644 --- a/changelog.txt +++ b/changelog.txt @@ -1,5 +1,6 @@ vNext ---------- +- [MINOR] Adding a state parameter to prevent phish attacks, in “switch_browser” flow. (#2631) - [MINOR] Pass Work Profile existence, OS Version, and Manufacturer to ESTS (#2627) - [MINOR] Add telemetry for the switch browser protocol (#2612) From 511bc1feea9b9ec51eea718342bd6057b213e2f4 Mon Sep 17 00:00:00 2001 From: p3dr0rv Date: Tue, 29 Apr 2025 10:45:31 -0700 Subject: [PATCH 3/4] add flight to check state validation --- .../SwitchBrowserProtocolCoordinator.kt | 6 +- .../switchbrowser/SwitchBrowserUriHelper.kt | 69 +++++++++++++++---- .../SwitchBrowserRequestHandlerTest.kt | 39 +++++++++-- .../SwitchBrowserProtocolCoordinatorTest.kt | 61 +++++++++++++++- .../SwitchBrowserUriHelperTest.kt | 47 ++++++++++++- .../common/java/flighting/CommonFlight.java | 8 ++- 6 files changed, 200 insertions(+), 30 deletions(-) rename common/src/test/java/com/microsoft/identity/common/internal/ui/webview/{challengehandlers => switchbrowser}/SwitchBrowserUriHelperTest.kt (75%) diff --git a/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserProtocolCoordinator.kt b/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserProtocolCoordinator.kt index 3372909102..dc13330708 100644 --- a/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserProtocolCoordinator.kt +++ b/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserProtocolCoordinator.kt @@ -37,7 +37,6 @@ import com.microsoft.identity.common.java.opentelemetry.AttributeName import com.microsoft.identity.common.java.opentelemetry.OTelUtility import com.microsoft.identity.common.java.opentelemetry.SpanExtension import com.microsoft.identity.common.java.opentelemetry.SpanName -import com.microsoft.identity.common.java.providers.oauth2.AuthorizationRequest import com.microsoft.identity.common.java.ui.AuthorizationAgent import com.microsoft.identity.common.logging.Logger import io.opentelemetry.api.trace.Span @@ -121,12 +120,11 @@ class SwitchBrowserProtocolCoordinator( val actionUri = extras.getString(SWITCH_BROWSER.ACTION_URI) val code = extras.getString(SWITCH_BROWSER.CODE) val state = extras.getString(SWITCH_BROWSER.STATE) - if (actionUri.isNullOrEmpty() || code.isNullOrEmpty() || state.isNullOrEmpty()) { + if (actionUri.isNullOrEmpty() || code.isNullOrEmpty()) { val clientException = ClientException( ClientException.MISSING_PARAMETER, "Action URI is null/empty: ${actionUri.isNullOrEmpty()}," + - " code is null/empty: ${code.isNullOrEmpty()}," + - " state is null/empty: ${state.isNullOrEmpty()}" + " code is null/empty: ${code.isNullOrEmpty()}." ) span.setStatus(StatusCode.ERROR) span.recordException(clientException) diff --git a/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserUriHelper.kt b/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserUriHelper.kt index 11b2bb91a7..0894c1483f 100644 --- a/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserUriHelper.kt +++ b/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserUriHelper.kt @@ -25,6 +25,8 @@ package com.microsoft.identity.common.internal.ui.webview.switchbrowser import android.net.Uri import com.microsoft.identity.common.adal.internal.AuthenticationConstants.SWITCH_BROWSER import com.microsoft.identity.common.java.exception.ClientException +import com.microsoft.identity.common.java.flighting.CommonFlight +import com.microsoft.identity.common.java.flighting.CommonFlightsManager import com.microsoft.identity.common.java.opentelemetry.SpanExtension import com.microsoft.identity.common.logging.Logger import io.opentelemetry.api.trace.StatusCode @@ -36,6 +38,13 @@ object SwitchBrowserUriHelper { private const val TAG = "SwitchBrowserUriHelper" + internal val STATE_VALIDATION_REQUIRED: Boolean by lazy { + CommonFlightsManager + .getFlightsProvider() + .isFlightEnabled(CommonFlight.SWITCH_BROWSER_PROTOCOL_REQUIRES_STATE) + } + + /** * Build the process uri for the switch browser challenge. * @@ -70,20 +79,24 @@ object SwitchBrowserUriHelper { Logger.error(methodTag, errorMessage, exception) throw exception } - val state = uri.getQueryParameter( - SWITCH_BROWSER.STATE - ) - if (state.isNullOrEmpty()) { - // This should never happen, but if it does, we should log it and throw. - val errorMessage = "switch browser action state is null or empty" - val exception = ClientException(ClientException.MALFORMED_URL, errorMessage) - Logger.error(methodTag, errorMessage, exception) - throw exception - } + // Query parameters for the process uri. val queryParams = hashMapOf() queryParams[SWITCH_BROWSER.CODE] = code - queryParams[SWITCH_BROWSER.STATE] = state + if (STATE_VALIDATION_REQUIRED) { + val state = uri.getQueryParameter( + SWITCH_BROWSER.STATE + ) + if (state.isNullOrEmpty()) { + // This should never happen, but if it does, we should log it and throw. + val errorMessage = "switch browser action state is null or empty" + val exception = ClientException(ClientException.MALFORMED_URL, errorMessage) + Logger.error(methodTag, errorMessage, exception) + throw exception + } else { + queryParams[SWITCH_BROWSER.STATE] = state + } + } // Construct the uri to the process endpoint. return buildSwitchBrowserUri(actionUri, queryParams) } @@ -97,9 +110,21 @@ object SwitchBrowserUriHelper { * @return The resume uri constructed from the bundle. * e.g. actionUri */ - fun buildResumeUri(actionUri: String, state: String): Uri { + fun buildResumeUri(actionUri: String, state: String?): Uri { + val methodTag = "$TAG:buildResumeUri" // Construct the uri to the resume endpoint. - val queryParams = hashMapOf(SWITCH_BROWSER.STATE to state) + val queryParams = hashMapOf() + if (STATE_VALIDATION_REQUIRED) { + if (state.isNullOrEmpty()) { + // This should never happen, but if it does, we should log it and throw. + val errorMessage = "State is null or empty" + val exception = ClientException(ClientException.MISSING_PARAMETER, errorMessage) + Logger.error(methodTag, errorMessage, exception) + throw exception + } else { + queryParams[SWITCH_BROWSER.STATE] = state + } + } return buildSwitchBrowserUri(actionUri, queryParams) } @@ -126,10 +151,15 @@ object SwitchBrowserUriHelper { * Check if state in the auth request matches the state provided. */ fun statesMatch(authorizationUrl: String, state: String?) { + val methodTag = "$TAG:statesMatch" + if (!STATE_VALIDATION_REQUIRED) { + Logger.info(methodTag, "State validation is not required.") + return + } val span = SpanExtension.current() // Validate the state from auth request and redirect URL is the same if (state.isNullOrEmpty()) { - val clientException = ClientException( + val clientException = ClientException( ClientException.STATE_MISMATCH, "State is null." ) @@ -139,6 +169,16 @@ object SwitchBrowserUriHelper { throw clientException } val authRequestState = Uri.parse(authorizationUrl).getQueryParameter(SWITCH_BROWSER.STATE) + if (authRequestState.isNullOrEmpty()) { + val clientException = ClientException( + ClientException.STATE_MISMATCH, + "Authorization request state is null." + ) + span.setStatus(StatusCode.ERROR) + span.recordException(clientException) + span.end() + throw clientException + } if (state != authRequestState) { val clientException = ClientException( ClientException.STATE_MISMATCH, @@ -149,6 +189,7 @@ object SwitchBrowserUriHelper { span.end() throw clientException } + Logger.info(methodTag, "States match.") } /** diff --git a/common/src/test/java/com/microsoft/identity/common/internal/ui/webview/challengehandlers/SwitchBrowserRequestHandlerTest.kt b/common/src/test/java/com/microsoft/identity/common/internal/ui/webview/challengehandlers/SwitchBrowserRequestHandlerTest.kt index 59b45a5103..1c9ed0e7d0 100644 --- a/common/src/test/java/com/microsoft/identity/common/internal/ui/webview/challengehandlers/SwitchBrowserRequestHandlerTest.kt +++ b/common/src/test/java/com/microsoft/identity/common/internal/ui/webview/challengehandlers/SwitchBrowserRequestHandlerTest.kt @@ -27,9 +27,12 @@ import android.content.Context import android.content.Intent import android.net.Uri import com.microsoft.identity.common.internal.ui.browser.CustomTabsManager +import com.microsoft.identity.common.internal.ui.webview.switchbrowser.SwitchBrowserUriHelper import com.microsoft.identity.common.java.browser.Browser import com.microsoft.identity.common.java.browser.IBrowserSelector import com.microsoft.identity.common.java.exception.ClientException +import io.mockk.every +import io.mockk.mockkObject import org.junit.Assert import org.junit.Test import org.junit.runner.RunWith @@ -46,7 +49,8 @@ import org.robolectric.RobolectricTestRunner class SwitchBrowserRequestHandlerTest { @Test - fun `test processChallenge success`() { + fun `test processChallenge success (stateRequired)`() { + isStateRequired(true) // Mock parameters val mockActivity = mock() var activityExecuted = false @@ -66,6 +70,28 @@ class SwitchBrowserRequestHandlerTest { Assert.assertTrue(activityExecuted) } + @Test + fun `test processChallenge success (StateNotRequired)`() { + isStateRequired(false) + // Mock parameters + val mockActivity = mock() + var activityExecuted = false + doAnswer { + activityExecuted = true + null + }.whenever(mockActivity).startActivity(any()) + val context = mock(Context::class.java) + val customTabsManager = mock(CustomTabsManager::class.java) + val challenge = mock(SwitchBrowserChallenge::class.java) + `when`(challenge.processUri).thenReturn(Uri.parse("https://example.com")) + `when`(challenge.authorizationUrl).thenReturn("https://auth.com") + val browserSelector = // Browser available + IBrowserSelector { _, _ -> Browser("fakeBrowser", emptySet(), "browser", false) } + val handler = SwitchBrowserRequestHandler(mockActivity, context, customTabsManager, browserSelector, null) + handler.processChallenge(challenge) + Assert.assertTrue(activityExecuted) + } + @Test fun `test processChallenge no browser available`() { // Mock parameters @@ -87,13 +113,9 @@ class SwitchBrowserRequestHandlerTest { @Test fun `test processChallenge states mismatch`() { + isStateRequired(true) // Mock parameters val mockActivity = mock() - var activityExecuted = false - doAnswer { - activityExecuted = true - null - }.whenever(mockActivity).startActivity(any()) val context = mock(Context::class.java) val customTabsManager = mock(CustomTabsManager::class.java) val challenge = mock(SwitchBrowserChallenge::class.java) @@ -108,4 +130,9 @@ class SwitchBrowserRequestHandlerTest { Assert.assertEquals(ClientException.STATE_MISMATCH, exception.errorCode) Assert.assertEquals("State does not match with the auth request state.", exception.message) } + + private fun isStateRequired(isStateRequired: Boolean) { + mockkObject(SwitchBrowserUriHelper) + every { SwitchBrowserUriHelper.STATE_VALIDATION_REQUIRED } returns isStateRequired + } } diff --git a/common/src/test/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserProtocolCoordinatorTest.kt b/common/src/test/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserProtocolCoordinatorTest.kt index 031a0ae6c8..61df3ca103 100644 --- a/common/src/test/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserProtocolCoordinatorTest.kt +++ b/common/src/test/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserProtocolCoordinatorTest.kt @@ -32,6 +32,8 @@ import com.microsoft.identity.common.internal.ui.webview.challengehandlers.Switc import com.microsoft.identity.common.java.AuthenticationConstants.AAD.AUTHORIZATION import com.microsoft.identity.common.java.exception.ClientException import com.microsoft.identity.common.java.ui.AuthorizationAgent +import io.mockk.every +import io.mockk.mockkObject import org.junit.Assert import org.junit.Test import org.junit.runner.RunWith @@ -44,7 +46,8 @@ import org.robolectric.RobolectricTestRunner class SwitchBrowserProtocolCoordinatorTest { @Test - fun `test processSwitchBrowserResume with valid extras`() { + fun `test processSwitchBrowserResume with valid extras (stateRequired)`() { + isStateRequired(true) // Mock parameters val mockSwitchBrowserRequestHandler = mock(SwitchBrowserRequestHandler::class.java) doNothing().`when`(mockSwitchBrowserRequestHandler).resetChallengeState() @@ -67,8 +70,57 @@ class SwitchBrowserProtocolCoordinatorTest { } } + @Test + fun `test processSwitchBrowserResume with valid extras (StateNotRequired)`() { + isStateRequired(false) + // Mock parameters + val mockSwitchBrowserRequestHandler = mock(SwitchBrowserRequestHandler::class.java) + doNothing().`when`(mockSwitchBrowserRequestHandler).resetChallengeState() + val code = "switch_browser_code" + val actionUrl = "test.example.com/switchbrowser/path" + val extras = Bundle().apply { + putString(SWITCH_BROWSER.CODE, code) + putString(SWITCH_BROWSER.ACTION_URI, actionUrl) + } + // Create an instance of SwitchBrowserProtocolCoordinator + val coordinator = SwitchBrowserProtocolCoordinator(mockSwitchBrowserRequestHandler) + + // Call the method to be tested + coordinator.processSwitchBrowserResume("https://auth.com",extras) { uri, headers -> + // Verify the resume URI + Assert.assertEquals(actionUrl, uri.host + uri.path) + Assert.assertEquals(code, headers[AUTHORIZATION]) + } + } + + @Test + fun `test processSwitchBrowserResume with missing state (stateRequired)`() { + isStateRequired(true) + // Mock parameters + val mockSwitchBrowserRequestHandler = mock(SwitchBrowserRequestHandler::class.java) + val code = "switch_browser_code" + val actionUrl = "test.example.com/switchbrowser/path" + val extras = Bundle().apply { + putString(SWITCH_BROWSER.CODE, code) + putString(SWITCH_BROWSER.ACTION_URI, actionUrl) + } + // Create an instance of SwitchBrowserProtocolCoordinator + val coordinator = SwitchBrowserProtocolCoordinator(mockSwitchBrowserRequestHandler) + + val exception = Assert.assertThrows(ClientException::class.java) { + // Call the method to be tested + coordinator.processSwitchBrowserResume("",extras) { _, _ -> + // This block should not be executed + Assert.fail() + } + } + Assert.assertEquals(ClientException.STATE_MISMATCH, exception.errorCode) + Assert.assertEquals("State is null.", exception.message) + } + @Test fun `test processSwitchBrowserResume with missing extras`() { + isStateRequired(false) // Mock parameters val mockSwitchBrowserRequestHandler = mock(SwitchBrowserRequestHandler::class.java) val extras = Bundle().apply { @@ -86,7 +138,7 @@ class SwitchBrowserProtocolCoordinatorTest { } } Assert.assertEquals(ClientException.MISSING_PARAMETER, exception.errorCode) - Assert.assertEquals("Action URI is null/empty: true, code is null/empty: true, state is null/empty: true", exception.message) + Assert.assertEquals("Action URI is null/empty: true, code is null/empty: true.", exception.message) } @Test @@ -170,4 +222,9 @@ class SwitchBrowserProtocolCoordinatorTest { intent.getSerializableExtra(AUTHORIZATION_AGENT) as AuthorizationAgent ) } + + private fun isStateRequired(isStateRequired: Boolean) { + mockkObject(SwitchBrowserUriHelper) + every { SwitchBrowserUriHelper.STATE_VALIDATION_REQUIRED } returns isStateRequired + } } diff --git a/common/src/test/java/com/microsoft/identity/common/internal/ui/webview/challengehandlers/SwitchBrowserUriHelperTest.kt b/common/src/test/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserUriHelperTest.kt similarity index 75% rename from common/src/test/java/com/microsoft/identity/common/internal/ui/webview/challengehandlers/SwitchBrowserUriHelperTest.kt rename to common/src/test/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserUriHelperTest.kt index c65aba2abc..7c7e094d9f 100644 --- a/common/src/test/java/com/microsoft/identity/common/internal/ui/webview/challengehandlers/SwitchBrowserUriHelperTest.kt +++ b/common/src/test/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserUriHelperTest.kt @@ -20,13 +20,14 @@ // LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN // THE SOFTWARE. -package com.microsoft.identity.common.internal.ui.webview.challengehandlers +package com.microsoft.identity.common.internal.ui.webview.switchbrowser import android.net.Uri import com.microsoft.identity.common.adal.internal.AuthenticationConstants.Broker import com.microsoft.identity.common.adal.internal.AuthenticationConstants.SWITCH_BROWSER -import com.microsoft.identity.common.internal.ui.webview.switchbrowser.SwitchBrowserUriHelper import com.microsoft.identity.common.java.exception.ClientException +import io.mockk.every +import io.mockk.mockkObject import org.junit.Assert import org.junit.Test import org.junit.runner.RunWith @@ -42,13 +43,38 @@ class SwitchBrowserUriHelperTest { } @Test - fun `test constructFromRedirectUri with valid redirect uri`() { + fun `test constructFromRedirectUri with valid redirect uri (StateRequired)`() { + isStateRequired(true) val redirectString = "${Broker.NEW_BROKER_REDIRECT_URI}?" + "${SWITCH_BROWSER.CODE}=$CODE&" + "${SWITCH_BROWSER.ACTION_URI}=$ACTION_URI&" + "${SWITCH_BROWSER.STATE}=$STATE" val redirectUri = Uri.parse(redirectString) + val switchBrowserProcessUri = SwitchBrowserUriHelper.buildProcessUri(redirectUri) + Assert.assertNotNull(switchBrowserProcessUri) + Assert.assertEquals( + CODE, + switchBrowserProcessUri.getQueryParameter(SWITCH_BROWSER.CODE) + ) + Assert.assertEquals( + ACTION_URI, + switchBrowserProcessUri.host + switchBrowserProcessUri.path + ) + Assert.assertEquals( + STATE, + switchBrowserProcessUri.getQueryParameter(SWITCH_BROWSER.STATE) + ) + } + + @Test + fun `test constructFromRedirectUri with valid redirect uri (StateNotRequired)`() { + isStateRequired(false) + val redirectString = "${Broker.NEW_BROKER_REDIRECT_URI}?" + + "${SWITCH_BROWSER.CODE}=$CODE&" + + "${SWITCH_BROWSER.ACTION_URI}=$ACTION_URI" + val redirectUri = Uri.parse(redirectString) + val switchBrowserProcessUri = SwitchBrowserUriHelper.buildProcessUri(redirectUri) Assert.assertNotNull(switchBrowserProcessUri) Assert.assertEquals( @@ -110,8 +136,17 @@ class SwitchBrowserUriHelperTest { ) } + @Test + fun `test states match (statesNotRequired)`() { + isStateRequired(false) + SwitchBrowserUriHelper.statesMatch( + "", null + ) + } + @Test fun `test states match`() { + isStateRequired(true) SwitchBrowserUriHelper.statesMatch( "https://example.auth.com/path?${SWITCH_BROWSER.STATE}=$STATE", STATE ) @@ -119,6 +154,7 @@ class SwitchBrowserUriHelperTest { @Test fun `test states don't match`() { + isStateRequired(true) val exception = Assert.assertThrows( ClientException::class.java) { SwitchBrowserUriHelper.statesMatch( @@ -134,4 +170,9 @@ class SwitchBrowserUriHelperTest { exception.message ) } + + private fun isStateRequired(isStateRequired: Boolean) { + mockkObject(SwitchBrowserUriHelper) + every { SwitchBrowserUriHelper.STATE_VALIDATION_REQUIRED } returns isStateRequired + } } diff --git a/common4j/src/main/com/microsoft/identity/common/java/flighting/CommonFlight.java b/common4j/src/main/com/microsoft/identity/common/java/flighting/CommonFlight.java index 7cddcaca5b..ce7373ab5b 100644 --- a/common4j/src/main/com/microsoft/identity/common/java/flighting/CommonFlight.java +++ b/common4j/src/main/com/microsoft/identity/common/java/flighting/CommonFlight.java @@ -98,7 +98,13 @@ public enum CommonFlight implements IFlightConfig { /** * Flight to enable the attachment of PRT header in cross cloud requests. Default is true. */ - ENABLE_ATTACH_PRT_HEADER_WHEN_CROSS_CLOUD("EnableAttachPrtHeaderWhenCrossCloud", true); + ENABLE_ATTACH_PRT_HEADER_WHEN_CROSS_CLOUD("EnableAttachPrtHeaderWhenCrossCloud", true), + + /** + * Flight to make the state parameter required for the switch browser protocol. + */ + SWITCH_BROWSER_PROTOCOL_REQUIRES_STATE("SwitchBrowserProtocolRequiresState", false); + private String key; private Object defaultValue; CommonFlight(@NonNull String key, @NonNull Object defaultValue) { From f84da49bc1a22a471ef29286d0acc8643a079853 Mon Sep 17 00:00:00 2001 From: p3dr0rv Date: Tue, 29 Apr 2025 17:47:39 -0700 Subject: [PATCH 4/4] add addStateToQueryParams to reuse code --- .../switchbrowser/SwitchBrowserUriHelper.kt | 50 +++++++++---------- .../SwitchBrowserUriHelperTest.kt | 43 +++++++++++++++- 2 files changed, 65 insertions(+), 28 deletions(-) diff --git a/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserUriHelper.kt b/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserUriHelper.kt index 0894c1483f..de0f48fffa 100644 --- a/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserUriHelper.kt +++ b/common/src/main/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserUriHelper.kt @@ -79,24 +79,13 @@ object SwitchBrowserUriHelper { Logger.error(methodTag, errorMessage, exception) throw exception } - + val state = uri.getQueryParameter( + SWITCH_BROWSER.STATE + ) // Query parameters for the process uri. val queryParams = hashMapOf() queryParams[SWITCH_BROWSER.CODE] = code - if (STATE_VALIDATION_REQUIRED) { - val state = uri.getQueryParameter( - SWITCH_BROWSER.STATE - ) - if (state.isNullOrEmpty()) { - // This should never happen, but if it does, we should log it and throw. - val errorMessage = "switch browser action state is null or empty" - val exception = ClientException(ClientException.MALFORMED_URL, errorMessage) - Logger.error(methodTag, errorMessage, exception) - throw exception - } else { - queryParams[SWITCH_BROWSER.STATE] = state - } - } + addStateToQueryParams(queryParams, state, methodTag) // Construct the uri to the process endpoint. return buildSwitchBrowserUri(actionUri, queryParams) } @@ -114,17 +103,7 @@ object SwitchBrowserUriHelper { val methodTag = "$TAG:buildResumeUri" // Construct the uri to the resume endpoint. val queryParams = hashMapOf() - if (STATE_VALIDATION_REQUIRED) { - if (state.isNullOrEmpty()) { - // This should never happen, but if it does, we should log it and throw. - val errorMessage = "State is null or empty" - val exception = ClientException(ClientException.MISSING_PARAMETER, errorMessage) - Logger.error(methodTag, errorMessage, exception) - throw exception - } else { - queryParams[SWITCH_BROWSER.STATE] = state - } - } + addStateToQueryParams(queryParams, state, methodTag) return buildSwitchBrowserUri(actionUri, queryParams) } @@ -192,6 +171,25 @@ object SwitchBrowserUriHelper { Logger.info(methodTag, "States match.") } + /** + * Add state to the query parameters If STATE_VALIDATION_REQUIRED is enabled. + * If STATE_VALIDATION_REQUIRED is disabled, this method does nothing. + * If STATE_VALIDATION_REQUIRED is enabled and state is null or empty, this method throws an exception. + */ + private fun addStateToQueryParams(queryParams: HashMap, state: String?, methodTag: String) { + if (STATE_VALIDATION_REQUIRED) { + if (state.isNullOrEmpty()) { + // This should never happen, but if it does, we should log it and throw. + val errorMessage = "State is null or empty" + val exception = ClientException(ClientException.MISSING_PARAMETER, errorMessage) + Logger.error(methodTag, errorMessage, exception) + throw exception + } else { + queryParams[SWITCH_BROWSER.STATE] = state + } + } + } + /** * Build a generic switch browser uri. * diff --git a/common/src/test/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserUriHelperTest.kt b/common/src/test/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserUriHelperTest.kt index 7c7e094d9f..1be8fa5f36 100644 --- a/common/src/test/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserUriHelperTest.kt +++ b/common/src/test/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserUriHelperTest.kt @@ -88,7 +88,7 @@ class SwitchBrowserUriHelperTest { } @Test - fun `test constructFromRedirectUri with missing code`() { + fun `test buildProcessUri with missing code`() { val redirectString = "${Broker.NEW_BROKER_REDIRECT_URI}?" + "${SWITCH_BROWSER.ACTION_URI}=$ACTION_URI" val redirectUri = Uri.parse(redirectString) @@ -100,6 +100,21 @@ class SwitchBrowserUriHelperTest { Assert.assertEquals("switch browser code is null or empty", exception.message) } + @Test + fun `test buildProcessUri with missing sate (StateRequired)`() { + isStateRequired(true) + val redirectString = "${Broker.NEW_BROKER_REDIRECT_URI}?" + + "${SWITCH_BROWSER.CODE}=$CODE&" + + "${SWITCH_BROWSER.ACTION_URI}=$ACTION_URI" + val redirectUri = Uri.parse(redirectString) + + val exception = Assert.assertThrows(ClientException::class.java) { + SwitchBrowserUriHelper.buildProcessUri(redirectUri) + } + Assert.assertEquals(ClientException.MISSING_PARAMETER, exception.errorCode) + Assert.assertEquals("State is null or empty", exception.message) + } + @Test fun `test isSwitchBrowserRedirectUrl incorrect url`() { val url = "https://login.microsoftonline.com/" @@ -121,7 +136,21 @@ class SwitchBrowserUriHelperTest { } @Test - fun `test buildResumeUri valid params`() { + fun `test buildResumeUri valid params (stateNotRequired)`() { + isStateRequired(false) + val uri = SwitchBrowserUriHelper.buildResumeUri( + ACTION_URI, null + ) + Assert.assertNotNull(uri) + Assert.assertEquals( + ACTION_URI, + uri.host + uri.path + ) + Assert.assertNull(uri.getQueryParameter(SWITCH_BROWSER.STATE)) + } + + @Test + fun `test buildResumeUri valid params (stateRequired)`() { val uri = SwitchBrowserUriHelper.buildResumeUri( ACTION_URI, STATE ) @@ -136,6 +165,16 @@ class SwitchBrowserUriHelperTest { ) } + @Test + fun `test buildResumeUri missing state (stateRequired)`() { + isStateRequired(true) + val exception = Assert.assertThrows(ClientException::class.java) { + SwitchBrowserUriHelper.buildResumeUri(ACTION_URI, null) + } + Assert.assertEquals(ClientException.MISSING_PARAMETER, exception.errorCode) + Assert.assertEquals("State is null or empty", exception.message) + } + @Test fun `test states match (statesNotRequired)`() { isStateRequired(false)