Skip to content

Commit 36a9cec

Browse files
mjcheethammjcheetham
committed
msauth: accept IMicrosoftAccount on GetTokenForUserAsync
`IMicrosoftAuthentication.GetTokenForUserAsync` predated the `IMicrosoftAccount` shape that the rest of the auth surface now uses for identifying cached accounts. It took a bare `string userName`, matched by UPN against `app.GetAccountsAsync()`, and quietly picked the first match. That works fine for single-tenant single-account users but loses information in two scenarios: - Guest accounts where the same UPN exists in two tenants (e.g. `alice@contoso.com` as a guest in fabrikam). Both accounts share a UPN but have distinct `HomeAccountId` values; selecting by UPN alone non-deterministically picks one. - Callers that already carry a stable `HomeAccountId` (e.g. an upcoming binding manager rewrite) have to translate it back to a UPN before calling the API, which both wastes the stable identifier and reintroduces the ambiguity. Pivot the interface to take an `IMicrosoftAccount`: Task<IMicrosoftAuthenticationResult> GetTokenForUserAsync( string authority, string clientId, Uri redirectUri, string[] scopes, IMicrosoftAccount account, bool msaPt = false); Internal resolution prefers `HomeAccountId` when present, falls back to `UserName` otherwise, traces a warning when both are set and the cached account's UPN differs from the supplied one (HomeAccountId wins, supplied UPN is informational), and traces a warning when UPN-only resolution returns multiple matches (first-match returned, today's behaviour preserved). `null` account remains "let MSAL pick interactively". The silent-acquisition helper unifies behind a single `GetAccessTokenSilentlyAsync(app, scopes, IAccount, msaPt)`: callers with an explicit cached account pass it through, and the broker's "default OS account" path resolves to `PublicClientApplication.OperatingSystemAccount` (an `IAccount` sentinel) and goes through the same path. The MSA-passthrough tenant-id workaround switches to a null-safe lookup so it no-ops cleanly when the sentinel's `HomeAccountId` isn't populated. Add a new `MicrosoftAuthenticationExtensions` static class to host an `[Obsolete]` extension that preserves the pre-existing `(…, string userName, …)` signature. Existing in-tree production call sites keep building with a deprecation warning as a TODO list; the extension wraps by constructing `new MicrosoftAccount(homeAccountId: null, userName)` — the "UPN-only" shape — so the legacy semantics are preserved exactly. A follow-on commit migrates the one remaining production caller (Azure Repos), then a final commit deletes the extension. `Mock<IMicrosoftAuthentication>` cannot set up an extension method, so the eight `.Setup` expressions in `AzureReposHostProviderTests` and the one direct call in `MicrosoftAuthenticationTests` migrate to the new interface method in this commit. Each test hoists a local `var expectedAccount = new MicrosoftAccount(homeAccountId: null, userName: …)` (or `IMicrosoftAccount expectedAccount = null` for the unconstrained case) alongside its other expected-mock-inputs and passes the value directly to `Setup`; the equality contract introduced alongside `IMicrosoftAccount` itself is what lets Moq match these by value rather than reference. Production code stays on the obsolete extension for now. Assisted-by: Claude Opus 4.7 Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
1 parent 95e3d15 commit 36a9cec

3 files changed

Lines changed: 79 additions & 51 deletions

File tree

‎src/shared/Core.Tests/Authentication/MicrosoftAuthenticationTests.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ public async Task MicrosoftAuthentication_GetTokenForUserAsync_NoInteraction_Thr
1616
const string clientId = "C9E8FDA6-1D46-484C-917C-3DBD518F27C3";
1717
Uri redirectUri = new Uri("https://localhost");
1818
string[] scopes = {"user.read"};
19-
const string userName = null; // No user to ensure we do not use an existing token
19+
IMicrosoftAccount account = null; // No account to ensure we do not use an existing token
2020

2121
var context = new TestCommandContext
2222
{
@@ -26,7 +26,7 @@ public async Task MicrosoftAuthentication_GetTokenForUserAsync_NoInteraction_Thr
2626
var msAuth = new MicrosoftAuthentication(context);
2727

2828
await Assert.ThrowsAsync<Trace2InvalidOperationException>(
29-
() => msAuth.GetTokenForUserAsync(authority, clientId, redirectUri, scopes, userName, false));
29+
() => msAuth.GetTokenForUserAsync(authority, clientId, redirectUri, scopes, account, false));
3030
}
3131

3232
[Theory]

‎src/shared/Core/Authentication/MicrosoftAuthentication.cs‎

Lines changed: 61 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -53,11 +53,14 @@ public interface IMicrosoftAuthentication
5353
/// <param name="clientId">Client ID.</param>
5454
/// <param name="redirectUri">Redirect URI for the client.</param>
5555
/// <param name="scopes">Set of scopes to request.</param>
56-
/// <param name="userName">Optional user name for an existing account.</param>
56+
/// <param name="account">Optional account to acquire a token for. <see langword="null"/>
57+
/// lets MSAL pick interactively. When set, resolved against the MSAL cache by
58+
/// <see cref="IMicrosoftAccount.HomeAccountId"/> when present, otherwise by
59+
/// <see cref="IMicrosoftAccount.UserName"/>.</param>
5760
/// <param name="msaPt">Use MSA-Passthrough behavior when authenticating.</param>
5861
/// <returns>Authentication result.</returns>
5962
Task<IMicrosoftAuthenticationResult> GetTokenForUserAsync(string authority, string clientId, Uri redirectUri,
60-
string[] scopes, string userName, bool msaPt = false);
63+
string[] scopes, IMicrosoftAccount account, bool msaPt = false);
6164

6265
/// <summary>
6366
/// Acquire an access token for the given service principal with the specified scopes.
@@ -152,6 +155,25 @@ public override int GetHashCode()
152155
}
153156
}
154157

158+
public static class MicrosoftAuthenticationExtensions
159+
{
160+
/// <summary>
161+
/// Convenience overload of <see cref="IMicrosoftAuthentication.GetTokenForUserAsync"/>
162+
/// that takes a bare UPN string.
163+
/// </summary>
164+
[Obsolete("Construct a MicrosoftAccount and call the IMicrosoftAccount overload directly.")]
165+
public static Task<IMicrosoftAuthenticationResult> GetTokenForUserAsync(
166+
this IMicrosoftAuthentication msAuth,
167+
string authority, string clientId, Uri redirectUri, string[] scopes, string userName, bool msaPt = false)
168+
{
169+
EnsureArgument.NotNull(msAuth, nameof(msAuth));
170+
IMicrosoftAccount account = string.IsNullOrWhiteSpace(userName)
171+
? null
172+
: new MicrosoftAccount(homeAccountId: null, userName: userName);
173+
return msAuth.GetTokenForUserAsync(authority, clientId, redirectUri, scopes, account, msaPt);
174+
}
175+
}
176+
155177
public class MicrosoftAuthentication : AuthenticationBase, IMicrosoftAuthentication
156178
{
157179
public static readonly string[] AuthorityIds =
@@ -254,7 +276,7 @@ private static string DescribeAccount(IMicrosoftAccount account) =>
254276
!string.IsNullOrWhiteSpace(account.HomeAccountId) ? account.HomeAccountId : account.UserName;
255277

256278
public async Task<IMicrosoftAuthenticationResult> GetTokenForUserAsync(
257-
string authority, string clientId, Uri redirectUri, string[] scopes, string userName, bool msaPt)
279+
string authority, string clientId, Uri redirectUri, string[] scopes, IMicrosoftAccount account, bool msaPt)
258280
{
259281
var uiCts = new CancellationTokenSource();
260282

@@ -276,11 +298,25 @@ public async Task<IMicrosoftAuthenticationResult> GetTokenForUserAsync(
276298

277299
AuthenticationResult result = null;
278300

279-
// Try silent authentication first if we know about an existing user
280-
bool hasExistingUser = !string.IsNullOrWhiteSpace(userName);
301+
// Resolve the account against the MSAL cache once (HomeAccountId-first, UPN-fallback,
302+
// with trace warnings on mismatch/ambiguity). The same resolved account is then
303+
// reused for silent acquisition and the device-code fallback.
304+
IAccount resolvedAccount = null;
305+
if (account is not null)
306+
{
307+
IReadOnlyList<IAccount> cached = (await app.GetAccountsAsync()).ToList();
308+
resolvedAccount = ResolveAccount(cached, account);
309+
if (resolvedAccount is null)
310+
{
311+
Context.Trace.WriteLine($"No cached account matches '{DescribeAccount(account)}'.");
312+
}
313+
}
314+
315+
// Try silent authentication first if we resolved an account against the cache.
316+
bool hasExistingUser = resolvedAccount is not null;
281317
if (hasExistingUser)
282318
{
283-
result = await GetAccessTokenSilentlyAsync(app, scopes, userName, msaPt);
319+
result = await GetAccessTokenSilentlyAsync(app, scopes, resolvedAccount, msaPt);
284320
}
285321

286322
//
@@ -318,7 +354,7 @@ public async Task<IMicrosoftAuthenticationResult> GetTokenForUserAsync(
318354
// account then the user may become stuck in a loop of authentication failures.
319355
if (!hasExistingUser && Context.Settings.UseMsAuthDefaultAccount)
320356
{
321-
result = await GetAccessTokenSilentlyAsync(app, scopes, null, msaPt);
357+
result = await GetAccessTokenSilentlyAsync(app, scopes, PublicClientApplication.OperatingSystemAccount, msaPt);
322358

323359
if (result is null || !await UseDefaultAccountAsync(result.Account.Username))
324360
{
@@ -604,48 +640,32 @@ internal InteractiveFlowType GetFlowType()
604640
}
605641

606642
/// <summary>
607-
/// Obtain an access token without showing UI or prompts.
643+
/// Obtain an access token without showing UI or prompts for the given cached account.
644+
/// Pass <see cref="PublicClientApplication.OperatingSystemAccount"/> to acquire silently
645+
/// against the broker's default OS account (broker only).
608646
/// </summary>
609647
private async Task<AuthenticationResult> GetAccessTokenSilentlyAsync(
610-
IPublicClientApplication app, string[] scopes, string userName, bool msaPt)
648+
IPublicClientApplication app, string[] scopes, IAccount account, bool msaPt)
611649
{
612650
try
613651
{
614-
if (userName is null)
652+
string label = ReferenceEquals(account, PublicClientApplication.OperatingSystemAccount)
653+
? "current operating system account"
654+
: $"account '{account.Username}'";
655+
Context.Trace.WriteLine($"Attempting to acquire token silently for {label}...");
656+
657+
var atsBuilder = app.AcquireTokenSilent(scopes, account);
658+
659+
// If we are operating with an MSA passthrough app we need to ensure that we target the
660+
// special MSA 'transfer' tenant explicitly. This is a workaround for MSAL issue:
661+
// https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/issues/3077
662+
if (msaPt && Guid.TryParse(account.HomeAccountId?.TenantId, out Guid homeTenantId) &&
663+
homeTenantId == Constants.MsaHomeTenantId)
615664
{
616-
Context.Trace.WriteLine(
617-
"Attempting to acquire token silently for current operating system account...");
618-
619-
return await app.AcquireTokenSilent(scopes, PublicClientApplication.OperatingSystemAccount)
620-
.ExecuteAsync();
665+
atsBuilder = atsBuilder.WithTenantId(Constants.MsaTransferTenantId.ToString("D"));
621666
}
622-
else
623-
{
624-
Context.Trace.WriteLine($"Attempting to acquire token silently for user '{userName}'...");
625667

626-
// Enumerate all accounts and find the one matching the user name
627-
IEnumerable<IAccount> accounts = await app.GetAccountsAsync();
628-
IAccount account = accounts.FirstOrDefault(x =>
629-
StringComparer.OrdinalIgnoreCase.Equals(x.Username, userName));
630-
if (account is null)
631-
{
632-
Context.Trace.WriteLine($"No cached account found for user '{userName}'...");
633-
return null;
634-
}
635-
636-
var atsBuilder = app.AcquireTokenSilent(scopes, account);
637-
638-
// Is we are operating with an MSA passthrough app we need to ensure that we target the
639-
// special MSA 'transfer' tenant explicitly. This is a workaround for MSAL issue:
640-
// https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/issues/3077
641-
if (msaPt && Guid.TryParse(account.HomeAccountId.TenantId, out Guid homeTenantId) &&
642-
homeTenantId == Constants.MsaHomeTenantId)
643-
{
644-
atsBuilder = atsBuilder.WithTenantId(Constants.MsaTransferTenantId.ToString("D"));
645-
}
646-
647-
return await atsBuilder.ExecuteAsync();
648-
}
668+
return await atsBuilder.ExecuteAsync();
649669
}
650670
catch (MsalUiRequiredException)
651671
{

‎src/shared/Microsoft.AzureRepos.Tests/AzureReposHostProviderTests.cs‎

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,7 @@ public async Task AzureReposProvider_GetCredentialAsync_JwtMode_CachedAuthority_
158158
var expectedRedirectUri = AzureDevOpsConstants.AadRedirectUri;
159159
var expectedScopes = AzureDevOpsConstants.AzureDevOpsDefaultScopes;
160160
var accessToken = "ACCESS-TOKEN";
161+
var expectedAccount = new MicrosoftAccount(homeAccountId: null, userName: urlAccount);
161162
var authResult = CreateAuthResult(urlAccount, accessToken);
162163

163164
var context = new TestCommandContext();
@@ -170,7 +171,7 @@ public async Task AzureReposProvider_GetCredentialAsync_JwtMode_CachedAuthority_
170171
azDevOpsMock.Setup(x => x.GetAuthorityAsync(expectedOrgUri)).ReturnsAsync(authorityUrl);
171172

172173
var msAuthMock = new Mock<IMicrosoftAuthentication>(MockBehavior.Strict);
173-
msAuthMock.Setup(x => x.GetTokenForUserAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, urlAccount, true))
174+
msAuthMock.Setup(x => x.GetTokenForUserAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, expectedAccount, true))
174175
.ReturnsAsync(authResult);
175176

176177
var authorityCacheMock = new Mock<IAzureDevOpsAuthorityCache>(MockBehavior.Strict);
@@ -208,6 +209,7 @@ public async Task AzureReposProvider_GetCredentialAsync_JwtMode_CachedAuthority_
208209
var expectedRedirectUri = AzureDevOpsConstants.AadRedirectUri;
209210
var expectedScopes = AzureDevOpsConstants.AzureDevOpsDefaultScopes;
210211
var accessToken = "ACCESS-TOKEN";
212+
var expectedAccount = new MicrosoftAccount(homeAccountId: null, userName: urlAccount);
211213
var authResult = CreateAuthResult(urlAccount, accessToken);
212214

213215
var context = new TestCommandContext();
@@ -220,7 +222,7 @@ public async Task AzureReposProvider_GetCredentialAsync_JwtMode_CachedAuthority_
220222
azDevOpsMock.Setup(x => x.GetAuthorityAsync(expectedOrgUri)).ReturnsAsync(authorityUrl);
221223

222224
var msAuthMock = new Mock<IMicrosoftAuthentication>(MockBehavior.Strict);
223-
msAuthMock.Setup(x => x.GetTokenForUserAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, urlAccount, true))
225+
msAuthMock.Setup(x => x.GetTokenForUserAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, expectedAccount, true))
224226
.ReturnsAsync(authResult);
225227

226228
var authorityCacheMock = new Mock<IAzureDevOpsAuthorityCache>(MockBehavior.Strict);
@@ -254,6 +256,7 @@ public async Task AzureReposProvider_GetCredentialAsync_JwtMode_CachedAuthority_
254256
var expectedRedirectUri = AzureDevOpsConstants.AadRedirectUri;
255257
var expectedScopes = AzureDevOpsConstants.AzureDevOpsDefaultScopes;
256258
var accessToken = "ACCESS-TOKEN";
259+
IMicrosoftAccount expectedAccount = null;
257260
var account = "jane.doe";
258261
var authResult = CreateAuthResult(account, accessToken);
259262

@@ -267,7 +270,7 @@ public async Task AzureReposProvider_GetCredentialAsync_JwtMode_CachedAuthority_
267270
azDevOpsMock.Setup(x => x.GetAuthorityAsync(expectedOrgUri)).ReturnsAsync(authorityUrl);
268271

269272
var msAuthMock = new Mock<IMicrosoftAuthentication>(MockBehavior.Strict);
270-
msAuthMock.Setup(x => x.GetTokenForUserAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, null, true))
273+
msAuthMock.Setup(x => x.GetTokenForUserAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, expectedAccount, true))
271274
.ReturnsAsync(authResult);
272275

