devise: current_user should not perform sign in by default

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!

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.

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.

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

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?

@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

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.

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

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

current_user(no_login_attempt: true) or temp_user.

we just add functionality and we won’t break anything

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.