Fix 2FA challenge and password challenge for non-database users (#11831)

* Fix 2FA challenge not appearing for non-database users

Fix #11685

* Fix account deletion not working when using external login

Fix #11691

app/controllers/auth/sessions_controller.rb

@@ -8,8 +8,6 @@ class Auth::SessionsController < Devise::SessionsController
   skip_before_action :require_no_authentication, only: [:create]
   skip_before_action :require_functional!
 
-  prepend_before_action :authenticate_with_two_factor, if: :two_factor_enabled?, only: [:create]
-
   before_action :set_instance_presenter, only: [:new]
   before_action :set_body_classes
 
@@ -22,9 +20,22 @@ class Auth::SessionsController < Devise::SessionsController
   end
 
   def create
-    super do |resource|
+    self.resource = begin
-      remember_me(resource)
+      if user_params[:email].blank? && session[:otp_user_id].present?
-      flash.delete(:notice)
+        User.find(session[:otp_user_id])
+      else
+        warden.authenticate!(auth_options)
+      end
+    end
+
+    if resource.otp_required_for_login?
+      if user_params[:otp_attempt].present? && session[:otp_user_id].present?
+        authenticate_with_two_factor_via_otp(resource)
+      else
+        prompt_for_two_factor(resource)
+      end
+    else
+      authenticate_and_respond(resource)
     end
   end
 
@@ -37,18 +48,6 @@ class Auth::SessionsController < Devise::SessionsController
 
   protected
 
-  def find_user
-    if session[:otp_user_id]
-      User.find(session[:otp_user_id])
-    elsif user_params[:email]
-      if use_seamless_external_login? && Devise.check_at_sign && user_params[:email].index('@').nil?
-        User.joins(:account).find_by(accounts: { username: user_params[:email] })
-      else
-        User.find_for_authentication(email: user_params[:email])
-      end
-    end
-  end
-
   def user_params
     params.require(:user).permit(:email, :password, :otp_attempt)
   end
@@ -71,32 +70,17 @@ class Auth::SessionsController < Devise::SessionsController
     super
   end
 
-  def two_factor_enabled?
-    find_user.try(:otp_required_for_login?)
-  end
-
   def valid_otp_attempt?(user)
     user.validate_and_consume_otp!(user_params[:otp_attempt]) ||
       user.invalidate_otp_backup_code!(user_params[:otp_attempt])
-  rescue OpenSSL::Cipher::CipherError => _error
+  rescue OpenSSL::Cipher::CipherError
     false
   end
 
-  def authenticate_with_two_factor
-    user = self.resource = find_user
-
-    if user_params[:otp_attempt].present? && session[:otp_user_id]
-      authenticate_with_two_factor_via_otp(user)
-    elsif user&.valid_password?(user_params[:password])
-      prompt_for_two_factor(user)
-    end
-  end
-
   def authenticate_with_two_factor_via_otp(user)
     if valid_otp_attempt?(user)
       session.delete(:otp_user_id)
-      remember_me(user)
+      authenticate_and_respond(user)
-      sign_in(user)
     else
       flash.now[:alert] = I18n.t('users.invalid_otp_token')
       prompt_for_two_factor(user)
@@ -108,6 +92,13 @@ class Auth::SessionsController < Devise::SessionsController
     render :two_factor
   end
 
+  def authenticate_and_respond(user)
+    sign_in(user)
+    remember_me(user)
+
+    respond_with user, location: after_sign_in_path_for(user)
+  end
+
   private
 
   def set_instance_presenter
@@ -120,9 +111,11 @@ class Auth::SessionsController < Devise::SessionsController
 
   def home_paths(resource)
     paths = [about_path]
+
     if single_user_mode? && resource.is_a?(User)
       paths << short_account_path(username: resource.account)
     end
+
     paths
   end
 

app/controllers/settings/deletes_controller.rb

@@ -14,12 +14,11 @@ class Settings::DeletesController < Settings::BaseController
   end
 
   def destroy
-    if current_user.valid_password?(delete_params[:password])
+    if challenge_passed?
-      Admin::SuspensionWorker.perform_async(current_user.account_id, true)
+      destroy_account!
-      sign_out
       redirect_to new_user_session_path, notice: I18n.t('deletes.success_msg')
     else
-      redirect_to settings_delete_path, alert: I18n.t('deletes.bad_password_msg')
+      redirect_to settings_delete_path, alert: I18n.t('deletes.challenge_not_passed')
     end
   end
 
@@ -29,11 +28,25 @@ class Settings::DeletesController < Settings::BaseController
     redirect_to root_path unless Setting.open_deletion
   end
 
-  def delete_params
+  def resource_params
-    params.require(:form_delete_confirmation).permit(:password)
+    params.require(:form_delete_confirmation).permit(:password, :username)
   end
 
   def require_not_suspended!
     forbidden if current_account.suspended?
   end
+
+  def challenge_passed?
+    if current_user.encrypted_password.blank?
+      current_account.username == resource_params[:username]
+    else
+      current_user.valid_password?(resource_params[:password])
+    end
+  end
+
+  def destroy_account!
+    current_account.suspend!
+    Admin::SuspensionWorker.perform_async(current_user.account_id, true)
+    sign_out
+  end
 end

app/models/form/delete_confirmation.rb

@@ -3,5 +3,5 @@
 class Form::DeleteConfirmation
   include ActiveModel::Model
 
-  attr_accessor :password
+  attr_accessor :password, :username
 end

app/views/settings/deletes/show.html.haml

@@ -20,7 +20,10 @@
 
   %hr.spacer/
 
+  - if current_user.encrypted_password.present?
     = f.input :password, wrapper: :with_block_label, input_html: { :autocomplete => 'off' }, hint: t('deletes.confirm_password')
+  - else
+    = f.input :username, wrapper: :with_block_label, input_html: { :autocomplete => 'off' }, hint: t('deletes.confirm_username')
 
   .actions
     = f.button :button, t('deletes.proceed'), type: :submit, class: 'negative'

config/initializers/devise.rb

@@ -71,10 +71,13 @@ end
 
 Devise.setup do |config|
   config.warden do |manager|
+    manager.default_strategies(scope: :user).unshift :database_authenticatable
     manager.default_strategies(scope: :user).unshift :ldap_authenticatable if Devise.ldap_authentication
     manager.default_strategies(scope: :user).unshift :pam_authenticatable  if Devise.pam_authentication
-    manager.default_strategies(scope: :user).unshift :two_factor_authenticatable
+
-    manager.default_strategies(scope: :user).unshift :two_factor_backupable
+    # We handle 2FA in our own sessions controller so this gets in the way
+    manager.default_strategies(scope: :user).delete :two_factor_backupable
+    manager.default_strategies(scope: :user).delete :two_factor_authenticatable
   end
 
   # The secret key used by Devise. Devise uses this key to generate

config/locales/en.yml

@@ -632,8 +632,9 @@ en:
       x_months: "%{count}mo"
       x_seconds: "%{count}s"
   deletes:
-    bad_password_msg: The password you entered was incorrect
+    challenge_not_passed: The information you entered was not correct
     confirm_password: Enter your current password to verify your identity
+    confirm_username: Enter your username to confirm the procedure
     proceed: Delete account
     success_msg: Your account was successfully deleted
     warning:

spec/controllers/auth/sessions_controller_spec.rb

@@ -5,11 +5,11 @@ require 'rails_helper'
 RSpec.describe Auth::SessionsController, type: :controller do
   render_views
 
-  describe 'GET #new' do
   before do
     request.env['devise.mapping'] = Devise.mappings[:user]
   end
 
+  describe 'GET #new' do
     it 'returns http success' do
       get :new
       expect(response).to have_http_status(200)
@@ -19,10 +19,6 @@ RSpec.describe Auth::SessionsController, type: :controller do
   describe 'DELETE #destroy' do
     let(:user) { Fabricate(:user) }
 
-    before do
-      request.env['devise.mapping'] = Devise.mappings[:user]
-    end
-
     context 'with a regular user' do
       it 'redirects to home after sign out' do
         sign_in(user, scope: :user)
@@ -51,10 +47,6 @@ RSpec.describe Auth::SessionsController, type: :controller do
   end
 
   describe 'POST #create' do
-    before do
-      request.env['devise.mapping'] = Devise.mappings[:user]
-    end
-
     context 'using PAM authentication', if: ENV['PAM_ENABLED'] == 'true' do
       context 'using a valid password' do
         before do
@@ -191,11 +183,11 @@ RSpec.describe Auth::SessionsController, type: :controller do
     end
 
     context 'using two-factor authentication' do
-      let(:user) do
+      let!(:user) do
-        Fabricate(:user, email: 'x@y.com', password: 'abcdefgh',
+        Fabricate(:user, email: 'x@y.com', password: 'abcdefgh', otp_required_for_login: true, otp_secret: User.generate_otp_secret(32))
-                         otp_required_for_login: true, otp_secret: User.generate_otp_secret(32))
       end
-      let(:recovery_codes) do
+
+      let!(:recovery_codes) do
         codes = user.generate_otp_backup_codes!
         user.save
         return codes