Overwriting Devise::SessionsController#create doesn't stop signing in

[WozniakMac]

Environment

• Ruby 3.2.2
• Rails 7.0.5
• Devise 4.9.2

Current behavior

If I overwrite Devise::SessionsController#create in custom controller to not sign in user and just redirect it still signs in the user if login and password are correct. Confirmed on 2 different apps. I think it could be related to implementation of current_user that calls warden.authenticate

# app/controllers/users/sessions_controller.rb
class Users::SessionsController < Devise::SessionsController
  # POST /resource/sign_in
  def create
    redirect_to root_path
  end
end

# config/routes.rb
  devise_for :users, controllers: {
    sessions: "users/sessions"
  }

Expected behavior

It should not sign in the user

[JoshingYou1]

I'm having this issue as well. I wanted to prevent users that have a certain flag from signing in and just redirected them instead. I'm noticing two things:

1. The ApplicationController is hit before the User::SessionsController is. When I place a debugger in a before action inside of application controller, the user is already signed in.

2. The same issue you're seeing where the user is signed in prior to hitting the create action.

I would love to hear from other people or the team as to why they might think this is happening.

[humphreyja]

+1

[humphreyja]

#4951

Figured this out @WozniakMac and @JoshingYou1. If you put this code in your application_controller or sessions_controller, it won't sign in. The Devise team decided that calling current_user needs to sign in the user circumventing anything you'd do in the create method. Caused a vulnerability for me, hopefully you caught it before anything like that.

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

This is still hacky IMO, I'd love to hear from the team on something better for this. Or at least some updated docs...

[pnomolos]

You're correct on the implementation of current_#{mapping} - ref https://github.com/heartcombo/devise/blob/main/lib/devise/controllers/helpers.rb#L127

However, it's been that way for 10 years, so it may be a conscious design decision.

I haven't tested this, but if you do this in your devise controller:

prepend_before_action -> { warden.lock! }, only: %i[create]

it will hopefully do what you're expecting, as long as it's ending up early enough in the request chain. warden#lock! disables any further authentication strategies for the current request, but doesn't touch existing sessions.

If you need to access warden outside of a subclass of Devise::Controller then request.env['warden'] will return the same value

[humphreyja]

I mean saying "its been that way for 10 years" isn't exactly the mindset I'd want for my authentication code....

Things should be updated to make sense to the conventions and what the developers are using today. Additionally, they never changed the documentation from that issue. For example, the docs still say you can overwrite the create method in the sessions controller to create a custom sign in, which is not true at all: https://github.com/heartcombo/devise#configuring-controllers

[pnomolos]

Yeah, that example in the README has also been around awhile. My recommendation would be to put together a PR with a test that fails with the current code, and change the implementation to something such as

def current_#{mapping}
  @current_#{mapping} ||= warden.authenticated?(scope: :#{mapping}) ? nil : warden.authenticate(scope: :#{mapping})
end

haven't tested but my guess is that this should retain the existing behaviour while also satisfying your needs - and arguably being a slightly better design due to the lack of unintended side effect.

[crespire]

Yeah, that example in the README has also been around awhile. My recommendation would be to put together a PR with a test that fails with the current code, and change the implementation to something such as

def current_#{mapping}
@current_#{mapping} ||= warden.authenticated?(scope: :#{mapping}) ? nil : warden.authenticate(scope: :#{mapping})
end

haven't tested but my guess is that this should retain the existing behaviour while also satisfying your needs - and arguably being a slightly better design due to the lack of unintended side effect.

I think your code example has it backwards:

def current_#{mapping}
  @current_#{mapping} ||= warden.authenticated?(scope: :#{mapping}) ? warden.authenticate(scope: :#{mapping}) : nil
end