273276
var authorityCacheMock = new Mock<IAzureDevOpsAuthorityCache>(MockBehavior.Strict);
@@ -303,6 +306,7 @@ public async Task AzureReposProvider_GetCredentialAsync_JwtMode_CachedAuthority_
303306
var expectedRedirectUri = AzureDevOpsConstants.AadRedirectUri;
304307
var expectedScopes = AzureDevOpsConstants.AzureDevOpsDefaultScopes;
305308
var accessToken = "ACCESS-TOKEN";
309+
IMicrosoftAccount expectedAccount = null;
306310
var account = "john.doe";
307311
var authResult = CreateAuthResult(account, accessToken);
308312

@@ -315,7 +319,7 @@ public async Task AzureReposProvider_GetCredentialAsync_JwtMode_CachedAuthority_
315319
var azDevOpsMock = new Mock<IAzureDevOpsRestApi>(MockBehavior.Strict);
316320

317321
var msAuthMock = new Mock<IMicrosoftAuthentication>(MockBehavior.Strict);
318-
msAuthMock.Setup(x => x.GetTokenForUserAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, null, true))
322+
msAuthMock.Setup(x => x.GetTokenForUserAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, expectedAccount, true))
319323
.ReturnsAsync(authResult);
320324

321325
var authorityCacheMock = new Mock<IAzureDevOpsAuthorityCache>(MockBehavior.Strict);
@@ -353,6 +357,7 @@ public async Task AzureReposProvider_GetCredentialAsync_JwtMode_CachedAuthority_
353357
var expectedScopes = AzureDevOpsConstants.AzureDevOpsDefaultScopes;
354358
var accessToken = "ACCESS-TOKEN";
355359
var account = "john.doe";
360+
var expectedAccount = new MicrosoftAccount(homeAccountId: null, userName: account);
356361
var authResult = CreateAuthResult(account, accessToken);
357362

358363
var context = new TestCommandContext();
@@ -364,7 +369,7 @@ public async Task AzureReposProvider_GetCredentialAsync_JwtMode_CachedAuthority_
364369
var azDevOpsMock = new Mock<IAzureDevOpsRestApi>(MockBehavior.Strict);
365370

366371
var msAuthMock = new Mock<IMicrosoftAuthentication>(MockBehavior.Strict);
367-
msAuthMock.Setup(x => x.GetTokenForUserAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, account, true))
372+
msAuthMock.Setup(x => x.GetTokenForUserAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, expectedAccount, true))
368373
.ReturnsAsync(authResult);
369374

370375
var authorityCacheMock = new Mock<IAzureDevOpsAuthorityCache>(MockBehavior.Strict);
@@ -401,6 +406,7 @@ public async Task AzureReposProvider_GetCredentialAsync_JwtMode_NoCachedAuthorit
401406
var expectedRedirectUri = AzureDevOpsConstants.AadRedirectUri;
402407
var expectedScopes = AzureDevOpsConstants.AzureDevOpsDefaultScopes;
403408
var accessToken = "ACCESS-TOKEN";
409+
IMicrosoftAccount expectedAccount = null;
404410
var account = "john.doe";
405411
var authResult = CreateAuthResult(account, accessToken);
406412

