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

Adding authenticity token so session will persist #674

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@beerlington

beerlington commented Mar 27, 2013

This fixes issue #670
When using the developer login strategy sessions are not being persisted
in Rails 4.0.0.beta1 because the authenticity token is not present when
submitting the form. By adding the CSRF token to the form as a hidden
field, it can be verified, and sessions work properly.

beerlington added some commits Mar 27, 2013

Adding authenticity token so session will persist
When using the developer login strategy sessions are not being persisted
in Rails 4.0.0.beta1 because the authenticity token is not present when
submitting the form. By adding the CSRF token to the form as a hidden
field, it can be verified, and sessions work properly.
@sferik

This comment has been minimized.

Show comment
Hide comment
@sferik

sferik Apr 8, 2013

Contributor

Not sure how I feel about adding Rails-specific behavior to the form.

@mbleigh What are your thoughts about this?

Contributor

sferik commented Apr 8, 2013

Not sure how I feel about adding Rails-specific behavior to the form.

@mbleigh What are your thoughts about this?

@beerlington

This comment has been minimized.

Show comment
Hide comment
@beerlington

beerlington Apr 8, 2013

@sferik I sort of felt the same way writing it, but by the time I figured out what the actual issue was, I just wanted to get it working. 😀 I think another solution is to just add skip_before_filter :verify_authenticity_token to the session controller since the token is only needed for the developer strategy. A developer can do this without having to change the behavior of omniauth, but it would need to be very explicit in the setup instructions, otherwise they'll be banging their head if they forget to add it.

beerlington commented Apr 8, 2013

@sferik I sort of felt the same way writing it, but by the time I figured out what the actual issue was, I just wanted to get it working. 😀 I think another solution is to just add skip_before_filter :verify_authenticity_token to the session controller since the token is only needed for the developer strategy. A developer can do this without having to change the behavior of omniauth, but it would need to be very explicit in the setup instructions, otherwise they'll be banging their head if they forget to add it.

@tmilewski

This comment has been minimized.

Show comment
Hide comment
@tmilewski

tmilewski Sep 3, 2013

Member

Added to FAQ. Thanks!

Member

tmilewski commented Sep 3, 2013

Added to FAQ. Thanks!

@tmilewski tmilewski closed this Sep 3, 2013

@joshjordan

This comment has been minimized.

Show comment
Hide comment
@joshjordan

joshjordan Sep 3, 2013

Contributor

omniauth does a great job of depending directly on Rack instead of Rails, even if it is arguably used in Rails apps more than other frameworks. This pull request causes omniauth to render a hidden field with an empty value outside of Rails, which doesn't make sense. It also duplicates Rails' notion of how the token is generated, which means that it would be incompatible with other frameworks that use the same session key and would break if Rails changed their implementation.

How about, instead, we make the CSRF token retrieval a config option, with the default value being a Proc that checks to see if ActionController::Metal is defined, and uses the generation/retrieval strategy there? Then, only render the hidden field if the CSRF token is not nil.

Contributor

joshjordan commented Sep 3, 2013

omniauth does a great job of depending directly on Rack instead of Rails, even if it is arguably used in Rails apps more than other frameworks. This pull request causes omniauth to render a hidden field with an empty value outside of Rails, which doesn't make sense. It also duplicates Rails' notion of how the token is generated, which means that it would be incompatible with other frameworks that use the same session key and would break if Rails changed their implementation.

How about, instead, we make the CSRF token retrieval a config option, with the default value being a Proc that checks to see if ActionController::Metal is defined, and uses the generation/retrieval strategy there? Then, only render the hidden field if the CSRF token is not nil.

@joshjordan

This comment has been minimized.

Show comment
Hide comment
@joshjordan

joshjordan Sep 3, 2013

Contributor

Damn. Closed this as I was typing out my response.

Contributor

joshjordan commented Sep 3, 2013

Damn. Closed this as I was typing out my response.

@tmilewski

This comment has been minimized.

Show comment
Hide comment
@tmilewski

tmilewski Sep 3, 2013

Member

@joshjordan While that's certainly a good option, I'm worried about adding Rails-specific functionality. Any thoughts @sferik?

Member

tmilewski commented Sep 3, 2013

@joshjordan While that's certainly a good option, I'm worried about adding Rails-specific functionality. Any thoughts @sferik?

@tmilewski tmilewski reopened this Sep 3, 2013

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 3, 2013

Coverage Status

Coverage decreased (-0.12%) when pulling 64b33b7 on beerlington:rails4-dev-strategy into 6ddc70b on intridea:master.

coveralls commented Sep 3, 2013

Coverage Status

Coverage decreased (-0.12%) when pulling 64b33b7 on beerlington:rails4-dev-strategy into 6ddc70b on intridea:master.

@sferik

This comment has been minimized.

Show comment
Hide comment
@sferik

sferik Sep 3, 2013

Contributor

What @joshjordan has proposed sounds reasonable to me but I’d need to see the code before I 👍.

Contributor

sferik commented Sep 3, 2013

What @joshjordan has proposed sounds reasonable to me but I’d need to see the code before I 👍.

@bradobro

This comment has been minimized.

Show comment
Hide comment
@bradobro

bradobro Jan 8, 2014

Hey @sferic and @beerlington, I was running into this problem today with omniauth-identity and think I found a solution that requires zero changes in Omniauth code:

It looks to me like the culprit is the default NullSession forgery protection scheme. I changed that in the application controller and everything worked well:

class ApplicationController < ActionController::Base
  protect_from_forgery with: :reset_session
  ....

I'm still reading the docs and code on ActionController::RequestForgeryProtection::ProtectionMethods::ResetSession, but this looks like it might solve my problems and relate to @beerlington 's.

If this looks good to you, I'd be glad to turn it into a pull request to the omniauth-identity readme.

bradobro commented Jan 8, 2014

Hey @sferic and @beerlington, I was running into this problem today with omniauth-identity and think I found a solution that requires zero changes in Omniauth code:

It looks to me like the culprit is the default NullSession forgery protection scheme. I changed that in the application controller and everything worked well:

class ApplicationController < ActionController::Base
  protect_from_forgery with: :reset_session
  ....

I'm still reading the docs and code on ActionController::RequestForgeryProtection::ProtectionMethods::ResetSession, but this looks like it might solve my problems and relate to @beerlington 's.

If this looks good to you, I'd be glad to turn it into a pull request to the omniauth-identity readme.

@andrewhavens

This comment has been minimized.

Show comment
Hide comment
@andrewhavens

andrewhavens Jun 16, 2014

I ran into this issue today while adding OmniAuth to a Rails 4.1.1 app, using the :developer strategy, and the example SessionsController found in the README. I think the solution might simply be to update the code example in the README to the following:

class SessionsController < ApplicationController

  skip_before_filter :verify_authenticity_token

  def create
    @user = User.find_or_create_from_auth_hash(auth_hash)
    self.current_user = @user
    redirect_to '/'
  end

  protected

  def auth_hash
    request.env['omniauth.auth']
  end
end

Considering the create request is typically going to be a callback from some third party service, I don't think it is appropriate to verify a CSRF token. Unless your service provides a way to forward a CSRF token, the service would not know that it needs to send a CSRF token.

What do you think?

andrewhavens commented Jun 16, 2014

I ran into this issue today while adding OmniAuth to a Rails 4.1.1 app, using the :developer strategy, and the example SessionsController found in the README. I think the solution might simply be to update the code example in the README to the following:

class SessionsController < ApplicationController

  skip_before_filter :verify_authenticity_token

  def create
    @user = User.find_or_create_from_auth_hash(auth_hash)
    self.current_user = @user
    redirect_to '/'
  end

  protected

  def auth_hash
    request.env['omniauth.auth']
  end
end

Considering the create request is typically going to be a callback from some third party service, I don't think it is appropriate to verify a CSRF token. Unless your service provides a way to forward a CSRF token, the service would not know that it needs to send a CSRF token.

What do you think?

@mltsy

This comment has been minimized.

Show comment
Hide comment
@mltsy

mltsy Dec 15, 2016

I agree that this doesn't need to be addressed in this gem. For clarification of the security concerns:

I had this same problem trying to implement a password grant flow omniauth strategy. The request phase in this flow is just a form (in the client) that gathers the password from the user and submits to the callback (again in the client). When thinking about whether a CSRF token is really necessary in this form, I realized that the whole purpose of the form is to authenticate the user (they are entering their password on this form). If some forged request has that information (the user's password) to submit from off the site, they've already totally compromised the user's security - a CSRF token isn't going to protect the user from an attacker that already has their password. Therefore I believe that skipping CSRF token verification is the right solution, at least for this particular case.

In the case of other grant flows (i.e. authorization code), the request should protected from request forgery using a state parameter, which is specified in the OAuth spec. The omniauth-oauth2 strategy gem implements this for you in the client. In this case, again, I believe Rails' built-in CSRF token verification would be redundant (if it even worked), and can therefore be skipped, as recommended above (skip_before_filter :verify_authenticity_token)

mltsy commented Dec 15, 2016

I agree that this doesn't need to be addressed in this gem. For clarification of the security concerns:

I had this same problem trying to implement a password grant flow omniauth strategy. The request phase in this flow is just a form (in the client) that gathers the password from the user and submits to the callback (again in the client). When thinking about whether a CSRF token is really necessary in this form, I realized that the whole purpose of the form is to authenticate the user (they are entering their password on this form). If some forged request has that information (the user's password) to submit from off the site, they've already totally compromised the user's security - a CSRF token isn't going to protect the user from an attacker that already has their password. Therefore I believe that skipping CSRF token verification is the right solution, at least for this particular case.

In the case of other grant flows (i.e. authorization code), the request should protected from request forgery using a state parameter, which is specified in the OAuth spec. The omniauth-oauth2 strategy gem implements this for you in the client. In this case, again, I believe Rails' built-in CSRF token verification would be redundant (if it even worked), and can therefore be skipped, as recommended above (skip_before_filter :verify_authenticity_token)

@bradobro

This comment has been minimized.

Show comment
Hide comment
@bradobro

bradobro Dec 15, 2016

hey @mltsy would you please start a new issue and ref this one if it's pertinent.

bradobro commented Dec 15, 2016

hey @mltsy would you please start a new issue and ref this one if it's pertinent.

@mltsy

This comment has been minimized.

Show comment
Hide comment
@mltsy

mltsy Dec 15, 2016

Sorry, I was just verifying that this doesn't need to be fixed (with justifications of the workaround for various cases, for people who find this issue as I did) - I should have led with that 😉 I'll update my comment!

mltsy commented Dec 15, 2016

Sorry, I was just verifying that this doesn't need to be fixed (with justifications of the workaround for various cases, for people who find this issue as I did) - I should have led with that 😉 I'll update my comment!

@GUI GUI referenced this pull request Dec 16, 2016

Closed

Fix Admin LDAP login option #316

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