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 AuthorizationMessageHandler remark #26739

Closed
enetstudio opened this issue Aug 14, 2022 · 6 comments · Fixed by #26860
Closed

Confusing AuthorizationMessageHandler remark #26739

enetstudio opened this issue Aug 14, 2022 · 6 comments · Fixed by #26860
Assignees
Labels
Blazor doc-enhancement Pri3 Source - Docs.ms Docs Customer feedback via GitHub Issue

Comments

@enetstudio
Copy link

enetstudio commented Aug 14, 2022

[EDIT by guardrex to add the metadata. You can add the metadata when you open an issue using the This page feedback button at the bottom of any article.]

AuthorizationMessageHandler is a DelegatingHandler used to attach access tokens to outgoing HttpResponseMessage instances.

I was wondering if something is the matter with me or with your documents... Shouldn't outgoing HttpResponseMessage be outgoing HttpRequestMessage. All the related documents use outgoing HttpResponseMessage I'm about to doubt my sanity.


Document Details

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

@davidfowl davidfowl transferred this issue from dotnet/aspnetcore Aug 15, 2022
@guardrex guardrex self-assigned this Aug 15, 2022
@guardrex guardrex added Source - Docs.ms Docs Customer feedback via GitHub Issue Blazor and removed ⌚ Not Triaged labels Aug 15, 2022
@guardrex
Copy link
Collaborator

guardrex commented Aug 15, 2022

Hello @enetstudio ... Yes, I agree ... it's confusing blurted out like that. We might make a change in the articles, and the product unit might want make a change to the API docs from which the article's remarks are derived.

The tokens are attached to HttpRequestMessage, as the following will show, but AuthorizationMessageHandler in the API docs explain the same thing as our Blazor article ("outgoing" for the HTTP response message) ...

A DelegatingHandler that attaches access tokens to outgoing HttpResponseMessage instances.

Reference: https://docs.microsoft.com/dotnet/api/microsoft.aspnetcore.components.webassembly.authentication.authorizationmessagehandler

... where the DelegatingHandler API doc defines ...

A type for HTTP handlers that delegate the processing of HTTP response messages to another handler ...

Reference: https://docs.microsoft.com/dotnet/api/system.net.http.delegatinghandler

Take a look at the reference source at ...

https://github.com/dotnet/aspnetcore/blob/main/src/Components/WebAssembly/WebAssembly.Authentication/src/Services/AuthorizationMessageHandler.cs#L47-L82

From the API docs again, AuthorizationMessageHandler.SendAsync ...

Sends an HTTP request to the inner handler to send to the server as an asynchronous operation.

Reference: https://docs.microsoft.com/dotnet/api/microsoft.aspnetcore.components.webassembly.authentication.authorizationmessagehandler.sendasync

The delegating handler attaches a token (existing or a new one that it obtains from the token provider) to the HttpRequestMessage (request) parameter and base.SendAsync() is called/awaited at the end with the method returning a Task<HttpResponseMessage>. This makes it so that API calls don't need to deal with the ceremony in developer code or have duplicated code to get access tokens on web API requests.

The confusing "outgoing" language is probably present because this is from the handler as a Task<HttpResponseMessage>.

We have some light coverage in the HTTP Requests article ...

https://docs.microsoft.com/aspnet/core/fundamentals/http-requests#outgoing-request-middleware

Now, I'm aware of Steve Gordon's (old but still valuable) article ...

https://www.stevejgordon.co.uk/httpclientfactory-aspnetcore-outgoing-request-middleware-pipeline-delegatinghandlers

His article probably explains similar things about delegating handlers that the .NET docs explain, noting that HttpClient and DelegatingHandler are actually over in System.Net.Http, not here ... not ASP.NET Core. Outside of that bit in the HTTP Requests article, you probably won't find further coverage in ASP.NET Core docs.

Shameless plug btw for @stevejgordon's PluralSight courses, which are highly regarded in the community ... and No! I'm not getting a kickback 💰 for mentioning that 😆. Indeed! Note that Steve wrote the initial HTTP Requests article, and I think it has been popular with our readers. You'll see the parallels in that Outgoing Request Middleware section to his blog post.

Perhaps, we should expand the explanations on this in the article seeking to avoid this confusion of an "outgoing" "response" for a token attached to a "request." I'd like to ping Javier later on this. I'd like to see if we can clear up the confusion. I can't reach out to him until after 7.0 ships later this year. The product unit is 🏃😅 under a heavy workload to get the next release out. I'll definitely touch base with Javier in either EOY or 23Q1.

... and please @stevejgordon if you receive this, correct me if I'm wrong about anything or if you have some additional color that would help here with "outgoing HttpResponseMessage instances."

When I worked on this language ... ei ei ei ... it may have been three years ago at this point ... I believe that I derived the language from the API docs thinking that external resources would fill in any detail on that. The cross-linked content leaves me unhappy with the current choice of words without further explanation, and that's assuming that their choice of words is sane. They may decide that that's a blown API description.

I'll get back to this as soon as I can, but expect work on it EOY or 23Q1. 🏃😅

@guardrex guardrex changed the title Do I need some fixing ? Confusing AuthorizationMessageHandler remark Aug 15, 2022
@enetstudio
Copy link
Author

@guardrex,

Thanks for your exhaustive response... The issue begins with the following:

A <see cref="DelegatingHandler"/> that attaches access tokens to outgoing <see cref="HttpResponseMessage"/> instances.

Source: https://github.com/dotnet/aspnetcore/blob/ce8419e91d84d42690da18d7af40e7ac086c11f4/src/Components/WebAssembly/WebAssembly.Authentication/src/Services/AuthorizationMessageHandler.cs#L11

And it is being cascaded to all the articles and documents that deal with DelegatingHandler. It simply should be HttpRequestMessage instead of HttpResponseMessage.

Perhaps, we should expand the explanations on this in the article seeking to avoid this confusion of an "outgoing" "response" for a token attached to a "request."

I don't think more explanations is needed. The issue is not the explanations, but the use of the wrong word. Just change AuthorizationMessageHandler is a DelegatingHandler used to attach access tokens to outgoing HttpResponseMessage instances. To AuthorizationMessageHandler is a DelegatingHandler used to attach access tokens to outgoing HttpRequestMessage instances

Good day...

@guardrex
Copy link
Collaborator

FYI: The articles are under our control. The API docs are under the direct control of the product unit. The API docs are derived directly from their API comments in the framework code.

I don't think more explanations is needed ... the use of the wrong word

I don't think that's going to turn out to be the case. The product unit usually doesn't make mistakes like that. I think Javier wrote that code/text, and he almost never makes mistakes, especially something that obvious. Additionally, all of the framework code is reviewed by other TOP ✈️ GUN devs on the team, and they would likely catch something like that. This code has been here a LONG time, too ... several years ... and likely would've been challenged by a dev along the way if it were wrong.

I can inquire with them, but it will need to wait until 7.0 releases later this year. They're just too busy to chat about something this minor at this time. Our best bet in the meantime is if @stevejgordon sees this and tells us what's what ... if my interpretation is correct/incorrect.

@enetstudio
Copy link
Author

@guardrex,

Again, I do not suggest that the code is faulty. The code is perfectly fine... The issue is with the comments that are being cascaded to the documents, and express something that is not true. When you read in the documents: AuthorizationMessageHandler is a DelegatingHandler used to attach access tokens to outgoing HttpResponseMessage instances.
You read something completely wrong. An access token is added to an outgoing request message, not to a response message of which you can't speak in terms of outgoing...

I don't think that's going to turn out to be the case... challenged by a dev along the way if it were wrong.

I completely disagree with you. To prove that you're wrong I decided to peruse the Blazor source code, and of course started with ComponentBase class. Here's what I found: Override this method if you will perform an asynchronous operation https://github.com/dotnet/aspnetcore/blob/a4906cd3e44136bd210bd522ff61eadba3f84a86/src/Components/Components/src/ComponentBase.cs#L69

Now, this grammar mistake is being cascaded to the docs, a formal document of Microsoft. 13 contributors, and no one have noticed if you will perform? What say you ? OK, I won't be childish, and stop here.

A couple of years ago I posted in Github an issue which contained a list of problems related to the comments. There were two types of mistakes. As for instance, the code employs an HttpClient object, but in the comments the HttpClient becomes HttpContext, and such like. The other type of mistakes are related to the English language: grammar mistakes, incomplete sentences ( a whole sentence is a subject with no predicate, no object...), wrong use of the indefinite article an, not being aware that you apply the article to singular noun that begins with initial vowel SOUND, and such like. All these are being cascaded to the docs, which is your own domain, right. Anyhow, Steve Sanderson suggested that I would open new pull requests. Poor me, I didn't even know what is a pull request. I only wanted to attract the Blazor team's attention to the existence of these issues. Years after these issues are still persisted. It's a pity I find it hard now to locate them.

Good day. Sorry for taking your time.

@guardrex
Copy link
Collaborator

guardrex commented Aug 16, 2022

Let me try to explain that in more detail: The framework code contains the API remarks, and the API remarks are what generates the API documentation. That must be updated on the product unit's repository. Nothing is "cascaded," per se, directly from their API remarks to the ASP.NET Core documentation set of this repo. Yes, we use their remarks in articles, and we do seek to improve grammar and style of those remarks when we use them. To any extent that we've failed in this regard, we welcome issues and PRs for corrections. If there's an error in the API remarks, the product unit must make that update (or at least agree to an update if a community member submits the PR).

WRT poor grammar in API remarks, I agree with you 😄, and I've raised that issue with management a few times over the years. It hasn't been a priority for them to take on a formal effort to improve it ... and not just API remarks ... framework error messages also use some ... er ... interesting English! 😆 They allow community developers to submit changes to those remarks (and error messages) on their repo. It's always best to ask first tho in an issue. PRs without an issue are generally only welcome for true patches (e.g., a misspelled word).

WRT there being a simple typo or not in the framework's API remarks (HttpResponseMessage vs. HttpRequestMessage), that is undetermined at this time. You might be right. I'm just saying that we should get formal guidance from Javier before making any changes to the articles. I will return to this and get it sorted out as soon as I can. I don't want to disturb Javier at this time given the heavy workload over there for .NET 7.

@guardrex
Copy link
Collaborator

I took another look at this, and I'm going to try and reach out (internally) to see if my hunch is correct. I'm pitching that we merely clean up the confusing language by changing the remark to ...

AuthorizationMessageHandler is a DelegatingHandler used to process access tokens.

However, the API docs probably won't change. I think what they say is correct given the way that these message handlers process the tokens and requests/responses internally. That's not too important for the doc tho. I think we can just say "process access tokens" and not get into the weeds on the underlying implementation and behavior.

If my guess is wrong tho, Javier will probably make a change to the API docs. There are two spots that he'd need to look at. The AuthorizationMessageHandler API remarks and the DelegatingHandler API remarks would probably need to be updated or expanded. Those are in the framework code and not under our control here.

I'll ping you on the PR when I get a response and create some kind of fix for this.

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

Successfully merging a pull request may close this issue.

3 participants