Skip to content

Commit 511bc1f

Browse files
p3dr0rvp3dr0rv
committed
add flight to check state validation
1 parent 0a5f815 commit 511bc1f

6 files changed

Lines changed: 200 additions & 30 deletions

File tree

‎common/src/main/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserProtocolCoordinator.kt‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@ import com.microsoft.identity.common.java.opentelemetry.AttributeName
3737
import com.microsoft.identity.common.java.opentelemetry.OTelUtility
3838
import com.microsoft.identity.common.java.opentelemetry.SpanExtension
3939
import com.microsoft.identity.common.java.opentelemetry.SpanName
40-
import com.microsoft.identity.common.java.providers.oauth2.AuthorizationRequest
4140
import com.microsoft.identity.common.java.ui.AuthorizationAgent
4241
import com.microsoft.identity.common.logging.Logger
4342
import io.opentelemetry.api.trace.Span
@@ -121,12 +120,11 @@ class SwitchBrowserProtocolCoordinator(
121120
val actionUri = extras.getString(SWITCH_BROWSER.ACTION_URI)
122121
val code = extras.getString(SWITCH_BROWSER.CODE)
123122
val state = extras.getString(SWITCH_BROWSER.STATE)
124-
if (actionUri.isNullOrEmpty() || code.isNullOrEmpty() || state.isNullOrEmpty()) {
123+
if (actionUri.isNullOrEmpty() || code.isNullOrEmpty()) {
125124
val clientException = ClientException(
126125
ClientException.MISSING_PARAMETER,
127126
"Action URI is null/empty: ${actionUri.isNullOrEmpty()}," +
128-
" code is null/empty: ${code.isNullOrEmpty()}," +
129-
" state is null/empty: ${state.isNullOrEmpty()}"
127+
" code is null/empty: ${code.isNullOrEmpty()}."
130128
)
131129
span.setStatus(StatusCode.ERROR)
132130
span.recordException(clientException)

‎common/src/main/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserUriHelper.kt‎

Lines changed: 55 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@ package com.microsoft.identity.common.internal.ui.webview.switchbrowser
2525
import android.net.Uri
2626
import com.microsoft.identity.common.adal.internal.AuthenticationConstants.SWITCH_BROWSER
2727
import com.microsoft.identity.common.java.exception.ClientException
28+
import com.microsoft.identity.common.java.flighting.CommonFlight
29+
import com.microsoft.identity.common.java.flighting.CommonFlightsManager
2830
import com.microsoft.identity.common.java.opentelemetry.SpanExtension
2931
import com.microsoft.identity.common.logging.Logger
3032
import io.opentelemetry.api.trace.StatusCode
@@ -36,6 +38,13 @@ object SwitchBrowserUriHelper {
3638

3739
private const val TAG = "SwitchBrowserUriHelper"
3840

41+
internal val STATE_VALIDATION_REQUIRED: Boolean by lazy {
42+
CommonFlightsManager
43+
.getFlightsProvider()
44+
.isFlightEnabled(CommonFlight.SWITCH_BROWSER_PROTOCOL_REQUIRES_STATE)
45+
}
46+
47+
3948
/**
4049
* Build the process uri for the switch browser challenge.
4150
*
@@ -70,20 +79,24 @@ object SwitchBrowserUriHelper {
7079
Logger.error(methodTag, errorMessage, exception)
7180
throw exception
7281
}
73-
val state = uri.getQueryParameter(
74-
SWITCH_BROWSER.STATE
75-
)
76-
if (state.isNullOrEmpty()) {
77-
// This should never happen, but if it does, we should log it and throw.
78-
val errorMessage = "switch browser action state is null or empty"
79-
val exception = ClientException(ClientException.MALFORMED_URL, errorMessage)
80-
Logger.error(methodTag, errorMessage, exception)
81-
throw exception
82-
}
82+
8383
// Query parameters for the process uri.
8484
val queryParams = hashMapOf<String, String>()
8585
queryParams[SWITCH_BROWSER.CODE] = code
86-
queryParams[SWITCH_BROWSER.STATE] = state
86+
if (STATE_VALIDATION_REQUIRED) {
87+
val state = uri.getQueryParameter(
88+
SWITCH_BROWSER.STATE
89+
)
90+
if (state.isNullOrEmpty()) {
91+
// This should never happen, but if it does, we should log it and throw.
92+
val errorMessage = "switch browser action state is null or empty"
93+
val exception = ClientException(ClientException.MALFORMED_URL, errorMessage)
94+
Logger.error(methodTag, errorMessage, exception)
95+
throw exception
96+
} else {
97+
queryParams[SWITCH_BROWSER.STATE] = state
98+
}
99+
}
87100
// Construct the uri to the process endpoint.
88101
return buildSwitchBrowserUri(actionUri, queryParams)
89102
}
@@ -97,9 +110,21 @@ object SwitchBrowserUriHelper {
97110
* @return The resume uri constructed from the bundle.
98111
* e.g. actionUri
99112
*/
100-
fun buildResumeUri(actionUri: String, state: String): Uri {
113+
fun buildResumeUri(actionUri: String, state: String?): Uri {
114+
val methodTag = "$TAG:buildResumeUri"
101115
// Construct the uri to the resume endpoint.
102-
val queryParams = hashMapOf(SWITCH_BROWSER.STATE to state)
116+
val queryParams = hashMapOf<String, String>()
117+
if (STATE_VALIDATION_REQUIRED) {
118+
if (state.isNullOrEmpty()) {
119+
// This should never happen, but if it does, we should log it and throw.
120+
val errorMessage = "State is null or empty"
121+
val exception = ClientException(ClientException.MISSING_PARAMETER, errorMessage)
122+
Logger.error(methodTag, errorMessage, exception)
123+
throw exception
124+
} else {
125+
queryParams[SWITCH_BROWSER.STATE] = state
126+
}
127+
}
103128
return buildSwitchBrowserUri(actionUri, queryParams)
104129
}
105130

@@ -126,10 +151,15 @@ object SwitchBrowserUriHelper {
126151
* Check if state in the auth request matches the state provided.
127152
*/
128153
fun statesMatch(authorizationUrl: String, state: String?) {
154+
val methodTag = "$TAG:statesMatch"
155+
if (!STATE_VALIDATION_REQUIRED) {
156+
Logger.info(methodTag, "State validation is not required.")
157+
return
158+
}
129159
val span = SpanExtension.current()
130160
// Validate the state from auth request and redirect URL is the same
131161
if (state.isNullOrEmpty()) {
132-
val clientException = ClientException(
162+
val clientException = ClientException(
133163
ClientException.STATE_MISMATCH,
134164
"State is null."
135165
)
@@ -139,6 +169,16 @@ object SwitchBrowserUriHelper {
139169
throw clientException
140170
}
141171
val authRequestState = Uri.parse(authorizationUrl).getQueryParameter(SWITCH_BROWSER.STATE)
172+
if (authRequestState.isNullOrEmpty()) {
173+
val clientException = ClientException(
174+
ClientException.STATE_MISMATCH,
175+
"Authorization request state is null."
176+
)
177+
span.setStatus(StatusCode.ERROR)
178+
span.recordException(clientException)
179+
span.end()
180+
throw clientException
181+
}
142182
if (state != authRequestState) {
143183
val clientException = ClientException(
144184
ClientException.STATE_MISMATCH,
@@ -149,6 +189,7 @@ object SwitchBrowserUriHelper {
149189
span.end()
150190
throw clientException
151191
}
192+
Logger.info(methodTag, "States match.")
152193
}
153194

154195
/**

‎common/src/test/java/com/microsoft/identity/common/internal/ui/webview/challengehandlers/SwitchBrowserRequestHandlerTest.kt‎

Lines changed: 33 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,12 @@ import android.content.Context
2727
import android.content.Intent
2828
import android.net.Uri
2929
import com.microsoft.identity.common.internal.ui.browser.CustomTabsManager
30+
import com.microsoft.identity.common.internal.ui.webview.switchbrowser.SwitchBrowserUriHelper
3031
import com.microsoft.identity.common.java.browser.Browser
3132
import com.microsoft.identity.common.java.browser.IBrowserSelector
3233
import com.microsoft.identity.common.java.exception.ClientException
34+
import io.mockk.every
35+
import io.mockk.mockkObject
3336
import org.junit.Assert
3437
import org.junit.Test
3538
import org.junit.runner.RunWith
@@ -46,7 +49,8 @@ import org.robolectric.RobolectricTestRunner
4649
class SwitchBrowserRequestHandlerTest {
4750

4851
@Test
49-
fun `test processChallenge success`() {
52+
fun `test processChallenge success (stateRequired)`() {
53+
isStateRequired(true)
5054
// Mock parameters
5155
val mockActivity = mock<Activity>()
5256
var activityExecuted = false
@@ -66,6 +70,28 @@ class SwitchBrowserRequestHandlerTest {
6670
Assert.assertTrue(activityExecuted)
6771
}
6872

73+
@Test
74+
fun `test processChallenge success (StateNotRequired)`() {
75+
isStateRequired(false)
76+
// Mock parameters
77+
val mockActivity = mock<Activity>()
78+
var activityExecuted = false
79+
doAnswer {
80+
activityExecuted = true
81+
null
82+
}.whenever(mockActivity).startActivity(any())
83+
val context = mock(Context::class.java)
84+
val customTabsManager = mock(CustomTabsManager::class.java)
85+
val challenge = mock(SwitchBrowserChallenge::class.java)
86+
`when`(challenge.processUri).thenReturn(Uri.parse("https://example.com"))
87+
`when`(challenge.authorizationUrl).thenReturn("https://auth.com")
88+
val browserSelector = // Browser available
89+
IBrowserSelector { _, _ -> Browser("fakeBrowser", emptySet(), "browser", false) }
90+
val handler = SwitchBrowserRequestHandler(mockActivity, context, customTabsManager, browserSelector, null)
91+
handler.processChallenge(challenge)
92+
Assert.assertTrue(activityExecuted)
93+
}
94+
6995
@Test
7096
fun `test processChallenge no browser available`() {
7197
// Mock parameters
@@ -87,13 +113,9 @@ class SwitchBrowserRequestHandlerTest {
87113

88114
@Test
89115
fun `test processChallenge states mismatch`() {
116+
isStateRequired(true)
90117
// Mock parameters
91118
val mockActivity = mock<Activity>()
92-
var activityExecuted = false
93-
doAnswer {
94-
activityExecuted = true
95-
null
96-
}.whenever(mockActivity).startActivity(any())
97119
val context = mock(Context::class.java)
98120
val customTabsManager = mock(CustomTabsManager::class.java)
99121
val challenge = mock(SwitchBrowserChallenge::class.java)
@@ -108,4 +130,9 @@ class SwitchBrowserRequestHandlerTest {
108130
Assert.assertEquals(ClientException.STATE_MISMATCH, exception.errorCode)
109131
Assert.assertEquals("State does not match with the auth request state.", exception.message)
110132
}
133+
134+
private fun isStateRequired(isStateRequired: Boolean) {
135+
mockkObject(SwitchBrowserUriHelper)
136+
every { SwitchBrowserUriHelper.STATE_VALIDATION_REQUIRED } returns isStateRequired
137+
}
111138
}

‎common/src/test/java/com/microsoft/identity/common/internal/ui/webview/switchbrowser/SwitchBrowserProtocolCoordinatorTest.kt‎

Lines changed: 59 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@ import com.microsoft.identity.common.internal.ui.webview.challengehandlers.Switc
3232
import com.microsoft.identity.common.java.AuthenticationConstants.AAD.AUTHORIZATION
3333
import com.microsoft.identity.common.java.exception.ClientException
3434
import com.microsoft.identity.common.java.ui.AuthorizationAgent
35+
import io.mockk.every
36+
import io.mockk.mockkObject
3537
import org.junit.Assert
3638
import org.junit.Test
3739
import org.junit.runner.RunWith
@@ -44,7 +46,8 @@ import org.robolectric.RobolectricTestRunner
4446
class SwitchBrowserProtocolCoordinatorTest {
4547

4648
@Test
47-
fun `test processSwitchBrowserResume with valid extras`() {
49+
fun `test processSwitchBrowserResume with valid extras (stateRequired)`() {
50+
isStateRequired(true)
4851
// Mock parameters
4952
val mockSwitchBrowserRequestHandler = mock(SwitchBrowserRequestHandler::class.java)
5053
doNothing().`when`(mockSwitchBrowserRequestHandler).resetChallengeState()
@@ -67,8 +70,57 @@ class SwitchBrowserProtocolCoordinatorTest {
6770
}
6871
}
6972

73+
@Test
74+
fun `test processSwitchBrowserResume with valid extras (StateNotRequired)`() {
75+
isStateRequired(false)
76+
// Mock parameters
77+
val mockSwitchBrowserRequestHandler = mock(SwitchBrowserRequestHandler::class.java)
78+
doNothing().`when`(mockSwitchBrowserRequestHandler).resetChallengeState()
79+
val code = "switch_browser_code"
80+
val actionUrl = "test.example.com/switchbrowser/path"
81+
val extras = Bundle().apply {
82+
putString(SWITCH_BROWSER.CODE, code)
83+
putString(SWITCH_BROWSER.ACTION_URI, actionUrl)
84+
}
85+
// Create an instance of SwitchBrowserProtocolCoordinator
86+
val coordinator = SwitchBrowserProtocolCoordinator(mockSwitchBrowserRequestHandler)
87+
88+
// Call the method to be tested
89+
coordinator.processSwitchBrowserResume("https://auth.com",extras) { uri, headers ->
90+
// Verify the resume URI
91+
Assert.assertEquals(actionUrl, uri.host + uri.path)
92+
Assert.assertEquals(code, headers[AUTHORIZATION])
93+
}
94+
}
95+
96+
@Test
97+
fun `test processSwitchBrowserResume with missing state (stateRequired)`() {
98+
isStateRequired(true)
99+
// Mock parameters
100+
val mockSwitchBrowserRequestHandler = mock(SwitchBrowserRequestHandler::class.java)
101+
val code = "switch_browser_code"
102+
val actionUrl = "test.example.com/switchbrowser/path"
103+
val extras = Bundle().apply {
104+
putString(SWITCH_BROWSER.CODE, code)
105+
putString(SWITCH_BROWSER.ACTION_URI, actionUrl)
106+
}
107+
// Create an instance of SwitchBrowserProtocolCoordinator
108+
val coordinator = SwitchBrowserProtocolCoordinator(mockSwitchBrowserRequestHandler)
109+
110+
val exception = Assert.assertThrows(ClientException::class.java) {
111+
// Call the method to be tested
112+
coordinator.processSwitchBrowserResume("",extras) { _, _ ->
113+
// This block should not be executed
114+
Assert.fail()
115+
}
116+
}
117+
Assert.assertEquals(ClientException.STATE_MISMATCH, exception.errorCode)
118+
Assert.assertEquals("State is null.", exception.message)
119+
}
120+
70121
@Test
71122
fun `test processSwitchBrowserResume with missing extras`() {
123+
isStateRequired(false)
72124
// Mock parameters
73125
val mockSwitchBrowserRequestHandler = mock(SwitchBrowserRequestHandler::class.java)
74126
val extras = Bundle().apply {
@@ -86,7 +138,7 @@ class SwitchBrowserProtocolCoordinatorTest {
86138
}
87139
}
88140
Assert.assertEquals(ClientException.MISSING_PARAMETER, exception.errorCode)
89-
Assert.assertEquals("Action URI is null/empty: true, code is null/empty: true, state is null/empty: true", exception.message)
141+
Assert.assertEquals("Action URI is null/empty: true, code is null/empty: true.", exception.message)
90142
}
91143

92144
@Test
@@ -170,4 +222,9 @@ class SwitchBrowserProtocolCoordinatorTest {
170222
intent.getSerializableExtra(AUTHORIZATION_AGENT) as AuthorizationAgent
171223
)
172224
}
225+
226+
private fun isStateRequired(isStateRequired: Boolean) {
227+
mockkObject(SwitchBrowserUriHelper)
228+
every { SwitchBrowserUriHelper.STATE_VALIDATION_REQUIRED } returns isStateRequired
229+
}
173230
}

0 commit comments

Comments
 (0)