android websocket: include cookies with request - #6851

Closed

srikanthkh wants to merge 2 commits into

react:masterfrom

srikanthkh:android-websocket-cookie

srikanthkh commented Apr 7, 2016

Contributor

Previously cookie headers weren't sending on websocket connection requests for android.
This commit fixes the issue.

facebook-github-bot commented Apr 7, 2016

Contributor

By analyzing the blame information on this pull request, we identified @aprock, @satya164 and @christopherdro to be potential reviewers.

facebook-github-bot added GH Review: review-needed CLA Signed labels

christopherdro reviewed

View reviewed changes

ReactAndroid/src/main/java/com/facebook/react/modules/websocket/WebSocketModule.java Outdated

christopherdroApr 7, 2016

Contributor

Nit: Alphabetize

christopherdro commented Apr 7, 2016

Contributor

Build is failing from import com.facebook.react.modules.network.ForwardingCookieHandler;
Not sure why it says it test says doesn't exist.

/cc @mkonicek Thoughts?

bestander commented Apr 7, 2016

Contributor

@srikanthkh, Buck build is failing.
With Buck we are very strict with dependencies being explicit and not cyclic.
You've added a new dependency on com.facebook.react.modules.network package that has its own BUCK file.
You need to add it as dependency in the websocket BUCK, refer to failing CircleCI script to see how to run it locally.
Thanks :)

srikanthkh force-pushed the android-websocket-cookie branch from 47763c8 to 0a0d5c4Compare

April 7, 2016 17:04

facebook-github-bot commented Apr 7, 2016

Contributor

@srikanthkh updated the pull request.

srikanthkh force-pushed the android-websocket-cookie branch from 0a0d5c4 to 6db13ecCompare

April 7, 2016 17:17

facebook-github-bot commented Apr 7, 2016

Contributor

@srikanthkh updated the pull request.

srikanthkh commented Apr 7, 2016

ContributorAuthor

@christopherdro & @bestander I have made the suggested changes. Please let me know if anymore changes are required. Thank you.

facebook-github-bot commented Apr 7, 2016

Contributor

@srikanthkh updated the pull request.

srikanthkh force-pushed the android-websocket-cookie branch from 5b3b997 to b5e3937Compare

April 7, 2016 17:22

facebook-github-bot commented Apr 7, 2016

Contributor

@srikanthkh updated the pull request.

srikanthkh force-pushed the android-websocket-cookie branch from 6e270f7 to 015b002Compare

April 7, 2016 18:12

facebook-github-bot commented Apr 7, 2016

Contributor

@srikanthkh updated the pull request.

android websocket: include cookies with request

2220e7f

srikanthkh force-pushed the android-websocket-cookie branch from 015b002 to 2220e7fCompare

April 7, 2016 18:35

facebook-github-bot commented Apr 7, 2016

Contributor

@srikanthkh updated the pull request.

srikanthkh commented Apr 7, 2016

ContributorAuthor

code was failing one of my tests when cookie was null, So just fixed it and re-pushed the code. Thank you

Merge branch 'master' into android-websocket-cookie

1a8637a

facebook-github-bot commented Apr 7, 2016

Contributor

@srikanthkh updated the pull request.

facebook-github-bot commented Apr 12, 2016

Contributor

By analyzing the blame information on this pull request, we identified @satya164, @zxcpoiu and @mkonicek to be potential reviewers.

ghost commented Apr 30, 2016

@satya164 would you mind taking a look at this pull request? It's been a while since the last commit was reviewed.

satya164 commented May 4, 2016

Contributor

@christopherdro fine with shipping this?

christopherdro commented May 4, 2016

Contributor

@satya164 👍 LGTM!

satya164 commented May 4, 2016

Contributor

@facebook-github-bot shipit

mkonicek commented May 9, 2016

Contributor

@srikanthkh The fix itself to include cookies in WebSocket requests is good, if you can send a new pull request we should get this in.

mkonicek commented May 9, 2016

Contributor

Note that the setDefaultOrigin method should be called getDefaultOrigin - it transforms a string into another string, but doesn't (and shouldn't) mutate anything. There's also an extra line after its Javadoc. I missed this in the code review, sorry about that!

christopherdro commented May 9, 2016

Contributor

@mkonicek Yea thats my bad for not looking it over properly. 😞

mkonicek commented May 9, 2016

Contributor

No worries @christopherdro! I totally understand there are a lot of pull requests to look at and the motivation to get this fix in quickly.

mkonicek commented May 9, 2016

Contributor

@srikanthkh Is there a method similar to getCookie already available elsewhere in the codebase, maybe in the networking module?

christopherdro commented May 9, 2016•
edited
Loading

Contributor

Re: setDefault was from a previous PR of mine. I can update that based on comments after we sort this one out.

ghost pushed a commit that referenced this pull request

Revert Android websocket: Include cookies with request

128d93b

Summary:
This reverts the commit bf8b549
Pull request: #6851
Internal Phabricator revision: D3257466
See the pull request for discussion.
Reviewed By: bestander
Differential Revision: D3277433
fbshipit-source-id: 623f93e1bce47ac156ffab154c57495b85ffa936

mkonicek commented May 9, 2016

Contributor

👍 Thanks @christopherdro.

mkonicek added Reverted and removed Reverted labels

This was referenced Jun 1, 2016

[Android] Unable to get cookie from the https://myapp.com/websocket inProgress-team/react-native-meteor#77

Closed

Websocket compatiblity v0.25 vs v0.26 #7871

Closed

[Android] Unable to get cookie from the https://myapp.com/websocket #7816

Closed

fakenickels commented Jun 3, 2016

Really needing this

salbito commented Jun 6, 2016

Any updates on this getting shipped soon?

christopherdro commented Jun 6, 2016

Contributor

It appears this PR was reverted but I am not sure why.

antoinerousseau commented Jun 6, 2016•
edited
Loading

Contributor

Cookies were being sent along in WebSockets with react-native 0.26 (OkHttp2) but not anymore with react-native 0.27 (OkHttp3).

@mkonicek: "I'll revert this now and let's do it again and properly." => Any ETA? I wish I could help but I know nothing about Android dev 😓

zxcpoiu commented Jun 7, 2016•
edited
Loading

Contributor

I think @mkonicek means:

This PR should fix coding conventions accoring to @mkonicek 's comments in commits.
This PR should at least have a test plan

Basically, the author should fulfill conditions above.
Given that the origin author may be busying and don't have time to do this,
if someone wanna help:

fork branch android-websocket-cookie from @srikanthkh 's repo( to keep the authorship )
help to fulfill conditions mentioned above
send a new PR which refers to this one

I can help the forking stuff and fix coding conventions, but I am not using this functionality, so don't know what is a proper test plan.
Are you interested to help the test plan part @antoinerousseau ?

antoinerousseau commented Jun 7, 2016

Contributor

@zxcpoiu well I'm not really good at tests and I'm not sure how to write a plan for this feature...

I suppose it would be like sending a first HTTP request which response includes a test cookie, and then connecting through a WebSocket at the same address than the HTTP request, and check on the server side if the test cookie is present... at least that's how I use it (my users are authenticated using an HttpOnly cookie set through an HTTPS request, and this auth must also be readable by the server when receiving WebSocket connections).

The test plan is described as:

Demonstrate the code is solid. Example: The exact commands you ran and their output, screenshots / videos if the pull request changes UI.
Make sure tests pass on both Travis and Circle CI.

antoinerousseau mentioned this pull request

Android WebSockets: include cookie in request #7989

Closed

zxcpoiu commented Jun 8, 2016

Contributor

@antoinerousseau has created a follow up PR #7989
Let's track there.

zebulgar pushed a commit to nightingale/react-native that referenced this pull request

android websocket: include cookies with request

9bf2bd2

Summary:
Previously cookie headers weren't sending on websocket connection requests for android.
This commit fixes the issue.
Closesreact#6851
Differential Revision: D3257466
fb-gh-sync-id: 4225d11c8c6efd09493ef938a65f024dcbaff749
fbshipit-source-id: 4225d11c8c6efd09493ef938a65f024dcbaff749

zebulgar pushed a commit to nightingale/react-native that referenced this pull request

Revert Android websocket: Include cookies with request

1d08bc6

Summary:
This reverts the commit react@bf8b549
Pull request: react#6851
Internal Phabricator revision: D3257466
See the pull request for discussion.
Reviewed By: bestander
Differential Revision: D3277433
fbshipit-source-id: 623f93e1bce47ac156ffab154c57495b85ffa936

antoinerousseau mentioned this pull request

Android WebSocket: include cookies in request #9114

Closed

facebook-github-bot pushed a commit that referenced this pull request

Android WebSocket: include cookies in request

be4afdd

Summary:
This PR updates #6851 from srikanthkh, fixing coding conventions and javadoc, and adding a test plan.
Added testing functions into the WebSocketExample page of the UIExplorer, including a tiny http server to set a cookie on demand. Instructions included in the UIExplorer app.
Closes#9114
Differential Revision: D4140534
Pulled By: lacker
fbshipit-source-id: e020ad0c6d1d3ea09c0c3564c1795b4e1bc4517d

This pull request was closed.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment