Adding authenticity token so session will persist

[beerlington]

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.

[sferik]

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

@mbleigh What are your thoughts about this?

[beerlington]

@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]

Added to FAQ. Thanks!

[joshjordan]

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]

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

[tmilewski]

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

[coveralls]

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

[sferik]

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

[bradobro]

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]

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]

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]

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

[mltsy]

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!