@@ -414,7 +420,7 @@ public async Task AzureReposProvider_GetCredentialAsync_JwtMode_NoCachedAuthorit
414420
azDevOpsMock.Setup(x => x.GetAuthorityAsync(expectedOrgUri)).ReturnsAsync(authorityUrl);
415421

416422
var msAuthMock = new Mock<IMicrosoftAuthentication>(MockBehavior.Strict);
417-
msAuthMock.Setup(x => x.GetTokenForUserAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, null, true))
423+
msAuthMock.Setup(x => x.GetTokenForUserAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, expectedAccount, true))
418424
.ReturnsAsync(authResult);
419425

420426
var authorityCacheMock = new Mock<IAzureDevOpsAuthorityCache>(MockBehavior.Strict);
@@ -450,6 +456,7 @@ public async Task AzureReposProvider_GetCredentialAsync_PatMode_OrgInUserName_No
450456
var expectedRedirectUri = AzureDevOpsConstants.AadRedirectUri;
451457
var expectedScopes = AzureDevOpsConstants.AzureDevOpsDefaultScopes;
452458
var accessToken = "ACCESS-TOKEN";
459+
IMicrosoftAccount expectedAccount = null;
453460
var personalAccessToken = "PERSONAL-ACCESS-TOKEN";
454461
var account = "john.doe";
455462
var authResult = CreateAuthResult(account, accessToken);
@@ -462,7 +469,7 @@ public async Task AzureReposProvider_GetCredentialAsync_PatMode_OrgInUserName_No
462469
.ReturnsAsync(personalAccessToken);
463470

464471
var msAuthMock = new Mock<IMicrosoftAuthentication>(MockBehavior.Strict);
465-
msAuthMock.Setup(x => x.GetTokenForUserAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, null, true))
472+
msAuthMock.Setup(x => x.GetTokenForUserAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, expectedAccount, true))
466473
.ReturnsAsync(authResult);
467474

468475
var authorityCacheMock = new Mock<IAzureDevOpsAuthorityCache>(MockBehavior.Strict);
@@ -496,6 +503,7 @@ public async Task AzureReposProvider_GetCredentialAsync_PatMode_NoExistingPat_Ge
496503
var expectedRedirectUri = AzureDevOpsConstants.AadRedirectUri;
497504
var expectedScopes = AzureDevOpsConstants.AzureDevOpsDefaultScopes;
498505
var accessToken = "ACCESS-TOKEN";
506+
IMicrosoftAccount expectedAccount = null;
499507
var personalAccessToken = "PERSONAL-ACCESS-TOKEN";
500508
var account = "john.doe";
501509
var authResult = CreateAuthResult(account, accessToken);
@@ -508,7 +516,7 @@ public async Task AzureReposProvider_GetCredentialAsync_PatMode_NoExistingPat_Ge
508516
.ReturnsAsync(personalAccessToken);
509517

510518
var msAuthMock = new Mock<IMicrosoftAuthentication>(MockBehavior.Strict);
511-
msAuthMock.Setup(x => x.GetTokenForUserAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, null, true))
519+
msAuthMock.Setup(x => x.GetTokenForUserAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, expectedAccount, true))
512520
.ReturnsAsync(authResult);
513521

514522
var authorityCacheMock = new Mock<IAzureDevOpsAuthorityCache>(MockBehavior.Strict);

0 commit comments

Comments
 (0)