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

Support for concurrent authentication (multiple states and origins) #103

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

pierre-pretorius
Copy link

@pierre-pretorius pierre-pretorius commented Mar 1, 2017

Consider a scenario where you only allow your users to authenticate against a single oauth2 provider such as Google. With this scenario it's common not to have a login page and just immediately attempt authentication against the oauth2 provider. If the user starts up his browser and restores multiple tabs of your application, all these tabs redirect to the provider login screen. If the user then signs in on each tab he gets a CSRF error on all except the last tab that opened on your application callback because this gem only allows one state parameter in the session. Furthermore it doesn't redirect back to the origin correctly because it only stores one origin as well. This pull request stores multiple state parameters in the session and an origin for each of the states.

@pierre-pretorius pierre-pretorius changed the title Multiple origins Support concurrent sign ins (multiple states and origins) Mar 1, 2017
@pierre-pretorius pierre-pretorius changed the title Support concurrent sign ins (multiple states and origins) Support for concurrent authentication (multiple states and origins) Mar 1, 2017
@coveralls
Copy link

coveralls commented Mar 1, 2017

Coverage Status

Coverage decreased (-6.4%) to 78.571% when pulling 24b847d on pierre-pretorius:multiple_origins into 464fcef on intridea:master.

1 similar comment
@coveralls
Copy link

coveralls commented Mar 1, 2017

Coverage Status

Coverage decreased (-6.4%) to 78.571% when pulling 24b847d on pierre-pretorius:multiple_origins into 464fcef on intridea:master.

@coveralls
Copy link

coveralls commented Mar 2, 2017

Coverage Status

Coverage decreased (-6.4%) to 78.571% when pulling e246fa9 on pierre-pretorius:multiple_origins into 464fcef on intridea:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-6.4%) to 78.571% when pulling e246fa9 on pierre-pretorius:multiple_origins into 464fcef on intridea:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-6.4%) to 78.571% when pulling e246fa9 on pierre-pretorius:multiple_origins into 464fcef on intridea:master.

@justinhoward
Copy link

This is related to #95 and also seems like a duplicate of #88 and #75. Anyway, a lot of people seem interested in a fix for these issues.

  • Allow authorization after user clicks the back button, returning to the provider login form (multiple use of the same state).
  • Allow simultaneous login from multiple providers or multiple tabs (multiple simultaneous states).

@renchap
Copy link

renchap commented Jan 9, 2018

This will fix a lot of issues related to the state CSRF mechanism. It is good to have it here to prevent CSRF attempts but the current implementation is way to strict.
I would love to have this PR (or one of the mentionned above) merged.

is it anything I can do to get it merged @tmilewski ?

@tmilewski
Copy link
Member

I'll have to take a look through all of this later.

That said, any help would be greatly appreciated! At first glance, the big things, at this point, would be adding specs and getting it to pass CI.

@senny
Copy link

senny commented May 15, 2023

Just wanted to drop a quick note that we are seeing exactly the issue described in the PR. We use a single oauth provider for authentication and do indeed trigger authentication right away. When two or more tabs are going through authentication at the same time, csrf_detected errors are happening.

Any chances to get this patch finalized?

@xhyrom
Copy link

xhyrom commented Jul 24, 2024

Is this gonna happen or should i cherry pick?

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.

7 participants