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

Blazor Hybrid security content #25791

Merged
merged 22 commits into from
May 23, 2022
Merged

Blazor Hybrid security content #25791

merged 22 commits into from
May 23, 2022

Conversation

guardrex
Copy link
Collaborator

@guardrex guardrex commented May 3, 2022

Fixes #25771
Fixes #25854

Internal Review Topic

Notes

  • If this requires a lot of work, I recommend editing the PR directly over placing many suggestions on the diff. I'll circle around after your edit with a grammar/style pass.
  • Currently, I'm avoiding localization of "Web View" across the Hybrid docs with the inline markers :::no-loc text="Web View":::. I might move that to a global no-loc later. This is fine for now tho.
  • I removed underscores from the code examples per our earlier discussions. Underscores have been 'banished to the Land of Wind and Ghosts™.' —Ryan Nowak, 2019

❓ and 👂

  1. I'm not sure if the cross-links that I found are the best, if they all should be kept, and if they're in the best order.
  2. I couldn't find WinForms with AAD/AAD B2C guidance in WinForms, Azure, or MSAL docs. What should we do there?
  3. WRT the line I asked in the email about (Line 168 on the PR) ... "From anywhere in the app, we can resolve ..." I think we need to break that out into a few sentences and explain it out a bit more. For example, don't we need to add a bit that says that the code shown is for the developer's implementation for authenticating users using the identity provider's SDK?
  4. The code in the PR is now based on the sample that went up on 5/3 at https://github.com/javiercn/BlazorWpfApp/blob/main/BlazorWpfApp/MainWindow.xaml.cs, not based on the PU issue remarks. I had to guess on a few aspects (e.g., logout and null checks), so please 👁️ the bits.

UPDATE (5/4) via email

AFAICT, all of the following is already present on the PR via the initial content on the PU issue. The questions/concerns above ☝️ are still open for a little additional work.

I've created some samples with additional code that I had laying around. The main gist is BlazorWpfApp/MainWindow.xaml.cs at main · javiercn/BlazorWpfApp · GitHub

The main options are:

  • Do the auth and set Thread.CurentPrincipal
    • The provider just returns Thread.CurrentPrinicipal
    • You don't get updates when the user changes unless the component explicitly queries again, so it is best to do the auth before launching the webview.
  • Do the auth and set the results on an external service that blazor can consume (that's the AuthService in the doc).
    • Same deal, you are just propagating the external auth and everything is dealt with outside of blazor.
  • Trigger the auth from the webview.
    • In this case people extend AuthenticationStateProvider with methods to log in, etc. Trigger login from a login button within the UI, which does the auth flow and then triggers the update notification.

I think it might be interesting to add a section at the beginning that helps clarify the scenarios. If you only have one user through the entire app and that user is authenticated from the beginning and never changes, the one with Thread.CurrentPrincipal is the easiest option.

Unless you need to log/in and log/out users from the app and that must be driven by the webview (you want the login button on the Web UI) i would avoid the third point.

@guardrex guardrex requested a review from javiercn May 3, 2022 12:42
@guardrex guardrex mentioned this pull request May 3, 2022
25 tasks
@javiercn
Copy link
Member

javiercn commented May 3, 2022

@guardrex feel free to correct/rephrase things as you see fit. From my part this is mostly a braindump of content and didn't pay much attention to grammar/phrasing.

@javiercn
Copy link
Member

javiercn commented May 10, 2022

1. I'm not sure if the cross-links that I found are the best, if they all should be kept, and if they're in the best order.
2. I couldn't find WinForms with AAD/AAD B2C guidance in WinForms, Azure, or MSAL docs. What should we do there?

@danroth27 can you query some PMs on the identity side for docs/samples we can cross-reference to?

I've sent an email to see if we can get links to the recommended docs.

@javiercn
Copy link
Member

3. WRT the line I asked in the email about (Line 168 on the PR) ... "From anywhere in the app, we can resolve ..." I think we need to break that out into a few sentences and explain it out a bit more. For example, don't we need to add a bit that says that the code shown is for the developer's implementation for authenticating users using the identity provider's SDK?

I'm not sure how I would phrase this differently. The gist of the code is something as follows:

Before the Blazor application starts (this usually happens within the main window code behind (on the constructor).

var authenticatedUser = services.GetRequiredService<ExternalAuthService>();

// This is where the MSAL or similar code for authenticating the user goes.
ExternalAuthService.LogIn();

Somewhere https://github.com/javiercn/BlazorWpfApp/blob/main/BlazorWpfApp/MainWindow.xaml.cs after you've built the service provider.

@guardrex guardrex self-assigned this May 10, 2022
@guardrex guardrex marked this pull request as draft May 10, 2022 16:08
@guardrex
Copy link
Collaborator Author

guardrex commented May 10, 2022

I need to apply a few minor updates to this, which I'll apply on Wednesday morning.

I'm marking this DRAFT for the evening.

UPDATE (5/11): Too many updates this morning to merge immediately. I need to 🛌💤 on this one more night and make final updates on Thursday morning.

@guardrex guardrex marked this pull request as ready for review May 12, 2022 10:20
@guardrex
Copy link
Collaborator Author

guardrex commented May 12, 2022

@javiercn ... A couple of updates that you may want to inspect ...

  • Headings: I didn't care for the initial heading levels because we can only show H2 level headings in the topic's sidebar ToC, so I refactored them so that these primary subject sections are H2 level.
  • Code examples: YOU probably won't hear about it; but if we only publish WPF code, I think the MAUI and WinForms readers will come for me with sharpened 🔪😨. I've sought to flesh out the examples for MAUI, WPF, and WinForms. I'm not familiar with the frameworks (yet). Although the code that I've placed on the PR builds, I can't say if it will run (yet). I've instituted pivots so that readers can focus on the code for the framework that they're using.
  • Cross-links: I'm on stand-by to hear back from DR. I have the one link for MAUI, the WPF links that I found, and NO links for WinForms.

@guardrex guardrex marked this pull request as draft May 13, 2022 13:10
@guardrex
Copy link
Collaborator Author

Placing this back on DRAFT in order to fix #25854 on this PR.

@guardrex
Copy link
Collaborator Author

guardrex commented May 14, 2022

UPDATE (5/14 10:30am CST): The last commit places the new content at the top. I place the content in a tentative Overview section. I then pick up with the rest of the content (the next bit is on integration) in a tentative Integrate authentication section.

This is a rough DRAFT at this point for the new content. There are likely phrasing and organizational issues. I'm going to 🛌💤 on the new content Saturday night and give it another pass on Sunday morning. I'll provide an update here on Sunday after that pass, then you can look at the new text in detail. I don't recommend looking at it until after the next pass. 🙈

Today, Saturday, if you just wish to comment on the overall layout and moving bits into new topics under this node, identify the sections for new topics and the new topic titles and then I'll take it from there on Sunday.

General update:

Cross-ref tracking issue:

Blazor Hybrid tracking
#24956

@guardrex
Copy link
Collaborator Author

guardrex commented May 15, 2022

UPDATE (5/15 6:30am CST): New commits this morning.

Yes, my confusion resulted from your use of "The title" and "the content" for the live topic. When u made that remark, everything was on the PR that would've filled in the authn/z pieces making the title more relevant. The original notion for the title of the overview is that the title should match the naming of the other security overviews around the docs, especially the Blazor node security overview. Ok ... that's all cleared up now.

UPDATE (5/16): I've made the coverage split better by placing the new content into the new Security considerations topic.

Alternatively, we could flip the content around between these topics ...

  • The overview contains the security considerations coverage. The title probably changes to something more generic, such as ASP.NET Core Blazor Hybrid security.
  • The integration of auth/Identity becomes a sub-topic with a different title, such as Integrate Identity in ASP.NET Core Blazor Hybrid apps or ASP.NET Core Blazor Hybrid Identity integration.

UPDATE (5/17, Tuesday 8:20AM CST): HOLD as DRAFT for a couple of hours this morning. I need to check on something with the code examples. I should be able to resolve it quickly.

UPDATE (5/17 10:05AM): Nevermind! False alarm. I think we're good.

UPDATE (5/23): I'll go with a MS Identity Platform MSAL cross-link for WinForms.

@guardrex guardrex marked this pull request as ready for review May 16, 2022 10:30
@guardrex guardrex marked this pull request as draft May 17, 2022 13:19
@guardrex guardrex marked this pull request as ready for review May 17, 2022 15:05
@guardrex guardrex merged commit df50542 into main May 23, 2022
@guardrex guardrex deleted the guardrex/blazor-hybrid-security branch May 23, 2022 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants