[blazor-wasm] Pass access token as query string when running SignalR in the browser #20115

wtgodbe · 2020-03-24T22:11:08Z

Attempts to resolve #18697

When the signalR client is running in the browser, we need to append the access token as a query string rather than using a request header.

Since this is in blazor-wasm, there were some infra changes needed to get the package building & whatnot.

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.

analogrelay · 2020-03-24T22:22:42Z

...ients/csharp/Http.Connections.Client/ref/Microsoft.AspNetCore.Http.Connections.Client.csproj

@@ -1,18 +0,0 @@
-<!-- This file is automatically generated. -->
-<Project Sdk="Microsoft.NET.Sdk">


Hrm, I wonder why we had this. It wasn't in the shared framework before... (I just checked)

src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/Utils.cs

src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/WebSocketsTransport.cs

src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/Utils.cs

src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/WebSocketsTransport.cs

src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/Utils.cs

wtgodbe · 2020-03-24T22:45:55Z

@pranavkm @BrennanConroy @anurse I pushed a commit responding to feedback

...ients/csharp/Http.Connections.Client/src/Microsoft.AspNetCore.Http.Connections.Client.csproj

BrennanConroy · 2020-03-25T17:12:50Z

src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/WebSocketsTransport.cs

+                    // We can't use request headers in the browser, so instead append the token as a query string in that case
+                    if (_isRunningInBrowser)
+                    {
+                        var accessTokenEncoded = UrlEncoder.Default.Encode(accessToken);


Would be nice to test this code path. Maybe we can make _isRunningInBrowser internal and modify it in tests to be true.

Writing a test for this is probably something I'd need help with - do you know of a test I can use as an example of some similar scenario?

Probably something similar to

aspnetcore/src/SignalR/server/SignalR/test/WebSocketsTransportTests.cs

Line 68 in 20f6d65

public async Task WebSocketsTransportSendsUserAgent()

pranavkm

Looks good to me. Good to go once SignalR's happy

src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/Utils.cs

BrennanConroy · 2020-03-30T02:26:19Z

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.

analogrelay · 2020-03-30T18:24:30Z

We might just have to disable SSE when running the .NET client in the browser.

Yep, let's do that. In 5.0 we should just remove it altogether.

wtgodbe · 2020-03-31T20:19:39Z

This guy? https://github.com/dotnet/aspnetcore/blob/master/src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/ServerSentEventsTransport.cs

Is there standard practice for this? Throw a PlatformNotSupportedException or similar in StartAsync?

BrennanConroy · 2020-03-31T20:54:51Z

We are going to want to disable transport fallback to SSE in the browser, and probably throw if the client specifies SSE as the only transport it wants to use.

halter73 · 2020-03-31T20:58:39Z

This guy?

Yep.

Is there standard practice for this? Throw a PlatformNotSupportedException or similar in StartAsync?

ServerSentEventsTransport is internal, so as long as StartAsync doesn't get called, we shouldn't need to change it. I would take a look at how we skip WebSockets on platforms where it's not supported and do something similar:

aspnetcore/src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs

Lines 361 to 366 in 8a134f8

    
           if (transportType == HttpTransportType.WebSockets && !IsWebSocketsSupported()) 
        
           { 
        
               Log.WebSocketsNotSupportedByOperatingSystem(_logger); 
        
               transportExceptions.Add(new TransportFailedException("WebSockets", "The transport is not supported on this operating system.")); 
        
               continue; 
        
           }

wtgodbe · 2020-03-31T21:31:45Z

We should also throw an exception if the user passes only SSE, and is running in the browser

aspnetcore/src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs

Lines 146 to 147 in 33d775a

	_httpConnectionOptions = httpConnectionOptions;

src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.Log.cs

BrennanConroy · 2020-04-01T21:33:27Z

src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.Log.cs

@@ -66,6 +66,9 @@ private static class Log
            private static readonly Action<ILogger, HttpTransportType, Exception> _transportStarted =
                LoggerMessage.Define<HttpTransportType>(LogLevel.Debug, new EventId(18, "TransportStarted"), "Transport '{Transport}' started.");

+            private static readonly Action<ILogger, Exception> _serverSentEventsNotSupportedByBrowser =
+                LoggerMessage.Define(LogLevel.Debug, new EventId(19, "ServerSentEventsNotSupportedByBrowser"), "Skipping ServerSentEvents because they are not supported by the browser.");


Suggested change

LoggerMessage.Define(LogLevel.Debug, new EventId(19, "ServerSentEventsNotSupportedByBrowser"), "Skipping ServerSentEvents because they are not supported by the browser.");

LoggerMessage.Define(LogLevel.Debug, new EventId(19, "ServerSentEventsNotSupportedByBrowser"), "Skipping ServerSentEvents because it is not supported by the browser.");

Should I change https://github.com/dotnet/aspnetcore/pull/20115/files#diff-0d9edc1e2ea2a38f9a4be7f30733cc9eR61 as well?

Don't know what line you're trying to point out

Whoops

aspnetcore/src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.Log.cs

Line 61 in 9908f8a

LoggerMessage.Define(LogLevel.Debug, new EventId(16, "WebSocketsNotSupportedByOperatingSystem"), "Skipping WebSockets because they are not supported by the operating system.");

Hmm, we can just keep both as is. Not too picky

The difference is that WebSockets is the name of a transport and a name for the type of socket. In that latter sense, that could be plural. ServerSentEvents is just the name of a transport, so it should probably be singular. Super nitpicky though.

src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs

wtgodbe · 2020-04-14T19:59:44Z

Closing in favor of #20466, as this change is going into release/3.1 instead of blazor-wasm

wtgodbe added 8 commits March 10, 2020 13:08

Use query string instead of header if running in WASM

dc1aa2a

Try new method for calculating URI

c14b0bf

Add Http.Connections.Client to build.props

05cf101

Add Http.Connections.Client to ProjectReferences.props

5ca0f1b

Fix ProjectReferences.props

a03cb87

Delete ref assembly for Http.Connections.Client

dd7a83c

Prepend 'access_token=' to query string

348d79b

Use Util for detecting WASM

e0af487

wtgodbe requested review from analogrelay, pranavkm and BrennanConroy March 24, 2020 22:11

wtgodbe requested a review from halter73 as a code owner March 24, 2020 22:11

wtgodbe changed the title ~~Wtgodbe/access token provider~~ Pass access token as query string when running SignalR in the browser Mar 24, 2020

wtgodbe changed the title ~~Pass access token as query string when running SignalR in the browser~~ [blazor-wasm] Pass access token as query string when running SignalR in the browser Mar 24, 2020

analogrelay reviewed Mar 24, 2020

View reviewed changes

src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/Utils.cs Show resolved Hide resolved

src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/Utils.cs Outdated Show resolved Hide resolved

BrennanConroy reviewed Mar 24, 2020

View reviewed changes

src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/WebSocketsTransport.cs Outdated Show resolved Hide resolved

pranavkm reviewed Mar 24, 2020

View reviewed changes

Respond to feedback

e70e52d

halter73 reviewed Mar 24, 2020

View reviewed changes

...ients/csharp/Http.Connections.Client/src/Microsoft.AspNetCore.Http.Connections.Client.csproj Show resolved Hide resolved

mkArtakMSFT added Blazor ♥ SignalR This issue is related to the experience of Signal R and Blazor working together feature-blazor-wasm This issue is related to and / or impacts Blazor WebAssembly area-blazor Includes: Blazor, Razor Components labels Mar 25, 2020

BrennanConroy reviewed Mar 25, 2020

View reviewed changes

...ients/csharp/Http.Connections.Client/src/Microsoft.AspNetCore.Http.Connections.Client.csproj Outdated Show resolved Hide resolved

BrennanConroy reviewed Mar 25, 2020

View reviewed changes

pranavkm approved these changes Mar 25, 2020

View reviewed changes

src/SignalR/clients/csharp/Http.Connections.Client/src/Internal/Utils.cs Show resolved Hide resolved

wtgodbe added 2 commits March 25, 2020 13:49

Remove unneeded reference

6272fa3

Change package version for http.connections.client to 3.1.4

4a56599

wtgodbe added 2 commits April 1, 2020 14:21

Disable SSE in browser

f83f5ef

Throw exception when user passes only SSE when running in browser

9908f8a

BrennanConroy reviewed Apr 1, 2020

View reviewed changes

wtgodbe added 2 commits April 1, 2020 14:35

Fixup log

7d931f6

Fix typo

f9341c8

BrennanConroy approved these changes Apr 1, 2020

View reviewed changes

halter73 approved these changes Apr 2, 2020

View reviewed changes

mkArtakMSFT removed the feature-blazor-wasm This issue is related to and / or impacts Blazor WebAssembly label Apr 8, 2020

mkArtakMSFT assigned wtgodbe Apr 8, 2020

wtgodbe closed this Apr 14, 2020

wtgodbe deleted the wtgodbe/AccessTokenProvider branch April 14, 2020 19:59

		@@ -1,18 +0,0 @@
		<!-- This file is automatically generated. -->
		<Project Sdk="Microsoft.NET.Sdk">

	LoggerMessage.Define(LogLevel.Debug, new EventId(19, "ServerSentEventsNotSupportedByBrowser"), "Skipping ServerSentEvents because they are not supported by the browser.");
	LoggerMessage.Define(LogLevel.Debug, new EventId(19, "ServerSentEventsNotSupportedByBrowser"), "Skipping ServerSentEvents because it is not supported by the browser.");

[blazor-wasm] Pass access token as query string when running SignalR in the browser #20115

[blazor-wasm] Pass access token as query string when running SignalR in the browser #20115

Uh oh!

Conversation

wtgodbe commented Mar 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wtgodbe commented Mar 24, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wtgodbe Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pranavkm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BrennanConroy commented Mar 30, 2020

Uh oh!

analogrelay commented Mar 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wtgodbe commented Mar 31, 2020

Uh oh!

BrennanConroy commented Mar 31, 2020

Uh oh!

halter73 commented Mar 31, 2020

Uh oh!

wtgodbe commented Mar 31, 2020

Uh oh!

Uh oh!

BrennanConroy Apr 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wtgodbe commented Apr 14, 2020

Uh oh!

Uh oh!

wtgodbe Mar 25, 2020 •

edited

Loading

analogrelay commented Mar 30, 2020 •

edited

Loading

BrennanConroy Apr 1, 2020 •

edited

Loading