Added Cookie Header to XML and Websocket request by clozr · Pull Request #10575 · react/react-native

clozr · 2016-10-27T01:25:13Z

Continuation of Pull Request #7167

Needed to clean my repository. So created this Pull Request

facebook-github-bot · 2016-10-27T01:26:18Z

By analyzing the blame information on this pull request, we identified @javache and @philikon to be potential reviewers.

lacker · 2016-10-29T00:29:22Z

Why is this the right behavior? It's not clear to me that these libraries should automatically use cookies.

FWIW #9114 is a similar pull request.

clozr · 2016-10-29T08:29:02Z

Fetch & Websocket API should be a drop-in replacement for the web version of the code.
In webapp, the http protocol handler will send any cookie for that domain when making websocket request. Same thing for any fetch request.
Besides, this is a very common pattern and if we go by DRY philosophy, it makes sense to support this pattern so that other users don't have to reinvent the wheel. I hope this clarifies.

lacker · 2016-11-07T17:29:04Z

I accepted #9114 for websockets since that had already been reviewed by a bunch of folks, so I think you will need to update this to just center on non websocket things.

clozr · 2016-11-07T21:31:22Z

@lacker I just took a quick look at #9114 and that seems to be android counterpart of this PR. We still need to do it for objective-c. However, since he has already written the test for this scenario we don't need a separate testcase, his testcase should be enough to test this one as well.

lacker · 2016-11-07T22:06:58Z

OK sounds good to me.

lacker · 2016-11-30T23:33:04Z

Hey, so I am unclear on the status of this, maybe I wasn't specific enough. With #9114 does this PR need to be updated, or do you think it's ready to go right now? In particular I am curious how we should test given the two different PRs that the behavior is the same cross-platform.

clozr · 2016-12-08T16:49:03Z

@lacker I reviewed the #9114, that PR fixes sending cookie header in websocket request for android websocket. This PR also does the same for IOS side. Since the test for #9114 was written in JS and platform agnostics, same test also works for this PR.
However this PR also fixes, saving and sending cookies in XHR requests on IOS as well.

lacker · 2016-12-08T19:40:59Z

Can you make this be tested automatically? It seems like that manual procedure in the other pull request, nobody is ever going to do that.

hramos · 2017-01-19T21:29:29Z

Ping @clozr, see earlier feedback.

clozr · 2017-01-21T19:36:29Z

To test automatically we will need a test mode callback to examine the headers being sent for websocket connect request. Give me couple of days to do it.

ycai2 · 2017-03-29T17:07:07Z

Is this PR ready to be merged? @clozr

GrigoryPtashko · 2017-04-26T17:03:49Z

I side up with everybody who's waiting for this feature. Are there any plans on when this PR is going to be merged?

shergin · 2017-04-26T17:41:50Z

I believe we should merge this.

facebook-github-bot · 2017-04-26T17:42:25Z

@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2017-04-27T05:48:17Z

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

facebook-github-bot · 2017-05-05T05:36:10Z

@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2017-05-05T05:36:48Z

@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

shergin · 2017-05-07T21:16:35Z

@clozr Looks like we have weird difficulties with merging this PR. Could you please rebase it on master?

clozr · 2017-05-08T02:04:49Z

Just updated the PR to facebook/master. Please let me know if you come across any problem

facebook-github-bot · 2017-05-08T16:01:40Z

@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

ycai2 · 2017-05-18T00:01:15Z

@clozr It seems like this PR can't be merged. Could you take a look at the build failure?

clozr · 2017-05-18T00:42:45Z

Looks like there were some unrelated issues. I have rebased it again. Let's see how the current build goes.

facebook-github-bot · 2017-05-18T05:36:31Z

@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

clozr · 2017-05-18T16:10:17Z

Looks Good now

shergin

I am soooory for late detailed review but code style should be fixed.

shergin · 2017-05-18T19:53:36Z

 callbackQueue.maxConcurrentOperationCount = 1;
 callbackQueue.underlyingQueue = [[_bridge networking] methodQueue];
 NSURLSessionConfiguration *configuration = [NSURLSessionConfiguration defaultSessionConfiguration];
+ [ configuration setHTTPShouldSetCookies:YES];


Please, fix codestyle across all changed lines. This diff cannot be merged without it (because of linter).

shergin · 2017-05-18T19:54:15Z

+ NSDictionary* headers = [RCTConvert NSDictionary:query[@"headers"]];
+ [headers enumerateKeysAndObjectsUsingBlock:^(NSString *key, id value, BOOL *stop) {
+ //request.allHTTPHeaderFields = [self stripNullsInRequestHeaders:[RCTConvert NSDictionary:query[@"headers"]]];
+ if(value) {


Please, add space.

shergin · 2017-05-18T19:54:30Z

+ // set supplied headers
+ NSDictionary* headers = [RCTConvert NSDictionary:query[@"headers"]];
+ [headers enumerateKeysAndObjectsUsingBlock:^(NSString *key, id value, BOOL *stop) {
+ //request.allHTTPHeaderFields = [self stripNullsInRequestHeaders:[RCTConvert NSDictionary:query[@"headers"]]];


Please remove this line.

shergin · 2017-05-18T19:55:36Z

+ // fetch). To get secure cookies for wss URLs, replace wss with https
+ // in the URL.
+ NSURLComponents *components = [NSURLComponents componentsWithURL:URL resolvingAgainstBaseURL:true];
+ if ([[components.scheme lowercaseString] isEqualToString:@"wss"]) {


Please, use dot notation for lowercaseString.

shergin · 2017-05-18T19:55:46Z

+ if ([[components.scheme lowercaseString] isEqualToString:@"wss"]) {
+ components.scheme = @"https";
+ }
+ // Load and set the cookie header.


Please add new line.

shergin · 2017-05-18T19:56:48Z

+ // Load and set the cookie header.
+ NSArray<NSHTTPCookie *> *cookies = [[NSHTTPCookieStorage sharedHTTPCookieStorage] cookiesForURL:URL];
+ request.allHTTPHeaderFields = [NSHTTPCookie requestHeaderFieldsWithCookies:cookies];
+


Please remove this line.

shergin · 2017-05-18T19:57:37Z

+ request.allHTTPHeaderFields = [NSHTTPCookie requestHeaderFieldsWithCookies:cookies];
+
+
+ // set supplied headers


Capital letter, full stop at the end.

shergin · 2017-05-18T19:58:00Z

+
+
+ // set supplied headers
+ NSDictionary* headers = [RCTConvert NSDictionary:query[@"headers"]];


NSDictionary *headers = ...

clozr · 2017-05-19T01:38:40Z

Fixed the lint errors. Let me know any more issues

facebook-github-bot · 2017-05-19T03:19:08Z

@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot added the GH Review: review-needed label Oct 27, 2016

clozr mentioned this pull request Oct 27, 2016
added cookie headers to websocket and XMLHttpRequest #7167
Closed

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 27, 2016

lacker mentioned this pull request Oct 29, 2016
Android WebSocket: include cookies in request #9114
Closed

lacker added the Core Team label Nov 5, 2016

clozr added GH Review: needs-revision and removed Core Team labels Nov 7, 2016

lacker self-assigned this Nov 7, 2016

somoso mentioned this pull request Dec 23, 2016
Cannot use websockets on iOS olofd/react-native-signalr#12
Closed

facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: needs-revision labels Apr 26, 2017

facebook-github-bot removed the Import Started This pull request has been imported. This does not imply the PR has been approved. label Apr 27, 2017

facebook-github-bot added the Import Failed label Apr 27, 2017

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label May 5, 2017

clozr force-pushed the network branch 3 times, most recently from 22a2a5e to d3b3c35Compare May 8, 2017 02:02

clozr force-pushed the network branch 2 times, most recently from 3b6849a to 15a9471Compare May 18, 2017 00:37

Added Cookie Header to XML and Websocket request
a4b663e

clozr force-pushed the network branch from 15a9471 to a4b663eCompare May 18, 2017 00:40

shergin suggested changes May 18, 2017
View reviewed changes

lint error fixes
f80ad95

shergin approved these changes May 19, 2017
View reviewed changes

facebook-github-bot closed this in 047961fMay 24, 2017

jamesreggio mentioned this pull request Jul 26, 2017
Fix withCredentials flag for fetch requests #14931
Closed

jamesreggio mentioned this pull request Sep 28, 2017
Restore platform-specific cookie behavior #16127
Closed

jamesreggio mentioned this pull request Jan 3, 2018
Cookies and authorization headers dropped when making HTTP requests that redirect [iOS] #15918
Closed

		request.allHTTPHeaderFields = [NSHTTPCookie requestHeaderFieldsWithCookies:cookies];


		// set supplied headers



		// set supplied headers
		NSDictionary* headers = [RCTConvert NSDictionary:query[@"headers"]];

Uh oh!

Conversation

clozr commented Oct 27, 2016

Uh oh!

facebook-github-bot commented Oct 27, 2016

Uh oh!

lacker commented Oct 29, 2016

Uh oh!

clozr commented Oct 29, 2016

Uh oh!

lacker commented Nov 7, 2016

Uh oh!

clozr commented Nov 7, 2016

Uh oh!

lacker commented Nov 7, 2016

Uh oh!

lacker commented Nov 30, 2016

Uh oh!

clozr commented Dec 8, 2016

Uh oh!

lacker commented Dec 8, 2016

Uh oh!

hramos commented Jan 19, 2017

Uh oh!

clozr commented Jan 21, 2017

Uh oh!

ycai2 commented Mar 29, 2017

Uh oh!

GrigoryPtashko commented Apr 26, 2017

Uh oh!

shergin commented Apr 26, 2017

Uh oh!

facebook-github-bot commented Apr 26, 2017

Uh oh!

facebook-github-bot commented Apr 27, 2017

Uh oh!

facebook-github-bot commented May 5, 2017

Uh oh!

facebook-github-bot commented May 5, 2017

Uh oh!

shergin commented May 7, 2017

Uh oh!

clozr commented May 8, 2017

Uh oh!

facebook-github-bot commented May 8, 2017

Uh oh!

ycai2 commented May 18, 2017

Uh oh!

clozr commented May 18, 2017

Uh oh!

facebook-github-bot commented May 18, 2017

Uh oh!

clozr commented May 18, 2017

Uh oh!

shergin 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!

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!

clozr commented May 19, 2017

Uh oh!

facebook-github-bot commented May 19, 2017

Uh oh!

Reviewers

Assignees

Labels