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

Demonstrate B2C features #4

Merged
merged 10 commits into from
Nov 7, 2019
Merged

Demonstrate B2C features #4

merged 10 commits into from
Nov 7, 2019

Conversation

rayluo
Copy link
Contributor

@rayluo rayluo commented Oct 8, 2019

  • 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-B2C app_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.

@rayluo rayluo marked this pull request as ready for review October 31, 2019 22:24
app_config_b2c.py Outdated Show resolved Hide resolved
app_config_b2c.py Outdated Show resolved Hide resolved
README_B2C.md Outdated Show resolved Hide resolved
README_B2C.md Outdated Show resolved Hide resolved
README_B2C.md Outdated Show resolved Hide resolved
README_B2C.md Outdated Show resolved Hide resolved
README_B2C.md Outdated Show resolved Hide resolved
Copy link

@jennyf19 jennyf19 left a comment

Choose a reason for hiding this comment

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

🕐

@jennyf19
Copy link

jennyf19 commented Nov 1, 2019

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. :)

@jennyf19 jennyf19 requested review from jmprieur and valnav November 1, 2019 01:26
Copy link
Contributor

@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.

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

templates/index.html Outdated Show resolved Hide resolved
app.py Outdated Show resolved Hide resolved
app_config_b2c.py Show resolved Hide resolved
app_config_b2c.py Show resolved Hide resolved
Copy link
Contributor

@mmacy mmacy left a 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).

README_B2C.md Outdated Show resolved Hide resolved
README_B2C.md Outdated Show resolved Hide resolved
README_B2C.md Outdated Show resolved Hide resolved
README_B2C.md Outdated Show resolved Hide resolved
README_B2C.md Outdated Show resolved Hide resolved
README_B2C.md Outdated Show resolved Hide resolved
@rayluo
Copy link
Contributor Author

rayluo commented Nov 1, 2019

Thanks @mmacy for introducing me to the Github's Suggestion feature! I did not notice that handy tool before. #LearnSomethingNewEveryday :-D

Copy link
Contributor

@mmacy mmacy left a 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.

Copy link

@jennyf19 jennyf19 left a comment

Choose a reason for hiding this comment

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

:shipit:

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.

5 participants