diff options
author | GitLab Release Tools Bot <robert+release-tools@gitlab.com> | 2019-09-26 16:53:28 +0300 |
---|---|---|
committer | GitLab Release Tools Bot <robert+release-tools@gitlab.com> | 2019-09-26 16:53:28 +0300 |
commit | fc921391d26120198a81be24389cfc1b8c668cbe (patch) | |
tree | 4d65d360a2c9f2135744afbe7a65be21bcc0534c /spec | |
parent | a31eb11c90c3bf00cac0d6f2ec2c3bd1aa96609f (diff) | |
parent | 2b94f55325c737c6acc6866799a0188abc180cf3 (diff) |
Merge branch 'security-sarcila-verify-saml-request-origin-12-3' into '12-3-stable'
Check that SAML identity linking validates the origin of the request
See merge request gitlab/gitlabhq!3396
Diffstat (limited to 'spec')
-rw-r--r-- | spec/controllers/omniauth_callbacks_controller_spec.rb | 58 | ||||
-rw-r--r-- | spec/lib/gitlab/auth/saml/identity_linker_spec.rb | 66 | ||||
-rw-r--r-- | spec/lib/gitlab/auth/saml/origin_validator_spec.rb | 42 | ||||
-rw-r--r-- | spec/lib/omni_auth/strategies/saml_spec.rb | 22 | ||||
-rw-r--r-- | spec/support/omniauth_strategy.rb | 39 |
5 files changed, 191 insertions, 36 deletions
diff --git a/spec/controllers/omniauth_callbacks_controller_spec.rb b/spec/controllers/omniauth_callbacks_controller_spec.rb index 6e374a8daa7..f41cd55ce8c 100644 --- a/spec/controllers/omniauth_callbacks_controller_spec.rb +++ b/spec/controllers/omniauth_callbacks_controller_spec.rb @@ -219,34 +219,70 @@ describe OmniauthCallbacksController, type: :controller do end describe '#saml' do + let(:last_request_id) { 'ONELOGIN_4fee3b046395c4e751011e97f8900b5273d56685' } let(:user) { create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: 'saml') } let(:mock_saml_response) { File.read('spec/fixtures/authentication/saml_response.xml') } let(:saml_config) { mock_saml_config_with_upstream_two_factor_authn_contexts } + def stub_last_request_id(id) + session['last_authn_request_id'] = id + end + before do + stub_last_request_id(last_request_id) stub_omniauth_saml_config({ enabled: true, auto_link_saml_user: true, allow_single_sign_on: ['saml'], providers: [saml_config] }) mock_auth_hash_with_saml_xml('saml', +'my-uid', user.email, mock_saml_response) - request.env["devise.mapping"] = Devise.mappings[:user] + request.env['devise.mapping'] = Devise.mappings[:user] request.env['omniauth.auth'] = Rails.application.env_config['omniauth.auth'] - post :saml, params: { SAMLResponse: mock_saml_response } end - context 'when worth two factors' do - let(:mock_saml_response) do - File.read('spec/fixtures/authentication/saml_response.xml') + context 'with GitLab initiated request' do + before do + post :saml, params: { SAMLResponse: mock_saml_response } + end + + context 'when worth two factors' do + let(:mock_saml_response) do + File.read('spec/fixtures/authentication/saml_response.xml') .gsub('urn:oasis:names:tc:SAML:2.0:ac:classes:Password', 'urn:oasis:names:tc:SAML:2.0:ac:classes:SecondFactorIGTOKEN') + end + + it 'expects user to be signed_in' do + expect(request.env['warden']).to be_authenticated + end end - it 'expects user to be signed_in' do - expect(request.env['warden']).to be_authenticated + context 'when not worth two factors' do + it 'expects user to provide second factor' do + expect(response).to render_template('devise/sessions/two_factor') + expect(request.env['warden']).not_to be_authenticated + end end end - context 'when not worth two factors' do - it 'expects user to provide second factor' do - expect(response).to render_template('devise/sessions/two_factor') - expect(request.env['warden']).not_to be_authenticated + context 'with IdP initiated request' do + let(:user) { create(:user) } + let(:last_request_id) { '99999' } + + before do + sign_in user + end + + it 'lets the user know their account isn\'t linked yet' do + post :saml, params: { SAMLResponse: mock_saml_response } + + expect(flash[:notice]).to eq 'Request to link SAML account must be authorized' + end + + it 'redirects to profile account page' do + post :saml, params: { SAMLResponse: mock_saml_response } + + expect(response).to redirect_to(profile_account_path) + end + + it 'doesn\'t link a new identity to the user' do + expect { post :saml, params: { SAMLResponse: mock_saml_response } }.not_to change { user.identities.count } end end end diff --git a/spec/lib/gitlab/auth/saml/identity_linker_spec.rb b/spec/lib/gitlab/auth/saml/identity_linker_spec.rb index 89118ff05ba..7912c8fb4b1 100644 --- a/spec/lib/gitlab/auth/saml/identity_linker_spec.rb +++ b/spec/lib/gitlab/auth/saml/identity_linker_spec.rb @@ -6,45 +6,61 @@ describe Gitlab::Auth::Saml::IdentityLinker do let(:user) { create(:user) } let(:provider) { 'saml' } let(:uid) { user.email } - let(:oauth) { { 'provider' => provider, 'uid' => uid } } + let(:in_response_to) { '12345' } + let(:saml_response) { instance_double(OneLogin::RubySaml::Response, in_response_to: in_response_to) } + let(:session) { { 'last_authn_request_id' => in_response_to } } - subject { described_class.new(user, oauth) } + let(:oauth) do + OmniAuth::AuthHash.new(provider: provider, uid: uid, extra: { response_object: saml_response }) + end - context 'linked identity exists' do - let!(:identity) { user.identities.create!(provider: provider, extern_uid: uid) } + subject { described_class.new(user, oauth, session) } - it "doesn't create new identity" do - expect { subject.link }.not_to change { Identity.count } - end + context 'with valid GitLab initiated request' do + context 'linked identity exists' do + let!(:identity) { user.identities.create!(provider: provider, extern_uid: uid) } - it "sets #changed? to false" do - subject.link + it "doesn't create new identity" do + expect { subject.link }.not_to change { Identity.count } + end - expect(subject).not_to be_changed - end - end + it "sets #changed? to false" do + subject.link - context 'identity needs to be created' do - it 'creates linked identity' do - expect { subject.link }.to change { user.identities.count } + expect(subject).not_to be_changed + end end - it 'sets identity provider' do - subject.link + context 'identity needs to be created' do + it 'creates linked identity' do + expect { subject.link }.to change { user.identities.count } + end - expect(user.identities.last.provider).to eq provider - end + it 'sets identity provider' do + subject.link - it 'sets identity extern_uid' do - subject.link + expect(user.identities.last.provider).to eq provider + end - expect(user.identities.last.extern_uid).to eq uid + it 'sets identity extern_uid' do + subject.link + + expect(user.identities.last.extern_uid).to eq uid + end + + it 'sets #changed? to true' do + subject.link + + expect(subject).to be_changed + end end + end - it 'sets #changed? to true' do - subject.link + context 'with identity provider initiated request' do + let(:session) { { 'last_authn_request_id' => nil } } - expect(subject).to be_changed + it 'attempting to link accounts raises an exception' do + expect { subject.link }.to raise_error(Gitlab::Auth::Saml::IdentityLinker::UnverifiedRequest) end end end diff --git a/spec/lib/gitlab/auth/saml/origin_validator_spec.rb b/spec/lib/gitlab/auth/saml/origin_validator_spec.rb new file mode 100644 index 00000000000..ae120b328ab --- /dev/null +++ b/spec/lib/gitlab/auth/saml/origin_validator_spec.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Auth::Saml::OriginValidator do + let(:session) { instance_double(ActionDispatch::Request::Session) } + + subject { described_class.new(session) } + + describe '#store_origin' do + it 'stores the SAML request ID' do + request_id = double + authn_request = instance_double(OneLogin::RubySaml::Authrequest, uuid: request_id) + + expect(session).to receive(:[]=).with('last_authn_request_id', request_id) + + subject.store_origin(authn_request) + end + end + + describe '#gitlab_initiated?' do + it 'returns false if InResponseTo is not present' do + saml_response = instance_double(OneLogin::RubySaml::Response, in_response_to: nil) + + expect(subject.gitlab_initiated?(saml_response)).to eq(false) + end + + it 'returns false if InResponseTo does not match stored value' do + saml_response = instance_double(OneLogin::RubySaml::Response, in_response_to: "abc") + allow(session).to receive(:[]).with('last_authn_request_id').and_return('123') + + expect(subject.gitlab_initiated?(saml_response)).to eq(false) + end + + it 'returns true if InResponseTo matches stored value' do + saml_response = instance_double(OneLogin::RubySaml::Response, in_response_to: "123") + allow(session).to receive(:[]).with('last_authn_request_id').and_return('123') + + expect(subject.gitlab_initiated?(saml_response)).to eq(true) + end + end +end diff --git a/spec/lib/omni_auth/strategies/saml_spec.rb b/spec/lib/omni_auth/strategies/saml_spec.rb new file mode 100644 index 00000000000..3c59de86d98 --- /dev/null +++ b/spec/lib/omni_auth/strategies/saml_spec.rb @@ -0,0 +1,22 @@ +require 'spec_helper' + +describe OmniAuth::Strategies::SAML, type: :strategy do + let(:idp_sso_target_url) { 'https://login.example.com/idp' } + let(:strategy) { [OmniAuth::Strategies::SAML, { idp_sso_target_url: idp_sso_target_url }] } + + describe 'POST /users/auth/saml' do + it 'redirects to the provider login page' do + post '/users/auth/saml' + + expect(last_response).to redirect_to(/\A#{Regexp.quote(idp_sso_target_url)}/) + end + + it 'stores request ID during request phase' do + request_id = double + allow_any_instance_of(OneLogin::RubySaml::Authrequest).to receive(:uuid).and_return(request_id) + + post '/users/auth/saml' + expect(session['last_authn_request_id']).to eq(request_id) + end + end +end diff --git a/spec/support/omniauth_strategy.rb b/spec/support/omniauth_strategy.rb new file mode 100644 index 00000000000..eefa04bd9dd --- /dev/null +++ b/spec/support/omniauth_strategy.rb @@ -0,0 +1,39 @@ +module StrategyHelpers + include Rack::Test::Methods + include ActionDispatch::Assertions::ResponseAssertions + include Shoulda::Matchers::ActionController + include OmniAuth::Test::StrategyTestCase + + def post(*args) + super.tap do + @response = ActionDispatch::TestResponse.from_response(last_response) + end + end + + def auth_hash + last_request.env['omniauth.auth'] + end + + def self.without_test_mode + original_mode = OmniAuth.config.test_mode + original_on_failure = OmniAuth.config.on_failure + + OmniAuth.config.test_mode = false + OmniAuth.config.on_failure = OmniAuth::FailureEndpoint + + yield + ensure + OmniAuth.config.test_mode = original_mode + OmniAuth.config.on_failure = original_on_failure + end +end + +RSpec.configure do |config| + config.include StrategyHelpers, type: :strategy + + config.around(:all, type: :strategy) do |example| + StrategyHelpers.without_test_mode do + example.run + end + end +end |