diff --git a/ee/app/controllers/groups/omniauth_callbacks_controller.rb b/ee/app/controllers/groups/omniauth_callbacks_controller.rb index f0c444e2e63c5d60f25f7ffe288c9cdfb36a8462..1f281aae91713fa4fe7f7638e79dab0fd8feb9bf 100644 --- a/ee/app/controllers/groups/omniauth_callbacks_controller.rb +++ b/ee/app/controllers/groups/omniauth_callbacks_controller.rb @@ -45,12 +45,16 @@ def redirect_identity_exists override :redirect_identity_link_failed def redirect_identity_link_failed(error_message) - flash[:notice] = "SAML authentication failed: #{error_message}" + flash[:alert] = format( + s_("GroupSAML|%{group_name} SAML authentication failed: %{message}"), + group_name: @unauthenticated_group.name, + message: error_message + ) if ::Feature.enabled?(:sign_up_on_sso, @unauthenticated_group) && @saml_provider.enforced_group_managed_accounts? redirect_to_group_sign_up else - redirect_to after_sign_in_path_for(current_user) + redirect_to root_path end end diff --git a/ee/lib/gitlab/auth/group_saml/identity_linker.rb b/ee/lib/gitlab/auth/group_saml/identity_linker.rb index d5f11e74a0968f8ade34c0901292b11e8299976a..fc608a71743e388e244d61adf3943a3b8e236d4a 100644 --- a/ee/lib/gitlab/auth/group_saml/identity_linker.rb +++ b/ee/lib/gitlab/auth/group_saml/identity_linker.rb @@ -4,23 +4,45 @@ module Gitlab module Auth module GroupSaml class IdentityLinker < Gitlab::Auth::Saml::IdentityLinker - attr_reader :saml_provider + include ::Gitlab::Utils::StrongMemoize + attr_reader :saml_provider, :auth_hash def initialize(current_user, oauth, session, saml_provider) super(current_user, oauth, session) + @auth_hash = AuthHash.new(oauth) @saml_provider = saml_provider end + override :link + def link + super + + update_extern_uid if extern_uid_update_required? + end + + override :failed? + def failed? + super || update_extern_uid_failed? + end + + override :error_message + def error_message + return super unless update_extern_uid_failed? + + s_('GroupSAML|SAML Name ID and email address do not match your user account. Contact an administrator.') + end + protected # rubocop: disable CodeReuse/ActiveRecord + override :identity def identity - @identity ||= current_user.identities.where(provider: :group_saml, - saml_provider: saml_provider, - extern_uid: uid.to_s) - .first_or_initialize + current_user.identities + .where(provider: :group_saml, saml_provider: saml_provider) + .first_or_initialize(extern_uid: uid.to_s) end + strong_memoize_attr :identity # rubocop: enable CodeReuse/ActiveRecord override :update_group_membership @@ -28,6 +50,32 @@ def update_group_membership auth_hash = AuthHash.new(oauth) MembershipUpdater.new(current_user, saml_provider, auth_hash).execute end + + def update_extern_uid + return unless update_extern_uid_allowed? + + identity.extern_uid = uid.to_s + save + end + + # When the current extern_uid doesn't match the + # uid (Name ID) from SAML, we need to update. + def extern_uid_update_required? + identity.extern_uid != uid.to_s + end + + # Updating the extern_uid is only allowed if the email + # address sent from the IdP matches a verified email + # address of the current user. This prevents accidentally + # linking an unintended IdP account. + def update_extern_uid_allowed? + current_user.verified_email?(auth_hash.email) + end + strong_memoize_attr :update_extern_uid_allowed?, :update_extern_uid_allowed + + def update_extern_uid_failed? + extern_uid_update_required? && !update_extern_uid_allowed? + end end end end diff --git a/ee/spec/controllers/groups/omniauth_callbacks_controller_spec.rb b/ee/spec/controllers/groups/omniauth_callbacks_controller_spec.rb index c253c6ec2aa88bb46fbab641a3a7b6163a2b3420..19ea3e8d55ce0217c9aef5fd14214bd039ecfebd 100644 --- a/ee/spec/controllers/groups/omniauth_callbacks_controller_spec.rb +++ b/ee/spec/controllers/groups/omniauth_callbacks_controller_spec.rb @@ -177,10 +177,29 @@ def stub_last_request_id(id) create(:identity, user: user, extern_uid: 'some-other-name-id', provider: provider, saml_provider: saml_provider) end - it 'displays warning to user' do + it "displays a flash message verifying group sign in" do post provider, params: { group_id: group } - expect(flash[:notice]).to match(/has already been taken*/) + expect(flash[:notice]).to eq(s_("SAML|Your organization's SSO has been connected to your GitLab account")) + end + + context 'when user email address does not match auth hash email address' do + before do + mock_auth_hash(provider, uid, generate(:email), response_object: saml_response) + stub_omniauth_provider(provider, context: request) + end + + it 'redirects and displays an error', :aggregate_failures do + post provider, params: { group_id: group } + + expect(flash[:alert]) + .to eq(format( + s_("GroupSAML|%{group_name} SAML authentication failed: %{message}"), + group_name: group.name, + message: s_('GroupSAML|SAML Name ID and email address do not match your user account. Contact an administrator.') + )) + expect(response).to redirect_to(root_path) + end end end @@ -189,10 +208,16 @@ def stub_last_request_id(id) create_linked_user end - it 'displays warning to user' do + it 'redirects and displays an error' do post provider, params: { group_id: group } - expect(flash[:notice]).to match(/has already been taken*/) + expect(flash[:alert]) + .to eq(format( + s_("GroupSAML|%{group_name} SAML authentication failed: %{message}"), + group_name: group.name, + message: 'Extern uid has already been taken' + )) + expect(response).to redirect_to(root_path) end end diff --git a/ee/spec/lib/gitlab/auth/group_saml/identity_linker_spec.rb b/ee/spec/lib/gitlab/auth/group_saml/identity_linker_spec.rb index 1ed9c4b38f4859402067ddb913f5039545de7629..7a4d1c36f607fd61208130d01d84b42c0c3f5002 100644 --- a/ee/spec/lib/gitlab/auth/group_saml/identity_linker_spec.rb +++ b/ee/spec/lib/gitlab/auth/group_saml/identity_linker_spec.rb @@ -8,29 +8,79 @@ let(:uid) { user.email } let(:in_response_to) { '12345' } let(:saml_response) { instance_double(OneLogin::RubySaml::Response, in_response_to: in_response_to) } - let(:oauth) { OmniAuth::AuthHash.new(provider: provider, uid: uid, extra: { response_object: saml_response }) } let(:saml_provider) { create(:saml_provider) } let(:session) { {} } + let(:oauth) do + OmniAuth::AuthHash.new(provider: provider, uid: uid, + info: { email: user.email }, extra: { response_object: saml_response }) + end - subject { described_class.new(user, oauth, session, saml_provider) } + subject(:identity_linker) { described_class.new(user, oauth, session, saml_provider) } context 'linked identity exists' do - let!(:identity) { user.identities.create!(provider: provider, extern_uid: uid, saml_provider: saml_provider) } - - it "doesn't create new identity" do - expect { subject.link }.not_to change { Identity.count } + let!(:identity) do + user.identities.create!(provider: provider, extern_uid: extern_uid, saml_provider: saml_provider) end - it "sets #changed? to false" do - subject.link + context 'when the extern_uid matches' do + let(:extern_uid) { uid } + + it "doesn't create new identity" do + expect { subject.link }.not_to change { Identity.count } + end + + it "sets #changed? to false" do + subject.link + + expect(subject).not_to be_changed + end + + it 'adds user to group' do + subject.link - expect(subject).not_to be_changed + expect(saml_provider.group.member?(user)).to eq(true) + end end - it 'adds user to group' do - subject.link + context 'when the extern_uid does not match' do + let(:extern_uid) { 'ioKaiph5' } + + it 'updates the identity when the email address matches' do + expect(identity.extern_uid).to eq(extern_uid) + + identity_linker.link + + expect(identity.reload.extern_uid).to eq(uid) + expect(identity_linker.failed?).to eq(false) + expect(identity_linker.error_message).to be_empty + end + + it 'does not update the identity when the email address does not match', :aggregate_failures do + oauth.info.email = generate(:email) - expect(saml_provider.group.member?(user)).to eq(true) + identity_linker.link + + expect(identity.reload.extern_uid).to eq(extern_uid) + expect(identity_linker.failed?).to eq(true) + expect(identity_linker.error_message) + .to eq( + s_('GroupSAML|SAML Name ID and email address do not match your user account. Contact an administrator.') + ) + end + + context 'when the extern_uid is already taken' do + before do + saml_provider.identities.create!(provider: provider, extern_uid: uid, user: create(:user)) + end + + it 'fails and does not update the identity', :aggregate_failures do + identity_linker.link + + expect(identity.reload.extern_uid).to eq(extern_uid) + expect(identity_linker.failed?).to eq(true) + expect(identity_linker.error_message).to eq('Extern uid has already been taken') + end + end end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 7abfb806d2a145095f42e9bbef90c0aad3563f98..a5ff887d39813341c0fe30d7f275bec3656efeb3 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -19146,6 +19146,9 @@ msgstr "" msgid "GroupSAML|\"persistent\" recommended" msgstr "" +msgid "GroupSAML|%{group_name} SAML authentication failed: %{message}" +msgstr "" + msgid "GroupSAML|%{strongOpen}Warning%{strongClose} - Enable %{linkStart}SSO enforcement%{linkEnd} to reduce security risks." msgstr "" @@ -19251,6 +19254,9 @@ msgstr "" msgid "GroupSAML|SAML Group Name: %{saml_group_name}" msgstr "" +msgid "GroupSAML|SAML Name ID and email address do not match your user account. Contact an administrator." +msgstr "" + msgid "GroupSAML|SAML Response Output" msgstr ""