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.

Sign up for GitHub

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

Add suggested cache key for confidential client scenarios #1908

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6c8d8b7
initial commit for adding a suggested cache key to the token args
jennyf19 Jun 26, 2020
22cd934
Update for cleaner use
bgavrilMS Jun 29, 2020
db69c6a
add a few more tests for cache key
jennyf19 Jun 30, 2020
2335ed0
fix tests
jennyf19 Jul 1, 2020
b854c1e
More tests
bgavrilMS Jul 1, 2020
0bf303a
Test
bgavrilMS Jul 1, 2020
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
4 changes: 2 additions & 2 deletions src/client/Microsoft.Identity.Client/Cache/CacheSessionManager.cs
View file
Open in desktop
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.SuggestedCacheKey ?? _requestParams.Account?.HomeAccountId?.Identifier);

try
{
Expand Down
27 changes: 21 additions & 6 deletions src/client/Microsoft.Identity.Client/ClientApplicationBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,18 @@ 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>();
Expand All @@ -89,14 +101,17 @@ public async Task<IEnumerable<IAccount>> GetAccountsAsync()
}
else
{
var authParameters = new AuthenticationRequestParameters(
ServiceBundle,
UserTokenCacheInternal,
new AcquireTokenCommonParameters(),
requestContext);

authParameters.Account = new Account(homeAccountId, null, Authority);
jmprieur marked this conversation as resolved.
Show resolved Hide resolved
// a simple session consisting of a single call
CacheSessionManager cacheSessionManager = new CacheSessionManager(
UserTokenCacheInternal,
new AuthenticationRequestParameters(
ServiceBundle,
UserTokenCacheInternal,
new AcquireTokenCommonParameters(),
requestContext));
authParameters);

localAccounts = await cacheSessionManager.GetAccountsAsync(Authority).ConfigureAwait(false);
}
Expand Down Expand Up @@ -150,7 +165,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
2 changes: 2 additions & 0 deletions src/client/Microsoft.Identity.Client/Internal/Requests/AuthenticationRequestParameters.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,8 @@ public IEnumerable<KeyValuePair<string, string>> GetApiTelemetryFeatures()

public string ClaimsAndClientCapabilities { get; private set; }

public string SuggestedCacheKey { get; set; }

/// <summary>
/// Indicates if the user configured claims via .WithClaims. Not affected by Client Capabilities
/// </summary>
Expand Down
1 change: 1 addition & 0 deletions src/client/Microsoft.Identity.Client/Internal/Requests/ClientCredentialRequest.cs
View file
Open in desktop
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.SuggestedCacheKey = AuthenticationRequestParameters.ClientId + "_AppTokenCache";
cachedAccessTokenItem = await CacheManager.FindAccessTokenAsync().ConfigureAwait(false);

if (cachedAccessTokenItem != null && !cachedAccessTokenItem.NeedsRefresh())
Expand Down
2 changes: 1 addition & 1 deletion src/client/Microsoft.Identity.Client/Internal/Requests/OnBehalfOfRequest.cs
View file
Open in desktop
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.SuggestedCacheKey = AuthenticationRequestParameters.UserAssertion.AssertionHash;
MsalAccessTokenCacheItem msalAccessTokenItem = await CacheManager.FindAccessTokenAsync().ConfigureAwait(false);
if (msalAccessTokenItem != null)
{
Expand Down
1 change: 1 addition & 0 deletions src/client/Microsoft.Identity.Client/Internal/Requests/Silent/SilentRequest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ protected override async Task<AuthenticationResult> ExecuteAsync(CancellationTok
{
try
{
AuthenticationRequestParameters.SuggestedCacheKey = _silentParameters.Account?.HomeAccountId?.Identifier;
_logger.Info("Attempting to acquire token using using local cache...");
return await _clientStrategy.ExecuteAsync(cancellationToken).ConfigureAwait(false);
}
Expand Down
24 changes: 18 additions & 6 deletions src/client/Microsoft.Identity.Client/TokenCache.ITokenCacheInternal.cs
View file
Open in desktop
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.SuggestedCacheKey ?? homeAccountId);

#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
9 changes: 8 additions & 1 deletion src/client/Microsoft.Identity.Client/TokenCacheNotificationArgs.cs
View file
Open in desktop
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,10 @@ internal TokenCacheNotificationArgs(
/// See https://aka.ms/msal-net-app-cache-serialization for details.
/// </remarks>
public bool IsApplicationCache { get; }

/// <summary>
///
/// </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 )

}
}
20 changes: 18 additions & 2 deletions ...osoft.Identity.Test.Integration.net45/HeadlessTests/ConfidentialClientIntegrationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ public class ConfidentialClientIntegrationTests
private const string RedirectUri = "https://login.microsoftonline.com/common/oauth2/nativeclient";
private const string PublicCloudTestAuthority = "https://login.windows.net/72f988bf-86f1-41af-91ab-2d7cd011db47";
private const string AdfsCertName = "IDLABS-APP-Confidential-Client-Cert-OnPrem";
private const string AppCacheKey = "16dab2ba-145d-4b1b-8569-bf4b9aed4dc8_AppTokenCache";
private KeyVaultSecretsProvider _keyVault;
private static string _publicCloudCcaSecret;
private static string _arlingtonCCASecret;
Expand Down Expand Up @@ -131,6 +132,7 @@ public async Task ConfidentialClientWithCertificateTestAsync()
MsalAssert.AssertAuthResult(authResult);
appCacheRecorder.AssertAccessCounts(1, 1);
Assert.IsTrue(appCacheRecorder.LastNotificationArgs.IsApplicationCache);
Assert.AreEqual(AppCacheKey, appCacheRecorder.LastNotificationArgs.SuggestedCacheKey);

// Call again to ensure token cache is hit
authResult = await confidentialApp
Expand All @@ -141,6 +143,7 @@ public async Task ConfidentialClientWithCertificateTestAsync()
MsalAssert.AssertAuthResult(authResult);
appCacheRecorder.AssertAccessCounts(2, 1);
Assert.IsTrue(appCacheRecorder.LastNotificationArgs.IsApplicationCache);
Assert.AreEqual(AppCacheKey, appCacheRecorder.LastNotificationArgs.SuggestedCacheKey);
Copy link
Member

@bgavrilMS bgavrilMS Jun 29, 2020 •
edited by jennyf19
Loading

Choose a reason for hiding this comment

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

Integration tests seem to only cover AcquireTokenForClient and OBO, but not AcquireTokenSilent.
We need some unit test coverage as well - should be easy to update some existing tests for Client Creds and OBO. For AcquireTokenSilent as well. For GetAccount I think you need a dedicated unit test. #Resolved

}

[TestMethod]
Expand All @@ -166,6 +169,7 @@ public async Task ConfidentialClientWithRSACertificateTestAsync()
MsalAssert.AssertAuthResult(authResult);
appCacheRecorder.AssertAccessCounts(1, 1);
Assert.IsTrue(appCacheRecorder.LastNotificationArgs.IsApplicationCache);
Assert.AreEqual(AppCacheKey, appCacheRecorder.LastNotificationArgs.SuggestedCacheKey);

// Call again to ensure token cache is hit
authResult = await confidentialApp
Expand All @@ -176,6 +180,7 @@ public async Task ConfidentialClientWithRSACertificateTestAsync()
MsalAssert.AssertAuthResult(authResult);
appCacheRecorder.AssertAccessCounts(2, 1);
Assert.IsTrue(appCacheRecorder.LastNotificationArgs.IsApplicationCache);
Assert.AreEqual(AppCacheKey, appCacheRecorder.LastNotificationArgs.SuggestedCacheKey);
}

[TestMethod]
Expand Down Expand Up @@ -214,6 +219,7 @@ public async Task RunTestWithClientSecretAsync(string clientID, string authority
MsalAssert.AssertAuthResult(authResult);
appCacheRecorder.AssertAccessCounts(1, 1);
Assert.IsTrue(appCacheRecorder.LastNotificationArgs.IsApplicationCache);
Assert.AreEqual(AppCacheKey, appCacheRecorder.LastNotificationArgs.SuggestedCacheKey);

// Call again to ensure token cache is hit
authResult = await confidentialApp.AcquireTokenForClient(s_keyvaultScope)
Expand All @@ -223,6 +229,7 @@ public async Task RunTestWithClientSecretAsync(string clientID, string authority
MsalAssert.AssertAuthResult(authResult);
appCacheRecorder.AssertAccessCounts(2, 1);
Assert.IsTrue(appCacheRecorder.LastNotificationArgs.IsApplicationCache);
Assert.AreEqual(AppCacheKey, appCacheRecorder.LastNotificationArgs.SuggestedCacheKey);
}

[TestMethod]
Expand Down Expand Up @@ -299,6 +306,7 @@ public async Task ConfidentialClientWithSignedAssertionTestAsync()

appCacheRecorder.AssertAccessCounts(1, 1);
Assert.IsTrue(appCacheRecorder.LastNotificationArgs.IsApplicationCache);
Assert.AreEqual(AppCacheKey, appCacheRecorder.LastNotificationArgs.SuggestedCacheKey);
ValidateClaimsInAssertion(claims, ((ConfidentialClientApplication)confidentialApp).ClientCredential.SignedAssertion);
MsalAssert.AssertAuthResult(authResult);

Expand All @@ -309,6 +317,7 @@ public async Task ConfidentialClientWithSignedAssertionTestAsync()

appCacheRecorder.AssertAccessCounts(2, 1);
Assert.IsTrue(appCacheRecorder.LastNotificationArgs.IsApplicationCache);
Assert.AreEqual(AppCacheKey, appCacheRecorder.LastNotificationArgs.SuggestedCacheKey);
}

private void ValidateClaimsInAssertion(IDictionary<string, string> claims, string assertion)
Expand Down Expand Up @@ -543,7 +552,7 @@ private async Task RunOnBehalfOfTestAsync(LabResponse labResponse)
break;
}

//TODO: acquire scenario specific client ids from the lab resonse
//TODO: acquire scenario specific client ids from the lab response

SecureString securePassword = new NetworkCredential("", user.GetOrFetchPassword()).SecurePassword;

Expand All @@ -566,11 +575,18 @@ private async Task RunOnBehalfOfTestAsync(LabResponse labResponse)
.WithTestLogging()
.Build();

authResult = await confidentialApp.AcquireTokenOnBehalfOf(s_scopes, new UserAssertion(authResult.AccessToken))
var userCacheRecorder = confidentialApp.UserTokenCache.RecordAccess();

UserAssertion userAssertion = new UserAssertion(authResult.AccessToken);

string atHash = userAssertion.AssertionHash;

authResult = await confidentialApp.AcquireTokenOnBehalfOf(s_scopes, userAssertion)
.ExecuteAsync(CancellationToken.None)
.ConfigureAwait(false);

MsalAssert.AssertAuthResult(authResult, user);
Assert.AreEqual(atHash, userCacheRecorder.LastNotificationArgs.SuggestedCacheKey);
}
}
}