Skip to content
This repository has been archived by the owner on Dec 18, 2018. It is now read-only.

Add error to negotiate #2998

Merged
merged 3 commits into from
Sep 28, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -113,10 +113,13 @@ public HubConnection(String url, Transport transport, Logger logger, boolean ski
}
}

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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,15 @@ class NegotiateResponse {
private Set<String> availableTransports = new HashSet<>();
private String redirectUrl;
private String accessToken;
private String error;
private JsonParser jsonParser = new JsonParser();

public NegotiateResponse(String negotiatePayload) {
JsonObject negotiateResponse = jsonParser.parse(negotiatePayload).getAsJsonObject();
if (negotiateResponse.has("error")) {
this.error = negotiateResponse.get("url").getAsString();
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the parsing logic should exit early like this. You're right that an error property generally means everything else should be ignored, but let the caller decide that.

}
if (negotiateResponse.has("url")) {
this.redirectUrl = negotiateResponse.get("url").getAsString();
if (negotiateResponse.has("accessToken")) {
Expand Down Expand Up @@ -48,4 +53,8 @@ public String getRedirectUrl() {
public String getAccessToken() {
return accessToken;
}

public String getError() {
return error;
}
}
5 changes: 5 additions & 0 deletions clients/ts/signalr/src/HttpConnection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export interface INegotiateResponse {
availableTransports?: IAvailableTransport[];
url?: string;
accessToken?: string;
error?: string;
}

/** @private */
Expand Down Expand Up @@ -170,6 +171,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.");
}
Expand Down
20 changes: 20 additions & 0 deletions clients/ts/signalr/tests/HttpConnection.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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"] };
Expand Down
15 changes: 14 additions & 1 deletion specs/TransportProtocols.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ 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.

Expand Down Expand Up @@ -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 abort the connection attempt.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change abort to stop.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we need a protocol version here @anurse

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's not a bad idea. But do we want to do the two-part handshake that ASP.NET SignalR does? Where the client reports it's max supported version?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ERRR, I hope we don't need to change it that much.


```
{
"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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,9 +169,9 @@ internal HttpConnection(HttpConnectionOptions httpConnectionOptions, ILoggerFact
/// 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>
Expand Down Expand Up @@ -428,6 +428,10 @@ private async Task<NegotiationResponse> NegotiateAsync(Uri url, HttpClient httpC
{
negotiateResponse = NegotiateProtocol.ParseResponse(responseStream);
}
if (!string.IsNullOrEmpty(negotiateResponse.Error))
{
throw new InvalidOperationException(negotiateResponse.Error);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InvalidOperationException isn't really right here... We don't know anything about the underlying problem, it's no better than just throwing Exception.

}
Log.ConnectionEstablished(_logger, negotiateResponse.ConnectionId);
return negotiateResponse;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ public static class NegotiateProtocol
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";

Expand Down Expand Up @@ -99,6 +100,7 @@ public static NegotiationResponse ParseResponse(Stream content)
string url = null;
string accessToken = null;
List<AvailableTransport> availableTransports = null;
string error = null;

var completed = false;
while (!completed && JsonUtils.CheckRead(reader))
Expand Down Expand Up @@ -136,6 +138,9 @@ public static NegotiationResponse ParseResponse(Stream content)
}
}
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:
Expand All @@ -151,9 +156,9 @@ public static NegotiationResponse ParseResponse(Stream content)
}
}

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}'.");
Expand All @@ -170,7 +175,8 @@ public static NegotiationResponse ParseResponse(Stream content)
ConnectionId = connectionId,
Url = url,
AccessToken = accessToken,
AvailableTransports = availableTransports
AvailableTransports = availableTransports,
Error = error,
};
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,6 @@ public class NegotiationResponse
public string AccessToken { get; set; }
public string ConnectionId { get; set; }
public IList<AvailableTransport> AvailableTransports { get; set; }
public string Error { get; set; }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@
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;
Expand Down Expand Up @@ -380,6 +382,37 @@ public async Task StartSkipsOverTransportsThatDoNotSupportTheRequredTransferForm
});
}

[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 = "Negotiate failed."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Change this error message to something that isn't about negotiation. This seems like something that could be ambiguous

}));
});

using (var noErrorScope = new VerifyNoErrorsScope(expectedErrorsFilter: ExpectedError))
{
await WithConnectionAsync(
CreateConnection(testHttpHandler, loggerFactory: noErrorScope.LoggerFactory),
async (connection) =>
{
var exception = await Assert.ThrowsAsync<InvalidOperationException>(() => connection.StartAsync(TransferFormat.Text).OrTimeout());
Assert.Equal("Negotiate failed.", exception.Message);
});
}
}

private async Task RunInvalidNegotiateResponseTest<TException>(string negotiatePayload, string expectedExceptionMessage) where TException : Exception
{
var testHttpHandler = new TestHttpMessageHandler(autoNegotiate: false);
Expand Down