Skip to content

Commit c9c2b24

Browse files
mohitc1Azure DevOps PipelinefadidurahMohitcacosta33
authored
Update release/24.1.1 (#3087)
Co-authored-by: Azure DevOps Pipeline <build-agent@azuredevops.com> Co-authored-by: fadidurah <fadidurah@microsoft.com> Co-authored-by: Mohit <mchand@microsoft.com> Co-authored-by: Cesar Acosta <cacosta333@hotmail.com> Co-authored-by: Siddhi <siddhijain@microsoft.com>
1 parent 694e05e commit c9c2b24

File tree

11 files changed

+684
-74
lines changed

11 files changed

+684
-74
lines changed

‎changelog.txt‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,8 @@
1-
vNext
1+
Version 24.1.1
2+
----------
3+
- [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)
4+
5+
Version 24.1.0
26
----------
37
- [MINOR] Add sovereign cloud (Bleu/Delos/SovSG) instance discovery support with pre-seeded cloud metadata, cache-aware discovery routing, and ensureCloudDiscoveryForAuthority API (#3027)
48
- [PATCH] Fix bug in Authority.getKnownAuthorityResult where cloud discovery failure would skip knownAuthorities check and fix thread safety in Authority.isKnownAuthority and getEquivalentConfiguredAuthority with synchronized block (#3027)

‎common/build.gradle‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ tasks.register("jacocoTestReport", JacocoReport) {
8787

8888
// In dev, we want to keep the dependencies(common4j, broker4j, common) to 1.0.+ to be able to be consumed by daily dev pipeline.
8989
// In release/*, we change these to specific versions being consumed.
90-
def common4jVersion = "1.0.+"
90+
def common4jVersion = "24.1.1"
9191
if (project.hasProperty("distCommon4jVersion") && project.distCommon4jVersion != '') {
9292
common4jVersion = project.distCommon4jVersion
9393
}
@@ -96,6 +96,9 @@ boolean newBrokerDiscoveryEnabledFlag = project.hasProperty("newBrokerDiscoveryE
9696
boolean trustDebugBrokerFlag = project.hasProperty("trustDebugBrokerFlag")
9797
boolean bypassRedirectUriCheck = project.hasProperty("bypassRedirectUriCheck")
9898

99+
def robolectricRepoUsername = System.getenv("ENV_VSTS_MVN_CRED_USERNAME") != null ? System.getenv("ENV_VSTS_MVN_CRED_USERNAME") : project.findProperty("vstsUsername")
100+
def robolectricRepoPassword = System.getenv("ENV_VSTS_MVN_CRED_ACCESSTOKEN") != null ? System.getenv("ENV_VSTS_MVN_CRED_ACCESSTOKEN") : project.findProperty("vstsMavenAccessToken")
101+
99102
android {
100103
compileSdk rootProject.ext.compileSdkVersion
101104
defaultConfig {
@@ -160,6 +163,11 @@ android {
160163
unitTests.all {
161164
exclude 'com/microsoft/identity/common/integration'
162165
exclude 'com/microsoft/identity/common/ropc'
166+
if (robolectricRepoUsername?.toString()?.trim() && robolectricRepoPassword?.toString()?.trim()) {
167+
it.systemProperty("robolectric.dependency.repo.url", "https://identitydivision.pkgs.visualstudio.com/_packaging/NewAndroid/maven/v1")
168+
it.systemProperty("robolectric.dependency.repo.username", robolectricRepoUsername)
169+
it.systemProperty("robolectric.dependency.repo.password", robolectricRepoPassword)
170+
}
163171
}
164172
}
165173

‎common/src/main/java/com/microsoft/identity/common/internal/broker/PackageHelper.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,10 @@ public boolean isPackageInstalledAndEnabled(final String packageName) {
167167
boolean enabled = false;
168168
try {
169169
final ApplicationInfo applicationInfo = mPackageManager.getApplicationInfo(packageName, 0);
170+
if (applicationInfo == null) {
171+
Logger.verbose(methodTag, packageName + " applicationInfo is null");
172+
return false;
173+
}
170174
if (CommonFlightsManager.INSTANCE.getFlightsProvider().isFlightEnabled(
171175
CommonFlight.USE_ENABLED_SETTING_FOR_PACKAGE_CHECK)) {
172176
try {
@@ -183,6 +187,7 @@ public boolean isPackageInstalledAndEnabled(final String packageName) {
183187
}
184188
} catch (NameNotFoundException e) {
185189
Logger.verbose(methodTag, packageName + " is not found");
190+
return false;
186191
}
187192

188193
Logger.verbose(methodTag, packageName + " is installed. enabled? [" + enabled + "]");

‎common/src/test/java/com/microsoft/identity/common/internal/broker/PackageHelperTest.java‎

Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,138 @@
1+
// Copyright (c) Microsoft Corporation.
2+
// All rights reserved.
3+
//
4+
// This code is licensed under the MIT License.
5+
//
6+
// Permission is hereby granted, free of charge, to any person obtaining a copy
7+
// of this software and associated documentation files(the "Software"), to deal
8+
// in the Software without restriction, including without limitation the rights
9+
// to use, copy, modify, merge, publish, distribute, sublicense, and / or sell
10+
// copies of the Software, and to permit persons to whom the Software is
11+
// furnished to do so, subject to the following conditions :
12+
//
13+
// The above copyright notice and this permission notice shall be included in
14+
// all copies or substantial portions of the Software.
15+
//
16+
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
17+
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
18+
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
19+
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
20+
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
21+
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
22+
// THE SOFTWARE.
23+
package com.microsoft.identity.common.internal.broker;
24+
25+
import static org.junit.Assert.assertFalse;
26+
import static org.junit.Assert.assertTrue;
27+
import static org.mockito.Mockito.mock;
28+
import static org.mockito.Mockito.when;
29+
30+
import android.content.pm.ApplicationInfo;
31+
import android.content.pm.PackageManager;
32+
import android.content.pm.PackageManager.NameNotFoundException;
33+
34+
import com.microsoft.identity.common.internal.mocks.MockCommonFlightsManager;
35+
import com.microsoft.identity.common.java.flighting.CommonFlight;
36+
import com.microsoft.identity.common.java.flighting.CommonFlightsManager;
37+
import com.microsoft.identity.common.java.flighting.IFlightsProvider;
38+
39+
import org.junit.After;
40+
import org.junit.Before;
41+
import org.junit.Test;
42+
import org.junit.runner.RunWith;
43+
import org.robolectric.RobolectricTestRunner;
44+
45+
@RunWith(RobolectricTestRunner.class)
46+
public class PackageHelperTest {
47+
48+
private static final String TEST_PACKAGE = "com.example.test";
49+
50+
private PackageManager mockPackageManager;
51+
private PackageHelper packageHelper;
52+
private IFlightsProvider mockFlightsProvider;
53+
54+
@Before
55+
public void setUp() {
56+
mockPackageManager = mock(PackageManager.class);
57+
packageHelper = new PackageHelper(mockPackageManager);
58+
59+
mockFlightsProvider = mock(IFlightsProvider.class);
60+
when(mockFlightsProvider.isFlightEnabled(CommonFlight.USE_ENABLED_SETTING_FOR_PACKAGE_CHECK))
61+
.thenReturn(false);
62+
63+
final MockCommonFlightsManager mockFlightsManager = new MockCommonFlightsManager();
64+
mockFlightsManager.setMockCommonFlightsProvider(mockFlightsProvider);
65+
CommonFlightsManager.INSTANCE.initializeCommonFlightsManager(mockFlightsManager);
66+
}
67+
68+
@After
69+
public void tearDown() {
70+
CommonFlightsManager.INSTANCE.resetFlightsManager();
71+
}
72+
73+
@Test
74+
public void testIsPackageInstalledAndEnabled_packageNotFound_returnsFalse() throws Exception {
75+
when(mockPackageManager.getApplicationInfo(TEST_PACKAGE, 0))
76+
.thenThrow(new NameNotFoundException(TEST_PACKAGE));
77+
78+
assertFalse(packageHelper.isPackageInstalledAndEnabled(TEST_PACKAGE));
79+
}
80+
81+
@Test
82+
public void testIsPackageInstalledAndEnabled_applicationInfoNull_returnsFalse() throws Exception {
83+
when(mockPackageManager.getApplicationInfo(TEST_PACKAGE, 0))
84+
.thenReturn(null);
85+
86+
assertFalse(packageHelper.isPackageInstalledAndEnabled(TEST_PACKAGE));
87+
}
88+
89+
@Test
90+
public void testIsPackageInstalledAndEnabled_packageEnabled_returnsTrue() throws Exception {
91+
final ApplicationInfo appInfo = new ApplicationInfo();
92+
appInfo.enabled = true;
93+
when(mockPackageManager.getApplicationInfo(TEST_PACKAGE, 0))
94+
.thenReturn(appInfo);
95+
96+
assertTrue(packageHelper.isPackageInstalledAndEnabled(TEST_PACKAGE));
97+
}
98+
99+
@Test
100+
public void testIsPackageInstalledAndEnabled_packageDisabled_returnsFalse() throws Exception {
101+
final ApplicationInfo appInfo = new ApplicationInfo();
102+
appInfo.enabled = false;
103+
when(mockPackageManager.getApplicationInfo(TEST_PACKAGE, 0))
104+
.thenReturn(appInfo);
105+
106+
assertFalse(packageHelper.isPackageInstalledAndEnabled(TEST_PACKAGE));
107+
}
108+
109+
@Test
110+
public void testIsPackageInstalledAndEnabled_flightEnabled_usesEnabledSetting() throws Exception {
111+
when(mockFlightsProvider.isFlightEnabled(CommonFlight.USE_ENABLED_SETTING_FOR_PACKAGE_CHECK))
112+
.thenReturn(true);
113+
114+
final ApplicationInfo appInfo = new ApplicationInfo();
115+
appInfo.enabled = false;
116+
when(mockPackageManager.getApplicationInfo(TEST_PACKAGE, 0))
117+
.thenReturn(appInfo);
118+
when(mockPackageManager.getApplicationEnabledSetting(TEST_PACKAGE))
119+
.thenReturn(PackageManager.COMPONENT_ENABLED_STATE_ENABLED);
120+
121+
assertTrue(packageHelper.isPackageInstalledAndEnabled(TEST_PACKAGE));
122+
}
123+
124+
@Test
125+
public void testIsPackageInstalledAndEnabled_flightEnabled_enabledSettingThrows_fallsBackToAppInfo() throws Exception {
126+
when(mockFlightsProvider.isFlightEnabled(CommonFlight.USE_ENABLED_SETTING_FOR_PACKAGE_CHECK))
127+
.thenReturn(true);
128+
129+
final ApplicationInfo appInfo = new ApplicationInfo();
130+
appInfo.enabled = true;
131+
when(mockPackageManager.getApplicationInfo(TEST_PACKAGE, 0))
132+
.thenReturn(appInfo);
133+
when(mockPackageManager.getApplicationEnabledSetting(TEST_PACKAGE))
134+
.thenThrow(new IllegalArgumentException("test"));
135+
136+
assertTrue(packageHelper.isPackageInstalledAndEnabled(TEST_PACKAGE));
137+
}
138+
}

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

Lines changed: 49 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -318,40 +318,52 @@ public static void addKnownAuthorities(List<Authority> authorities) {
318318
}
319319

320320
/**
321-
* Authorities are either known by the developer and communicated to the library via configuration or they
322-
* are known to Microsoft based on the list of clouds returned from:
321+
* Clears all known authorities. For test use only.
322+
*/
323+
// Visible for testing
324+
static void clearKnownAuthorities() {
325+
synchronized (sLock) {
326+
knownAuthorities.clear();
327+
}
328+
}
329+
330+
/**
331+
* Authorities are either known by the developer and communicated to the library via
332+
* configuration, or they are known to Microsoft based on the cloud discovery metadata cache.
323333
*
324334
* @param authority Authority to check against.
325335
* @return True if the authority is known to Microsoft or defined in the configuration.
326336
*/
327337
public static boolean isKnownAuthority(Authority authority) {
328-
final String methodName = ":isKnownAuthority";
338+
final String methodTag = TAG + ":isKnownAuthority";
329339
boolean knownToDeveloper = false;
330340
boolean knownToMicrosoft;
331341

332342
if (authority == null) {
333-
Logger.warn(
334-
TAG + methodName,
335-
"Authority is null"
336-
);
343+
Logger.warn(methodTag, "Authority is null");
344+
return false;
345+
}
346+
347+
// Resolve the authority URL outside any lock to avoid calling a potentially-
348+
// synchronized polymorphic method (e.g. AzureActiveDirectoryAuthority.getAuthorityURL())
349+
// while holding sLock, which could create a lock-ordering inversion.
350+
final URL authorityUrl = authority.getAuthorityURL();
351+
352+
if (authorityUrl == null) {
353+
Logger.warn(methodTag, "Authority URL is null");
337354
return false;
338355
}
339356

340-
if (BuildConfig.ALLOW_ONEBOX_AUTHORITIES && AzureActiveDirectoryEnvironment.ONEBOX_AUTHORITY.equals(authority.getAuthorityURL().getAuthority())) {
357+
if (BuildConfig.ALLOW_ONEBOX_AUTHORITIES && AzureActiveDirectoryEnvironment.ONEBOX_AUTHORITY.equals(authorityUrl.getAuthority())) {
341358
return true; // onebox authorities are always considered to be known.
342359
}
343360

344-
//Check if authority was added to configuration
345361
synchronized (sLock) {
346362
for (final Authority currentAuthority : knownAuthorities) {
347363
if (currentAuthority.mAuthorityUrlString != null &&
348-
authority.getAuthorityURL() != null &&
349-
authority.getAuthorityURL().getAuthority() != null &&
364+
authorityUrl.getAuthority() != null &&
350365
currentAuthority.mAuthorityUrlString.toLowerCase(Locale.ROOT).contains(
351-
authority
352-
.getAuthorityURL()
353-
.getAuthority()
354-
.toLowerCase(Locale.ROOT))) {
366+
authorityUrl.getAuthority().toLowerCase(Locale.ROOT))) {
355367
knownToDeveloper = true;
356368
break;
357369
}
@@ -360,19 +372,14 @@ public static boolean isKnownAuthority(Authority authority) {
360372

361373
// Check whether the authority is known to Microsoft or not. Microsoft can recognize authorities that exist within public clouds.
362374
// Microsoft does not maintain a list of B2C authorities or a list of ADFS or 3rd party authorities (issuers).
363-
knownToMicrosoft = AzureActiveDirectory.hasCloudHost(authority.getAuthorityURL());
375+
knownToMicrosoft = AzureActiveDirectory.hasCloudHost(authorityUrl);
364376

365377
final boolean isKnown = (knownToDeveloper || knownToMicrosoft);
366378

367-
Logger.verbose(
368-
TAG + methodName,
369-
"Authority is known to developer? [" + knownToDeveloper + "]"
370-
);
371-
372-
Logger.verbose(
373-
TAG + methodName,
374-
"Authority is known to Microsoft? [" + knownToMicrosoft + "]"
375-
);
379+
Logger.verbose(methodTag,
380+
"Authority is known to developer? [" + knownToDeveloper + "]");
381+
Logger.verbose(methodTag,
382+
"Authority is known to Microsoft? [" + knownToMicrosoft + "]");
376383

377384
return isKnown;
378385
}
@@ -383,37 +390,43 @@ public static KnownAuthorityResult getKnownAuthorityResult(Authority authority)
383390
methodTag,
384391
"Getting known authority result..."
385392
);
386-
ClientException clientException = null;
387-
boolean known = false;
388393

394+
ClientException discoveryException = null;
389395
try {
390396
AzureActiveDirectory.ensureCloudDiscoveryForAuthority(authority);
391397
Logger.info(methodTag, "Cloud discovery complete.");
392398
} catch (final ClientException ex) {
393399
// Cloud discovery failed (e.g. network error).
394400
// Log but continue — the authority may still be known via hardcoded
395401
// metadata or developer configuration.
402+
discoveryException = ex;
396403
Logger.warn(methodTag,
397404
"Cloud discovery failed, will check hardcoded/configured authorities. Error: "
398405
+ ex.getErrorCode());
399406
}
400407

401-
if (!isKnownAuthority(authority)) {
402-
clientException = new ClientException(
408+
if (isKnownAuthority(authority)) {
409+
Logger.info(methodTag, "Authority is known.");
410+
return new KnownAuthorityResult(true, null);
411+
}
412+
413+
// Authority is not known via any source.
414+
if (discoveryException != null) {
415+
// Discovery failed — propagate the original error
416+
return new KnownAuthorityResult(false, discoveryException);
417+
} else {
418+
// Discovery succeeded but authority is not in any known list.
419+
final ClientException clientException = new ClientException(
403420
ClientException.UNKNOWN_AUTHORITY,
404421
"Provided authority is not known. MSAL will only make requests to known authorities"
405422
);
406-
} else {
407-
Logger.info(methodTag, "Cloud is known.");
408-
known = true;
423+
return new KnownAuthorityResult(false, clientException);
409424
}
410-
411-
return new KnownAuthorityResult(known, clientException);
412425
}
413426

414427
public static class KnownAuthorityResult {
415-
private boolean mKnown;
416-
private ClientException mClientException;
428+
private final boolean mKnown;
429+
private final ClientException mClientException;
417430

418431
KnownAuthorityResult(boolean known, ClientException exception) {
419432
mKnown = known;

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ public class AzureActiveDirectoryAuthority extends Authority {
7171

7272
/** Gets {@link AzureActiveDirectoryCloud}, if the cloud metadata is already initialized. */
7373
@Nullable
74-
private static synchronized AzureActiveDirectoryCloud getAzureActiveDirectoryCloud(
74+
private static AzureActiveDirectoryCloud getAzureActiveDirectoryCloud(
7575
@NonNull final AzureActiveDirectoryAudience audience) {
7676
final String methodName = ":getAzureActiveDirectoryCloud";
7777

@@ -175,7 +175,7 @@ public OAuth2Strategy createOAuth2Strategy(@NonNull final OAuth2StrategyParamete
175175
* @return true if both authorities belong to same cloud, otherwise false.
176176
*/
177177
//@WorkerThread
178-
public synchronized boolean isSameCloudAsAuthority(@NonNull final AzureActiveDirectoryAuthority authorityToCheck)
178+
public boolean isSameCloudAsAuthority(@NonNull final AzureActiveDirectoryAuthority authorityToCheck)
179179
throws ClientException {
180180
// Cloud discovery is needed to make sure that we have preferred_network_host_name to cloud aliases mappings
181181
AzureActiveDirectory.ensureCloudDiscoveryForAuthority(this);

0 commit comments

Comments
 (0)