Skip to content

Commit

Permalink
NRE fix in authority creation logic when using ATS with OS account. (#…
Browse files Browse the repository at this point in the history 
…3779)

* Fix. Add check for OS account.

* Update OS account check. Add tests.
  • Loading branch information
@pmaytak
pmaytak authored Nov 2, 2022
1 parent bf67d67 commit 2a0d410
Show file tree
Hide file tree
Showing 3 changed files with 63 additions and 52 deletions.
20 changes: 10 additions & 10 deletions src/client/Microsoft.Identity.Client/AppConfig/AuthorityInfo.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,13 +130,13 @@ private AuthorityInfo(
public string UserRealmUriPrefix { get; }
public bool ValidateAuthority { get; }

internal bool IsInstanceDiscoverySupported => (AuthorityType == AuthorityType.Aad);
internal bool IsInstanceDiscoverySupported => AuthorityType == AuthorityType.Aad;

internal bool IsUserAssertionSupported => (AuthorityType != AuthorityType.Adfs && AuthorityType != AuthorityType.B2C);
internal bool IsUserAssertionSupported => AuthorityType != AuthorityType.Adfs && AuthorityType != AuthorityType.B2C;

internal bool IsTenantOverrideSupported => (AuthorityType == AuthorityType.Aad);
internal bool IsMultiTenantSupported => (AuthorityType != AuthorityType.Adfs);
internal bool IsClientInfoSupported => (AuthorityType == AuthorityType.Aad || AuthorityType == AuthorityType.Dsts || AuthorityType == AuthorityType.B2C);
internal bool IsTenantOverrideSupported => AuthorityType == AuthorityType.Aad;
internal bool IsMultiTenantSupported => AuthorityType != AuthorityType.Adfs;
internal bool IsClientInfoSupported => AuthorityType == AuthorityType.Aad || AuthorityType == AuthorityType.Dsts || AuthorityType == AuthorityType.B2C;

#region Builders
internal static AuthorityInfo FromAuthorityUri(string authorityUri, bool validateAuthority)
Expand All @@ -154,7 +154,7 @@ internal static AuthorityInfo FromAuthorityUri(string authorityUri, bool validat

return new AuthorityInfo(authorityType, canonicalUri, validateAuthority);
}

internal static AuthorityInfo FromAadAuthority(Uri cloudInstanceUri, Guid tenantId, bool validateAuthority)
{
#pragma warning disable CA1305 // Specify IFormatProvider
Expand Down Expand Up @@ -272,7 +272,7 @@ internal static string CanonicalizeAuthorityUri(string uri)
{
if (!string.IsNullOrWhiteSpace(uri) && !uri.EndsWith("/", StringComparison.OrdinalIgnoreCase))
{
uri = uri + "/";
uri += "/";
}

return uri?.ToLowerInvariant() ?? string.Empty;
Expand Down Expand Up @@ -390,7 +390,7 @@ internal static string GetSecondPathSegment(Uri authority)
throw new InvalidOperationException(MsalErrorMessage.DstsAuthorityDoesNotHaveThreeSegments);
}

private static AuthorityType GetAuthorityType(string authority)
private static AuthorityType GetAuthorityType(string authority)
{
string firstPathSegment = GetFirstPathSegment(authority);

Expand All @@ -411,7 +411,7 @@ private static AuthorityType GetAuthorityType(string authority)

return AuthorityType.Aad;
}

private static string[] GetPathSegments(string absolutePath)
{
string[] pathSegments = absolutePath.Substring(1).Split(
Expand Down Expand Up @@ -494,7 +494,7 @@ public static async Task<Authority> CreateAuthorityForRequestAsync(RequestContex

case AuthorityType.Aad:

bool updateEnvironment = requestContext.ServiceBundle.Config.MultiCloudSupportEnabled && account != null;
bool updateEnvironment = requestContext.ServiceBundle.Config.MultiCloudSupportEnabled && account != null && !PublicClientApplication.IsOperatingSystemAccount(account);

if (requestAuthorityInfo == null)
{
Expand Down
62 changes: 34 additions & 28 deletions tests/Microsoft.Identity.Test.Unit/ApiConfigTests/AuthorityTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,6 @@ public void WithTenantIdFromAuthority_NullUriAuthority_Failure(Uri authorityValu
.WithClientSecret("secret")
.Build();


AssertException.Throws<ArgumentNullException>(() =>
app
.AcquireTokenByAuthorizationCode(TestConstants.s_scope, "code")
Expand All @@ -142,43 +141,48 @@ public void WithTenantIdFromAuthority_NullUriAuthority_Failure(Uri authorityValu
[TestMethod]
public void VerifyAuthorityTest()
{
const string utid = TestConstants.Utid;
const string utid2 = TestConstants.Utid2;

VerifyAuthority(
config: s_commonAuthority,
request: null,
configAuthority: s_commonAuthority,
requestAuthority: null,
account: null,
resultTid: "common",
expectedTenantId: "common",
_testRequestContext);

VerifyAuthority(
config: s_commonAuthority,
request: s_commonAuthority,
configAuthority: s_commonAuthority,
requestAuthority: s_commonAuthority,
account: null,
resultTid: "common",
expectedTenantId: "common",
_testRequestContext);

VerifyAuthority(
config: s_commonAuthority,
request: s_commonAuthority,
account: new Account(TestConstants.s_userIdentifier, "username", s_commonAuthority.AuthorityInfo.Host),
resultTid: utid,
configAuthority: s_commonAuthority,
requestAuthority: s_commonAuthority,
account: new Account(TestConstants.s_userIdentifier, "username", s_commonAuthority.AuthorityInfo.Host),
expectedTenantId: TestConstants.Utid,
_testRequestContext);

VerifyAuthority(
config: s_commonAuthority,
request: s_utidAuthority,
configAuthority: s_commonAuthority,
requestAuthority: s_utidAuthority,
account: null,
resultTid: utid,
expectedTenantId: TestConstants.Utid,
_testRequestContext);

VerifyAuthority(
config: s_commonAuthority,
request: s_utid2Authority,
configAuthority: s_commonAuthority,
requestAuthority: s_utid2Authority,
account: new Account(TestConstants.s_userIdentifier, "username", s_utid2Authority.AuthorityInfo.Host),
resultTid: utid2,
expectedTenantId: TestConstants.Utid2,
_testRequestContext);

VerifyAuthority(
configAuthority: s_commonAuthority,
requestAuthority: null,
account: PublicClientApplication.OperatingSystemAccount,
expectedTenantId: "common",
_testRequestContext,
multiCloudSupport: true);
}

[TestMethod]
Expand Down Expand Up @@ -228,7 +232,7 @@ public async Task DifferentHostsAsync()

_testRequestContext.ServiceBundle.Config.Authority = Authority.CreateAuthority(TestConstants.B2CAuthority, true);
var ex4 = await Assert.ThrowsExceptionAsync<MsalClientException>(
() => Authority.CreateAuthorityForRequestAsync(
() => Authority.CreateAuthorityForRequestAsync(
_testRequestContext,
AuthorityInfo.FromAuthorityUri(TestConstants.B2CCustomDomain, true),
null)).ConfigureAwait(false);
Expand Down Expand Up @@ -260,15 +264,17 @@ public void IsDefaultAuthorityTest()
}

private static void VerifyAuthority(
Authority config,
Authority request,
Authority configAuthority,
Authority requestAuthority,
IAccount account,
string resultTid,
RequestContext requestContext)
string expectedTenantId,
RequestContext requestContext,
bool multiCloudSupport = false)
{
requestContext.ServiceBundle.Config.Authority = config;
var resultAuthority = Authority.CreateAuthorityForRequestAsync(requestContext, request?.AuthorityInfo, account).Result;
Assert.AreEqual(resultTid, resultAuthority.TenantId);
requestContext.ServiceBundle.Config.Authority = configAuthority;
requestContext.ServiceBundle.Config.MultiCloudSupportEnabled = multiCloudSupport;
var resultAuthority = Authority.CreateAuthorityForRequestAsync(requestContext, requestAuthority?.AuthorityInfo, account).Result;
Assert.AreEqual(expectedTenantId, resultAuthority.TenantId);
}
}
}
33 changes: 19 additions & 14 deletions tests/Microsoft.Identity.Test.Unit/BrokerTests/BrokerRequestTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
using Microsoft.VisualStudio.TestTools.UnitTesting;
using NSubstitute;
using NSubstitute.ExceptionExtensions;
using NSubstitute.Extensions;

namespace Microsoft.Identity.Test.Unit.BrokerTests
{
Expand Down Expand Up @@ -460,11 +461,16 @@ public override bool IsBrokerInstalledAndInvokable(AuthorityType authorityType)

private static IBroker CreateBroker(Type brokerType)
{
if (brokerType == typeof(NullBroker))
return new NullBroker(null);
if (brokerType == typeof(IosBrokerMock))
return new IosBrokerMock(null);

if (brokerType == typeof(NullBroker))
{
return new NullBroker(null);
}

if (brokerType == typeof(IosBrokerMock))
{
return new IosBrokerMock(null);
}

throw new NotImplementedException();
}

Expand Down Expand Up @@ -769,10 +775,8 @@ public async Task BrokerSilentStrategy_DefaultAccountAsync()
// Assert
Assert.AreSame(tokenResponse.AccessToken, result.AccessToken);
}
}



}

[TestMethod]
public void NoTokenFoundThrowsUIRequiredTest()
{
Expand Down Expand Up @@ -862,7 +866,7 @@ public async Task MultiCloud_WithBroker_Async()
.WithMultiCloudSupport(true)
.WithHttpManager(harness.HttpManager);

var broker = NSubstitute.Substitute.For<IBroker>();
var broker = Substitute.For<IBroker>();
builder.Config.BrokerCreatorFunc = (_, _, _) => broker;

var globalPca = builder.WithBroker(true).BuildConcrete();
Expand All @@ -878,10 +882,13 @@ public async Task MultiCloud_WithBroker_Async()
var result = await globalPca.AcquireTokenInteractive(TestConstants.s_graphScopes).ExecuteAsync().ConfigureAwait(false);
Assert.AreEqual("login.microsoftonline.us", result.Account.Environment);
Assert.AreEqual(TestConstants.Utid, result.TenantId);

var account = (await globalPca.GetAccountsAsync().ConfigureAwait(false)).Single();
Assert.AreEqual("login.microsoftonline.us", account.Environment);
Assert.AreEqual(TestConstants.Utid, result.TenantId);

await Assert.ThrowsExceptionAsync<MsalUiRequiredException>(
async () => await globalPca.AcquireTokenSilent(TestConstants.s_graphScopes, PublicClientApplication.OperatingSystemAccount).ExecuteAsync().ConfigureAwait(false)).ConfigureAwait(false);
}
}

Expand Down Expand Up @@ -995,7 +1002,7 @@ private MockHttpAndServiceBundle CreateBrokerHelper()
}

private MsalTokenResponse CreateTokenResponseForTest()

{
return new MsalTokenResponse()
{
Expand All @@ -1012,6 +1019,4 @@ private MsalTokenResponse CreateTokenResponseForTest()
}
}



}

0 comments on commit 2a0d410

Please sign in to comment.