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

Question - Recommended expiration setting #786

Closed
dmcweeney opened this issue Nov 25, 2020 · 8 comments
Closed

Question - Recommended expiration setting #786

dmcweeney opened this issue Nov 25, 2020 · 8 comments
Labels
answered question Further information is requested

Comments

@dmcweeney
Copy link

Hi,

Quick question - I'm wondering what you recommended SlidingExpiration setting would/should be in relation to token lifetimes when using the distributed cache.

Thanks, Donal

@jmprieur jmprieur added the question Further information is requested label Nov 25, 2020
@jmprieur
Copy link
Collaborator

@dmcweeney : this is an interesting question, but we'd want to understand your scenario a bit more:

  • do you have any concern?
  • do you see any issue?

@dmcweeney
Copy link
Author

Hi @jmprieur,

No concerns from a security perspective.
Thinking in terms of ensuring that the tokens are auto-purged/expired in a timely manner.
For example,

  • you may have trial users that access the app and don't come back
  • and trial users that access the app and do come back a couple of times during a trial.
  • and "active" users that might access the app once a month
  • and "active" users that are actively accessing the app daily/weekly
  • and "active" users that may move on to another company and will never come back into the app.

So it is really trying to balance the user accessing the app versus token purge (which would require the user to sign back into the app again) versus redis cache memory usage?

Any thoughts greatly appreciated.

Thanks, Donal

@Mek7
Copy link

Mek7 commented Dec 11, 2020

Hi,
I had the same question in the linked issue. It's not like any problem or bug, just a design decision.
Now that I think of it, maybe we could set the tokens to never expire. Over time, they will get invalid and MSAL figures it out and forces re-auth anyway, correct?

@dmcweeney
Copy link
Author

@Mek7 correct its a design decision related to picking a good number...

Over time, they will get invalid and MSAL figures it out and forces re-auth anyway, correct

Correct however if the user does not come back to login, the entry stays in the redis cache and it will never get expired, eventually using all the allocated cache storage.

@jmprieur
Copy link
Collaborator

Thanks @Mek7 @dmcweeney for this discussion
I've added a few sentences to the wiki: https://github.com/AzureAD/microsoft-identity-web/wiki/Token-cache-serialization#recommended-expiration-setting

Closing this issue for now.
Feel free to reopen if you disagree

@creativebrother
Copy link
Contributor

Thanks @Mek7 @dmcweeney for this discussion
I've added a few sentences to the wiki: https://github.com/AzureAD/microsoft-identity-web/wiki/Token-cache-serialization#recommended-expiration-setting

Closing this issue for now.
Feel free to reopen if you disagree

Hi @jmprieur, so if we have access token life time of 1 hour, we should set the cache expiration to, say 90 minutes to balance the cache storage and re-authentication traffic(give the refresh token a chance to work?).
When using the default expiration of 20 minutes, the refresh token is never used because the ConfidentialClientApplication.GetAccountAsync(Id) will get nothing from cache due to the triggered cache purge on read, which leads to AcquireTokenSilent call fail and trigger re-authentication.
So why do we end up with 20 minutes as the default, is it mainly the concern of the large system with huge cache entries?
Of course the purge interval plays a role too, and the default is 30 minutes though. Is the 20 min + 30 min < 60 min (default access token life time) plays a role here?

@jmprieur
Copy link
Collaborator

@jennyf19
Copy link
Collaborator

jennyf19 commented Mar 18, 2021

@creativebrother we've updated the documentation around cache eviction. thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
answered question Further information is requested
Projects
None yet
Development

No branches or pull requests

5 participants