AzureAD / microsoft-authentication-library-for-java

Microsoft Authentication Library (MSAL) for Java http://aka.ms/aadv2
MIT License
285 stars 143 forks source link

[Bug] Expired tokens being returned from the cache #874

Open drsgoodall opened 6 days ago

drsgoodall commented 6 days ago

Library version used

1.17.1

Java version

21

Scenario

Other - please specify

Is this a new or an existing app?

The app is in production, I haven't upgraded MSAL, but started seeing this issue

Issue description and reproduction steps

We've been seeing issues with token refreshes. When requesting a new token the MSAL library is returning the cached token until 5 minutes after expiry.

I believe the problem lies in AcquireTokenSilentSupplier

//If the access token is expired or within 5 minutes of becoming expired, refresh it if (!StringHelper.isBlank(cachedResult.accessToken()) && cachedResult.expiresOn() < (currTimeStampSec - ACCESS_TOKEN_EXPIRE_BUFFER_IN_SEC)) { setCacheTelemetry(CacheTelemetry.REFRESH_ACCESS_TOKEN_EXPIRED.telemetryValue); log.debug("Refreshing access token because it is expired."); return true; } which should really be currTimeStampSec + ACCESS_TOKEN_EXPIRE_BUFFER_IN_SEC

We are using 1.17.1 but I can see the same line of code in current dev branch.

Relevant code snippets

// Token persistence based off msal sample
class TokenPersistence implements ITokenCacheAccessAspect {
public String data;
  TokenPersistence(final String data) { this.data = data; }

  public void beforeCacheAccess(final ITokenCacheAccessContext iTokenCacheAccessContext) {
    iTokenCacheAccessContext.tokenCache().deserialize(data);
  }

  public void afterCacheAccess(final ITokenCacheAccessContext iTokenCacheAccessContext) {
    this.data = iTokenCacheAccessContext.tokenCache().serialize();
  }
}

var pca = PublicClientApplication.builder(clientId)
.setTokenCacheAccessAspect(persistenceAspect)
.authority("https://login.microsoftonline.com/<tenantId>")
.build();

// Once we have a valid token, issue request around expiry.
SilentParameters parameters = SilentParameters.builder(scope, account).build();
var tokens = pca.acquireTokenSilently(parameters).join();

Expected behavior

Any request to a token issued from after 5 minutes before expiry of the current one should generate a new token.

Identity provider

Microsoft Entra ID (Work and School accounts and Personal Microsoft accounts)

Regression

No response

Solution and workarounds

I can manually check the token expiry and use the following to force a refresh SilentParameters.builder(scope, account).forceRefresh(true).build();

bgavrilMS commented 6 days ago

Agreed, thanks for reporting this.

now: 12:00 token's expires_on: 11:59

Expected: token is considered expired Actual: token is NOT considered expired .. because (exp) < now - 5 min, i.e. 11:59 < 12:00 - 5 min, i.e. 11:59 < 11:55 .. so the condition evaluates to false.

Also, the left side (expiresOn) is a date, and the right side is in seconds. We should double check that comparison is done correctly.

Avery-Dunn commented 5 days ago

Thanks for finding this @drsgoodall ! Looks like this bug would be present in versions 1.17.0 to 1.17.2. I've just created a PR to fix it, and we should have a hotfix release out this week: https://github.com/AzureAD/microsoft-authentication-library-for-java/pull/875