current_user should not perform sign in by default

[vincentwoo]

Environment

• Ruby [2.5.1]
• Rails [5.2]
• Devise [4.5]

Current behavior

It is very hard to prevent sign in with Devise. If you subclass SessionsController to override create, even if you don't allow super to run, any template code that tries to check current_user will automatically run warden's authentication, which signs in the user.

Expected behavior

Users should only be signed in after an invocation of sign_in.

[lacostenycoder]

@vincentwoo can you provide a code example of how to reproduce the current behavior in this issue?

[vincentwoo]

I can later but this snippet from the devise codebase explains the behavior: https://github.com/plataformatec/devise/blob/715192a7709a4c02127afb067e66230061b82cf2/lib/devise/controllers/helpers.rb#L60

[vincentwoo]

Okay, so let's say you have a SessionsController with the goal to prevent logins conditionally:

class SessionsController < Devise::SessionsController
  def create
    user = User.find_by_email(sign_in_params[:email])
    if user.some_condition?
      return redirect_to some_other_url
    end
    super # otherwise login as usual
  end
end

If, say, the view for some_other_url calls current_user, the the user will be logged in anyway. This seems bad.

[brendancarney]

I agree. This made it more challenging to implement two-factor auth. We want to redirect to the two-factor auth page after checking the user's password, but some other code (like session tracking) checks if there is a current_user, causing the user to be logged in.

[lacostenycoder]

@brendancarney what did you do as a workaround?

[lacostenycoder]

@vincentwoo This may seem obvious to you, but, why would you call current_user in a view for some_other_url if that user should not be authenticated in the first place? If you want to insure user is not authenticated? Why would you not just do this?

class SessionsController < Devise::SessionsController
  def create
    user = User.find_by_email(sign_in_params[:email])
    if user.some_condition?
      sign_out_and_redirect current_user
    end
    super # otherwise login as usual
  end
end

and handle where to send them in ApplicactionController#after_sign_out_path_for(user) ?

[vincentwoo]

Because you want to conditionally show a different view to logged in users who mistakenly end up on that page through an old link from email, slack, etc.

[lacostenycoder]

Ok I see how that might propose a challenge. But for security reasons, if you don't know how or if the user has properly authenticated (i.e. example you've provided) this seems to be an insecure design approach. You still have referrer which might help you handle where to route the users if they are still properly authenticated. It still seems to me that current_user is doing what it is intended to do. Without it, how else do you imagine this could/should work instead?

[vincentwoo]

Your statement gives me the impression that you may be confused - current_user's explicit purpose is to tell you whether or not the user is signed in. I am saying that it should NOT be a secret, double purpose to perform a login if one is not found, which it does now. Application code everywhere in the app must rely on current_user (or equivalent helper, user_signed_in?) in order to decide whether the user is authorized to view the current page, etc.

Logins should only explicitly be created in controller code, not automatically by helpers.

[shaicoleman]

That behavior is very surprising, and I spent a significant amount of time debugging an issue because of that

[lacostenycoder]

I understand this be may problematic in some cases, but how do you propose to solve the problem? current_user method doesn't "sign in the user" but rather authenticates them via warden if the cached instance @current_user is somehow not already set.

https://github.com/plataformatec/devise/blob/40f02ae69baf7e9b0449aaab2aba0d0e166f77a3/lib/devise/controllers/helpers.rb#L125

The user should only have an active session if they have actually done a sign_in dance, (i.e. username & password) or some other auth type, and their session has not been destroyed.

@vincentwoo in your case, it might make sense to expire sessions after a certain amount of inactivity, or when your application is expiring pages/links as you've described.

[vincentwoo]

Easy enough. Probably you would have current_user stop calling warden.authenticate, and use the more appropriate warden method, authenticated? https://github.com/wardencommunity/warden/blob/61b22a6ffb53f2816a414c5d256650361942db71/lib/warden/proxy.rb#L149

[lacostenycoder]

That may seem logical, however it is clearly a departure from the current_user's intended purpose. Consider the Devise README:

To verify if a user is signed in, use the following helper:

user_signed_in?

For the current signed-in user, this helper is available:

current_user

So generally speaking it seems clear that according to devise docs you should not call current_user if you don't expect one to be authenticated.

[vincentwoo]

