Skip to content

Commit

Permalink
Workaround MSAL.NET issue with MSA-PT account silent auth (#1321)
Browse files Browse the repository at this point in the history
When we have a Microsoft Account (MSA) in the cache and attempt to do a
silent authentication, if we're an MSA-PT app we need to specify the
special MSA transfer tenant ID to make sure we get the a token silently,
correctly.

See the
[issue](AzureAD/microsoft-authentication-library-for-dotnet#3077)
in the MSAL repo for more information.

Fixes: #1297
  • Loading branch information
mjcheetham committed Jul 31, 2023
2 parents 2fcbd77 + 725ab49 commit 02ba62f
Show file tree
Hide file tree
Showing 5 changed files with 70 additions and 23 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public async System.Threading.Tasks.Task MicrosoftAuthentication_GetAccessTokenA
var msAuth = new MicrosoftAuthentication(context);

await Assert.ThrowsAsync<Trace2InvalidOperationException>(
() => msAuth.GetTokenAsync(authority, clientId, redirectUri, scopes, userName));
() => msAuth.GetTokenAsync(authority, clientId, redirectUri, scopes, userName, false));
}
}
}
60 changes: 47 additions & 13 deletions src/shared/Core/Authentication/MicrosoftAuthentication.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Net.Http;
using System.Threading.Tasks;
using GitCredentialManager.Interop.Windows.Native;
Expand All @@ -23,7 +24,7 @@ namespace GitCredentialManager.Authentication
public interface IMicrosoftAuthentication
{
Task<IMicrosoftAuthenticationResult> GetTokenAsync(string authority, string clientId, Uri redirectUri,
string[] scopes, string userName);
string[] scopes, string userName, bool msaPt = false);
}

public interface IMicrosoftAuthenticationResult
Expand Down Expand Up @@ -59,26 +60,31 @@ public MicrosoftAuthentication(ICommandContext context)
#region IMicrosoftAuthentication

public async Task<IMicrosoftAuthenticationResult> GetTokenAsync(
string authority, string clientId, Uri redirectUri, string[] scopes, string userName)
string authority, string clientId, Uri redirectUri, string[] scopes, string userName, bool msaPt)
{
// Check if we can and should use OS broker authentication
bool useBroker = CanUseBroker();
Context.Trace.WriteLine(useBroker
? "OS broker is available and enabled."
: "OS broker is not available or enabled.");

if (msaPt)
{
Context.Trace.WriteLine("MSA passthrough is enabled.");
}

try
{
// Create the public client application for authentication
IPublicClientApplication app = await CreatePublicClientApplicationAsync(authority, clientId, redirectUri, useBroker);
IPublicClientApplication app = await CreatePublicClientApplicationAsync(authority, clientId, redirectUri, useBroker, msaPt);

AuthenticationResult result = null;

// Try silent authentication first if we know about an existing user
bool hasExistingUser = !string.IsNullOrWhiteSpace(userName);
if (hasExistingUser)
{
result = await GetAccessTokenSilentlyAsync(app, scopes, userName);
result = await GetAccessTokenSilentlyAsync(app, scopes, userName, msaPt);
}

//
Expand Down Expand Up @@ -116,7 +122,7 @@ public MicrosoftAuthentication(ICommandContext context)
// account then the user may become stuck in a loop of authentication failures.
if (!hasExistingUser && Context.Settings.UseMsAuthDefaultAccount)
{
result = await GetAccessTokenSilentlyAsync(app, scopes, null);
result = await GetAccessTokenSilentlyAsync(app, scopes, null, msaPt);

if (result is null || !await UseDefaultAccountAsync(result.Account.Username))
{
Expand Down Expand Up @@ -281,34 +287,62 @@ internal MicrosoftAuthenticationFlowType GetFlowType()
/// <summary>
/// Obtain an access token without showing UI or prompts.
/// </summary>
private async Task<AuthenticationResult> GetAccessTokenSilentlyAsync(IPublicClientApplication app, string[] scopes, string userName)
private async Task<AuthenticationResult> GetAccessTokenSilentlyAsync(
IPublicClientApplication app, string[] scopes, string userName, bool msaPt)
{
try
{
if (userName is null)
{
Context.Trace.WriteLine("Attempting to acquire token silently for current operating system account...");
Context.Trace.WriteLine(
"Attempting to acquire token silently for current operating system account...");

return await app.AcquireTokenSilent(scopes, PublicClientApplication.OperatingSystemAccount).ExecuteAsync();
return await app.AcquireTokenSilent(scopes, PublicClientApplication.OperatingSystemAccount)
.ExecuteAsync();
}
else
{
Context.Trace.WriteLine($"Attempting to acquire token silently for user '{userName}'...");

// We can either call `app.GetAccountsAsync` and filter through the IAccount objects for the instance with the correct user name,
// or we can just pass the user name string we have as the `loginHint` and let MSAL do exactly that for us instead!
return await app.AcquireTokenSilent(scopes, loginHint: userName).ExecuteAsync();
// Enumerate all accounts and find the one matching the user name
IEnumerable<IAccount> accounts = await app.GetAccountsAsync();
IAccount account = accounts.FirstOrDefault(x =>
StringComparer.OrdinalIgnoreCase.Equals(x.Username, userName));
if (account is null)
{
Context.Trace.WriteLine($"No cached account found for user '{userName}'...");
return null;
}

var atsBuilder = app.AcquireTokenSilent(scopes, account);

// Is we are operating with an MSA passthrough app we need to ensure that we target the
// special MSA 'transfer' tenant explicitly. This is a workaround for MSAL issue:
// https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/issues/3077
if (msaPt && Guid.TryParse(account.HomeAccountId.TenantId, out Guid homeTenantId) &&
homeTenantId == Constants.MsaHomeTenantId)
{
atsBuilder = atsBuilder.WithTenantId(Constants.MsaTransferTenantId.ToString("D"));
}

return await atsBuilder.ExecuteAsync();
}
}
catch (MsalUiRequiredException)
{
Context.Trace.WriteLine("Failed to acquire token silently; user interaction is required.");
return null;
}
catch (Exception ex)
{
Context.Trace.WriteLine("Failed to acquire token silently.");
Context.Trace.WriteException(ex);
return null;
}
}

private async Task<IPublicClientApplication> CreatePublicClientApplicationAsync(
string authority, string clientId, Uri redirectUri, bool enableBroker)
string authority, string clientId, Uri redirectUri, bool enableBroker, bool msaPt)
{
var httpFactoryAdaptor = new MsalHttpClientFactoryAdaptor(Context.HttpClientFactory);

Expand Down Expand Up @@ -370,7 +404,7 @@ private async Task<AuthenticationResult> GetAccessTokenSilentlyAsync(IPublicClie
new BrokerOptions(BrokerOptions.OperatingSystems.Windows)
{
Title = "Git Credential Manager",
MsaPassthrough = true,
MsaPassthrough = msaPt,
}
);
#endif
Expand Down
11 changes: 11 additions & 0 deletions src/shared/Core/Constants.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,17 @@ public static class Constants

public static readonly Guid DevBoxPartnerId = new("e3171dd9-9a5f-e5be-b36c-cc7c4f3f3bcf");

/// <summary>
/// Home tenant ID for Microsoft Accounts (MSA).
/// </summary>
public static readonly Guid MsaHomeTenantId = new("9188040d-6c67-4c5b-b112-36a304b66dad");

/// <summary>
/// Special tenant ID for transferring between Microsoft Account (MSA) native tokens
/// and AAD tokens. Only required for MSA-Passthrough applications.
/// </summary>
public static readonly Guid MsaTransferTenantId = new("f8cdef31-a31e-4b4a-93e4-5f571e91255a");

public static class CredentialStoreNames
{
public const string WindowsCredentialManager = "wincredman";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ public async Task AzureReposProvider_GetCredentialAsync_JwtMode_CachedAuthority_
azDevOpsMock.Setup(x => x.GetAuthorityAsync(expectedOrgUri)).ReturnsAsync(authorityUrl);

var msAuthMock = new Mock<IMicrosoftAuthentication>(MockBehavior.Strict);
msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, urlAccount))
msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, urlAccount, true))
.ReturnsAsync(authResult);

var authorityCacheMock = new Mock<IAzureDevOpsAuthorityCache>(MockBehavior.Strict);
Expand Down Expand Up @@ -219,7 +219,7 @@ public async Task AzureReposProvider_GetCredentialAsync_JwtMode_CachedAuthority_
azDevOpsMock.Setup(x => x.GetAuthorityAsync(expectedOrgUri)).ReturnsAsync(authorityUrl);

var msAuthMock = new Mock<IMicrosoftAuthentication>(MockBehavior.Strict);
msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, urlAccount))
msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, urlAccount, true))
.ReturnsAsync(authResult);

var authorityCacheMock = new Mock<IAzureDevOpsAuthorityCache>(MockBehavior.Strict);
Expand Down Expand Up @@ -268,7 +268,7 @@ public async Task AzureReposProvider_GetCredentialAsync_JwtMode_CachedAuthority_
azDevOpsMock.Setup(x => x.GetAuthorityAsync(expectedOrgUri)).ReturnsAsync(authorityUrl);

var msAuthMock = new Mock<IMicrosoftAuthentication>(MockBehavior.Strict);
msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, null))
msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, null, true))
.ReturnsAsync(authResult);

var authorityCacheMock = new Mock<IAzureDevOpsAuthorityCache>(MockBehavior.Strict);
Expand Down Expand Up @@ -315,7 +315,7 @@ public async Task AzureReposProvider_GetCredentialAsync_JwtMode_CachedAuthority_
var azDevOpsMock = new Mock<IAzureDevOpsRestApi>(MockBehavior.Strict);

var msAuthMock = new Mock<IMicrosoftAuthentication>(MockBehavior.Strict);
msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, null))
msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, null, true))
.ReturnsAsync(authResult);

var authorityCacheMock = new Mock<IAzureDevOpsAuthorityCache>(MockBehavior.Strict);
Expand Down Expand Up @@ -363,7 +363,7 @@ public async Task AzureReposProvider_GetCredentialAsync_JwtMode_CachedAuthority_
var azDevOpsMock = new Mock<IAzureDevOpsRestApi>(MockBehavior.Strict);

var msAuthMock = new Mock<IMicrosoftAuthentication>(MockBehavior.Strict);
msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, account))
msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, account, true))
.ReturnsAsync(authResult);

var authorityCacheMock = new Mock<IAzureDevOpsAuthorityCache>(MockBehavior.Strict);
Expand Down Expand Up @@ -413,7 +413,7 @@ public async Task AzureReposProvider_GetCredentialAsync_JwtMode_NoCachedAuthorit
azDevOpsMock.Setup(x => x.GetAuthorityAsync(expectedOrgUri)).ReturnsAsync(authorityUrl);

var msAuthMock = new Mock<IMicrosoftAuthentication>(MockBehavior.Strict);
msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, null))
msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, null, true))
.ReturnsAsync(authResult);

var authorityCacheMock = new Mock<IAzureDevOpsAuthorityCache>(MockBehavior.Strict);
Expand Down Expand Up @@ -462,7 +462,7 @@ public async Task AzureReposProvider_GetCredentialAsync_PatMode_NoExistingPat_Ge
.ReturnsAsync(personalAccessToken);

var msAuthMock = new Mock<IMicrosoftAuthentication>(MockBehavior.Strict);
msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, null))
msAuthMock.Setup(x => x.GetTokenAsync(authorityUrl, expectedClientId, expectedRedirectUri, expectedScopes, null, true))
.ReturnsAsync(authResult);

var authorityCacheMock = new Mock<IAzureDevOpsAuthorityCache>(MockBehavior.Strict);
Expand Down
6 changes: 4 additions & 2 deletions src/shared/Microsoft.AzureRepos/AzureReposHostProvider.cs
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,8 @@ private async Task<ICredential> GeneratePersonalAccessTokenAsync(InputArguments
GetClientId(),
GetRedirectUri(),
AzureDevOpsConstants.AzureDevOpsDefaultScopes,
null);
null,
msaPt: true);
_context.Trace.WriteLineSecrets(
$"Acquired Azure access token. Account='{result.AccountUpn}' Token='{{0}}'", new object[] {result.AccessToken});

Expand Down Expand Up @@ -293,7 +294,8 @@ private async Task<IMicrosoftAuthenticationResult> GetAzureAccessTokenAsync(Inpu
GetClientId(),
GetRedirectUri(),
AzureDevOpsConstants.AzureDevOpsDefaultScopes,
userName);
userName,
msaPt: true);
_context.Trace.WriteLineSecrets(
$"Acquired Azure access token. Account='{result.AccountUpn}' Token='{{0}}'", new object[] {result.AccessToken});

Expand Down

0 comments on commit 02ba62f

Please sign in to comment.