-
Notifications
You must be signed in to change notification settings - Fork 202
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
Conversation
There was a problem hiding this 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
There was a problem hiding this 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): |
There was a problem hiding this comment.
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"},
...)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -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): |
There was a problem hiding this comment.
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.
(Cleaning up osbolete PRs) This PR is no longer needed, as we've moved to a different direction of not introducing |
No description provided.