Fix authentication before 2FA challenge (#11943)

Regression from #11831

app/controllers/auth/sessions_controller.rb

@@ -8,6 +8,8 @@ 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
 
@@ -20,22 +22,9 @@ class Auth::SessionsController < Devise::SessionsController
   end
 
   def create
-    self.resource = begin
-      if user_params[:email].blank? && session[:otp_user_id].present?
-        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)
+    super do |resource|
+      remember_me(resource)
+      flash.delete(:notice)
     end
   end
 
@@ -49,6 +38,16 @@ class Auth::SessionsController < Devise::SessionsController
 
   protected
 
+  def find_user
+    if session[:otp_user_id]
+      User.find(session[:otp_user_id])
+    else
+      user   = User.authenticate_with_ldap(user_params) if Devise.ldap_authentication
+      user ||= User.authenticate_with_pam(user_params) if Devise.pam_authentication
+      user ||= User.find_for_authentication(email: user_params[:email])
+    end
+  end
+
   def user_params
     params.require(:user).permit(:email, :password, :otp_attempt)
   end
@@ -71,6 +70,10 @@ class Auth::SessionsController < Devise::SessionsController
     super
   end
 
+  def two_factor_enabled?
+    find_user&.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])
@@ -78,10 +81,24 @@ class Auth::SessionsController < Devise::SessionsController
     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.present? && (user.encrypted_password.blank? || user.valid_password?(user_params[:password]))
+      # If encrypted_password is blank, we got the user from LDAP or PAM,
+      # so credentials are already valid
+
+      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)
-      authenticate_and_respond(user)
+      remember_me(user)
+      sign_in(user)
     else
       flash.now[:alert] = I18n.t('users.invalid_otp_token')
       prompt_for_two_factor(user)
@@ -90,16 +107,10 @@ class Auth::SessionsController < Devise::SessionsController
 
   def prompt_for_two_factor(user)
     session[:otp_user_id] = user.id
+    @body_classes = 'lighter'
     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
@@ -112,11 +123,9 @@ 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/models/concerns/ldap_authenticable.rb

@@ -3,24 +3,50 @@
 module LdapAuthenticable
   extend ActiveSupport::Concern
 
-  def ldap_setup(_attributes)
-    self.confirmed_at = Time.now.utc
-    self.admin        = false
-    self.external     = true
-
-    save!
-  end
-
   class_methods do
+    def authenticate_with_ldap(params = {})
+      ldap   = Net::LDAP.new(ldap_options)
+      filter = format(Devise.ldap_search_filter, uid: Devise.ldap_uid, email: params[:email])
+
+      if (user_info = ldap.bind_as(base: Devise.ldap_base, filter: filter, password: params[:password]))
+        ldap_get_user(user_info.first)
+      end
+    end
+
     def ldap_get_user(attributes = {})
       resource = joins(:account).find_by(accounts: { username: attributes[Devise.ldap_uid.to_sym].first })
 
       if resource.blank?
-        resource = new(email: attributes[:mail].first, agreement: true, account_attributes: { username: attributes[Devise.ldap_uid.to_sym].first })
-        resource.ldap_setup(attributes)
+        resource = new(email: attributes[:mail].first, agreement: true, account_attributes: { username: attributes[Devise.ldap_uid.to_sym].first }, admin: false, external: true, confirmed_at: Time.now.utc)
+        resource.save!
       end
 
       resource
     end
+
+    def ldap_options
+      opts = {
+        host: Devise.ldap_host,
+        port: Devise.ldap_port,
+        base: Devise.ldap_base,
+
+        auth: {
+          method: :simple,
+          username: Devise.ldap_bind_dn,
+          password: Devise.ldap_password,
+        },
+
+        connect_timeout: 10,
+      }
+
+      if [:simple_tls, :start_tls].include?(Devise.ldap_method)
+        opts[:encryption] = {
+          method: Devise.ldap_method,
+          tls_options: OpenSSL::SSL::SSLContext::DEFAULT_PARAMS.tap { |options| options[:verify_mode] = OpenSSL::SSL::VERIFY_NONE if Devise.ldap_tls_no_verify },
+        }
+      end
+
+      opts
+    end
   end
 end

config/application.rb

@@ -13,7 +13,8 @@ require_relative '../lib/paperclip/video_transcoder'
 require_relative '../lib/paperclip/type_corrector'
 require_relative '../lib/mastodon/snowflake'
 require_relative '../lib/mastodon/version'
-require_relative '../lib/devise/ldap_authenticatable'
+require_relative '../lib/devise/two_factor_ldap_authenticatable'
+require_relative '../lib/devise/two_factor_pam_authenticatable'
 
 Dotenv::Railtie.load
 

config/initializers/devise.rb

@@ -71,13 +71,10 @@ 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
-
-    # 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
+    manager.default_strategies(scope: :user).unshift :two_factor_ldap_authenticatable if Devise.ldap_authentication
+    manager.default_strategies(scope: :user).unshift :two_factor_pam_authenticatable  if Devise.pam_authentication
+    manager.default_strategies(scope: :user).unshift :two_factor_authenticatable
+    manager.default_strategies(scope: :user).unshift :two_factor_backupable
   end
 
   # The secret key used by Devise. Devise uses this key to generate

lib/devise/ldap_authenticatable.rb

@@ -1,55 +0,0 @@
-# frozen_string_literal: true
-
-require 'net/ldap'
-require 'devise/strategies/authenticatable'
-
-module Devise
-  module Strategies
-    class LdapAuthenticatable < Authenticatable
-      def authenticate!
-        if params[:user]
-          ldap = Net::LDAP.new(
-            host: Devise.ldap_host,
-            port: Devise.ldap_port,
-            base: Devise.ldap_base,
-            encryption: {
-              method: Devise.ldap_method,
-              tls_options: tls_options,
-            },
-            auth: {
-              method: :simple,
-              username: Devise.ldap_bind_dn,
-              password: Devise.ldap_password,
-            },
-            connect_timeout: 10
-          )
-
-          filter = format(Devise.ldap_search_filter, uid: Devise.ldap_uid, email: email)
-
-          if (user_info = ldap.bind_as(base: Devise.ldap_base, filter: filter, password: password))
-            user = User.ldap_get_user(user_info.first)
-            success!(user)
-          else
-            return fail(:invalid)
-          end
-        end
-      end
-
-      def email
-        params[:user][:email]
-      end
-
-      def password
-        params[:user][:password]
-      end
-
-      def tls_options
-        OpenSSL::SSL::SSLContext::DEFAULT_PARAMS.tap do |options|
-          options[:verify_mode] = OpenSSL::SSL::VERIFY_NONE if Devise.ldap_tls_no_verify
-        end
-      end
-    end
-  end
-end
-
-Warden::Strategies.add(:ldap_authenticatable, Devise::Strategies::LdapAuthenticatable)

lib/devise/two_factor_ldap_authenticatable.rb

@@ -0,0 +1,32 @@
+# frozen_string_literal: true
+
+require 'net/ldap'
+require 'devise/strategies/base'
+
+module Devise
+  module Strategies
+    class TwoFactorLdapAuthenticatable < Base
+      def valid?
+        valid_params? && mapping.to.respond_to?(:authenticate_with_ldap)
+      end
+
+      def authenticate!
+        resource = mapping.to.authenticate_with_ldap(params[scope])
+
+        if resource && !resource.otp_required_for_login?
+          success!(resource)
+        else
+          fail(:invalid)
+        end
+      end
+
+      protected
+
+      def valid_params?
+        params[scope] && params[scope][:password].present?
+      end
+    end
+  end
+end
+
+Warden::Strategies.add(:two_factor_ldap_authenticatable, Devise::Strategies::TwoFactorLdapAuthenticatable)

lib/devise/two_factor_pam_authenticatable.rb

@@ -0,0 +1,31 @@
+# frozen_string_literal: true
+
+require 'devise/strategies/base'
+
+module Devise
+  module Strategies
+    class TwoFactorPamAuthenticatable < Base
+      def valid?
+        valid_params? && mapping.to.respond_to?(:authenticate_with_pam)
+      end
+
+      def authenticate!
+        resource = mapping.to.authenticate_with_pam(params[scope])
+
+        if resource && !resource.otp_required_for_login?
+          success!(resource)
+        else
+          fail(:invalid)
+        end
+      end
+
+      protected
+
+      def valid_params?
+        params[scope] && params[scope][:password].present?
+      end
+    end
+  end
+end
+
+Warden::Strategies.add(:two_factor_pam_authenticatable, Devise::Strategies::TwoFactorPamAuthenticatable)