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

[Feature Request] In memory token cache in confidential client application is well partitioned for all flows, non longer serialized and can be shared between instances of confidential client apps #2861

Closed
jmprieur opened this issue Sep 2, 2021 · 7 comments · Fixed by #2881
Assignees

Comments

@jmprieur
Copy link
Contributor

jmprieur commented Sep 2, 2021

Is your feature request related to a problem? Please describe.
For context see Token cache vision 1-pager

Describe the solution you'd like
Out of the box, confidential client applications should internally have a token cache which is:

P1:

ConfidentialClientApplicationBuilder.Create(clientId)
                                                          .WithDefaultCache(shared: true) // Shares the cache between instances of CCA

P2:

ConfidentialClientApplicationBuilder.Create(clientId)
                                                          .WithDefaultCache(shared: false,
                                                           new EvictionParameters(sizeSimit:1000,
                                                                                                  slidingExpiration: TimeSpan.FromMinutes(30) )
                                                          );

Describe alternatives you've considered
see Token cache vision 1-pager

Additional context
This issue overrides:

@bgavrilMS
Copy link
Member

bgavrilMS commented Sep 3, 2021

Flow 1 - TokenCacheAdapter to move to MSAL (P2 - not required to solve this problem)

- interface vs abstract class
- force encryption, i.e. add Protect / Unprotect to interface ?
- API review
- MSAL Implementation
- MIW implementation

IdWeb work

- Split the package
- Hook into the new MSAL interface 
- A more aggressive way to force people to use encryption 

Flow 2 - Well partitioned cache for User Cache (lots of testing required) P1

Flow 3 - Static option in MSAL P0

- API design 
- Add additional constraints (e.g. it is dangerous to use static internal cache + external serialization - ban this)
- API review
- evition based on number of items
- Implementation 

IdWeb work (P1)

  • AddInMemoryTokenCache to call new MSAL method

Better eviction (P2)

- Additional eviction policies
- Wilson LRU

Flow 4 - Emit warnings for bad usage of MSAL P0

Emit warnings / errors when we detect bad usage of MSAL.

1. Choose an initial delivery mechanism (Log.Error, Telemetry, Exceptions, Roslyn Analyzer)
2. How would SDKs proxy this mechanism?
3. Formalize some rules:

a. Cache serialization not used for web app / web api scenarios (we have a log.error for this today)
b. Encryption at rest for external serialization not used?
c. Web app / Web api + ADAL support used but Serialize methods not used?

4. Api review
    5. Implementation

@jennyf19
Copy link
Collaborator

jennyf19 commented Sep 3, 2021

What do you mean by flows?
also, i have a slightly different perspective on flow 1 and the id web work and have already made a plan for it.

as discussed yesterday, none of the work, except removing the id web memory cache is dependent on the other work.

nit: can you write out id web or microsoft.identity.web so it's clear what you are referring to? we never use an acronym for it. thx

should this be in the one-pager or a separate doc so we can comment more easily?

@bgavrilMS
Copy link
Member

bgavrilMS commented Sep 3, 2021

Just work streams for the problem we are trying to solve. The work streams can be tacked in parallel. The first one is indeed not mandatory for tackling this problem and we should take the time to design right.

@pmaytak is looking at 2, @trwalke is looking at 4, and I'm looking at 3

For 1, since Id web is taking a breaking change, let's make sure we get the right token cache interface in place. Since the interface goes into MSAL, the design needs to go in an API governance doc at some point, so that other MSALs can benefit from it. Until then - whatever is simpler for you (1-pager, github issue, api review doc etc) is fine. @trwalke also looked so looking forward to his comments / help.

Would also like your opinion the rest of the work items, especially 4.

@jennyf19
Copy link
Collaborator

jennyf19 commented Sep 3, 2021

Flow 1 is actually two separate items.
One is separating Id Web into smaller nuget packages for fewer dependencies for .NET Framework.

  • this work is in progress
  • we might take a major version for this (2.*), as is a binary breaking change
  • involves a good amount of testing w/samples and 1P.

Two is moving the MsalAbstractTokenAdapter to MSAL.NET

  • not dependent on any other tasks/flows.
  • no breaking change for Id Web
  • i'll sync w/ @bgavrilMS next week on this.

@zhenyar
Copy link

zhenyar commented Sep 22, 2021

Creating a default MSAL token cache that works for all auth flows would definitely make ADAL to MSAL migration much easier! I just want to add that my service has code that clears the ADAL's token cache sometimes, so this may be something to think about when developing the MSAL's token cache or maybe "multiple_matching_tokens_detected" is not something that'll ever happen with MSAL.

        catch (AdalException e)
        {
            if (e.Message.Contains("multiple_matching_tokens_detected"))
            {
                authenticationContext.TokenCache.Clear();
                var result = await authenticationContext.AcquireTokenAsync(resourceUri, clientId, userCredential).ConfigureAwait(false);
                return result.AccessToken;
            }
        }

Here's another piece of code that deletes a specific item from ADAL's token cache:
lock (adalCacheLock)
{
var item = authenticationContext.TokenCache.ReadItems()
.FirstOrDefault(i => userName.Equals(i.DisplayableId, StringComparison.OrdinalIgnoreCase)
&& clientId.Equals(i.ClientId, StringComparison.OrdinalIgnoreCase)
&& resourceUri.Equals(i.Resource, StringComparison.OrdinalIgnoreCase));
if (item != null)
{
authenticationContext.TokenCache.DeleteItem(item);
}
}

The above code is used to get a fresh MS Graph token using a native app and username/password flow in a multi-tenant scenario. Here's the full code:

    public Task<string> AuthenticateUserToTenantWithFreshTokenAsync(
        string resourceUri,
        string clientId,
        string authorityFormatString,
        string tenant,
        string userName,
        string password)
    {
        var authority = string.Format(authorityFormatString, tenant);
        return AuthenticateUserWithFreshTokenAsync(resourceUri, clientId, authority, userName, password);
    }

    private async Task<string> AuthenticateUserWithFreshTokenAsync(
        string resourceUri,
        string clientId,
        string authority,
        string userName,
        string password)
    {
        var authenticationContext = new AuthenticationContext(authority);
        var userCredential = new UserPasswordCredential(userName, password);

        try
        {
            lock (adalCacheLock)
            {
                var item = authenticationContext.TokenCache.ReadItems()
                    .FirstOrDefault(i => userName.Equals(i.DisplayableId, StringComparison.OrdinalIgnoreCase)
                                        && clientId.Equals(i.ClientId, StringComparison.OrdinalIgnoreCase)
                                        && resourceUri.Equals(i.Resource, StringComparison.OrdinalIgnoreCase));
                if (item != null)
                {
                    authenticationContext.TokenCache.DeleteItem(item);
                }
            }

            var result = await authenticationContext.AcquireTokenAsync(resourceUri, clientId, userCredential).ConfigureAwait(false);
            return result.AccessToken;
        }
        catch (AdalException e)
        {
            if (e.Message.Contains("multiple_matching_tokens_detected"))
            {
                authenticationContext.TokenCache.Clear();
                var result = await authenticationContext.AcquireTokenAsync(resourceUri, clientId, userCredential).ConfigureAwait(false);
                return result.AccessToken;
            }
            else
            {
                throw;
            }
        }
    }

I don't really know why we had to write this code. Folks who did it have left the company. I do wonder what to do with this code during ADAL to MSAL migration. Thanks.

@jmprieur
Copy link
Contributor Author

@zhenyar. I think that this was defensive code due to the fact that ADAL's cache had not initially been designed to support some scenarios. You shouldn't need to do any of that with MSAL. I would recommend you just delete this code.

@zhenyar
Copy link

zhenyar commented Sep 28, 2021

When is the default cache support expected to be available in Microsoft.Identity.Client? Will the default cache support multi-tenant OBO, username/password and client credentials auth flows?

Unfortunately Microsoft.Identity.Web.TokenCache is not going to work for me due to nuget hell.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants