[release/3.1] Pass access token as query string when running SignalR in the browser #20466
Conversation
wtgodbe
added
Servicing-consider
|
Hello human! Please make sure you've included the Shiproom Template in a comment or (preferably) the PR description. Also, make sure this PR is not marked as a draft and is ready-to-merge. |
ghost
added
the
|
|
||
| internal static bool IsRunningInBrowser() | ||
| { | ||
| return RuntimeInformation.IsOSPlatform(OSPlatform.Create("WEBASSEMBLY")); |
There was a problem hiding this comment.
Is this what we agreed on with the BCL team?
There was a problem hiding this comment.
This is what we have now. I believe discussions are ongoing and we can react later if necessary. Personally I’d prefer a string like “browser” since WASM is not browser-specific, but 🤷♀️
There was a problem hiding this comment.
The string needs to be browser. Can we circle back and make sure? Last I checked we were leaning in that direction. I worry about future breaking changes
There was a problem hiding this comment.
@pranavkm @danroth27 @SteveSandersonMS What's the story here? Is there a final decision on the string to use for detecting the browser? We'd really like to get this in and right the first time before 3.1.4 closes on Tuesday.
There was a problem hiding this comment.
Is there any idea when that'll happen? I guess we'll need to either wait, or change the string at the same time they change it
There was a problem hiding this comment.
The change should land before 3.2-RC. Since this change aligns with the RTM release, using BROWSER might be the correct thing to do. Again @lewing in case you think otherwise.
There was a problem hiding this comment.
We can change it now to BROWSER. The timing should be fine (3.1.4 is shipping no earlier than 3.2 GA, by the current schedule). It's not going to fail if the string is wrong, right? It just won't light up the fixed behavior until you're running on a runtime with the updated OS platform string.
There was a problem hiding this comment.
It's not going to fail if the string is wrong, right? It just won't light up the fixed behavior until you're running on a runtime with the updated OS platform string.
That'd be my understanding, it won't fail, it just won't do anything until the corresponding change is made. I'll make the change
| if (transportType == HttpTransportType.ServerSentEvents && _isRunningInBrowser) | ||
| { | ||
| Log.ServerSentEventsNotSupportedByBrowser(_logger); | ||
| transportExceptions.Add(new TransportFailedException("ServerSentEvents", "The transport is not supported in the browser.")); |
There was a problem hiding this comment.
Why is that? Is it because the http client isn't fetch based?
There was a problem hiding this comment.
From @BrennanConroy on the other PR:
Was thinking about this over the weekend and realized we need to do something about SSE.
Right now it doesn't work because we use an HttpClient GET with "text/event-stream" Accept header. Which doesn't work in the browser, we use the EventSource API in the Typescript client to do a streaming response.
We might just have to disable SSE when running the .NET client in the browser.
There was a problem hiding this comment.
I believe @BrennanConroy tried it and it didn’t work. We can always make adjustments and reenable. Plus, we are considering deprecating SSE in general.
leecow
added
Servicing-approved
|
Windows failures are #20687 |
|
With #20736 merged, are we ready to merge this in (once the build passes)? |
That was my plan, just want to get a green CI first |
|
🎉 🚀 |
Resolves #18697
Description
When running in the browser, we can't use request headers. The SignalR client tries to do so anyways, which causes issues w/ Blazor Wasm. This PR detects if we're running in the browser, and sends the access token as a query string instead.
Customer Impact
Customers writing Blazor Wasm apps w/ SignalR will not see issues when adding Auth to their apps
Regression?
No
Risk
Behavior has been tested locally, but there could be obscure scenarios we've missed.
I used https://github.com/wtgodbe/scratch/tree/master/SigRBlazor/BlazorSignalRApp to validate that the token gets passed as a query string & can be pulled out and referenced by the server.