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] Add an enum that indicates the source of a token - cache, IdP or broker. #1728

Closed
bgavrilMS opened this issue Mar 25, 2020 · 7 comments

Comments

@bgavrilMS
Copy link
Member

bgavrilMS commented Mar 25, 2020

Is your feature request related to a problem? Please describe.
Services need to monitor and alarm around cache hits, including the token cache.

Describe the solution you'd like
AuthenticationResult.TokenSource

Describe alternatives you've considered
Logs and ExpiresOn property can also be used but logs require lots of post-processing and ExpiresOn cannot be used reliably.

@jmprieur
Copy link
Contributor

@bgavrilMS : would we want it to be a public property?

@bgavrilMS
Copy link
Member Author

Yes, this would benefit our customers in Web Api / Web Api and Daemon scenarios at scale. Some of hte issues they are facing are described below. This improvement hope to address problem 4.

Problem 1: if a customer brings online a large number of machines at once (e.g. during a big scale up), then each of these machines will go to AAD to fetch a token, even though the token is identical
Solution: persist the token cache to Redis or some other distributed cache

Problem 2: It takes a few seconds to acquire a token from AAD (mostly network I/O). If during these few seconds other machines require the token, they would go to AAD to get it. Especially problematic if AAD is under high load.
Problem 3: If AAD is down when the token expires, your service will become unavailable.
Solution: pro-active token renewal as described here

Problem 4: You do not have a mechanism to monitor and alarm for when MSAL fails to hit the token cache.
Solution: flag on AuthResult

@henrik-me
Copy link
Contributor

Docs and samples should be updated to showcase usage of this. Similar to logging to be on by default.

@bgavrilMS
Copy link
Member Author

I would like to get this in sooner rather than later. I see a pattern among our customers to create new CCA objects for each call, and to not use any cache serialisation. AcquireTokenForClient is the main problem here

If we had this flag, we could guide customers to writing integration tests / monitoring to check that they don't hit AAD on every request.

@jmprieur
Copy link
Contributor

jmprieur commented May 6, 2020

LGTM., @bgavrilMS.

@bgavrilMS bgavrilMS removed this from the 4.15 milestone May 14, 2020
@bgavrilMS bgavrilMS added this to the 4.16.0 milestone Jun 4, 2020
@bgavrilMS bgavrilMS removed this from the 4.16.0 milestone Jun 25, 2020
@bgavrilMS bgavrilMS changed the title [Feature Request] Add a flag that indicates if the token cache was hit [Feature Request] Add an enum that indicates if the token cache was hit Jul 7, 2020
@bgavrilMS bgavrilMS changed the title [Feature Request] Add an enum that indicates if the token cache was hit [Feature Request] Add an enum that indicates the source of a token - cache, IdP or broker. Jul 7, 2020
@bgavrilMS
Copy link
Member Author

@neha-bhargava
Copy link
Contributor

Fixed in MSAL 4.17.0 release

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