diff --git a/ee/app/models/saml_provider.rb b/ee/app/models/saml_provider.rb
index fe0b1083df727e48eb945d5c0b0368f17ef3b91d..fa13d356f5165e57b607c8767ca7e69e2a197e4d 100644
--- a/ee/app/models/saml_provider.rb
+++ b/ee/app/models/saml_provider.rb
@@ -41,6 +41,13 @@ def prohibited_outer_forks?
enforced_group_managed_accounts? && super
end
+ def last_linked_owner?(user)
+ return false unless group.owned_by?(user)
+ return false unless identities.for_user(user).exists?
+
+ identities.for_user(group.owners).count == 1
+ end
+
class DefaultOptions
include Gitlab::Routing
diff --git a/ee/app/policies/ee/identity_provider_policy.rb b/ee/app/policies/ee/identity_provider_policy.rb
index ae9c173abb88644faec2b6c55079e7e24b7a9942..ceb230105c09f4b31c9ad37a71db15a958690ffd 100644
--- a/ee/app/policies/ee/identity_provider_policy.rb
+++ b/ee/app/policies/ee/identity_provider_policy.rb
@@ -8,7 +8,25 @@ module IdentityProviderPolicy
desc "User account is managed by group SAML"
condition(:group_managed_account, scope: :user) { @user.group_managed_account? }
+ desc "Group Managed Accounts is enforced"
+ condition(:managed_group, scope: :subject) { @subject.is_a?(SamlProvider) && @subject.enforced_group_managed_accounts? }
+
+ desc "No other Group owners have SSO for this SAML provider"
+ condition(:last_group_saml_owner) do
+ @subject.is_a?(SamlProvider) && @subject.last_linked_owner?(@user)
+ end
+
rule { group_managed_account }.prevent_all
+
+ # User is last SSO owner of a managed group
+ #
+ # Owners without SSO won't have access, this ensures
+ # that we don't remove the last owner with access
+ #
+ # Unlike plain SSO Enforcment, it won't be possible to re-join
+ # with SSO if the owner leaves, as they will need to create a
+ # new account as a guest with a different email.
+ rule { managed_group && last_group_saml_owner }.prevent(:unlink)
end
end
end
diff --git a/ee/changelogs/unreleased/jej-prevent-last-gma-owner-with-sso-unlinking.yml b/ee/changelogs/unreleased/jej-prevent-last-gma-owner-with-sso-unlinking.yml
new file mode 100644
index 0000000000000000000000000000000000000000..c8501924831ef2829ea0f3b86ea708a969de1c99
--- /dev/null
+++ b/ee/changelogs/unreleased/jej-prevent-last-gma-owner-with-sso-unlinking.yml
@@ -0,0 +1,5 @@
+---
+title: Prevent last Group Managed Account owner with access from accidental unlinking
+merge_request: 32473
+author:
+type: fixed
diff --git a/ee/spec/factories/group_saml_identities.rb b/ee/spec/factories/group_saml_identities.rb
index 10b7d0abc3962b683df941f32263c14855c8eb6c..5062e6387a4a5511227511401e422ed89f45e3be 100644
--- a/ee/spec/factories/group_saml_identities.rb
+++ b/ee/spec/factories/group_saml_identities.rb
@@ -6,4 +6,10 @@
saml_provider
user
end
+
+ trait :group_owner do
+ after(:create) do |identity, evaluator|
+ identity.saml_provider.group.add_owner(identity.user)
+ end
+ end
end
diff --git a/ee/spec/models/saml_provider_spec.rb b/ee/spec/models/saml_provider_spec.rb
index ad250dd565316139b1404be1f605635b842bfadd..43cd0b467822b70970c80b1fabdccaf78821e823 100644
--- a/ee/spec/models/saml_provider_spec.rb
+++ b/ee/spec/models/saml_provider_spec.rb
@@ -213,4 +213,46 @@
end
end
end
+
+ describe '#last_linked_owner?' do
+ let_it_be(:user) { create(:user) }
+
+ context 'for a non-owner' do
+ it { is_expected.not_to be_last_linked_owner(user) }
+ end
+
+ context 'for a group owner' do
+ before do
+ group.add_owner(user)
+ end
+
+ context 'with saml linked' do
+ before do
+ create(:group_saml_identity, user: user, saml_provider: subject)
+ end
+
+ it { is_expected.to be_last_linked_owner(user) }
+
+ context 'another owner has SSO linked' do
+ before do
+ create(:group_saml_identity, :group_owner, saml_provider: subject)
+ end
+
+ it { is_expected.not_to be_last_linked_owner(user) }
+ end
+ end
+
+ context 'without saml linked' do
+ it { is_expected.not_to be_last_linked_owner(user) }
+
+ context 'another owner has SSO linked' do
+ before do
+ create(:group_saml_identity, :group_owner, saml_provider: subject)
+ end
+
+ it { is_expected.not_to be_last_linked_owner(user) }
+ end
+ end
+ end
+ end
end
diff --git a/ee/spec/policies/identity_provider_policy_spec.rb b/ee/spec/policies/identity_provider_policy_spec.rb
index a4ecdc6c1ff9c9244f1b0e705faf1bcafcb8d1aa..540ef5b6c011081c047125e3bc04de002b59ec35 100644
--- a/ee/spec/policies/identity_provider_policy_spec.rb
+++ b/ee/spec/policies/identity_provider_policy_spec.rb
@@ -10,6 +10,49 @@
it { is_expected.not_to be_allowed(:link) }
it { is_expected.not_to be_allowed(:unlink) }
+
+ context 'owner is not yet group managed' do
+ let_it_be(:identity) { create(:group_saml_identity) }
+ let_it_be(:saml_provider) { identity.saml_provider }
+ let_it_be(:group) { saml_provider.group }
+ let_it_be(:user) { identity.user }
+
+ subject(:policy) { described_class.new(user, saml_provider) }
+
+ before do
+ group.add_owner(user)
+ end
+
+ context 'no other owners exist' do
+ it { is_expected.not_to be_allowed(:unlink) }
+ end
+
+ context 'another group owner exists' do
+ let_it_be(:second_owner) { create(:user) }
+
+ before do
+ group.add_owner(second_owner)
+ end
+
+ context 'without sso linked' do
+ it { is_expected.not_to be_allowed(:unlink) }
+ end
+
+ context 'with sso linked' do
+ before do
+ create(:group_saml_identity, saml_provider: saml_provider, user: second_owner)
+ end
+
+ it { is_expected.to be_allowed(:unlink) }
+ end
+
+ context 'managed by the group' do
+ let(:second_owner) { create(:user, :group_managed, managing_group: group) }
+
+ it { is_expected.to be_allowed(:unlink) }
+ end
+ end
+ end
end
end
end