Respectfully, that is silly - current_user returns nil as a valid return type in case of auth returning nothing, and you are suggesting that users should never learn to examine this return type. Many Devise programmers across the internet have learned to use current_user returning nil as a pattern for conditionally doing things when a user is not logged in. Many users write code like if current_user&.some_flag? (and that is a good thing).

Tutorials like this one will commonly contain (apparently mistaken) refrains like:

current_user
The current_user method , whose purpose is self-explanatory, simply returns the model class relating to the signed in user. It returns nil if a user has not, as yet, signed in.

user_signed_in?
The user_signed_in? query method, which really just checks if the current_user method returns a value that’s not nil.

Even more simply, though, the whole design here is somewhat crazy. These helpers are defined by Devise:

#     authenticate_user!  # Signs user in or redirect
#     user_signed_in?     # Checks whether there is a user signed in or not
#     current_user        # Current signed in user
#     user_session        # Session data available only to the user scope

Of these, both authenticate_user! and current_user perform logins. There is literally no way to access the current user without potentially performing a login. This is a huge design flaw and should be corrected. If a new major version needs to be cut, so be it.

[brendancarney]

@lacostenycoder we ended up adding a custom warden strategy. Looks something like:

Warden::Strategies.add(:two_factor) do
  def authenticate!
    user = User.find_by(email: params["user"]["email"])
    return fail! unless user.present?
    if user.use_two_factor?
     # Don't set up the user session yet
      ...
      halt!
    else
      # Move on to the other strategies
      pass
    end
  end

[krtschmr]

I agree. This made it more challenging to implement two-factor auth. We want to redirect to the two-factor auth page after checking the user's password, but some other code (like session tracking) checks if there is a current_user, causing the user to be logged in.

won't it be better to capture the 2FA code in the same form as the password?

[krtschmr]

@brendancarney which gem-extension do you use for 2FA?

[lacostenycoder]

@krtschmr I can't answer for brendan but we're using https://github.com/tinfoil/devise-two-factor with no problems.

[theshashiverma]

@vincentwoo @lacostenycoder @brendancarney @shaicoleman @krtschmr please have a look on this #5017

[tegon]

Hi everyone, thanks for all points raised in this discussion.

First of all, I want to say that I would also prefer an explicit behavior rather than an implicit one, which is what's happening in #current_user. But I will also say that this is a very common pattern in Ruby on Rails applications - I've seen it everywhere in my career - it's the memoization pattern, where people try to "cache" side effects in a method with an instance variable that has the same name.
I've seen people use in database queries, network requests and so on. So, even though I'd prefer an explicit behavior, I think since it's documented, it's ok to keep it as it is.
Most people are used to, and relying on this behavior by now, so changing would make it a pain to upgrade - even in a major version.

There is something that should be addressed though: the #user_signed_in? method, that is mentioned on the documentation as the way to check whether the user is authenticated or not, is actually performing the sign in too. You can see more about it here. We will fix that one in the next major version.

As for the use cases mentioned before, I think overriding the sessions controller is not the best alternative. The two-factor auth, for example, is another way of authenticating a user, so a Warden strategy is the best way to solve this problem, as you mentioned already here before.

That being said I think it's better to close this issue and continue with #5013 where we are going to fix #user_sign_in?. What do you guys think?

[vincentwoo]

I don't follow your reasoning. Why is memoization the relevant pattern here? The problem isn't that current_user caches its results, its that it performs login.

[brendancarney]

won't it be better to capture the 2FA code in the same form as the password?

I'm not sure. I've seen it both ways. It seems like most of the services I use have you authenticate with username and password before getting to 2FA.

which gem-extension do you use for 2FA?

We use https://github.com/twilio/authy-devise along with the custom warden strategy I mentioned in a previous comment.

[tegon]

@vincentwoo My point is that it's commonly used in methods that have side-effects. That's why I think that as long it's documented and another version (that doesn't have this side-effect) is available, it's ok to keep it as it is.

[vincentwoo]

Okay. Are you going to add a version of current_user that does not perform login?

[krtschmr]

should be either temp_user or attempted_user

something like current_user(no_login_attempt: true) would suck, however no_login_attempt could be done via configurator, then we can decide what current_user should do.

the question is: is current_user suitable since we don't have a user actually?

[tegon]

@vincentwoo The version that I mean is user_signed_in?.

[vincentwoo]

The boolean method alone is not very helpful for porting code to correct it. I'd rather have a version of current_user that does not login, but I suppose I could add it myself as a helper.

[tegon]

Since the code is out there for almost 10 years we can't know the impact a change like that would have, that's why we rather not change this behavior, even in a major version (we don't want to make it a pain for people to update it).

I'm going to close this then and user_signed_in? will be fixed in #5013. Thanks, everyone!

[krtschmr]

@tegon

current_user(no_login_attempt: true) or temp_user.

we just add functionality and we won't break anything

[tegon]

@krtschmr That also adds a complexity to the method that I don't think it's justified. All of the use cases showed here in this thread could be achieved using alternative APIs that are already available.

[krtschmr]

had a problem today. sessions#create . if i call current_user i have a logged in user.
meh, that's really bad.

[abigoroth]

I don't know whether i'm on the same page, but I had this problem with my custom strategies. I don't where to get help. tons of google or perhaps misleading guide.

My view:

            - if current_user
              li.nav-item
               = link_to "sign out", destroy_user_session_path, method: :delete, class:'nav-link'

My strategies :

require 'devise/strategies/authenticatable'

module Devise
  module Strategies
    class SsoStrategy < Authenticatable
      def valid?
        # token.present?
        true
      end

      def authenticate!
        Rails.logger.debug "SsoStrategy"
        # if !params.nil? && !params[:admin].nil?
        user = User.find_by(authentication_token: token, email: email)
    
        if user
          success!(user)
        else
          fail!('Invalid credentials')
        end
        # end
      end


      def email
        params[:user][:email]
      end

      def token
        params[:user][:token]
      end

      # def authentication_token
      #   params[:admin][:authentication_token] if params && params[:admin]
      # end
    end
  end
end

My controller :

class TokenAuthsController < ApplicationController
  skip_before_action :verify_authenticity_token
  def create
    Rails.logger.debug "manaaa"
    warden.authenticate!(:sso_strategy)
    redirect_to root_path
  end
end

its calling authenticate! method and its breaking it. 😢

[tegon]

@abigoroth I'm not sure your problem is the same as the above. By the error message, it seems like params[:user] is returning nil, which then fails to call [:token] on it.
How are you calling this endpoint?

[joeyparis]

What if this functionality was added as a setting in Devise.setup?

[joshco]

Glad I found this. Using devise I have some controllers which require sign in, but others that don't require it, but if the user is signed in, the UI reflects that. (imagine the user prefs in the nav bar)

Between this and the documentation, I couldn't figure the idiomatic way to do this. What works for me is a before action below. What is the right way to do this?

MyController < ApplicationController
  before_action: :optional_authentication
  ......
  def optional_authentication
    if warden.user.present?
      sign_in(warden.user)
    end
  end
end

[pokrovskyy]

Faced this same problem today when working on custom implementation of 2FA. I understand @tegon's point on backwards compatibility, that's indeed too big of a change nowadays. But I do feel such implicit sign-in is not quite right either.

So I ended up adding this simple instance method override logic to application_controller.rb:

  alias_method :original_current_user, :current_user

  def current_user
    original_current_user if warden.authenticated?(scope: :user)
  end

This way it omits implicit sign-in while keeping all the existing logic intact. Hope that helps!

[frullah]

Faced the same problem that the current_user performs authentication.

Tried workaround from @pokrovskyy (here), but i got this error instead:

`alias_method': undefined method `current_user' for class `ApplicationController'

So I ended up having this workaround instead

  def current_user
    super if warden.authenticated?(scope: :user)
  end

[humphreyja]

Pretty disappointed in the outcome of this issue.....

First off for those looking: @frullah's solution does work. I put it in the SessionsController where I rewrote the #create method to add a custom 2FA.

This "solution" @tegon came to is very misleading to developers. Take this documentation for example:
num 4 here tells you how you can rewrite the session controller to provide custom sign in, but that's not true at all is it? In fact you can't change the sign in at all from the controller, it just looks like you can. I spent 3 hours today debugging this after a coworker discovered a way to circumvent the entire 2FA flow.

So as you can see, misleading documentation, misleading method names, actually led to a vulnerability in our auth.

@tegon Why not default the behavior to NOT sign in the user "magically" and instead provide a config option for those older code bases? This would both be forward thinking but also keeps in mind those older codebases. Like this:

Devise.setup do |config|
  config.sign_in_when_calling_current = true
end