diff options
author | Sebastian Arcila Valenzuela <sarcila@gitlab.com> | 2019-08-19 15:19:19 +0200 |
---|---|---|
committer | Yorick Peterse <yorick@yorickpeterse.com> | 2019-09-30 14:22:06 +0200 |
commit | 3692e9f8a23386c627942ca2a9edd8c00af7e904 (patch) | |
tree | 0d092bdbdfc954e1a9e2b520291a7244c0cd679e /spec/lib/gitlab/auth | |
parent | 010e3c5ed41db96f68549e01373a9aacadd995d7 (diff) | |
download | gitlab-ce-3692e9f8a23386c627942ca2a9edd8c00af7e904.tar.gz |
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
Diffstat (limited to 'spec/lib/gitlab/auth')
-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 |
2 files changed, 83 insertions, 25 deletions
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 |