-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Update OIDC lib remark to MSAL #30421
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
Conversation
A few notes on your comment, @JohnGalt1717 ...
I can't address that. We'll need a PU member to take a look. The articles in the Hybrid security node ...
... were written by the engineers ... IIRC, Javier. My advice on your ask about that is to open a new issue using the This page feedback button and form at the bottom of the English-US topic. Use of the This page feedback form adds metadata to your GitHub issue that cross-links the topic. Beth Massi, the PM, and I will be pinged. She (or I) can then ask Javier (or Jeremy) to take a look at your ask.
I don't think anything else will be done WRT additional flow support. Only PKCE auth code flow is supported by the team ... this is in spite of MSAL being able to support other flows. Other flows would need to be implemented by the dev using the documentation that the Microsoft Identity Platform folks put up in their doc set. You're probably catching on now that we're highly compartmentalized around here on docs. Indeed so! 😄 I work from the ASP.NET Core Blazor side. The security doc folks are a different group of doc authors and engineers. We interact, but most of our content here came from our team, not directly from them. We probably won't be addressing other flows in our Blazor docs. |
... another quick note on that last bit ... The BUT 😆 ... I'm NOT a security pro. I leave that to Javier, Jeremy, and the PU. I'm just the docs dino 🦖 around here. |
Ok, I'd suggest that it just explicitly state on the Auth provider specific page that it uses code flow, and that on the overview page it be made clear you don't need msal to make code flow work and that it works with the Auth provider standalone too. |
Be more specific ...
AFAICT, all of the content after this PR goes in say that only PKCE auth code is supported. BUT, that's what I'm asking ... exactly what article/section/paragraph/sentence are you saying is a problem? ... keeping in mind that I'm referring to the Blazor docs. We don't control docs outside of the Blazor docs.
Well, you do tho if you're using the MSAL NuGet package maintained by the ASP.NET Core team (and thus MSAL script) OOB. That's all that we support. That's all that Javier wants to tell devs. I think this subject came up years ago, and he said something like we shouldn't be mentioning to devs how to do anything different than what we describe/show because it isn't safe (in the product unit's opinion) and isn't supported by management here ... our direct management. MSAL does support such things, but they don't want to even mention it. You can go to the Identity Platform/MSAL docs and read all about all of the flows and how to implement them. We do cross-link the guidance here, we just don't speak further on it. |
This is the problem: Devs don't want to be using MSAL. It's lock in. They want to be using what's listed under the standalone Authentication page using Authentication Provider. That page needs to state explicitly that this is using OpenIdConnect Authorization Code Flow and does not support implicit code flow. And then the introduction page should make it clear that MSAL is an alternative approach and not required to get OpenIdConnect Authorization Code Flow. |
Ok, I see what you mean now. Yes, I'll work on that for the next commit.
That's true in the general sense. The article's remarks are focused on our package that uses MSAL, so it isn't an alternative in that sense. Let me see if I can add a carefully-worded remark about not using the package. |
Ok .... that should be getting close ... if it's accurate. I'll ping Javier offline to look at these updates. |
@guardrex tokens in the browser is no longer recommended, maybe this should be updated as well. Greetings Damien |
Sure ... let me hear from Javier on that, too. ... and I wasn't focused on token-related remarks this morning. I'll need to take another look on Thursday morning. I'm OOF for most of the rest of the day. |
I'm not going to hold up fixing the bug for mere review of the other line updates. Given that it's Friday, this won't be seen today. I'll make a note to ping the product unit on Monday morning to take a look. For now, we'll go with this. @damienbod ... I'll ask about the token language, too, and we'll work on that after I hear back. |
Fixes #30418
It looks like there's one offensive line 😈 that slipped by the MSAL updates. That will be fixed on this PR.
I'm not aware of any problems with the OIDC process remarks further down the article.
This article ... all of them, actually ... will be updated and reviewed in the RC2 period sometime around the end of October/start of November. We'll be adding the BWA guidance in that period and adjusting for any framework changes made for WASM apps (and WASM components of BWA apps).
Internal previews