From 1b49c36a3edcd4c15445ffe2954795bac58688c8 Mon Sep 17 00:00:00 2001 From: Thomas Norling Date: Thu, 9 Jul 2020 10:20:51 -0700 Subject: [PATCH 01/12] Implement Telemetry in browser --- .../src/app/PublicClientApplication.ts | 123 +++++++++++------- lib/msal-browser/src/cache/BrowserStorage.ts | 2 + .../interaction_handler/InteractionHandler.ts | 6 +- .../interaction_handler/RedirectHandler.ts | 7 +- .../src/utils/BrowserConstants.ts | 10 ++ 5 files changed, 93 insertions(+), 55 deletions(-) diff --git a/lib/msal-browser/src/app/PublicClientApplication.ts b/lib/msal-browser/src/app/PublicClientApplication.ts index 3cb33bc392..49d41dff59 100644 --- a/lib/msal-browser/src/app/PublicClientApplication.ts +++ b/lib/msal-browser/src/app/PublicClientApplication.ts @@ -28,7 +28,8 @@ import { SilentFlowClient, EndSessionRequest, BaseAuthRequest, - Logger + Logger, + TelemetryManager } from "@azure/msal-common"; import { buildConfiguration, Configuration } from "../config/Configuration"; import { BrowserStorage } from "../cache/BrowserStorage"; @@ -37,7 +38,7 @@ import { RedirectHandler } from "../interaction_handler/RedirectHandler"; import { PopupHandler } from "../interaction_handler/PopupHandler"; import { SilentHandler } from "../interaction_handler/SilentHandler"; import { BrowserAuthError } from "../error/BrowserAuthError"; -import { BrowserConstants, TemporaryCacheKeys } from "../utils/BrowserConstants"; +import { BrowserConstants, TemporaryCacheKeys, ApiId } from "../utils/BrowserConstants"; import { BrowserUtils } from "../utils/BrowserUtils"; import { version } from "../../package.json"; import { IPublicClientApplication } from "./IPublicClientApplication"; @@ -179,12 +180,20 @@ export class PublicClientApplication implements IPublicClientApplication { this.browserStorage.cleanRequest(); return null; } + const correlationId = this.browserStorage.getItem(this.browserStorage.generateCacheKey(TemporaryCacheKeys.CORRELATION_ID), CacheSchemaType.TEMPORARY) as string; + const telemetryManager = new TelemetryManager(this.browserStorage, ApiId.handleRedirectPromise, correlationId); - // Hash contains known properties - handle and return in callback - const currentAuthority = this.browserStorage.getCachedAuthority(); - const authClient = await this.createAuthCodeClient(currentAuthority); - const interactionHandler = new RedirectHandler(authClient, this.browserStorage); - return interactionHandler.handleCodeResponse(responseHash, this.browserCrypto); + try { + // Hash contains known properties - handle and return in callback + const currentAuthority = this.browserStorage.getCachedAuthority(); + const authClient = await this.createAuthCodeClient(currentAuthority); + const interactionHandler = new RedirectHandler(authClient, this.browserStorage); + return interactionHandler.handleCodeResponse(responseHash, telemetryManager, this.browserCrypto); + } catch (e) { + telemetryManager.cacheFailedRequest(e); + this.browserStorage.cleanRequest(); + throw e; + } } /** @@ -211,10 +220,11 @@ export class PublicClientApplication implements IPublicClientApplication { * To acquire only idToken, please pass clientId as the only scope in the Authentication Parameters */ async acquireTokenRedirect(request: RedirectRequest): Promise { + // Preflight request + const validRequest: AuthorizationUrlRequest = this.preflightInteractiveRequest(request); + const telemetryManager = new TelemetryManager(this.browserStorage, ApiId.acquireTokenRedirect, validRequest.correlationId); + try { - // Preflight request - const validRequest: AuthorizationUrlRequest = this.preflightInteractiveRequest(request); - // Create auth code request and generate PKCE params const authCodeRequest: AuthorizationCodeRequest = await this.initializeAuthorizationCodeRequest(validRequest); @@ -230,6 +240,7 @@ export class PublicClientApplication implements IPublicClientApplication { // Show the UI once the url has been created. Response will come back in the hash, which will be handled in the handleRedirectCallback function. interactionHandler.initiateAuthRequest(navigateUrl, authCodeRequest, request.redirectStartPage, this.browserCrypto); } catch (e) { + telemetryManager.cacheFailedRequest(e); this.browserStorage.cleanRequest(); throw e; } @@ -258,10 +269,11 @@ export class PublicClientApplication implements IPublicClientApplication { * @returns {Promise.} - a promise that is fulfilled when this function has completed, or rejected if an error was raised. Returns the {@link AuthResponse} object */ async acquireTokenPopup(request: PopupRequest): Promise { + // Preflight request + const validRequest: AuthorizationUrlRequest = this.preflightInteractiveRequest(request); + const telemetryManager = new TelemetryManager(this.browserStorage, ApiId.acquireTokenPopup, validRequest.correlationId); + try { - // Preflight request - const validRequest: AuthorizationUrlRequest = this.preflightInteractiveRequest(request); - // Create auth code request and generate PKCE params const authCodeRequest: AuthorizationCodeRequest = await this.initializeAuthorizationCodeRequest(validRequest); @@ -272,8 +284,9 @@ export class PublicClientApplication implements IPublicClientApplication { const navigateUrl = await authClient.getAuthCodeUrl(validRequest); // Acquire token with popup - return await this.popupTokenHelper(navigateUrl, authCodeRequest, authClient); + return await this.popupTokenHelper(navigateUrl, authCodeRequest, authClient, telemetryManager); } catch (e) { + telemetryManager.cacheFailedRequest(e); this.browserStorage.cleanRequest(); throw e; } @@ -283,7 +296,7 @@ export class PublicClientApplication implements IPublicClientApplication { * Helper which acquires an authorization code with a popup from given url, and exchanges the code for a set of OAuth tokens. * @param navigateUrl */ - private async popupTokenHelper(navigateUrl: string, authCodeRequest: AuthorizationCodeRequest, authClient: AuthorizationCodeClient): Promise { + private async popupTokenHelper(navigateUrl: string, authCodeRequest: AuthorizationCodeRequest, authClient: AuthorizationCodeClient, telemetryManager: TelemetryManager): Promise { // Create popup interaction handler. const interactionHandler = new PopupHandler(authClient, this.browserStorage); // Show the UI once the url has been created. Get the window handle for the popup. @@ -291,7 +304,7 @@ export class PublicClientApplication implements IPublicClientApplication { // Monitor the window for the hash. Return the string value and close the popup when the hash is received. Default timeout is 60 seconds. const hash = await interactionHandler.monitorPopupForHash(popupWindow, this.config.system.windowHashTimeout); // Handle response from hash string. - return await interactionHandler.handleCodeResponse(hash); + return await interactionHandler.handleCodeResponse(hash, telemetryManager); } // #endregion @@ -334,19 +347,27 @@ export class PublicClientApplication implements IPublicClientApplication { prompt: PromptValue.NONE }); - // Create auth code request and generate PKCE params - const authCodeRequest: AuthorizationCodeRequest = await this.initializeAuthorizationCodeRequest(silentRequest); + const telemetryManager = new TelemetryManager(this.browserStorage, ApiId.ssoSilent, silentRequest.correlationId); + + try { + // Create auth code request and generate PKCE params + const authCodeRequest: AuthorizationCodeRequest = await this.initializeAuthorizationCodeRequest(silentRequest); - // Get scopeString for iframe ID - const scopeString = silentRequest.scopes ? silentRequest.scopes.join(" ") : ""; + // Get scopeString for iframe ID + const scopeString = silentRequest.scopes ? silentRequest.scopes.join(" ") : ""; - // Initialize the client - const authClient: AuthorizationCodeClient = await this.createAuthCodeClient(silentRequest.authority); + // Initialize the client + const authClient: AuthorizationCodeClient = await this.createAuthCodeClient(silentRequest.authority); - // Create authorize request url - const navigateUrl = await authClient.getAuthCodeUrl(silentRequest); + // Create authorize request url + const navigateUrl = await authClient.getAuthCodeUrl(silentRequest); - return this.silentTokenHelper(navigateUrl, authCodeRequest, authClient, scopeString); + return this.silentTokenHelper(navigateUrl, authCodeRequest, authClient, scopeString, telemetryManager); + } catch (e) { + telemetryManager.cacheFailedRequest(e); + this.browserStorage.cleanRequest(); + throw e; + } } /** @@ -368,11 +389,13 @@ export class PublicClientApplication implements IPublicClientApplication { ...request, ...this.initializeBaseRequest(request) }; + let telemetryManager = new TelemetryManager(this.browserStorage, ApiId.acquireTokenSilent_silentFlow, silentRequest.correlationId, silentRequest.forceRefresh); try { const silentAuthClient = await this.createSilentFlowClient(silentRequest.authority); // Send request to renew token. Auth module will throw errors if token cannot be renewed. - return await silentAuthClient.acquireToken(silentRequest); + return await silentAuthClient.acquireToken(silentRequest, telemetryManager); } catch (e) { + telemetryManager.cacheFailedRequest(e); const isServerError = e instanceof ServerError; const isInteractionRequiredError = e instanceof InteractionRequiredAuthError; const isInvalidGrantError = (e.errorCode === BrowserConstants.INVALID_GRANT_ERROR); @@ -381,20 +404,27 @@ export class PublicClientApplication implements IPublicClientApplication { ...silentRequest, prompt: PromptValue.NONE }); + telemetryManager = new TelemetryManager(this.browserStorage, ApiId.acquireTokenSilent_authCode, silentAuthUrlRequest.correlationId); - // Create auth code request and generate PKCE params - const authCodeRequest: AuthorizationCodeRequest = await this.initializeAuthorizationCodeRequest(silentAuthUrlRequest); + try { + // Create auth code request and generate PKCE params + const authCodeRequest: AuthorizationCodeRequest = await this.initializeAuthorizationCodeRequest(silentAuthUrlRequest); - // Initialize the client - const authClient: AuthorizationCodeClient = await this.createAuthCodeClient(silentAuthUrlRequest.authority); + // Initialize the client + const authClient: AuthorizationCodeClient = await this.createAuthCodeClient(silentAuthUrlRequest.authority); - // Create authorize request url - const navigateUrl = await authClient.getAuthCodeUrl(silentAuthUrlRequest); + // Create authorize request url + const navigateUrl = await authClient.getAuthCodeUrl(silentAuthUrlRequest); - // Get scopeString for iframe ID - const scopeString = silentAuthUrlRequest.scopes ? silentAuthUrlRequest.scopes.join(" ") : ""; + // Get scopeString for iframe ID + const scopeString = silentAuthUrlRequest.scopes ? silentAuthUrlRequest.scopes.join(" ") : ""; - return this.silentTokenHelper(navigateUrl, authCodeRequest, authClient, scopeString); + return this.silentTokenHelper(navigateUrl, authCodeRequest, authClient, scopeString, telemetryManager); + } catch (e) { + telemetryManager.cacheFailedRequest(e); + this.browserStorage.cleanRequest(); + throw e; + } } throw e; @@ -407,20 +437,15 @@ export class PublicClientApplication implements IPublicClientApplication { * @param navigateUrl * @param userRequestScopes */ - private async silentTokenHelper(navigateUrl: string, authCodeRequest: AuthorizationCodeRequest, authClient: AuthorizationCodeClient, userRequestScopes: string): Promise { - try { - // Create silent handler - const silentHandler = new SilentHandler(authClient, this.browserStorage, this.config.system.loadFrameTimeout); - // Get the frame handle for the silent request - const msalFrame = await silentHandler.initiateAuthRequest(navigateUrl, authCodeRequest, userRequestScopes); - // Monitor the window for the hash. Return the string value and close the popup when the hash is received. Default timeout is 60 seconds. - const hash = await silentHandler.monitorIframeForHash(msalFrame, this.config.system.iframeHashTimeout); - // Handle response from hash string. - return await silentHandler.handleCodeResponse(hash); - } catch (e) { - this.browserStorage.cleanRequest(); - throw e; - } + private async silentTokenHelper(navigateUrl: string, authCodeRequest: AuthorizationCodeRequest, authClient: AuthorizationCodeClient, userRequestScopes: string, telemetryManager: TelemetryManager): Promise { + // Create silent handler + const silentHandler = new SilentHandler(authClient, this.browserStorage, this.config.system.loadFrameTimeout); + // Get the frame handle for the silent request + const msalFrame = await silentHandler.initiateAuthRequest(navigateUrl, authCodeRequest, userRequestScopes); + // Monitor the window for the hash. Return the string value and close the popup when the hash is received. Default timeout is 60 seconds. + const hash = await silentHandler.monitorIframeForHash(msalFrame, this.config.system.iframeHashTimeout); + // Handle response from hash string. + return await silentHandler.handleCodeResponse(hash, telemetryManager); } // #endregion diff --git a/lib/msal-browser/src/cache/BrowserStorage.ts b/lib/msal-browser/src/cache/BrowserStorage.ts index 105be2f6e0..5bd651bc52 100644 --- a/lib/msal-browser/src/cache/BrowserStorage.ts +++ b/lib/msal-browser/src/cache/BrowserStorage.ts @@ -270,6 +270,7 @@ export class BrowserStorage extends CacheManager { this.clearItemCookie(this.generateCacheKey(nonceKey)); this.clearItemCookie(this.generateCacheKey(TemporaryCacheKeys.REQUEST_STATE)); this.clearItemCookie(this.generateCacheKey(TemporaryCacheKeys.ORIGIN_URI)); + this.clearItemCookie(this.generateCacheKey(TemporaryCacheKeys.CORRELATION_ID)); } /** @@ -385,6 +386,7 @@ export class BrowserStorage extends CacheManager { this.removeItem(this.generateCacheKey(TemporaryCacheKeys.REQUEST_PARAMS)); this.removeItem(this.generateCacheKey(TemporaryCacheKeys.ORIGIN_URI)); this.removeItem(this.generateCacheKey(TemporaryCacheKeys.URL_HASH)); + this.removeItem(this.generateCacheKey(TemporaryCacheKeys.CORRELATION_ID)); } cleanRequest(): void { diff --git a/lib/msal-browser/src/interaction_handler/InteractionHandler.ts b/lib/msal-browser/src/interaction_handler/InteractionHandler.ts index ed1ba9cf1f..40d2cafe01 100644 --- a/lib/msal-browser/src/interaction_handler/InteractionHandler.ts +++ b/lib/msal-browser/src/interaction_handler/InteractionHandler.ts @@ -2,7 +2,7 @@ * Copyright (c) Microsoft Corporation. All rights reserved. * Licensed under the MIT License. */ -import { StringUtils, AuthorizationCodeRequest, CacheSchemaType, AuthenticationResult, AuthorizationCodeClient } from "@azure/msal-common"; +import { StringUtils, AuthorizationCodeRequest, CacheSchemaType, AuthenticationResult, AuthorizationCodeClient, TelemetryManager } from "@azure/msal-common"; import { BrowserStorage } from "../cache/BrowserStorage"; import { BrowserAuthError } from "../error/BrowserAuthError"; import { TemporaryCacheKeys } from "../utils/BrowserConstants"; @@ -31,7 +31,7 @@ export abstract class InteractionHandler { * Function to handle response parameters from hash. * @param locationHash */ - async handleCodeResponse(locationHash: string): Promise { + async handleCodeResponse(locationHash: string, telemetryManager: TelemetryManager): Promise { // Check that location hash isn't empty. if (StringUtils.isEmpty(locationHash)) { throw BrowserAuthError.createEmptyHashError(locationHash); @@ -49,7 +49,7 @@ export abstract class InteractionHandler { this.authCodeRequest.code = authCode; // Acquire token with retrieved code. - const tokenResponse = await this.authModule.acquireToken(this.authCodeRequest, cachedNonce, requestState); + const tokenResponse = await this.authModule.acquireToken(this.authCodeRequest, cachedNonce, requestState, telemetryManager); this.browserStorage.cleanRequest(); return tokenResponse; } diff --git a/lib/msal-browser/src/interaction_handler/RedirectHandler.ts b/lib/msal-browser/src/interaction_handler/RedirectHandler.ts index 6a832a48f6..214dbbaa99 100644 --- a/lib/msal-browser/src/interaction_handler/RedirectHandler.ts +++ b/lib/msal-browser/src/interaction_handler/RedirectHandler.ts @@ -2,7 +2,7 @@ * Copyright (c) Microsoft Corporation. All rights reserved. * Licensed under the MIT License. */ -import { StringUtils, AuthorizationCodeRequest, ICrypto, CacheSchemaType, AuthenticationResult } from "@azure/msal-common"; +import { StringUtils, AuthorizationCodeRequest, ICrypto, CacheSchemaType, AuthenticationResult, TelemetryManager } from "@azure/msal-common"; import { InteractionHandler } from "./InteractionHandler"; import { BrowserAuthError } from "../error/BrowserAuthError"; import { BrowserConstants, TemporaryCacheKeys } from "../utils/BrowserConstants"; @@ -20,6 +20,7 @@ export class RedirectHandler extends InteractionHandler { // Cache start page, returns to this page after redirectUri if navigateToLoginRequestUrl is true const loginStartPage = redirectStartPage || window.location.href; this.browserStorage.setItem(this.browserStorage.generateCacheKey(TemporaryCacheKeys.ORIGIN_URI), loginStartPage, CacheSchemaType.TEMPORARY); + this.browserStorage.setItem(this.browserStorage.generateCacheKey(TemporaryCacheKeys.CORRELATION_ID), authCodeRequest.correlationId, CacheSchemaType.TEMPORARY); // Set interaction status in the library. this.browserStorage.setItem(this.browserStorage.generateCacheKey(BrowserConstants.INTERACTION_STATUS_KEY), BrowserConstants.INTERACTION_IN_PROGRESS_VALUE, CacheSchemaType.TEMPORARY); @@ -45,7 +46,7 @@ export class RedirectHandler extends InteractionHandler { * Handle authorization code response in the window. * @param hash */ - async handleCodeResponse(locationHash: string, browserCrypto?: ICrypto): Promise { + async handleCodeResponse(locationHash: string, telemetryManager: TelemetryManager, browserCrypto?: ICrypto): Promise { // Check that location hash isn't empty. if (StringUtils.isEmpty(locationHash)) { throw BrowserAuthError.createEmptyHashError(locationHash); @@ -68,7 +69,7 @@ export class RedirectHandler extends InteractionHandler { this.browserStorage.removeItem(this.browserStorage.generateCacheKey(TemporaryCacheKeys.URL_HASH)); // Acquire token with retrieved code. - const tokenResponse = await this.authModule.acquireToken(this.authCodeRequest, cachedNonce, requestState); + const tokenResponse = await this.authModule.acquireToken(this.authCodeRequest, cachedNonce, requestState, telemetryManager); this.browserStorage.cleanRequest(); return tokenResponse; } diff --git a/lib/msal-browser/src/utils/BrowserConstants.ts b/lib/msal-browser/src/utils/BrowserConstants.ts index 6902f1103c..705acfe977 100644 --- a/lib/msal-browser/src/utils/BrowserConstants.ts +++ b/lib/msal-browser/src/utils/BrowserConstants.ts @@ -45,8 +45,18 @@ export enum TemporaryCacheKeys { REQUEST_STATE = "request.state", NONCE_IDTOKEN = "nonce.id_token", ORIGIN_URI = "request.origin", + CORRELATION_ID = "request.correlationId", RENEW_STATUS = "token.renew.status", URL_HASH = "urlHash", REQUEST_PARAMS = "request.params", SCOPES = "scopes" } + +export enum ApiId { + acquireTokenRedirect = 861, + acquireTokenPopup = 862, + ssoSilent = 863, + acquireTokenSilent_authCode = 864, + handleRedirectPromise = 865, + acquireTokenSilent_silentFlow = 61 +}; From b6598c60db78d5eb78049be1c8af764e46b61bb8 Mon Sep 17 00:00:00 2001 From: Thomas Norling Date: Mon, 13 Jul 2020 10:16:50 -0700 Subject: [PATCH 02/12] Always generateCacheKey --- lib/msal-browser/src/cache/BrowserStorage.ts | 30 ++++++++++++++------ 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/lib/msal-browser/src/cache/BrowserStorage.ts b/lib/msal-browser/src/cache/BrowserStorage.ts index 5bd651bc52..a24f8410f7 100644 --- a/lib/msal-browser/src/cache/BrowserStorage.ts +++ b/lib/msal-browser/src/cache/BrowserStorage.ts @@ -109,21 +109,27 @@ export class BrowserStorage extends CacheManager { * @param value */ setItem(key: string, value: string | object, type: string): void { + const cacheKey = this.generateCacheKey(key); // save the cacheItem switch (type) { case CacheSchemaType.ACCOUNT: case CacheSchemaType.CREDENTIAL: case CacheSchemaType.APP_META_DATA: - this.windowStorage.setItem(key, JSON.stringify(value)); + this.windowStorage.setItem(cacheKey, JSON.stringify(value)); break; case CacheSchemaType.TEMPORARY: { const stringVal = value as string; - this.windowStorage.setItem(key, stringVal); + this.windowStorage.setItem(cacheKey, stringVal); if (this.cacheConfig.storeAuthStateInCookie) { - this.setItemCookie(key, stringVal); + this.setItemCookie(cacheKey, stringVal); } break; } + case CacheSchemaType.TELEMETRY: { + const stringVal = value as string; + this.windowStorage.setItem(cacheKey, stringVal); + break; + } default: { throw BrowserAuthError.createInvalidCacheTypeError(); } @@ -136,7 +142,8 @@ export class BrowserStorage extends CacheManager { * @param key */ getItem(key: string, type: string): string | object { - const value = this.windowStorage.getItem(key); + const cacheKey = this.generateCacheKey(key); + const value = this.windowStorage.getItem(cacheKey); if (StringUtils.isEmpty(value)) { return null; } @@ -146,7 +153,7 @@ export class BrowserStorage extends CacheManager { return (CacheManager.toObject(account, JSON.parse(value)) as AccountEntity); } case CacheSchemaType.CREDENTIAL: { - const credentialType = CredentialEntity.getCredentialType(key); + const credentialType = CredentialEntity.getCredentialType(cacheKey); switch (credentialType) { case CredentialType.ID_TOKEN: { const idTokenEntity: IdTokenEntity = new IdTokenEntity(); @@ -166,12 +173,15 @@ export class BrowserStorage extends CacheManager { return (JSON.parse(value) as AppMetadataEntity); } case CacheSchemaType.TEMPORARY: { - const itemCookie = this.getItemCookie(key); + const itemCookie = this.getItemCookie(cacheKey); if (this.cacheConfig.storeAuthStateInCookie) { return itemCookie; } return value; } + case CacheSchemaType.TELEMETRY: { + return value; + } default: { throw BrowserAuthError.createInvalidCacheTypeError(); } @@ -184,9 +194,10 @@ export class BrowserStorage extends CacheManager { * @param key */ removeItem(key: string): boolean { - this.windowStorage.removeItem(key); + const cacheKey = this.generateCacheKey(key); + this.windowStorage.removeItem(cacheKey); if (this.cacheConfig.storeAuthStateInCookie) { - this.clearItemCookie(key); + this.clearItemCookie(cacheKey); } return true; } @@ -196,7 +207,8 @@ export class BrowserStorage extends CacheManager { * @param key */ containsKey(key: string): boolean { - return this.windowStorage.hasOwnProperty(key); + const cacheKey = this.generateCacheKey(key); + return this.windowStorage.hasOwnProperty(cacheKey); } /** From 455f3e5c40661dca7850ef93cf562a95a9916161 Mon Sep 17 00:00:00 2001 From: Thomas Norling Date: Mon, 13 Jul 2020 10:21:14 -0700 Subject: [PATCH 03/12] Add comment to API codes --- lib/msal-browser/src/utils/BrowserConstants.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/msal-browser/src/utils/BrowserConstants.ts b/lib/msal-browser/src/utils/BrowserConstants.ts index 705acfe977..a08a5711f7 100644 --- a/lib/msal-browser/src/utils/BrowserConstants.ts +++ b/lib/msal-browser/src/utils/BrowserConstants.ts @@ -52,6 +52,12 @@ export enum TemporaryCacheKeys { SCOPES = "scopes" } +/** + * API Codes for Telemetry purposes. + * Before adding a new code you must claim it in the MSAL Telemetry tracker as these number spaces are shared across all MSALs + * 0-99 Silent Flow + * 800-899 Auth Code Flow + */ export enum ApiId { acquireTokenRedirect = 861, acquireTokenPopup = 862, From f73ed7d06ab7430fedb0221b4ef3e83f9870606e Mon Sep 17 00:00:00 2001 From: Thomas Norling Date: Tue, 14 Jul 2020 16:05:33 -0700 Subject: [PATCH 04/12] Tests passing --- .../test/app/PublicClientApplication.spec.ts | 95 +++++++++++++++---- .../RedirectHandler.spec.ts | 10 +- 2 files changed, 79 insertions(+), 26 deletions(-) diff --git a/lib/msal-browser/test/app/PublicClientApplication.spec.ts b/lib/msal-browser/test/app/PublicClientApplication.spec.ts index d44eb0ecfa..acd2846c0e 100644 --- a/lib/msal-browser/test/app/PublicClientApplication.spec.ts +++ b/lib/msal-browser/test/app/PublicClientApplication.spec.ts @@ -10,7 +10,7 @@ import { TEST_CONFIG, TEST_URIS, TEST_HASHES, TEST_TOKENS, TEST_DATA_CLIENT_INFO import { ServerError, Constants, AccountInfo, IdTokenClaims, PromptValue, AuthenticationResult, AuthorizationCodeRequest, AuthorizationUrlRequest, IdToken, PersistentCacheKeys, SilentFlowRequest, CacheSchemaType, TimeUtils, AuthorizationCodeClient, ResponseMode, SilentFlowClient, TrustedAuthority, EndSessionRequest, CloudDiscoveryMetadata } from "@azure/msal-common"; import { BrowserConfigurationAuthError } from "../../src/error/BrowserConfigurationAuthError"; import { BrowserUtils } from "../../src/utils/BrowserUtils"; -import { BrowserConstants, TemporaryCacheKeys } from "../../src/utils/BrowserConstants"; +import { BrowserConstants, TemporaryCacheKeys, ApiId } from "../../src/utils/BrowserConstants"; import { Base64Encode } from "../../src/encode/Base64Encode"; import { XhrClient } from "../../src/network/XhrClient"; import { BrowserAuthErrorMessage, BrowserAuthError } from "../../src/error/BrowserAuthError"; @@ -437,11 +437,24 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { challenge: TEST_CONFIG.TEST_CHALLENGE, verifier: TEST_CONFIG.TEST_VERIFIER }); - const loginUrlErr = "loginUrlErr"; - sinon.stub(AuthorizationCodeClient.prototype, "getAuthCodeUrl").throws(new BrowserAuthError(loginUrlErr)); - await expect(pca.loginRedirect(emptyRequest)).to.be.rejectedWith(loginUrlErr); - await expect(pca.loginRedirect(emptyRequest)).to.be.rejectedWith(BrowserAuthError); - expect(browserStorage.getKeys()).to.be.empty; + + const testError = { + errorCode: "create_login_url_error", + errorMessage: "Error in creating a login url" + } + sinon.stub(AuthorizationCodeClient.prototype, "getAuthCodeUrl").throws(testError); + try { + await pca.loginRedirect(emptyRequest); + } catch (e) { + // Test that error was cached for telemetry purposes and then thrown + expect(window.sessionStorage).to.be.length(1); + const failures = window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.server-telemetry-failures`) + const failureObj = JSON.parse(failures); + expect(failureObj.requests).to.be.length(2); + expect(failureObj.requests[0]).to.eq(ApiId.acquireTokenRedirect); + expect(failureObj.errors[0]).to.eq(testError.errorCode); + expect(e).to.be.eq(testError); + } }); it("Uses adal token from cache if it is present.", async () => { @@ -628,12 +641,25 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { sinon.stub(CryptoOps.prototype, "generatePkceCodes").resolves({ challenge: TEST_CONFIG.TEST_CHALLENGE, verifier: TEST_CONFIG.TEST_VERIFIER - }); - const loginUrlErr = "loginUrlErr"; - sinon.stub(AuthorizationCodeClient.prototype, "getAuthCodeUrl").throws(new BrowserAuthError(loginUrlErr)); - await expect(pca.acquireTokenRedirect(emptyRequest)).to.be.rejectedWith(loginUrlErr); - await expect(pca.acquireTokenRedirect(emptyRequest)).to.be.rejectedWith(BrowserAuthError); - expect(browserStorage.getKeys()).to.be.empty; + }); + + const testError = { + errorCode: "create_login_url_error", + errorMessage: "Error in creating a login url" + } + sinon.stub(AuthorizationCodeClient.prototype, "getAuthCodeUrl").throws(testError); + try { + await pca.acquireTokenRedirect(emptyRequest); + } catch (e) { + // Test that error was cached for telemetry purposes and then thrown + expect(window.sessionStorage).to.be.length(1); + const failures = window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.server-telemetry-failures`) + const failureObj = JSON.parse(failures); + expect(failureObj.requests).to.be.length(2); + expect(failureObj.requests[0]).to.eq(ApiId.acquireTokenRedirect); + expect(failureObj.errors[0]).to.eq(testError.errorCode); + expect(e).to.be.eq(testError); + } }); it("Uses adal token from cache if it is present.", async () => { @@ -798,7 +824,10 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { }); it("catches error and cleans cache before rethrowing", async () => { - const testError = "Error in creating a login url"; + const testError = { + errorCode: "create_login_url_error", + errorMessage: "Error in creating a login url" + } sinon.stub(AuthorizationCodeClient.prototype, "getAuthCodeUrl").resolves(testNavUrl); sinon.stub(PopupHandler.prototype, "initiateAuthRequest").throws(testError); sinon.stub(CryptoOps.prototype, "generatePkceCodes").resolves({ @@ -809,8 +838,14 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { try { const tokenResp = await pca.loginPopup(null); } catch (e) { - expect(window.sessionStorage).to.be.empty; - expect(`${e}`).to.be.eq(testError); + // Test that error was cached for telemetry purposes and then thrown + expect(window.sessionStorage).to.be.length(1); + const failures = window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.server-telemetry-failures`) + const failureObj = JSON.parse(failures); + expect(failureObj.requests).to.be.length(2); + expect(failureObj.requests[0]).to.eq(ApiId.acquireTokenPopup); + expect(failureObj.errors[0]).to.eq(testError.errorCode); + expect(e).to.be.eq(testError); } }); }); @@ -886,7 +921,10 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { }); it("catches error and cleans cache before rethrowing", async () => { - const testError = "Error in creating a login url"; + const testError = { + errorCode: "create_login_url_error", + errorMessage: "Error in creating a login url" + } sinon.stub(AuthorizationCodeClient.prototype, "getAuthCodeUrl").resolves(testNavUrl); sinon.stub(PopupHandler.prototype, "initiateAuthRequest").throws(testError); sinon.stub(CryptoOps.prototype, "generatePkceCodes").resolves({ @@ -900,8 +938,14 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { scopes: TEST_CONFIG.DEFAULT_SCOPES }); } catch (e) { - expect(window.sessionStorage).to.be.empty; - expect(`${e}`).to.be.eq(testError); + // Test that error was cached for telemetry purposes and then thrown + expect(window.sessionStorage).to.be.length(1); + const failures = window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.server-telemetry-failures`) + const failureObj = JSON.parse(failures); + expect(failureObj.requests).to.be.length(2); + expect(failureObj.requests[0]).to.eq(ApiId.acquireTokenPopup); + expect(failureObj.errors[0]).to.eq(testError.errorCode); + expect(e).to.be.eq(testError); } }); }); @@ -1036,7 +1080,10 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { }); it("throws error that SilentFlowClient.acquireToken() throws", async () => { - const testError = "Error in creating a login url"; + const testError = { + errorCode: "create_login_url_error", + errorMessage: "Error in creating a login url" + } const testAccount: AccountInfo = { homeAccountId: TEST_DATA_CLIENT_INFO.TEST_HOME_ACCOUNT_ID, environment: "login.windows.net", @@ -1050,8 +1097,14 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { account: testAccount }); } catch (e) { - expect(`${e}`).to.contain(testError); - expect(window.sessionStorage).to.be.empty; + // Test that error was cached for telemetry purposes and then thrown + expect(window.sessionStorage).to.be.length(1); + const failures = window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.server-telemetry-failures`) + const failureObj = JSON.parse(failures); + expect(failureObj.requests).to.be.length(2); + expect(failureObj.requests[0]).to.eq(ApiId.acquireTokenSilent_silentFlow); + expect(failureObj.errors[0]).to.eq(testError.errorCode); + expect(e).to.be.eq(testError); } }); diff --git a/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts b/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts index 000a87ce9b..d643823fc2 100644 --- a/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts +++ b/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts @@ -173,11 +173,11 @@ describe("RedirectHandler.ts Unit Tests", () => { describe("handleCodeResponse()", () => { it("throws error if given hash is empty", async () => { - await expect(redirectHandler.handleCodeResponse("")).to.be.rejectedWith(BrowserAuthErrorMessage.hashEmptyError.desc); - await expect(redirectHandler.handleCodeResponse("")).to.be.rejectedWith(BrowserAuthError); + await expect(redirectHandler.handleCodeResponse("", null)).to.be.rejectedWith(BrowserAuthErrorMessage.hashEmptyError.desc); + await expect(redirectHandler.handleCodeResponse("", null)).to.be.rejectedWith(BrowserAuthError); - await expect(redirectHandler.handleCodeResponse(null)).to.be.rejectedWith(BrowserAuthErrorMessage.hashEmptyError.desc); - await expect(redirectHandler.handleCodeResponse(null)).to.be.rejectedWith(BrowserAuthError); + await expect(redirectHandler.handleCodeResponse(null, null)).to.be.rejectedWith(BrowserAuthErrorMessage.hashEmptyError.desc); + await expect(redirectHandler.handleCodeResponse(null, null)).to.be.rejectedWith(BrowserAuthError); }); it("successfully handles response", async () => { @@ -223,7 +223,7 @@ describe("RedirectHandler.ts Unit Tests", () => { sinon.stub(AuthorizationCodeClient.prototype, "handleFragmentResponse").returns(testCodeResponse); sinon.stub(AuthorizationCodeClient.prototype, "acquireToken").resolves(testTokenResponse); - const tokenResponse = await redirectHandler.handleCodeResponse(TEST_HASHES.TEST_SUCCESS_CODE_HASH, browserCrypto); + const tokenResponse = await redirectHandler.handleCodeResponse(TEST_HASHES.TEST_SUCCESS_CODE_HASH, undefined, browserCrypto); expect(tokenResponse).to.deep.eq(testTokenResponse); expect(browserStorage.getItem(browserStorage.generateCacheKey(BrowserConstants.INTERACTION_STATUS_KEY), CacheSchemaType.TEMPORARY)).to.be.null; expect(browserStorage.getItem(browserStorage.generateCacheKey(TemporaryCacheKeys.URL_HASH), CacheSchemaType.TEMPORARY)).to.be.null; From da8b6c1e5e3c2e5d01734f425950443dac6764ff Mon Sep 17 00:00:00 2001 From: Thomas Norling Date: Thu, 16 Jul 2020 17:05:32 -0700 Subject: [PATCH 05/12] Update name to ServerTelemetryManager --- .../src/app/PublicClientApplication.ts | 18 +++++++++--------- .../interaction_handler/InteractionHandler.ts | 4 ++-- .../src/interaction_handler/RedirectHandler.ts | 4 ++-- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/lib/msal-browser/src/app/PublicClientApplication.ts b/lib/msal-browser/src/app/PublicClientApplication.ts index 49d41dff59..7450a2ca5a 100644 --- a/lib/msal-browser/src/app/PublicClientApplication.ts +++ b/lib/msal-browser/src/app/PublicClientApplication.ts @@ -29,7 +29,7 @@ import { EndSessionRequest, BaseAuthRequest, Logger, - TelemetryManager + ServerTelemetryManager } from "@azure/msal-common"; import { buildConfiguration, Configuration } from "../config/Configuration"; import { BrowserStorage } from "../cache/BrowserStorage"; @@ -181,7 +181,7 @@ export class PublicClientApplication implements IPublicClientApplication { return null; } const correlationId = this.browserStorage.getItem(this.browserStorage.generateCacheKey(TemporaryCacheKeys.CORRELATION_ID), CacheSchemaType.TEMPORARY) as string; - const telemetryManager = new TelemetryManager(this.browserStorage, ApiId.handleRedirectPromise, correlationId); + const telemetryManager = new ServerTelemetryManager(this.browserStorage, ApiId.handleRedirectPromise, correlationId); try { // Hash contains known properties - handle and return in callback @@ -222,7 +222,7 @@ export class PublicClientApplication implements IPublicClientApplication { async acquireTokenRedirect(request: RedirectRequest): Promise { // Preflight request const validRequest: AuthorizationUrlRequest = this.preflightInteractiveRequest(request); - const telemetryManager = new TelemetryManager(this.browserStorage, ApiId.acquireTokenRedirect, validRequest.correlationId); + const telemetryManager = new ServerTelemetryManager(this.browserStorage, ApiId.acquireTokenRedirect, validRequest.correlationId); try { // Create auth code request and generate PKCE params @@ -271,7 +271,7 @@ export class PublicClientApplication implements IPublicClientApplication { async acquireTokenPopup(request: PopupRequest): Promise { // Preflight request const validRequest: AuthorizationUrlRequest = this.preflightInteractiveRequest(request); - const telemetryManager = new TelemetryManager(this.browserStorage, ApiId.acquireTokenPopup, validRequest.correlationId); + const telemetryManager = new ServerTelemetryManager(this.browserStorage, ApiId.acquireTokenPopup, validRequest.correlationId); try { // Create auth code request and generate PKCE params @@ -296,7 +296,7 @@ export class PublicClientApplication implements IPublicClientApplication { * Helper which acquires an authorization code with a popup from given url, and exchanges the code for a set of OAuth tokens. * @param navigateUrl */ - private async popupTokenHelper(navigateUrl: string, authCodeRequest: AuthorizationCodeRequest, authClient: AuthorizationCodeClient, telemetryManager: TelemetryManager): Promise { + private async popupTokenHelper(navigateUrl: string, authCodeRequest: AuthorizationCodeRequest, authClient: AuthorizationCodeClient, telemetryManager: ServerTelemetryManager): Promise { // Create popup interaction handler. const interactionHandler = new PopupHandler(authClient, this.browserStorage); // Show the UI once the url has been created. Get the window handle for the popup. @@ -347,7 +347,7 @@ export class PublicClientApplication implements IPublicClientApplication { prompt: PromptValue.NONE }); - const telemetryManager = new TelemetryManager(this.browserStorage, ApiId.ssoSilent, silentRequest.correlationId); + const telemetryManager = new ServerTelemetryManager(this.browserStorage, ApiId.ssoSilent, silentRequest.correlationId); try { // Create auth code request and generate PKCE params @@ -389,7 +389,7 @@ export class PublicClientApplication implements IPublicClientApplication { ...request, ...this.initializeBaseRequest(request) }; - let telemetryManager = new TelemetryManager(this.browserStorage, ApiId.acquireTokenSilent_silentFlow, silentRequest.correlationId, silentRequest.forceRefresh); + let telemetryManager = new ServerTelemetryManager(this.browserStorage, ApiId.acquireTokenSilent_silentFlow, silentRequest.correlationId, silentRequest.forceRefresh); try { const silentAuthClient = await this.createSilentFlowClient(silentRequest.authority); // Send request to renew token. Auth module will throw errors if token cannot be renewed. @@ -404,7 +404,7 @@ export class PublicClientApplication implements IPublicClientApplication { ...silentRequest, prompt: PromptValue.NONE }); - telemetryManager = new TelemetryManager(this.browserStorage, ApiId.acquireTokenSilent_authCode, silentAuthUrlRequest.correlationId); + telemetryManager = new ServerTelemetryManager(this.browserStorage, ApiId.acquireTokenSilent_authCode, silentAuthUrlRequest.correlationId); try { // Create auth code request and generate PKCE params @@ -437,7 +437,7 @@ export class PublicClientApplication implements IPublicClientApplication { * @param navigateUrl * @param userRequestScopes */ - private async silentTokenHelper(navigateUrl: string, authCodeRequest: AuthorizationCodeRequest, authClient: AuthorizationCodeClient, userRequestScopes: string, telemetryManager: TelemetryManager): Promise { + private async silentTokenHelper(navigateUrl: string, authCodeRequest: AuthorizationCodeRequest, authClient: AuthorizationCodeClient, userRequestScopes: string, telemetryManager: ServerTelemetryManager): Promise { // Create silent handler const silentHandler = new SilentHandler(authClient, this.browserStorage, this.config.system.loadFrameTimeout); // Get the frame handle for the silent request diff --git a/lib/msal-browser/src/interaction_handler/InteractionHandler.ts b/lib/msal-browser/src/interaction_handler/InteractionHandler.ts index 578f66a8e3..8fc118fe06 100644 --- a/lib/msal-browser/src/interaction_handler/InteractionHandler.ts +++ b/lib/msal-browser/src/interaction_handler/InteractionHandler.ts @@ -2,7 +2,7 @@ * Copyright (c) Microsoft Corporation. All rights reserved. * Licensed under the MIT License. */ -import { StringUtils, AuthorizationCodeRequest, CacheSchemaType, AuthenticationResult, AuthorizationCodeClient, TelemetryManager } from "@azure/msal-common"; +import { StringUtils, AuthorizationCodeRequest, CacheSchemaType, AuthenticationResult, AuthorizationCodeClient, ServerTelemetryManager } from "@azure/msal-common"; import { BrowserStorage } from "../cache/BrowserStorage"; import { BrowserAuthError } from "../error/BrowserAuthError"; import { TemporaryCacheKeys } from "../utils/BrowserConstants"; @@ -31,7 +31,7 @@ export abstract class InteractionHandler { * Function to handle response parameters from hash. * @param locationHash */ - async handleCodeResponse(locationHash: string, telemetryManager: TelemetryManager): Promise { + async handleCodeResponse(locationHash: string, telemetryManager: ServerTelemetryManager): Promise { // Check that location hash isn't empty. if (StringUtils.isEmpty(locationHash)) { throw BrowserAuthError.createEmptyHashError(locationHash); diff --git a/lib/msal-browser/src/interaction_handler/RedirectHandler.ts b/lib/msal-browser/src/interaction_handler/RedirectHandler.ts index 4ead512bd9..fdfa3d2cc3 100644 --- a/lib/msal-browser/src/interaction_handler/RedirectHandler.ts +++ b/lib/msal-browser/src/interaction_handler/RedirectHandler.ts @@ -2,7 +2,7 @@ * Copyright (c) Microsoft Corporation. All rights reserved. * Licensed under the MIT License. */ -import { StringUtils, AuthorizationCodeRequest, ICrypto, CacheSchemaType, AuthenticationResult, TelemetryManager } from "@azure/msal-common"; +import { StringUtils, AuthorizationCodeRequest, ICrypto, CacheSchemaType, AuthenticationResult, ServerTelemetryManager } from "@azure/msal-common"; import { InteractionHandler } from "./InteractionHandler"; import { BrowserAuthError } from "../error/BrowserAuthError"; import { BrowserConstants, TemporaryCacheKeys } from "../utils/BrowserConstants"; @@ -46,7 +46,7 @@ export class RedirectHandler extends InteractionHandler { * Handle authorization code response in the window. * @param hash */ - async handleCodeResponse(locationHash: string, telemetryManager: TelemetryManager, browserCrypto?: ICrypto): Promise { + async handleCodeResponse(locationHash: string, telemetryManager: ServerTelemetryManager, browserCrypto?: ICrypto): Promise { // Check that location hash isn't empty. if (StringUtils.isEmpty(locationHash)) { throw BrowserAuthError.createEmptyHashError(locationHash); From a20f4b9f5e42a3d40bda396bc1e5ed02c01c269b Mon Sep 17 00:00:00 2001 From: Thomas Norling Date: Wed, 22 Jul 2020 12:10:09 -0700 Subject: [PATCH 06/12] Update BrowserStorage to return object --- lib/msal-browser/src/cache/BrowserStorage.ts | 7 +++---- lib/msal-common/src/index.ts | 1 + 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/msal-browser/src/cache/BrowserStorage.ts b/lib/msal-browser/src/cache/BrowserStorage.ts index 2add146cbe..f9f3070d18 100644 --- a/lib/msal-browser/src/cache/BrowserStorage.ts +++ b/lib/msal-browser/src/cache/BrowserStorage.ts @@ -2,7 +2,7 @@ * Copyright (c) Microsoft Corporation. All rights reserved. * Licensed under the MIT License. */ -import { Constants, PersistentCacheKeys, StringUtils, AuthorizationCodeRequest, ICrypto, CacheSchemaType, AccountEntity, IdTokenEntity, CredentialType, AccessTokenEntity, RefreshTokenEntity, AppMetadataEntity, CacheManager, CredentialEntity } from "@azure/msal-common"; +import { Constants, PersistentCacheKeys, StringUtils, AuthorizationCodeRequest, ICrypto, CacheSchemaType, AccountEntity, IdTokenEntity, CredentialType, AccessTokenEntity, RefreshTokenEntity, AppMetadataEntity, CacheManager, CredentialEntity, ServerTelemetryCacheValue } from "@azure/msal-common"; import { CacheOptions } from "../config/Configuration"; import { BrowserAuthError } from "../error/BrowserAuthError"; import { BrowserConfigurationAuthError } from "../error/BrowserConfigurationAuthError"; @@ -126,8 +126,7 @@ export class BrowserStorage extends CacheManager { break; } case CacheSchemaType.TELEMETRY: { - const stringVal = value as string; - this.windowStorage.setItem(cacheKey, stringVal); + this.windowStorage.setItem(cacheKey, JSON.stringify(value)); break; } default: { @@ -180,7 +179,7 @@ export class BrowserStorage extends CacheManager { return value; } case CacheSchemaType.TELEMETRY: { - return value; + return JSON.parse(value) as ServerTelemetryCacheValue; } default: { throw BrowserAuthError.createInvalidCacheTypeError(); diff --git a/lib/msal-common/src/index.ts b/lib/msal-common/src/index.ts index 01081040e8..85f6ed4f1f 100644 --- a/lib/msal-common/src/index.ts +++ b/lib/msal-common/src/index.ts @@ -55,4 +55,5 @@ export { StringDict } from "./utils/MsalTypes"; export { ProtocolUtils } from "./utils/ProtocolUtils"; export { TimeUtils } from "./utils/TimeUtils"; // Telemetry +export { ServerTelemetryCacheValue } from "./telemetry/server/ServerTelemetryCacheValue"; export { ServerTelemetryManager } from "./telemetry/server/ServerTelemetryManager"; From 4d6d28576675181f339c60adad5ede86c93f2c66 Mon Sep 17 00:00:00 2001 From: Thomas Norling Date: Wed, 22 Jul 2020 14:12:19 -0700 Subject: [PATCH 07/12] Fix tests --- lib/msal-browser/src/cache/BrowserStorage.ts | 24 +++++------ .../test/app/PublicClientApplication.spec.ts | 42 +++++++++---------- 2 files changed, 31 insertions(+), 35 deletions(-) diff --git a/lib/msal-browser/src/cache/BrowserStorage.ts b/lib/msal-browser/src/cache/BrowserStorage.ts index f9f3070d18..6f6e67eb27 100644 --- a/lib/msal-browser/src/cache/BrowserStorage.ts +++ b/lib/msal-browser/src/cache/BrowserStorage.ts @@ -109,24 +109,23 @@ export class BrowserStorage extends CacheManager { * @param value */ setItem(key: string, value: string | object, type: string): void { - const cacheKey = this.generateCacheKey(key); // save the cacheItem switch (type) { case CacheSchemaType.ACCOUNT: case CacheSchemaType.CREDENTIAL: case CacheSchemaType.APP_META_DATA: - this.windowStorage.setItem(cacheKey, JSON.stringify(value)); + this.windowStorage.setItem(key, JSON.stringify(value)); break; case CacheSchemaType.TEMPORARY: { const stringVal = value as string; - this.windowStorage.setItem(cacheKey, stringVal); + this.windowStorage.setItem(key, stringVal); if (this.cacheConfig.storeAuthStateInCookie) { - this.setItemCookie(cacheKey, stringVal); + this.setItemCookie(key, stringVal); } break; } case CacheSchemaType.TELEMETRY: { - this.windowStorage.setItem(cacheKey, JSON.stringify(value)); + this.windowStorage.setItem(key, JSON.stringify(value)); break; } default: { @@ -141,8 +140,7 @@ export class BrowserStorage extends CacheManager { * @param key */ getItem(key: string, type: string): string | object { - const cacheKey = this.generateCacheKey(key); - const value = this.windowStorage.getItem(cacheKey); + const value = this.windowStorage.getItem(key); if (StringUtils.isEmpty(value)) { return null; } @@ -152,7 +150,7 @@ export class BrowserStorage extends CacheManager { return (CacheManager.toObject(account, JSON.parse(value)) as AccountEntity); } case CacheSchemaType.CREDENTIAL: { - const credentialType = CredentialEntity.getCredentialType(cacheKey); + const credentialType = CredentialEntity.getCredentialType(key); switch (credentialType) { case CredentialType.ID_TOKEN: { const idTokenEntity: IdTokenEntity = new IdTokenEntity(); @@ -172,7 +170,7 @@ export class BrowserStorage extends CacheManager { return (JSON.parse(value) as AppMetadataEntity); } case CacheSchemaType.TEMPORARY: { - const itemCookie = this.getItemCookie(cacheKey); + const itemCookie = this.getItemCookie(key); if (this.cacheConfig.storeAuthStateInCookie) { return itemCookie; } @@ -193,10 +191,9 @@ export class BrowserStorage extends CacheManager { * @param key */ removeItem(key: string): boolean { - const cacheKey = this.generateCacheKey(key); - this.windowStorage.removeItem(cacheKey); + this.windowStorage.removeItem(key); if (this.cacheConfig.storeAuthStateInCookie) { - this.clearItemCookie(cacheKey); + this.clearItemCookie(key); } return true; } @@ -206,8 +203,7 @@ export class BrowserStorage extends CacheManager { * @param key */ containsKey(key: string): boolean { - const cacheKey = this.generateCacheKey(key); - return this.windowStorage.hasOwnProperty(cacheKey); + return this.windowStorage.hasOwnProperty(key); } /** diff --git a/lib/msal-browser/test/app/PublicClientApplication.spec.ts b/lib/msal-browser/test/app/PublicClientApplication.spec.ts index 1680e95484..bd877c0427 100644 --- a/lib/msal-browser/test/app/PublicClientApplication.spec.ts +++ b/lib/msal-browser/test/app/PublicClientApplication.spec.ts @@ -7,7 +7,7 @@ const expect = chai.expect; import sinon from "sinon"; import { PublicClientApplication } from "../../src/app/PublicClientApplication"; import { TEST_CONFIG, TEST_URIS, TEST_HASHES, TEST_TOKENS, TEST_DATA_CLIENT_INFO, TEST_TOKEN_LIFETIMES, RANDOM_TEST_GUID, DEFAULT_OPENID_CONFIG_RESPONSE, testNavUrl, testLogoutUrl, TEST_STATE_VALUES, testNavUrlNoRequest } from "../utils/StringConstants"; -import { ServerError, Constants, AccountInfo, IdTokenClaims, PromptValue, AuthenticationResult, AuthorizationCodeRequest, AuthorizationUrlRequest, IdToken, PersistentCacheKeys, SilentFlowRequest, CacheSchemaType, TimeUtils, AuthorizationCodeClient, ResponseMode, SilentFlowClient, TrustedAuthority, EndSessionRequest, CloudDiscoveryMetadata, AccountEntity, ProtocolUtils } from "@azure/msal-common"; +import { ServerError, Constants, AccountInfo, IdTokenClaims, PromptValue, AuthenticationResult, AuthorizationCodeRequest, AuthorizationUrlRequest, IdToken, PersistentCacheKeys, SilentFlowRequest, CacheSchemaType, TimeUtils, AuthorizationCodeClient, ResponseMode, SilentFlowClient, TrustedAuthority, EndSessionRequest, CloudDiscoveryMetadata, AccountEntity, ProtocolUtils, ServerTelemetryCacheValue } from "@azure/msal-common"; import { BrowserUtils } from "../../src/utils/BrowserUtils"; import { BrowserConstants, TemporaryCacheKeys, ApiId } from "../../src/utils/BrowserConstants"; import { Base64Encode } from "../../src/encode/Base64Encode"; @@ -554,10 +554,10 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { } catch (e) { // Test that error was cached for telemetry purposes and then thrown expect(window.sessionStorage).to.be.length(1); - const failures = window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.server-telemetry-failures`) - const failureObj = JSON.parse(failures); - expect(failureObj.requests).to.be.length(2); - expect(failureObj.requests[0]).to.eq(ApiId.acquireTokenRedirect); + const failures = window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.server-telemetry`) + const failureObj = JSON.parse(failures) as ServerTelemetryCacheValue; + expect(failureObj.failedRequests).to.be.length(2); + expect(failureObj.failedRequests[0]).to.eq(ApiId.acquireTokenRedirect); expect(failureObj.errors[0]).to.eq(testError.errorCode); expect(e).to.be.eq(testError); } @@ -759,10 +759,10 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { } catch (e) { // Test that error was cached for telemetry purposes and then thrown expect(window.sessionStorage).to.be.length(1); - const failures = window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.server-telemetry-failures`) - const failureObj = JSON.parse(failures); - expect(failureObj.requests).to.be.length(2); - expect(failureObj.requests[0]).to.eq(ApiId.acquireTokenRedirect); + const failures = window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.server-telemetry`) + const failureObj = JSON.parse(failures) as ServerTelemetryCacheValue; + expect(failureObj.failedRequests).to.be.length(2); + expect(failureObj.failedRequests[0]).to.eq(ApiId.acquireTokenRedirect); expect(failureObj.errors[0]).to.eq(testError.errorCode); expect(e).to.be.eq(testError); } @@ -946,10 +946,10 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { } catch (e) { // Test that error was cached for telemetry purposes and then thrown expect(window.sessionStorage).to.be.length(1); - const failures = window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.server-telemetry-failures`) - const failureObj = JSON.parse(failures); - expect(failureObj.requests).to.be.length(2); - expect(failureObj.requests[0]).to.eq(ApiId.acquireTokenPopup); + const failures = window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.server-telemetry`) + const failureObj = JSON.parse(failures) as ServerTelemetryCacheValue; + expect(failureObj.failedRequests).to.be.length(2); + expect(failureObj.failedRequests[0]).to.eq(ApiId.acquireTokenPopup); expect(failureObj.errors[0]).to.eq(testError.errorCode); expect(e).to.be.eq(testError); } @@ -1046,10 +1046,10 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { } catch (e) { // Test that error was cached for telemetry purposes and then thrown expect(window.sessionStorage).to.be.length(1); - const failures = window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.server-telemetry-failures`) - const failureObj = JSON.parse(failures); - expect(failureObj.requests).to.be.length(2); - expect(failureObj.requests[0]).to.eq(ApiId.acquireTokenPopup); + const failures = window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.server-telemetry`) + const failureObj = JSON.parse(failures) as ServerTelemetryCacheValue; + expect(failureObj.failedRequests).to.be.length(2); + expect(failureObj.failedRequests[0]).to.eq(ApiId.acquireTokenPopup); expect(failureObj.errors[0]).to.eq(testError.errorCode); expect(e).to.be.eq(testError); } @@ -1216,10 +1216,10 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { } catch (e) { // Test that error was cached for telemetry purposes and then thrown expect(window.sessionStorage).to.be.length(1); - const failures = window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.server-telemetry-failures`) - const failureObj = JSON.parse(failures); - expect(failureObj.requests).to.be.length(2); - expect(failureObj.requests[0]).to.eq(ApiId.acquireTokenSilent_silentFlow); + const failures = window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.server-telemetry`) + const failureObj = JSON.parse(failures) as ServerTelemetryCacheValue; + expect(failureObj.failedRequests).to.be.length(2); + expect(failureObj.failedRequests[0]).to.eq(ApiId.acquireTokenSilent_silentFlow); expect(failureObj.errors[0]).to.eq(testError.errorCode); expect(e).to.be.eq(testError); } From eed0241574c3828587f5b4e3de60def88a257b19 Mon Sep 17 00:00:00 2001 From: Thomas Norling Date: Wed, 22 Jul 2020 17:10:54 -0700 Subject: [PATCH 08/12] Pass telemetryManager to authCodeClient --- .../src/app/PublicClientApplication.ts | 82 +++++++++++-------- .../interaction_handler/InteractionHandler.ts | 4 +- .../interaction_handler/RedirectHandler.ts | 4 +- .../test/app/PublicClientApplication.spec.ts | 10 +-- .../RedirectHandler.spec.ts | 10 +-- 5 files changed, 62 insertions(+), 48 deletions(-) diff --git a/lib/msal-browser/src/app/PublicClientApplication.ts b/lib/msal-browser/src/app/PublicClientApplication.ts index 4c2571af60..682688c55c 100644 --- a/lib/msal-browser/src/app/PublicClientApplication.ts +++ b/lib/msal-browser/src/app/PublicClientApplication.ts @@ -29,7 +29,8 @@ import { EndSessionRequest, BaseAuthRequest, Logger, - ServerTelemetryManager + ServerTelemetryManager, + ServerTelemetryRequest } from "@azure/msal-common"; import { buildConfiguration, Configuration } from "../config/Configuration"; import { BrowserStorage } from "../cache/BrowserStorage"; @@ -183,17 +184,18 @@ export class PublicClientApplication implements IPublicClientApplication { this.browserStorage.cleanRequest(); return null; } + const correlationId = this.browserStorage.getItem(this.browserStorage.generateCacheKey(TemporaryCacheKeys.CORRELATION_ID), CacheSchemaType.TEMPORARY) as string; - const telemetryManager = new ServerTelemetryManager(this.browserStorage, ApiId.handleRedirectPromise, correlationId); + const serverTelemetryManager = this.initializeServerTelemetryManager(ApiId.handleRedirectPromise, correlationId); try { // Hash contains known properties - handle and return in callback const currentAuthority = this.browserStorage.getCachedAuthority(); - const authClient = await this.createAuthCodeClient(currentAuthority); + const authClient = await this.createAuthCodeClient(serverTelemetryManager, currentAuthority); const interactionHandler = new RedirectHandler(authClient, this.browserStorage); - return interactionHandler.handleCodeResponse(responseHash, telemetryManager, this.browserCrypto); + return interactionHandler.handleCodeResponse(responseHash, this.browserCrypto); } catch (e) { - telemetryManager.cacheFailedRequest(e); + serverTelemetryManager.cacheFailedRequest(e); this.browserStorage.cleanRequest(); throw e; } @@ -224,14 +226,14 @@ export class PublicClientApplication implements IPublicClientApplication { async acquireTokenRedirect(request: RedirectRequest): Promise { // Preflight request const validRequest: AuthorizationUrlRequest = this.preflightInteractiveRequest(request); - const telemetryManager = new ServerTelemetryManager(this.browserStorage, ApiId.acquireTokenRedirect, validRequest.correlationId); + const serverTelemetryManager = this.initializeServerTelemetryManager(ApiId.acquireTokenRedirect, validRequest.correlationId); try { // Create auth code request and generate PKCE params const authCodeRequest: AuthorizationCodeRequest = await this.initializeAuthorizationCodeRequest(validRequest); // Initialize the client - const authClient: AuthorizationCodeClient = await this.createAuthCodeClient(validRequest.authority); + const authClient: AuthorizationCodeClient = await this.createAuthCodeClient(serverTelemetryManager, validRequest.authority); // Create redirect interaction handler. const interactionHandler = new RedirectHandler(authClient, this.browserStorage); @@ -243,7 +245,7 @@ export class PublicClientApplication implements IPublicClientApplication { // Show the UI once the url has been created. Response will come back in the hash, which will be handled in the handleRedirectCallback function. interactionHandler.initiateAuthRequest(navigateUrl, authCodeRequest, redirectStartPage, this.browserCrypto); } catch (e) { - telemetryManager.cacheFailedRequest(e); + serverTelemetryManager.cacheFailedRequest(e); this.browserStorage.cleanRequest(); throw e; } @@ -273,22 +275,22 @@ export class PublicClientApplication implements IPublicClientApplication { async acquireTokenPopup(request: PopupRequest): Promise { // Preflight request const validRequest: AuthorizationUrlRequest = this.preflightInteractiveRequest(request); - const telemetryManager = new ServerTelemetryManager(this.browserStorage, ApiId.acquireTokenPopup, validRequest.correlationId); + const serverTelemetryManager = this.initializeServerTelemetryManager(ApiId.acquireTokenPopup, validRequest.correlationId); try { // Create auth code request and generate PKCE params const authCodeRequest: AuthorizationCodeRequest = await this.initializeAuthorizationCodeRequest(validRequest); // Initialize the client - const authClient: AuthorizationCodeClient = await this.createAuthCodeClient(validRequest.authority); + const authClient: AuthorizationCodeClient = await this.createAuthCodeClient(serverTelemetryManager, validRequest.authority); // Create acquire token url. const navigateUrl = await authClient.getAuthCodeUrl(validRequest); // Acquire token with popup - return await this.popupTokenHelper(navigateUrl, authCodeRequest, authClient, telemetryManager); + return await this.popupTokenHelper(navigateUrl, authCodeRequest, authClient); } catch (e) { - telemetryManager.cacheFailedRequest(e); + serverTelemetryManager.cacheFailedRequest(e); this.browserStorage.cleanRequest(); throw e; } @@ -298,7 +300,7 @@ export class PublicClientApplication implements IPublicClientApplication { * Helper which acquires an authorization code with a popup from given url, and exchanges the code for a set of OAuth tokens. * @param navigateUrl */ - private async popupTokenHelper(navigateUrl: string, authCodeRequest: AuthorizationCodeRequest, authClient: AuthorizationCodeClient, telemetryManager: ServerTelemetryManager): Promise { + private async popupTokenHelper(navigateUrl: string, authCodeRequest: AuthorizationCodeRequest, authClient: AuthorizationCodeClient): Promise { // Create popup interaction handler. const interactionHandler = new PopupHandler(authClient, this.browserStorage); // Show the UI once the url has been created. Get the window handle for the popup. @@ -306,7 +308,7 @@ export class PublicClientApplication implements IPublicClientApplication { // Monitor the window for the hash. Return the string value and close the popup when the hash is received. Default timeout is 60 seconds. const hash = await interactionHandler.monitorPopupForHash(popupWindow, this.config.system.windowHashTimeout); // Handle response from hash string. - return await interactionHandler.handleCodeResponse(hash, telemetryManager); + return await interactionHandler.handleCodeResponse(hash); } // #endregion @@ -348,7 +350,7 @@ export class PublicClientApplication implements IPublicClientApplication { prompt: PromptValue.NONE }); - const telemetryManager = new ServerTelemetryManager(this.browserStorage, ApiId.ssoSilent, silentRequest.correlationId); + const serverTelemetryManager = this.initializeServerTelemetryManager(ApiId.ssoSilent, silentRequest.correlationId); try { // Create auth code request and generate PKCE params @@ -358,14 +360,14 @@ export class PublicClientApplication implements IPublicClientApplication { const scopeString = silentRequest.scopes ? silentRequest.scopes.join(" ") : ""; // Initialize the client - const authClient: AuthorizationCodeClient = await this.createAuthCodeClient(silentRequest.authority); + const authClient: AuthorizationCodeClient = await this.createAuthCodeClient(serverTelemetryManager, silentRequest.authority); // Create authorize request url const navigateUrl = await authClient.getAuthCodeUrl(silentRequest); - return this.silentTokenHelper(navigateUrl, authCodeRequest, authClient, scopeString, telemetryManager); + return this.silentTokenHelper(navigateUrl, authCodeRequest, authClient, scopeString); } catch (e) { - telemetryManager.cacheFailedRequest(e); + serverTelemetryManager.cacheFailedRequest(e); this.browserStorage.cleanRequest(); throw e; } @@ -390,13 +392,13 @@ export class PublicClientApplication implements IPublicClientApplication { ...request, ...this.initializeBaseRequest(request) }; - let telemetryManager = new ServerTelemetryManager(this.browserStorage, ApiId.acquireTokenSilent_silentFlow, silentRequest.correlationId, silentRequest.forceRefresh); + let serverTelemetryManager = this.initializeServerTelemetryManager(ApiId.acquireTokenSilent_silentFlow, silentRequest.correlationId); try { - const silentAuthClient = await this.createSilentFlowClient(silentRequest.authority); + const silentAuthClient = await this.createSilentFlowClient(serverTelemetryManager, silentRequest.authority); // Send request to renew token. Auth module will throw errors if token cannot be renewed. - return await silentAuthClient.acquireToken(silentRequest, telemetryManager); + return await silentAuthClient.acquireToken(silentRequest); } catch (e) { - telemetryManager.cacheFailedRequest(e); + serverTelemetryManager.cacheFailedRequest(e); const isServerError = e instanceof ServerError; const isInteractionRequiredError = e instanceof InteractionRequiredAuthError; const isInvalidGrantError = (e.errorCode === BrowserConstants.INVALID_GRANT_ERROR); @@ -406,14 +408,14 @@ export class PublicClientApplication implements IPublicClientApplication { redirectUri: request.redirectUri, prompt: PromptValue.NONE }); - telemetryManager = new ServerTelemetryManager(this.browserStorage, ApiId.acquireTokenSilent_authCode, silentAuthUrlRequest.correlationId); + serverTelemetryManager = this.initializeServerTelemetryManager(ApiId.acquireTokenSilent_authCode, silentAuthUrlRequest.correlationId); try { // Create auth code request and generate PKCE params const authCodeRequest: AuthorizationCodeRequest = await this.initializeAuthorizationCodeRequest(silentAuthUrlRequest); // Initialize the client - const authClient: AuthorizationCodeClient = await this.createAuthCodeClient(silentAuthUrlRequest.authority); + const authClient: AuthorizationCodeClient = await this.createAuthCodeClient(serverTelemetryManager, silentAuthUrlRequest.authority); // Create authorize request url const navigateUrl = await authClient.getAuthCodeUrl(silentAuthUrlRequest); @@ -421,9 +423,9 @@ export class PublicClientApplication implements IPublicClientApplication { // Get scopeString for iframe ID const scopeString = silentAuthUrlRequest.scopes ? silentAuthUrlRequest.scopes.join(" ") : ""; - return this.silentTokenHelper(navigateUrl, authCodeRequest, authClient, scopeString, telemetryManager); + return this.silentTokenHelper(navigateUrl, authCodeRequest, authClient, scopeString); } catch (e) { - telemetryManager.cacheFailedRequest(e); + serverTelemetryManager.cacheFailedRequest(e); this.browserStorage.cleanRequest(); throw e; } @@ -439,7 +441,7 @@ export class PublicClientApplication implements IPublicClientApplication { * @param navigateUrl * @param userRequestScopes */ - private async silentTokenHelper(navigateUrl: string, authCodeRequest: AuthorizationCodeRequest, authClient: AuthorizationCodeClient, userRequestScopes: string, telemetryManager: ServerTelemetryManager): Promise { + private async silentTokenHelper(navigateUrl: string, authCodeRequest: AuthorizationCodeRequest, authClient: AuthorizationCodeClient, userRequestScopes: string): Promise { // Create silent handler const silentHandler = new SilentHandler(authClient, this.browserStorage, this.config.system.loadFrameTimeout); // Get the frame handle for the silent request @@ -447,7 +449,7 @@ export class PublicClientApplication implements IPublicClientApplication { // Monitor the window for the hash. Return the string value and close the popup when the hash is received. Default timeout is 60 seconds. const hash = await silentHandler.monitorIframeForHash(msalFrame, this.config.system.iframeHashTimeout); // Handle response from hash string. - return await silentHandler.handleCodeResponse(hash, telemetryManager); + return await silentHandler.handleCodeResponse(hash); } // #endregion @@ -461,7 +463,7 @@ export class PublicClientApplication implements IPublicClientApplication { */ async logout(logoutRequest?: EndSessionRequest): Promise { const validLogoutRequest = this.initializeLogoutRequest(logoutRequest); - const authClient = await this.createAuthCodeClient(validLogoutRequest && validLogoutRequest.authority); + const authClient = await this.createAuthCodeClient(null, validLogoutRequest && validLogoutRequest.authority); // create logout string and navigate user window to logout. Auth module will clear cache. const logoutUri: string = authClient.getLogoutUri(validLogoutRequest); BrowserUtils.navigateWindow(logoutUri); @@ -537,9 +539,9 @@ export class PublicClientApplication implements IPublicClientApplication { * Creates an Authorization Code Client with the given authority, or the default authority. * @param authorityUrl */ - private async createAuthCodeClient(authorityUrl?: string): Promise { + private async createAuthCodeClient(serverTelemetryManager: ServerTelemetryManager, authorityUrl?: string): Promise { // Create auth module. - const clientConfig = await this.getClientConfiguration(authorityUrl); + const clientConfig = await this.getClientConfiguration(serverTelemetryManager, authorityUrl); return new AuthorizationCodeClient(clientConfig); } @@ -547,9 +549,9 @@ export class PublicClientApplication implements IPublicClientApplication { * Creates an Silent Flow Client with the given authority, or the default authority. * @param authorityUrl */ - private async createSilentFlowClient(authorityUrl?: string): Promise { + private async createSilentFlowClient(serverTelemetryManager: ServerTelemetryManager, authorityUrl?: string): Promise { // Create auth module. - const clientConfig = await this.getClientConfiguration(authorityUrl); + const clientConfig = await this.getClientConfiguration(serverTelemetryManager, authorityUrl); return new SilentFlowClient(clientConfig); } @@ -557,7 +559,7 @@ export class PublicClientApplication implements IPublicClientApplication { * Creates a Client Configuration object with the given request authority, or the default authority. * @param requestAuthority */ - private async getClientConfiguration(requestAuthority?: string): Promise { + private async getClientConfiguration(serverTelemetryManager: ServerTelemetryManager, requestAuthority?: string): Promise { // If the requestAuthority is passed and is not equivalent to the default configured authority, create new authority and discover endpoints. Return default authority otherwise. const discoveredAuthority = (!StringUtils.isEmpty(requestAuthority) && requestAuthority !== this.config.auth.authority) ? await AuthorityFactory.createDiscoveredInstance(requestAuthority, this.config.system.networkClient) : await this.getDiscoveredDefaultAuthority(); @@ -578,6 +580,7 @@ export class PublicClientApplication implements IPublicClientApplication { cryptoInterface: this.browserCrypto, networkInterface: this.networkClient, storageInterface: this.browserStorage, + serverTelemetryManager: serverTelemetryManager, libraryInfo: { sku: BrowserConstants.MSAL_SKU, version: version, @@ -620,6 +623,17 @@ export class PublicClientApplication implements IPublicClientApplication { return validatedRequest; } + private initializeServerTelemetryManager(apiId: number, correlationId: string, forceRefresh?: boolean): ServerTelemetryManager { + const telemetryPayload: ServerTelemetryRequest = { + clientId: this.config.auth.clientId, + correlationId: correlationId, + apiId: apiId, + forceRefresh: forceRefresh || false + }; + + return new ServerTelemetryManager(telemetryPayload, this.browserStorage); + } + /** * Generates a request that will contain the openid and profile scopes. * @param request diff --git a/lib/msal-browser/src/interaction_handler/InteractionHandler.ts b/lib/msal-browser/src/interaction_handler/InteractionHandler.ts index d0ae42a6f8..ed1ba9cf1f 100644 --- a/lib/msal-browser/src/interaction_handler/InteractionHandler.ts +++ b/lib/msal-browser/src/interaction_handler/InteractionHandler.ts @@ -2,7 +2,7 @@ * Copyright (c) Microsoft Corporation. All rights reserved. * Licensed under the MIT License. */ -import { StringUtils, AuthorizationCodeRequest, CacheSchemaType, AuthenticationResult, AuthorizationCodeClient, ServerTelemetryManager } from "@azure/msal-common"; +import { StringUtils, AuthorizationCodeRequest, CacheSchemaType, AuthenticationResult, AuthorizationCodeClient } from "@azure/msal-common"; import { BrowserStorage } from "../cache/BrowserStorage"; import { BrowserAuthError } from "../error/BrowserAuthError"; import { TemporaryCacheKeys } from "../utils/BrowserConstants"; @@ -31,7 +31,7 @@ export abstract class InteractionHandler { * Function to handle response parameters from hash. * @param locationHash */ - async handleCodeResponse(locationHash: string, telemetryManager: ServerTelemetryManager): Promise { + async handleCodeResponse(locationHash: string): Promise { // Check that location hash isn't empty. if (StringUtils.isEmpty(locationHash)) { throw BrowserAuthError.createEmptyHashError(locationHash); diff --git a/lib/msal-browser/src/interaction_handler/RedirectHandler.ts b/lib/msal-browser/src/interaction_handler/RedirectHandler.ts index ea69e838be..b4ed660c22 100644 --- a/lib/msal-browser/src/interaction_handler/RedirectHandler.ts +++ b/lib/msal-browser/src/interaction_handler/RedirectHandler.ts @@ -2,7 +2,7 @@ * Copyright (c) Microsoft Corporation. All rights reserved. * Licensed under the MIT License. */ -import { StringUtils, AuthorizationCodeRequest, ICrypto, CacheSchemaType, AuthenticationResult, ServerTelemetryManager } from "@azure/msal-common"; +import { StringUtils, AuthorizationCodeRequest, ICrypto, CacheSchemaType, AuthenticationResult } from "@azure/msal-common"; import { InteractionHandler } from "./InteractionHandler"; import { BrowserAuthError } from "../error/BrowserAuthError"; import { BrowserConstants, TemporaryCacheKeys } from "../utils/BrowserConstants"; @@ -47,7 +47,7 @@ export class RedirectHandler extends InteractionHandler { * Handle authorization code response in the window. * @param hash */ - async handleCodeResponse(locationHash: string, telemetryManager: ServerTelemetryManager, browserCrypto?: ICrypto): Promise { + async handleCodeResponse(locationHash: string, browserCrypto?: ICrypto): Promise { // Check that location hash isn't empty. if (StringUtils.isEmpty(locationHash)) { throw BrowserAuthError.createEmptyHashError(locationHash); diff --git a/lib/msal-browser/test/app/PublicClientApplication.spec.ts b/lib/msal-browser/test/app/PublicClientApplication.spec.ts index bd877c0427..1a7fc56ce0 100644 --- a/lib/msal-browser/test/app/PublicClientApplication.spec.ts +++ b/lib/msal-browser/test/app/PublicClientApplication.spec.ts @@ -554,7 +554,7 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { } catch (e) { // Test that error was cached for telemetry purposes and then thrown expect(window.sessionStorage).to.be.length(1); - const failures = window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.server-telemetry`) + const failures = window.sessionStorage.getItem(`server-telemetry-${TEST_CONFIG.MSAL_CLIENT_ID}`); const failureObj = JSON.parse(failures) as ServerTelemetryCacheValue; expect(failureObj.failedRequests).to.be.length(2); expect(failureObj.failedRequests[0]).to.eq(ApiId.acquireTokenRedirect); @@ -759,7 +759,7 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { } catch (e) { // Test that error was cached for telemetry purposes and then thrown expect(window.sessionStorage).to.be.length(1); - const failures = window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.server-telemetry`) + const failures = window.sessionStorage.getItem(`server-telemetry-${TEST_CONFIG.MSAL_CLIENT_ID}`); const failureObj = JSON.parse(failures) as ServerTelemetryCacheValue; expect(failureObj.failedRequests).to.be.length(2); expect(failureObj.failedRequests[0]).to.eq(ApiId.acquireTokenRedirect); @@ -946,7 +946,7 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { } catch (e) { // Test that error was cached for telemetry purposes and then thrown expect(window.sessionStorage).to.be.length(1); - const failures = window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.server-telemetry`) + const failures = window.sessionStorage.getItem(`server-telemetry-${TEST_CONFIG.MSAL_CLIENT_ID}`); const failureObj = JSON.parse(failures) as ServerTelemetryCacheValue; expect(failureObj.failedRequests).to.be.length(2); expect(failureObj.failedRequests[0]).to.eq(ApiId.acquireTokenPopup); @@ -1046,7 +1046,7 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { } catch (e) { // Test that error was cached for telemetry purposes and then thrown expect(window.sessionStorage).to.be.length(1); - const failures = window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.server-telemetry`) + const failures = window.sessionStorage.getItem(`server-telemetry-${TEST_CONFIG.MSAL_CLIENT_ID}`); const failureObj = JSON.parse(failures) as ServerTelemetryCacheValue; expect(failureObj.failedRequests).to.be.length(2); expect(failureObj.failedRequests[0]).to.eq(ApiId.acquireTokenPopup); @@ -1216,7 +1216,7 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { } catch (e) { // Test that error was cached for telemetry purposes and then thrown expect(window.sessionStorage).to.be.length(1); - const failures = window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.server-telemetry`) + const failures = window.sessionStorage.getItem(`server-telemetry-${TEST_CONFIG.MSAL_CLIENT_ID}`); const failureObj = JSON.parse(failures) as ServerTelemetryCacheValue; expect(failureObj.failedRequests).to.be.length(2); expect(failureObj.failedRequests[0]).to.eq(ApiId.acquireTokenSilent_silentFlow); diff --git a/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts b/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts index de36a48409..6017acb425 100644 --- a/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts +++ b/lib/msal-browser/test/interaction_handler/RedirectHandler.spec.ts @@ -172,11 +172,11 @@ describe("RedirectHandler.ts Unit Tests", () => { describe("handleCodeResponse()", () => { it("throws error if given hash is empty", async () => { - await expect(redirectHandler.handleCodeResponse("", null)).to.be.rejectedWith(BrowserAuthErrorMessage.hashEmptyError.desc); - await expect(redirectHandler.handleCodeResponse("", null)).to.be.rejectedWith(BrowserAuthError); + await expect(redirectHandler.handleCodeResponse("")).to.be.rejectedWith(BrowserAuthErrorMessage.hashEmptyError.desc); + await expect(redirectHandler.handleCodeResponse("")).to.be.rejectedWith(BrowserAuthError); - await expect(redirectHandler.handleCodeResponse(null, null)).to.be.rejectedWith(BrowserAuthErrorMessage.hashEmptyError.desc); - await expect(redirectHandler.handleCodeResponse(null, null)).to.be.rejectedWith(BrowserAuthError); + await expect(redirectHandler.handleCodeResponse(null)).to.be.rejectedWith(BrowserAuthErrorMessage.hashEmptyError.desc); + await expect(redirectHandler.handleCodeResponse(null)).to.be.rejectedWith(BrowserAuthError); }); it("successfully handles response", async () => { @@ -222,7 +222,7 @@ describe("RedirectHandler.ts Unit Tests", () => { sinon.stub(AuthorizationCodeClient.prototype, "handleFragmentResponse").returns(testCodeResponse); sinon.stub(AuthorizationCodeClient.prototype, "acquireToken").resolves(testTokenResponse); - const tokenResponse = await redirectHandler.handleCodeResponse(TEST_HASHES.TEST_SUCCESS_CODE_HASH, undefined, browserCrypto); + const tokenResponse = await redirectHandler.handleCodeResponse(TEST_HASHES.TEST_SUCCESS_CODE_HASH, browserCrypto); expect(tokenResponse).to.deep.eq(testTokenResponse); expect(browserStorage.getItem(browserStorage.generateCacheKey(BrowserConstants.INTERACTION_STATUS_KEY), CacheSchemaType.TEMPORARY)).to.be.null; expect(browserStorage.getItem(browserStorage.generateCacheKey(TemporaryCacheKeys.URL_HASH), CacheSchemaType.TEMPORARY)).to.be.null; From c1e755d0d0e81b00d0cbd237ece45a7a6c8cf941 Mon Sep 17 00:00:00 2001 From: Thomas Norling Date: Thu, 30 Jul 2020 11:27:15 -0700 Subject: [PATCH 09/12] Get correlationId from cached request --- lib/msal-browser/src/app/PublicClientApplication.ts | 5 +++-- lib/msal-browser/src/interaction_handler/RedirectHandler.ts | 1 - 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/msal-browser/src/app/PublicClientApplication.ts b/lib/msal-browser/src/app/PublicClientApplication.ts index bf43dec69d..d0d4e3c248 100644 --- a/lib/msal-browser/src/app/PublicClientApplication.ts +++ b/lib/msal-browser/src/app/PublicClientApplication.ts @@ -185,8 +185,9 @@ export class PublicClientApplication implements IPublicClientApplication { return null; } - const correlationId = this.browserStorage.getItem(this.browserStorage.generateCacheKey(TemporaryCacheKeys.CORRELATION_ID), CacheSchemaType.TEMPORARY) as string; - const serverTelemetryManager = this.initializeServerTelemetryManager(ApiId.handleRedirectPromise, correlationId); + const requestState = this.browserStorage.getItem(this.browserStorage.generateCacheKey(TemporaryCacheKeys.REQUEST_STATE), CacheSchemaType.TEMPORARY) as string; + const cachedRequest = this.browserStorage.getCachedRequest(requestState, this.browserCrypto); + const serverTelemetryManager = this.initializeServerTelemetryManager(ApiId.handleRedirectPromise, cachedRequest.correlationId); try { // Hash contains known properties - handle and return in callback diff --git a/lib/msal-browser/src/interaction_handler/RedirectHandler.ts b/lib/msal-browser/src/interaction_handler/RedirectHandler.ts index b4ed660c22..f6edaa6dd0 100644 --- a/lib/msal-browser/src/interaction_handler/RedirectHandler.ts +++ b/lib/msal-browser/src/interaction_handler/RedirectHandler.ts @@ -24,7 +24,6 @@ export class RedirectHandler extends InteractionHandler { // Set interaction status in the library. this.browserStorage.setItem(this.browserStorage.generateCacheKey(BrowserConstants.INTERACTION_STATUS_KEY), BrowserConstants.INTERACTION_IN_PROGRESS_VALUE, CacheSchemaType.TEMPORARY); - this.browserStorage.setItem(this.browserStorage.generateCacheKey(TemporaryCacheKeys.CORRELATION_ID), authCodeRequest.correlationId, CacheSchemaType.TEMPORARY); this.browserStorage.cacheCodeRequest(authCodeRequest, browserCrypto); this.authModule.logger.infoPii("Navigate to:" + requestUrl); const isIframedApp = BrowserUtils.isInIframe(); From 76c3b735c972cbe6ba3cac810ea11ab71251d5e2 Mon Sep 17 00:00:00 2001 From: Thomas Norling Date: Mon, 3 Aug 2020 16:41:03 -0700 Subject: [PATCH 10/12] Remove leftover references to correlationId cache entry --- lib/msal-browser/src/cache/BrowserStorage.ts | 2 -- lib/msal-browser/src/utils/BrowserConstants.ts | 1 - 2 files changed, 3 deletions(-) diff --git a/lib/msal-browser/src/cache/BrowserStorage.ts b/lib/msal-browser/src/cache/BrowserStorage.ts index 6f6e67eb27..06d65039ea 100644 --- a/lib/msal-browser/src/cache/BrowserStorage.ts +++ b/lib/msal-browser/src/cache/BrowserStorage.ts @@ -279,7 +279,6 @@ export class BrowserStorage extends CacheManager { this.clearItemCookie(this.generateCacheKey(nonceKey)); this.clearItemCookie(this.generateCacheKey(TemporaryCacheKeys.REQUEST_STATE)); this.clearItemCookie(this.generateCacheKey(TemporaryCacheKeys.ORIGIN_URI)); - this.clearItemCookie(this.generateCacheKey(TemporaryCacheKeys.CORRELATION_ID)); } /** @@ -395,7 +394,6 @@ export class BrowserStorage extends CacheManager { this.removeItem(this.generateCacheKey(TemporaryCacheKeys.REQUEST_PARAMS)); this.removeItem(this.generateCacheKey(TemporaryCacheKeys.ORIGIN_URI)); this.removeItem(this.generateCacheKey(TemporaryCacheKeys.URL_HASH)); - this.removeItem(this.generateCacheKey(TemporaryCacheKeys.CORRELATION_ID)); } cleanRequest(): void { diff --git a/lib/msal-browser/src/utils/BrowserConstants.ts b/lib/msal-browser/src/utils/BrowserConstants.ts index 7284860340..89e9787e57 100644 --- a/lib/msal-browser/src/utils/BrowserConstants.ts +++ b/lib/msal-browser/src/utils/BrowserConstants.ts @@ -47,7 +47,6 @@ export enum TemporaryCacheKeys { REQUEST_STATE = "request.state", NONCE_IDTOKEN = "nonce.id_token", ORIGIN_URI = "request.origin", - CORRELATION_ID = "request.correlationId", RENEW_STATUS = "token.renew.status", URL_HASH = "urlHash", REQUEST_PARAMS = "request.params", From 9a4e4f18997a860556f334958ec9a4fb70b08e0d Mon Sep 17 00:00:00 2001 From: Thomas Norling Date: Mon, 3 Aug 2020 17:11:02 -0700 Subject: [PATCH 11/12] Update cachedRequest lookup --- lib/msal-browser/src/app/PublicClientApplication.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/msal-browser/src/app/PublicClientApplication.ts b/lib/msal-browser/src/app/PublicClientApplication.ts index 4885b195bd..b13e281517 100644 --- a/lib/msal-browser/src/app/PublicClientApplication.ts +++ b/lib/msal-browser/src/app/PublicClientApplication.ts @@ -185,8 +185,8 @@ export class PublicClientApplication implements IPublicClientApplication { return null; } - const requestState = this.browserStorage.getItem(this.browserStorage.generateCacheKey(TemporaryCacheKeys.REQUEST_STATE), CacheSchemaType.TEMPORARY) as string; - const cachedRequest = this.browserStorage.getCachedRequest(requestState, this.browserCrypto); + const encodedTokenRequest = this.browserStorage.getItem(this.browserStorage.generateCacheKey(TemporaryCacheKeys.REQUEST_PARAMS), CacheSchemaType.TEMPORARY) as string; + const cachedRequest = JSON.parse(this.browserCrypto.base64Decode(encodedTokenRequest)) as AuthorizationCodeRequest; const serverTelemetryManager = this.initializeServerTelemetryManager(ApiId.handleRedirectPromise, cachedRequest.correlationId); try { From 0e8f72b642a0c9de83f0a66d97c1f892f8376cdd Mon Sep 17 00:00:00 2001 From: Thomas Norling Date: Mon, 3 Aug 2020 17:27:56 -0700 Subject: [PATCH 12/12] Merge branch 'dev' of https://github.com/AzureAD/microsoft-authentication-library-for-js into msal-browser-MSER-Telemetry --- .../package-lock.json | 2 +- .../src/app/PublicClientApplication.ts | 106 ++++++++++++------ .../interaction_handler/RedirectHandler.ts | 4 +- .../src/utils/BrowserConstants.ts | 9 ++ .../src/utils/BrowserProtocolUtils.ts | 32 ++++++ lib/msal-browser/src/utils/BrowserUtils.ts | 9 ++ .../test/app/PublicClientApplication.spec.ts | 31 +++-- .../test/error/BrowserAuthError.spec.ts | 2 +- .../test/utils/BrowserProtocolUtils.spec.ts | 35 ++++++ .../test/utils/StringConstants.ts | 2 +- .../src/client/AuthorizationCodeClient.ts | 12 +- lib/msal-common/src/client/BaseClient.ts | 2 +- .../src/client/DeviceCodeClient.ts | 4 +- .../src/client/RefreshTokenClient.ts | 4 +- lib/msal-common/src/index.ts | 3 +- .../RequestParameterBuilder.ts | 4 +- .../src/response/AuthenticationResult.ts | 4 +- .../src/response/ResponseHandler.ts | 12 +- .../ServerAuthorizationCodeResponse.ts | 0 .../ServerAuthorizationTokenResponse.ts | 0 lib/msal-common/src/url/UrlString.ts | 33 +++--- lib/msal-common/src/utils/ProtocolUtils.ts | 18 ++- .../test/client/RefreshTokenClient.spec.ts | 3 +- .../test/client/SilentFlowClient.spec.ts | 3 +- .../RequestParameterBuilder.spec.ts | 2 +- .../test/response/ResponseHandler.spec.ts | 2 +- lib/msal-common/test/url/UrlString.spec.ts | 2 +- .../test/utils/ProtocolUtils.spec.ts | 28 ++--- 28 files changed, 254 insertions(+), 114 deletions(-) create mode 100644 lib/msal-browser/src/utils/BrowserProtocolUtils.ts create mode 100644 lib/msal-browser/test/utils/BrowserProtocolUtils.spec.ts rename lib/msal-common/src/{server => request}/RequestParameterBuilder.ts (98%) rename lib/msal-common/src/{server => response}/ServerAuthorizationCodeResponse.ts (100%) rename lib/msal-common/src/{server => response}/ServerAuthorizationTokenResponse.ts (100%) rename lib/msal-common/test/{server => request}/RequestParameterBuilder.spec.ts (98%) diff --git a/experimental/msal-react-native-android-poc/package-lock.json b/experimental/msal-react-native-android-poc/package-lock.json index d8168abbb7..9b95b3b22e 100644 --- a/experimental/msal-react-native-android-poc/package-lock.json +++ b/experimental/msal-react-native-android-poc/package-lock.json @@ -1,6 +1,6 @@ { "name": "@azuread/msal-react-native-android-poc", - "version": "1.0.0", + "version": "1.0.5", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/lib/msal-browser/src/app/PublicClientApplication.ts b/lib/msal-browser/src/app/PublicClientApplication.ts index b13e281517..f976ec360b 100644 --- a/lib/msal-browser/src/app/PublicClientApplication.ts +++ b/lib/msal-browser/src/app/PublicClientApplication.ts @@ -30,7 +30,8 @@ import { BaseAuthRequest, Logger, ServerTelemetryManager, - ServerTelemetryRequest + ServerTelemetryRequest, + ServerAuthorizationCodeResponse } from "@azure/msal-common"; import { buildConfiguration, Configuration } from "../config/Configuration"; import { BrowserStorage } from "../cache/BrowserStorage"; @@ -39,13 +40,14 @@ import { RedirectHandler } from "../interaction_handler/RedirectHandler"; import { PopupHandler } from "../interaction_handler/PopupHandler"; import { SilentHandler } from "../interaction_handler/SilentHandler"; import { BrowserAuthError } from "../error/BrowserAuthError"; -import { BrowserConstants, TemporaryCacheKeys, ApiId, DEFAULT_REQUEST } from "../utils/BrowserConstants"; +import { BrowserConstants, TemporaryCacheKeys, ApiId, DEFAULT_REQUEST, InteractionType } from "../utils/BrowserConstants"; import { BrowserUtils } from "../utils/BrowserUtils"; import { version } from "../../package.json"; import { IPublicClientApplication } from "./IPublicClientApplication"; import { RedirectRequest } from "../request/RedirectRequest"; import { PopupRequest } from "../request/PopupRequest"; import { SilentRequest } from "../request/SilentRequest"; +import { BrowserProtocolUtils, BrowserStateObject } from "../utils/BrowserProtocolUtils"; /** * The PublicClientApplication class is the object exposed by the library to perform authentication and authorization functions in Single Page Applications @@ -134,38 +136,41 @@ export class PublicClientApplication implements IPublicClientApplication { * - if true, performs logic to cache and navigate * - if false, handles hash string and parses response */ - private async handleRedirectResponse(): Promise { - // Get current location hash from window or cache. - const { location: { hash } } = window; - const cachedHash = this.browserStorage.getItem(this.browserStorage.generateCacheKey(TemporaryCacheKeys.URL_HASH), CacheSchemaType.TEMPORARY) as string; - const isResponseHash = UrlString.hashContainsKnownProperties(hash); - const loginRequestUrl = this.browserStorage.getItem(this.browserStorage.generateCacheKey(TemporaryCacheKeys.ORIGIN_URI), CacheSchemaType.TEMPORARY) as string; + private async handleRedirectResponse(): Promise { + const responseHash = this.getRedirectResponseHash(); + if (!responseHash) { + // Not a recognized server response hash or hash not associated with a redirect request + return null; + } - const currentUrlNormalized = UrlString.removeHashFromUrl(window.location.href); + // If navigateToLoginRequestUrl is true, get the url where the redirect request was initiated + const loginRequestUrl = this.browserStorage.getItem(this.browserStorage.generateCacheKey(TemporaryCacheKeys.ORIGIN_URI), CacheSchemaType.TEMPORARY) as string; const loginRequestUrlNormalized = UrlString.removeHashFromUrl(loginRequestUrl || ""); + const currentUrlNormalized = UrlString.removeHashFromUrl(window.location.href); + if (loginRequestUrlNormalized === currentUrlNormalized && this.config.auth.navigateToLoginRequestUrl) { + if (loginRequestUrl.indexOf("#") > -1) { + // Replace current hash with non-msal hash, if present + BrowserUtils.replaceHash(loginRequestUrl); + } // We are on the page we need to navigate to - handle hash - // Replace current hash with non-msal hash, if present - BrowserUtils.replaceHash(loginRequestUrl); - return this.handleHash(isResponseHash ? hash : cachedHash); - } - - if (!this.config.auth.navigateToLoginRequestUrl) { - // We don't need to navigate - handle hash - BrowserUtils.clearHash(); - return this.handleHash(isResponseHash ? hash : cachedHash); - } - - if (isResponseHash && !BrowserUtils.isInIframe()) { + return this.handleHash(responseHash); + } else if (!this.config.auth.navigateToLoginRequestUrl) { + return this.handleHash(responseHash); + } else if (!BrowserUtils.isInIframe()) { // Returned from authority using redirect - need to perform navigation before processing response + // Cache the hash to be retrieved after the next redirect const hashKey = this.browserStorage.generateCacheKey(TemporaryCacheKeys.URL_HASH); - this.browserStorage.setItem(hashKey, hash, CacheSchemaType.TEMPORARY); - if (StringUtils.isEmpty(loginRequestUrl) || loginRequestUrl === "null") { + this.browserStorage.setItem(hashKey, responseHash, CacheSchemaType.TEMPORARY); + if (!loginRequestUrl || loginRequestUrl === "null") { // Redirect to home page if login request url is null (real null or the string null) + const homepage = BrowserUtils.getHomepage(); + // Cache the homepage under ORIGIN_URI to ensure cached hash is processed on homepage + this.browserStorage.setItem(this.browserStorage.generateCacheKey(TemporaryCacheKeys.ORIGIN_URI), homepage, CacheSchemaType.TEMPORARY); this.logger.warning("Unable to get valid login request url from cache, redirecting to home page"); - BrowserUtils.navigateWindow("/", true); + BrowserUtils.navigateWindow(homepage, true); } else { - // Navigate to target url + // Navigate to page that initiated the redirect request BrowserUtils.navigateWindow(loginRequestUrl, true); } } @@ -173,6 +178,34 @@ export class PublicClientApplication implements IPublicClientApplication { return null; } + /** + * Gets the response hash for a redirect request + * Returns null if interactionType in the state value is not "redirect" or the hash does not contain known properties + * @returns {string} + */ + private getRedirectResponseHash(): string { + // Get current location hash from window or cache. + const { location: { hash } } = window; + const isResponseHash = UrlString.hashContainsKnownProperties(hash); + const cachedHash = this.browserStorage.getItem(this.browserStorage.generateCacheKey(TemporaryCacheKeys.URL_HASH), CacheSchemaType.TEMPORARY) as string; + this.browserStorage.removeItem(this.browserStorage.generateCacheKey(TemporaryCacheKeys.URL_HASH)); + + const responseHash = isResponseHash ? hash : cachedHash; + if (responseHash) { + // Deserialize hash fragment response parameters. + const serverParams: ServerAuthorizationCodeResponse = UrlString.getDeserializedHash(responseHash); + const platformStateObj: BrowserStateObject = BrowserProtocolUtils.extractBrowserRequestState(this.browserCrypto, serverParams.state); + if (platformStateObj.interactionType !== InteractionType.REDIRECT) { + return null; + } else { + BrowserUtils.clearHash(); + return responseHash; + } + } + + return null; + } + /** * Checks if hash exists and handles in window. * @param responseHash @@ -226,7 +259,7 @@ export class PublicClientApplication implements IPublicClientApplication { */ async acquireTokenRedirect(request: RedirectRequest): Promise { // Preflight request - const validRequest: AuthorizationUrlRequest = this.preflightInteractiveRequest(request); + const validRequest: AuthorizationUrlRequest = this.preflightInteractiveRequest(request, InteractionType.REDIRECT); const serverTelemetryManager = this.initializeServerTelemetryManager(ApiId.acquireTokenRedirect, validRequest.correlationId); try { @@ -275,7 +308,7 @@ export class PublicClientApplication implements IPublicClientApplication { */ async acquireTokenPopup(request: PopupRequest): Promise { // Preflight request - const validRequest: AuthorizationUrlRequest = this.preflightInteractiveRequest(request); + const validRequest: AuthorizationUrlRequest = this.preflightInteractiveRequest(request, InteractionType.POPUP); const serverTelemetryManager = this.initializeServerTelemetryManager(ApiId.acquireTokenPopup, validRequest.correlationId); try { @@ -349,7 +382,7 @@ export class PublicClientApplication implements IPublicClientApplication { const silentRequest: AuthorizationUrlRequest = this.initializeAuthorizationRequest({ ...request, prompt: PromptValue.NONE - }); + }, InteractionType.SILENT); const serverTelemetryManager = this.initializeServerTelemetryManager(ApiId.ssoSilent, silentRequest.correlationId); @@ -408,7 +441,7 @@ export class PublicClientApplication implements IPublicClientApplication { ...silentRequest, redirectUri: request.redirectUri, prompt: PromptValue.NONE - }); + }, InteractionType.SILENT); serverTelemetryManager = this.initializeServerTelemetryManager(ApiId.acquireTokenSilent_authCode, silentAuthUrlRequest.correlationId); try { @@ -594,7 +627,7 @@ export class PublicClientApplication implements IPublicClientApplication { /** * Helper to validate app environment before making a request. */ - private preflightInteractiveRequest(request: RedirectRequest|PopupRequest): AuthorizationUrlRequest { + private preflightInteractiveRequest(request: RedirectRequest|PopupRequest, interactionType: InteractionType): AuthorizationUrlRequest { // block the reload if it occurred inside a hidden iframe BrowserUtils.blockReloadInHiddenIframes(); @@ -603,7 +636,7 @@ export class PublicClientApplication implements IPublicClientApplication { throw BrowserAuthError.createInteractionInProgressError(); } - return this.initializeAuthorizationRequest(request); + return this.initializeAuthorizationRequest(request, interactionType); } /** @@ -650,7 +683,7 @@ export class PublicClientApplication implements IPublicClientApplication { * Helper to initialize required request parameters for interactive APIs and ssoSilent() * @param request */ - private initializeAuthorizationRequest(request: AuthorizationUrlRequest|RedirectRequest|PopupRequest): AuthorizationUrlRequest { + private initializeAuthorizationRequest(request: AuthorizationUrlRequest|RedirectRequest|PopupRequest, interactionType: InteractionType): AuthorizationUrlRequest { let validatedRequest: AuthorizationUrlRequest = { ...request }; @@ -670,15 +703,20 @@ export class PublicClientApplication implements IPublicClientApplication { } } + const browserState: BrowserStateObject = { + interactionType: interactionType + }; + validatedRequest.state = ProtocolUtils.setRequestState( + this.browserCrypto, (request && request.state) || "", - this.browserCrypto + browserState ); if (StringUtils.isEmpty(validatedRequest.nonce)) { validatedRequest.nonce = this.browserCrypto.createNewGuid(); } - + validatedRequest.responseMode = ResponseMode.FRAGMENT; validatedRequest = { diff --git a/lib/msal-browser/src/interaction_handler/RedirectHandler.ts b/lib/msal-browser/src/interaction_handler/RedirectHandler.ts index f6edaa6dd0..5cfa23e2d4 100644 --- a/lib/msal-browser/src/interaction_handler/RedirectHandler.ts +++ b/lib/msal-browser/src/interaction_handler/RedirectHandler.ts @@ -65,11 +65,9 @@ export class RedirectHandler extends InteractionHandler { this.authCodeRequest = this.browserStorage.getCachedRequest(requestState, browserCrypto); this.authCodeRequest.code = authCode; - // Hash was processed successfully - remove from cache - this.browserStorage.removeItem(this.browserStorage.generateCacheKey(TemporaryCacheKeys.URL_HASH)); - // Acquire token with retrieved code. const tokenResponse = await this.authModule.acquireToken(this.authCodeRequest, cachedNonce, requestState); + this.browserStorage.cleanRequest(); return tokenResponse; } diff --git a/lib/msal-browser/src/utils/BrowserConstants.ts b/lib/msal-browser/src/utils/BrowserConstants.ts index 89e9787e57..54d78160b3 100644 --- a/lib/msal-browser/src/utils/BrowserConstants.ts +++ b/lib/msal-browser/src/utils/BrowserConstants.ts @@ -68,6 +68,15 @@ export enum ApiId { acquireTokenSilent_silentFlow = 61 } +/* + * Interaction type of the API - used for state and telemetry + */ +export enum InteractionType { + REDIRECT = "redirect", + POPUP = "popup", + SILENT = "silent" +} + export const DEFAULT_REQUEST: AuthorizationUrlRequest = { scopes: [Constants.OPENID_SCOPE, Constants.PROFILE_SCOPE] }; diff --git a/lib/msal-browser/src/utils/BrowserProtocolUtils.ts b/lib/msal-browser/src/utils/BrowserProtocolUtils.ts new file mode 100644 index 0000000000..65fa825824 --- /dev/null +++ b/lib/msal-browser/src/utils/BrowserProtocolUtils.ts @@ -0,0 +1,32 @@ +/* + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. + */ + +import { InteractionType } from "./BrowserConstants"; +import { StringUtils, ClientAuthError, ICrypto, RequestStateObject, ProtocolUtils } from "@azure/msal-common"; + +export type BrowserStateObject = { + interactionType: InteractionType +}; + +export class BrowserProtocolUtils { + + /** + * Extracts the BrowserStateObject from the state string. + * @param browserCrypto + * @param state + */ + static extractBrowserRequestState(browserCrypto: ICrypto, state: string): BrowserStateObject { + if (StringUtils.isEmpty(state)) { + return null; + } + + try { + const requestStateObj: RequestStateObject = ProtocolUtils.parseRequestState(browserCrypto, state); + return requestStateObj.libraryState.meta as BrowserStateObject; + } catch (e) { + throw ClientAuthError.createInvalidStateError(state, e); + } + } +} diff --git a/lib/msal-browser/src/utils/BrowserUtils.ts b/lib/msal-browser/src/utils/BrowserUtils.ts index 6e8b403727..cc6510d3dc 100644 --- a/lib/msal-browser/src/utils/BrowserUtils.ts +++ b/lib/msal-browser/src/utils/BrowserUtils.ts @@ -60,6 +60,15 @@ export class BrowserUtils { return window.location.href.split("?")[0].split("#")[0]; } + /** + * Gets the homepage url for the current window location. + */ + static getHomepage(): string { + const currentUrl = new UrlString(window.location.href); + const urlComponents = currentUrl.getUrlComponents(); + return `${urlComponents.Protocol}//${urlComponents.HostNameAndPort}/`; + } + /** * Returns best compatible network client object. */ diff --git a/lib/msal-browser/test/app/PublicClientApplication.spec.ts b/lib/msal-browser/test/app/PublicClientApplication.spec.ts index c29eb08cbb..bf548dbd92 100644 --- a/lib/msal-browser/test/app/PublicClientApplication.spec.ts +++ b/lib/msal-browser/test/app/PublicClientApplication.spec.ts @@ -58,7 +58,7 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { done(); }); - it("navigates and caches hash if navigateToLoginRequestUri is true", (done) => { + it("navigates and caches hash if navigateToLoginRequestUri is true and interaction type is redirect", (done) => { window.location.hash = TEST_HASHES.TEST_SUCCESS_CODE_HASH; window.sessionStorage.setItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.ORIGIN_URI}`, TEST_URIS.TEST_ALTERNATE_REDIR_URI); sinon.stub(BrowserUtils, "navigateWindow").callsFake((urlNavigate: string, noHistory?: boolean) => { @@ -67,18 +67,19 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { done(); }); pca.handleRedirectPromise(); - expect(window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.URL_HASH}`)).to.be.eq(TEST_HASHES.TEST_SUCCESS_CODE_HASH); + expect(window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.URL_HASH}`)).to.be.eq(null); }); it("navigates to root and caches hash if navigateToLoginRequestUri is true", (done) => { window.location.hash = TEST_HASHES.TEST_SUCCESS_CODE_HASH; sinon.stub(BrowserUtils, "navigateWindow").callsFake((urlNavigate: string, noHistory?: boolean) => { expect(noHistory).to.be.true; - expect(urlNavigate).to.be.eq("/"); + expect(urlNavigate).to.be.eq("https://localhost:8081/"); + expect(window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.ORIGIN_URI}`)).to.be.eq("https://localhost:8081/"); done(); }); pca.handleRedirectPromise(); - expect(window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.URL_HASH}`)).to.be.eq(TEST_HASHES.TEST_SUCCESS_CODE_HASH); + expect(window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.URL_HASH}`)).to.be.eq(null); }); it("navigates to root and caches hash if navigateToLoginRequestUri is true and loginRequestUrl is 'null'", (done) => { @@ -86,11 +87,12 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { window.sessionStorage.setItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.ORIGIN_URI}`, "null"); sinon.stub(BrowserUtils, "navigateWindow").callsFake((urlNavigate: string, noHistory?: boolean) => { expect(noHistory).to.be.true; - expect(urlNavigate).to.be.eq("/"); + expect(urlNavigate).to.be.eq("https://localhost:8081/"); + expect(window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.ORIGIN_URI}`)).to.be.eq("https://localhost:8081/"); done(); }); pca.handleRedirectPromise(); - expect(window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.URL_HASH}`)).to.be.eq(TEST_HASHES.TEST_SUCCESS_CODE_HASH); + expect(window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.URL_HASH}`)).to.be.eq(null); }); it("navigates and caches hash if navigateToLoginRequestUri is true and loginRequestUrl contains query string", (done) => { @@ -103,7 +105,7 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { done(); }); pca.handleRedirectPromise(); - expect(window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.URL_HASH}`)).to.be.eq(TEST_HASHES.TEST_SUCCESS_CODE_HASH); + expect(window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.URL_HASH}`)).to.be.eq(null); }); it("navigates and caches hash if navigateToLoginRequestUri is true and loginRequestUrl contains query string and hash", (done) => { @@ -116,7 +118,7 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { done(); }); pca.handleRedirectPromise(); - expect(window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.URL_HASH}`)).to.be.eq(TEST_HASHES.TEST_SUCCESS_CODE_HASH); + expect(window.sessionStorage.getItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.URL_HASH}`)).to.be.eq(null); }); it("replaces custom hash if navigateToLoginRequestUri is true and loginRequestUrl contains custom hash", (done) => { @@ -131,6 +133,17 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { pca.handleRedirectPromise(); }); + it("processes hash if navigateToLoginRequestUri is true and loginRequestUrl contains trailing slash", (done) => { + const loginRequestUrl = window.location.href.endsWith('/') ? window.location.href.slice(0, -1) : window.location.href + "/"; + window.location.hash = TEST_HASHES.TEST_SUCCESS_CODE_HASH; + window.sessionStorage.setItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${TemporaryCacheKeys.ORIGIN_URI}`, loginRequestUrl); + sinon.stub(PublicClientApplication.prototype, "handleHash").callsFake((responseHash) => { + expect(responseHash).to.be.eq(TEST_HASHES.TEST_SUCCESS_CODE_HASH); + done(); + }); + pca.handleRedirectPromise(); + }); + it("clears hash if navigateToLoginRequestUri is false and loginRequestUrl contains custom hash", (done) => { pca = new PublicClientApplication({ auth: { @@ -484,7 +497,7 @@ describe("PublicClientApplication.ts Class Unit Tests", () => { sinon.stub(CryptoOps.prototype, "createNewGuid").returns(RANDOM_TEST_GUID); sinon.stub(TimeUtils, "nowSeconds").returns(TEST_STATE_VALUES.TEST_TIMESTAMP); - pca.loginRedirect(); + pca.loginRedirect(null); }); it("Updates cache entries correctly", async () => { diff --git a/lib/msal-browser/test/error/BrowserAuthError.spec.ts b/lib/msal-browser/test/error/BrowserAuthError.spec.ts index e3a8eb9637..322fb5cb96 100644 --- a/lib/msal-browser/test/error/BrowserAuthError.spec.ts +++ b/lib/msal-browser/test/error/BrowserAuthError.spec.ts @@ -166,7 +166,7 @@ describe("BrowserAuthError Unit Tests", () => { }); it("createMonitorWindowTimeoutError()", () => { - const err: BrowserAuthError = BrowserAuthError.createMonitorWindowTimeoutError("https://contoso.com/redirect"); + const err: BrowserAuthError = BrowserAuthError.createMonitorWindowTimeoutError(); expect(err instanceof BrowserAuthError).to.be.true; expect(err instanceof AuthError).to.be.true; diff --git a/lib/msal-browser/test/utils/BrowserProtocolUtils.spec.ts b/lib/msal-browser/test/utils/BrowserProtocolUtils.spec.ts new file mode 100644 index 0000000000..021ed9d31b --- /dev/null +++ b/lib/msal-browser/test/utils/BrowserProtocolUtils.spec.ts @@ -0,0 +1,35 @@ +import { expect } from "chai"; +import { BrowserProtocolUtils, BrowserStateObject } from "../../src/utils/BrowserProtocolUtils"; +import { InteractionType } from "../../src/utils/BrowserConstants"; +import { ICrypto, PkceCodes, ProtocolUtils } from "@azure/msal-common"; +import { RANDOM_TEST_GUID, TEST_CONFIG, TEST_STATE_VALUES } from "./StringConstants"; +import { BrowserCrypto } from "../../src/crypto/BrowserCrypto"; +import { CryptoOps } from "../../src/crypto/CryptoOps"; + +describe("BrowserProtocolUtils.ts Unit Tests", () => { + + const browserRedirectRequestState: BrowserStateObject = { interactionType: InteractionType.REDIRECT }; + const browserPopupRequestState: BrowserStateObject = { interactionType: InteractionType.POPUP }; + + let cryptoInterface: CryptoOps; + beforeEach(() => { + cryptoInterface = new CryptoOps(); + }); + + it("extractBrowserRequestState() returns an null if given interaction type is null or empty", () => { + const requestState1 = BrowserProtocolUtils.extractBrowserRequestState(cryptoInterface, null); + expect(requestState1).to.be.null; + + const requestState2 = BrowserProtocolUtils.extractBrowserRequestState(cryptoInterface, ""); + expect(requestState2).to.be.null; + }); + + it("extractBrowserRequestState() returns a valid platform state string", () => { + const redirectState = ProtocolUtils.setRequestState(cryptoInterface, null, browserRedirectRequestState); + const popupState = ProtocolUtils.setRequestState(cryptoInterface, null, browserPopupRequestState); + const redirectPlatformState = BrowserProtocolUtils.extractBrowserRequestState(cryptoInterface, redirectState); + expect(redirectPlatformState.interactionType).to.be.eq(InteractionType.REDIRECT); + const popupPlatformState = BrowserProtocolUtils.extractBrowserRequestState(cryptoInterface, popupState); + expect(popupPlatformState.interactionType).to.be.eq(InteractionType.POPUP); + }); +}); diff --git a/lib/msal-browser/test/utils/StringConstants.ts b/lib/msal-browser/test/utils/StringConstants.ts index 3b202b15cc..ee95ac9031 100644 --- a/lib/msal-browser/test/utils/StringConstants.ts +++ b/lib/msal-browser/test/utils/StringConstants.ts @@ -73,7 +73,7 @@ export const TEST_STATE_VALUES = { TEST_TIMESTAMP: 1592846482, DECODED_LIB_STATE: `{"id":"${RANDOM_TEST_GUID}","ts":1592846482}`, ENCODED_LIB_STATE: `eyJpZCI6IjExNTUzYTliLTcxMTYtNDhiMS05ZDQ4LWY2ZDRhOGZmODM3MSIsInRzIjoxNTkyODQ2NDgyfQ==`, - TEST_STATE: `eyJpZCI6IjExNTUzYTliLTcxMTYtNDhiMS05ZDQ4LWY2ZDRhOGZmODM3MSIsInRzIjoxNTkyODQ2NDgyfQ==${Constants.RESOURCE_DELIM}userState` + TEST_STATE: `eyJpZCI6IjExNTUzYTliLTcxMTYtNDhiMS05ZDQ4LWY2ZDRhOGZmODM3MSIsInRzIjoxNTkyODQ2NDgyLCJtZXRhIjp7ImludGVyYWN0aW9uVHlwZSI6InJlZGlyZWN0In19${Constants.RESOURCE_DELIM}userState` }; // Test Hashes diff --git a/lib/msal-common/src/client/AuthorizationCodeClient.ts b/lib/msal-common/src/client/AuthorizationCodeClient.ts index 8da2e6668b..f33902519d 100644 --- a/lib/msal-common/src/client/AuthorizationCodeClient.ts +++ b/lib/msal-common/src/client/AuthorizationCodeClient.ts @@ -7,10 +7,10 @@ import { BaseClient } from "./BaseClient"; import { AuthorizationUrlRequest } from "../request/AuthorizationUrlRequest"; import { AuthorizationCodeRequest } from "../request/AuthorizationCodeRequest"; import { Authority } from "../authority/Authority"; -import { RequestParameterBuilder } from "../server/RequestParameterBuilder"; +import { RequestParameterBuilder } from "../request/RequestParameterBuilder"; import { GrantType, AADServerParamKeys } from "../utils/Constants"; import { ClientConfiguration } from "../config/ClientConfiguration"; -import { ServerAuthorizationTokenResponse } from "../server/ServerAuthorizationTokenResponse"; +import { ServerAuthorizationTokenResponse } from "../response/ServerAuthorizationTokenResponse"; import { NetworkResponse } from "../network/NetworkManager"; import { ScopeSet } from "../request/ScopeSet"; import { ResponseHandler } from "../response/ResponseHandler"; @@ -18,7 +18,7 @@ import { AuthenticationResult } from "../response/AuthenticationResult"; import { StringUtils } from "../utils/StringUtils"; import { ClientAuthError } from "../error/ClientAuthError"; import { UrlString } from "../url/UrlString"; -import { ServerAuthorizationCodeResponse } from "../server/ServerAuthorizationCodeResponse"; +import { ServerAuthorizationCodeResponse } from "../response/ServerAuthorizationCodeResponse"; import { AccountEntity } from "../cache/entities/AccountEntity"; import { EndSessionRequest } from "../request/EndSessionRequest"; import { ClientConfigurationError } from "../error/ClientConfigurationError"; @@ -83,9 +83,11 @@ export class AuthorizationCodeClient extends BaseClient { handleFragmentResponse(hashFragment: string, cachedState: string): string { // Handle responses. const responseHandler = new ResponseHandler(this.config.authOptions.clientId, this.cacheManager, this.cryptoUtils, this.logger); - // Deserialize hash fragment response parameters. + + // Create UrlString object to remove leading # using getHash() const hashUrlString = new UrlString(hashFragment); - const serverParams = hashUrlString.getDeserializedHash(); + // Deserialize hash fragment response parameters. + const serverParams: ServerAuthorizationCodeResponse = UrlString.getDeserializedHash(hashUrlString.getHash()); // Get code response responseHandler.validateServerAuthorizationCodeResponse(serverParams, cachedState, this.cryptoUtils); diff --git a/lib/msal-common/src/client/BaseClient.ts b/lib/msal-common/src/client/BaseClient.ts index b160b889ca..685a748d65 100644 --- a/lib/msal-common/src/client/BaseClient.ts +++ b/lib/msal-common/src/client/BaseClient.ts @@ -10,7 +10,7 @@ import { Authority } from "../authority/Authority"; import { Logger } from "../logger/Logger"; import { AADServerParamKeys, Constants, HeaderNames } from "../utils/Constants"; import { NetworkResponse } from "../network/NetworkManager"; -import { ServerAuthorizationTokenResponse } from "../server/ServerAuthorizationTokenResponse"; +import { ServerAuthorizationTokenResponse } from "../response/ServerAuthorizationTokenResponse"; import { TrustedAuthority } from "../authority/TrustedAuthority"; import { CacheManager } from "../cache/CacheManager"; import { ServerTelemetryManager } from "../telemetry/server/ServerTelemetryManager"; diff --git a/lib/msal-common/src/client/DeviceCodeClient.ts b/lib/msal-common/src/client/DeviceCodeClient.ts index 97921a72bf..c05236298b 100644 --- a/lib/msal-common/src/client/DeviceCodeClient.ts +++ b/lib/msal-common/src/client/DeviceCodeClient.ts @@ -7,11 +7,11 @@ import { DeviceCodeResponse, ServerDeviceCodeResponse } from "../response/Device import { BaseClient } from "./BaseClient"; import { DeviceCodeRequest } from "../request/DeviceCodeRequest"; import { ClientAuthError } from "../error/ClientAuthError"; -import { RequestParameterBuilder } from "../server/RequestParameterBuilder"; +import { RequestParameterBuilder } from "../request/RequestParameterBuilder"; import { Constants, GrantType } from "../utils/Constants"; import { ClientConfiguration } from "../config/ClientConfiguration"; import { TimeUtils } from "../utils/TimeUtils"; -import { ServerAuthorizationTokenResponse } from "../server/ServerAuthorizationTokenResponse"; +import { ServerAuthorizationTokenResponse } from "../response/ServerAuthorizationTokenResponse"; import { ScopeSet } from "../request/ScopeSet"; import { ResponseHandler } from "../response/ResponseHandler"; import { AuthenticationResult } from "../response/AuthenticationResult"; diff --git a/lib/msal-common/src/client/RefreshTokenClient.ts b/lib/msal-common/src/client/RefreshTokenClient.ts index d2caed23c6..cc9bf7b706 100644 --- a/lib/msal-common/src/client/RefreshTokenClient.ts +++ b/lib/msal-common/src/client/RefreshTokenClient.ts @@ -7,8 +7,8 @@ import { ClientConfiguration } from "../config/ClientConfiguration"; import { BaseClient } from "./BaseClient"; import { RefreshTokenRequest } from "../request/RefreshTokenRequest"; import { Authority, NetworkResponse } from ".."; -import { ServerAuthorizationTokenResponse } from "../server/ServerAuthorizationTokenResponse"; -import { RequestParameterBuilder } from "../server/RequestParameterBuilder"; +import { ServerAuthorizationTokenResponse } from "../response/ServerAuthorizationTokenResponse"; +import { RequestParameterBuilder } from "../request/RequestParameterBuilder"; import { ScopeSet } from "../request/ScopeSet"; import { GrantType } from "../utils/Constants"; import { ResponseHandler } from "../response/ResponseHandler"; diff --git a/lib/msal-common/src/index.ts b/lib/msal-common/src/index.ts index d2fb10145a..2d16f4041b 100644 --- a/lib/msal-common/src/index.ts +++ b/lib/msal-common/src/index.ts @@ -40,6 +40,7 @@ export { SilentFlowRequest } from "./request/SilentFlowRequest"; export { DeviceCodeRequest } from "./request/DeviceCodeRequest"; export { EndSessionRequest } from "./request/EndSessionRequest"; export { AuthenticationResult } from "./response/AuthenticationResult"; +export { ServerAuthorizationCodeResponse } from "./response/ServerAuthorizationCodeResponse"; // Logger Callback export { ILoggerCallback, LogLevel, Logger } from "./logger/Logger"; // Errors @@ -52,7 +53,7 @@ export { ClientConfigurationError, ClientConfigurationErrorMessage } from "./err export { Constants, PromptValue, PersistentCacheKeys, ResponseMode, CacheSchemaType, CredentialType } from "./utils/Constants"; export { StringUtils } from "./utils/StringUtils"; export { StringDict } from "./utils/MsalTypes"; -export { ProtocolUtils } from "./utils/ProtocolUtils"; +export { ProtocolUtils, RequestStateObject, LibraryStateObject } from "./utils/ProtocolUtils"; export { TimeUtils } from "./utils/TimeUtils"; // Telemetry export { ServerTelemetryCacheValue } from "./telemetry/server/ServerTelemetryCacheValue"; diff --git a/lib/msal-common/src/server/RequestParameterBuilder.ts b/lib/msal-common/src/request/RequestParameterBuilder.ts similarity index 98% rename from lib/msal-common/src/server/RequestParameterBuilder.ts rename to lib/msal-common/src/request/RequestParameterBuilder.ts index a9fb111f7a..8d7d47e45c 100644 --- a/lib/msal-common/src/server/RequestParameterBuilder.ts +++ b/lib/msal-common/src/request/RequestParameterBuilder.ts @@ -4,10 +4,10 @@ */ import { AADServerParamKeys, Constants, ResponseMode, SSOTypes, ClientInfo } from "../utils/Constants"; -import { ScopeSet } from "../request/ScopeSet"; +import { ScopeSet } from "./ScopeSet"; import { ClientConfigurationError } from "../error/ClientConfigurationError"; import { StringDict } from "../utils/MsalTypes"; -import { RequestValidator } from "../request/RequestValidator"; +import { RequestValidator } from "./RequestValidator"; import { LibraryInfo } from "../config/ClientConfiguration"; import { StringUtils } from "../utils/StringUtils"; diff --git a/lib/msal-common/src/response/AuthenticationResult.ts b/lib/msal-common/src/response/AuthenticationResult.ts index d93ab5645b..853f980966 100644 --- a/lib/msal-common/src/response/AuthenticationResult.ts +++ b/lib/msal-common/src/response/AuthenticationResult.ts @@ -21,7 +21,7 @@ import { AccountInfo } from "../account/AccountInfo"; * - state - Value passed in by user in request * - familyId - Family ID identifier, usually only used for refresh tokens */ -export class AuthenticationResult { +export type AuthenticationResult = { uniqueId: string; tenantId: string; scopes: Array; @@ -34,4 +34,4 @@ export class AuthenticationResult { extExpiresOn?: Date; state?: string; familyId?: string; -} +}; diff --git a/lib/msal-common/src/response/ResponseHandler.ts b/lib/msal-common/src/response/ResponseHandler.ts index edf88b88a2..31969371a6 100644 --- a/lib/msal-common/src/response/ResponseHandler.ts +++ b/lib/msal-common/src/response/ResponseHandler.ts @@ -2,12 +2,12 @@ * Copyright (c) Microsoft Corporation. All rights reserved. * Licensed under the MIT License. */ -import { ServerAuthorizationTokenResponse } from "../server/ServerAuthorizationTokenResponse"; +import { ServerAuthorizationTokenResponse } from "./ServerAuthorizationTokenResponse"; import { buildClientInfo, ClientInfo } from "../account/ClientInfo"; import { ICrypto } from "../crypto/ICrypto"; import { ClientAuthError } from "../error/ClientAuthError"; import { StringUtils } from "../utils/StringUtils"; -import { ServerAuthorizationCodeResponse } from "../server/ServerAuthorizationCodeResponse"; +import { ServerAuthorizationCodeResponse } from "./ServerAuthorizationCodeResponse"; import { Logger } from "../logger/Logger"; import { ServerError } from "../error/ServerError"; import { IdToken } from "../account/IdToken"; @@ -112,13 +112,13 @@ export class ResponseHandler { // save the response tokens let requestStateObj: RequestStateObject = null; if (!StringUtils.isEmpty(cachedState)) { - requestStateObj = ProtocolUtils.parseRequestState(cachedState, this.cryptoObj); + requestStateObj = ProtocolUtils.parseRequestState(this.cryptoObj, cachedState); } const cacheRecord = this.generateCacheRecord(serverTokenResponse, idTokenObj, authority, requestStateObj && requestStateObj.libraryState); this.cacheStorage.saveCacheRecord(cacheRecord); - return ResponseHandler.generateAuthenticationResult(cacheRecord, idTokenObj, false, requestStateObj ? requestStateObj.userRequestState : null); + return ResponseHandler.generateAuthenticationResult(cacheRecord, idTokenObj, false, requestStateObj); } /** @@ -222,7 +222,7 @@ export class ResponseHandler { * @param fromTokenCache * @param stateString */ - static generateAuthenticationResult(cacheRecord: CacheRecord, idTokenObj: IdToken, fromTokenCache: boolean, stateString?: string): AuthenticationResult { + static generateAuthenticationResult(cacheRecord: CacheRecord, idTokenObj: IdToken, fromTokenCache: boolean, requestState?: RequestStateObject): AuthenticationResult { let accessToken: string = ""; let responseScopes: Array = []; let expiresOn: Date = null; @@ -249,7 +249,7 @@ export class ResponseHandler { expiresOn: expiresOn, extExpiresOn: extExpiresOn, familyId: familyId, - state: stateString || "" + state: requestState ? requestState.userRequestState : "" }; } } diff --git a/lib/msal-common/src/server/ServerAuthorizationCodeResponse.ts b/lib/msal-common/src/response/ServerAuthorizationCodeResponse.ts similarity index 100% rename from lib/msal-common/src/server/ServerAuthorizationCodeResponse.ts rename to lib/msal-common/src/response/ServerAuthorizationCodeResponse.ts diff --git a/lib/msal-common/src/server/ServerAuthorizationTokenResponse.ts b/lib/msal-common/src/response/ServerAuthorizationTokenResponse.ts similarity index 100% rename from lib/msal-common/src/server/ServerAuthorizationTokenResponse.ts rename to lib/msal-common/src/response/ServerAuthorizationTokenResponse.ts diff --git a/lib/msal-common/src/url/UrlString.ts b/lib/msal-common/src/url/UrlString.ts index 348c54d154..71aa5ef029 100644 --- a/lib/msal-common/src/url/UrlString.ts +++ b/lib/msal-common/src/url/UrlString.ts @@ -2,7 +2,7 @@ * Copyright (c) Microsoft Corporation. All rights reserved. * Licensed under the MIT License. */ -import { ServerAuthorizationCodeResponse } from "../server/ServerAuthorizationCodeResponse"; +import { ServerAuthorizationCodeResponse } from "../response/ServerAuthorizationCodeResponse"; import { ClientConfigurationError } from "../error/ClientConfigurationError"; import { ClientAuthError } from "../error/ClientAuthError"; import { StringUtils } from "../utils/StringUtils"; @@ -23,7 +23,7 @@ export class UrlString { constructor(url: string) { this._urlString = url; if (!StringUtils.isEmpty(this._urlString) && StringUtils.isEmpty(this.getHash())) { - this._urlString = this.canonicalizeUri(url); + this._urlString = UrlString.canonicalizeUri(url); } else if (StringUtils.isEmpty(this._urlString)) { // Throws error if url is empty throw ClientConfigurationError.createUrlEmptyError(); @@ -34,7 +34,7 @@ export class UrlString { * Ensure urls are lower case and end with a / character. * @param url */ - private canonicalizeUri(url: string): string { + static canonicalizeUri(url: string): string { if (url) { url = url.toLowerCase(); } @@ -87,7 +87,7 @@ export class UrlString { } static removeHashFromUrl(url: string): string { - return url.split("#")[0]; + return UrlString.canonicalizeUri(url.split("#")[0]); } /** @@ -118,18 +118,6 @@ export class UrlString { return ""; } - /** - * Returns deserialized portion of URL hash - */ - getDeserializedHash(): T { - const hash = this.getHash(); - const deserializedHash: T = StringUtils.queryStringToObject(hash); - if (!deserializedHash) { - throw ClientAuthError.createHashNotDeserializedError(JSON.stringify(deserializedHash)); - } - return deserializedHash; - } - /** * Parses out the components from a url string. * @returns An object with the various components. Please cache this value insted of calling this multiple times on the same url. @@ -161,6 +149,17 @@ export class UrlString { return new UrlString(urlObject.Protocol + "//" + urlObject.HostNameAndPort + "/" + urlObject.PathSegments.join("/")); } + /** + * Returns deserialized portion of URL hash + */ + static getDeserializedHash(hash: string): ServerAuthorizationCodeResponse { + const deserializedHash: ServerAuthorizationCodeResponse = StringUtils.queryStringToObject(hash); + if (!deserializedHash) { + throw ClientAuthError.createHashNotDeserializedError(JSON.stringify(deserializedHash)); + } + return deserializedHash; + } + /** * Check if the hash of the URL string contains known properties */ @@ -169,7 +168,7 @@ export class UrlString { return false; } const urlString = new UrlString(url); - const parameters = urlString.getDeserializedHash(); + const parameters: ServerAuthorizationCodeResponse = UrlString.getDeserializedHash(urlString.getHash()); return !!( parameters.code || parameters.error_description || diff --git a/lib/msal-common/src/utils/ProtocolUtils.ts b/lib/msal-common/src/utils/ProtocolUtils.ts index 6486c38d9e..6552817519 100644 --- a/lib/msal-common/src/utils/ProtocolUtils.ts +++ b/lib/msal-common/src/utils/ProtocolUtils.ts @@ -13,10 +13,12 @@ import { ClientAuthError } from "../error/ClientAuthError"; * Contains the following: * - id - unique identifier for this request * - ts - timestamp for the time the request was made. Used to ensure that token expiration is not calculated incorrectly. + * - platformState - string value sent from the platform. */ export type LibraryStateObject = { id: string, - ts: number + ts: number, + meta?: Record }; /** @@ -37,17 +39,17 @@ export class ProtocolUtils { * @param userState * @param randomGuid */ - static setRequestState(userState: string, cryptoObj: ICrypto): string { - const libraryState = ProtocolUtils.generateLibraryState(cryptoObj); + static setRequestState(cryptoObj: ICrypto, userState?: string, meta?: Record): string { + const libraryState = ProtocolUtils.generateLibraryState(cryptoObj, meta); return !StringUtils.isEmpty(userState) ? `${libraryState}${Constants.RESOURCE_DELIM}${userState}` : libraryState; } /** - * Generates the state value used by the library. + * Generates the state value used by the common library. * @param randomGuid * @param cryptoObj */ - static generateLibraryState(cryptoObj: ICrypto): string { + static generateLibraryState(cryptoObj: ICrypto, meta?: Record): string { if (!cryptoObj) { throw ClientAuthError.createNoCryptoObjectError("generateLibraryState"); } @@ -58,6 +60,10 @@ export class ProtocolUtils { ts: TimeUtils.nowSeconds() }; + if (meta) { + stateObj.meta = meta; + } + const stateString = JSON.stringify(stateObj); return cryptoObj.base64Encode(stateString); @@ -68,7 +74,7 @@ export class ProtocolUtils { * @param state * @param cryptoObj */ - static parseRequestState(state: string, cryptoObj: ICrypto): RequestStateObject { + static parseRequestState(cryptoObj: ICrypto, state: string): RequestStateObject { if (!cryptoObj) { throw ClientAuthError.createNoCryptoObjectError("parseRequestState"); } diff --git a/lib/msal-common/test/client/RefreshTokenClient.spec.ts b/lib/msal-common/test/client/RefreshTokenClient.spec.ts index 3d9e2226d0..5ba69d4ba3 100644 --- a/lib/msal-common/test/client/RefreshTokenClient.spec.ts +++ b/lib/msal-common/test/client/RefreshTokenClient.spec.ts @@ -62,8 +62,7 @@ describe("RefreshTokenClient unit tests", () => { const client = new RefreshTokenClient(config); const refreshTokenRequest: RefreshTokenRequest = { scopes: TEST_CONFIG.DEFAULT_GRAPH_SCOPE, - refreshToken: TEST_TOKENS.REFRESH_TOKEN, - redirectUri: TEST_URIS.TEST_REDIR_URI + refreshToken: TEST_TOKENS.REFRESH_TOKEN }; const authResult: AuthenticationResult = await client.acquireToken(refreshTokenRequest); diff --git a/lib/msal-common/test/client/SilentFlowClient.spec.ts b/lib/msal-common/test/client/SilentFlowClient.spec.ts index 87a09d7a1a..7f99577df2 100644 --- a/lib/msal-common/test/client/SilentFlowClient.spec.ts +++ b/lib/msal-common/test/client/SilentFlowClient.spec.ts @@ -168,8 +168,7 @@ describe("SilentFlowClient unit tests", () => { const silentFlowRequest: SilentFlowRequest = { scopes: TEST_CONFIG.DEFAULT_GRAPH_SCOPE, account: testAccount, - forceRefresh: true, - redirectUri: TEST_URIS.TEST_REDIR_URI + forceRefresh: true }; const authResult: AuthenticationResult = await client.acquireToken(silentFlowRequest); diff --git a/lib/msal-common/test/server/RequestParameterBuilder.spec.ts b/lib/msal-common/test/request/RequestParameterBuilder.spec.ts similarity index 98% rename from lib/msal-common/test/server/RequestParameterBuilder.spec.ts rename to lib/msal-common/test/request/RequestParameterBuilder.spec.ts index 1e8df39c08..52cca34621 100644 --- a/lib/msal-common/test/server/RequestParameterBuilder.spec.ts +++ b/lib/msal-common/test/request/RequestParameterBuilder.spec.ts @@ -6,7 +6,7 @@ import { TEST_TOKENS, DEVICE_CODE_RESPONSE } from "../utils/StringConstants"; -import { RequestParameterBuilder } from "../../src/server/RequestParameterBuilder"; +import { RequestParameterBuilder } from "../../src/request/RequestParameterBuilder"; import { ScopeSet } from "../../src/request/ScopeSet"; import { ClientConfigurationError } from "../../src"; diff --git a/lib/msal-common/test/response/ResponseHandler.spec.ts b/lib/msal-common/test/response/ResponseHandler.spec.ts index e31f68f53d..0bdf253736 100644 --- a/lib/msal-common/test/response/ResponseHandler.spec.ts +++ b/lib/msal-common/test/response/ResponseHandler.spec.ts @@ -1,7 +1,7 @@ import * as Mocha from "mocha"; import { expect } from "chai"; import sinon from "sinon"; -import { ServerAuthorizationTokenResponse } from "../../src/server/ServerAuthorizationTokenResponse"; +import { ServerAuthorizationTokenResponse } from "../../src/response/ServerAuthorizationTokenResponse"; import { ResponseHandler } from "../../src/response/ResponseHandler"; import { AUTHENTICATION_RESULT, RANDOM_TEST_GUID, TEST_CONFIG, ID_TOKEN_CLAIMS, TEST_DATA_CLIENT_INFO, TEST_STATE_VALUES } from "../utils/StringConstants"; import { Authority } from "../../src/authority/Authority"; diff --git a/lib/msal-common/test/url/UrlString.spec.ts b/lib/msal-common/test/url/UrlString.spec.ts index 28745ff1ce..99d94b65c2 100644 --- a/lib/msal-common/test/url/UrlString.spec.ts +++ b/lib/msal-common/test/url/UrlString.spec.ts @@ -123,7 +123,7 @@ describe("UrlString.ts Class Unit Tests", () => { const urlWithHash = TEST_URIS.TEST_AUTH_ENDPT + serializedHash; const urlObjWithHash = new UrlString(urlWithHash); - expect(urlObjWithHash.getDeserializedHash()).to.be.deep.eq(deserializedHash); + expect(UrlString.getDeserializedHash(urlObjWithHash.getHash())).to.be.deep.eq(deserializedHash); }); it("getUrlComponents returns all path components", () => { diff --git a/lib/msal-common/test/utils/ProtocolUtils.spec.ts b/lib/msal-common/test/utils/ProtocolUtils.spec.ts index 3cbf03ab65..c30ff20650 100644 --- a/lib/msal-common/test/utils/ProtocolUtils.spec.ts +++ b/lib/msal-common/test/utils/ProtocolUtils.spec.ts @@ -1,4 +1,4 @@ -import { expect, use } from "chai"; +import { expect } from "chai"; import { ProtocolUtils } from "../../src/utils/ProtocolUtils"; import { RANDOM_TEST_GUID, TEST_CONFIG } from "./StringConstants"; import { ICrypto, PkceCodes } from "../../src/crypto/ICrypto"; @@ -23,7 +23,7 @@ describe("ProtocolUtils.ts Class Unit Tests", () => { }, base64Decode(input: string): string { switch (input) { - case `eyJpZCI6IiR7UkFORE9NX1RFU1RfR1VJRH0iLCJ0cyI6JHt0ZXN0VGltZVN0YW1wfX0=`: + case encodedLibState: return decodedLibState; default: return input; @@ -32,7 +32,7 @@ describe("ProtocolUtils.ts Class Unit Tests", () => { base64Encode(input: string): string { switch (input) { case `${decodedLibState}`: - return "eyJpZCI6IiR7UkFORE9NX1RFU1RfR1VJRH0iLCJ0cyI6JHt0ZXN0VGltZVN0YW1wfX0="; + return encodedLibState; default: return input; } @@ -52,36 +52,36 @@ describe("ProtocolUtils.ts Class Unit Tests", () => { it("setRequestState() appends library state to given state", () => { sinon.stub(TimeUtils, "nowSeconds").returns(testTimeStamp); - const requestState = ProtocolUtils.setRequestState(userState, cryptoInterface); + const requestState = ProtocolUtils.setRequestState(cryptoInterface, userState); expect(requestState).to.be.eq(testState); }); it("setRequestState() only creates library state", () => { sinon.stub(TimeUtils, "nowSeconds").returns(testTimeStamp); - const requestState = ProtocolUtils.setRequestState("", cryptoInterface); + const requestState = ProtocolUtils.setRequestState(cryptoInterface, ""); expect(requestState).to.be.eq(encodedLibState); }); it("setRequestState throws error if no crypto object is passed to it", () => { - expect(() => ProtocolUtils.setRequestState(userState, null)).to.throw(ClientAuthError); - expect(() => ProtocolUtils.setRequestState(userState, null)).to.throw(ClientAuthErrorMessage.noCryptoObj.desc); - }) + expect(() => ProtocolUtils.setRequestState(null, userState)).to.throw(ClientAuthError); + expect(() => ProtocolUtils.setRequestState(null, userState)).to.throw(ClientAuthErrorMessage.noCryptoObj.desc); + }); it("parseRequestState() throws error if given state is null or empty", () => { - expect(() => ProtocolUtils.parseRequestState("", cryptoInterface)).to.throw(ClientAuthError); - expect(() => ProtocolUtils.parseRequestState("", cryptoInterface)).to.throw(ClientAuthErrorMessage.invalidStateError.desc); + expect(() => ProtocolUtils.parseRequestState(cryptoInterface, "")).to.throw(ClientAuthError); + expect(() => ProtocolUtils.parseRequestState(cryptoInterface, "")).to.throw(ClientAuthErrorMessage.invalidStateError.desc); - expect(() => ProtocolUtils.parseRequestState(null, cryptoInterface)).to.throw(ClientAuthError); - expect(() => ProtocolUtils.parseRequestState(null, cryptoInterface)).to.throw(ClientAuthErrorMessage.invalidStateError.desc); + expect(() => ProtocolUtils.parseRequestState(cryptoInterface, null)).to.throw(ClientAuthError); + expect(() => ProtocolUtils.parseRequestState(cryptoInterface, null)).to.throw(ClientAuthErrorMessage.invalidStateError.desc); }); it("parseRequestState() returns empty userRequestState if no resource delimiter found in state string", () => { - const requestState = ProtocolUtils.parseRequestState(decodedLibState, cryptoInterface); + const requestState = ProtocolUtils.parseRequestState(cryptoInterface, decodedLibState); expect(requestState.userRequestState).to.be.empty; }); it("parseRequestState() correctly splits the state by the resource delimiter", () => { - const requestState = ProtocolUtils.parseRequestState(testState, cryptoInterface); + const requestState = ProtocolUtils.parseRequestState(cryptoInterface, testState); expect(requestState.userRequestState).to.be.eq(userState); }); });