Skip to content

Conversation

@tnorling
Copy link
Collaborator

@tnorling tnorling commented May 1, 2020

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 and AAD authority classes into a single generic class

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

  1. Set validateAuthority: false
  2. Provide all trusted authorities they intend to use in knownAuthorities config option
  3. Provide instance metadata via authorityMetadata config option

Note: This does not include preferred_cache or preferred_network fields and does not include a scheduled refresh. These can be added in separate PRs if needed for core.

@tnorling tnorling added [email protected] work-in-progress Issue or PR is not finished. labels May 1, 2020
@tnorling tnorling mentioned this pull request May 15, 2020
@coveralls
Copy link

coveralls commented May 20, 2020

Coverage Status

Coverage decreased (-0.2%) to 76.627% when pulling 84dd6a2 on cloud-discovery into 8c86372 on dev.

@tnorling tnorling changed the title [WIP][msal-core] Use instance discovery for trusted hosts and combine authority classes [msal-core] Use instance discovery for trusted hosts and combine authority classes May 22, 2020
@tnorling tnorling marked this pull request as ready for review May 26, 2020 23:05
@tnorling tnorling added ready-for-review and removed work-in-progress Issue or PR is not finished. labels May 26, 2020
@tnorling tnorling removed angularjs msal-node Related to msal-node package labels Jun 3, 2020
@github-actions github-actions bot added angularjs msal-node Related to msal-node package labels Jun 3, 2020
@tnorling
Copy link
Collaborator Author

tnorling commented Jun 3, 2020

Per discussion: First invocation of resolveEndpointsAsync will call Discovery Endpoint if TrustedHostList is not already set.

@tnorling tnorling added ready-for-review and removed work-in-progress Issue or PR is not finished. angularjs msal-node Related to msal-node package labels Jun 3, 2020
@tnorling tnorling mentioned this pull request Jun 5, 2020
2 tasks
@tnorling tnorling added this to the [email protected] - Release milestone Jun 5, 2020
Copy link
Contributor

@jmckennon jmckennon left a comment

Choose a reason for hiding this comment

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

As a general thought, do we want to include logging in PRs going forward? Probably most applicable to the new network requests in this PR. Otherwise LGTM

Copy link
Contributor

@DarylThayil DarylThayil left a comment

Choose a reason for hiding this comment

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

Largely looks good to me, thanks for all the tests!

import { UrlUtils } from "../utils/UrlUtils";
import { TrustedAuthority } from "../authority/TrustedAuthority";

export const scrubTenantFromUri = (uri: string): String => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we create a utility class with static functions instead of exporting function constants?

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 think that's reasonable but since this PR isn't really changing any telemetry files other than to fix compilation, I think that can be taken separately.

@github-actions github-actions bot added angularjs msal-node Related to msal-node package labels Jun 15, 2020
@tnorling tnorling merged commit 504d352 into dev Jun 15, 2020
@tnorling tnorling deleted the cloud-discovery branch June 15, 2020 19:49
This was referenced Jun 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

msal-node Related to msal-node package samples Related to the samples apps for the library.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants