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.

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

Ignore account hints when prompt=select_account #3315

Merged
merged 10 commits into from
Mar 31, 2021
Merged

Conversation

tnorling
Copy link
Collaborator

@tnorling tnorling commented Mar 26, 2021

AAD throws an error when you pass both prompt=select_account and a login_hint or sid as these are conflicting. This becomes problematic when someone wants to use prompt=select_account to switch users and has previously used the setActiveAccount API in browser (we quietly pass down the active account if none is provided to the interactive API).

This PR ignores account hints if prompt=select_account to prevent this error from happening unexpectedly.

@tnorling tnorling added this to the @azure/msal-browser@2.13.1 milestone Mar 26, 2021
@github-actions github-actions bot added the msal-common Related to msal-common package label Mar 26, 2021
@coveralls
Copy link

coveralls commented Mar 26, 2021

Coverage Status

Coverage remained the same at 83.575% when pulling 757f87e on ignore-account-hints into 20779b8 on dev.

Copy link
Contributor

@jasonnutter jasonnutter left a comment

Choose a reason for hiding this comment

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

One question otherwise looks good!

@sameerag
Copy link
Member

The server mandates that sid/login_hint should be used only for prompt=none use cases. Did we remove that check?

@tnorling
Copy link
Collaborator Author

The server mandates that sid/login_hint should be used only for prompt=none use cases. Did we remove that check?

AFAIK this was never present in 2.x. This PR only sends sid when prompt=none, however, login_hint is valid for prompt=login so will only be ignored if prompt=select_account.

Copy link
Member

@sameerag sameerag left a comment

Choose a reason for hiding this comment

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

Approved. One thing: test cases to ignore sid for prompt!=none (if user provides it), please check that once.

@tnorling
Copy link
Collaborator Author

Approved. One thing: test cases to ignore sid for prompt!=none (if user provides it), please check that once.

Yes, it's there (test cases to use login_hint over sid if prompt != none covers this as login_hint has lower priority than sid)

@sameerag
Copy link
Member

Yes, it's there (test cases to use login_hint over sid if prompt != none covers this as login_hint has lower priority than sid)

I don't think we are testing this use case: Only sid and ignoring it if passed with prompt != none. This is different from making sid lower priority. Not blocking this PR, but this test case is critical to have around, in case something changes tomorrow.

@tnorling
Copy link
Collaborator Author

Yes, it's there (test cases to use login_hint over sid if prompt != none covers this as login_hint has lower priority than sid)

I don't think we are testing this use case: Only sid and ignoring it if passed with prompt != none. This is different from making sid lower priority. Not blocking this PR, but this test case is critical to have around, in case something changes tomorrow.

Gotcha, added!

@tnorling tnorling merged commit 2ab277f into dev Mar 31, 2021
@tnorling tnorling deleted the ignore-account-hints branch March 31, 2021 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
msal-common Related to msal-common package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants