Merge pull request #1860 from AzureAD/fix-expiration-issues

[msal-common] Fix expiration calculation for AuthenticationResult object

Author: Prithvi Kanherkar

lib/msal-browser/test/app/PublicClientApplication.spec.ts

@@ -172,7 +172,7 @@ describe("PublicClientApplication.ts Class Unit Tests", () => {
                 expect(tokenResponse.idToken).to.be.eq(testTokenResponse.idToken);
                 expect(tokenResponse.idTokenClaims).to.be.contain(testTokenResponse.idTokenClaims);
                 expect(tokenResponse.accessToken).to.be.eq(testTokenResponse.accessToken);
-                // expect(testTokenResponse.expiresOn.getMilliseconds() >= tokenResponse.expiresOn.getMilliseconds()).to.be.true;
+                expect(testTokenResponse.expiresOn.getMilliseconds() >= tokenResponse.expiresOn.getMilliseconds()).to.be.true;
                 expect(window.sessionStorage.length).to.be.eq(4);
             });
 
@@ -273,7 +273,7 @@ describe("PublicClientApplication.ts Class Unit Tests", () => {
                 expect(tokenResponse.idToken).to.be.eq(testTokenResponse.idToken);
                 expect(tokenResponse.idTokenClaims).to.be.contain(testTokenResponse.idTokenClaims);
                 expect(tokenResponse.accessToken).to.be.eq(testTokenResponse.accessToken);
-                // expect(testTokenResponse.expiresOn.getMilliseconds() >= tokenResponse.expiresOn.getMilliseconds()).to.be.true;
+                expect(testTokenResponse.expiresOn.getMilliseconds() >= tokenResponse.expiresOn.getMilliseconds()).to.be.true;
                 expect(window.sessionStorage.length).to.be.eq(4);
                 expect(window.location.hash).to.be.empty;
             });

lib/msal-common/src/cache/CacheManager.ts

