Refactor AuthTabStrategyProvider to use a single volatile Registration object for strategy and support checker, enhancing thread safety. Update tests to verify behavior with fallback strategies.

Author: p3dr0rv

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

@@ -35,36 +35,41 @@ import com.microsoft.identity.common.logging.Logger
  */
 object AuthTabStrategyProvider {
 
-    private val lock = Any()
-
-    @Volatile
-    private var strategyFactory: ((FragmentActivity, (Bundle) -> Unit) -> BrowserLaunchStrategy)? = null
+    /**
+     * Immutable holder so both callbacks are published atomically through a single
+     * volatile reference. This prevents readers from observing a half-registered
+     * state where, for example, [isAvailable] returns true while
+     * [isAuthTabSupported] still sees a null support checker.
+     */
+    private data class Registration(
+        val strategyFactory: (FragmentActivity, (Bundle) -> Unit) -> BrowserLaunchStrategy,
+        val supportChecker: (Context, String) -> Boolean
+    )
 
     @Volatile
-    private var supportChecker: ((Context, String) -> Boolean)? = null
+    private var registration: Registration? = null
 
     private val tag = AuthTabStrategyProvider::class.java.simpleName
 
     /**
      * Registers Auth Tab strategy creation and support checking callbacks.
+     *
+     * Both callbacks become visible to other threads atomically.
      */
     fun register(
         factory: (FragmentActivity, (Bundle) -> Unit) -> BrowserLaunchStrategy,
         isSupported: (Context, String) -> Boolean
     ) {
-        synchronized(lock) {
-            strategyFactory = factory
-            supportChecker = isSupported
-        }
+        registration = Registration(factory, isSupported)
         Logger.info("$tag:register", "Auth Tab strategy provider registered")
     }
 
     /**
      * Returns true if Auth Tab is supported for the provided browser package.
      */
     fun isAuthTabSupported(context: Context, browserPackage: String): Boolean {
-        val checker = supportChecker ?: return false
-        return checker(context, browserPackage)
+        val current = registration ?: return false
+        return current.supportChecker(context, browserPackage)
     }
 
     /**
@@ -74,19 +79,16 @@ object AuthTabStrategyProvider {
         activity: FragmentActivity,
         onResult: (Bundle) -> Unit
     ): BrowserLaunchStrategy? {
-        val factory = strategyFactory ?: return null
-        return factory(activity, onResult)
+        val current = registration ?: return null
+        return current.strategyFactory(activity, onResult)
     }
 
     /**
      * Returns true if an Auth Tab strategy factory has been registered.
      */
-    fun isAvailable(): Boolean = strategyFactory != null
+    fun isAvailable(): Boolean = registration != null
 
     internal fun resetForTest() {
-        synchronized(lock) {
-            strategyFactory = null
-            supportChecker = null
-        }
+        registration = null
     }
 }

common/src/test/java/com/microsoft/identity/common/internal/providers/oauth2/SwitchBrowserActivityTest.kt

@@ -80,6 +80,7 @@ class SwitchBrowserActivityTest {
     @Test
     fun onCreate_fallsBackToCustomTabsStrategy_whenBrowserPackageMissing() {
         mockkObject(AuthTabStrategyProvider)
+        every { AuthTabStrategyProvider.isAuthTabSupported(any(), any()) } returns false
         mockkConstructor(CustomTabsLaunchStrategy::class)
         every { anyConstructed<CustomTabsLaunchStrategy>().launch() } just runs
 
@@ -90,7 +91,31 @@ class SwitchBrowserActivityTest {
 
         Robolectric.buildActivity(SwitchBrowserActivity::class.java, intentWithoutPackage).create()
 
-        verify(exactly = 0) { AuthTabStrategyProvider.isAuthTabSupported(any(), any()) }
+        // isAuthTabSupported is called but returns false (package is empty)
+        verify(exactly = 1) { AuthTabStrategyProvider.isAuthTabSupported(any(), "") }
+        // createStrategy should NOT be called because browserPackageName.isNotBlank() fails
+        verify(exactly = 0) { AuthTabStrategyProvider.createStrategy(any(), any()) }
+        // CustomTabs strategy is used as fallback
+        verify(exactly = 1) { anyConstructed<CustomTabsLaunchStrategy>().launch() }
+    }
+
+    @Test
+    fun onCreate_fallsBackToCustomTabsStrategy_whenCreateStrategyReturnsNull() {
+        mockkObject(AuthTabStrategyProvider)
+        // isAuthTabSupported returns true, but createStrategy returns null
+        // This can occur if the provider is misconfigured or fails to create a strategy
+        every { AuthTabStrategyProvider.isAuthTabSupported(any(), any()) } returns true
+        every { AuthTabStrategyProvider.createStrategy(any(), any()) } returns null
+        mockkConstructor(CustomTabsLaunchStrategy::class)
+        every { anyConstructed<CustomTabsLaunchStrategy>().launch() } just runs
+
+        Robolectric.buildActivity(SwitchBrowserActivity::class.java, getIntent()).create()
+
+        // Verify that isAuthTabSupported was called
+        verify(exactly = 1) { AuthTabStrategyProvider.isAuthTabSupported(any(), any()) }
+        // Verify that createStrategy was called
+        verify(exactly = 1) { AuthTabStrategyProvider.createStrategy(any(), any()) }
+        // Verify that CustomTabsLaunchStrategy.launch() was called as fallback
         verify(exactly = 1) { anyConstructed<CustomTabsLaunchStrategy>().launch() }
     }
 

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

@@ -59,6 +59,7 @@ class SwitchBrowserRequestHandlerTest {
         val challenge = mock(SwitchBrowserChallenge::class.java)
         `when`(challenge.processUri).thenReturn(Uri.parse("https://login.microsoft.com?state=123"))
         `when`(challenge.authorizationUrl).thenReturn("https://auth.com?state=123")
+        `when`(challenge.redirectUri).thenReturn("https://myapp.example.com/callback")
         val browserSelector = // Browser available
             IBrowserSelector { _, _ -> Browser("fakeBrowser", emptySet(), "browser", false) }
         val handler = SwitchBrowserRequestHandler(mockActivity, browserSelector, null)
@@ -79,6 +80,7 @@ class SwitchBrowserRequestHandlerTest {
         val challenge = mock(SwitchBrowserChallenge::class.java)
         `when`(challenge.processUri).thenReturn(Uri.parse("https://login.microsoft.com"))
         `when`(challenge.authorizationUrl).thenReturn("https://auth.com")
+        `when`(challenge.redirectUri).thenReturn("https://myapp.example.com/callback")
         val browserSelector = // Browser available
             IBrowserSelector { _, _ -> Browser("fakeBrowser", emptySet(), "browser", false) }
         val handler = SwitchBrowserRequestHandler(mockActivity, browserSelector, null)
@@ -94,6 +96,7 @@ class SwitchBrowserRequestHandlerTest {
         val challenge = mock(SwitchBrowserChallenge::class.java)
         `when`(challenge.processUri).thenReturn(Uri.parse("https://login.microsoft.com?state=123"))
         `when`(challenge.authorizationUrl).thenReturn("https://auth.com?state=123")
+        `when`(challenge.redirectUri).thenReturn("https://myapp.example.com/callback")
         val browserSelector = IBrowserSelector { _, _ -> null } // No browser available
         val handler = SwitchBrowserRequestHandler(activity, browserSelector, null)
         val exception = Assert.assertThrows(ClientException::class.java) {
@@ -111,6 +114,7 @@ class SwitchBrowserRequestHandlerTest {
         val challenge = mock(SwitchBrowserChallenge::class.java)
         `when`(challenge.processUri).thenReturn(Uri.parse("https://login.microsoft.com?state=123"))
         `when`(challenge.authorizationUrl).thenReturn("https://auth.com?state=456")
+        `when`(challenge.redirectUri).thenReturn("https://myapp.example.com/callback")
         val browserSelector = // Browser available
             IBrowserSelector { _, _ -> Browser("fakeBrowser", emptySet(), "browser", false) }
         val handler = SwitchBrowserRequestHandler(mockActivity, browserSelector, null)