Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cookie based authentication issues aggregation #23185

Open
Return-1 opened this Issue Jan 28, 2019 · 14 comments

Comments

Projects
None yet
9 participants
@Return-1
Copy link

Return-1 commented Jan 28, 2019

Environment

[skip envinfo]

Reproducible Demo

Provided in corresponding issues

Description

Issues closed while still open. Cookie based authentication is at this moment not usable. This is partially due to the following issues:

  • #23005 //TL;DR can only ever work with one cookie on Android
  • #929 //TL;DR redirect: 'manual' doesnt work

These issues have been closed even though they are still open and very relevant.
There's more around cookies/fetch that i will try to hunt down in the following days. E.g one of the two platforms, i believe iOS , wont store cookies after app restart.

Conclusion

In general cookie based authentication is very problematic on multiple levels. If cookie based authentication is claimed implied to be supported on React Native and developers unknowingly structure their architecture around this these issues need attention. Otherwise people need to know before implementing a project using such an authentication mechanism as dozens of hours could be spend working on an architecture that is inevitably simply not supported.

This is not a matter of pointing fingers or demanding features. It is currently unfortunately misleading to leave people unaware of all these limitations as they might set out to create an architecture that's unsupported as i have.

At the very least maybe we should revise the documentation of fetch and explain how some things like "redirect:manual" dont work right now.

@react-native-bot

This comment was marked as resolved.

Copy link
Collaborator

react-native-bot commented Jan 28, 2019

Can you run react-native info and edit your issue to include these results under the Environment section?

If you believe this information is irrelevant to the reported issue, you may write [skip envinfo] under Environment to let us know.

@cirediew

This comment has been minimized.

Copy link

cirediew commented Jan 28, 2019

I have the same issue as mentioned in #23005.
Using the fetch functionality to do an authentication post returns a header with two cookies with the same name. On iOS these are concatenated. On Android only the second cookie is returned.

Example using postman:

set-cookie →token=XXXXXXXXXXXXXXXXXXXXXX; expires=Mon, 27-Jan-2020 12:45:43 GMT; Max-Age=31449600; Path=/

set-cookie →session=YYYYYYYYYYYYYYYYYYYYYYYYY; httponly; Path=/

Example RN iOS:
"set-cookie": "token=XXXXXXXXXXXXXXXXXXXXXX; expires=Mon, 27-Jan-2020 12:47:58 GMT; Max-Age=31449600; Path=/, session=YYYYYYYYYYYYYYYYYYYYYYYYY; httponly; Path=/"

Example RN Android:
"set-cookie": "session=YYYYYYYYYYYYYYYYYYYYYYYYY; httponly; Path=/"

React Native Environment Info:
System:
OS: Windows 10
CPU: (4) x64 Intel(R) Core(TM) i5-4460 CPU @ 3.20GHz
Memory: 4.83 GB / 15.68 GB
Binaries:
npm: 6.0.0 - C:\Program Files\nodejs\npm.CMD

@hey99xx

This comment has been minimized.

Copy link

hey99xx commented Jan 29, 2019

Yes, this is still a problem, and regarding your comment in other issue

means that it's a problem with the data structure used maybe?

you're absolutely right, it's a recently introduced bug in WritableNativeMap / WritableNativeArray classes. See #21795 (comment) #21795 (comment) #22064 (comment).

@kelset kelset changed the title Cookie based authentication issue aggregation + Issues got closed without resolution. Cookie based authentication issues aggregation Jan 29, 2019

@kelset

This comment has been minimized.

Copy link
Collaborator

kelset commented Jan 29, 2019

If cookie based authentication is claimed to be supported on React Native

can you point out to me where this "claim" is written in the documentation?

@Return-1

This comment has been minimized.

Copy link
Author

Return-1 commented Jan 29, 2019

Surely. On the documentation found here:
https://facebook.github.io/react-native/docs/network

React Native provides the Fetch API for your networking needs. Fetch will seem familiar if you have used XMLHttpRequest or other networking APIs before. You may refer to MDN's guide on Using Fetch for additional information.

However fetch does not support all the options as described at MDN. And since the only documentation provided on how to use it is MDN's site it follows that things like credentials:omit are supported by the networking layer.

@kelset

This comment has been minimized.

Copy link
Collaborator

kelset commented Jan 29, 2019

Then I guess we should add in the docs, where we link to fetch, that cookies are not fully supported?

I mean, there is no direct reference to cookies in the RN docs so this phrase

cookie based authentication is claimed to be supported on React Native

is a bit passive-aggressive I feel 😅

Have you tried using third-party libraries? Do they use fetch?

@Return-1

This comment has been minimized.

Copy link
Author

Return-1 commented Jan 29, 2019

@kelset i apologise, implied is definitely a better word for it.

I hold immense respect both for you ( i am familiar with your contributions ) and the rest of the team and as i've already solved this issue by migrating to a token based architecture it is only for the purposes of helping others that i'm bringing this up.

How would you suggest proceeding from here? I can go ahead and edit the documentation referencing the corresponding issues but i am unaware of the processes as upon resolution documentation should be re-revisited.

Thanks so much for attending to this. Hopefully i will find some time to contribute to this issue myself.

@kelset

This comment has been minimized.

Copy link
Collaborator

kelset commented Jan 29, 2019

I can go ahead and edit the documentation referencing the corresponding issues

As I mentioned above, probably the immediate step to be taken would be to clarify in the documentation that while we use fetch, there is not full support for cookies. And link to this issue as reference? Here's the link to the repo & file to change, if you can do a PR it would be lovely: https://github.com/facebook/react-native-website/blob/master/docs/network.md

In the meantime for now I've added the "known issues" label which should help framing this problem in the right perspective.

@Return-1

This comment has been minimized.

Copy link
Author

Return-1 commented Jan 29, 2019

Great, will proceed with that. There's more than cookies, for example a redirect cannot be omitted with redirect:manual. Im on it.

@rheng001

This comment has been minimized.

Copy link

rheng001 commented Mar 1, 2019

@Return-1

Hi, is this issue still being looked into? Thanks

@Return-1

This comment has been minimized.

Copy link
Author

Return-1 commented Mar 2, 2019

@rheng001 not actively by me, i did add some documentation which is merged warning readers against usage which should at least save people a few hours.

@nyilmaz

This comment has been minimized.

Copy link

nyilmaz commented Mar 4, 2019

@Return-1 is there a resolution plan for this issue in like 0.59? I also think this is a recent outstanding issue. There are a number of referenced issues which end up in here. It would be nice to see some progress (some commits mb) referenced from this issue.

@rheng001

This comment has been minimized.

Copy link

rheng001 commented Mar 16, 2019

I agree,

It would be appreciated to see some references of this being addressed.

@cpojer cpojer referenced this issue Mar 19, 2019

Closed

RN Issue Triage Group 3 #24030

47 of 160 tasks complete
@thymikee

This comment has been minimized.

Copy link
Collaborator

thymikee commented Mar 19, 2019

AFAIK nobody's currently working on it. We'd appreciate some movement around this issue though, so please keep the comments going and send PRs if possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.