-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
[Identity] InteractiveBrowserCredential to use MSAL Browser 2.0 #13263
[Identity] InteractiveBrowserCredential to use MSAL Browser 2.0 #13263
Conversation
This comment has been minimized.
This comment has been minimized.
sdk/identity/identity/src/credentials/interactiveBrowserCredential.browser.ts
Outdated
Show resolved
Hide resolved
sdk/identity/identity/src/credentials/interactiveBrowserCredential.browser.ts
Outdated
Show resolved
Hide resolved
This looks okay to me. @sameerag - can you take a look (or tag someone on your side)? Would be good to get some eyes on this from the MSAL side. |
@jonathandturner -> @pkanher617 is from Sameera's side 🌞 I'll do some tests today to ensure I see more clearly all the moving parts, then I'll make this out of draft 👍 |
sdk/identity/identity/src/credentials/interactiveBrowserCredential.browser.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial review comments: looks good. Will approve once out of draft.
sdk/identity/identity/src/credentials/interactiveBrowserCredential.browser.ts
Outdated
Show resolved
Hide resolved
sdk/identity/identity/src/credentials/interactiveBrowserCredential.browser.ts
Outdated
Show resolved
Hide resolved
|
||
this.msalObject = new msalBrowser.PublicClientApplication(this.msalConfig); | ||
|
||
this.handleRedirectResponse(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important:
After redirect, the first time the user initializes any InteractiveBrowserCredential
is when we'll be able to authenticate based on the redirect response. To finish the authentication flow, we need the fully formed configuration object, which we generate based on the parameters to this credential. Is it safe to assume that the first initialization of InteractiveBrowserCredential
can be used to finish the MSAL redirect authentication flow?
An alternative I can think of is to store each set of new InteractiveBrowserCredential
parameters in something like localStorage
, then to cache the promise on page load based on the latest set of parameters, then on constructor initialization to match the promise based on parameters again.
My thought process is that I believe we can get away with asking the user to instantiate the InteractiveBrowserCredential
on page load for the redirect style. Is this a reasonable assumption?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe msal-browser
already addresses this. @pkanher617 to confirm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main issue is that if I have an app that has two instances of this credential, without this cache, the first one that is able to handle the redirect will get authenticated, the second one won't 🤔 I could be doing something wrong though.
Also, a separate problem perhaps:
Consider an application trying to authenticate two different accounts, how would one identify each individual redirect response? One would happen first, of course, then the other one would need to happen, but each time the page loads we lose track of the other credential. Does that make sense? I wonder if I'm doing something wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so we arrived at the following:
- We'll only allow one active account per client ID and tenant ID pair.
- If two or more clients try to authenticate with the same client ID and tenant ID, if the first succeeds authenticating, the others will be able to retrieve this same account, so there will be no further authentication attempts, and our limitation above will be respected: only one account per client ID and tenant ID.
- If the user happens to use MSAL-browser by their own account, and the storage is polluted, we might be able to get into two accounts. In that case, we will throw with an error, asking them to clear their browser cache.
I'll be asking @schaabs about this approach, but in the mean time, this is the update I'll be making to this PR.
Adding [DO NOT MERGE] since we're pondering to release a patch first from a stable master, then include CAE and MSAL 2.0 in a beta release. |
39ba458
to
f989b2c
Compare
/azp run js - core - ci |
Azure Pipelines successfully started running 1 pipeline(s). |
As part of Azure#13263 I added this link to the list of ignored links to make the build pass. I'm removing it now since it's not necessary anymore. Fixes Azure#13566
This PR is intended to fix #13155. I'll keep it as draft until @jonathandturner is able to do an initial review at least. I've tested a bit of it, but I'd like to run some more tests before I make it out of draft.
This PR switches from using the
msal
onInteractiveBrowserCredential
, which uses the implicit flow, to use@azure/msal-browser
2.0^, which uses the PKCE authentication flow.Reviews appreciated as always!
If you are curious on how I tested this, you can see my changes on this other PR:
#13308
The current state of this PR defaults on MSAL 2.0, but also allows users to go back to MSAL 1 by passing the
flow
property on the options bag to theInteractiveBrowserCredential
constructor.Fixes #13155
Fixes #13080