[Foundation] Add CookieContainer support in NSUrlSessionHandler. by mandel-macaque · Pull Request #7654 · dotnet/macios

mandel-macaque · 2019-12-26T22:06:31Z

There are two important things to look at this:

That we do set the cookies that are present in the CookieContainer in
the request. That is, we need to set Cookie headers for all of them.
That if we receive a Set-Cookie from the server, the CookieContainer
gets correctly updated else we will have issues since we do not respect
the data sent from the server side.

Tests show both that we are setting the Cookie header and that we honour
the Set-Cookie header.

There are two important things to look at this: 1. That we do set the cookies that are present in the CookieContainer in the request. That is, we need to set Cookie headers for all of them. 2. That if we receive a Set-Cookie from the server, the CookieContainer gets correctly updated else we will have issues since we do not respect the data sent from the server side. Tests show both that we are setting the Cookie header and that we honour the Set-Cookie header. Fixes: dotnet#5665

monojenkins · 2019-12-27T00:53:04Z

Build failure
✅ Build succeeded
✅ API Diff (from stable)
ℹ️ API Diff (from PR only) (please review changes)
✅ Generator Diff (no change)
🔥 Test run failed 🔥

Test results

1 tests failed, 86 tests passed.

Failed tests

monotouch-test/watchOS 32-bits - simulator/Debug: Crashed

chamons

Love the test. I think you left in debug spam.

src/Foundation/NSUrlSessionHandler.cs

spouliot · 2019-12-27T14:31:09Z

src/Foundation/NSUrlSessionHandler.cs

+			{
+				var uri = new Uri (url.AbsoluteString);
+				if (sessionHandler.cookieContainer != null && cookies.Length > 0)
+					lock (sessionHandler.inflightRequestsLock) { // esure we lock when writing to the collection


minor: type eNsure

tests/monotouch-test/System.Net.Http/MessageHandlers.cs

spouliot

after CWL are removed

cosminstirbu · 2019-12-27T15:58:58Z

src/Foundation/NSUrlSessionHandler.cs

+			if (cookies != null && cookies.Count > 0) {
+				foreach (var cookie in cookies) {
+					Console.WriteLine ($"Adding cookie: {cookie.ToString ()} ");
+					request.Headers.TryAddWithoutValidation (Cookie, cookie.ToString ()); // as per docs: Returns a string representation of this Cookie object that is suitable for including in a HTTP Cookie: request header.


Not sure the standard allows having multiple Cookie headers.
I think the recommendation is to "join" all cookies into a single Cookie header using the ; character.

https://stackoverflow.com/questions/16305814/are-multiple-cookie-headers-allowed-in-an-http-request

We might want to use GetCookieHeader instead of GetCookies.

It looks that this is the approach that AndroidClientHandler.cs takes.

It is a very good point. And I won't have to deal with the specific Header details!

cosminstirbu · 2019-12-27T16:09:08Z

src/Foundation/NSUrlSessionHandler.cs

 			}
 		}

+		public CookieContainer CookieContainer {


AndroidClientHandler.cs also has a UseCookies property inherited from HttpClientHandler. Interestingly enough, NSUrlSessionHandler inherits from HttpMessageHandler instead :-?

I'd say the property is useful since it allows you to opt-out from this feature.

I can implement such a property BUT we should do it in a diff PR. The issue is that we also have to deal with the fact that iOS has its own 'CookieContainer' so if we set the property, we have to make sure that we do not use the native CookieContainer or the managed one. I'd like to keep the PR small since this code is complicated. Added an issue that I'll fix once we land this: #7659

1. Remove poor mans debugger CWL. 2. Fix manuel typos. 3. Get the cookies header directly from the cookie container. Less looping and easier code.

cosminstirbu · 2019-12-27T16:24:25Z

tests/monotouch-test/System.Net.Http/MessageHandlers.cs

+			Assert.IsNull (ex, "Exception");
+			Assert.IsNotNull (managedCookieResult, "Managed cookies result");
+			Assert.IsNotNull (nativeCookieResult, "Native cookies result");
+			Assert.AreEqual (managedCookieResult, nativeCookieResult, "Cookies");


My two cents - we should have an assertion on the generated NSUrlRequest that verifies the cookie header (name and value). This could be done by injecting the underlying NSUrlSession and capturing the parameter passed to CreateDataTask

Not sure if the current structure of the code allows this verification.

Or better yet - this test might be a better candidate for an integration test that uses some sort of test server - either in process (e.g. https://github.com/WireMock-Net/WireMock.Net) or a live server (e.g. https://httpbin.org/get)

The test is ensuring that the same cookies are sent when using the managed handler and the unmanaged. The request from httpbin.org returns a dictionary of cookies + values. So if they are equal, we should be fine.

cosminstirbu · 2019-12-27T16:25:11Z

tests/monotouch-test/System.Net.Http/MessageHandlers.cs

+			Assert.IsNotNull (nativeCookieResult, "Native cookies result");
+			var cookiesFromServer = cookieContainer.GetCookies (new Uri (url));
+			Assert.AreEqual (1, cookiesFromServer.Count, "Cookies received from server.");
+		}


It would worth checking the name and the value of the actual cookie as well.

Valid point. Merge and will update the test :)

monojenkins · 2019-12-27T16:30:14Z

Build failure
❌ Build failed or was aborted

✅ Build succeeded

monojenkins · 2019-12-27T20:11:12Z

Build failure
✅ Build succeeded
✅ API Diff (from stable)
ℹ️ API Diff (from PR only) (please review changes)
✅ Generator Diff (no change)
🔥 Test run failed 🔥

Test results

1 tests failed, 86 tests passed.

Failed tests

monotouch-test/watchOS 32-bits - simulator/Debug: Crashed

mandel-macaque · 2019-12-27T20:36:33Z

Known issue: https://github.com/xamarin/maccore/issues/581

mandel-macaque requested review from chamons, rolfbjarne and spouliot as code owners December 26, 2019 22:06

chamons approved these changes Dec 27, 2019

View reviewed changes

src/Foundation/NSUrlSessionHandler.cs Outdated Show resolved Hide resolved

src/Foundation/NSUrlSessionHandler.cs Outdated Show resolved Hide resolved

spouliot reviewed Dec 27, 2019

View reviewed changes

tests/monotouch-test/System.Net.Http/MessageHandlers.cs Outdated Show resolved Hide resolved

spouliot approved these changes Dec 27, 2019

View reviewed changes

cosminstirbu reviewed Dec 27, 2019

View reviewed changes

Address reviews:

f15dd11

1. Remove poor mans debugger CWL. 2. Fix manuel typos. 3. Get the cookies header directly from the cookie container. Less looping and easier code.

cosminstirbu reviewed Dec 27, 2019

View reviewed changes

Mino style issue.

8bb2ba7

mandel-macaque mentioned this pull request Dec 27, 2019

[Foundation] Allow to ignore the use of cookies in the NSUrlSessionHanlder #7659

Closed

mandel-macaque merged commit fffa889 into dotnet:master Dec 27, 2019

valentinba mentioned this pull request Oct 8, 2025

Lifecycle of NSUrlSessionHandler CookieContainer vs the underlying NsUrlSession NSHttpCookieStorage.SharedStorage #23950

Closed

Conversation

mandel-macaque commented Dec 26, 2019

Uh oh!

monojenkins commented Dec 27, 2019

Test results

Failed tests

Uh oh!

chamons left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

spouliot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cosminstirbu Dec 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cosminstirbu Dec 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monojenkins commented Dec 27, 2019

Uh oh!

monojenkins commented Dec 27, 2019

Test results

Failed tests

Uh oh!

mandel-macaque commented Dec 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cosminstirbu Dec 27, 2019 •

edited

Loading

cosminstirbu Dec 27, 2019 •

edited

Loading