Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixing navigateToLoginRequestUrl in Auth Code Library #1320

Merged
merged 12 commits into from
Mar 3, 2020
Merged

Fixing navigateToLoginRequestUrl in Auth Code Library #1320

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3bc6554
Updating code to fix issues with navigateToLoginRequesturl
pkanher617 Feb 27, 2020
bee03b7
Merge branch 'authorization-code-flow-samples' into authorization-cod…
pkanher617 Feb 28, 2020
324ac65
Adding changes from comments
pkanher617 Mar 2, 2020
a3b4e39
change log to warning instead of error
pkanher617 Mar 2, 2020
a8dbe54
Update CacheHelpers.ts
pkanher617 Mar 2, 2020
b1bd0c9
update comment on navigatewindow
pkanher617 Mar 2, 2020
bab908a
Update Configuration.spec.ts
pkanher617 Mar 2, 2020
95bf760
Removing iframe check out of navigateWindow util, adding tests
pkanher617 Mar 3, 2020
a63fc74
Update PublicClientApplication.ts
pkanher617 Mar 3, 2020
4222e37
Update auth.js
pkanher617 Mar 3, 2020
c1d539a
Update rollup.config.js
pkanher617 Mar 3, 2020
53e6599
Adding tests for browser package
pkanher617 Mar 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 15 additions & 4 deletions lib/msal-browser/src/app/PublicClientApplication.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,19 @@ export class PublicClientApplication {
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;
const interactionHandler = new RedirectHandler(this.authModule, this.browserStorage);
let responseHash: string;
if (this.config.auth.navigateToLoginRequestUrl && UrlString.hashContainsKnownProperties(hash)) {
pkanher617 marked this conversation as resolved.
Show resolved Hide resolved
this.browserStorage.setItem(TemporaryCacheKeys.URL_HASH, hash);
interactionHandler.navigateToRequestUrl();
return;
} else if (!this.config.auth.navigateToLoginRequestUrl) {
responseHash = UrlString.hashContainsKnownProperties(hash) ? hash : cachedHash;
window.location.hash = "";
} else {
responseHash = cachedHash;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This block of code is confusing to understand, given the if/else if/else and mutation. From our discussion, each of these is for a different scenario, but it is hard to deduce that just from reading the code. My suggestions:

  1. Regardless of changes to syntax, add comments making clear why each part is happening.
  2. Avoid mutation if possible (not all mutation is bad, but I personally treat it as the exception, not the rule).
  3. Similar to redirect, I think there should be a helper function to modifying window.location.hash.
  4. else if not needed when previous block returns, removing it helps break up code.
  5. Refactor code as below:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed the if/else mutation and added some helpers instead. Please take a look.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let responseHash: string;
if (this.config.auth.navigateToLoginRequestUrl && UrlString.hashContainsKnownProperties(hash)) {
this.browserStorage.setItem(TemporaryCacheKeys.URL_HASH, hash);
interactionHandler.navigateToRequestUrl();
return;
} else if (!this.config.auth.navigateToLoginRequestUrl) {
responseHash = UrlString.hashContainsKnownProperties(hash) ? hash : cachedHash;
window.location.hash = "";
} else {
responseHash = cachedHash;
}
const validHash = UrlString.hashContainsKnownProperties(hash);
if (this.config.auth.navigateToLoginRequestUrl && validHash) {
this.browserStorage.setItem(TemporaryCacheKeys.URL_HASH, hash);
const loginRequestUrl = this.browserStorage.getItem(TemporaryCacheKeys.ORIGIN_URI);
interactionHandler.navigateToRequestUrl(loginRequestUrl);
return;
}
if (!this.config.auth.navigateToLoginRequestUrl) {
window.location.hash = "";
}
const responseHash = validHash ? hash : cachedHash;

Copy link
Member

Choose a reason for hiding this comment

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

Are we back porting (I believe only the syntax) to msal-core?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've done something similar to this - please take a look


if (responseHash) {
const tokenResponse = await interactionHandler.handleCodeResponse(responseHash);
this.authCallback(null, tokenResponse);
Expand Down Expand Up @@ -146,7 +157,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 +191,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
46 changes: 17 additions & 29 deletions lib/msal-browser/src/interaction_handler/RedirectHandler.ts
View file
Open in desktop
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 @@ -41,6 +32,20 @@ export class RedirectHandler extends InteractionHandler {
return window;
}

navigateToRequestUrl(): void {
if (window.parent === window) {
const loginRequestUrl = this.browserStorage.getItem(TemporaryCacheKeys.ORIGIN_URI);
pkanher617 marked this conversation as resolved.
Show resolved Hide resolved

// 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");
pkanher617 marked this conversation as resolved.
Show resolved Hide resolved
window.location.href = "/";
} else {
window.location.href = loginRequestUrl;
}
}
}

/**
* Handle authorization code response in the window.
* @param hash
Expand All @@ -51,29 +56,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);
jasonnutter marked this conversation as resolved.
Show resolved Hide resolved
// Acquire token with retrieved code.
return this.authModule.acquireToken(codeResponse);
}
Expand Down
4 changes: 2 additions & 2 deletions lib/msal-common/rollup.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export default [
{
file: "./lib/msal-common.js",
format: "umd",
name: "msal",
name: "msal-common",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these match the npm package name @azure/msal-common?

Copy link
Contributor Author

@pkanher617 pkanher617 Mar 2, 2020 •
edited

Choose a reason for hiding this comment

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

this is the name of the output from rollup (i.e. in the sample when you call import xyz from "@azure/msal-common"), I wanted to change it to differentiate between msal-browser output.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right...we saw issue in msal where there was a mismatch between the umd name and the package name.

banner: fileHeader,
sourcemap: "inline"
}
Expand All @@ -49,7 +49,7 @@ export default [
{
file: "./lib/msal-common.min.js",
format: "umd",
name: "msal",
name: "msal-common",
banner: useStrictHeader,
sourcemap: true
}
Expand Down
10 changes: 9 additions & 1 deletion lib/msal-common/src/cache/CacheHelpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,9 +165,17 @@ export class CacheHelpers {
});
}

/**
* Checks that any parameters are exact matches for key value, since key.match in the above functions only do contains checks, not exact matches.
* @param atKey
* @param clientId
* @param authority
* @param resource
* @param homeAccountIdentifier
*/
private checkForExactKeyMatch(atKey: AccessTokenKey, clientId: string, authority: string, resource?: string, homeAccountIdentifier?: string): boolean {
const hasClientId = (atKey.clientId === clientId);
const hasAuthorityUri = (atKey.authority === authority);
const hasAuthorityUri = !StringUtils.isEmpty(authority) ? (atKey.authority === authority) : true;
pkanher617 marked this conversation as resolved.
Show resolved Hide resolved
const hasResourceUri = !StringUtils.isEmpty(resource) ? (atKey.resource === resource) : true;
const hasHomeAccountId = !StringUtils.isEmpty(homeAccountIdentifier) ? (atKey.homeAccountIdentifier === homeAccountIdentifier) : true;

Expand Down