From 6853b8a3c9486880ffc20aca89d72f45f9282d1b Mon Sep 17 00:00:00 2001 From: Brennan Conroy Date: Mon, 28 Oct 2019 15:02:35 -0700 Subject: [PATCH 1/4] Don't access CookieContainer unless needed --- .../csharp/Http.Connections.Client/src/HttpConnection.cs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs b/src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs index 1fb9ba10aa66..a362de3ba06d 100644 --- a/src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs +++ b/src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs @@ -517,13 +517,14 @@ private HttpClient CreateHttpClient() { httpClientHandler.Proxy = _httpConnectionOptions.Proxy; } - if (_httpConnectionOptions.Cookies != null) + + // Only access HttpClientHandler.ClientCertificates and HttpClientHandler.CookieContainer + // if the user has configured client certs + // Some skews of Mono do not support client certs or cookies and will throw NotImplementedException + if (_httpConnectionOptions?.Cookies.Count > 0) { httpClientHandler.CookieContainer = _httpConnectionOptions.Cookies; } - - // Only access HttpClientHandler.ClientCertificates if the user has configured client certs - // Mono does not support client certs and will throw NotImplementedException // https://github.com/aspnet/SignalR/issues/2232 var clientCertificates = _httpConnectionOptions.ClientCertificates; if (clientCertificates?.Count > 0) From bd6438fec52a1831a119fd1723700741b95c29f2 Mon Sep 17 00:00:00 2001 From: Brennan Date: Mon, 28 Oct 2019 15:22:34 -0700 Subject: [PATCH 2/4] Apply suggestions from code review Co-Authored-By: Andrew Stanton-Nurse --- .../csharp/Http.Connections.Client/src/HttpConnection.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs b/src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs index a362de3ba06d..4da850b5736f 100644 --- a/src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs +++ b/src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs @@ -519,8 +519,8 @@ private HttpClient CreateHttpClient() } // Only access HttpClientHandler.ClientCertificates and HttpClientHandler.CookieContainer - // if the user has configured client certs - // Some skews of Mono do not support client certs or cookies and will throw NotImplementedException + // if the user has configured those options + // Some variants of Mono do not support client certs or cookies and will throw NotImplementedException if (_httpConnectionOptions?.Cookies.Count > 0) { httpClientHandler.CookieContainer = _httpConnectionOptions.Cookies; From 9b8e73a07abd941d6c87b7aea0bfd9b09eba62ed Mon Sep 17 00:00:00 2001 From: Brennan Date: Thu, 31 Oct 2019 16:25:32 -0700 Subject: [PATCH 3/4] Update src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs Co-Authored-By: Stephen Halter --- .../csharp/Http.Connections.Client/src/HttpConnection.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs b/src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs index 4da850b5736f..a407896f8cb1 100644 --- a/src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs +++ b/src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs @@ -521,7 +521,7 @@ private HttpClient CreateHttpClient() // Only access HttpClientHandler.ClientCertificates and HttpClientHandler.CookieContainer // if the user has configured those options // Some variants of Mono do not support client certs or cookies and will throw NotImplementedException - if (_httpConnectionOptions?.Cookies.Count > 0) + if (_httpConnectionOptions.Cookies?.Count > 0) { httpClientHandler.CookieContainer = _httpConnectionOptions.Cookies; } From 8538ba9e2bc5c45273c0f0685a7a1d8311e291dd Mon Sep 17 00:00:00 2001 From: Brennan Conroy Date: Thu, 31 Oct 2019 16:33:42 -0700 Subject: [PATCH 4/4] test --- .../csharp/Client/test/UnitTests/HttpConnectionTests.cs | 8 ++++++++ .../csharp/Http.Connections.Client/src/HttpConnection.cs | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/SignalR/clients/csharp/Client/test/UnitTests/HttpConnectionTests.cs b/src/SignalR/clients/csharp/Client/test/UnitTests/HttpConnectionTests.cs index a2fd1a61fd53..421af402d8bc 100644 --- a/src/SignalR/clients/csharp/Client/test/UnitTests/HttpConnectionTests.cs +++ b/src/SignalR/clients/csharp/Client/test/UnitTests/HttpConnectionTests.cs @@ -89,6 +89,14 @@ await WithConnectionAsync( Assert.Same(httpOptions.Credentials, httpClientHandler.Credentials); } + [Fact] + public void HttpOptionsCannotSetNullCookieContainer() + { + var httpOptions = new HttpConnectionOptions(); + Assert.NotNull(httpOptions.Cookies); + Assert.Throws(() => httpOptions.Cookies = null); + } + [Fact] public async Task HttpRequestAndErrorResponseLogged() { diff --git a/src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs b/src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs index a407896f8cb1..4375d11285cd 100644 --- a/src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs +++ b/src/SignalR/clients/csharp/Http.Connections.Client/src/HttpConnection.cs @@ -521,7 +521,7 @@ private HttpClient CreateHttpClient() // Only access HttpClientHandler.ClientCertificates and HttpClientHandler.CookieContainer // if the user has configured those options // Some variants of Mono do not support client certs or cookies and will throw NotImplementedException - if (_httpConnectionOptions.Cookies?.Count > 0) + if (_httpConnectionOptions.Cookies.Count > 0) { httpClientHandler.CookieContainer = _httpConnectionOptions.Cookies; }