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] Expose refreshOn in IAuthenticationResult #822

Closed
g2vinay opened this issue May 28, 2024 · 9 comments
Closed

[Feature Request] Expose refreshOn in IAuthenticationResult #822

g2vinay opened this issue May 28, 2024 · 9 comments
Labels
AzureSDK Issues and requests affecting the Azure SDK Enhancement A request or suggestion to improve some aspect of the library

Comments

@g2vinay
Copy link

g2vinay commented May 28, 2024

MSAL client type

Public, Confidential, Managed identity

Problem Statement

The refreshOn property of the token isn't returned as part of IAuthenticationResult.
It prevents users from accessing and honoring this property.

This feature is required for :

  1. Cross-language consistency with .NET and other Azure languages which support this capability already.
  2. Enterprise Azure customers need this capability exposed so that token cache refreshes can happen more aggressively and reduce the chance of performance issues with near-expired or expired tokens

Today, Azure SDK has its Token Cache implementation which defaults to a refresh offset of 5 minutes.
This cache caters to all implementations of TokenCredential which may or may not be using Msal.
So, today the refresh offset of 5 minutes overrides the Msal's refresh_on , as the refresh_on is not available at Azure SDK cache layer.
We need the refresh_on information to correctly invoke the TokenCredential at its desired refresh_on time.

Further, the default refresh offset of 5 minutes in Azure SDK token cache is creating reliability issues for our enterprise customers and the ask is for Azure SDK to use the refresh on info in their cache implementation to invoke the token credential at the given refresh on time.

Proposed solution

Expose the refreshOn property of the token similar to the expiry time.

Alternatives

No response

@g2vinay g2vinay added needs attention Automatically used when an issue is created through an issue template untriaged Automatically used when an issue is created through an issue template labels May 28, 2024
@bgavrilMS
Copy link
Member

Hi @g2vinay - not understanding how "refreshOn" is useful on AuthenticationResult object? We can of course add it in case ppl want to log this value for their own telemetry purposes. But does it have any functional value?

RefreshOn is for MSAL to perform background token refresh.

@Avery-Dunn Avery-Dunn added Enhancement A request or suggestion to improve some aspect of the library Requires more info More information is needed, from either the person who opened the issue or another team and removed needs attention Automatically used when an issue is created through an issue template untriaged Automatically used when an issue is created through an issue template labels May 30, 2024
@g2vinay
Copy link
Author

g2vinay commented Jun 7, 2024

Hi @g2vinay - not understanding how "refreshOn" is useful on AuthenticationResult object? We can of course add it in case ppl want to log this value for their own telemetry purposes. But does it have any functional value?

RefreshOn is for MSAL to perform background token refresh.

Yeah we have a cache layer in azure-core, that defaults to refresh offset of 5 minutes.
Having this info available will allow the cache in azure-core to honor it.

@g2vinay
Copy link
Author

g2vinay commented Jun 7, 2024

Based on the discussion with @Avery-Dunn
This feature will align with already exposed property in Msal.NET.
This feature is expected to be included in next release of msal4j.

To further highlight,
Enterprise Azure customers need this capability exposed so that token cache refreshes can happen more aggressively and reduce the chance of performance issues with near-expired or expired tokens

@bgavrilMS
Copy link
Member

The value in MSAL.NET is read-only and it is only exposed for telemetry and monitoring purposes. MSALs will handle all "proactive" refreshes internally. This is not the responsibility of Azure SDK.

The way it works is:

  • Azure SDK calls one of MSAL's AcquireToken* methods which looks in the cache (AcquireTokenForClient, AcquireTokenSilent)
  • MSAL internally looks in the cache and returns any non-expired access token
  • if the access token needs to be refresh - i.e. expiry between (refresh_on, expires_on) - it will queue a background request

Some details might be a bit different in MSAL4j, e.g. the refresh might not be on background thread but on the same thread as the request.

