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

Confusing instructions, Code example cause internal Server error #27279

Closed
hjrb opened this issue Oct 13, 2022 · 10 comments
Closed

Confusing instructions, Code example cause internal Server error #27279

hjrb opened this issue Oct 13, 2022 · 10 comments
Assignees
Labels
Blazor needs-more-info Source - Docs.ms Docs Customer feedback via GitHub Issue

Comments

@hjrb
Copy link

hjrb commented Oct 13, 2022

Hello,
overall it works locally. Here are some points to improve:

  1. It isn't clear what are step when the reader has something to do and when it just information what the generated code / configuration does. That should be clearly marked
  2. It is sometimes not obvious if the reference code is part of the server project or the client project
  3. Why not add the given instructions what the code does and why it is needed as comments in the generated code.
  4. The section about using UserManager is unclear and doesn't work. The ApplicationUser Class doesn't exists. Obviously the reader is supposed to create it somehow like this:
    public class ApplicationUser : IdentityUser
    {
    // public virtual string Email { get; set; } // example, not necessary
    }
    Else the code sample will not compile.
    The code sample causes the API server to crash. Already adding the dependency injction reference
    public WeatherForecastController(ILogger logger
    ,UserManager userManager
    )
    {...
    is enough for the app to crash.
    Add
    builder.Services.Configure(options =>
    options.ClaimsIdentity.UserIdClaimType = ClaimTypes.Upn);
    to the program.cs file BEFOR builder.Build() doesn't help
  • When I deploy the app to Azure using VS 2022 17.4.0 the deployment work but when I try to login there is a crash.

Microsoft.AspNetCore.Components.WebAssembly.Rendering.WebAssemblyRenderer[100]
Unhandled exception rendering component: ConstructorContainsNullParameterNames, Microsoft.AspNetCore.Components.WebAssembly.Authentication.RemoteAuthenticationService3+JavaScriptLoggingOptions[Microsoft.AspNetCore.Components.WebAssembly.Authentication.RemoteAuthenticationState,Microsoft.AspNetCore.Components.WebAssembly.Authentication.RemoteUserAccount,Microsoft.Authentication.WebAssembly.Msal.Models.MsalProviderOptions] SerializationNotSupportedParentType, System.Object Path: $. System.NotSupportedException: ConstructorContainsNullParameterNames, Microsoft.AspNetCore.Components.WebAssembly.Authentication.RemoteAuthenticationService3+JavaScriptLoggingOptions[Microsoft.AspNetCore.Components.WebAssembly.Authentication.RemoteAuthenticationState,Microsoft.AspNetCore.Components.WebAssembly.Authentication.RemoteUserAccount,Microsoft.Authentication.WebAssembly.Msal.Models.MsalProviderOptions] SerializationNotSupportedParentType, System.Object Path: $.
---> System.NotSupportedException: ConstructorContainsNullParameterNames, Microsoft.AspNetCore.Components.WebAssembly.Authentication.RemoteAuthenticationService`3+JavaScriptLoggingOptions[Microsoft.AspNetCore.Components.WebAssembly.Authentication.RemoteAuthenticationState,Microsoft.AspNetCore.Components.WebAssembly.Authentication.RemoteUserAccount,Microsoft.Authentication.WebAssembly.Msal.Models.MsalProviderOptions]

PLEASE ALWAYS ADD a section how to deploy to Azure. IT ISN'T TRIVIAL!!

Overall using Blazor and .NET Core with AAD feels like black magic. EXTREMLY complicated. The support by VS 2022 isn't really working.

Document Details

Do not edit this section. It is required for learn.microsoft.com ➟ GitHub issue linking.

@dotnet-bot dotnet-bot added Blazor Source - Docs.ms Docs Customer feedback via GitHub Issue labels Oct 13, 2022
@guardrex
Copy link
Collaborator

Hello @hjrb ... I'm OOF for only about an hour. I'll take a look at this and get back to you soon.

@guardrex
Copy link
Collaborator

guardrex commented Oct 13, 2022

Ok ... I'm BACK!

This topic, and the other WASM security node topics are due for a pass on #24615, which is scheduled for the 🦃 to 🎁 time frame at the end of this year. I was going to perform a considerable overhaul of the whole node, but we decided that that would be too costly 💰 given that how Blazor WASM deals with security is likely to receive a big review next year with possible major updates for .NET 8 at the end of 2023. Therefore, it was decided that spending a lot of 💲💵💲on it right now isn't a good idea.

Still tho ... I'd like to give the node a pass when things cool off after .NET 7 comes out in November. That's approved by management. What's probably going to happen to your issue here is that I'll close this but cross-link these remarks on that tracking issue for review when the updates are being made. I'll refer back to this at that time.

Now, for your specific points ...

It isn't clear what are step when the reader has something to do and when it just information what the generated code / configuration does. That should be clearly marked

Fair enough ... it's when you reach the Server app configuration section that it begins to just talk about what's in the solution. I agree with you on this point. I'll tend to this on the updates later.

It is sometimes not obvious if the reference code is part of the server project or the client project

I think this is because the sizes of our headings aren't very different and there are perhaps too many subsections between the major headings for Server and Client app sections. Therefore, it's hard to see the divisions between the Server app configuration section (with its subsections) and the Client app configuration section (with its subsections). It's a good idea to clean this problem up, too. I agree with you, and I'll tend to this here and on all of the topics in this node.

Why not add the given instructions what the code does and why it is needed as comments in the generated code.

Ah! Good question ... and the answer is localization into other languages. Our docs are automatically (machine) translated and human translated, BUT not for code comments. Therefore to reach everyone, we need to keep remarks in the text. Now, I do try to explain the code in the text. To the extent that I don't do a good job on a section ... or a topic 🙈😄, I make updates to improve the text. I'll assess this point as well on the updates later.

The section about using UserManager is unclear and doesn't work.

It should work. Make sure you're looking at the version of the topic that matches your app version because there was a change at 6.0. There could be a version-specific problem that I'm not aware of. This will require further investigation if the exact problem isn't reported to me. I'd look as soon as I can, but it might get pushed to November if the specific problem isn't reported because this is the first time the feedback has come in and this was working earlier.

The ApplicationUser Class doesn't exists. Obviously the reader is supposed to create it somehow like this:

IIRC, this section of the topic was added relatively recently, possibly at reader request. It's not helpful to avoid placing an ApplicationUser class example here, so I will do that. I agree with you on this.

When I deploy the app to Azure using VS 2022 17.4.0 the deployment work but when I try to login there is a crash.
PLEASE ALWAYS ADD a section how to deploy to Azure. IT ISN'T TRIVIAL!!

The whole topic is about hosting in Azure. That's what this topic describes. I think what's happened is just that the error you hit needs to be resolved and it will work 🤞🍀.

Can you put up a minimal repro project of your failing app for me to look at? Just strip out the sensitive strings in the app settings files of the Client and Server projects. If you can't tho, there might be a delay ... maybe only a short delay ... for me to take a look, as I'm working on a number of .NET 7 release issues right now and have a hard deadline coming for final release of 7.0 in early November.

Overall using Blazor and .NET Core with AAD feels like black magic. EXTREMLY complicated.

I agree. A lot of it is due to the wide array of features and use case scenarios that can be covered by the framework(s) and the Azure platform. This is meant to be the simplest set of instructions ... with a few extra explanations and 😈 gotcha items to watch out for ... and it's still complicated 😩. For the most part, the topics in this node don't generate a lot of negative feedback, but I know that devs run into problems. Let's see if we can get your app running, and I'll take a look at the whole node using everyone's feedback later this year. You can see the wide array of issues opened at that tracking issue ...

#24615

... scroll down to the UE pass tracking bit and look at the Security and Identity node entries.

@guardrex
Copy link
Collaborator

guardrex commented Oct 13, 2022

I'm going to need to temporarily remove that INCLUDE for the UserManager/SignInManager until I can spend more time with it. That section was really written for the Identity Server-based hosted WASM scenario, where it works as written ...

https://learn.microsoft.com/en-us/aspnet/core/blazor/security/webassembly/hosted-with-identity-server?view=aspnetcore-6.0&tabs=visual-studio#usermanager-and-signinmanager

BTW ... The ApplicationUser class is automatically added to the app when Individual Accounts auth is selected when creating the app from a project template for that topic, which is why you don't see it shown in the text there.

Let me know if you can put up that repro project for the basic Azure hosting scenario (without the UserManager). If you can't do that, then I'll check on the coverage in November along with the rest of the content. I'll make the changes that you suggested above.

Unfortunately, I don't have a web API cross-link. The UserManager coverage is mostly sprinkled into other specific scenarios and examples ... for example ...

https://learn.microsoft.com/en-us/aspnet/core/security/authentication/add-user-data?view=aspnetcore-6.0&tabs=visual-studio

It's the same general concept for a web API (i.e., the Server app of a hosted WASM solution); but like I said, it's going to take me more time to work out the coverage for the scenario than I have at the moment.

@hjrb
Copy link
Author

hjrb commented Oct 14, 2022

Hi my test app is on github https://github.com/hjrb/VSSApp - which user would I need to grant access?

@guardrex guardrex reopened this Oct 14, 2022
@guardrex
Copy link
Collaborator

I'm not sure because I don't work with private GH repos. Prior to writing docs for the .NET foundation/Microsoft, I never used GH. We had different source control systems at every company I ever worked for.

Normally, we request a public minimal repro project. If you can't grant me access to that, then you'd either need to make it public or wait for me to circle around to this later this year ... I hope 🤞🍀.

@hjrb
Copy link
Author

hjrb commented Oct 14, 2022

I invited you

@guardrex
Copy link
Collaborator

Ok ... cool. I see the repo now.

I'll try and take a look today (Friday). I'm buried in .NET 7 stuff at the moment ⛰️⛏️😅, but I might be free of it later this morning. If not tho ... if you don't hear back from me today ... I'll be back Monday/Tuesday to take a look and respond.

@guardrex
Copy link
Collaborator

FYI ... Yes! ... I'm running out of time today ... rapidly running out of time 🏃‍♂️🕐😄. I can see that just to finish this .NET 7 thing that I'm working on that I'll exhaust my schedule today with it.

I'll get back to this on Monday/Tuesday next week. I apologize for the delay, but we have a hard deadline for the .NET 7 documentation. It all has to be in place and ⚡ LIVE ⚡ on release day or HEADS WILL ROLL! 🔪😨🤣

@guardrex
Copy link
Collaborator

guardrex commented Oct 17, 2022

I took a look, @hjrb ... a few points ...

  • AFAIK, that's an unverified/untrusted publisher domain that you're using. Therefore, ... again, AFAIK ... you should adopt our article's guidance for scopes and the Audience for unverified/untrusted publisher domains.

    As a side-note, I do see that the Azure product unit (via Azure docs) has been making changes in this area to some degree. I see NEW content over in their docs on verified/unverified (trusted/untrusted) publisher domains. I'm not going to read all of that content right now (I'm too busy with .NET 7 coverage 🏃 at the moment) to see exactly what's changed, but there are at least some changes ... of some sort ... perhaps, merely changes to how/where the Azure portal deals with the publisher domain.

  • I see that you're mixing 7.0 RC1 with release packages for Azure/MS Identity (e.g., there is a 5.0.0-preview.12 preview package for Microsoft.Graph). That's almost always a recipe for disaster 💥😢 due to incompatibilities. Try setting up the app with only .NET 6-era release packages. After you get it all working in .NET 6, then you could try and update the TFM and packages to 7.0-era prerelease/preview and see how it goes.

  • Finally, I'm not aware (YET) of all of the Azure/MS Identity updates that were made even for 6.0. I've been chat'in with management for the last two years about performing an overhaul of the whole Blazor security node in order to capture all of the updates on the Azure/MS Identity side, but I've been too 🏃😅 on all kinds of other work here this year without having a large block of time to totally overhaul this node. In fact, I said that it would take a huge block of time ... thus 💰💰💰... to perform such a vast overhaul, and it was decided in the discussion that it probably isn't a good idea to spend so much treasure on such an overhaul at this time given the following considerations ...

    • Azure docs have improved considerably since Blazor was released and these docs were first placed. Back in those 3.x days, there was next to nothing for Blazor apps. We had to provide most of this content because use cases were so difficult to wire up in Blazor apps based on the generic coverage in Azure/auth docs. These days, they have a lot more coverage, including Blazor tutorials; and in theory, Azure/MS Identity for scenarios like MS Graph can be completely covered by their docs and dropped from our doc set here ... in theory 😄.
    • The other thing is that it has been a while now ... several years ... since Blazor framework's security approaches were invented. Blazor might get an exhaustive security review and set of updates for perhaps .NET 8 or .NET 9. It doesn't make sense to spend a lot of time and money on a set of docs that will be for OLD approaches to implementing authn/z in Blazor apps.

    For these reasons, it was decided that a pass for accuracy, not a full overhaul and rewrite, of these docs will be performed, and I hope to be able to do that in the Thanksgiving 🦃 to New Years 🎆 holiday season, when the product unit has wound down and is taking some well-deserved time off to recharge.

Anyway, I mention all of this to let you know that several of your suggestions can be rolled into the updates, BUT it's unlikely that we'll duplicate any coverage here that I can cross-link in the Azure docs (e.g., MS Graph). It depends ... if I can use their guidance cleanly without problems, then I can just concentrate our coverage on the Razor component aspects. If I encounter pain 😩 and problems 😩 implementing their guidance in Blazor apps, I shouldn't drop coverage here and try to fix it up as much as I can without performing a total overhaul.

Well ... I wrote another BOOK 📖 here! 🙈🤣

What you can do is consider my points for your app ...

  • Work further on the trusted/untrusted publisher domain situation. If you do have an unverified/untrusted domain in use as I think you do, try following those aspects of our article for an untrusted/unverified domain. It affects the scope configuration and stating the Audience in the Client and Server apps, and think IIRC 🤔 that that's just a matching Audience app settings value on each side. The doc should explain it out, and it at least cross-links to some of the older Azure docs coverage on the subject for further reading.
  • Don't risk release packages with prerelease packages! 😨😄 ... and probably don't risk prerelease at all right now if you're angling authn/z for a production app at this time. I think you're better off just trying to get things working with .NET 6 across the board first. I'm not saying that this is a ✨ magic bullet ✨ necessarily, but I've seen many devs over the years have major problems mixing release/prerelease. Besides, security code in prerelease is too unstable for production apps.

WRT to this issue: I am going to close it shortly as just a discussion. However, I will be making the agreed-upon suggestions that you made when I make that pass on this node after 🦃 day. I'll be working through all of the docs with test apps to update the code. For example, I missed one thing for sure in one spot ... Minimal APIs for the server app ... I see Startup bits in here! Yikes! 🙈 ... and I'll be checking Azure/MS Identity docs and cross-linking and re-writing content where needed.

Even with this issue closed, please feel free to provide additional information on this issue for me to see later. For example, I hope to hear that you did get your app working and what you were able to fix to get it running. That could be an important thing for the doc updates EOY. I like to call out gotchas 😈 that devs might hit when building apps. This issue is cross-linked to my tracking issue for the work, so your comments here won't be lost after I close this.

@guardrex
Copy link
Collaborator

I'll close now ... but again ... please feel free to post here what the problem turned out to be with the app. That might be something that articles can call out if it might bite other devs 😈.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blazor needs-more-info Source - Docs.ms Docs Customer feedback via GitHub Issue
Projects
Archived in project
Development

No branches or pull requests

3 participants