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] [L] TokenCacheNotificationArgs should expose the token expiry so that cache can optimize eviction (client_credentials only) #2486

Closed
jmprieur opened this issue Mar 19, 2021 · 9 comments

Comments

@jmprieur
Copy link
Contributor

jmprieur commented Mar 19, 2021

Is your feature request related to a problem? Please describe.
In token cache serializers for confidential client applications:

  • for user flows, a RT is present in the cache and expiration cannot be determined, because tokens can be refreshed indefinitely.
  • for AcquireTokenForClient however, there is no RT and the expiration of the cache can be computed as the max over the expiration of each access token in cache.

Describe the solution you'd like

In TokenCacheNotificationArgs add a new property of type SuggestedCacheExpiry, and of the same type as the expiry in the token cache times (I suppose this is DateTimeOffset, but this needs to be checked).

This property will be computed by items.Max(item => item.ExpiresOnUnixTimestamp), and then convert that to a UtcDateTime.

class TokenCacheNotificationArgs
{
 // Other members

 /// <Summary>
 /// Suggested value of the expiry, to help determining the cache eviction time. 
 // This value is <b>only</b> set on the <code>OnAfterAcces</code> delegate, on a cache write
 /// operation (that is when <code>args.HasStateChanged<code> is <code>true</code>) and when the cache write 
 /// is triggered from the <code>AcquireTokenForClient<code> method. In all other cases it's <code>null<code>, as there is a refresh token, and therefore the
 /// access tokens are refreshable.
 /// </Summary> 
 UtcDateTime? SuggestedCacheExpiry {get; private set;}
@jmprieur
Copy link
Contributor Author

jmprieur commented Mar 24, 2021

Requires a bit more spec:

The problem is that the cache corresponding to an incoming token into a web app contains several OBO tokens. What should be the expiry? How do the OBO token expire vs the incoming token ? (for which MSAL.NET does not know the expiry)

@bgavrilMS
Copy link
Member

Needs more spec and a better understanding of OBO being able to refresh using RT or not.

@jmprieur jmprieur assigned jmprieur and unassigned pmaytak Mar 25, 2021
@bgavrilMS bgavrilMS removed this from the 4.29.0 milestone Mar 25, 2021
@jmprieur
Copy link
Contributor Author

Some things to consider:

  • For client credential tokens don't have a refresh token, so they could expire. The expiry of the cache could be the max of the expiry of the tokens in the cache
  • @jmprieur and @jennyf19 to propose an end to end (including what would happen in the app/Ms.Id.Web)

@jmprieur
Copy link
Contributor Author

After doing an investigation it appears that given the access tokens can be refreshed for 90d, this does not make sense to expose an expiry.
Closing this issue.

@bgavrilMS bgavrilMS reopened this May 26, 2021
@bgavrilMS bgavrilMS changed the title [Feature Request] TokenCacheNotificationArgs should expose the token expiry so that cache can optimize eviction [Feature Request] TokenCacheNotificationArgs should expose the token expiry so that cache can optimize eviction (client_credentials only) May 26, 2021
@bgavrilMS bgavrilMS changed the title [Feature Request] TokenCacheNotificationArgs should expose the token expiry so that cache can optimize eviction (client_credentials only) [Feature Request] [L] TokenCacheNotificationArgs should expose the token expiry so that cache can optimize eviction (client_credentials only) May 27, 2021
@bgavrilMS bgavrilMS self-assigned this Jun 1, 2021
@bgavrilMS bgavrilMS removed their assignment Jun 2, 2021
@bgavrilMS bgavrilMS added this to the 4.33.0 milestone Jun 3, 2021
@trwalke trwalke self-assigned this Jun 9, 2021
@jmprieur
Copy link
Contributor Author

@bgavrilMS: I've improved a bit the spec. do you want to check?
cc: @trwalke

@bgavrilMS
Copy link
Member

bgavrilMS commented Jun 11, 2021

Lgtm @jmprieur, thx for providing clarity. The return type should be nullable DateTimeOffset?, Since in most cases we will not set this.

Also, this property will only be set on OnAfterAccess callbacks.

@jmprieur
Copy link
Contributor Author

@bgavrilMS : yes it should be nullable, but I don't think we have nullable in MSAL.NET?
Yes, in the InAfterAccess, and probably only when (args.HasStateChanged) ?

@bgavrilMS
Copy link
Member

We have nullable structs since C# 3.0 or smth. The "nullable reference types" feature is C# 8.0 only, so MSAL can't use that.

Another way of achieving this is to set the expiry to max, via DateTimeOffset.MaxValue, but I feel that null is more descriptive.

@trwalke
Copy link
Member

trwalke commented Jul 8, 2021

released in 4.34.0

@trwalke trwalke closed this as completed Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants