Add suggested cache key for confidential client scenarios #1908
Conversation
jennyf19
force-pushed
the
jennyf/cacheKey
branch
from
1059c32 to
6c8d8b7
Compare
| @@ -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); | |||
There was a problem hiding this comment.
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
| new AcquireTokenCommonParameters(), | ||
| requestContext)); | ||
| var accountsFromCache = await GetAccountsFromCacheAsync(homeAccountId, requestContext).ConfigureAwait(false); | ||
| IEnumerable<IAccount> cacheAndBrokerAccounts = |
There was a problem hiding this comment.
Small refactoring of this method, not related to your work. #Resolved
| /// <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; } |
There was a problem hiding this comment.
I added comments, please review. (CC @jmprieur )
| Assert.AreEqual(homeAccId, cacheAccess.LastNotificationArgs.SuggestedCacheKey); | ||
|
|
||
| // Act | ||
| await app.AcquireTokenSilent(TestConstants.s_scope, accountById.Username).ExecuteAsync().ConfigureAwait(false); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Yes, @bgavrilMS : we need to discuss.
this API, as well as GetAccountsAsync() don't make sense for confidential client applications.
but this is a different problem. I was even thinking of not proposing them any longer for CCA (this would be a breaking change)
There was a problem hiding this comment.
We can duplicate GetAccountsAsync methods so that they exist on both PCA and CCA. And then Obsolete them on CCA. But why would I not want to do GetAccountsAsync on a cca?
|
@bgavrilMS the tests don't seem to pass. i notice the same for your PR...its that expected? |
|
@jennyf19 - on my PR I have a unit test failing, I'll take care of this. On this PR some unrelated integration tests are failing. Let me rerun the build. |
|
@bgavrilMS sounds good. go ahead and merge whenever you want. ty |
This is for #1902 for confidential client scenarios, and has more info on the issue.
[ ] OBO should use the AT hash as the cache key, because the claims in the AT can change, so we use the AT directly to determine the cache key so the right AT is used.
[ ] Client credentials uses
{client_id} + "_AppTokenCache", as it's using the app token cache, so it needs the token based on the client id. MS Identity Web suffixes it w/"_AppTokenCache", so we'd like to keep this pattern to not break current customers.[ ] all other confidential client flows (including silent) use the home account id, as the key, because there is a user account.
In order to make the changes in MS Identity Web, we'd need to have this addition included in a nuget package. We have a deadline of July 6 on our end. cc: @jmprieur @henrik-me