Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add suggested cache key for confidential client scenarios #1908

Merged
merged 6 commits into from
Jul 2, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/client/Microsoft.Identity.Client/AccountId.cs
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ public override bool Equals(object obj)
return false;
}

return string.Compare(Identifier, otherMsalAccountId.Identifier, StringComparison.OrdinalIgnoreCase) == 0;
return string.Equals(Identifier, otherMsalAccountId.Identifier, StringComparison.OrdinalIgnoreCase);
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,15 +101,15 @@ private async Task RefreshCacheForReadOperationsAsync(CacheEvent.TokenTypes cach
{
if (!_cacheRefreshedForRead) // double check locking
{

using (_requestParams.RequestContext.CreateTelemetryHelper(cacheEvent))
{
TokenCacheNotificationArgs args = new TokenCacheNotificationArgs(
TokenCacheInternal,
_requestParams.ClientId,
_requestParams.Account,
hasStateChanged: false,
TokenCacheInternal.IsApplicationCache);
TokenCacheInternal.IsApplicationCache,
_requestParams.SuggestedWebAppCacheKey);

try
{
Expand Down
80 changes: 50 additions & 30 deletions src/client/Microsoft.Identity.Client/ClientApplicationBase.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Copyright (c) Microsoft Corporation. All rights reserved.77
// Licensed under the MIT License.

using System;
Expand Down Expand Up @@ -75,50 +75,70 @@ internal ClientApplicationBase(ApplicationConfiguration config)
/// Returns all the available <see cref="IAccount">accounts</see> in the user token cache for the application.
/// </summary>
public async Task<IEnumerable<IAccount>> GetAccountsAsync()
{
return await GetAccountsAndSetCacheKeyAsync(null).ConfigureAwait(false);
}

/// <summary>
/// Returns all the available <see cref="IAccount">accounts</see> in the user token cache for the application.
/// Also sets the cache key based on a given home account id, which is the account id of the home account for the user.
/// This uniquely identifies the user across AAD tenants.
/// </summary>
/// <param name="homeAccountId">The identifier is the home account id of the account being targetted in the cache./>
/// </param>
private async Task<IEnumerable<IAccount>> GetAccountsAndSetCacheKeyAsync(string homeAccountId)
{
RequestContext requestContext = CreateRequestContext(Guid.NewGuid());
IEnumerable<IAccount> localAccounts = Enumerable.Empty<IAccount>();
IEnumerable<IAccount> brokerAccounts = Enumerable.Empty<IAccount>();

if (UserTokenCache == null)
{
if (!AppConfig.IsBrokerEnabled)
{
requestContext.Logger.Info("Token cache is null or empty. Returning empty list of accounts.");
}
}
else
{
// a simple session consisting of a single call
CacheSessionManager cacheSessionManager = new CacheSessionManager(
UserTokenCacheInternal,
new AuthenticationRequestParameters(
ServiceBundle,
UserTokenCacheInternal,
new AcquireTokenCommonParameters(),
requestContext));
var accountsFromCache = await GetAccountsFromCacheAsync(homeAccountId, requestContext).ConfigureAwait(false);
IEnumerable<IAccount> cacheAndBrokerAccounts =
Copy link
Member

@bgavrilMS bgavrilMS Jun 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small refactoring of this method, not related to your work. #Resolved

await MergeWithBrokerAccountsAsync(accountsFromCache).ConfigureAwait(false);


localAccounts = await cacheSessionManager.GetAccountsAsync(Authority).ConfigureAwait(false);
}
return cacheAndBrokerAccounts;
}

private async Task<IEnumerable<IAccount>> MergeWithBrokerAccountsAsync(IEnumerable<IAccount> accountsFromCache)
{
if (AppConfig.IsBrokerEnabled && ServiceBundle.PlatformProxy.CanBrokerSupportSilentAuth())
{
List<IAccount> allAccounts = new List<IAccount>(accountsFromCache);
//Broker is available so accounts will be merged using home account ID with local accounts taking priority
var broker = ServiceBundle.PlatformProxy.CreateBroker(null);
brokerAccounts = await broker.GetAccountsAsync(AppConfig.ClientId, AppConfig.RedirectUri).ConfigureAwait(false);
var brokerAccounts = await broker.GetAccountsAsync(AppConfig.ClientId, AppConfig.RedirectUri).ConfigureAwait(false);

foreach(IAccount account in brokerAccounts)
foreach (IAccount account in brokerAccounts)
{
if (!localAccounts.Any(x => x.HomeAccountId.Equals(account.HomeAccountId)))
if (!accountsFromCache.Any(x => x.HomeAccountId.Equals(account.HomeAccountId)))
{
(localAccounts as List<IAccount>).Add(account);
allAccounts.Add(account);
}
}

return localAccounts;
return allAccounts;
}

return localAccounts;
return accountsFromCache;
}

private async Task<IEnumerable<IAccount>> GetAccountsFromCacheAsync(
string homeAccountId,
RequestContext requestContext)
{
var authParameters = new AuthenticationRequestParameters(
ServiceBundle,
UserTokenCacheInternal,
new AcquireTokenCommonParameters(),
requestContext);

authParameters.SuggestedWebAppCacheKey = homeAccountId;
bgavrilMS marked this conversation as resolved.
Show resolved Hide resolved

// a simple session consisting of a single call
CacheSessionManager cacheSessionManager = new CacheSessionManager(
UserTokenCacheInternal,
authParameters);

return await cacheSessionManager.GetAccountsAsync(Authority).ConfigureAwait(false);
}

/// <summary>
Expand All @@ -136,7 +156,7 @@ public async Task<IEnumerable<IAccount>> GetAccountsAsync(string userFlow)

var accounts = await GetAccountsAsync().ConfigureAwait(false);

return accounts.Where(acc =>
return accounts.Where(acc =>
acc.HomeAccountId.ObjectId.Split('.')[0].EndsWith(
userFlow, StringComparison.OrdinalIgnoreCase));
}
Expand All @@ -150,7 +170,7 @@ public async Task<IEnumerable<IAccount>> GetAccountsAsync(string userFlow)
/// </param>
public async Task<IAccount> GetAccountAsync(string accountId)
{
var accounts = await GetAccountsAsync().ConfigureAwait(false);
var accounts = await GetAccountsAndSetCacheKeyAsync(accountId).ConfigureAwait(false);
return accounts.FirstOrDefault(account => account.HomeAccountId.Identifier.Equals(accountId, StringComparison.OrdinalIgnoreCase));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ public IEnumerable<KeyValuePair<string, string>> GetApiTelemetryFeatures()
public bool HasScopes => Scope != null && Scope.Any();

public string ClientId { get; }

public Uri RedirectUri { get; set; }

/// <summary>
Expand All @@ -86,6 +87,11 @@ public IEnumerable<KeyValuePair<string, string>> GetApiTelemetryFeatures()

public string ClaimsAndClientCapabilities { get; private set; }

/// <summary>
/// Used by Identity.Web (and others) to store token caches given the 1 cache per user pattern.
/// </summary>
public string SuggestedWebAppCacheKey { get; set; }

/// <summary>
/// Indicates if the user configured claims via .WithClaims. Not affected by Client Capabilities
/// </summary>
Expand All @@ -100,7 +106,7 @@ public string Claims

public AuthorityInfo AuthorityOverride => _commonParameters.AuthorityOverride;

internal bool IsBrokerConfigured { get; set; }
internal bool IsBrokerConfigured { get; set; /* set only for test */ }

public IAuthenticationScheme AuthenticationScheme => _commonParameters.AuthenticationScheme;

Expand Down Expand Up @@ -158,6 +164,13 @@ public void LogParameters()
builder.AppendLine("Redirect Uri - " + RedirectUri?.OriginalString);
builder.AppendLine("Extra Query Params Keys (space separated) - " + ExtraQueryParameters.Keys.AsSingleString());
builder.AppendLine("ClaimsAndClientCapabilities - " + ClaimsAndClientCapabilities);
builder.AppendLine("Authority - " + AuthorityInfo?.CanonicalAuthority);
builder.AppendLine("ApiId - " + ApiId);
builder.AppendLine("SuggestedCacheKey - " + SuggestedWebAppCacheKey);
builder.AppendLine("IsConfidentialClient - " + IsConfidentialClient);
builder.AppendLine("SendX5C - " + SendX5C);
builder.AppendLine("LoginHint - " + LoginHint);
builder.AppendLine("IsBrokerConfigured - " + IsBrokerConfigured);

string messageWithPii = builder.ToString();

Expand All @@ -168,6 +181,13 @@ public void LogParameters()
Environment.NewLine);
builder.AppendLine("Scopes - " + Scope?.AsSingleString());
builder.AppendLine("Extra Query Params Keys (space separated) - " + ExtraQueryParameters.Keys.AsSingleString());
builder.AppendLine("ApiId - " + ApiId);
builder.AppendLine("SuggestedCacheKey - " + SuggestedWebAppCacheKey);
builder.AppendLine("IsConfidentialClient - " + IsConfidentialClient);
builder.AppendLine("SendX5C - " + SendX5C);
builder.AppendLine("LoginHint ? " + !string.IsNullOrEmpty(LoginHint));
builder.AppendLine("IsBrokerConfigured - " + IsBrokerConfigured);

logger.InfoPii(messageWithPii, builder.ToString());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ protected override async Task<AuthenticationResult> ExecuteAsync(CancellationTok
if (!_clientParameters.ForceRefresh &&
string.IsNullOrEmpty(AuthenticationRequestParameters.Claims))
{
AuthenticationRequestParameters.SuggestedWebAppCacheKey = AuthenticationRequestParameters.ClientId + "_AppTokenCache";
cachedAccessTokenItem = await CacheManager.FindAccessTokenAsync().ConfigureAwait(false);

if (cachedAccessTokenItem != null && !cachedAccessTokenItem.NeedsRefresh())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ protected override async Task<AuthenticationResult> ExecuteAsync(CancellationTok
// or new assertion has been passed. We should not use Refresh Token
// for the user because the new incoming token may have updated claims
// like mfa etc.

AuthenticationRequestParameters.SuggestedWebAppCacheKey = AuthenticationRequestParameters.UserAssertion.AssertionHash;
MsalAccessTokenCacheItem msalAccessTokenItem = await CacheManager.FindAccessTokenAsync().ConfigureAwait(false);
if (msalAccessTokenItem != null)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,12 @@ public async Task PreRunAsync()
{
IAccount account = await GetAccountFromParamsOrLoginHintAsync(_silentParameters).ConfigureAwait(false);
AuthenticationRequestParameters.Account = account;
AuthenticationRequestParameters.SuggestedWebAppCacheKey = account.HomeAccountId?.Identifier;

AuthenticationRequestParameters.Authority = Authority.CreateAuthorityForRequest(
ServiceBundle.Config.AuthorityInfo,
AuthenticationRequestParameters.AuthorityOverride,
account?.HomeAccountId?.TenantId);
account.HomeAccountId?.TenantId);
}

public async Task<AuthenticationResult> ExecuteAsync(CancellationToken cancellationToken)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,13 +111,13 @@ async Task<Tuple<MsalAccessTokenCacheItem, MsalIdTokenCacheItem>> ITokenCacheInt
await _semaphoreSlim.WaitAsync().ConfigureAwait(false);
try
{

var args = new TokenCacheNotificationArgs(
this,
ClientId,
account,
hasStateChanged: true,
(this as ITokenCacheInternal).IsApplicationCache);
(this as ITokenCacheInternal).IsApplicationCache,
requestParams.SuggestedWebAppCacheKey);

#pragma warning disable CS0618 // Type or member is obsolete
HasStateChanged = true;
Expand Down Expand Up @@ -163,7 +163,7 @@ async Task<Tuple<MsalAccessTokenCacheItem, MsalIdTokenCacheItem>> ITokenCacheInt
if (!requestParams.IsClientCredentialRequest &&
requestParams.AuthorityInfo.AuthorityType != AuthorityType.B2C)
{
var authorityWithPrefferedCache = Authority.CreateAuthorityWithEnvironment(
var authorityWithPreferredCache = Authority.CreateAuthorityWithEnvironment(
requestParams.TenantUpdatedCanonicalAuthority.AuthorityInfo,
instanceDiscoveryMetadata.PreferredCache);

Expand All @@ -172,7 +172,7 @@ async Task<Tuple<MsalAccessTokenCacheItem, MsalIdTokenCacheItem>> ITokenCacheInt
LegacyCachePersistence,
msalRefreshTokenCacheItem,
msalIdTokenCacheItem,
authorityWithPrefferedCache.AuthorityInfo.CanonicalAuthority,
authorityWithPreferredCache.AuthorityInfo.CanonicalAuthority,
msalIdTokenCacheItem.IdToken.ObjectId,
response.Scope);
}
Expand Down Expand Up @@ -661,7 +661,13 @@ async Task ITokenCacheInternal.RemoveAccountAsync(IAccount account, RequestConte

try
{
var args = new TokenCacheNotificationArgs(this, ClientId, account, true, (this as ITokenCacheInternal).IsApplicationCache);
var args = new TokenCacheNotificationArgs(
this,
ClientId,
account,
true,
(this as ITokenCacheInternal).IsApplicationCache,
account.HomeAccountId.Identifier);

try
{
Expand Down Expand Up @@ -748,7 +754,13 @@ async Task ITokenCacheInternal.ClearAsync()
await _semaphoreSlim.WaitAsync().ConfigureAwait(false);
try
{
TokenCacheNotificationArgs args = new TokenCacheNotificationArgs(this, ClientId, null, true, (this as ITokenCacheInternal).IsApplicationCache);
TokenCacheNotificationArgs args = new TokenCacheNotificationArgs(
this,
ClientId,
null,
true,
(this as ITokenCacheInternal).IsApplicationCache,
null);

try
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,15 @@ internal TokenCacheNotificationArgs(
string clientId,
IAccount account,
bool hasStateChanged,
bool isAppCache)
bool isAppCache,
string suggestedCacheKey = null)
{
TokenCache = tokenCacheSerializer;
ClientId = clientId;
Account = account;
HasStateChanged = hasStateChanged;
IsApplicationCache = isAppCache;
SuggestedCacheKey = suggestedCacheKey;
}

/// <summary>
Expand Down Expand Up @@ -55,5 +57,21 @@ internal TokenCacheNotificationArgs(
/// See https://aka.ms/msal-net-app-cache-serialization for details.
/// </remarks>
public bool IsApplicationCache { get; }

/// <summary>
/// A suggested token cache key, which can be used with general purpose storage mechanisms that allow
/// storing key-value pairs and key based retrieval. Useful in applications that store 1 token cache per user,
/// the recommended pattern for web apps.
///
/// The value is:
///
/// <list type="bullet">
/// <item>the homeAccountId for AcquireTokenSilent and GetAccount(homeAccountId)</item>
/// <item>clientID + "_AppTokenCache" for AcquireTokenForClient</item>
/// <item>the hash of the original token for AcquireTokenOnBehalfOf</item>
/// <item>null for all other calls, such as PubliClientApplication calls, which should persist the token cache in a single location</item>
/// </list>
/// </summary>
public string SuggestedCacheKey { get; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added comments, please review. (CC @jmprieur )

}
}
Loading