Fixing navigateToLoginRequestUrl in Auth Code Library #1320

pkanher617 · 2020-02-27T22:03:54Z

This PR fixes an issue with looping redirects during the navigateToLoginRequestUrl flow in the 2.0 version of the MSAL library.

It also changes the name of the msal-common export from msal to msal-common.

…e-flow-fixnavtologinrequesturl

jmckennon

Looks good to me, also like extracting out sets of logic into specific functions similar to navigateToRequestUrl here, makes it easier to test!

lib/msal-browser/src/interaction_handler/RedirectHandler.ts

lib/msal-browser/src/app/PublicClientApplication.ts

lib/msal-browser/src/interaction_handler/RedirectHandler.ts

jasonnutter · 2020-03-02T18:40:05Z

lib/msal-browser/src/app/PublicClientApplication.ts

+                window.location.hash = "";
+            } else {
+                responseHash = cachedHash;
+            }


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:

Regardless of changes to syntax, add comments making clear why each part is happening.

Avoid mutation if possible (not all mutation is bad, but I personally treat it as the exception, not the rule).

Similar to redirect, I think there should be a helper function to modifying window.location.hash.

else if not needed when previous block returns, removing it helps break up code.

Refactor code as below:

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

lib/msal-browser/src/interaction_handler/RedirectHandler.ts

jasonnutter · 2020-03-02T18:44:26Z

lib/msal-common/rollup.config.js

@@ -26,7 +26,7 @@ export default [
            {
                file: "./lib/msal-common.js",
                format: "umd",
-                name: "msal",
+                name: "msal-common",


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

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.

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

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

jasonnutter

Left a few comments.

jasonnutter · 2020-03-02T18:56:01Z

lib/msal-browser/src/app/PublicClientApplication.ts

+            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;
+            }


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;

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

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

refactored handleRedirectCallbacks, renamed it to better reflect the feature, added utils for navigating without history.

sameerag · 2020-03-02T21:10:47Z

lib/msal-browser/src/app/PublicClientApplication.ts

+            BrowserUtils.clearHash();
+            return this.handleHash(hash);
+        }
+    }


nitpick: comments before the if statements may make this more readable.

sameerag · 2020-03-02T21:11:35Z

lib/msal-browser/src/app/PublicClientApplication.ts

+     * - if true, performs logic to cache and navigate 
+     * - if false, handles hash string and parses response
+     */
+    private async handleRedirectResponse(): Promise<void> {


general comment: Are we back porting these to msal-core?

Adding @tnorling

Yes, I'm looking into a related issue in msal-core so some of these changes may be ported over but I'm not able to say which ones yet

sameerag

approved with some comments!

jasonnutter · 2020-03-02T21:13:01Z

lib/msal-browser/src/utils/BrowserUtils.ts


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


This can be done incrementally, but I think we should have an interface for navigating a browser window, since it will be different in Node, for example.

I think it's better to leave that entirely in the browser/node packages rather than exposing an interface in common. There is too much that goes into managing the UI that is platform specific.

Although I may just be overthinking the node vs browser implementation differences.

I was thinking generally about an interface for "I want to send the user to this url" and then have each platform implement how that actually happens.

Although the noHistory nuance does make that a little more complicated, I suppose.

lib/msal-browser/src/utils/BrowserUtils.ts

jmckennon

The changes to that if/elif/else are great! Looks good to me

lib/msal-browser/src/utils/BrowserUtils.ts

lib/msal-browser/src/app/PublicClientApplication.ts

Updating code to fix issues with navigateToLoginRequesturl

3bc6554

pkanher617 requested review from jasonnutter, DarylThayil, tnorling, jmckennon, sameerag and sangonzal February 27, 2020 22:03

pkanher617 mentioned this pull request Feb 27, 2020

Authorization Code Flow for Single-Page Applications: Samples and Docs Updates Round 1 #1321

Merged

Merge branch 'authorization-code-flow-samples' into authorization-cod…

bee03b7

…e-flow-fixnavtologinrequesturl

jmckennon approved these changes Feb 28, 2020

View reviewed changes

jasonnutter reviewed Feb 28, 2020

View reviewed changes

lib/msal-browser/src/interaction_handler/RedirectHandler.ts Show resolved Hide resolved

jmckennon reviewed Feb 28, 2020

View reviewed changes

lib/msal-browser/src/app/PublicClientApplication.ts Outdated Show resolved Hide resolved

jasonnutter reviewed Mar 2, 2020

View reviewed changes

lib/msal-browser/src/interaction_handler/RedirectHandler.ts Outdated Show resolved Hide resolved

jasonnutter reviewed Mar 2, 2020

View reviewed changes

lib/msal-browser/src/interaction_handler/RedirectHandler.ts Outdated Show resolved Hide resolved

jasonnutter reviewed Mar 2, 2020

View reviewed changes

lib/msal-common/src/cache/CacheHelpers.ts Outdated Show resolved Hide resolved

jasonnutter suggested changes Mar 2, 2020

View reviewed changes

jasonnutter reviewed Mar 2, 2020

View reviewed changes

pkanher617 added 3 commits March 2, 2020 13:02

Adding changes from comments

324ac65

refactored handleRedirectCallbacks, renamed it to better reflect the feature, added utils for navigating without history.

change log to warning instead of error

a3b4e39

Update CacheHelpers.ts

a8dbe54

sameerag reviewed Mar 2, 2020

View reviewed changes

sameerag approved these changes Mar 2, 2020

View reviewed changes

jasonnutter reviewed Mar 2, 2020

View reviewed changes

lib/msal-browser/src/utils/BrowserUtils.ts Show resolved Hide resolved

pkanher617 added 2 commits March 2, 2020 13:45

update comment on navigatewindow

b1bd0c9

Update Configuration.spec.ts

bab908a

jmckennon approved these changes Mar 2, 2020

View reviewed changes

jasonnutter reviewed Mar 3, 2020

View reviewed changes

lib/msal-browser/src/utils/BrowserUtils.ts Outdated Show resolved Hide resolved

jasonnutter reviewed Mar 3, 2020

View reviewed changes

lib/msal-browser/src/app/PublicClientApplication.ts Outdated Show resolved Hide resolved

pkanher617 added 5 commits March 3, 2020 09:46

Removing iframe check out of navigateWindow util, adding tests

95bf760

Update PublicClientApplication.ts

a63fc74

Update auth.js

4222e37

Update rollup.config.js

c1d539a

Adding tests for browser package

53e6599

pkanher617 merged commit 65683c7 into authorization-code-flow-samples Mar 3, 2020

pkanher617 deleted the authorization-code-flow-fixnavtologinrequesturl branch March 12, 2020 22:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing navigateToLoginRequestUrl in Auth Code Library #1320

Fixing navigateToLoginRequestUrl in Auth Code Library #1320

pkanher617 commented Feb 27, 2020

jmckennon left a comment

jasonnutter Mar 2, 2020 •

edited

pkanher617 Mar 2, 2020

jasonnutter Mar 2, 2020

pkanher617 Mar 2, 2020 •

edited

jasonnutter Mar 2, 2020

jasonnutter left a comment

jasonnutter Mar 2, 2020 •

edited

sameerag Mar 2, 2020

pkanher617 Mar 2, 2020

sameerag Mar 2, 2020

sameerag Mar 2, 2020

pkanher617 Mar 2, 2020

tnorling Mar 2, 2020

sameerag left a comment

jasonnutter Mar 2, 2020

pkanher617 Mar 2, 2020

pkanher617 Mar 2, 2020

jasonnutter Mar 2, 2020

jasonnutter Mar 2, 2020

jmckennon left a comment

Fixing navigateToLoginRequestUrl in Auth Code Library #1320

Fixing navigateToLoginRequestUrl in Auth Code Library #1320

Conversation

pkanher617 commented Feb 27, 2020

jmckennon left a comment

Choose a reason for hiding this comment

jasonnutter Mar 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkanher617 Mar 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonnutter left a comment

Choose a reason for hiding this comment

jasonnutter Mar 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sameerag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmckennon left a comment

Choose a reason for hiding this comment

jasonnutter Mar 2, 2020 •

edited

pkanher617 Mar 2, 2020 •

edited

jasonnutter Mar 2, 2020 •

edited