@@ -82,7 +82,7 @@ export abstract class CacheManager implements ICacheManager {
      * saves a cache record
      * @param cacheRecord
      */
-    saveCacheRecord(cacheRecord: CacheRecord, responseScopes?: ScopeSet): void {
+    saveCacheRecord(cacheRecord: CacheRecord): void {
         if (!cacheRecord) {
             throw ClientAuthError.createNullOrUndefinedCacheRecord();
         }
@@ -96,7 +96,7 @@ export abstract class CacheManager implements ICacheManager {
         }
 
         if (!!cacheRecord.accessToken) {
-            this.saveAccessToken(cacheRecord.accessToken, responseScopes);
+            this.saveAccessToken(cacheRecord.accessToken);
         }
 
         if (!!cacheRecord.refreshToken) {
@@ -134,19 +134,20 @@ export abstract class CacheManager implements ICacheManager {
      * saves access token credential
      * @param credential
      */
-    private saveAccessToken(credential: AccessTokenEntity, responseScopes: ScopeSet): void {
+    private saveAccessToken(credential: AccessTokenEntity): void {
         const currentTokenCache = this.getCredentialsFilteredBy({
             clientId: credential.clientId,
             credentialType: CredentialType.ACCESS_TOKEN,
             environment: credential.environment,
             homeAccountId: credential.homeAccountId,
             realm: credential.realm
         });
+        const currentScopes = ScopeSet.fromString(credential.target);
         const currentAccessTokens: AccessTokenEntity[] = Object.values(currentTokenCache.accessTokens) as AccessTokenEntity[];
         if (currentAccessTokens) {
             currentAccessTokens.forEach((tokenEntity) => {
                 const tokenScopeSet = ScopeSet.fromString(tokenEntity.target);
-                if (tokenScopeSet.intersectingScopeSets(responseScopes)) {
+                if (tokenScopeSet.intersectingScopeSets(currentScopes)) {
                     this.removeCredential(tokenEntity);
                 }
             });

lib/msal-common/src/cache/interface/ICacheManager.ts

@@ -11,7 +11,6 @@ import {
     CredentialFilter
 } from "../utils/CacheTypes";
 import { CacheRecord } from "../entities/CacheRecord";
-import { ScopeSet } from "../../request/ScopeSet";
 import { AccountEntity } from "../entities/AccountEntity";
 import { AccountInfo } from "../../account/AccountInfo";
 
@@ -26,7 +25,7 @@ export interface ICacheManager {
      * saves a cache record
      * @param cacheRecord
      */
-    saveCacheRecord(cacheRecord: CacheRecord, responseScopes: ScopeSet): void;
+    saveCacheRecord(cacheRecord: CacheRecord): void;
 
     /**
      * Given account key retrieve an account

lib/msal-common/src/client/AuthorizationCodeClient.ts

@@ -70,7 +70,7 @@ export class AuthorizationCodeClient extends BaseClient {
 
         // Validate response. This function throws a server error if an error is returned by the server.
         responseHandler.validateTokenResponse(response.body);
-        const tokenResponse = responseHandler.generateAuthenticationResult(response.body, this.authority, cachedNonce, cachedState);
+        const tokenResponse = responseHandler.handleServerTokenResponse(response.body, this.authority, cachedNonce, cachedState);
 
         return tokenResponse;
     }

lib/msal-common/src/client/RefreshTokenClient.ts

@@ -34,7 +34,7 @@ export class RefreshTokenClient extends BaseClient {
         );
 
         responseHandler.validateTokenResponse(response.body);
-        const tokenResponse = responseHandler.generateAuthenticationResult(
+        const tokenResponse = responseHandler.handleServerTokenResponse(
             response.body,
             this.authority
         );

lib/msal-common/src/client/SilentFlowClient.ts

@@ -21,6 +21,7 @@ import { CredentialFilter, CredentialCache } from "../cache/utils/CacheTypes";
 import { AccountEntity } from "../cache/entities/AccountEntity";
 import { CredentialEntity } from "../cache/entities/CredentialEntity";
 import { ClientConfigurationError } from "../error/ClientConfigurationError";
+import { ResponseHandler } from "../response/ResponseHandler";
 
 export class SilentFlowClient extends BaseClient {
 
@@ -77,22 +78,13 @@ export class SilentFlowClient extends BaseClient {
 
         const cachedIdToken = this.readIdTokenFromCache(homeAccountId, environment, cachedAccount.realm);
         const idTokenObj = new IdToken(cachedIdToken.secret, this.config.cryptoInterface);
-        const cachedScopes = ScopeSet.fromString(cachedAccessToken.target);
-
-        // generate Authentication Result
-        return {
-            uniqueId: idTokenObj.claims.oid || idTokenObj.claims.sub,
-            tenantId: idTokenObj.claims.tid,
-            scopes: cachedScopes.asArray(),
-            account: cachedAccount.getAccountInfo(),
-            idToken: cachedIdToken.secret,
-            idTokenClaims: idTokenObj.claims,
-            accessToken: cachedAccessToken.secret,
-            fromCache: true,
-            expiresOn: new Date(cachedAccessToken.expiresOn),
-            extExpiresOn: new Date(cachedAccessToken.extendedExpiresOn),
-            familyId: null,
-        };
+
+        return ResponseHandler.generateAuthenticationResult({
+            account: cachedAccount,
+            accessToken: cachedAccessToken,
+            idToken: cachedIdToken,
+            refreshToken: cachedRefreshToken
+        }, idTokenObj, true);
     }
 
     /**

lib/msal-common/src/response/ResponseHandler.ts

@@ -98,7 +98,7 @@ export class ResponseHandler {
      * @param serverTokenResponse
      * @param authority
      */
-    generateAuthenticationResult(serverTokenResponse: ServerAuthorizationTokenResponse, authority: Authority, cachedNonce?: string, cachedState?: string): AuthenticationResult {
+    handleServerTokenResponse(serverTokenResponse: ServerAuthorizationTokenResponse, authority: Authority, cachedNonce?: string, cachedState?: string): AuthenticationResult {
         // create an idToken object (not entity)
         const idTokenObj = new IdToken(serverTokenResponse.id_token, this.cryptoObj);
 
@@ -116,43 +116,9 @@ export class ResponseHandler {
         }
 
         const cacheRecord = this.generateCacheRecord(serverTokenResponse, idTokenObj, authority, requestStateObj && requestStateObj.libraryState);
-        const responseScopes = ScopeSet.fromString(serverTokenResponse.scope);
-        this.cacheStorage.saveCacheRecord(cacheRecord, responseScopes);
-
-        const authenticationResult: AuthenticationResult = {
-            uniqueId: idTokenObj.claims.oid || idTokenObj.claims.sub,
-            tenantId: idTokenObj.claims.tid,
-            scopes: responseScopes.asArray(),
-            account: cacheRecord.account.getAccountInfo(),
-            idToken: idTokenObj.rawIdToken,
-            idTokenClaims: idTokenObj.claims,
-            accessToken: serverTokenResponse.access_token,
-            fromCache: true,
-            expiresOn: new Date(cacheRecord.accessToken.expiresOn),
-            extExpiresOn: new Date(cacheRecord.accessToken.extendedExpiresOn),
-            familyId: serverTokenResponse.foci || null,
-            state: requestStateObj ? requestStateObj.userRequestState : ""
-        };
+        this.cacheStorage.saveCacheRecord(cacheRecord);
 
-        return authenticationResult;
-    }
-
-    /**
-     * Generate Account
-     * @param serverTokenResponse
-     * @param idToken
-     * @param authority
-     */
-    generateAccountEntity(serverTokenResponse: ServerAuthorizationTokenResponse, idToken: IdToken, authority: Authority): AccountEntity {
-        const authorityType = authority.authorityType;
-
-        if (StringUtils.isEmpty(serverTokenResponse.client_info)) {
-            throw ClientAuthError.createClientInfoEmptyError(serverTokenResponse.client_info);
-        }
-
-        return (authorityType === AuthorityType.Adfs)? 
-            AccountEntity.createADFSAccount(authority, idToken): 
-            AccountEntity.createAccount(serverTokenResponse.client_info, authority, idToken, this.cryptoObj);
+        return ResponseHandler.generateAuthenticationResult(cacheRecord, idTokenObj, false, requestStateObj ? requestStateObj.userRequestState : null);
     }
 
     /**
@@ -161,7 +127,7 @@ export class ResponseHandler {
      * @param idTokenObj
      * @param authority
      */
-    generateCacheRecord(serverTokenResponse: ServerAuthorizationTokenResponse, idTokenObj: IdToken, authority: Authority, libraryState?: LibraryStateObject): CacheRecord {
+    private generateCacheRecord(serverTokenResponse: ServerAuthorizationTokenResponse, idTokenObj: IdToken, authority: Authority, libraryState?: LibraryStateObject): CacheRecord {
         // Account
         const cachedAccount  = this.generateAccountEntity(
             serverTokenResponse,
@@ -202,7 +168,7 @@ export class ResponseHandler {
             serverTokenResponse.access_token,
             this.clientId,
             idTokenObj.claims.tid,
-            responseScopes.asArray().join(" "),
+            responseScopes.printScopes(),
             tokenExpirationSeconds,
             extendedTokenExpirationSeconds
         );
@@ -218,4 +184,50 @@ export class ResponseHandler {
 
         return new CacheRecord(cachedAccount, cachedIdToken, cachedAccessToken, cachedRefreshToken);
     }
+
+    /**
+     * Generate Account
+     * @param serverTokenResponse
+     * @param idToken
+     * @param authority
+     */
+    private generateAccountEntity(serverTokenResponse: ServerAuthorizationTokenResponse, idToken: IdToken, authority: Authority): AccountEntity {
+        const authorityType = authority.authorityType;
+
+        if (StringUtils.isEmpty(serverTokenResponse.client_info)) {
+            throw ClientAuthError.createClientInfoEmptyError(serverTokenResponse.client_info);
+        }
+
+        return (authorityType === AuthorityType.Adfs)? 
+            AccountEntity.createADFSAccount(authority, idToken): 
+            AccountEntity.createAccount(serverTokenResponse.client_info, authority, idToken, this.cryptoObj);
+    }
+
+    /**
+     * Creates an @AuthenticationResult from @CacheRecord , @IdToken , and a boolean that states whether or not the result is from cache.
+     * 
+     * Optionally takes a state string that is set as-is in the response.
+     * 
+     * @param cacheRecord 
+     * @param idTokenObj 
+     * @param fromTokenCache 
+     * @param stateString 
+     */
+    static generateAuthenticationResult(cacheRecord: CacheRecord, idTokenObj: IdToken, fromTokenCache: boolean, stateString?: string): AuthenticationResult {
+        const responseScopes = ScopeSet.fromString(cacheRecord.accessToken.target);
+        return {
+            uniqueId: idTokenObj.claims.oid || idTokenObj.claims.sub,
+            tenantId: idTokenObj.claims.tid,
+            scopes: responseScopes.asArray(),
+            account: cacheRecord.account.getAccountInfo(),
+            idToken: idTokenObj.rawIdToken,
+            idTokenClaims: idTokenObj.claims,
+            accessToken: cacheRecord.accessToken.secret,
+            fromCache: fromTokenCache,
+            expiresOn: new Date(Number(cacheRecord.accessToken.expiresOn) * 1000),
+            extExpiresOn: new Date(Number(cacheRecord.accessToken.extendedExpiresOn) * 1000),
+            familyId: cacheRecord.refreshToken.familyId || null,
+            state: stateString || ""
+        };
+    }
 }

lib/msal-common/test/client/AuthorizationCodeClient.spec.ts

@@ -339,6 +339,7 @@ describe("AuthorizationCodeClient unit tests", () => {
             const authenticationResult = await client.acquireToken(authCodeRequest, idTokenClaims.nonce, testState);
 
             expect(authenticationResult.accessToken).to.deep.eq(AUTHENTICATION_RESULT.body.access_token);
+            expect((Date.now() + (AUTHENTICATION_RESULT.body.expires_in * 1000)) >= authenticationResult.expiresOn.getMilliseconds()).to.be.true;
             expect(createTokenRequestBodySpy.calledWith(authCodeRequest)).to.be.ok;
 
             expect(createTokenRequestBodySpy.returnValues[0]).to.contain(`${AADServerParamKeys.SCOPE}=${TEST_CONFIG.DEFAULT_GRAPH_SCOPE}%20${Constants.OPENID_SCOPE}%20${Constants.PROFILE_SCOPE}%20${Constants.OFFLINE_ACCESS_SCOPE}`);