-
Notifications
You must be signed in to change notification settings - Fork 144
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
Demonstrate B2C features #4
Conversation
26f152d
to
69cb64f
Compare
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.
🕐
left a few comments and questions, but overall, i think it looks pretty good. i assume you've tested it, but am surprised it works w/localhost, unless that just rolled out and b2c didn't tell us. :) |
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 agree with @jennyf19 that for the moment, we'd better catch the reset password error: the context in case you don't know, @rayluo, @abhidnya13 @navyasric is when the user clicks on the reset password link when signing-in, this provokes an error that the app needs to manage by calling a policy. This is bad design from B2C, and they are fixing it, but that's the way it is today
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.
Only one of the suggested changes is a required change (removing the en-us locale identifier).
Co-Authored-By: Marsh Macy <[email protected]>
Thanks @mmacy for introducing me to the Github's Suggestion feature! I did not notice that handy tool before. #LearnSomethingNewEveryday :-D |
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. The user flow/policy issue is not a blocker for me.
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.
The web app in this repo was originally developed to demonstrate behaviors in non-B2C scenario.
With the current (
not-yet-merged) B2C implementation in MSAL Python (and its improvement), the MSAL Python API surface remains unchanged, so all those pre-existing behaviors just work in B2C. That means any existing MSAL-Python-powered web app can be used in B2C scenario without any source code adjustment. Just swap in a different set of configuration and it would work.In this PR, we did add several HTML links pointing to the B2C profile page and B2C reset password page. Such new adjustments all start with an
if config.get("B2C_something")
statement, so they are self-documented and won't be confused with non-B2C scenarios.TODO: Somehow update the README to describe how to provide a set of configurations for B2C.We now also provide a new README_B2C.md to show how to run this sample in the B2C context. This page is derived from the existing ASP.Net tutorial page for web app using B2C. It also follows the guidance to "decouple ...sign-in... from API access at runtime", even though this sample itself does not serve as a web API.
We also add a new
app_config_b2c.py
as a configuration template for B2C, so that it won't mess up with the pre-existing, non-B2Capp_config.py
. The new configuration file also follows the guidance to "use a string variable for tenant name (contosob2c), and construct full authority host and tenant names at runtime".PS: This is not part of this sample or PR, but it is the high level B2C how-to wiki page in MSAL Python wiki.