Commit 9c851d6d authored by Igor Drozdov's avatar Igor Drozdov

Merge branch 'jej/prevent-last-gma-owner-with-sso-unlinking' into 'master'

Prevent last Group Managed Account owner with access from accidental unlinking

See merge request gitlab-org/gitlab!32473
parents 84c36a86 f5d1d40a
...@@ -41,6 +41,13 @@ class SamlProvider < ApplicationRecord ...@@ -41,6 +41,13 @@ class SamlProvider < ApplicationRecord
enforced_group_managed_accounts? && super enforced_group_managed_accounts? && super
end 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 class DefaultOptions
include Gitlab::Routing include Gitlab::Routing
......
...@@ -8,7 +8,25 @@ module EE ...@@ -8,7 +8,25 @@ module EE
desc "User account is managed by group SAML" desc "User account is managed by group SAML"
condition(:group_managed_account, scope: :user) { @user.group_managed_account? } 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 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 end
end end
---
title: Prevent last Group Managed Account owner with access from accidental unlinking
merge_request: 32473
author:
type: fixed
...@@ -6,4 +6,10 @@ FactoryBot.define do ...@@ -6,4 +6,10 @@ FactoryBot.define do
saml_provider saml_provider
user user
end end
trait :group_owner do
after(:create) do |identity, evaluator|
identity.saml_provider.group.add_owner(identity.user)
end
end
end end
...@@ -213,4 +213,46 @@ describe SamlProvider do ...@@ -213,4 +213,46 @@ describe SamlProvider do
end end
end 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 end
...@@ -10,6 +10,49 @@ describe IdentityProviderPolicy do ...@@ -10,6 +10,49 @@ describe IdentityProviderPolicy do
it { is_expected.not_to be_allowed(:link) } it { is_expected.not_to be_allowed(:link) }
it { is_expected.not_to be_allowed(:unlink) } 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 end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment