From 3773115066d4521f9a83fd9eeab9c694ef144469 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Tue, 24 Sep 2019 04:35:36 +0200 Subject: [PATCH] Fix authentication before 2FA challenge (#11943) Regression from #11831 --- app/controllers/auth/sessions_controller.rb | 61 +++++++++++-------- app/models/concerns/ldap_authenticable.rb | 44 ++++++++++--- config/application.rb | 3 +- config/initializers/devise.rb | 11 ++-- lib/devise/ldap_authenticatable.rb | 55 ----------------- lib/devise/two_factor_ldap_authenticatable.rb | 32 ++++++++++ lib/devise/two_factor_pam_authenticatable.rb | 31 ++++++++++ 7 files changed, 139 insertions(+), 98 deletions(-) delete mode 100644 lib/devise/ldap_authenticatable.rb create mode 100644 lib/devise/two_factor_ldap_authenticatable.rb create mode 100644 lib/devise/two_factor_pam_authenticatable.rb diff --git a/app/controllers/auth/sessions_controller.rb b/app/controllers/auth/sessions_controller.rb index b3113bbefc..f48b17c799 100644 --- a/app/controllers/auth/sessions_controller.rb +++ b/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 diff --git a/app/models/concerns/ldap_authenticable.rb b/app/models/concerns/ldap_authenticable.rb index 84ff84c4b0..1179939478 100644 --- a/app/models/concerns/ldap_authenticable.rb +++ b/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 + 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]) - save! - end + if (user_info = ldap.bind_as(base: Devise.ldap_base, filter: filter, password: params[:password])) + ldap_get_user(user_info.first) + end + end - class_methods do 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 diff --git a/config/application.rb b/config/application.rb index 5fd37120d0..3ced81b8f3 100644 --- a/config/application.rb +++ b/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 diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 311583820a..fd9a5a8b9b 100644 --- a/config/initializers/devise.rb +++ b/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 diff --git a/lib/devise/ldap_authenticatable.rb b/lib/devise/ldap_authenticatable.rb deleted file mode 100644 index 6903d468dc..0000000000 --- a/lib/devise/ldap_authenticatable.rb +++ /dev/null @@ -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) diff --git a/lib/devise/two_factor_ldap_authenticatable.rb b/lib/devise/two_factor_ldap_authenticatable.rb new file mode 100644 index 0000000000..065aa2de88 --- /dev/null +++ b/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) diff --git a/lib/devise/two_factor_pam_authenticatable.rb b/lib/devise/two_factor_pam_authenticatable.rb new file mode 100644 index 0000000000..5ce723b331 --- /dev/null +++ b/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)