[Android] Implement cookies #3723
Conversation
|
By analyzing the blame information on this pull request, we identified @mkonicek to be a potential reviewer. |
facebook-github-bot
added
GH Review: review-needed
CLA Signed
|
How have you tested this? Can you extend an example in the UI Explorer showing how this works? |
|
Do you have ideas how I can show how it works? In my project I tested this on authentication on internal site using fetch API. |
|
Ideally you should come up with an exhaustive set of examples showing this implements the Same origin policy: https://developer.mozilla.org/en-US/docs/Web/Security/Same-origin_policy There should be an example in the UI Explorer demonstrating this, here are instructions on how to run it: |
|
Does SOP make sense here? What is the value of the origin? |
|
I believe this is to fix #2792 Based the discussion there it looks like we should honor |
|
@mkonicek I added example, please review |
|
@nucleartux updated the pull request. |
|
@ide I'm not sure I understand - in the browser, XHR respects the same origin policy, correct? @nucleartux Thanks for the example. I think we really need to demonstrate here that the implementation respects the same origin policy. I'm not sure I think you'd need to send a request and get back cookies, then send another request to the same domain and demonstrate those cookies were sent as part of the second request. Then you'd send a third request to a different domain (and ideally to a multiple of them) and demonstrate the cookies were not sent. This should really be an integration test and we'll work on open sourcing those so you can add them, but an example would be a good start. Note I'll be away for a week and will continue reviewing when I'm back. |
|
@mkonicek the same-origin policy compares the current page's origin with the origin of the request endpoint. If google.com does a fetch request to google.com, then Anyway, with RN the app doesn't have an origin, so I don't know what it means to enforce the SOP. There are just two settings: either send the cookies or don't. (maybe there's a third option of whether to send cookies if there's a 3xx redirect?) |
|
I think there may be some confusion -- when an XHR makes a request to facebook.com, it never should send the google.com cookies under any circumstance (this is separate from the SOP). |
|
@nucleartux updated the pull request. |
|
@nucleartux updated the pull request. |
|
I have read RFCs and I think @mkonicek is right. Changed policy to more secure ACCEPT_ORIGINAL_SERVER. |
|
+1 |
Fix XHR example
|
@nucleartux updated the pull request. |
|
@nucleartux updated the pull request. |
Yes this is what I meant by "SOP", sorry about the confusion. @nucleartux Can you verify this is the case? You mentioned:
But don't see this change. |
|
ACCEPT_ORIGINAL_SERVER sounds like it prevents Facebook.com from setting cookies for Google.com (also important!!). I'm not sure if there is an explicit setting to tell the client to send only the Facebook.com cookies to Facebook.com. We should verify but the http client might always do that by default. |
|
@mkonicek this is default policy |
|
I've been working on this internally but I think we want to merge this just to have some support. As long as the policy is ACCEPT_ORIGINAL_SERVER |
| @@ -45,6 +48,7 @@ | |||
| private static final String USER_AGENT_HEADER_NAME = "user-agent"; | |||
|
|
|||
| private final OkHttpClient mClient; | |||
| private final CookieManager mCookieManager; | |||
There was a problem hiding this comment.
You can skip this field and just construct in inline. mClient.setCookieHandler(new CookieManager());
|
@nucleartux updated the pull request. |
|
Is there any chance that this PR would be in 0.15 release? |
|
@sooth-sayer it will likely go out with 0.16. |
IMHO it's always better to be explicit, you clearly state the intention and defaults can change. |
|
@lexs Thanks for reviewing this PR! |
|
@nucleartux updated the pull request. |
|
@nucleartux updated the pull request. |
|
+1 - Any updates on the progress of this? I'm working on a project that requires the use of session to use the api. I would love to use react-native but without this feature it can't really be done. |
|
ping @mkonicek |
|
👍 another vote for this, as it's an instant show stopper when trying to run my iOS apps on Android as all are based on Rails API servers with cookie sessions. |
|
Sorry for the delay - I have an update: @lexs started working on a different approach in the meantime - one that shares the cookies between okhttp and the Android WebView. |
|
Closing this out as 274c5c7 just landed. Thanks for you effort @nucleartux. Hopefully this fixes your issue otherwise feel free to open another issue. |
|
Will applying this fix mean that |
|
@6i6arka For 274c5c7 yes. With this PR yes, but only in-memory. |
|
is cookies can be used in android? |
No description provided.