Skip to content

[msal-common] ADFS 2019 support #1617

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

Merged
merged 9 commits into from
Jun 3, 2020
Merged

[msal-common] ADFS 2019 support #1617

merged 9 commits into from
Jun 3, 2020

Conversation

sangonzal
Copy link
Contributor

@sangonzal sangonzal commented May 7, 2020

Notes from taken from python implementation and email thread with AzureStack team:

  • ADFS does not have concept of tenant id. Instead, path will always contain "adfs", and should contain nothing aftewards. We should consider validating url to either throw error or format authority to delete everything after "adfs".
  • ADFS supports OpenId configuration endpoint, but not the instance discovery endpoint. OpenId configuration endpoint is formatted differently than AAD and B2C. (there is no 'v2' in path)
  • Only ADFS 2019 works with MSALs scope format. Earlier versions will not. This behavior must be enabled manually by customer. It's common that customers think that AFDS is on server 2019, but it might not actually have internally upgraded it's runitime to support the newer flow. To check if ADFS supports MSAL, check for "nbf" claim in OpenId configuration endpoint. We should consider checking for this claim and throwing an error if not found.

Cache:

  • Tenant id is null, and therefore HomeAccountId should just use oid (which is subject claim from IdToken)
  • Id token from ADFS 2019 contains "upn", but not preferred_username
  • Client info is not available from ADFS 2019 response

@sangonzal sangonzal changed the title ADFS 2019 support [msal-common] ADFS 2019 support May 7, 2020
@sangonzal sangonzal marked this pull request as ready for review May 12, 2020 16:36
@sangonzal
Copy link
Contributor Author

Two open issues for msal-browser:

  • Need to get ADFS 2019 to support CORS
  • browser's cache relies on ClientInfo returned in the response. Instead of updating current cache design, we will wait until it is moved to new cache.

@sangonzal sangonzal changed the base branch from sagonzal/msal-node-authority to dev May 15, 2020 16:48
@tnorling tnorling mentioned this pull request May 15, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Jun 1, 2020

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

@github-actions github-actions bot added the no-pr-activity PR has been inactive for 14 days label Jun 1, 2020
@sangonzal sangonzal requested a review from jmckennon as a code owner June 2, 2020 23:31
@github-actions 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 2, 2020
@coveralls
Copy link

coveralls commented Jun 2, 2020

Coverage Status

Coverage decreased (-0.2%) to 78.353% when pulling 2429e01 on sagonzal/msal-common-adfs into f6cc560 on dev.

@github-actions github-actions bot removed the no-pr-activity PR has been inactive for 14 days label Jun 3, 2020
@sangonzal sangonzal merged commit 955b2f9 into dev Jun 3, 2020
@sangonzal sangonzal deleted the sagonzal/msal-common-adfs branch July 17, 2020 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
msal-browser Related to msal-browser package msal-common Related to msal-common package samples Related to the samples apps for the library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants