Skip to content

Support for refresh_in #305

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

Merged
merged 4 commits into from
Oct 27, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import java.net.MalformedURLException;
import java.util.Collections;
import java.util.Date;
import java.util.Set;
import java.util.concurrent.ExecutionException;

Expand Down Expand Up @@ -218,6 +219,52 @@ public void acquireTokenSilent_ConfidentialClient_acquireTokenSilentDifferentSco
.get();
}

@Test(dataProvider = "environments", dataProviderClass = EnvironmentsProvider.class)
Copy link
Contributor

@SomkaPe SomkaPe Oct 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to have unit test to test this scenario, current test does not cover end to end scenario , also it is hybrid of unit test + integration testing , i would keep them separated

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without a way to get refresh_in in an actual token response, I'm not sure if there's a way to have meaningful tests without a hybrid like this. A unit test of setting the refreshOn field would just be 'set refreshOn manually, then assert that it was set'. The only time it's used is halfway through the acquireTokenSilent flow, and a unit test of that would need a lot of hardcoding that the actual token request process takes care of anyway.

public void acquireTokenSilent_WithRefreshOn(String environment) throws Exception{
cfg = new Config(environment);

User user = labUserProvider.getDefaultUser(cfg.azureEnvironment);

PublicClientApplication pca = PublicClientApplication.builder(
user.getAppId()).
authority(cfg.organizationsAuthority()).
build();

IAuthenticationResult resultOriginal = acquireTokenUsernamePassword(user, pca, cfg.graphDefaultScope());
assertResultNotNull(resultOriginal);

IAuthenticationResult resultSilent = acquireTokenSilently(pca, resultOriginal.account(), cfg.graphDefaultScope(), false);
Assert.assertNotNull(resultSilent);
assertTokensAreEqual(resultOriginal, resultSilent);

//When this test was made, token responses did not contain the refresh_in field needed for an end-to-end test.
//In order to test silent flow behavior as though the service returned refresh_in, we manually change a cached
// token's refreshOn value from 0 (default if refresh_in missing) to a minute before/after the current time
String key = pca.tokenCache.accessTokens.keySet().iterator().next();
AccessTokenCacheEntity token = pca.tokenCache.accessTokens.get(key);
long currTimestampSec = new Date().getTime() / 1000;

Assert.assertEquals(token.refreshOn(), "0");

token.refreshOn(Long.toString(currTimestampSec + 60));
pca.tokenCache.accessTokens.put(key, token);

IAuthenticationResult resultSilentWithRefreshOn = acquireTokenSilently(pca, resultOriginal.account(), cfg.graphDefaultScope(), false);
//Current time is before refreshOn, so token should not have been refreshed
Assert.assertNotNull(resultSilentWithRefreshOn);
Assert.assertEquals(pca.tokenCache.accessTokens.get(key).refreshOn(), Long.toString(currTimestampSec + 60));
assertTokensAreEqual(resultSilent, resultSilentWithRefreshOn);

token = pca.tokenCache.accessTokens.get(key);
token.refreshOn(Long.toString(currTimestampSec - 60));
pca.tokenCache.accessTokens.put(key, token);

resultSilentWithRefreshOn = acquireTokenSilently(pca, resultOriginal.account(), cfg.graphDefaultScope(), false);
//Current time is after refreshOn, so token should be refreshed
Assert.assertNotNull(resultSilentWithRefreshOn);
assertResultRefreshed(resultSilent, resultSilentWithRefreshOn);
}

private IConfidentialClientApplication getConfidentialClientApplications() throws Exception{
String clientId = cfg.appProvider.getOboAppId();
String password = cfg.appProvider.getOboAppPassword();
Expand Down Expand Up @@ -293,4 +340,9 @@ private void assertResultRefreshed(IAuthenticationResult result, IAuthentication
Assert.assertNotEquals(result.accessToken(), resultAfterRefresh.accessToken());
Assert.assertNotEquals(result.idToken(), resultAfterRefresh.idToken());
}

private void assertTokensAreEqual(IAuthenticationResult result, IAuthenticationResult resultAfterRefresh) {
Assert.assertEquals(result.accessToken(), resultAfterRefresh.accessToken());
Assert.assertEquals(result.idToken(), resultAfterRefresh.idToken());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ class AccessTokenCacheEntity extends Credential {
@JsonProperty("extended_expires_on")
private String extExpiresOn;

@JsonProperty("refresh_on")
private String refreshOn;

String getKey() {
List<String> keyParts = new ArrayList<>();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

package com.microsoft.aad.msal4j;

import java.util.Date;

class AcquireTokenSilentSupplier extends AuthenticationResultSupplier {

private SilentRequest silentRequest;
Expand Down Expand Up @@ -35,11 +37,20 @@ AuthenticationResult execute() throws Exception {
silentRequest.parameters().scopes(),
clientApplication.clientId());

if (res == null) {
throw new MsalClientException(AuthenticationErrorMessage.NO_TOKEN_IN_CACHE, AuthenticationErrorCode.CACHE_MISS);
}

if (!StringHelper.isBlank(res.accessToken())) {
clientApplication.getServiceBundle().getServerSideTelemetry().incrementSilentSuccessfulCount();
}

if (silentRequest.parameters().forceRefresh() || StringHelper.isBlank(res.accessToken())) {
//Determine if the current token needs to be refreshed according to the refresh_in value
long currTimeStampSec = new Date().getTime() / 1000;
boolean afterRefreshOn = res.refreshOn() != null && res.refreshOn() > 0 &&
res.refreshOn() < currTimeStampSec && res.expiresOn() >= currTimeStampSec;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

res.expiresOn() >= currTimeStampSec means you will not do refresh if AT is expired, why ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't want to change any behavior if the token was expired, so if the token was after the expiresOn time then refreshOn time is irrelevant and we shouldn't do any of the refreshOn behavior. If the token was after the refreshOn time, then if any service needed to refresh it was down then the current token should be returned as though it nothing happened, which is not the (existing) behavior for an expired token.


if (silentRequest.parameters().forceRefresh() || afterRefreshOn || StringHelper.isBlank(res.accessToken())) {
if (!StringHelper.isBlank(res.refreshToken())) {
RefreshTokenRequest refreshTokenRequest = new RefreshTokenRequest(
RefreshTokenParameters.builder(silentRequest.parameters().scopes(), res.refreshToken()).build(),
Expand All @@ -50,7 +61,16 @@ AuthenticationResult execute() throws Exception {
AcquireTokenByAuthorizationGrantSupplier acquireTokenByAuthorisationGrantSupplier =
new AcquireTokenByAuthorizationGrantSupplier(clientApplication, refreshTokenRequest, requestAuthority);

res = acquireTokenByAuthorisationGrantSupplier.execute();
try {
res = acquireTokenByAuthorisationGrantSupplier.execute();
} catch (MsalServiceException ex) {
//If the token refresh attempt threw a MsalServiceException but the refresh attempt was done
// only because of refreshOn, then simply return the existing cached token
if (afterRefreshOn && !(silentRequest.parameters().forceRefresh() || StringHelper.isBlank(res.accessToken()))) {
return res;
}
else throw ex;
}
} else {
res = null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ final class AuthenticationResult implements IAuthenticationResult {

private final String refreshToken;

private final Long refreshOn;

@Getter(value = AccessLevel.PACKAGE)
private final String familyId;

Expand Down
7 changes: 5 additions & 2 deletions src/main/java/com/microsoft/aad/msal4j/TokenCache.java
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,7 @@ static private AccessTokenCacheEntity createAccessTokenCacheEntity(TokenRequestE
long currTimestampSec = System.currentTimeMillis() / 1000;
at.cachedAt(Long.toString(currTimestampSec));
at.expiresOn(Long.toString(authenticationResult.expiresOn()));
at.refreshOn(authenticationResult.refreshOn() > 0 ? Long.toString(authenticationResult.refreshOn()) : "0");
if (authenticationResult.extExpiresOn() > 0) {
at.extExpiresOn(Long.toString(authenticationResult.extExpiresOn()));
}
Expand Down Expand Up @@ -555,7 +556,8 @@ AuthenticationResult getCachedAuthenticationResult(
if (atCacheEntity.isPresent()) {
builder.
accessToken(atCacheEntity.get().secret).
expiresOn(Long.parseLong(atCacheEntity.get().expiresOn()));
expiresOn(Long.parseLong(atCacheEntity.get().expiresOn())).
refreshOn(Long.parseLong(atCacheEntity.get().refreshOn()));
}
if (idTokenCacheEntity.isPresent()) {
builder.
Expand Down Expand Up @@ -600,7 +602,8 @@ AuthenticationResult getCachedAuthenticationResult(
if (atCacheEntity.isPresent()) {
builder.
accessToken(atCacheEntity.get().secret).
expiresOn(Long.parseLong(atCacheEntity.get().expiresOn()));
expiresOn(Long.parseLong(atCacheEntity.get().expiresOn())).
refreshOn(Long.parseLong(atCacheEntity.get().refreshOn()));
}
} finally {
lock.readLock().unlock();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ else if(type == AuthorityType.ADFS){
environment(requestAuthority.host()).
expiresOn(currTimestampSec + response.getExpiresIn()).
extExpiresOn(response.getExtExpiresIn() > 0 ? currTimestampSec + response.getExtExpiresIn() : 0).
refreshOn(response.getRefreshIn() > 0 ? currTimestampSec + response.getRefreshIn() : 0).
accountCacheEntity(accountCacheEntity).
scopes(response.getScope()).
build();
Expand Down
12 changes: 10 additions & 2 deletions src/main/java/com/microsoft/aad/msal4j/TokenResponse.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,17 @@ class TokenResponse extends OIDCTokenResponse {
private long expiresIn;
private long extExpiresIn;
private String foci;
private long refreshIn;

TokenResponse(final AccessToken accessToken, final RefreshToken refreshToken, final String idToken,
final String scope, String clientInfo, long expiresIn, long extExpiresIn, String foci) {
final String scope, String clientInfo, long expiresIn, long extExpiresIn, String foci,
long refreshIn) {
super(new OIDCTokens(idToken, accessToken, refreshToken));
this.scope = scope;
this.clientInfo = clientInfo;
this.expiresIn = expiresIn;
this.extExpiresIn = extExpiresIn;
this.refreshIn = refreshIn;
this.foci = foci;
}

Expand Down Expand Up @@ -90,11 +93,16 @@ static TokenResponse parseJsonObject(final JSONObject jsonObject)
foci = JSONObjectUtils.getString(jsonObject, "foci");
}

long refreshIn = 0;
if (jsonObject.containsKey("refresh_in")) {
refreshIn = getLongValue(jsonObject, "refresh_in");
}

try {
final AccessToken accessToken = AccessToken.parse(jsonObject);
final RefreshToken refreshToken = RefreshToken.parse(jsonObject);
return new TokenResponse(accessToken, refreshToken, idTokenValue, scopeValue, clientInfo,
expiresIn, ext_expires_in, foci);
expiresIn, ext_expires_in, foci, refreshIn);
} catch (ParseException e) {
throw new MsalClientException("Invalid or missing token, could not parse. If using B2C, information on a potential B2C issue and workaround can be found here: https://aka.ms/msal4j-b2c-known-issues",
AuthenticationErrorCode.INVALID_JSON);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,14 @@ public class TokenResponseTest extends AbstractMsalTests {
+ "gWpKmnue_2Mgl3jBozTSJJ34r-R6lnWWeN6lqZ2Svw7saI5pmPtC8OZbw";

private final long expiresIn = 12345678910L;
private final long extExpiresIn = 12345678910L;
private final long extExpiresIn = 12345678910L;
private final long refreshIn = 0;

@Test
public void testConstructor() throws ParseException {
final TokenResponse response = new TokenResponse(
new BearerAccessToken("access_token"), new RefreshToken(
"refresh_token"), idToken, null, null, expiresIn, extExpiresIn, null);
"refresh_token"), idToken, null, null, expiresIn, extExpiresIn, null, refreshIn);
Assert.assertNotNull(response);
OIDCTokens tokens = response.getOIDCTokens();
Assert.assertNotNull(tokens);
Expand All @@ -63,7 +64,7 @@ public void testEmptyIdToken() {
final TokenResponse response = new TokenResponse(
new BearerAccessToken(idToken),
new RefreshToken("refresh_token"),
"", null, null, expiresIn, extExpiresIn, null);
"", null, null, expiresIn, extExpiresIn, null, refreshIn);

Assert.assertNotNull(response);
OIDCTokens tokens = response.getOIDCTokens();
Expand Down