Skip to content

[msal-core] Fix circular dependency for B2C Authority #1479

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 1 commit into from
Apr 8, 2020

Conversation

tnorling
Copy link
Collaborator

@tnorling tnorling commented Apr 8, 2020

  • Moves B2CTrustedHostList to B2cAuthority class to fix circular dependency with AuthorityFactory

@tnorling tnorling requested review from jasonnutter and sameerag April 8, 2020 19:14
@coveralls
Copy link

Coverage Status

Coverage remained the same at 77.973% when pulling c9976d9 on fix-circular-dependency into 71b5f6a on dev.

@@ -6,7 +6,8 @@
import { Authority } from "./Authority";
import { AuthorityType } from "./Authority";
import { ClientConfigurationError } from "../error/ClientConfigurationError";
import { AuthorityFactory, B2CTrustedHostList } from "./AuthorityFactory";

export const B2CTrustedHostList: object = {};
Copy link
Member

Choose a reason for hiding this comment

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

So we want this to be a list or an array?? How are we parsing user i/p? You may have mentioned it, but I want to make sure we don't get type compat issues later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So in core I made this a dictionary to remain consistent with the AADTrustedHostList and to allow for flexibility to combine the AAD and B2C Authorities if that becomes the direction in the future. In msal-common this will be a list. The user facing knownAuthorities is a list in both core and common and setKnownAuthorities will populate the trusted host list slightly differently in both to account for the differences in type

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 agree this is a little confusing and I dont think dict is really the right type for either AAD or B2C but I think consistency is more important. If we want to change this for core we can discuss offline and open a separate PR

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. Please check the comments though once before we merge.

@tnorling tnorling merged commit 8298b9b into dev Apr 8, 2020
@mmacy mmacy mentioned this pull request Apr 9, 2020
@tnorling tnorling deleted the fix-circular-dependency branch April 13, 2020 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants