diff options
| -rw-r--r-- | app/controllers/sessions_controller.rb | 15 | ||||
| -rw-r--r-- | spec/controllers/sessions_controller_spec.rb | 77 |
2 files changed, 51 insertions, 41 deletions
diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 65677a3dd3c..c29f4609e93 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -5,7 +5,8 @@ class SessionsController < Devise::SessionsController skip_before_action :check_2fa_requirement, only: [:destroy] prepend_before_action :check_initial_setup, only: [:new] - prepend_before_action :authenticate_with_two_factor, only: [:create] + prepend_before_action :authenticate_with_two_factor, + if: :two_factor_enabled?, only: [:create] prepend_before_action :store_redirect_path, only: [:new] before_action :auto_sign_in_with_provider, only: [:new] @@ -56,10 +57,10 @@ class SessionsController < Devise::SessionsController end def find_user - if user_params[:login] - User.by_login(user_params[:login]) - elsif user_params[:otp_attempt] && session[:otp_user_id] + if session[:otp_user_id] User.find(session[:otp_user_id]) + elsif user_params[:login] + User.by_login(user_params[:login]) end end @@ -83,11 +84,13 @@ class SessionsController < Devise::SessionsController end end + def two_factor_enabled? + find_user.try(:two_factor_enabled?) + end + def authenticate_with_two_factor user = self.resource = find_user - return unless user && user.two_factor_enabled? - if user_params[:otp_attempt].present? && session[:otp_user_id] if valid_otp_attempt?(user) # Remove any lingering user data from login diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index e7dbc3bdad4..664b23d3214 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -6,7 +6,7 @@ describe SessionsController do @request.env['devise.mapping'] = Devise.mappings[:user] end - context 'standard authentications' do + context 'when using standard authentications' do context 'invalid password' do it 'does not authenticate user' do post(:create, user: { login: 'invalid', password: 'invalid' }) @@ -16,7 +16,7 @@ describe SessionsController do end end - context 'valid password' do + context 'when using valid password' do let(:user) { create(:user) } it 'authenticates user correctly' do @@ -28,7 +28,7 @@ describe SessionsController do end end - context 'two-factor authentication' do + context 'when using two-factor authentication' do let(:user) { create(:user, :two_factor) } def authenticate_2fa(user_params) @@ -38,52 +38,59 @@ describe SessionsController do ## # See #14900 issue # - context 'authenticating with login and OTP belonging to another user' do - let(:another_user) { create(:user, :two_factor) } + context 'when authenticating with login and OTP of another user' do + context 'when another user has 2FA enabled' do + let(:another_user) { create(:user, :two_factor) } + context 'when OTP is valid for another user' do + it 'does not authenticate' do + authenticate_2fa(login: another_user.username, + otp_attempt: another_user.current_otp) - context 'OTP valid for another user' do - it 'does not authenticate' do - authenticate_2fa(login: another_user.username, - otp_attempt: another_user.current_otp) - - expect(subject.current_user).to_not eq another_user + expect(subject.current_user).to_not eq another_user + end end - end - context 'OTP invalid for another user' do - before do - authenticate_2fa(login: another_user.username, - otp_attempt: 'invalid') - end + context 'when OTP is invalid for another user' do + it 'does not authenticate' do + authenticate_2fa(login: another_user.username, + otp_attempt: 'invalid') - it 'does not authenticate' do - expect(subject.current_user).to_not eq another_user + expect(subject.current_user).to_not eq another_user + end end - it 'does not leak information about 2FA enabled' do - expect(response).to_not set_flash.now[:alert].to /Invalid two-factor code/ - end - end + context 'when authenticating with OTP' do + context 'when OTP is valid' do + it 'authenticates correctly' do + authenticate_2fa(otp_attempt: user.current_otp) + + expect(subject.current_user).to eq user + end + end - context 'authenticating with OTP' do - context 'valid OTP' do - it 'authenticates correctly' do - authenticate_2fa(otp_attempt: user.current_otp) + context ' when OTP is invalid' do + before { authenticate_2fa(otp_attempt: 'invalid') } - expect(subject.current_user).to eq user + it 'does not authenticate' do + expect(subject.current_user).to_not eq user + end + + it 'warns about invalid OTP code' do + expect(response).to set_flash + .now[:alert].to /Invalid two-factor code/ + end end end - context 'invalid OTP' do - before { authenticate_2fa(otp_attempt: 'invalid') } + context 'when another user does not have 2FA enabled' do + let(:another_user) { create(:user) } - it 'does not authenticate' do - expect(subject.current_user).to_not eq user - end + it 'does not leak that 2FA is disabled for another user' do + authenticate_2fa(login: another_user.username, + otp_attempt: 'invalid') - it 'warns about invalid OTP code' do - expect(response).to set_flash.now[:alert].to /Invalid two-factor code/ + expect(response).to_not set_flash.now[:alert].to /Invalid login or password/ end end end |