From 21338fd70cc1e3e07a7a88f4546899522fb28385 Mon Sep 17 00:00:00 2001 From: Sebastian Arcila Valenzuela Date: Mon, 19 Aug 2019 15:19:19 +0200 Subject: Validate that SAML requests are originated from gitlab If the request wasn't initiated by gitlab we shouldn't add the new identity to the user, and instead show that we weren't able to link the identity to the user. This should fix: https://gitlab.com/gitlab-org/gitlab-ce/issues/56509 --- .../omniauth_callbacks_controller_spec.rb | 58 +++++++++++++++---- spec/lib/gitlab/auth/saml/identity_linker_spec.rb | 66 ++++++++++++++-------- spec/lib/gitlab/auth/saml/origin_validator_spec.rb | 42 ++++++++++++++ spec/lib/omni_auth/strategies/saml_spec.rb | 22 ++++++++ spec/support/omniauth_strategy.rb | 39 +++++++++++++ 5 files changed, 191 insertions(+), 36 deletions(-) create mode 100644 spec/lib/gitlab/auth/saml/origin_validator_spec.rb create mode 100644 spec/lib/omni_auth/strategies/saml_spec.rb create mode 100644 spec/support/omniauth_strategy.rb (limited to 'spec') 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 f3305d574cc..6c4db25a02f 100644 --- a/spec/lib/gitlab/auth/saml/identity_linker_spec.rb +++ b/spec/lib/gitlab/auth/saml/identity_linker_spec.rb @@ -4,45 +4,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 -- cgit v1.2.1