Skip to content

Instance Discovery 2.x #1756

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

Closed
wants to merge 37 commits into from
Closed

Conversation

tnorling
Copy link
Collaborator

@tnorling tnorling commented Jun 5, 2020

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 and ADFS only)
  2. Provide instance metadata via instanceMetadata config option (AAD only)

TODO:

  • Add a few more unit tests
  • Doc Updates

Scheduled Refresh will be taken in future PR

@tnorling tnorling added the work-in-progress Issue or PR is not finished. label Jun 5, 2020
@github-actions github-actions bot added the msal-common Related to msal-common package label Jun 5, 2020
@github-actions github-actions bot added msal-browser Related to msal-browser package msal-node Related to msal-node package labels Jun 18, 2020
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 76.656% when pulling 4aead0a on msal-common-instance-discovery into 25d53da on dev.

@tnorling tnorling changed the base branch from dev to instance-aware-2.0 June 19, 2020 23:06
@tnorling tnorling marked this pull request as ready for review June 19, 2020 23:42
@tnorling tnorling requested a review from DarylThayil as a code owner June 19, 2020 23:42
@tnorling tnorling removed the work-in-progress Issue or PR is not finished. label Jun 19, 2020
import { ClientConfigurationError } from "../error/ClientConfigurationError";

export class TrustedAuthority {
private static InstanceMetadata: InstanceMetadataType = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason to make these static? I would rather only make functions and fields static if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Things like knownAuthorities and the network interface can then be passed into the constructor and re-used

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I received feedback when I implemented knownAuthorities originally that it should be static. The idea is that it's a set once variable to prevent trying to switch up your knownAuthorities mid-stream so we don't want it to be tied to an instance where we may need to rebuild it.

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.

Looks good except for a couple things that we could change. Would also like to see a sample but it can be done later.

@tnorling
Copy link
Collaborator Author

Looks good except for a couple things that we could change. Would also like to see a sample but it can be done later.

Sample showing knownAuthorities in PR #1775
Will create a sample for instanceMetadata next week

@@ -14,6 +14,7 @@ export type BrowserAuthOptions = {
clientId: string;
authority?: string;
knownAuthorities?: Array<string>;
instanceMetadata?: Array<IInstanceDiscoveryMetadata>;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Naming here might get confusing when we implement the other perf changes which introduced authorityMetadata in msal-core. Open to suggestions for a better name here.

@tnorling tnorling changed the base branch from instance-aware-2.0 to logout-request June 22, 2020 20:57
@tnorling tnorling closed this Jun 22, 2020
@tnorling tnorling mentioned this pull request Jun 22, 2020
@tnorling tnorling deleted the msal-common-instance-discovery branch June 23, 2020 20:09
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.

3 participants