Merge branch 'master' of dev.gitlab.org:gitlab/gitlabhq
* 'master' of dev.gitlab.org:gitlab/gitlabhq: Make sessions controller specs more explicit Fix 2FA authentication spoofing vulnerability Add specs for sessions controller including 2FA
This commit is contained in:
commit
b30ebdaa1a
|
@ -5,7 +5,8 @@ class SessionsController < Devise::SessionsController
|
||||||
skip_before_action :check_2fa_requirement, only: [:destroy]
|
skip_before_action :check_2fa_requirement, only: [:destroy]
|
||||||
|
|
||||||
prepend_before_action :check_initial_setup, only: [:new]
|
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]
|
prepend_before_action :store_redirect_path, only: [:new]
|
||||||
|
|
||||||
before_action :auto_sign_in_with_provider, only: [:new]
|
before_action :auto_sign_in_with_provider, only: [:new]
|
||||||
|
@ -56,10 +57,10 @@ class SessionsController < Devise::SessionsController
|
||||||
end
|
end
|
||||||
|
|
||||||
def find_user
|
def find_user
|
||||||
if user_params[:login]
|
if session[:otp_user_id]
|
||||||
User.by_login(user_params[:login])
|
|
||||||
elsif user_params[:otp_attempt] && session[:otp_user_id]
|
|
||||||
User.find(session[:otp_user_id])
|
User.find(session[:otp_user_id])
|
||||||
|
elsif user_params[:login]
|
||||||
|
User.by_login(user_params[:login])
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
@ -83,11 +84,13 @@ class SessionsController < Devise::SessionsController
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def two_factor_enabled?
|
||||||
|
find_user.try(:two_factor_enabled?)
|
||||||
|
end
|
||||||
|
|
||||||
def authenticate_with_two_factor
|
def authenticate_with_two_factor
|
||||||
user = self.resource = find_user
|
user = self.resource = find_user
|
||||||
|
|
||||||
return unless user && user.two_factor_enabled?
|
|
||||||
|
|
||||||
if user_params[:otp_attempt].present? && session[:otp_user_id]
|
if user_params[:otp_attempt].present? && session[:otp_user_id]
|
||||||
if valid_otp_attempt?(user)
|
if valid_otp_attempt?(user)
|
||||||
# Remove any lingering user data from login
|
# Remove any lingering user data from login
|
||||||
|
|
|
@ -0,0 +1,101 @@
|
||||||
|
require 'spec_helper'
|
||||||
|
|
||||||
|
describe SessionsController do
|
||||||
|
describe '#create' do
|
||||||
|
before do
|
||||||
|
@request.env['devise.mapping'] = Devise.mappings[:user]
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'when using standard authentications' do
|
||||||
|
context 'invalid password' do
|
||||||
|
it 'does not authenticate user' do
|
||||||
|
post(:create, user: { login: 'invalid', password: 'invalid' })
|
||||||
|
|
||||||
|
expect(response)
|
||||||
|
.to set_flash.now[:alert].to /Invalid login or password/
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'when using valid password' do
|
||||||
|
let(:user) { create(:user) }
|
||||||
|
|
||||||
|
it 'authenticates user correctly' do
|
||||||
|
post(:create, user: { login: user.username, password: user.password })
|
||||||
|
|
||||||
|
expect(response).to set_flash.to /Signed in successfully/
|
||||||
|
expect(subject.current_user). to eq user
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'when using two-factor authentication' do
|
||||||
|
let(:user) { create(:user, :two_factor) }
|
||||||
|
|
||||||
|
def authenticate_2fa(user_params)
|
||||||
|
post(:create, { user: user_params }, { otp_user_id: user.id })
|
||||||
|
end
|
||||||
|
|
||||||
|
##
|
||||||
|
# See #14900 issue
|
||||||
|
#
|
||||||
|
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)
|
||||||
|
|
||||||
|
expect(subject.current_user).to_not eq another_user
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
context 'when OTP is invalid for another user' do
|
||||||
|
it 'does not authenticate' do
|
||||||
|
authenticate_2fa(login: another_user.username,
|
||||||
|
otp_attempt: 'invalid')
|
||||||
|
|
||||||
|
expect(subject.current_user).to_not eq another_user
|
||||||
|
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 'when OTP is invalid' do
|
||||||
|
before { authenticate_2fa(otp_attempt: 'invalid') }
|
||||||
|
|
||||||
|
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 'when another user does not have 2FA enabled' do
|
||||||
|
let(:another_user) { create(:user) }
|
||||||
|
|
||||||
|
it 'does not leak that 2FA is disabled for another user' do
|
||||||
|
authenticate_2fa(login: another_user.username,
|
||||||
|
otp_attempt: 'invalid')
|
||||||
|
|
||||||
|
expect(response).to set_flash.now[:alert]
|
||||||
|
.to /Invalid two-factor code/
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
||||||
|
end
|
Loading…
Reference in New Issue