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

Redirect on login and cookie handling #381

Merged
merged 13 commits into from Sep 2, 2022
Merged

Conversation

costrouc
Copy link
Member

No description provided.

@costrouc
Copy link
Member Author

Still a WIP and wondering how we might have a list of approved redirects.

@costrouc costrouc force-pushed the feat-redirect-on-login branch 4 times, most recently from 9c9d5f8 to 4427153 Compare August 22, 2022 23:02
@costrouc
Copy link
Member Author

costrouc commented Aug 22, 2022

Significant testing to get this working on browsers. I'm still not 100% sure I'm implementing this correctly.

@pierrotsmnrd I could use some help to test this on your end.

  1. Visit some page e.g. https://google.com

  2. Spin up the example/docker this configuration has two important settings cd example/docker; docker-compose up --build

c.CondaStoreServer.cors_allow_origins = []
c.CondaStoreServer.cors_allow_origin_regex = ".*"

This has been a fun journey getting cors + cookies + redirect to work together in username/password and oauth2 authentication.

Check that from the https://google.com in the console the following works

fetch("https://conda-store.localhost/conda-store/api/v1/permission/", {credentials: "include"}).then((r) => r.json()).then((d) => console.log(d.data.authenticated)) 
// should show false if haven't logged in
window.location = "https://conda-store.localhost/conda-store/login?next=https://google.com";
// should pass you through the login flow
fetch("https://conda-store.localhost/conda-store/api/v1/permission/", {credentials: "include"}).then((r) => r.json()).then((d) => console.log(d.data.authenticated)) 
// should now show true

Try the same with docker/standalone example. With the follow (only the urls have been changed).

fetch("http://localhost:5000/api/v1/permission/", {credentials: "include"}).then((r) => r.json()).then((d) => console.log(d.data.authenticated)) 
// should show false if haven't logged in
window.location = "http://localhost:5000/login?next=https://google.com";
// should pass you through the login flow
fetch("http://localhost:5000/api/v1/permission/", {credentials: "include"}).then((r) => r.json()).then((d) => console.log(d.data.authenticated)) 
// should now show true

I've been able to verify that this works for firefox and chrome.

A future PR will be to validate that the next parameter is in the allowed origins so that arbitrary urls like google.com do not work.

@costrouc
Copy link
Member Author

Well this is a bug in cypress:

The dilema is that I need to set secure=True on cookies otherwise chrome will not work (firefox will) due to https://developers.google.com/search/blog/2020/01/get-ready-for-new-samesitenone-secure.

Also the secure=True trips up aiohttp which enforces secure cookies a little too strictly. Which is worked around in this PR.

All this is to say cypress doesn't look like it is going to handle this. But this PR should be treated as passing.

@costrouc costrouc force-pushed the feat-redirect-on-login branch 2 times, most recently from 9abe406 to 82888b4 Compare August 23, 2022 05:48
@costrouc costrouc changed the title Redirect on login partial implementation Redirect on login and cookie handling Aug 23, 2022
@pierrotsmnrd
Copy link
Contributor

pierrotsmnrd commented Aug 24, 2022

Here are the results of my tests :

With examples/docker stack :

  • I always had to visit conda-store.localhost first at least once, to bypass the certificate validation error.
  • Chrome : I confirm it works in normal mode. It doesn't work in Incognito mode, I get false.
  • Firefox (v103.0.2) : the second fetch returns false :-/ Important note : when I run window.location = ... again, I get immediately redirected to the next url.
  • Safari (v15.2): exactly like firefox

with the standalone stack :

  • works on Chrome in normal mode, not in incognito mode
  • Firefox : doesn't work, I get false
  • Safari = the first fetch doesn't work, I get this error :
> fetch("http://localhost:5000/api/v1/permission/", {credentials: "include"}).then((r) => r.json()).then((d) => console.log(d.data.authenticated)) 
[Warning] [blocked] The page at https://www.google.com/?client=safari was not allowed to display insecure content from http://localhost:5000/api/v1/permission/.

[Error] Not allowed to request resource
	Évaluation de la console (Évaluation de la console 1:3)
	evaluateWithScopeExtension
	(fonction anonyme)
	_wrapCall
[Error] Fetch API cannot load http://localhost:5000/api/v1/permission/ due to access control checks.
	Évaluation de la console (Évaluation de la console 1:3)
	evaluateWithScopeExtension
	(fonction anonyme)
	_wrapCall
< Promise {status: "pending"}

On Firefox, with the standalone app, here is the full request of the second fetch :
Query header :

Host: localhost:5000
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:103.0) Gecko/20100101 Firefox/103.0
Accept: */*
Accept-Language: fr,fr-FR;q=0.8,en-US;q=0.5,en;q=0.3
Accept-Encoding: gzip, deflate, br
Referer: https://www.google.com/
Origin: https://www.google.com
DNT: 1
Connection: keep-alive
Sec-Fetch-Dest: empty
Sec-Fetch-Mode: cors
Sec-Fetch-Site: cross-site

Response header :

date: Wed, 24 Aug 2022 07:50:48 GMT
server: uvicorn
content-length: 224
content-type: application/json
access-control-allow-credentials: true
access-control-allow-origin: https://www.google.com
vary: Origin

I hope this confusing mess helps ...

@costrouc
Copy link
Member Author

costrouc commented Aug 25, 2022

Thanks for testing all this @pierrotsmnrd! So at best this is not a great solution... since we get unpredictable results. In fact other services like jupyterhub refuse to have crossorigin cookies. jupyterhub/jupyterhub#1087. Basically saying that for anything cross origin tokens are the way to go. We'll need to think on this and how this might be done.

@costrouc
Copy link
Member Author

costrouc commented Sep 2, 2022

Dropping the cookie functionally and only supporting samesite=strict. This is really the only secure option and the only way to ensure compatibility on all browsers. @pierrotsmnrd as long as the sites have the same hostname (port does not matter) testing should work.

@costrouc costrouc merged commit 25c5798 into main Sep 2, 2022
@costrouc costrouc deleted the feat-redirect-on-login branch September 2, 2022 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants