From a7d7178da3e84b1c987c9c14f399049c9294d9e3 Mon Sep 17 00:00:00 2001 From: Prithvi Kanherkar Date: Tue, 30 Jun 2020 11:07:22 -0700 Subject: [PATCH 1/2] Fix an issue where expiration is not calculated correctly for response object --- lib/msal-browser/test/app/PublicClientApplication.spec.ts | 4 ++-- lib/msal-common/src/response/ResponseHandler.ts | 4 ++-- lib/msal-common/test/client/AuthorizationCodeClient.spec.ts | 1 + 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/msal-browser/test/app/PublicClientApplication.spec.ts b/lib/msal-browser/test/app/PublicClientApplication.spec.ts index ff44e4cf24..2dfd880460 100644 --- a/lib/msal-browser/test/app/PublicClientApplication.spec.ts +++ b/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; }); diff --git a/lib/msal-common/src/response/ResponseHandler.ts b/lib/msal-common/src/response/ResponseHandler.ts index 5111fbaf45..096ec8f412 100644 --- a/lib/msal-common/src/response/ResponseHandler.ts +++ b/lib/msal-common/src/response/ResponseHandler.ts @@ -128,8 +128,8 @@ export class ResponseHandler { idTokenClaims: idTokenObj.claims, accessToken: serverTokenResponse.access_token, fromCache: true, - expiresOn: new Date(cacheRecord.accessToken.expiresOn), - extExpiresOn: new Date(cacheRecord.accessToken.extendedExpiresOn), + expiresOn: new Date(Number(cacheRecord.accessToken.expiresOn) * 1000), + extExpiresOn: new Date(Number(cacheRecord.accessToken.extendedExpiresOn) * 1000), familyId: serverTokenResponse.foci || null, state: requestStateObj ? requestStateObj.userRequestState : "" }; diff --git a/lib/msal-common/test/client/AuthorizationCodeClient.spec.ts b/lib/msal-common/test/client/AuthorizationCodeClient.spec.ts index cb9778e80c..8aa611cedf 100644 --- a/lib/msal-common/test/client/AuthorizationCodeClient.spec.ts +++ b/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}`); From ff1398ae8b589cb899ba0defb0ed688e2057f7bd Mon Sep 17 00:00:00 2001 From: Prithvi Kanherkar Date: Tue, 30 Jun 2020 12:00:01 -0700 Subject: [PATCH 2/2] Refactor SilentFlow and ReponseHandler to have a single place where the authentication result is generated --- lib/msal-common/src/cache/CacheManager.ts | 9 +- .../src/cache/interface/ICacheManager.ts | 3 +- .../src/client/AuthorizationCodeClient.ts | 2 +- .../src/client/RefreshTokenClient.ts | 2 +- .../src/client/SilentFlowClient.ts | 24 ++--- .../src/response/ResponseHandler.ts | 90 +++++++++++-------- 6 files changed, 67 insertions(+), 63 deletions(-) diff --git a/lib/msal-common/src/cache/CacheManager.ts b/lib/msal-common/src/cache/CacheManager.ts index a77a5a9fd8..c007998c29 100644 --- a/lib/msal-common/src/cache/CacheManager.ts +++ b/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,7 +134,7 @@ 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, @@ -142,11 +142,12 @@ export abstract class CacheManager implements ICacheManager { 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); } }); diff --git a/lib/msal-common/src/cache/interface/ICacheManager.ts b/lib/msal-common/src/cache/interface/ICacheManager.ts index a1730013cf..2ce9386fcd 100644 --- a/lib/msal-common/src/cache/interface/ICacheManager.ts +++ b/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 diff --git a/lib/msal-common/src/client/AuthorizationCodeClient.ts b/lib/msal-common/src/client/AuthorizationCodeClient.ts index c17001307d..41b984ae5a 100644 --- a/lib/msal-common/src/client/AuthorizationCodeClient.ts +++ b/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; } diff --git a/lib/msal-common/src/client/RefreshTokenClient.ts b/lib/msal-common/src/client/RefreshTokenClient.ts index 2259008e80..9750770548 100644 --- a/lib/msal-common/src/client/RefreshTokenClient.ts +++ b/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 ); diff --git a/lib/msal-common/src/client/SilentFlowClient.ts b/lib/msal-common/src/client/SilentFlowClient.ts index 5fc160fc2a..9ae6a63f20 100644 --- a/lib/msal-common/src/client/SilentFlowClient.ts +++ b/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); } /** diff --git a/lib/msal-common/src/response/ResponseHandler.ts b/lib/msal-common/src/response/ResponseHandler.ts index 096ec8f412..52bb23eca4 100644 --- a/lib/msal-common/src/response/ResponseHandler.ts +++ b/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); + this.cacheStorage.saveCacheRecord(cacheRecord); - 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(Number(cacheRecord.accessToken.expiresOn) * 1000), - extExpiresOn: new Date(Number(cacheRecord.accessToken.extendedExpiresOn) * 1000), - familyId: serverTokenResponse.foci || null, - state: requestStateObj ? requestStateObj.userRequestState : "" - }; - - 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 || "" + }; + } }