Can you please detail your scenario better? I do not see why this is important and no customer / app developer requested this before.

@g2vinay
Copy link
Author

g2vinay commented Jun 11, 2024

The value in MSAL.NET is read-only and it is only exposed for telemetry and monitoring purposes. MSALs will handle all "proactive" refreshes internally. This is not the responsibility of Azure SDK.

The way it works is:

  • Azure SDK calls one of MSAL's AcquireToken* methods which looks in the cache (AcquireTokenForClient, AcquireTokenSilent)
  • MSAL internally looks in the cache and returns any non-expired access token
  • if the access token needs to be refresh - i.e. expiry between (refresh_on, expires_on) - it will queue a background request

Some details might be a bit different in MSAL4j, e.g. the refresh might not be on background thread but on the same thread as the request.

Can you please detail your scenario better? I do not see why this is important and no customer / app developer requested this before.

Today, Azure SDK has its Token Cache implementation which defaults to a refresh offset of 5 minutes.
This cache caters to all implementations of TokenCredential which may or may not be using Msal.
So, today the refresh offset of 5 minutes overrides the Msal's refresh_on , as the refresh_on is not available at Azure SDK cache layer.
We need the refresh_on information to correctly invoke the TokenCredential at its desired refresh_on time.

Further, the default refresh offset of 5 minutes in Azure SDK token cache is creating reliability issues for our enterprise customers and the ask is for Azure SDK to use the refresh on info in their cache implementation to invoke the token credential at the given refresh on time.

@rayluo
Copy link

rayluo commented Jun 11, 2024

Today, Azure SDK has its Token Cache implementation which defaults to a refresh offset of 5 minutes. This cache caters to all implementations of TokenCredential which may or may not be using Msal. So, today the refresh offset of 5 minutes overrides the Msal's refresh_on , as the refresh_on is not available at Azure SDK cache layer. We need the refresh_on information to correctly invoke the TokenCredential at its desired refresh_on time.

I always wondered why Azure SDK would need to build its own token cache layer when MSAL already has one. Now that you mentioned "[some TokenCredential] may not be using Msal", so, that makes sense. Out of curiosity, what are those non-MSAL TokenCredentials? Managed Identity (MI) is probably one of them, but Azure SDK will soon move to use MSAL's MI (thus MSAL's token cache for it).

Further, the default refresh offset of 5 minutes in Azure SDK token cache is creating reliability issues for our enterprise customers and the ask is for Azure SDK to use the refresh on info in their cache implementation to invoke the token credential at the given refresh on time.

Just want to make sure we are using same terminology here. The refreshOn in MSAL is not a hard expireOn expiration time. MSAL's AcquireTokenSilent() will attempt a refresh after the refreshOn time, but if it fails, MSAL still reuses the current aging-but-still-good-enough token (rather than error out), and then MSAL will attempt refresh next time.

If Azure SDK chooses to use refreshOn, it better also implements the non-trivial logic above, if it hasn't already; or it simply outsources those behaviors to MSAL.

MSAL does not expect downstream callers to handle refreshOn (or anything about refresh token), so, that refreshOn did not mean to be exposed.

@Avery-Dunn Avery-Dunn added AzureSDK Issues and requests affecting the Azure SDK and removed Requires more info More information is needed, from either the person who opened the issue or another team labels Jun 13, 2024
@g2vinay
Copy link
Author

g2vinay commented Jun 24, 2024

@Avery-Dunn
Can you provide an update here for the planned release ?
This is required for our upcoming July release.

@Avery-Dunn
Copy link
Collaborator

@g2vinay : ETA for this is by end of this week at the latest, more likely in the next day or two. Just need to get #829 and some other PRs reviewed and merged in

@Avery-Dunn
Copy link
Collaborator

Released as part of 1.16.0, #829

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AzureSDK Issues and requests affecting the Azure SDK Enhancement A request or suggestion to improve some aspect of the library
Projects
None yet
Development

No branches or pull requests

4 participants