Updating handling of ssl error received in Android WebView's onReceivedSslError callback, Fixes AB#3268908 (#2691)

Update:
After talking to @iulico-1 from OneAuth.
1. First rolling out with Broker and keeping disabled for OneAuth (and
MSAL Android). Added SSL error telemetry
2. Enable for oneauth after we see no issues in Broker. So, the toggle
code to let OneAuth enable/disable is removed from the PR.
3. Investigate how we can pass the error to OneAuth to track SSL failure
and User canceled the flow because of SSL error.

=====================

Fixes
[AB#3268908](https://identitydivision.visualstudio.com/fac9d424-53d2-45c0-91b5-ef6ba7a6bf26/_workitems/edit/3268908)

**Summary:**  
This PR updates the handling of SSL errors in the Android WebView used
for authentication. The change is in response to an incident where the
MSA sign-up flow failed due to an expired certificate on an MSA UX web
page. The expired certificate was associated with a resource loaded by
the page; however, the existing SSL error handling in Common’s WebView
would cancel the loading of that resource and subsequently terminate the
entire authentication flow. (See PBI for details.)

**Solution**
**Option 1**
We simply log and stop loading the resource that caused the error by
calling `SssHandler.cancel()` but not stop the flow by erroring out.
This is a simple change. The only thing is the main URL (the url loading
in the WebView) itself has SSL error then the flow would end up in
*blank page*. This is as bad as finishing the flow but does not inform
calling app of the error via exception.

But if sub-resource has the error, then the flow continues.

**Flighting**
**New SSL Error Handler**: 
- ~~Adds a new intent key (`WEB_VIEW_NEW_SSL_ERROR_HANDLER_ENABLED`) in
`AuthenticationConstants.java` to control whether the new SSL error
handler logic is enabled. This can be used by OneAuth to opt-in or
opt-out. If the key is not passed, then Common's default logic based on
flighting will be used. If the key passed, that will be used (primarily
meant for OneAuth/MSALCPP).~~. The toggle would be added later if
required. This simplifies current logic to only rely on Common/broker's
flighting.
- Adds a new flight value is well defaulting to `false`. So, OneAuth and
MSAL would also use default flight value and continue using current
behavior. For Broker, we would enable new flow with default `true`
(AzureAD/ad-accounts-for-android#3141), which
can be controlled remotely to turn off if required.

**Option 2** (Not this PR, added for reference)
The objective of this update is to improve SSL error handling so that
the flow is only cancelled if an SSL error occurs on the main frame
resource (the primary page being loaded). If an SSL error affects a
sub-resource, only the loading of that specific resource is cancelled,
and the overall authentication flow continues. This approach helps
prevent unnecessary failures caused by SSL errors on secondary or
non-critical resources.
(#2688)

The con of the approach is client needs to track the active url that's
about and being loaded in WebView. The tracking adds cost of developing
and maintaining to solution without significant benefit over option 1.

Author: mohitc1

changelog.txt

@@ -1,5 +1,6 @@
 vNext
 ----------
+- [MINOR] Updating handling of ssl error received in Android WebView's onReceivedSslError callback (#2691)
 - [MINOR] Fixing the sign in screens when edge to edge is enabled (#2665)
 - [MINOR] Showing webcp flow in webview (#2673)
 - [MINOR] Native auth: Make native auth MFA feature more backward compatible(#2669)

common/src/main/java/com/microsoft/identity/common/internal/providers/oauth2/WebViewAuthorizationFragment.java

@@ -22,6 +22,16 @@
 // THE SOFTWARE.
 package com.microsoft.identity.common.internal.providers.oauth2;
 
+import static com.microsoft.identity.common.adal.internal.AuthenticationConstants.AuthorizationIntentKey.AUTH_INTENT;
+import static com.microsoft.identity.common.adal.internal.AuthenticationConstants.AuthorizationIntentKey.POST_PAGE_LOADED_URL;
+import static com.microsoft.identity.common.adal.internal.AuthenticationConstants.AuthorizationIntentKey.REDIRECT_URI;
+import static com.microsoft.identity.common.adal.internal.AuthenticationConstants.AuthorizationIntentKey.REQUEST_HEADERS;
+import static com.microsoft.identity.common.adal.internal.AuthenticationConstants.AuthorizationIntentKey.REQUEST_URL;
+import static com.microsoft.identity.common.adal.internal.AuthenticationConstants.AuthorizationIntentKey.WEB_VIEW_ZOOM_CONTROLS_ENABLED;
+import static com.microsoft.identity.common.adal.internal.AuthenticationConstants.AuthorizationIntentKey.WEB_VIEW_ZOOM_ENABLED;
+import static com.microsoft.identity.common.java.AuthenticationConstants.SdkPlatformFields.PRODUCT;
+import static com.microsoft.identity.common.java.AuthenticationConstants.SdkPlatformFields.VERSION;
+
 import android.annotation.SuppressLint;
 import android.app.Activity;
 import android.content.Context;
@@ -46,40 +56,30 @@
 import androidx.fragment.app.FragmentActivity;
 
 import com.microsoft.identity.common.R;
-import com.microsoft.identity.common.internal.fido.LegacyFidoActivityResultContract;
-import com.microsoft.identity.common.internal.fido.LegacyFido2ApiObject;
-import com.microsoft.identity.common.internal.ui.webview.ISendResultCallback;
-import com.microsoft.identity.common.internal.ui.webview.ProcessUtil;
-import com.microsoft.identity.common.internal.ui.webview.switchbrowser.SwitchBrowserProtocolCoordinator;
-import com.microsoft.identity.common.java.WarningType;
 import com.microsoft.identity.common.adal.internal.AuthenticationConstants;
 import com.microsoft.identity.common.adal.internal.util.StringExtensions;
+import com.microsoft.identity.common.internal.fido.LegacyFido2ApiObject;
+import com.microsoft.identity.common.internal.fido.LegacyFidoActivityResultContract;
 import com.microsoft.identity.common.internal.ui.webview.AzureActiveDirectoryWebViewClient;
+import com.microsoft.identity.common.internal.ui.webview.ISendResultCallback;
 import com.microsoft.identity.common.internal.ui.webview.OnPageLoadedCallback;
+import com.microsoft.identity.common.internal.ui.webview.ProcessUtil;
 import com.microsoft.identity.common.internal.ui.webview.WebViewUtil;
+import com.microsoft.identity.common.internal.ui.webview.switchbrowser.SwitchBrowserProtocolCoordinator;
+import com.microsoft.identity.common.java.WarningType;
 import com.microsoft.identity.common.java.constants.FidoConstants;
 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.ui.webview.authorization.IAuthorizationCompletionCallback;
 import com.microsoft.identity.common.java.providers.RawAuthorizationResult;
+import com.microsoft.identity.common.java.ui.webview.authorization.IAuthorizationCompletionCallback;
 import com.microsoft.identity.common.java.util.ClientExtraSku;
 import com.microsoft.identity.common.logging.Logger;
 
-import java.util.Arrays;
-import java.util.HashMap;
 import java.io.UnsupportedEncodingException;
 import java.net.URLEncoder;
-
-import static com.microsoft.identity.common.adal.internal.AuthenticationConstants.AuthorizationIntentKey.AUTH_INTENT;
-import static com.microsoft.identity.common.adal.internal.AuthenticationConstants.AuthorizationIntentKey.POST_PAGE_LOADED_URL;
-import static com.microsoft.identity.common.adal.internal.AuthenticationConstants.AuthorizationIntentKey.REDIRECT_URI;
-import static com.microsoft.identity.common.adal.internal.AuthenticationConstants.AuthorizationIntentKey.REQUEST_HEADERS;
-import static com.microsoft.identity.common.adal.internal.AuthenticationConstants.AuthorizationIntentKey.REQUEST_URL;
-import static com.microsoft.identity.common.adal.internal.AuthenticationConstants.AuthorizationIntentKey.WEB_VIEW_ZOOM_CONTROLS_ENABLED;
-import static com.microsoft.identity.common.adal.internal.AuthenticationConstants.AuthorizationIntentKey.WEB_VIEW_ZOOM_ENABLED;
-import static com.microsoft.identity.common.java.AuthenticationConstants.SdkPlatformFields.PRODUCT;
-import static com.microsoft.identity.common.java.AuthenticationConstants.SdkPlatformFields.VERSION;
+import java.util.Arrays;
+import java.util.HashMap;
 
 import io.opentelemetry.api.trace.SpanContext;
 

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

@@ -22,6 +22,8 @@
 // THE SOFTWARE.
 package com.microsoft.identity.common.internal.ui.webview;
 
+import static com.microsoft.identity.common.adal.internal.AuthenticationConstants.Browser.SSL_HELP_URL;
+
 import android.app.Activity;
 import android.graphics.Bitmap;
 import android.net.Uri;
@@ -41,23 +43,33 @@
 
 import com.microsoft.identity.common.adal.internal.AuthenticationConstants;
 import com.microsoft.identity.common.internal.ui.webview.challengehandlers.ChallengeFactory;
-import com.microsoft.identity.common.java.ui.webview.authorization.IAuthorizationCompletionCallback;
 import com.microsoft.identity.common.internal.ui.webview.challengehandlers.IChallengeHandler;
 import com.microsoft.identity.common.internal.ui.webview.challengehandlers.NtlmChallenge;
 import com.microsoft.identity.common.internal.ui.webview.challengehandlers.NtlmChallengeHandler;
 import com.microsoft.identity.common.internal.util.StringUtil;
 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.logging.DiagnosticContext;
+import com.microsoft.identity.common.java.opentelemetry.AttributeName;
+import com.microsoft.identity.common.java.opentelemetry.OTelUtility;
 import com.microsoft.identity.common.java.providers.RawAuthorizationResult;
+import com.microsoft.identity.common.java.ui.webview.authorization.IAuthorizationCompletionCallback;
 import com.microsoft.identity.common.logging.Logger;
 
-import static com.microsoft.identity.common.adal.internal.AuthenticationConstants.Browser.SSL_HELP_URL;
-
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+import io.opentelemetry.api.common.Attributes;
+import io.opentelemetry.api.metrics.LongCounter;
 
 public abstract class OAuth2WebViewClient extends WebViewClient {
     /* constants */
     private static final String TAG = OAuth2WebViewClient.class.getSimpleName();
 
+    private static final LongCounter sWebViewSslErrorCount = OTelUtility.createLongCounter(
+            "web_view_ssl_error_count",
+            "Number of SSL errors received in onReceivedSslError"
+    );
+
     private final IAuthorizationCompletionCallback mCompletionCallback;
     private final OnPageLoadedCallback mPageLoadedCallback;
     private final Activity mActivity;
@@ -89,12 +101,13 @@ IAuthorizationCompletionCallback getCompletionCallback() {
      */
     OAuth2WebViewClient(@NonNull final Activity activity,
                         @NonNull final IAuthorizationCompletionCallback completionCallback,
-                        @NonNull final OnPageLoadedCallback pageLoadedCallback) {
-        //the validation of redirect url and authorization request should be in upper level before launching the webview.
+                        @NonNull final OnPageLoadedCallback pageLoadedCallback
+    ) {
+        // the validation of redirect url and authorization request should be in upper level before launching the webview.
         mActivity = activity;
         mCompletionCallback = completionCallback;
         mPageLoadedCallback = pageLoadedCallback;
-     }
+    }
 
     @Override
     public void onReceivedHttpAuthRequest(WebView view, final HttpAuthHandler handler,
@@ -171,17 +184,21 @@ public void onReceivedSslError(final WebView view,
                                    final SslErrorHandler handler,
                                    final SslError error) {
         // Developer does not have option to control this for now
+        final String methodTag = TAG + ":onReceivedSslError";
         super.onReceivedSslError(view, handler, error);
-        handler.cancel();
-
-        final String errMsg = "Received SSL Error during request. For more info see: " + SSL_HELP_URL;
-
-        Logger.error(TAG + ":onReceivedSslError", errMsg, null);
-
-        // Send the result back to the calling activity
-        mCompletionCallback.onChallengeResponseReceived(
-                RawAuthorizationResult.fromException(
-                        new ClientException("Code:" + ERROR_FAILED_SSL_HANDSHAKE, error.toString())));
+        final String errMsg = "Received SSL Error during request. For more info see: " + SSL_HELP_URL + ". Error: " + error.toString();
+
+        Logger.warn(methodTag, errMsg);
+        final Attributes attributes = Attributes.builder()
+                .put(AttributeName.web_view_ssl_primary_error_code.name(), error.getPrimaryError())
+                .build();
+        sWebViewSslErrorCount.add(1, attributes);
+        if (!CommonFlightsManager.INSTANCE.getFlightsProvider().isFlightEnabled(CommonFlight.SHOULD_PRESERVE_WEBVIEW_FLOW_ON_SSL_ERROR)) {
+            // Send the result back to the calling activity
+            mCompletionCallback.onChallengeResponseReceived(
+                    RawAuthorizationResult.fromException(
+                            new ClientException("Code:" + ERROR_FAILED_SSL_HANDSHAKE, error.toString())));
+        }
     }
 
     @Override

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

@@ -22,8 +22,20 @@
 // THE SOFTWARE.
 package com.microsoft.identity.common.internal.ui.webview;
 
+import static com.microsoft.identity.common.adal.internal.AuthenticationConstants.Broker.AUTHENTICATOR_MFA_LINKING_PREFIX;
+import static com.microsoft.identity.common.adal.internal.AuthenticationConstants.Broker.COMPANY_PORTAL_APP_PACKAGE_NAME;
+import static com.microsoft.identity.common.adal.internal.AuthenticationConstants.Broker.PLAY_STORE_INSTALL_PREFIX;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.when;
+
 import android.app.Activity;
 import android.content.Context;
+import android.net.http.SslError;
+import android.webkit.SslErrorHandler;
 import android.webkit.WebView;
 
 import androidx.annotation.NonNull;
@@ -33,14 +45,15 @@
 import com.microsoft.identity.common.internal.mocks.MockCommonFlightsManager;
 import com.microsoft.identity.common.internal.ui.DualScreenActivity;
 import com.microsoft.identity.common.internal.ui.webview.challengehandlers.ReAttachPrtHeaderHandler;
+import com.microsoft.identity.common.internal.ui.webview.challengehandlers.SwitchBrowserRequestHandler;
 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.flighting.IFlightsManager;
 import com.microsoft.identity.common.java.flighting.IFlightsProvider;
+import com.microsoft.identity.common.java.providers.RawAuthorizationResult;
 import com.microsoft.identity.common.java.providers.microsoft.azureactivedirectory.AzureActiveDirectory;
-import com.microsoft.identity.common.internal.ui.webview.challengehandlers.SwitchBrowserRequestHandler;
 import com.microsoft.identity.common.java.ui.webview.authorization.IAuthorizationCompletionCallback;
-import com.microsoft.identity.common.java.providers.RawAuthorizationResult;
 import com.microsoft.identity.common.shadows.ShadowProcessUtil;
 
 import org.junit.Assert;
@@ -52,18 +65,13 @@
 import org.robolectric.RobolectricTestRunner;
 import org.robolectric.annotation.Config;
 
-import static com.microsoft.identity.common.adal.internal.AuthenticationConstants.Broker.AUTHENTICATOR_MFA_LINKING_PREFIX;
-import static com.microsoft.identity.common.adal.internal.AuthenticationConstants.Broker.COMPANY_PORTAL_APP_PACKAGE_NAME;
-import static com.microsoft.identity.common.adal.internal.AuthenticationConstants.Broker.PLAY_STORE_INSTALL_PREFIX;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
-import static org.mockito.Mockito.when;
-
 import java.util.HashMap;
 
 import io.opentelemetry.api.trace.Span;
 
-
+/**
+ * Tests for {@link AzureActiveDirectoryWebViewClient}.
+ */
 @RunWith(RobolectricTestRunner.class)
 public class AzureActiveDirectoryWebViewClientTest {
     private WebView mMockWebView;
@@ -129,7 +137,8 @@ public void onPageLoaded(final String url) {
                     }
                 },
                 TEST_REDIRECT_URI,
-                Mockito.mock(SwitchBrowserRequestHandler.class));
+                Mockito.mock(SwitchBrowserRequestHandler.class)
+        );
         HashMap<String, String> dummyHeaders = new HashMap<>();
         dummyHeaders.put("key", "value");
         mWebViewClient.setRequestHeaders(dummyHeaders);
@@ -374,12 +383,67 @@ public void onPageLoaded(final String url) {
                         }
                     },
                     TEST_REDIRECT_URI,
-                    Mockito.mock(SwitchBrowserRequestHandler.class));
+                    Mockito.mock(SwitchBrowserRequestHandler.class)
+            );
             mWebViewClient.shouldOverrideUrlLoading(mMockWebView, TEST_PASSKEY_REDIRECT_URL);
         } catch (ClassCastException e) {
             Assert.fail("Failure is not expected. The class checks should have prevented this." + e);
         } catch (Exception e) {
             Assert.fail("Failure is not expected." + e);
         }
     }
+
+    @Test
+    public void testOnReceivedSslError_Legacy() {
+        final String mockActiveUrl = "https://login.microsoftonline.com/organizations/oAuth2/v2.0/authorize";
+        final SslErrorHandler mockHandler = Mockito.mock(android.webkit.SslErrorHandler.class);
+        final SslError mockError = Mockito.mock(android.net.http.SslError.class);
+        final IAuthorizationCompletionCallback mockCallback = Mockito.mock(IAuthorizationCompletionCallback.class);
+        when(mockError.getUrl()).thenReturn("https://example.com");
+        final AzureActiveDirectoryWebViewClient mockWebViewClient = new AzureActiveDirectoryWebViewClient(
+                mActivity,
+                mockCallback,
+                url -> {},
+                TEST_REDIRECT_URI,
+                Mockito.mock(SwitchBrowserRequestHandler.class));
+        final WebView mockWebView = new WebView(mContext);
+        mockWebView.setWebViewClient(mockWebViewClient);
+
+        // act
+        mockWebViewClient.onReceivedSslError(mockWebView, mockHandler, mockError);
+
+        Mockito.verify(mockHandler, Mockito.times(1)).cancel();
+        Mockito.verify(mockCallback, Mockito.times(1)).onChallengeResponseReceived(any());
+    }
+
+    @Test
+    public void testOnReceivedSslError() {
+        final IFlightsManager mockFlightsManager = Mockito.mock(IFlightsManager.class);
+        final IFlightsProvider mockFlightsProvider = Mockito.mock(IFlightsProvider.class);
+        when(mockFlightsProvider.isFlightEnabled(eq(CommonFlight.SHOULD_PRESERVE_WEBVIEW_FLOW_ON_SSL_ERROR))).thenReturn(true);
+        when(mockFlightsManager.getFlightsProvider()).thenReturn(mockFlightsProvider);
+        CommonFlightsManager.INSTANCE.initializeCommonFlightsManager(mockFlightsManager);
+        final SslErrorHandler mockHandler = Mockito.mock(android.webkit.SslErrorHandler.class);
+        final SslError mockError = Mockito.mock(android.net.http.SslError.class);
+        final IAuthorizationCompletionCallback mockCallback = Mockito.mock(IAuthorizationCompletionCallback.class);
+        when(mockError.getUrl()).thenReturn("https://example.com");
+        final AzureActiveDirectoryWebViewClient mockWebViewClient = new AzureActiveDirectoryWebViewClient(
+                mActivity,
+                mockCallback,
+                url -> {},
+                TEST_REDIRECT_URI,
+                Mockito.mock(SwitchBrowserRequestHandler.class)
+        );
+        final WebView mockWebView = new WebView(mContext);
+        mockWebView.setWebViewClient(mockWebViewClient);
+
+        // act
+        mockWebViewClient.onReceivedSslError(mockWebView, mockHandler, mockError);
+
+        // verify that the handler is cancelled and the callback is invoked
+        Mockito.verify(mockHandler, Mockito.times(1)).cancel();
+        Mockito.verify(mockCallback, never()).onChallengeResponseReceived(any());
+
+        CommonFlightsManager.INSTANCE.resetFlightsManager();
+    }
 }

common4j/src/main/com/microsoft/identity/common/java/flighting/CommonFlight.java

@@ -139,7 +139,13 @@ public enum CommonFlight implements IFlightConfig {
     /**
      * Flight to enable the Web CP for a tenant list.
      */
-    TENANT_LIST_TO_ENABLE_WEB_CP_IN_WEBVIEW("TenantListToEnableWebCpInWebView", "");
+    TENANT_LIST_TO_ENABLE_WEB_CP_IN_WEBVIEW("TenantListToEnableWebCpInWebView", ""),
+
+    /**
+     * Flight to enable the WebView flow to not cancel and preserve WebView flow on SSL errors.
+     * The web resource running into SSL will itself not be loaded.
+     */
+    SHOULD_PRESERVE_WEBVIEW_FLOW_ON_SSL_ERROR("ShouldPreserveWebViewFlowOnSslError", false);
 
     private String key;
     private Object defaultValue;

common4j/src/main/com/microsoft/identity/common/java/opentelemetry/AttributeName.java

@@ -378,5 +378,11 @@ public enum AttributeName {
     /**
      * Records if the webcp is enabled in webview.
      */
-    is_webcp_in_webview_enabled
+    is_webcp_in_webview_enabled,
+
+    /**
+     * Records the if webview received an SSL error and
+     * corresponding primary error code.
+     */
+    web_view_ssl_primary_error_code
 }