[msal-browser][msal-common] Instance Aware Authentication (updated) #1804

pkanher617 · 2020-06-19T07:33:57Z

This PR implements the instance_aware feature for msal-browser. Specifically, it adds the authority object to the response and handles all parameters which come back in the fragment.

It also adds TokenResponse to the browser exports (as per PR #1551).

Replaces PR #1584.

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

lib/msal-common/src/client/AuthorizationCodeClient.ts

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

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

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

lib/msal-common/src/response/AuthenticationResult.ts

samples/VanillaJSTestApp2.0/app/instance_aware/graph.js

github-actions · 2020-07-09T00:20:06Z

This PR has not seen activity in 14 days. It may be closed if it remains stale.

jasonnutter · 2020-08-24T18:18:21Z

@pkanher617 Status on this PR?

pkanher617 · 2020-08-27T18:55:47Z

@pkanher617 Status on this PR?

Need to update this to latest dev and follow up with proper caching schema. Will update in the next week.

github-actions · 2020-09-11T00:29:09Z

This PR has not seen activity in 14 days. It may be closed if it remains stale.

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

lib/msal-common/src/response/AuthorizationCodePayload.ts

tnorling · 2020-11-25T00:19:18Z

samples/msal-browser-samples/VanillaJSTestApp2.0/app/instanceAware/authConfig.js

+const loginRequest = {
+    scopes: ["User.Read"],
+    extraQueryParameters: {
+        "instance_aware": "true"


Does this need its own sample? Can we write a short doc instead?

Would prefer to have a sample that shows how to use the flow and the graph endpoints correctly. Happy to revisit if we have a lot of pushback on this.

I think we should write a doc (or update an existing one) for this regardless. The sample may or may not become redundant at that point

Will definitely update the docs to add the instance_aware flow

tnorling

Generally implementation looks ok, a few comments and would like to see unit tests. Also curious if the msal-common additions are type safe, given stricter type checking is imminent.

pkanher617 · 2020-11-25T00:49:21Z

Generally implementation looks ok, a few comments and would like to see unit tests.

Unit tests are updated, and I am adding more for the scenarios around instance_aware. Also going to add e2e tests if I can.

Also curious if the msal-common additions are type safe, given stricter type checking is imminent.

Would like to merge the strictNullChecks PR before I fully confirm this, but I think we should be fine.

coveralls · 2020-11-25T01:02:53Z

Coverage increased (+0.8%) to 82.312% when pulling f6e5e47 on instance-aware-2.0 into 7f3a24f on dev.

sameerag · 2020-11-25T18:10:58Z

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


        // Get cached items
        const nonceKey = this.browserStorage.generateNonceKey(requestState);
        const cachedNonce = this.browserStorage.getTemporaryCache(nonceKey);

        // Assign code to request
-        this.authCodeRequest.code = authCode;
+        this.authCodeRequest.code = authCodeResponse.code;


nit: Isn't the name confusing? I am assuming here authCodeResponse is from the fetch-auth-code request and authcodeRequest is the auth call for token?

Yes that's correct. If you have suggestions please suggest, but I think it is clear enough. The authCodeRequest is a field and the response is not.

sameerag · 2020-11-25T18:12:44Z

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

+
+    protected async updateTokenEndpointAuthority(cloudInstanceHostname: string, authority: Authority, networkModule: INetworkModule): Promise<void> {
+        const cloudInstanceAuthorityUri = `https://${cloudInstanceHostname}/${authority.tenant}/`;
+        if (cloudInstanceAuthorityUri !== authority.canonicalAuthority) {


Should we check aliases here? eg: login.windows.net is the same as login.microsoftonline.com and should not reinstantiate the authority instance in that case.

jasonnutter · 2020-12-01T22:14:55Z

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

        this.browserCrypto = browserCrypto;
    }

    /**
     * Redirects window to given URL.
     * @param urlNavigate
     */
-    initiateAuthRequest(requestUrl: string, authCodeRequest: AuthorizationCodeRequest, redirectTimeout: number, redirectStartPage?: string): Promise<void> {
+    initiateAuthRequest(requestUrl: string, authCodeRequest: AuthorizationCodeRequest, redirectTimeout?: number, redirectStartPage?: string): Promise<void> {


Why is redirectTimeout set to optional? FYI in #2669, I also made redirectStartPage required, since it will always be passed: https://github.com/AzureAD/microsoft-authentication-library-for-js/pull/2669/files#diff-d592fb8a2a52b3d36d8625f2210c27b8360c6d1f0cff4fe06494d1baf77ac593

Oh, is it because it now implements InteractionHandler?

Yeah that's correct. The interface implementation does not include redirectTimeout.

Can we remove the initiateAuthRequest abstract from InteractionHandler so that we dont have to make these optional?

I would like to keep the abstract function as a constraint for subclasses implementing this. I understand that there needs to be a compiler level way to ensure that the redirectTimeout and redirectStartPage are passed, but for now can we ensure this by adding code errors?

I personally don't like making a param optional but then throwing if it's not provided. Would prefer either making this its own function that doesn't implement the abstract or getting rid of the abstract altogether.

I personally think it's worse design to take out the abstract functionality than to check if an optional parameter is provided or not. Let me see if I can find an alternative, but in the meantime @tnorling can you approve the PR so I can merge as soon as it is ready?

I also don't like the idea of throwing an error if the params are not passed. I would rather the code just handle it gracefully.

So you can also just leave this as you have it, and I can deal with adjustments that need to be made for my PR.

made an update to the initiateAuthRequest function, please check and see if it works for you guys.

jasonnutter

One question otherwise looks good!

tnorling

Looks good, thanks for addressing feedback

tnorling · 2020-12-02T23:43:24Z

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

@@ -303,7 +303,10 @@ export abstract class ClientApplication {

            const redirectStartPage = (request && request.redirectStartPage) || window.location.href;
            // Show the UI once the url has been created. Response will come back in the hash, which will be handled in the handleRedirectCallback function.
-            return interactionHandler.initiateAuthRequest(navigateUrl, authCodeRequest, this.config.system.redirectNavigationTimeout, redirectStartPage);
+            return interactionHandler.initiateAuthRequest(navigateUrl, authCodeRequest, {


nit: Can we define the params above and make this a one liner?

tnorling · 2020-12-02T23:43:32Z

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

@@ -379,7 +382,9 @@ export abstract class ClientApplication {
            const interactionHandler = new PopupHandler(authClient, this.browserStorage);

            // Show the UI once the url has been created. Get the window handle for the popup.
-            const popupWindow: Window = interactionHandler.initiateAuthRequest(navigateUrl, authCodeRequest, popup);
+            const popupWindow: Window = interactionHandler.initiateAuthRequest(navigateUrl, authCodeRequest, {


lib/msal-common/src/response/AuthorizationCodePayload.ts

jmckennon

LGTM!

pkanher617 added 5 commits June 19, 2020 00:07

Adding code for instance_aware authentication in msal-browser

2600876

Adding graph hostname and graph host to the auth result

5307fa8

Adding AuthCodePayload object

710075d

Rename AuthCodeResponse file

d96595f

Update PublicClientApplication.ts

b2c6aa7

pkanher617 requested review from DarylThayil, jasonnutter, jmckennon, jo-arroyo, sameerag, hectormmg and tnorling as code owners June 19, 2020 07:33

github-actions bot added msal-browser Related to msal-browser package msal-common Related to msal-common package samples Related to the samples apps for the library. labels Jun 19, 2020

adding instance aware sample

53de8da

jasonnutter suggested changes Jun 20, 2020

View reviewed changes

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

lib/msal-common/src/client/AuthorizationCodeClient.ts Show resolved Hide resolved

tnorling reviewed Jun 22, 2020

View reviewed changes

pkanher617 changed the base branch from logout-request to dev June 24, 2020 17:25

github-actions bot added the no-pr-activity PR has been inactive for 14 days label Jul 9, 2020

github-actions bot removed the no-pr-activity PR has been inactive for 14 days label Aug 25, 2020

github-actions bot added the no-pr-activity PR has been inactive for 14 days label Sep 11, 2020

github-actions bot closed this Sep 18, 2020

tnorling reopened this Sep 18, 2020

tnorling added work-in-progress Issue or PR is not finished. and removed no-pr-activity PR has been inactive for 14 days labels Sep 18, 2020

tnorling reviewed Nov 25, 2020

View reviewed changes

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

tnorling reviewed Nov 25, 2020

View reviewed changes

lib/msal-common/src/response/AuthorizationCodePayload.ts Show resolved Hide resolved

tnorling reviewed Nov 25, 2020

View reviewed changes

tnorling requested changes Nov 25, 2020

View reviewed changes

All unit tests passing

1dd6b02

Prithvi Kanherkar added 3 commits November 24, 2020 17:50

Change files

3e616e1

fixing react TestContants.ts

5efff86

Change files

b921279

github-actions bot added the msal-react Related to @azure/msal-react label Nov 25, 2020

sameerag reviewed Nov 25, 2020

View reviewed changes

Prithvi Kanherkar added 3 commits November 25, 2020 16:13

Merge branch 'dev' into instance-aware-2.0

994e437

Merge branch 'dev' into instance-aware-2.0

20d2aad

Update ClientCredentialClient.ts

67fc08a

jasonnutter reviewed Dec 1, 2020

View reviewed changes

jasonnutter approved these changes Dec 1, 2020

View reviewed changes

Prithvi Kanherkar added 6 commits December 1, 2020 16:15

updating to add an alias check for authority on token endpt

e0e6e47

Merge branch 'dev' into instance-aware-2.0

f5ca958

fixing async functionality in interactionHandler

98cb427

Merge branch 'dev' into instance-aware-2.0

bc061ad

Update RedirectHandler.spec.ts

6d93f14

adding params for interactive requests

63ac001

tnorling approved these changes Dec 2, 2020

View reviewed changes

Prithvi Kanherkar added 3 commits December 2, 2020 17:00

adding docs for instance_aware

5bffa9b

Update ClientApplication.ts

c4cc99b

Update instance-aware.md

f6e5e47

jmckennon approved these changes Dec 3, 2020

View reviewed changes

pkanher617 merged commit c6a7aad into dev Dec 3, 2020

pkanher617 deleted the instance-aware-2.0 branch December 3, 2020 00:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[msal-browser][msal-common] Instance Aware Authentication (updated) #1804

[msal-browser][msal-common] Instance Aware Authentication (updated) #1804

pkanher617 commented Jun 19, 2020

github-actions bot commented Jul 9, 2020

jasonnutter commented Aug 24, 2020

pkanher617 commented Aug 27, 2020

github-actions bot commented Sep 11, 2020

tnorling Nov 25, 2020

pkanher617 Nov 25, 2020

tnorling Nov 25, 2020

pkanher617 Nov 25, 2020

tnorling left a comment

pkanher617 commented Nov 25, 2020 •

edited

Loading

coveralls commented Nov 25, 2020 •

edited

Loading

sameerag Nov 25, 2020

pkanher617 Dec 1, 2020

sameerag Nov 25, 2020

pkanher617 Dec 1, 2020

jasonnutter Dec 1, 2020

jasonnutter Dec 1, 2020

pkanher617 Dec 1, 2020

tnorling Dec 1, 2020

pkanher617 Dec 2, 2020

tnorling Dec 2, 2020

pkanher617 Dec 2, 2020 •

edited

Loading

jasonnutter Dec 2, 2020

jasonnutter Dec 2, 2020

pkanher617 Dec 2, 2020

jasonnutter left a comment

tnorling left a comment

tnorling Dec 2, 2020

pkanher617 Dec 3, 2020

tnorling Dec 2, 2020

pkanher617 Dec 3, 2020

jmckennon left a comment

[msal-browser][msal-common] Instance Aware Authentication (updated) #1804

[msal-browser][msal-common] Instance Aware Authentication (updated) #1804

Conversation

pkanher617 commented Jun 19, 2020

github-actions bot commented Jul 9, 2020

jasonnutter commented Aug 24, 2020

pkanher617 commented Aug 27, 2020

github-actions bot commented Sep 11, 2020

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

tnorling left a comment

Choose a reason for hiding this comment

pkanher617 commented Nov 25, 2020 • edited Loading

coveralls commented Nov 25, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkanher617 Dec 2, 2020 • edited Loading

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

jasonnutter left a comment

Choose a reason for hiding this comment

tnorling 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

jmckennon left a comment

Choose a reason for hiding this comment

pkanher617 commented Nov 25, 2020 •

edited

Loading

coveralls commented Nov 25, 2020 •

edited

Loading

pkanher617 Dec 2, 2020 •

edited

Loading