Skip to content

Commit

Permalink
Merge pull request #1320 from AzureAD/authorization-code-flow-fixnavt…
Browse files Browse the repository at this point in the history 
…ologinrequesturl

Fixing navigateToLoginRequestUrl in Auth Code Library
  • Loading branch information
Prithvi Kanherkar committed Mar 3, 2020
2 parents 72f1b48 + 53e6599 commit 65683c7
Show file tree
Hide file tree
Showing 21 changed files with 965 additions and 65 deletions.
5 changes: 5 additions & 0 deletions lib/msal-browser/package-lock.json
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

76 changes: 61 additions & 15 deletions lib/msal-browser/src/app/PublicClientApplication.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License.
*/
import { Account, AuthorizationCodeModule, AuthenticationParameters, INetworkModule, TokenResponse, UrlString, TemporaryCacheKeys, TokenRenewParameters } from "@azure/msal-common";
import { Account, AuthorizationCodeModule, AuthenticationParameters, INetworkModule, TokenResponse, UrlString, TemporaryCacheKeys, TokenRenewParameters, StringUtils } from "@azure/msal-common";
import { Configuration, buildConfiguration } from "./Configuration";
import { BrowserStorage } from "../cache/BrowserStorage";
import { CryptoOps } from "../crypto/CryptoOps";
Expand Down Expand Up @@ -92,8 +92,9 @@ export class PublicClientApplication {
// #region Redirect Flow

/**
* Set the callback functions for the redirect flow to send back the success or error object.
* IMPORTANT: Please do not use this function when using the popup APIs, as it will break the response handling
* Set the callback functions for the redirect flow to send back the success or error object, and process
* any redirect-related data.
* IMPORTANT: Please do not use this function when using the popup APIs, as it may break the response handling
* in the main window.
*
* @param {@link (AuthCallback:type)} authCallback - Callback which contains
Expand All @@ -109,21 +110,66 @@ export class PublicClientApplication {

// Set the callback object.
this.authCallback = authCallback;

// Check if we need to navigate, otherwise handle hash
try {
await this.handleRedirectResponse();
} catch (err) {
this.authCallback(err);
}
}

/**
* Checks if navigateToLoginRequestUrl is set, and:
* - if true, performs logic to cache and navigate
* - if false, handles hash string and parses response
*/
private async handleRedirectResponse(): Promise<void> {
// Get current location hash from window or cache.
const { location: { hash } } = window;
const cachedHash = this.browserStorage.getItem(TemporaryCacheKeys.URL_HASH);
try {
// If hash exists, handle in window. Otherwise, cancel any current requests and continue.
const interactionHandler = new RedirectHandler(this.authModule, this.browserStorage, this.config.auth.navigateToLoginRequestUrl);
const responseHash = UrlString.hashContainsKnownProperties(hash) ? hash : cachedHash;
if (responseHash) {
const tokenResponse = await interactionHandler.handleCodeResponse(responseHash);
this.authCallback(null, tokenResponse);

const isResponseHash = UrlString.hashContainsKnownProperties(hash);
if (this.config.auth.navigateToLoginRequestUrl && isResponseHash && !BrowserUtils.isInIframe()) {
// Returned from authority using redirect - need to perform navigation before processing response
this.browserStorage.setItem(TemporaryCacheKeys.URL_HASH, hash);
const loginRequestUrl = this.browserStorage.getItem(TemporaryCacheKeys.ORIGIN_URI);
if (StringUtils.isEmpty(loginRequestUrl) || loginRequestUrl === "null") {
// Redirect to home page if login request url is null (real null or the string null)
this.authModule.logger.warning("Unable to get valid login request url from cache, redirecting to home page");
BrowserUtils.navigateWindow("/", true);
} else {
this.cleanRequest();
BrowserUtils.navigateWindow(loginRequestUrl, true);
}
} catch (err) {
this.authCallback(err);
return;
}

if (!isResponseHash) {
// Loaded page with no valid hash - pass in the value retrieved from cache, or null/empty string
return this.handleHash(cachedHash);
}

if (!this.config.auth.navigateToLoginRequestUrl) {
// We don't need to navigate - check for hash and prepare to process
BrowserUtils.clearHash();
return this.handleHash(hash);
}
}

/**
* Checks if hash exists and handles in window. Otherwise, cancel any current requests and continue.
* @param responseHash
* @param interactionHandler
*/
private async handleHash(responseHash: string): Promise<void> {
const interactionHandler = new RedirectHandler(this.authModule, this.browserStorage);
if (!StringUtils.isEmpty(responseHash)) {
// Hash contains known properties - handle and return in callback
const tokenResponse = await interactionHandler.handleCodeResponse(responseHash);
this.authCallback(null, tokenResponse);
} else {
// There is no hash - assume we are in clean state and clear any current request data.
this.cleanRequest();
}
}

Expand All @@ -146,7 +192,7 @@ export class PublicClientApplication {

try {
// Create redirect interaction handler.
const interactionHandler = new RedirectHandler(this.authModule, this.browserStorage, this.config.auth.navigateToLoginRequestUrl);
const interactionHandler = new RedirectHandler(this.authModule, this.browserStorage);

// Create login url, which will by default append the client id scope to the call.
this.authModule.createLoginUrl(request).then((navigateUrl) => {
Expand Down Expand Up @@ -180,7 +226,7 @@ export class PublicClientApplication {

try {
// Create redirect interaction handler.
const interactionHandler = new RedirectHandler(this.authModule, this.browserStorage, this.config.auth.navigateToLoginRequestUrl);
const interactionHandler = new RedirectHandler(this.authModule, this.browserStorage);

// Create acquire token url.
this.authModule.createAcquireTokenUrl(request).then((navigateUrl) => {
Expand Down
8 changes: 4 additions & 4 deletions lib/msal-browser/src/cache/BrowserStorage.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ export class BrowserStorage implements ICacheStorage {
*/
private generateCacheKey(key: string): string {
try {
// Defined schemas do not need the key appended
// Defined schemas do not need the key migrated
this.validateObjectKey(key);
return key;
} catch (e) {
Expand Down Expand Up @@ -237,9 +237,9 @@ export class BrowserStorage implements ICacheStorage {
*/
clearMsalCookie(state?: string): void {
const nonceKey = state ? `${TemporaryCacheKeys.NONCE_IDTOKEN}|${state}` : TemporaryCacheKeys.NONCE_IDTOKEN;
this.clearItemCookie(nonceKey);
this.clearItemCookie(TemporaryCacheKeys.REQUEST_STATE);
this.clearItemCookie(TemporaryCacheKeys.ORIGIN_URI);
this.clearItemCookie(this.generateCacheKey(nonceKey));
this.clearItemCookie(this.generateCacheKey(TemporaryCacheKeys.REQUEST_STATE));
this.clearItemCookie(this.generateCacheKey(TemporaryCacheKeys.ORIGIN_URI));
}

/**
Expand Down
9 changes: 9 additions & 0 deletions lib/msal-browser/src/crypto/GuidGenerator.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,4 +82,13 @@ export class GuidGenerator {
return guidResponse;
}
}

/**
* verifies if a string is GUID
* @param guid
*/
static isGuid(guid: string) {
const regexGuid = /^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i;
return regexGuid.test(guid);
}
}
17 changes: 15 additions & 2 deletions lib/msal-browser/src/error/BrowserAuthError.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@ export const BrowserAuthErrorMessage = {
code: "popup_window_timeout",
desc: "Popup window token acquisition operation failed due to timeout."
},
redirectInIframeError: {
code: "redirect_in_iframe",
desc: "Code flow is not supported inside an iframe. Please ensure you are using MSAL.js in a top frame of the window if using the redirect APIs, or use the popup APIs."
}
};

/**
Expand Down Expand Up @@ -83,7 +87,7 @@ export class BrowserAuthError extends AuthError {
* @param errDetail
*/
static createCryptoNotAvailableError(errDetail: string): BrowserAuthError {
return new BrowserAuthError(BrowserAuthErrorMessage.cryptoDoesNotExist.code,
return new BrowserAuthError(BrowserAuthErrorMessage.cryptoDoesNotExist.code,
`${BrowserAuthErrorMessage.cryptoDoesNotExist.desc} Detail:${errDetail}`);
}

Expand Down Expand Up @@ -137,12 +141,21 @@ export class BrowserAuthError extends AuthError {
}

/**
* Creates an error thrown when the
* Creates an error thrown when monitorWindowFromHash times out for a given popup.
* @param urlNavigate
*/
static createPopupWindowTimeoutError(urlNavigate: string): BrowserAuthError {
const errorMessage = `URL navigated to is ${urlNavigate}, ${BrowserAuthErrorMessage.popupWindowTimeoutError.desc}`;
return new BrowserAuthError(BrowserAuthErrorMessage.popupWindowTimeoutError.code,
errorMessage);
}

/**
* Creates an error thrown when navigateWindow is called inside an iframe.
* @param windowParentCheck
*/
static createRedirectInIframeError(windowParentCheck: boolean): BrowserAuthError {
return new BrowserAuthError(BrowserAuthErrorMessage.redirectInIframeError.code,
`${BrowserAuthErrorMessage.redirectInIframeError.desc} (window.parent !== window) => ${windowParentCheck}`);
}
}
5 changes: 3 additions & 2 deletions lib/msal-browser/src/error/BrowserConfigurationAuthError.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export class BrowserConfigurationAuthError extends AuthError {

constructor(errorCode: string, errorMessage?: string) {
super(errorCode, errorMessage);
this.name = "BrowserAuthError";
this.name = "BrowserConfigurationAuthError";

Object.setPrototypeOf(this, BrowserConfigurationAuthError.prototype);
}
Expand All @@ -57,6 +57,7 @@ export class BrowserConfigurationAuthError extends AuthError {
* Creates error thrown when redirect callbacks are not set before calling loginRedirect() or acquireTokenRedirect().
*/
static createRedirectCallbacksNotSetError(): BrowserConfigurationAuthError {
return new BrowserConfigurationAuthError(BrowserConfigurationAuthErrorMessage.noRedirectCallbacksSet.code, BrowserConfigurationAuthErrorMessage.noRedirectCallbacksSet.desc);
return new BrowserConfigurationAuthError(BrowserConfigurationAuthErrorMessage.noRedirectCallbacksSet.code,
BrowserConfigurationAuthErrorMessage.noRedirectCallbacksSet.desc);
}
}
37 changes: 8 additions & 29 deletions lib/msal-browser/src/interaction_handler/RedirectHandler.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,14 @@
* Copyright (c) Microsoft Corporation. All rights reserved.
* Licensed under the MIT License.
*/
import { StringUtils, AuthorizationCodeModule, TemporaryCacheKeys, TokenResponse } from "@azure/msal-common";
import { StringUtils, TemporaryCacheKeys, TokenResponse } from "@azure/msal-common";
import { InteractionHandler } from "./InteractionHandler";
import { BrowserStorage } from "../cache/BrowserStorage";
import { BrowserAuthError } from "../error/BrowserAuthError";
import { BrowserConstants } from "../utils/BrowserConstants";
import { BrowserUtils } from "../utils/BrowserUtils";

export class RedirectHandler extends InteractionHandler {

// Config to navigate to login request url. Set by user, default is true.
private navigateToLoginRequestUrl: boolean;

constructor(authCodeModule: AuthorizationCodeModule, storageImpl: BrowserStorage, navigateToLoginRequestUrl: boolean) {
super(authCodeModule, storageImpl);
this.navigateToLoginRequestUrl = navigateToLoginRequestUrl;
}

/**
* Redirects window to given URL.
* @param urlNavigate
Expand All @@ -30,6 +21,11 @@ export class RedirectHandler extends InteractionHandler {
this.browserStorage.setItem(TemporaryCacheKeys.ORIGIN_URI, window.location.href);
this.browserStorage.setItem(BrowserConstants.INTERACTION_STATUS_KEY, BrowserConstants.INTERACTION_IN_PROGRESS_VALUE);
this.authModule.logger.infoPii("Navigate to:" + requestUrl);
const isIframedApp = BrowserUtils.isInIframe();
if (isIframedApp) {
// If we are not in top frame, we shouldn't redirect. This is also handled by the service.
throw BrowserAuthError.createRedirectInIframeError(isIframedApp);
}
// Navigate window to request URL
BrowserUtils.navigateWindow(requestUrl);
} else {
Expand All @@ -51,29 +47,12 @@ export class RedirectHandler extends InteractionHandler {
throw BrowserAuthError.createEmptyHashError(locationHash);
}

// If navigateToLoginRequestUrl is true, then cache the hash and navigate to cached request URI.
if (this.navigateToLoginRequestUrl) {
this.browserStorage.setItem(TemporaryCacheKeys.URL_HASH, locationHash);
if (window.parent === window) {
const loginRequestUrl = this.browserStorage.getItem(TemporaryCacheKeys.ORIGIN_URI);

// Redirect to home page if login request url is null (real null or the string null)
if (!loginRequestUrl || loginRequestUrl === "null") {
this.authModule.logger.error("Unable to get valid login request url from cache, redirecting to home page");
window.location.href = "/";
} else {
window.location.href = loginRequestUrl;
}
}
return null;
} else {
window.location.hash = "";
}

// Interaction is completed - remove interaction status.
this.browserStorage.removeItem(BrowserConstants.INTERACTION_STATUS_KEY);
// Handle code response.
const codeResponse = this.authModule.handleFragmentResponse(locationHash);
// Hash was processed successfully - remove from cache
this.browserStorage.removeItem(TemporaryCacheKeys.URL_HASH);
// Acquire token with retrieved code.
return this.authModule.acquireToken(codeResponse);
}
Expand Down
25 changes: 22 additions & 3 deletions lib/msal-browser/src/utils/BrowserUtils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,33 @@ import { XhrClient } from "../network/XhrClient";
*/
export class BrowserUtils {

// #region Window Navigation
// #region Window Navigation and URL management

/**
* Used to redirect the browser to the STS authorization endpoint
* @param {string} urlNavigate - URL of the authorization endpoint
* @param {boolean} noHistory - boolean flag, uses .replace() instead of .assign() if true
*/
static navigateWindow(urlNavigate: string): void {
window.location.assign(urlNavigate);
static navigateWindow(urlNavigate: string, noHistory?: boolean): void {
if (noHistory) {
window.location.replace(urlNavigate);
} else {
window.location.assign(urlNavigate);
}
}

/**
* Clears hash from window url.
*/
static clearHash(): void {
window.location.hash = "";
}

/**
* Returns boolean of whether the current window is in an iframe or not.
*/
static isInIframe(): boolean {
return window.parent !== window;
}

// #endregion
Expand Down
24 changes: 20 additions & 4 deletions lib/msal-browser/test/app/Configuration.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@ import { expect } from "chai";
import { Configuration, buildConfiguration } from "../../src/app/Configuration";
import { TEST_CONFIG, TEST_URIS } from "../utils/StringConstants";
import { LogLevel } from "@azure/msal-common";
import sinon from "sinon";

/**
* Defaults for the Configuration Options
*/
const DEFAULT_POPUP_TIMEOUT_MS = 60000;
const OFFSET = 300;

/**
* Test values for the Configuration Options
Expand All @@ -30,7 +30,6 @@ describe("Configuration.ts Class Unit Tests", () => {
expect(emptyConfig.auth.clientId).to.be.empty;
expect(emptyConfig.auth.tmp_clientSecret).to.be.empty;
expect(emptyConfig.auth.authority).to.be.null;
expect(emptyConfig.auth.validateAuthority).to.be.true;
let redirUriResult: string = emptyConfig.auth.redirectUri instanceof Function ? emptyConfig.auth.redirectUri() : emptyConfig.auth.redirectUri;
let postLogoutRediUriResult: string = emptyConfig.auth.postLogoutRedirectUri instanceof Function ? emptyConfig.auth.postLogoutRedirectUri() : emptyConfig.auth.postLogoutRedirectUri;
expect(redirUriResult).to.be.eq(TEST_URIS.TEST_REDIR_URI);
Expand All @@ -54,6 +53,25 @@ describe("Configuration.ts Class Unit Tests", () => {
expect(emptyConfig.system.telemetry).to.be.undefined;
});

it("Tests default logger", () => {
const consoleErrorSpy = sinon.spy(console, "error");
const consoleInfoSpy = sinon.spy(console, "info");
const consoleDebugSpy = sinon.spy(console, "debug");
const consoleWarnSpy = sinon.spy(console, "warn");
const message = "log message";
let emptyConfig: Configuration = buildConfiguration({auth: null});
emptyConfig.system.loggerOptions.loggerCallback(LogLevel.Error, message, true)
expect(consoleErrorSpy.called).to.be.false;
emptyConfig.system.loggerOptions.loggerCallback(LogLevel.Error, message, false)
expect(consoleErrorSpy.calledOnce).to.be.true;
emptyConfig.system.loggerOptions.loggerCallback(LogLevel.Info, message, false)
expect(consoleInfoSpy.calledOnce).to.be.true;
emptyConfig.system.loggerOptions.loggerCallback(LogLevel.Verbose, message, false)
expect(consoleDebugSpy.calledOnce).to.be.true;
emptyConfig.system.loggerOptions.loggerCallback(LogLevel.Warning, message, false)
expect(consoleWarnSpy.calledOnce).to.be.true;
});

const testAppName = "MSAL.js App";
const testAppVersion = "v1.0.0";
let testProtectedResourceMap = new Map<string, Array<string>>();
Expand All @@ -64,7 +82,6 @@ describe("Configuration.ts Class Unit Tests", () => {
clientId: TEST_CONFIG.MSAL_CLIENT_ID,
tmp_clientSecret: TEST_CONFIG.MSAL_CLIENT_SECRET,
authority: TEST_CONFIG.validAuthority,
validateAuthority: false,
redirectUri: TEST_URIS.TEST_ALTERNATE_REDIR_URI,
postLogoutRedirectUri: TEST_URIS.TEST_LOGOUT_URI,
navigateToLoginRequestUrl: false
Expand All @@ -91,7 +108,6 @@ describe("Configuration.ts Class Unit Tests", () => {
expect(newConfig.auth.clientId).to.be.eq(TEST_CONFIG.MSAL_CLIENT_ID);
expect(newConfig.auth.tmp_clientSecret).to.be.eq(TEST_CONFIG.MSAL_CLIENT_SECRET);
expect(newConfig.auth.authority).to.be.eq(TEST_CONFIG.validAuthority);
expect(newConfig.auth.validateAuthority).to.be.false;
expect(newConfig.auth.redirectUri).to.be.eq(TEST_URIS.TEST_ALTERNATE_REDIR_URI);
expect(newConfig.auth.postLogoutRedirectUri).to.be.eq(TEST_URIS.TEST_LOGOUT_URI);
expect(newConfig.auth.navigateToLoginRequestUrl).to.be.false;
Expand Down

0 comments on commit 65683c7

Please sign in to comment.