Negotiate + SignalR Service Support for the Java client #2882

mikaelm12 · 2018-08-28T19:37:36Z

Added support for the negotiation dance between the client and server and also added support for the SignalR service.

Was originally going to make these two different PRs but I really wanted to see the Java client working with the service 😄
Issue: #2871
and issue: #2862

Need to add tests

mikaelm12 · 2018-08-28T19:38:10Z

clients/java/signalr/build.gradle

@@ -18,6 +18,7 @@ dependencies {
    testImplementation group: 'junit', name: 'junit', version: '4.12'
    implementation "org.java-websocket:Java-WebSocket:1.3.8"
    implementation 'com.google.code.gson:gson:2.8.5'
+    compile 'org.apache.httpcomponents:httpclient:4.5.6'


New dependency for an implementation of an HttpClient

This has been approved 👍

Looks like compile has been deprecated and implementation should be used instead (like the other dependencies)

mikaelm12 · 2018-08-28T19:38:51Z

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


-    public HubConnection(String url, Transport transport, Logger logger){
+    private static int MAX_NEGOTIATE_ATTEMPTS = 5;


Just arbitrarily put 5 for the max. I think it's much higher for TS

It's 100 in .NET: https://github.com/aspnet/SignalR/blob/release/2.2/src/Microsoft.AspNetCore.Http.Connections.Client/HttpConnection.cs#L28

mikaelm12 · 2018-08-28T19:40:25Z

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

+                if(negotiateAttempts > 0 && this.negotiateResponse.accessToken != null){
+                    this.accessToken = negotiateResponse.accessToken;
+                    this.negotiateResponse = Negotiate.processNegotiate(url, this.negotiateResponse.accessToken);
+                    this.url = this.url + "&id=" + negotiateResponse.connectionId + "&access_token=" + this.accessToken;


Should I just store the accessToken in the Authorization header instead?

This will just keep appending &id and &access_token as the client keeps getting redirected. I'm not sure the logic needs to be different based on which attempt we're on. The loop in the C# client
doesn't really do that. The initial URL and initial Access Token aren't relevant at all once the negotiate response comes in (#2859 notwithstanding, but we should come back to that)

BrennanConroy

Tests?

BrennanConroy · 2018-08-28T19:43:50Z

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

@@ -22,8 +22,13 @@
    private HubConnectionState connectionState = HubConnectionState.DISCONNECTED;
    private Logger logger;
    private List<Consumer<Exception>> onClosedCallbackList;
+    private boolean skipNegotiate = true;


false by default

BrennanConroy · 2018-08-28T19:44:55Z

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

@@ -30,10 +31,15 @@ public HubConnectionBuilder configureLogging(Logger logger) {
        return this;
    }

+    public HubConnectionBuilder skipNeotiate(boolean skip) {


Remove this, add it in a different PR when withUrl takes options

BrennanConroy · 2018-08-28T19:46:20Z

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

-    public HubConnection(String url, Transport transport, Logger logger){
+    private static int MAX_NEGOTIATE_ATTEMPTS = 5;
+
+    public HubConnection(String url, Transport transport, Logger logger, boolean skipNegotiate){


Remove this overload

BrennanConroy · 2018-08-28T21:09:41Z

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

+
+    public static NegotiateResponse processNegotiate(String url, String accessTokenHeader) throws IOException {
+        HttpClient client = HttpClientBuilder.create().build();
+        url = url.substring(0, url.indexOf('?')) + "negotiate" + url.substring(url.indexOf('?'));


This assumes the url ends with '/'

Is there some library code you can use instead of trying to correctly parse urls?

BrennanConroy · 2018-08-28T21:11:17Z

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

+
+    public static NegotiateResponse processNegotiate(String url) throws IOException {
+        HttpClient client = HttpClientBuilder.create().build();
+        HttpPost post = new HttpPost(url + "/negotiate");


This assumes there is no query string present

BrennanConroy · 2018-08-28T21:13:11Z

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

+    public NegotiateResponse(String negotiatePayload) {
+        JsonObject negotiateResponse= jsonParser.parse(negotiatePayload).getAsJsonObject();
+        if (negotiateResponse.has("url")) {
+            this.shouldRedirect = true;


unneeded, just check if the redirectUrl is non-empty

BrennanConroy · 2018-08-28T21:13:42Z

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

+            return;
+        }
+        this.connectionId = negotiateResponse.get("connectionId").getAsString();
+        negotiateResponse.get("availableTransports");


This was debugging

BrennanConroy · 2018-08-28T21:15:47Z

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


    public WebSocketTransport(String url, Logger logger) throws URISyntaxException {
        this.url = formatUrl(url);
        this.logger = logger;
+        if (url.indexOf(ACCESS_TOKEN_PREFIX) > 0){
+            this.accessToken = url.substring(url.indexOf(ACCESS_TOKEN_PREFIX) + ACCESS_TOKEN_PREFIX.length());


What is this for?

This was for when I was storing the accessToken and then setting it in the headers. I removed that code but forgot this. But looks like I'll be going back down that route

This looks like it should be removed.

davidfowl · 2018-08-29T04:14:53Z

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

+
+        HubConnection hubConnection = new HubConnectionBuilder()
+                .withUrl(input)
+                .skipNeotiate(false)


This shouldn’t be on by default

davidfowl · 2018-08-29T04:15:38Z

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

+                }
+                negotiateAttempts++;
+            } while (this.negotiateResponse.shouldRedirect && negotiateAttempts < MAX_NEGOTIATE_ATTEMPTS);
+            if(!negotiateResponse.availableTransports.contains("WebSockets")){


Formatting...

davidfowl · 2018-08-29T08:24:34Z

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

+        url = url.substring(0, url.indexOf('?')) + "negotiate" + url.substring(url.indexOf('?'));
+        HttpPost post = new HttpPost(url);
+        post.setHeader("Authorization", "Bearer " + accessTokenHeader);
+        HttpResponse response = client.execute(post);


Does this http client support futures?

It looks like there's an Async flavor of this HttpClient https://hc.apache.org/httpcomponents-asyncclient-ga/httpasyncclient/apidocs/org/apache/http/nio/client/HttpAsyncClient.html
I can update the Async issue to note that we should be using this once we start using CompletableFuture

mikaelm12 · 2018-08-30T00:53:04Z

So I was testing this on Android and it turns out that the Apache HttpClient library isn't supported.
https://developer.android.com/about/versions/marshmallow/android-6.0-changes#behavior-apache-http-client

I'm looking into using Squares OkHttp Library as a replacement.

davidfowl · 2018-08-30T01:07:07Z

OkHttp is the pretty common android replacement. Grpc uses it https://github.com/grpc/grpc-java/tree/master/okhttp

mikaelm12 · 2018-08-30T04:29:41Z

OkHttp is the pretty common android replacement.

Yeah, should have just started there. Hindsight is 20/20 I guess.

davidfowl · 2018-08-30T05:34:41Z

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

+        return result.toString();
+    }
+
+    private static String resolveNegotiateUrl(String url){


nit: the formatting in this method

davidfowl · 2018-08-30T06:59:27Z

clients/java/signalr/build.gradle

@@ -19,6 +19,7 @@ dependencies {
    implementation "org.java-websocket:Java-WebSocket:1.3.8"
    implementation 'com.google.code.gson:gson:2.8.5'
    implementation 'org.apache.httpcomponents:httpclient:4.5.6'
+    implementation 'com.squareup.okhttp3:okhttp:3.11.0'


We need to do an OSS request

cc @Eilon

Already sent an email before updating the PR

davidfowl · 2018-08-30T07:42:45Z

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

+
+        // Check if we have a query string. If we do then we ignore it for now.
+        int queryStringIndex = url.indexOf('?');
+        if(queryStringIndex > 0){


you have what I call @anurse virus. The if( always touching 😄

davidfowl · 2018-08-30T07:48:52Z

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

+            return;
+        }
+        this.connectionId = negotiateResponse.get("connectionId").getAsString();
+        JsonArray transports = ((JsonArray)negotiateResponse.get("availableTransports"));


nit: Get rid of the extra parenthesis

mikaelm12 · 2018-08-30T18:22:30Z

😄

mikaelm12 · 2018-08-30T21:44:03Z

Okay, this now works on Android and I no longer store the accessToken in the url. Moved to headers. And the formatting should be good too

mikaelm12 · 2018-08-30T22:47:28Z

Updated

davidfowl · 2018-08-31T05:48:04Z

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

            url = WS + url.substring(HTTP.length());
        }
+        if (url.indexOf(ACCESS_TOKEN_PREFIX) > 0) {


Why does this exist?

We parse the access token from the url so we can put it in a header when we construct the underlying WebSocketClient. We actually know the accessToken before we even contruct the transport. I can add a ctor that takes headers and get rid of this

That would be much cleaner.

davidfowl · 2018-09-01T02:14:56Z

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

            this.transport = transport;
        }
    }

+    public HubConnection(String url, Transport transport, Logger logger) {


Why did you add 2 constructors?

For testing. Once I add the skipNegotiate option to the builder I can clean this up

davidfowl · 2018-09-01T02:17:03Z

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

+    public static NegotiateResponse processNegotiate(String url, String accessTokenHeader) throws IOException {
+        url = resolveNegotiateUrl(url);
+        OkHttpClient client = new OkHttpClient();
+        RequestBody body = RequestBody.create(null, new byte[]{});


What are the create arguments?

Why aren’t we copying all of the headers here?

What are the create arguments?

create(MediaType contentType, byte[] content)

Why aren’t we copying all of the headers here?

This seems to be the way we do this in the TS client

It's a little hidden, but we do copy all the headers in the JavaScript client:

SignalR/clients/ts/signalr/src/HttpClient.ts

Lines 184 to 190 in 201c058

const headers = request.headers;

if (headers) {

Object.keys(headers)

.forEach((header) => {

xhr.setRequestHeader(header, headers[header]);

});

}

mikaelm12 · 2018-09-04T18:33:37Z

Ping

BrennanConroy

There are no tests that check the redirect url behavior.

BrennanConroy · 2018-09-04T20:26:59Z

clients/java/signalr/src/test/java/NegotiateResponseTest.java

+public class NegotiateResponseTest {
+
+    @Test
+    public void VerifyNegoitateResponse() {


nit: Negotiate

BrennanConroy · 2018-09-04T20:27:07Z

clients/java/signalr/src/test/java/NegotiateResponseTest.java

+    }
+
+    @Test
+    public void VerifyRedirectNegoitateResponse() {


nit: Negotiate

BrennanConroy · 2018-09-04T20:31:03Z

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

            onClosedCallbackList = new ArrayList<>();
        }

        onClosedCallbackList.add(callback);
    }
+
+    public void setSkipNegotiate(boolean skipNegotiate) {


mikaelm12 · 2018-09-04T22:42:36Z

There are no tests that check the redirect url behavior.

I wanted to add this but this requires adding infrastructure to allow passing in custom HttpClients for testing. That should happen eventually but not in this PR imo. Trying to stop this PR from growing any more

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

BrennanConroy · 2018-09-05T16:11:49Z

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

+        return new NegotiateResponse(result);
+    }
+
+    private static String resolveNegotiateUrl(String url) {


Add some tests for this please. See https://github.com/aspnet/SignalR/blob/release/2.2/clients/ts/signalr/tests/Common.ts#L17 and https://github.com/aspnet/SignalR/blob/release/2.2/clients/ts/signalr/tests/HttpConnection.test.ts#L220 for some reference

Good point. 👍

BrennanConroy · 2018-09-05T16:16:42Z

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

+        if (!skipNegotiate) {
+            int negotiateAttempts = 0;
+            do {
+                this.accessToken = (this.negotiateResponse == null) ? null : this.negotiateResponse.getAccessToken();


this this this

analogrelay · 2018-09-05T16:41:34Z

Formatting changes made a bunch of noise in HubConnection unfortunately :(

mikaelm12 · 2018-09-05T22:47:39Z

Final reviews?

BrennanConroy

We need to be consistent with our use of this

BrennanConroy · 2018-09-05T23:46:52Z

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

+        return new NegotiateResponse(result);
+    }
+
+    public static String resolveNegotiateUrl(String url) {


We'll need to look at the public API later

mikaelm12 added 3 commits August 28, 2018 00:27

Got things working. Clean up next

a4c0a0d

skipNegotiate flag + cleanup

2f186df

Clean up processNegotiate

2049b06

mikaelm12 commented Aug 28, 2018

View reviewed changes

BrennanConroy reviewed Aug 28, 2018

View reviewed changes

Some pr comments

d55530d

davidfowl reviewed Aug 29, 2018

View reviewed changes

mikaelm12 added 2 commits August 29, 2018 11:11

negotiate by default

64a7cec

Process negotiate url

f10686e

davidfowl reviewed Aug 30, 2018

View reviewed changes

OkHttp

5f52439

davidfowl reviewed Aug 30, 2018

View reviewed changes

mikaelm12 added 2 commits August 29, 2018 23:59

clean up

27812b3

;

f54e031

davidfowl reviewed Aug 30, 2018

View reviewed changes

mikaelm12 added 2 commits August 30, 2018 14:05

Format

593b2ea

Store access token in header instead of url

ea3f660

mikaelm12 added 2 commits August 30, 2018 15:00

+ NegotiateResponse tests and Getters

9493dc3

Fix redirect loop

ac509f4

davidfowl reviewed Aug 31, 2018

View reviewed changes

Pass in header map to WebSockets transport

c300b76

davidfowl reviewed Sep 1, 2018

View reviewed changes

BrennanConroy reviewed Sep 4, 2018

View reviewed changes

Pr comment

11addb6

BrennanConroy reviewed Sep 5, 2018

View reviewed changes

mikaelm12 added 2 commits September 5, 2018 12:56

+ resolveNegoitateUrl tests

a86f7ac

remove this

a7990fa

BrennanConroy approved these changes Sep 5, 2018

View reviewed changes

mikaelm12 merged commit dc6088b into release/2.2 Sep 5, 2018

BrennanConroy deleted the mikaelm12/Negotiate branch September 6, 2018 17:18


		public HubConnection(String url, Transport transport, Logger logger){
		private static int MAX_NEGOTIATE_ATTEMPTS = 5;

	const headers = request.headers;
	if (headers) {
	Object.keys(headers)
	.forEach((header) => {
	xhr.setRequestHeader(header, headers[header]);
	});
	}

Negotiate + SignalR Service Support for the Java client #2882

Negotiate + SignalR Service Support for the Java client #2882

Conversation

mikaelm12 commented Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrennanConroy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikaelm12 commented Aug 30, 2018

davidfowl commented Aug 30, 2018

mikaelm12 commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikaelm12 commented Aug 30, 2018

mikaelm12 commented Aug 30, 2018

mikaelm12 commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikaelm12 commented Sep 4, 2018

BrennanConroy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikaelm12 commented Sep 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

analogrelay commented Sep 5, 2018

mikaelm12 commented Sep 5, 2018

BrennanConroy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikaelm12 commented Aug 28, 2018 •

edited

Loading