Skip to content

Commit 66e2ab6

Browse files
somalayasomalaya
committed
Fix hasKnownClientId to fail-closed, wrap processEstsHostRedirect in try-catch, consolidate tests
1 parent d11545b commit 66e2ab6

2 files changed

Lines changed: 186 additions & 82 deletions

File tree

‎common/src/main/java/com/microsoft/identity/common/internal/ui/webview/AzureActiveDirectoryWebViewClient.java‎

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1216,8 +1216,8 @@ && hasPrtHeaderAttached()
12161216

12171217
/**
12181218
* Returns true if the redirect URL's client_id is a known client: either the original
1219-
* request's client_id or the broker's client_id. If the redirect has no client_id,
1220-
* returns true to avoid blocking legitimate redirects.
1219+
* request's client_id or the broker's client_id. Returns false if the redirect has no
1220+
* client_id or if the URL cannot be parsed.
12211221
*/
12221222
private boolean hasKnownClientId(@NonNull final String url) {
12231223
try {
@@ -1231,7 +1231,7 @@ private boolean hasKnownClientId(@NonNull final String url) {
12311231
|| redirectClientId.equalsIgnoreCase(BROKER_CLIENT_ID);
12321232
} catch (@NonNull final Exception e) {
12331233
Logger.warn(TAG, "Failed to extract client_id from redirect URL.");
1234-
return true;
1234+
return false;
12351235
}
12361236
}
12371237

@@ -1241,16 +1241,23 @@ private boolean hasKnownClientId(@NonNull final String url) {
12411241
private void processEstsHostRedirect(@NonNull final WebView view, @NonNull final String url) {
12421242
final String methodTag = TAG + ":processEstsHostRedirect";
12431243
Logger.info(methodTag, "Processing eSTS host redirect with PRT re-attachment.");
1244-
final Span span = createSpanWithAttributesFromParent(SpanName.EstsHostRedirectPrtAttach.name());
1244+
Span span = null;
12451245
try {
1246+
span = createSpanWithAttributesFromParent(SpanName.EstsHostRedirectPrtAttach.name());
12461247
final String host = new URL(url).getHost();
12471248
span.setAttribute(AttributeName.ests_redirect_host.name(), host);
1248-
} catch (final MalformedURLException e) {
1249-
// Domain attribute is best-effort for telemetry
1249+
final ReAttachPrtHeaderHandler reAttachPrtHeaderHandler =
1250+
new ReAttachPrtHeaderHandler(view, mRequestHeaders, span, getLoginHintFromRequestUrl(), true);
1251+
reAttachPrtHeader(url, reAttachPrtHeaderHandler, view, methodTag, span);
1252+
} catch (final Throwable e) {
1253+
Logger.warn(methodTag, "Failed to process eSTS host redirect. Falling back to loading URL without PRT." + e);
1254+
if (span != null) {
1255+
span.recordException(e);
1256+
span.setStatus(StatusCode.ERROR);
1257+
span.end();
1258+
}
1259+
view.loadUrl(url);
12501260
}
1251-
final ReAttachPrtHeaderHandler reAttachPrtHeaderHandler =
1252-
new ReAttachPrtHeaderHandler(view, mRequestHeaders, span, getLoginHintFromRequestUrl(), true);
1253-
reAttachPrtHeader(url, reAttachPrtHeaderHandler, view, methodTag, span);
12541261
}
12551262

12561263
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)

‎common/src/test/java/com/microsoft/identity/common/internal/ui/webview/AzureActiveDirectoryWebViewClientEstsHostRedirectTest.java‎

Lines changed: 170 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
import com.microsoft.identity.common.adal.internal.AuthenticationConstants;
4040
import com.microsoft.identity.common.java.exception.ClientException;
4141
import com.microsoft.identity.common.internal.mocks.MockCommonFlightsManager;
42+
import com.microsoft.identity.common.internal.ui.webview.challengehandlers.ReAttachPrtHeaderHandler;
4243
import com.microsoft.identity.common.internal.ui.webview.challengehandlers.SwitchBrowserRequestHandler;
4344
import com.microsoft.identity.common.java.flighting.CommonFlight;
4445
import com.microsoft.identity.common.java.flighting.CommonFlightsManager;
@@ -57,6 +58,8 @@
5758

5859
import java.util.HashMap;
5960

61+
import io.opentelemetry.api.trace.Span;
62+
6063
/**
6164
* Tests for eSTS cloud host detection and PRT header re-attachment in
6265
* {@link AzureActiveDirectoryWebViewClient}.
@@ -113,125 +116,219 @@ public void cleanUp() {
113116
CommonFlightsManager.INSTANCE.resetFlightsManager();
114117
}
115118

116-
// ------- isEstsCloudHost() tests -------
119+
// ------- Flight DISABLED regression tests -------
120+
// When the ENABLE_PRT_HEADER_FOR_ESTS_HOST_REDIRECT flight is OFF, all eSTS host URLs
121+
// must fall through to the default path (return false) — exactly the same behavior as
122+
// before this feature existed. reAttachPrtHeader must never be called.
117123

118-
@Test
119-
public void isEstsCloudHost_returnsTrue_forKnownEstsHost() {
120-
assertTrue(mWebViewClient.isEstsCloudHost(TEST_ESTS_URL));
124+
/**
125+
* Helper: initializes the flights manager with the eSTS host PRT flight set to the given value.
126+
*/
127+
private void initFlightsWithEstsPrtFlight(boolean enabled) {
128+
final IFlightsProvider mockFlightsProvider = Mockito.mock(IFlightsProvider.class);
129+
when(mockFlightsProvider.isFlightEnabled(CommonFlight.ENABLE_PRT_HEADER_FOR_ESTS_HOST_REDIRECT))
130+
.thenReturn(enabled);
131+
final MockCommonFlightsManager mockCommonFlightsManager = new MockCommonFlightsManager();
132+
mockCommonFlightsManager.setMockCommonFlightsProvider(mockFlightsProvider);
133+
CommonFlightsManager.INSTANCE.initializeCommonFlightsManager(mockCommonFlightsManager);
121134
}
122135

136+
/**
137+
* When the flight is disabled, every URL variant must fall through (return false)
138+
* and reAttachPrtHeader must never be called — same behavior as before this feature existed.
139+
*/
123140
@Test
124-
public void isEstsCloudHost_returnsFalse_forNonEstsHost() {
125-
assertFalse(mWebViewClient.isEstsCloudHost(TEST_NON_ESTS_URL));
141+
public void flightDisabled_allUrlVariants_fallThrough_returnsFalse() {
142+
initFlightsWithEstsPrtFlight(false);
143+
144+
final String[][] urlsWithLabels = {
145+
{"eSTS host /authorize", TEST_ESTS_URL},
146+
{"eSTS /authorize with client_id", "https://login.microsoftonline.com/common/oauth2/v2.0/authorize?client_id=test-client-id&response_type=code"},
147+
{"China cloud", "https://login.chinacloudapi.cn/organizations/oauth2/v2.0/authorize?x=10"},
148+
{"US Gov cloud", "https://login.microsoftonline.us/organizations/oauth2/v2.0/authorize?x=10"},
149+
{"eSTS /token path", "https://login.microsoftonline.com/common/oauth2/v2.0/token?code=abc"},
150+
{"eSTS KMSI path", "https://login.microsoftonline.com/common/oauth2/v2.0/authorize/kmsi"},
151+
{"non-eSTS HTTPS", TEST_NON_ESTS_URL},
152+
};
153+
154+
final AzureActiveDirectoryWebViewClient spyClient = spy(mWebViewClient);
155+
for (final String[] entry : urlsWithLabels) {
156+
final boolean result = spyClient.shouldOverrideUrlLoading(mMockWebView, entry[1]);
157+
assertFalse(entry[0] + " must fall through when flight is disabled", result);
158+
}
159+
160+
// Across all URLs, reAttachPrtHeader must never have been invoked.
161+
verify(spyClient, never()).reAttachPrtHeader(
162+
Mockito.anyString(),
163+
Mockito.any(ReAttachPrtHeaderHandler.class),
164+
Mockito.any(WebView.class),
165+
Mockito.anyString(),
166+
Mockito.any(Span.class)
167+
);
126168
}
127169

170+
// ------- Flight ENABLED tests -------
171+
// When the flight is ON, shouldReAttachPrtForEstsHost returns true iff:
172+
// isEstsCloudHost(url) AND hasPrtHeaderAttached()
173+
// AND (!url.contains("/authorize") || hasKnownClientId(url))
174+
//
175+
// hasKnownClientId(url):
176+
// - redirectClientId == null → false
177+
// - redirectClientId matches originalClientId (from mRequestUrl) → true
178+
// - redirectClientId matches BROKER_CLIENT_ID → true
179+
// - exception during extraction → true
180+
181+
private static final String TEST_ORIGINAL_CLIENT_ID = "test-original-client-id";
182+
private static final String BROKER_CLIENT_ID = "29d9ed98-a469-4536-ade2-f981bc1d605e";
183+
184+
// -- Non-/authorize paths: should always be handled (no client_id check) --
185+
128186
@Test
129-
public void isEstsCloudHost_returnsFalse_forMalformedUrl() {
130-
assertFalse(mWebViewClient.isEstsCloudHost(TEST_MALFORMED_URL));
187+
public void flightEnabled_nonAuthorizePaths_handled() {
188+
initFlightsWithEstsPrtFlight(true);
189+
190+
final String[][] handledUrls = {
191+
{"eSTS /token endpoint", "https://login.microsoftonline.com/common/oauth2/v2.0/token?code=abc"},
192+
{"Sovereign cloud /token", "https://login.chinacloudapi.cn/common/oauth2/v2.0/token?code=abc"},
193+
};
194+
195+
final AzureActiveDirectoryWebViewClient spyClient = spy(mWebViewClient);
196+
for (final String[] entry : handledUrls) {
197+
final boolean result = spyClient.shouldOverrideUrlLoading(mMockWebView, entry[1]);
198+
assertTrue(entry[0] + " should be handled", result);
199+
}
200+
201+
// reAttachPrtHeader must have been called for each handled URL.
202+
verify(spyClient, Mockito.times(handledUrls.length)).reAttachPrtHeader(
203+
Mockito.anyString(),
204+
Mockito.any(ReAttachPrtHeaderHandler.class),
205+
Mockito.any(WebView.class),
206+
Mockito.anyString(),
207+
Mockito.any(Span.class)
208+
);
131209
}
132210

133-
// ------- handleUrl() flight-gating tests -------
211+
// -- /authorize path with known client_id (original or broker) → handled --
134212

135213
@Test
136-
public void handleUrl_whenFlightEnabled_andEstsHost_returnsTrue() {
137-
final IFlightsProvider mockFlightsProvider = Mockito.mock(IFlightsProvider.class);
138-
when(mockFlightsProvider.isFlightEnabled(CommonFlight.ENABLE_PRT_HEADER_FOR_ESTS_HOST_REDIRECT))
139-
.thenReturn(true);
140-
// Keep other flights at default behaviour — not enabled
141-
final MockCommonFlightsManager mockCommonFlightsManager = new MockCommonFlightsManager();
142-
mockCommonFlightsManager.setMockCommonFlightsProvider(mockFlightsProvider);
143-
CommonFlightsManager.INSTANCE.initializeCommonFlightsManager(mockCommonFlightsManager);
214+
public void flightEnabled_authorizePath_knownClientId_handled() {
215+
initFlightsWithEstsPrtFlight(true);
216+
217+
// Set request URL with a client_id so getClientIdFromRequestUrl() returns it.
218+
mWebViewClient.setRequestUrl(
219+
"https://login.microsoftonline.com/common/oauth2/v2.0/authorize?client_id=" + TEST_ORIGINAL_CLIENT_ID);
220+
221+
final String[][] handledUrls = {
222+
{"matching original client_id",
223+
"https://login.microsoftonline.com/common/oauth2/v2.0/authorize?client_id=" + TEST_ORIGINAL_CLIENT_ID + "&response_type=code"},
224+
{"broker client_id",
225+
"https://login.microsoftonline.com/common/oauth2/v2.0/authorize?client_id=" + BROKER_CLIENT_ID},
226+
};
144227

145228
final AzureActiveDirectoryWebViewClient spyClient = spy(mWebViewClient);
146-
final boolean result = spyClient.shouldOverrideUrlLoading(mMockWebView, TEST_ESTS_URL);
147-
assertTrue(result);
229+
for (final String[] entry : handledUrls) {
230+
final boolean result = spyClient.shouldOverrideUrlLoading(mMockWebView, entry[1]);
231+
assertTrue("/authorize with " + entry[0] + " should be handled", result);
232+
}
233+
234+
verify(spyClient, Mockito.times(handledUrls.length)).reAttachPrtHeader(
235+
Mockito.anyString(),
236+
Mockito.any(ReAttachPrtHeaderHandler.class),
237+
Mockito.any(WebView.class),
238+
Mockito.anyString(),
239+
Mockito.any(Span.class)
240+
);
148241
}
149242

243+
// -- /authorize path with unrecognized/missing client_id → falls through --
244+
150245
@Test
151-
public void handleUrl_whenFlightDisabled_andEstsHost_doesNotCallProcessEstsHostRedirect() {
152-
final IFlightsProvider mockFlightsProvider = Mockito.mock(IFlightsProvider.class);
153-
when(mockFlightsProvider.isFlightEnabled(CommonFlight.ENABLE_PRT_HEADER_FOR_ESTS_HOST_REDIRECT))
154-
.thenReturn(false);
155-
final MockCommonFlightsManager mockCommonFlightsManager = new MockCommonFlightsManager();
156-
mockCommonFlightsManager.setMockCommonFlightsProvider(mockFlightsProvider);
157-
CommonFlightsManager.INSTANCE.initializeCommonFlightsManager(mockCommonFlightsManager);
246+
public void flightEnabled_authorizePath_unrecognizedClientId_fallsThrough() {
247+
initFlightsWithEstsPrtFlight(true);
248+
249+
mWebViewClient.setRequestUrl(
250+
"https://login.microsoftonline.com/common/oauth2/v2.0/authorize?client_id=" + TEST_ORIGINAL_CLIENT_ID);
251+
252+
// All these /authorize URLs should fail the hasKnownClientId check:
253+
// - unknown client_id doesn't match original or broker
254+
// - missing client_id returns null → false
255+
// - KMSI path contains "/authorize" substring, and has no client_id
256+
final String[][] fallThroughUrls = {
257+
{"unknown client_id",
258+
"https://login.microsoftonline.com/common/oauth2/v2.0/authorize?client_id=unknown-app-id"},
259+
{"no client_id param",
260+
"https://login.microsoftonline.com/common/oauth2/v2.0/authorize?response_type=code"},
261+
{"KMSI path (contains /authorize, no client_id)",
262+
"https://login.microsoftonline.com/common/oauth2/v2.0/authorize/kmsi"},
263+
};
158264

159265
final AzureActiveDirectoryWebViewClient spyClient = spy(mWebViewClient);
160-
// When the flight is disabled, the eSTS host check is bypassed entirely;
161-
// the URL falls through to the unrecognized-URL path and returns false.
162-
final boolean result = spyClient.shouldOverrideUrlLoading(mMockWebView, TEST_ESTS_URL);
163-
assertFalse(result);
266+
for (final String[] entry : fallThroughUrls) {
267+
final boolean result = spyClient.shouldOverrideUrlLoading(mMockWebView, entry[1]);
268+
assertFalse(entry[0] + " should fall through", result);
269+
}
270+
271+
verify(spyClient, never()).reAttachPrtHeader(
272+
Mockito.anyString(),
273+
Mockito.any(ReAttachPrtHeaderHandler.class),
274+
Mockito.any(WebView.class),
275+
Mockito.anyString(),
276+
Mockito.any(Span.class)
277+
);
164278
}
165279

280+
// -- Non-eSTS host → falls through --
281+
166282
@Test
167-
public void handleUrl_whenFlightEnabled_andNonEstsHost_doesNotHandleAsEstsHostRedirect() {
168-
final IFlightsProvider mockFlightsProvider = Mockito.mock(IFlightsProvider.class);
169-
when(mockFlightsProvider.isFlightEnabled(CommonFlight.ENABLE_PRT_HEADER_FOR_ESTS_HOST_REDIRECT))
170-
.thenReturn(true);
171-
final MockCommonFlightsManager mockCommonFlightsManager = new MockCommonFlightsManager();
172-
mockCommonFlightsManager.setMockCommonFlightsProvider(mockFlightsProvider);
173-
CommonFlightsManager.INSTANCE.initializeCommonFlightsManager(mockCommonFlightsManager);
283+
public void flightEnabled_nonEstsHost_returnsFalse() {
284+
initFlightsWithEstsPrtFlight(true);
174285

175286
final AzureActiveDirectoryWebViewClient spyClient = spy(mWebViewClient);
176-
// Non-eSTS HTTPS URL should not be caught by the new branch — it falls through
177-
// to the unrecognized-URL path and returns false.
178287
final boolean result = spyClient.shouldOverrideUrlLoading(mMockWebView, TEST_NON_ESTS_URL);
179-
assertFalse(result);
288+
assertFalse("Non-eSTS host should fall through even with flight enabled", result);
180289
}
181290

291+
// -- No PRT header → falls through --
292+
182293
@Test
183-
public void handleUrl_whenFlightEnabled_andEstsHost_butNoPrtHeader_doesNotHandleAsEstsHostRedirect() {
184-
final IFlightsProvider mockFlightsProvider = Mockito.mock(IFlightsProvider.class);
185-
when(mockFlightsProvider.isFlightEnabled(CommonFlight.ENABLE_PRT_HEADER_FOR_ESTS_HOST_REDIRECT))
186-
.thenReturn(true);
187-
final MockCommonFlightsManager mockCommonFlightsManager = new MockCommonFlightsManager();
188-
mockCommonFlightsManager.setMockCommonFlightsProvider(mockFlightsProvider);
189-
CommonFlightsManager.INSTANCE.initializeCommonFlightsManager(mockCommonFlightsManager);
294+
public void flightEnabled_noPrtHeader_returnsFalse() {
295+
initFlightsWithEstsPrtFlight(true);
190296

191-
// Remove PRT header from request headers to simulate a flow that was not PRT-authenticated.
297+
// Remove PRT header from request headers.
192298
final HashMap<String, String> headersWithoutPrt = new HashMap<>();
193299
headersWithoutPrt.put("key", "value");
194300
mWebViewClient.setRequestHeaders(headersWithoutPrt);
195301

196302
final AzureActiveDirectoryWebViewClient spyClient = spy(mWebViewClient);
197-
// Without a PRT header in the initial request, the eSTS host redirect should fall through.
198303
final boolean result = spyClient.shouldOverrideUrlLoading(mMockWebView, TEST_ESTS_URL);
199-
assertFalse(result);
304+
assertFalse("Without PRT header, eSTS host redirect should fall through", result);
200305
}
201306

202307
// ------- setRequestUrl() / mLoginHint extraction tests -------
203308

204309
@Test
205310
public void setRequestUrl_extractsLoginHint_whenPresent() {
206-
// Verifies that setRequestUrl does not throw and populates mLoginHint by
207-
// checking indirectly: when the flight is enabled and the redirect goes to
208-
// an eSTS host, the handler is called (i.e., processEstsHostRedirect
209-
// was triggered), meaning the URL was handled.
210-
final IFlightsProvider mockFlightsProvider = Mockito.mock(IFlightsProvider.class);
211-
when(mockFlightsProvider.isFlightEnabled(CommonFlight.ENABLE_PRT_HEADER_FOR_ESTS_HOST_REDIRECT))
212-
.thenReturn(true);
213-
final MockCommonFlightsManager mockCommonFlightsManager = new MockCommonFlightsManager();
214-
mockCommonFlightsManager.setMockCommonFlightsProvider(mockFlightsProvider);
215-
CommonFlightsManager.INSTANCE.initializeCommonFlightsManager(mockCommonFlightsManager);
216-
217-
// Set request URL with login_hint — this also populates mLoginHint
218-
mWebViewClient.setRequestUrl(TEST_ESTS_URL_WITH_LOGIN_HINT);
219-
220-
// The eSTS host redirect should be handled by the new code path
311+
// Verifies that setRequestUrl populates mLoginHint. We check this indirectly:
312+
// set a request URL with login_hint AND client_id, then verify a redirect with
313+
// matching client_id is handled (which triggers processEstsHostRedirect with mLoginHint).
314+
initFlightsWithEstsPrtFlight(true);
315+
316+
mWebViewClient.setRequestUrl(
317+
"https://login.microsoftonline.com/common/oauth2/v2.0/authorize?client_id="
318+
+ TEST_ORIGINAL_CLIENT_ID + "&login_hint=user%40contoso.com");
319+
320+
// Redirect with matching client_id so shouldReAttachPrtForEstsHost → true.
321+
final String redirectUrl =
322+
"https://login.microsoftonline.com/common/oauth2/v2.0/authorize?client_id=" + TEST_ORIGINAL_CLIENT_ID;
221323
final AzureActiveDirectoryWebViewClient spyClient = spy(mWebViewClient);
222-
final boolean result = spyClient.shouldOverrideUrlLoading(mMockWebView, TEST_ESTS_URL);
324+
final boolean result = spyClient.shouldOverrideUrlLoading(mMockWebView, redirectUrl);
223325
assertTrue("Expected eSTS host redirect to be handled after login_hint extraction", result);
224326
}
225327

226328
@Test
227-
public void setRequestUrl_doesNotThrow_whenLoginHintAbsent() {
228-
// No exception expected when login_hint is not present in the URL.
329+
public void setRequestUrl_doesNotThrow_forEdgeCases() {
330+
// Neither missing login_hint nor malformed URL should throw.
229331
mWebViewClient.setRequestUrl(TEST_ESTS_URL);
230-
}
231-
232-
@Test
233-
public void setRequestUrl_doesNotThrow_forMalformedUrl() {
234-
// Should log a warning and not propagate the exception.
235332
mWebViewClient.setRequestUrl(TEST_MALFORMED_URL);
236333
}
237334
}

0 commit comments

Comments
 (0)