Add error to negotiate (#2998)

clients/java/signalr/src/main/java/com/microsoft/aspnet/signalr/HubConnection.java

@@ -118,10 +118,13 @@ public class HubConnection {
         };
     }
 
-    private NegotiateResponse handleNegotiate() throws IOException {
+    private NegotiateResponse handleNegotiate() throws IOException, HubException {
         accessToken = (negotiateResponse == null) ? null : negotiateResponse.getAccessToken();
         negotiateResponse = Negotiate.processNegotiate(url, accessToken);
 
+        if (negotiateResponse.getError() != null) {
+            throw new HubException(negotiateResponse.getError());
+        }
         if (negotiateResponse.getConnectionId() != null) {
             if (url.contains("?")) {
                 url = url + "&id=" + negotiateResponse.getConnectionId();

clients/java/signalr/src/main/java/com/microsoft/aspnet/signalr/NegotiateResponse.java

@@ -3,34 +3,74 @@
 
 package com.microsoft.aspnet.signalr;
 
+import java.io.IOException;
+import java.io.StringReader;
 import java.util.HashSet;
 import java.util.Set;
 
-import com.google.gson.JsonArray;
+import com.google.gson.stream.JsonReader;
-import com.google.gson.JsonObject;
-import com.google.gson.JsonParser;
 
 class NegotiateResponse {
     private String connectionId;
     private Set<String> availableTransports = new HashSet<>();
     private String redirectUrl;
     private String accessToken;
-    private JsonParser jsonParser = new JsonParser();
+    private String error;
 
-    public NegotiateResponse(String negotiatePayload) {
+    public NegotiateResponse(String negotiatePayload) throws IOException {
-        JsonObject negotiateResponse = jsonParser.parse(negotiatePayload).getAsJsonObject();
+        JsonReader reader = new JsonReader(new StringReader(negotiatePayload));
-        if (negotiateResponse.has("url")) {
+        reader.beginObject();
-            this.redirectUrl = negotiateResponse.get("url").getAsString();
+
-            if (negotiateResponse.has("accessToken")) {
+        do {
-                this.accessToken = negotiateResponse.get("accessToken").getAsString();
+            String name = reader.nextName();
+            switch (name) {
+                case "error":
+                    this.error = reader.nextString();
+                    break;
+                case "url":
+                    this.redirectUrl = reader.nextString();
+                    break;
+                case "accessToken":
+                    this.accessToken = reader.nextString();
+                    break;
+                case "availableTransports":
+                    reader.beginArray();
+                    while (reader.hasNext()) {
+                        reader.beginObject();
+                        while (reader.hasNext()) {
+                            String transport = null;
+                            String property = reader.nextName();
+                            switch (property) {
+                                case "transport":
+                                    transport = reader.nextString();
+                                    break;
+                                case "transferFormats":
+                                    // transfer formats aren't supported currently
+                                    reader.skipValue();
+                                    break;
+                                default:
+                                    // Skip unknown property, allows new clients to still work with old protocols
+                                    reader.skipValue();
+                                    break;
+                            }
+                            this.availableTransports.add(transport);
+                        }
+                        reader.endObject();
+                    }
+                    reader.endArray();
+                    break;
+                case "connectionId":
+                    this.connectionId = reader.nextString();
+                    break;
+                default:
+                    // Skip unknown property, allows new clients to still work with old protocols
+                    reader.skipValue();
+                    break;
             }
-            return;
+        } while (reader.hasNext());
-        }
+
-        this.connectionId = negotiateResponse.get("connectionId").getAsString();
+        reader.endObject();
-        JsonArray transports = (JsonArray) negotiateResponse.get("availableTransports");
+        reader.close();
-        for (int i = 0; i < transports.size(); i++) {
-            availableTransports.add(transports.get(i).getAsJsonObject().get("transport").getAsString());
-        }
     }
 
     public String getConnectionId() {
@@ -48,4 +88,8 @@ class NegotiateResponse {
     public String getAccessToken() {
         return accessToken;
     }
+
+    public String getError() {
+        return error;
+    }
 }

clients/java/signalr/src/test/java/com/microsoft/aspnet/signalr/NegotiateResponseTest.java

@@ -5,13 +5,14 @@ package com.microsoft.aspnet.signalr;
 
 import static org.junit.jupiter.api.Assertions.*;
 
+import java.io.IOException;
+
 import org.junit.jupiter.api.Test;
 
 
 class NegotiateResponseTest {
-
     @Test
-    public void VerifyNegotiateResponse() {
+    public void VerifyNegotiateResponse() throws IOException {
         String stringNegotiateResponse = "{\"connectionId\":\"bVOiRPG8-6YiJ6d7ZcTOVQ\",\"" +
                 "availableTransports\":[{\"transport\":\"WebSockets\",\"transferFormats\":[\"Text\",\"Binary\"]}," +
                 "{\"transport\":\"ServerSentEvents\",\"transferFormats\":[\"Text\"]}," +
@@ -26,7 +27,7 @@ class NegotiateResponseTest {
     }
 
     @Test
-    public void VerifyRedirectNegotiateResponse() {
+    public void VerifyRedirectNegotiateResponse() throws IOException {
         String stringNegotiateResponse = "{\"url\":\"www.example.com\"," +
                 "\"accessToken\":\"some_access_token\"," +
                 "\"availableTransports\":[]}";
@@ -37,4 +38,20 @@ class NegotiateResponseTest {
         assertEquals("www.example.com", negotiateResponse.getRedirectUrl());
         assertNull(negotiateResponse.getConnectionId());
     }
+
+    @Test
+    public void NegotiateResponseIgnoresExtraProperties() throws IOException {
+        String stringNegotiateResponse = "{\"connectionId\":\"bVOiRPG8-6YiJ6d7ZcTOVQ\"," +
+                "\"extra\":\"something\"}";
+        NegotiateResponse negotiateResponse = new NegotiateResponse(stringNegotiateResponse);
+        assertEquals("bVOiRPG8-6YiJ6d7ZcTOVQ", negotiateResponse.getConnectionId());
+    }
+
+    @Test
+    public void NegotiateResponseIgnoresExtraComplexProperties() throws IOException {
+        String stringNegotiateResponse = "{\"connectionId\":\"bVOiRPG8-6YiJ6d7ZcTOVQ\"," +
+                "\"extra\":[\"something\"]}";
+        NegotiateResponse negotiateResponse = new NegotiateResponse(stringNegotiateResponse);
+        assertEquals("bVOiRPG8-6YiJ6d7ZcTOVQ", negotiateResponse.getConnectionId());
+    }
 }

clients/ts/signalr/src/HttpConnection.ts

@@ -25,6 +25,7 @@ export interface INegotiateResponse {
     availableTransports?: IAvailableTransport[];
     url?: string;
     accessToken?: string;
+    error?: string;
 }
 
 /** @private */
@@ -169,6 +170,10 @@ export class HttpConnection implements IConnection {
                         return;
                     }
 
+                    if (negotiateResponse.error) {
+                        throw Error(negotiateResponse.error);
+                    }
+
                     if ((negotiateResponse as any).ProtocolVersion) {
                         throw Error("Detected a connection attempt to an ASP.NET SignalR Server. This client only supports connecting to an ASP.NET Core SignalR Server. See https://aka.ms/signalr-core-differences for details.");
                     }

clients/ts/signalr/tests/HttpConnection.test.ts

@@ -554,6 +554,26 @@ describe("HttpConnection", () => {
         });
     });
 
+    it("throws error if negotiate response has error", async () => {
+        await VerifyLogger.run(async (logger) => {
+            const httpClient = new TestHttpClient()
+                .on("POST", /negotiate$/, () => ({ error: "Negotiate error." }));
+
+            const options: IHttpConnectionOptions = {
+                ...commonOptions,
+                httpClient,
+                logger,
+                transport: HttpTransportType.LongPolling,
+            } as IHttpConnectionOptions;
+
+            const connection = new HttpConnection("http://tempuri.org", options);
+            await expect(connection.start(TransferFormat.Text))
+                .rejects
+                .toThrow("Negotiate error.");
+        },
+        "Failed to start the connection: Error: Negotiate error.");
+    });
+
     it("authorization header removed when token factory returns null and using LongPolling", async () => {
         await VerifyLogger.run(async (logger) => {
             const availableTransport = { transport: "LongPolling", transferFormats: ["Text"] };

specs/TransportProtocols.md

@@ -18,13 +18,13 @@ Throughout this document, the term `[endpoint-base]` is used to refer to the rou
 
 ## `POST [endpoint-base]/negotiate` request
 
-The `POST [endpoint-base]/negotiate` request is used to establish a connection between the client and the server. The content type of the response is `application/json`. The response to the `POST [endpoint-base]/negotiate` request contains one of two types of responses:
+The `POST [endpoint-base]/negotiate` request is used to establish a connection between the client and the server. The content type of the response is `application/json`. The response to the `POST [endpoint-base]/negotiate` request contains one of three types of responses:
 
-1. A response that contains the `id` which will be used to identify the connection on the server and the list of the transports supported by the server.
+1. A response that contains the `connectionId` which will be used to identify the connection on the server and the list of the transports supported by the server.
 
   ```
   {
-    "id":"807809a5-31bf-470d-9e23-afaee35d8a0d",
+    "connectionId":"807809a5-31bf-470d-9e23-afaee35d8a0d",
     "availableTransports":[
       {
         "transport": "WebSockets",
@@ -44,7 +44,7 @@ The `POST [endpoint-base]/negotiate` request is used to establish a connection b
 
   The payload returned from this endpoint provides the following data:
 
-  * The `id` which is **required** by the Long Polling and Server-Sent Events transports (in order to correlate sends and receives).
+  * The `connectionId` which is **required** by the Long Polling and Server-Sent Events transports (in order to correlate sends and receives).
   * The `availableTransports` list which describes the transports the server supports. For each transport, the name of the transport (`transport`) is listed, as is a list of "transfer formats" supported by the transport (`transferFormats`)
 
 
@@ -62,6 +62,19 @@ The `POST [endpoint-base]/negotiate` request is used to establish a connection b
   * The `url` which is the URL the client should connect to.
   * The `accessToken` which is an optional bearer token for accessing the specified url.
 
+
+3. A response that contains an `error` which should stop the connection attempt.
+
+  ```
+  {
+    "error": "This connection is not allowed."
+  }
+  ```
+
+  The payload returned from this endpoint provides the following data:
+
+  * The `error` that gives details about why the negotiate failed.
+
 ## Transfer Formats
 
 ASP.NET Endpoints support two different transfer formats: `Text` and `Binary`. `Text` refers to UTF-8 text, and `Binary` refers to any arbitrary binary data. The transfer format serves two purposes. First, in the WebSockets transport, it is used to determine if `Text` or `Binary` WebSocket frames should be used to carry data. This is useful in debugging as most browser Dev Tools only show the content of `Text` frames. When using a text-based protocol like JSON, it is preferable for the WebSockets transport to use `Text` frames. How a client/server indicate the transfer format currently being used is implementation-defined.

src/Microsoft.AspNetCore.Http.Connections.Client/HttpConnection.cs

@@ -169,9 +169,9 @@ namespace Microsoft.AspNetCore.Http.Connections.Client
         /// A connection cannot be restarted after it has stopped. To restart a connection
         /// a new instance should be created using the same options.
         /// </remarks>
-        public async Task StartAsync(CancellationToken cancellationToken = default)
+        public Task StartAsync(CancellationToken cancellationToken = default)
         {
-            await StartAsync(TransferFormat.Binary, cancellationToken);
+            return StartAsync(TransferFormat.Binary, cancellationToken);
         }
 
         /// <summary>
@@ -428,6 +428,10 @@ namespace Microsoft.AspNetCore.Http.Connections.Client
                         {
                             negotiateResponse = NegotiateProtocol.ParseResponse(responseStream);
                         }
+                        if (!string.IsNullOrEmpty(negotiateResponse.Error))
+                        {
+                            throw new Exception(negotiateResponse.Error);
+                        }
                         Log.ConnectionEstablished(_logger, negotiateResponse.ConnectionId);
                         return negotiateResponse;
                     }

src/Microsoft.AspNetCore.Http.Connections.Common/NegotiateProtocol.cs

@@ -18,6 +18,7 @@ namespace Microsoft.AspNetCore.Http.Connections
         private const string AvailableTransportsPropertyName = "availableTransports";
         private const string TransportPropertyName = "transport";
         private const string TransferFormatsPropertyName = "transferFormats";
+        private const string ErrorPropertyName = "error";
         // Used to detect ASP.NET SignalR Server connection attempt
         private const string ProtocolVersionPropertyName = "ProtocolVersion";
 
@@ -99,6 +100,7 @@ namespace Microsoft.AspNetCore.Http.Connections
                     string url = null;
                     string accessToken = null;
                     List<AvailableTransport> availableTransports = null;
+                    string error = null;
 
                     var completed = false;
                     while (!completed && JsonUtils.CheckRead(reader))
@@ -136,6 +138,9 @@ namespace Microsoft.AspNetCore.Http.Connections
                                             }
                                         }
                                         break;
+                                    case ErrorPropertyName:
+                                        error = JsonUtils.ReadAsString(reader, ErrorPropertyName);
+                                        break;
                                     case ProtocolVersionPropertyName:
                                         throw new InvalidOperationException("Detected a connection attempt to an ASP.NET SignalR Server. This client only supports connecting to an ASP.NET Core SignalR Server. See https://aka.ms/signalr-core-differences for details.");
                                     default:
@@ -151,9 +156,9 @@ namespace Microsoft.AspNetCore.Http.Connections
                         }
                     }
 
-                    if (url == null)
+                    if (url == null && error == null)
                     {
-                        // if url isn't specified, connectionId and available transports are required
+                        // if url isn't specified or there isn't an error, connectionId and available transports are required
                         if (connectionId == null)
                         {
                             throw new InvalidDataException($"Missing required property '{ConnectionIdPropertyName}'.");
@@ -170,7 +175,8 @@ namespace Microsoft.AspNetCore.Http.Connections
                         ConnectionId = connectionId,
                         Url = url,
                         AccessToken = accessToken,
-                        AvailableTransports = availableTransports
+                        AvailableTransports = availableTransports,
+                        Error = error,
                     };
                 }
             }

src/Microsoft.AspNetCore.Http.Connections.Common/NegotiationResponse.cs

@@ -11,5 +11,6 @@ namespace Microsoft.AspNetCore.Http.Connections
         public string AccessToken { get; set; }
         public string ConnectionId { get; set; }
         public IList<AvailableTransport> AvailableTransports { get; set; }
+        public string Error { get; set; }
     }
 }

test/Microsoft.AspNetCore.SignalR.Client.Tests/HttpConnectionTests.Negotiate.cs

@@ -8,8 +8,10 @@ using System.Net.Http;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Connections;
 using Microsoft.AspNetCore.Http.Connections;
+using Microsoft.AspNetCore.Http.Connections.Client;
 using Microsoft.AspNetCore.Http.Connections.Client.Internal;
 using Microsoft.AspNetCore.SignalR.Tests;
+using Microsoft.Extensions.Logging.Testing;
 using Moq;
 using Newtonsoft.Json;
 using Xunit;
@@ -380,6 +382,37 @@ namespace Microsoft.AspNetCore.SignalR.Client.Tests
                     });
             }
 
+            [Fact]
+            public async Task NegotiateThatReturnsErrorThrowsFromStart()
+            {
+                bool ExpectedError(WriteContext writeContext)
+                {
+                    return writeContext.LoggerName == typeof(HttpConnection).FullName &&
+                        writeContext.EventId.Name == "ErrorWithNegotiation";
+                }
+
+                var testHttpHandler = new TestHttpMessageHandler(autoNegotiate: false);
+                testHttpHandler.OnNegotiate((request, cancellationToken) =>
+                {
+                    return ResponseUtils.CreateResponse(HttpStatusCode.OK,
+                            JsonConvert.SerializeObject(new
+                            {
+                                error = "Test error."
+                            }));
+                });
+
+                using (var noErrorScope = new VerifyNoErrorsScope(expectedErrorsFilter: ExpectedError))
+                {
+                    await WithConnectionAsync(
+                        CreateConnection(testHttpHandler, loggerFactory: noErrorScope.LoggerFactory),
+                        async (connection) =>
+                        {
+                            var exception = await Assert.ThrowsAsync<Exception>(() => connection.StartAsync(TransferFormat.Text).OrTimeout());
+                            Assert.Equal("Test error.", exception.Message);
+                        });
+                }
+            }
+
             private async Task RunInvalidNegotiateResponseTest<TException>(string negotiatePayload, string expectedExceptionMessage) where TException : Exception
             {
                 var testHttpHandler = new TestHttpMessageHandler(autoNegotiate: false);