diff --git a/app/assets/stylesheets/pages/profile.scss b/app/assets/stylesheets/pages/profile.scss index 7fbec963ab924e6216a24c01d042128f7107ed27..531f6a5ecbf37cc36d3fb37768fa675a3ed632ce 100644 --- a/app/assets/stylesheets/pages/profile.scss +++ b/app/assets/stylesheets/pages/profile.scss @@ -144,11 +144,13 @@ .provider-btn-group { display: inline-block; margin-right: 10px; + margin-bottom: 10px; border: 1px solid $border-color; border-radius: 3px; &:last-child { margin-right: 0; + margin-bottom: 0; } } diff --git a/app/controllers/profiles/accounts_controller.rb b/app/controllers/profiles/accounts_controller.rb index cb3180f419676ec83209de3dce0d282e94e927ba..8d60799390382ff6bcd3ac621c94116c5b8187fc 100644 --- a/app/controllers/profiles/accounts_controller.rb +++ b/app/controllers/profiles/accounts_controller.rb @@ -4,7 +4,7 @@ class Profiles::AccountsController < Profiles::ApplicationController include AuthHelper def show - @user = current_user + render(locals: show_view_variables) end # rubocop: disable CodeReuse/ActiveRecord @@ -23,4 +23,12 @@ def unlink redirect_to profile_account_path end # rubocop: enable CodeReuse/ActiveRecord + + private + + def show_view_variables + {} + end end + +Profiles::AccountsController.prepend(EE::Profiles::AccountsController) diff --git a/app/helpers/auth_helper.rb b/app/helpers/auth_helper.rb index b038f6e612fcd42dee97e5aa09df9d44c30c8001..0d9da67ec0f72ad38bd9a469007ddb1dabd3572c 100644 --- a/app/helpers/auth_helper.rb +++ b/app/helpers/auth_helper.rb @@ -57,6 +57,10 @@ def button_based_providers auth_providers.reject { |provider| form_based_provider?(provider) } end + def display_providers_on_profile? + button_based_providers.any? + end + def providers_for_base_controller auth_providers.reject { |provider| LDAP_PROVIDER === provider } end diff --git a/app/views/profiles/accounts/show.html.haml b/app/views/profiles/accounts/show.html.haml index 2220b4eee96e3f16e90603f60c77dfe27194fffa..e167e094240fbe511e77f4ab5939aa5889c9e35c 100644 --- a/app/views/profiles/accounts/show.html.haml +++ b/app/views/profiles/accounts/show.html.haml @@ -21,7 +21,7 @@ = link_to 'Enable two-factor authentication', profile_two_factor_auth_path, class: 'btn btn-success' %hr -- if button_based_providers.any? +- if display_providers_on_profile? .row.prepend-top-default .col-lg-4.profile-settings-sidebar %h4.prepend-top-0 @@ -46,6 +46,7 @@ - else = link_to omniauth_authorize_path(:user, provider), method: :post, class: 'provider-btn not-active' do Connect + = render_if_exists 'profiles/accounts/group_saml_unlink_buttons', group_saml_identities: local_assigns[:group_saml_identities] %hr - if current_user.can_change_username? .row.prepend-top-default @@ -66,7 +67,7 @@ %h4.prepend-top-0.danger-title = s_('Profiles|Delete account') .col-lg-8 - - if @user.can_be_removed? && can?(current_user, :destroy_user, @user) + - if current_user.can_be_removed? && can?(current_user, :destroy_user, current_user) %p = s_('Profiles|Deleting an account has the following effects:') = render 'users/deletion_guidance', user: current_user @@ -79,10 +80,10 @@ confirm_with_password: ('true' if current_user.confirm_deletion_with_password?), username: current_user.username } } - else - - if @user.solo_owned_groups.present? + - if current_user.solo_owned_groups.present? %p = s_('Profiles|Your account is currently an owner in these groups:') - %strong= @user.solo_owned_groups.map(&:name).join(', ') + %strong= current_user.solo_owned_groups.map(&:name).join(', ') %p = s_('Profiles|You must transfer ownership or delete these groups before you can delete your account.') - else diff --git a/ee/app/controllers/ee/profiles/accounts_controller.rb b/ee/app/controllers/ee/profiles/accounts_controller.rb new file mode 100644 index 0000000000000000000000000000000000000000..79b556f4c4cbfa795472b42870facef6d46b91c3 --- /dev/null +++ b/ee/app/controllers/ee/profiles/accounts_controller.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +module EE + module Profiles::AccountsController + extend ::Gitlab::Utils::Override + + private + + override :show_view_variables + def show_view_variables + group_saml_identities = GroupSamlIdentityFinder.new(user: current_user).all + + super.merge(group_saml_identities: group_saml_identities) + end + end +end diff --git a/ee/app/controllers/groups/sso_controller.rb b/ee/app/controllers/groups/sso_controller.rb index d1c7abcf491a9f8fe2ba679861b18fedaebc76ab..a5ade772afc3c68965e253708056e652e64d12e9 100644 --- a/ee/app/controllers/groups/sso_controller.rb +++ b/ee/app/controllers/groups/sso_controller.rb @@ -6,7 +6,8 @@ class Groups::SsoController < Groups::ApplicationController before_action :check_group_saml_configured before_action :check_group_saml_available! before_action :require_configured_provider - before_action :check_user_can_sign_in_with_provider + before_action :authenticate_user!, only: [:unlink] + before_action :check_user_can_sign_in_with_provider, only: [:saml] before_action :redirect_if_group_moved layout 'devise' @@ -16,8 +17,20 @@ def saml @group_name = @unauthenticated_group.full_name end + def unlink + return route_not_found unless linked_identity + + GroupSaml::Identity::DestroyService.new(linked_identity).execute + + redirect_to profile_account_path + end + private + def linked_identity + @linked_identity ||= GroupSamlIdentityFinder.new(user: current_user).find_linked(group: @unauthenticated_group) + end + def check_group_saml_available! route_not_found unless @unauthenticated_group.feature_available?(:group_saml) end diff --git a/ee/app/finders/group_saml_identity_finder.rb b/ee/app/finders/group_saml_identity_finder.rb new file mode 100644 index 0000000000000000000000000000000000000000..f9a415d1207f6e1dbd2a51ce123b661a77c83def --- /dev/null +++ b/ee/app/finders/group_saml_identity_finder.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class GroupSamlIdentityFinder + attr_reader :user + + def initialize(user:) + @user = user + end + + def find_linked(group:) + return unless user + + group&.saml_provider&.identities&.find_by(user: user) + end + + def all + user.group_saml_identities.preload_saml_group + end +end diff --git a/ee/app/helpers/ee/auth_helper.rb b/ee/app/helpers/ee/auth_helper.rb index b5154fc8ea562106f20cdb819cc1913330e1d6dc..42933dbdd433cb862ea066c5b6ba8ff7ad1f8426 100644 --- a/ee/app/helpers/ee/auth_helper.rb +++ b/ee/app/helpers/ee/auth_helper.rb @@ -8,6 +8,11 @@ module AuthHelper delegate :slack_app_id, to: :'Gitlab::CurrentSettings.current_application_settings' + override :display_providers_on_profile? + def display_providers_on_profile? + super || group_saml_enabled? + end + override :button_based_providers def button_based_providers super - GROUP_LEVEL_PROVIDERS @@ -45,6 +50,10 @@ def smartcard_enabled? ::Gitlab::Auth::Smartcard.enabled? end + def group_saml_enabled? + auth_providers.include?(:group_saml) + end + def slack_redirect_uri(project) slack_auth_project_settings_slack_url(project) end diff --git a/ee/app/models/ee/identity.rb b/ee/app/models/ee/identity.rb index 9da95000491f190b4c4381e6c91e69527c619fca..b145774ca1aab775c1daf5cdec63f01d87a92196 100644 --- a/ee/app/models/ee/identity.rb +++ b/ee/app/models/ee/identity.rb @@ -18,5 +18,11 @@ module Identity iwhere(secondary_extern_uid: normalize_uid(provider, secondary_extern_uid)).with_provider(provider) end end + + class_methods do + def preload_saml_group + preload(saml_provider: { group: :route }) + end + end end end diff --git a/ee/app/services/group_saml/identity/destroy_service.rb b/ee/app/services/group_saml/identity/destroy_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..c6e155382e3657d26f859bca68a80f7982ef7d66 --- /dev/null +++ b/ee/app/services/group_saml/identity/destroy_service.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +module GroupSaml + module Identity + class DestroyService + attr_reader :identity + + delegate :user, to: :identity + + def initialize(identity) + @identity = identity + end + + def execute + identity.destroy! + remove_group_access + end + + private + + def remove_group_access + return unless group_membership + return if group.last_owner?(user) + + Members::DestroyService.new(user).execute(group_membership) + end + + def group + @group ||= identity.saml_provider.group + end + + def group_membership + @group_membership ||= group.group_member(user) + end + end + end +end diff --git a/ee/app/views/profiles/accounts/_group_saml_unlink_buttons.html.haml b/ee/app/views/profiles/accounts/_group_saml_unlink_buttons.html.haml new file mode 100644 index 0000000000000000000000000000000000000000..bd54d52441e278f147c03c8af6bb78bda7f6626a --- /dev/null +++ b/ee/app/views/profiles/accounts/_group_saml_unlink_buttons.html.haml @@ -0,0 +1,7 @@ +- group_saml_identities.each do |identity| + - group = identity.saml_provider.group + .provider-btn-group + .provider-btn-image + = _("SAML for %{group_name}") % { group_name: group.name } + = link_to unlink_group_saml_providers_path(group), method: :delete, class: 'provider-btn' do + Disconnect diff --git a/ee/changelogs/unreleased/jej-group-saml-unlink-from-account.yml b/ee/changelogs/unreleased/jej-group-saml-unlink-from-account.yml new file mode 100644 index 0000000000000000000000000000000000000000..92182b383958fa4a478aa106b94edf47d8b67940 --- /dev/null +++ b/ee/changelogs/unreleased/jej-group-saml-unlink-from-account.yml @@ -0,0 +1,5 @@ +--- +title: Users can unlink Group SAML from accounts page +merge_request: 8682 +author: +type: changed diff --git a/ee/config/routes/group.rb b/ee/config/routes/group.rb index 2f53e33759256deb363c0ec71258e9e90f653c86..2288aee1bf39c6f7d132357c44a1a361ec3089b3 100644 --- a/ee/config/routes/group.rb +++ b/ee/config/routes/group.rb @@ -77,6 +77,7 @@ resource :saml_providers, path: 'saml', only: [:show, :create, :update] do post :callback, to: 'omniauth_callbacks#group_saml' get :sso, to: 'sso#saml' + delete :unlink, to: 'sso#unlink' end resource :roadmap, only: [:show], controller: 'roadmap' diff --git a/ee/spec/factories/group_saml_identities.rb b/ee/spec/factories/group_saml_identities.rb new file mode 100644 index 0000000000000000000000000000000000000000..6d425da743c42245334bc963d810f3b64da0c6d1 --- /dev/null +++ b/ee/spec/factories/group_saml_identities.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true +FactoryBot.define do + factory :group_saml_identity, class: Identity, parent: :identity do + provider 'group_saml' + extern_uid { generate(:username) } + saml_provider + user + end +end diff --git a/ee/spec/features/profiles/account_spec.rb b/ee/spec/features/profiles/account_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..597a3df5b3c4cc1e07f050699aad32363a0ffaf6 --- /dev/null +++ b/ee/spec/features/profiles/account_spec.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe 'Profile > Account' do + let(:user) { create(:user) } + + before do + sign_in(user) + end + + describe "Disconnect Group SAML", :js do + let(:group) { create(:group, :private, name: 'Test Group') } + let(:saml_provider) { create(:saml_provider, group: group) } + + def enable_group_saml + stub_licensed_features(group_saml: true) + allow(Devise).to receive(:omniauth_providers).and_return(%i(group_saml)) + end + + def create_linked_identity + oauth = { 'provider' => 'group_saml', 'uid' => '1' } + Gitlab::Auth::GroupSaml::IdentityLinker.new(user, oauth, saml_provider).link + end + + before do + enable_group_saml + create_linked_identity + end + + it 'unlinks account' do + visit profile_account_path + + unlink_label = "SAML for Test Group" + + expect(page).to have_content unlink_label + click_link "Disconnect" + + expect(current_path).to eq profile_account_path + expect(page).not_to have_content(unlink_label) + + visit group_path(group) + expect(page).to have_content('Page Not Found') + end + end +end diff --git a/ee/spec/finders/group_saml_identity_finder_spec.rb b/ee/spec/finders/group_saml_identity_finder_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..db096089accf8fdfdf2d64bb43eb1c255ad5b2cc --- /dev/null +++ b/ee/spec/finders/group_saml_identity_finder_spec.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe GroupSamlIdentityFinder do + include Gitlab::Routing + + let(:user) { create(:user) } + let!(:identity) { create(:group_saml_identity, user: user) } + let(:group) { identity.saml_provider.group } + + subject { described_class.new(user: user) } + + describe "#find_linked" do + it "finds identity matching user and group" do + expect(subject.find_linked(group: group)).to eq(identity) + end + + it "returns nil when no saml_provider exists" do + group.saml_provider.destroy! + + expect(subject.find_linked(group: group)).to eq(nil) + end + + it "returns nil when group is nil" do + expect(subject.find_linked(group: nil)).to eq(nil) + end + end + + describe "#all" do + it "finds Group SAML identities for a user" do + expect(subject.all.first).to eq(identity) + end + + it "avoids N+1 on access to provider and group path" do + identity = subject.all.first + + expect { group_path(identity.saml_provider.group) }.not_to exceed_query_limit(0) + end + end +end diff --git a/ee/spec/models/ee/members_preloader_spec.rb b/ee/spec/models/ee/members_preloader_spec.rb index fe01212cb021c834bf97975fc1b7ecb0826d5004..7dec082e054283ba8edd6edf56a7d11a86227547 100644 --- a/ee/spec/models/ee/members_preloader_spec.rb +++ b/ee/spec/models/ee/members_preloader_spec.rb @@ -14,12 +14,12 @@ def group_sso_with_preload(members) it 'preloads SAML identities to avoid N+1 queries in MembersPresenter' do member = create(:group_member, group: group) - create(:identity, :group_saml, user: member.user, saml_provider: saml_provider) + create(:group_saml_identity, user: member.user, saml_provider: saml_provider) control = ActiveRecord::QueryRecorder.new { group_sso_with_preload([member]) } members = create_list(:group_member, 3, group: group) - create(:identity, :group_saml, user: members.first.user, saml_provider: saml_provider) - create(:identity, :group_saml, user: members.last.user, saml_provider: saml_provider) + create(:group_saml_identity, user: members.first.user, saml_provider: saml_provider) + create(:group_saml_identity, user: members.last.user, saml_provider: saml_provider) expect { group_sso_with_preload(members) }.not_to exceed_query_limit(control) end diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb index 86865058684cb7369beaa32ea9af717764f24af8..aba3df299127542c7bde96b9bbca7f8af672ee4d 100644 --- a/ee/spec/models/ee/user_spec.rb +++ b/ee/spec/models/ee/user_spec.rb @@ -257,7 +257,7 @@ end context 'with linked identity' do - let!(:identity) { create(:identity, :group_saml, user: user) } + let!(:identity) { create(:group_saml_identity, user: user) } let(:saml_provider) { identity.saml_provider } let(:group) { saml_provider.group } @@ -267,7 +267,7 @@ end it 'does not cause ActiveRecord to loop through identites' do - create(:identity, :group_saml, user: user) + create(:group_saml_identity, user: user) expect(Identity).not_to receive(:instantiate) diff --git a/ee/spec/services/group_saml/identity/destroy_service_spec.rb b/ee/spec/services/group_saml/identity/destroy_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..48ebb065b47168aab32ef386ffd52e9395a7b745 --- /dev/null +++ b/ee/spec/services/group_saml/identity/destroy_service_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe GroupSaml::Identity::DestroyService do + let(:identity) { create(:group_saml_identity) } + + subject { described_class.new(identity) } + + before do + link_group_membership + end + + def link_group_membership + Gitlab::Auth::GroupSaml::MembershipUpdater.new(identity.user, identity.saml_provider).execute + end + + it "prevents future Group SAML logins" do + subject.execute + + expect(identity).to be_destroyed + end + + it "removes access to the group" do + expect do + subject.execute + end.to change(GroupMember, :count).by(-1) + end + + it "doesn't remove the last group owner" do + identity.saml_provider.group.members.first.update!(access_level: Gitlab::Access::OWNER) + + expect do + subject.execute + end.not_to change(GroupMember, :count) + end + + it 'logs an audit event' do + expect do + subject.execute + end.to change { SecurityEvent.count }.by(1) + end +end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 1ac5980efc1eb1e4b1d8cfa012d361dad6f1a67c..41567b8719fa757afaba95a78962215e260468b3 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -7289,6 +7289,9 @@ msgstr "" msgid "SAML Single Sign On Settings" msgstr "" +msgid "SAML for %{group_name}" +msgstr "" + msgid "SAST" msgstr "" diff --git a/spec/factories/identities.rb b/spec/factories/identities.rb index 8584c8bd737557a017cddc4e734aefa48445c90c..122d0d4293801e007a1c58f38579a85d0eccbf67 100644 --- a/spec/factories/identities.rb +++ b/spec/factories/identities.rb @@ -2,11 +2,5 @@ factory :identity do provider 'ldapmain' extern_uid 'my-ldap-id' - - trait :group_saml do - provider 'group_saml' - extern_uid { generate(:username) } - saml_provider - end end end