diff options
| -rw-r--r-- | app/controllers/auth/sessions_controller.rb | 61 | ||||
| -rw-r--r-- | app/controllers/settings/deletes_controller.rb | 25 | ||||
| -rw-r--r-- | app/models/form/delete_confirmation.rb | 2 | ||||
| -rw-r--r-- | app/views/settings/deletes/show.html.haml | 5 | ||||
| -rw-r--r-- | config/initializers/devise.rb | 7 | ||||
| -rw-r--r-- | config/locales/en.yml | 3 | ||||
| -rw-r--r-- | spec/controllers/auth/sessions_controller_spec.rb | 24 |
7 files changed, 66 insertions, 61 deletions
diff --git a/app/controllers/auth/sessions_controller.rb b/app/controllers/auth/sessions_controller.rb index 7e6dbf19e..3e93b2e68 100644 --- a/app/controllers/auth/sessions_controller.rb +++ b/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| - remember_me(resource) - flash.delete(:notice) + 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) 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) - sign_in(user) + authenticate_and_respond(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 diff --git a/app/controllers/settings/deletes_controller.rb b/app/controllers/settings/deletes_controller.rb index 97fe4d328..15a59c999 100644 --- a/app/controllers/settings/deletes_controller.rb +++ b/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]) - Admin::SuspensionWorker.perform_async(current_user.account_id, true) - sign_out + if challenge_passed? + destroy_account! 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 - params.require(:form_delete_confirmation).permit(:password) + def resource_params + 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 diff --git a/app/models/form/delete_confirmation.rb b/app/models/form/delete_confirmation.rb index 0884a09b8..99d04b331 100644 --- a/app/models/form/delete_confirmation.rb +++ b/app/models/form/delete_confirmation.rb @@ -3,5 +3,5 @@ class Form::DeleteConfirmation include ActiveModel::Model - attr_accessor :password + attr_accessor :password, :username end diff --git a/app/views/settings/deletes/show.html.haml b/app/views/settings/deletes/show.html.haml index 6e2ff31c5..08792e0af 100644 --- a/app/views/settings/deletes/show.html.haml +++ b/app/views/settings/deletes/show.html.haml @@ -20,7 +20,10 @@ %hr.spacer/ - = f.input :password, wrapper: :with_block_label, input_html: { :autocomplete => 'off' }, hint: t('deletes.confirm_password') + - 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' diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index cd9bacf68..311583820 100644 --- a/config/initializers/devise.rb +++ b/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 diff --git a/config/locales/en.yml b/config/locales/en.yml index 0a5ca31c1..8c9fe89f8 100644 --- a/config/locales/en.yml +++ b/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: diff --git a/spec/controllers/auth/sessions_controller_spec.rb b/spec/controllers/auth/sessions_controller_spec.rb index 87ef4f2bb..7ed5edde0 100644 --- a/spec/controllers/auth/sessions_controller_spec.rb +++ b/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 + 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 - Fabricate(:user, email: 'x@y.com', password: 'abcdefgh', - otp_required_for_login: true, otp_secret: User.generate_otp_secret(32)) + let!(:user) do + Fabricate(:user, email: 'x@y.com', password: 'abcdefgh', 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 |