Skip to content

Instance Discovery 2.x #1811

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 58 commits into from
Jun 26, 2020
Merged

Instance Discovery 2.x #1811

merged 58 commits into from
Jun 26, 2020

Conversation

tnorling
Copy link
Collaborator

@tnorling tnorling commented Jun 22, 2020

Rebased from #1756. Ports #1582 into msal-common
Similar to the previous PR for msal-core this PR does several things in regards to Authorities:

  • Removes the hardcoded trusted host list for AAD hosts and replaces it with a network call to discover hosts known to MS. This is done the first time resolveEndpointsAsync is called.
  • Combines B2C, AAD and ADFS authority classes into a single generic class
  • Introduces preferred_cache and preferred_network values used to determine which endpoint to hit and what authority to cache under. This is the only piece that differs from the msal-core implementation.

Developers can opt out of the network call in one of 2 ways:

  1. Provide all trusted authorities they intend to use in knownAuthorities config option (B2C, ADFS and custom domains)
  2. Provide instance metadata via instanceMetadata config option (AAD only)

Scheduled Refresh will be taken in future PR

@github-actions github-actions bot added msal-browser Related to msal-browser package msal-common Related to msal-common package msal-node Related to msal-node package labels Jun 22, 2020
@tnorling tnorling changed the base branch from dev to logout-request June 22, 2020 21:20
@tnorling tnorling marked this pull request as ready for review June 22, 2020 21:21
@sameerag sameerag mentioned this pull request Jun 24, 2020
1 task
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.

LGTM. Approved with some comments. Please check while testing.

import { ClientConfigurationError } from "../error/ClientConfigurationError";

export class TrustedAuthority {
private static CloudDiscoveryMetadata: CloudDiscoveryMetadataType = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to make these static? can we make these all member functions of the prototype instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes because we never instantiate TrustedAuthority this variable should only be set once

Copy link
Contributor

@pkanher617 pkanher617 left a comment

Choose a reason for hiding this comment

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

Couple small changes but otherwise is ok to merge

@tnorling tnorling merged commit 960c135 into dev Jun 26, 2020
@tnorling tnorling deleted the msal-common-instance-discovery-update branch July 6, 2020 19:53
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 msal-node Related to msal-node package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants