Skip to content
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

B2c implementation with policy parameter #110

Closed
wants to merge 9 commits into from
Closed

Conversation

abhidnya13
Copy link
Contributor

No description provided.

Copy link

@jmprieur jmprieur left a comment

Choose a reason for hiding this comment

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

LGTM.
This looks much better

cc: @negoe @navyasric @jennyf19

Copy link
Collaborator

@rayluo rayluo left a comment

Choose a reason for hiding this comment

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

Thanks for this effort! Made some suggestions below.

@@ -73,7 +73,7 @@ def __init__(
client_credential=None, authority=None, validate_authority=True,
token_cache=None,
verify=True, proxies=None, timeout=None,
client_claims=None):
client_claims=None, trust_framework_policy=None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe the currently proposal is:

.WithAuthority(string authority, string trustFrameworkPolicy)

not

.WithAuthority(string authority) .WithTrustFrameworkPolicy(string trustFrameworkPolicy)

So the precise mapping would be just change the "shape" of the existing authority parameter, rather than introducing a new parameter for PCA.

Perhaps we want to have something like this (with the comparing to C# version, side-by-side)?

authority = {"authority": "https://domain/tenant", "policy": "B2C_1_SignIn"}
# .WithAuthority(string authority, string trustFrameworkPolicy)

And then the pca can be created like this:

pca = PublicClientApplication(
    ...,
    authority={"authority": "https://domain/tenant", "policy": "B2C_1_SignIn"},
    ...)

Copy link
Contributor

Choose a reason for hiding this comment

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

note to use userFlow instead of policy or trustFrameworkPolicy. thanks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then let's use snake_case user_flow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

msal/authority.py Show resolved Hide resolved
@@ -109,15 +96,19 @@ def canonicalize(authority_url):
% authority_url)
return authority, authority.netloc, parts[1]

def instance_discovery(url, **kwargs):
return requests.get( # Note: This URL seemingly returns V1 endpoint only
def instance_discovery(url, response=None, **kwargs):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since B2C does not do Instance Discovery at all, this PR should not need to change this helper.

Regardless of the B2C api interface, this helper was recently updated as part of an improvement for non-B2C scenarios. Now that change was lost, possibly caused by an improper merge between the dev branch and your local branch? We can fix that, by either cherry-pick those lost commits back, or it might actually be easier to branch off the latest dev, and then add the needed changes on top of it.

@rayluo
Copy link
Collaborator

rayluo commented Jan 14, 2020

(Cleaning up osbolete PRs) This PR is no longer needed, as we've moved to a different direction of not introducing user_flow in our API surface.

@rayluo rayluo closed this Jan 14, 2020
@abhidnya13 abhidnya13 deleted the b2c_implementation branch April 21, 2020 00:46
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