Fix ABBA deadlock between AzureActiveDirectory and AzureActiveDirectoryAuthority class monitors, Fixes AB#3578299

changelog.txt

@@ -1,5 +1,6 @@
 vNext
 ----------
+- [PATCH] Fix ABBA deadlock between AzureActiveDirectory and AzureActiveDirectoryAuthority class monitors by extracting polymorphic getAuthorityURL() calls outside synchronized scopes and removing unnecessary synchronized from ConcurrentHashMap read-only methods (#3082)
 - [PATCH] Optimize AcquireTokenSilent save path: replace keySet() decrypt-all with in-memory map lookup in removeAccount()/removeCredential(), add telemetry for deleteAccessTokensWithIntersectingScopes, and remove unused elapsed_time_save_account_shared_preferences attribute (#3074)
 - [MINOR] Add DeviceRegistrationClientApplication as public API for OneAuth device registration with mandatory correlationId, DeviceState and DrsDiscoveryEndpoint enums (#3073)
 - [MINOR] Move device registration protocol types, domain types, controller, and packer from broker to common to enable OneAuth device registration support (#3066)

common4j/src/main/com/microsoft/identity/common/java/authorities/Authority.java

@@ -318,40 +318,52 @@ public static void addKnownAuthorities(List<Authority> authorities) {
     }
 
     /**
-     * Authorities are either known by the developer and communicated to the library via configuration or they
-     * are known to Microsoft based on the list of clouds returned from:
+     * Clears all known authorities. For test use only.
+     */
+    // Visible for testing
+    static void clearKnownAuthorities() {
+        synchronized (sLock) {
+            knownAuthorities.clear();
+        }
+    }
+
+    /**
+     * Authorities are either known by the developer and communicated to the library via
+     * configuration, or they are known to Microsoft based on the cloud discovery metadata cache.
      *
      * @param authority Authority to check against.
      * @return True if the authority is known to Microsoft or defined in the configuration.
      */
     public static boolean isKnownAuthority(Authority authority) {
-        final String methodName = ":isKnownAuthority";
+        final String methodTag = TAG + ":isKnownAuthority";
         boolean knownToDeveloper = false;
         boolean knownToMicrosoft;
 
         if (authority == null) {
-            Logger.warn(
-                    TAG + methodName,
-                    "Authority is null"
-            );
+            Logger.warn(methodTag, "Authority is null");
+            return false;
+        }
+
+        // Resolve the authority URL outside any lock to avoid calling a potentially-
+        // synchronized polymorphic method (e.g. AzureActiveDirectoryAuthority.getAuthorityURL())
+        // while holding sLock, which could create a lock-ordering inversion.
+        final URL authorityUrl = authority.getAuthorityURL();
+
+        if (authorityUrl == null) {
+            Logger.warn(methodTag, "Authority URL is null");
             return false;
         }
 
-        if (BuildConfig.ALLOW_ONEBOX_AUTHORITIES && AzureActiveDirectoryEnvironment.ONEBOX_AUTHORITY.equals(authority.getAuthorityURL().getAuthority())) {
+        if (BuildConfig.ALLOW_ONEBOX_AUTHORITIES && AzureActiveDirectoryEnvironment.ONEBOX_AUTHORITY.equals(authorityUrl.getAuthority())) {
             return true; // onebox authorities are always considered to be known.
         }
 
-        //Check if authority was added to configuration
         synchronized (sLock) {
             for (final Authority currentAuthority : knownAuthorities) {
                 if (currentAuthority.mAuthorityUrlString != null &&
-                        authority.getAuthorityURL() != null &&
-                        authority.getAuthorityURL().getAuthority() != null &&
+                        authorityUrl.getAuthority() != null &&
                         currentAuthority.mAuthorityUrlString.toLowerCase(Locale.ROOT).contains(
-                                authority
-                                        .getAuthorityURL()
-                                        .getAuthority()
-                                        .toLowerCase(Locale.ROOT))) {
+                                authorityUrl.getAuthority().toLowerCase(Locale.ROOT))) {
                     knownToDeveloper = true;
                     break;
                 }
@@ -360,19 +372,14 @@ public static boolean isKnownAuthority(Authority authority) {
 
         // Check whether the authority is known to Microsoft or not.  Microsoft can recognize authorities that exist within public clouds.
         // Microsoft does not maintain a list of B2C authorities or a list of ADFS or 3rd party authorities (issuers).
-        knownToMicrosoft = AzureActiveDirectory.hasCloudHost(authority.getAuthorityURL());
+        knownToMicrosoft = AzureActiveDirectory.hasCloudHost(authorityUrl);
 
         final boolean isKnown = (knownToDeveloper || knownToMicrosoft);
 
-        Logger.verbose(
-                TAG + methodName,
-                "Authority is known to developer? [" + knownToDeveloper + "]"
-        );
-
-        Logger.verbose(
-                TAG + methodName,
-                "Authority is known to Microsoft? [" + knownToMicrosoft + "]"
-        );
+        Logger.verbose(methodTag,
+                "Authority is known to developer? [" + knownToDeveloper + "]");
+        Logger.verbose(methodTag,
+                "Authority is known to Microsoft? [" + knownToMicrosoft + "]");
 
         return isKnown;
     }
@@ -383,37 +390,43 @@ public static KnownAuthorityResult getKnownAuthorityResult(Authority authority)
                 methodTag,
                 "Getting known authority result..."
         );
-        ClientException clientException = null;
-        boolean known = false;
 
+        ClientException discoveryException = null;
         try {
             AzureActiveDirectory.ensureCloudDiscoveryForAuthority(authority);
             Logger.info(methodTag, "Cloud discovery complete.");
         } catch (final ClientException ex) {
             // Cloud discovery failed (e.g. network error).
             // Log but continue — the authority may still be known via hardcoded
             // metadata or developer configuration.
+            discoveryException = ex;
             Logger.warn(methodTag,
                     "Cloud discovery failed, will check hardcoded/configured authorities. Error: "
                             + ex.getErrorCode());
         }
 
-        if (!isKnownAuthority(authority)) {
-            clientException = new ClientException(
+        if (isKnownAuthority(authority)) {
+            Logger.info(methodTag, "Authority is known.");
+            return new KnownAuthorityResult(true, null);
+        }
+
+        // Authority is not known via any source.
+        if (discoveryException != null) {
+            // Discovery failed — propagate the original error
+            return new KnownAuthorityResult(false, discoveryException);
+        } else {
+            // Discovery succeeded but authority is not in any known list.
+            final ClientException clientException = new ClientException(
                     ClientException.UNKNOWN_AUTHORITY,
                     "Provided authority is not known.  MSAL will only make requests to known authorities"
             );
-        } else {
-            Logger.info(methodTag, "Cloud is known.");
-            known = true;
+            return new KnownAuthorityResult(false, clientException);
         }
-
-        return new KnownAuthorityResult(known, clientException);
     }
 
     public static class KnownAuthorityResult {
-        private boolean mKnown;
-        private ClientException mClientException;
+        private final boolean mKnown;
+        private final ClientException mClientException;
 
         KnownAuthorityResult(boolean known, ClientException exception) {
             mKnown = known;

common4j/src/main/com/microsoft/identity/common/java/authorities/AzureActiveDirectoryAuthority.java

@@ -71,7 +71,7 @@ public class AzureActiveDirectoryAuthority extends Authority {
 
     /** Gets {@link AzureActiveDirectoryCloud}, if the cloud metadata is already initialized. */
     @Nullable
-    private static synchronized AzureActiveDirectoryCloud getAzureActiveDirectoryCloud(
+    private static AzureActiveDirectoryCloud getAzureActiveDirectoryCloud(
             @NonNull final AzureActiveDirectoryAudience audience) {
         final String methodName = ":getAzureActiveDirectoryCloud";
 
@@ -175,7 +175,7 @@ public OAuth2Strategy createOAuth2Strategy(@NonNull final OAuth2StrategyParamete
      * @return true if both authorities belong to same cloud, otherwise false.
      */
     //@WorkerThread
-    public synchronized boolean isSameCloudAsAuthority(@NonNull final AzureActiveDirectoryAuthority authorityToCheck)
+    public boolean isSameCloudAsAuthority(@NonNull final AzureActiveDirectoryAuthority authorityToCheck)
             throws ClientException {
         // Cloud discovery is needed to make sure that we have preferred_network_host_name to cloud aliases mappings
         AzureActiveDirectory.ensureCloudDiscoveryForAuthority(this);

common4j/src/main/com/microsoft/identity/common/java/providers/microsoft/azureactivedirectory/AzureActiveDirectory.java

@@ -164,12 +164,13 @@ public AzureActiveDirectoryOAuth2Strategy createOAuth2Strategy(@NonNull final Az
         return new AzureActiveDirectoryOAuth2Strategy(config, parameters);
     }
 
-    public static synchronized boolean hasCloudHost(@NonNull final URL authorityUrl) {
+    public static boolean hasCloudHost(@NonNull final URL authorityUrl) {
         return sAadClouds.containsKey(authorityUrl.getHost().toLowerCase(Locale.US));
     }
 
-    public static synchronized boolean isValidCloudHost(@NonNull final URL authorityUrl) {
-        return hasCloudHost(authorityUrl) && getAzureActiveDirectoryCloud(authorityUrl).isValidated();
+    public static boolean isValidCloudHost(@NonNull final URL authorityUrl) {
+        final AzureActiveDirectoryCloud cloud = getAzureActiveDirectoryCloud(authorityUrl);
+        return cloud != null && cloud.isValidated();
     }
 
     public static synchronized void setEnvironment(@NonNull final Environment environment) {
@@ -191,15 +192,15 @@ public static synchronized Environment getEnvironment() {
      * @param authorityUrl URL
      * @return AzureActiveDirectoryCloud
      */
-    public static synchronized AzureActiveDirectoryCloud getAzureActiveDirectoryCloud(@NonNull final URL authorityUrl) {
+    public static AzureActiveDirectoryCloud getAzureActiveDirectoryCloud(@NonNull final URL authorityUrl) {
         return sAadClouds.get(authorityUrl.getHost().toLowerCase(Locale.US));
     }
 
     /**
      * @param preferredCacheHostName String
      * @return AzureActiveDirectoryCloud
      */
-    public static synchronized AzureActiveDirectoryCloud getAzureActiveDirectoryCloudFromHostName(@NonNull final String preferredCacheHostName) {
+    public static AzureActiveDirectoryCloud getAzureActiveDirectoryCloudFromHostName(@NonNull final String preferredCacheHostName) {
         return sAadClouds.get(preferredCacheHostName.toLowerCase(Locale.US));
     }
 
@@ -368,11 +369,16 @@ public static synchronized void ensureCloudDiscovery() throws ClientException {
      *
      * @param authority The authority whose URL determines the discovery endpoint, or null to use the default.
      */
-    public static synchronized void ensureCloudDiscoveryForAuthority(@Nullable final Authority authority)
+    public static void ensureCloudDiscoveryForAuthority(@Nullable final Authority authority)
             throws ClientException {
-        ensureCloudDiscoveryForAuthority(
-                authority != null ? authority.getAuthorityURL() : null
-        );
+        // Resolve the URL outside the synchronized scope to avoid deadlock:
+        // getAuthorityURL() on AzureActiveDirectoryAuthority acquires
+        // AzureActiveDirectoryAuthority.class then AzureActiveDirectory.class,
+        // while ensureCloudDiscoveryForAuthority(URL) acquires AzureActiveDirectory.class.
+        // Calling getAuthorityURL() while holding AzureActiveDirectory.class would
+        // create a lock-ordering inversion with any concurrent getAuthorityURL() caller.
+        final URL authorityUrl = authority != null ? authority.getAuthorityURL() : null;
+        ensureCloudDiscoveryForAuthority(authorityUrl);
     }
 
     /**