From 301f4074aa05f25757396182490c3ebfffe1e81c Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 6 Apr 2016 12:26:10 +0200 Subject: Add specs for sessions controller including 2FA This also contains specs for a bug described in #14900 --- spec/controllers/sessions_controller_spec.rb | 93 ++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+) create mode 100644 spec/controllers/sessions_controller_spec.rb (limited to 'spec/controllers/sessions_controller_spec.rb') diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb new file mode 100644 index 00000000000..e7dbc3bdad4 --- /dev/null +++ b/spec/controllers/sessions_controller_spec.rb @@ -0,0 +1,93 @@ +require 'spec_helper' + +describe SessionsController do + describe '#create' do + before do + @request.env['devise.mapping'] = Devise.mappings[:user] + end + + context '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 '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 '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 'authenticating with login and OTP belonging to another user' do + let(:another_user) { create(:user, :two_factor) } + + + 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 + end + end + + context 'OTP invalid for another user' do + before do + authenticate_2fa(login: another_user.username, + otp_attempt: 'invalid') + end + + it 'does not authenticate' do + expect(subject.current_user).to_not eq another_user + 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 'authenticating with OTP' do + context 'valid OTP' do + it 'authenticates correctly' do + authenticate_2fa(otp_attempt: user.current_otp) + + expect(subject.current_user).to eq user + end + end + + context 'invalid OTP' 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 + end + end + end +end -- cgit v1.2.1 From 00da609cfd8bf1105fe433dfc92ab263d6205eaf Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 7 Apr 2016 11:19:29 +0200 Subject: Fix 2FA authentication spoofing vulnerability This commit attempts to change default user search scope if otp_user_id session variable has been set. If it is present, it means that user has 2FA enabled, and has already been verified with login and password. In this case we should look for user with otp_user_id first, before picking it up by login. --- spec/controllers/sessions_controller_spec.rb | 77 +++++++++++++++------------- 1 file changed, 42 insertions(+), 35 deletions(-) (limited to 'spec/controllers/sessions_controller_spec.rb') 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 -- cgit v1.2.1 From 33a8dfd04fbd1c0858ead20c020ede07e7b0962a Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 7 Apr 2016 11:45:04 +0200 Subject: Make sessions controller specs more explicit --- spec/controllers/sessions_controller_spec.rb | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) (limited to 'spec/controllers/sessions_controller_spec.rb') diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index 664b23d3214..83cc8ec6d26 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -69,7 +69,7 @@ describe SessionsController do end end - context ' when OTP is invalid' do + context 'when OTP is invalid' do before { authenticate_2fa(otp_attempt: 'invalid') } it 'does not authenticate' do @@ -77,8 +77,8 @@ describe SessionsController do end it 'warns about invalid OTP code' do - expect(response).to set_flash - .now[:alert].to /Invalid two-factor code/ + expect(response).to set_flash.now[:alert] + .to /Invalid two-factor code/ end end end @@ -90,7 +90,8 @@ describe SessionsController do authenticate_2fa(login: another_user.username, otp_attempt: 'invalid') - expect(response).to_not set_flash.now[:alert].to /Invalid login or password/ + expect(response).to set_flash.now[:alert] + .to /Invalid two-factor code/ end end end -- cgit v1.2.1