diff --git a/clients/java/signalr/src/main/java/com/microsoft/aspnet/signalr/HubConnection.java b/clients/java/signalr/src/main/java/com/microsoft/aspnet/signalr/HubConnection.java index 488ee28dc2..72abfea42a 100644 --- a/clients/java/signalr/src/main/java/com/microsoft/aspnet/signalr/HubConnection.java +++ b/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(); diff --git a/clients/java/signalr/src/main/java/com/microsoft/aspnet/signalr/NegotiateResponse.java b/clients/java/signalr/src/main/java/com/microsoft/aspnet/signalr/NegotiateResponse.java index d91a2ea4d3..f1d56a3fec 100644 --- a/clients/java/signalr/src/main/java/com/microsoft/aspnet/signalr/NegotiateResponse.java +++ b/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.JsonObject; -import com.google.gson.JsonParser; +import com.google.gson.stream.JsonReader; class NegotiateResponse { private String connectionId; private Set availableTransports = new HashSet<>(); private String redirectUrl; private String accessToken; - private JsonParser jsonParser = new JsonParser(); + private String error; - public NegotiateResponse(String negotiatePayload) { - JsonObject negotiateResponse = jsonParser.parse(negotiatePayload).getAsJsonObject(); - if (negotiateResponse.has("url")) { - this.redirectUrl = negotiateResponse.get("url").getAsString(); - if (negotiateResponse.has("accessToken")) { - this.accessToken = negotiateResponse.get("accessToken").getAsString(); + public NegotiateResponse(String negotiatePayload) throws IOException { + JsonReader reader = new JsonReader(new StringReader(negotiatePayload)); + reader.beginObject(); + + do { + 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; - } - this.connectionId = negotiateResponse.get("connectionId").getAsString(); - JsonArray transports = (JsonArray) negotiateResponse.get("availableTransports"); - for (int i = 0; i < transports.size(); i++) { - availableTransports.add(transports.get(i).getAsJsonObject().get("transport").getAsString()); - } + } while (reader.hasNext()); + + reader.endObject(); + reader.close(); } public String getConnectionId() { @@ -48,4 +88,8 @@ class NegotiateResponse { public String getAccessToken() { return accessToken; } + + public String getError() { + return error; + } } diff --git a/clients/java/signalr/src/test/java/com/microsoft/aspnet/signalr/NegotiateResponseTest.java b/clients/java/signalr/src/test/java/com/microsoft/aspnet/signalr/NegotiateResponseTest.java index ec2d4180ca..c4c64a9052 100644 --- a/clients/java/signalr/src/test/java/com/microsoft/aspnet/signalr/NegotiateResponseTest.java +++ b/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()); + } } diff --git a/clients/ts/signalr/src/HttpConnection.ts b/clients/ts/signalr/src/HttpConnection.ts index 1dbe24b846..2d4653aff1 100644 --- a/clients/ts/signalr/src/HttpConnection.ts +++ b/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."); } diff --git a/clients/ts/signalr/tests/HttpConnection.test.ts b/clients/ts/signalr/tests/HttpConnection.test.ts index bc7ed98e50..a1bb4e71c5 100644 --- a/clients/ts/signalr/tests/HttpConnection.test.ts +++ b/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"] }; diff --git a/specs/TransportProtocols.md b/specs/TransportProtocols.md index 4a932037f1..92ea787e70 100644 --- a/specs/TransportProtocols.md +++ b/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. diff --git a/src/Microsoft.AspNetCore.Http.Connections.Client/HttpConnection.cs b/src/Microsoft.AspNetCore.Http.Connections.Client/HttpConnection.cs index e1b4a9e373..e45d6f70a0 100644 --- a/src/Microsoft.AspNetCore.Http.Connections.Client/HttpConnection.cs +++ b/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. /// - public async Task StartAsync(CancellationToken cancellationToken = default) + public Task StartAsync(CancellationToken cancellationToken = default) { - await StartAsync(TransferFormat.Binary, cancellationToken); + return StartAsync(TransferFormat.Binary, cancellationToken); } /// @@ -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; } diff --git a/src/Microsoft.AspNetCore.Http.Connections.Common/NegotiateProtocol.cs b/src/Microsoft.AspNetCore.Http.Connections.Common/NegotiateProtocol.cs index f0c88ecdb1..49b3cc4336 100644 --- a/src/Microsoft.AspNetCore.Http.Connections.Common/NegotiateProtocol.cs +++ b/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 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, }; } } diff --git a/src/Microsoft.AspNetCore.Http.Connections.Common/NegotiationResponse.cs b/src/Microsoft.AspNetCore.Http.Connections.Common/NegotiationResponse.cs index 1d87b5e19f..02293bdc40 100644 --- a/src/Microsoft.AspNetCore.Http.Connections.Common/NegotiationResponse.cs +++ b/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 AvailableTransports { get; set; } + public string Error { get; set; } } } \ No newline at end of file diff --git a/test/Microsoft.AspNetCore.SignalR.Client.Tests/HttpConnectionTests.Negotiate.cs b/test/Microsoft.AspNetCore.SignalR.Client.Tests/HttpConnectionTests.Negotiate.cs index 8b0a90b148..89893a7e19 100644 --- a/test/Microsoft.AspNetCore.SignalR.Client.Tests/HttpConnectionTests.Negotiate.cs +++ b/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(() => connection.StartAsync(TransferFormat.Text).OrTimeout()); + Assert.Equal("Test error.", exception.Message); + }); + } + } + private async Task RunInvalidNegotiateResponseTest(string negotiatePayload, string expectedExceptionMessage) where TException : Exception { var testHttpHandler = new TestHttpMessageHandler(autoNegotiate: false);