Commit f5d1d40a authored by James Edwards-Jones's avatar James Edwards-Jones

Prevent last GMA owner with SSO unlinking

Previously it was possible for the only owner with SSO access to leave
the group. This would leave the group without any owners that could
access the group or turn off SSO Enforcement, as none of them would pass
the SSO Enforcement to gain access. Additionally Group Managed Accounts
means that attempting to re-join the group would result in a sign up
form being presented and requiring a different email, preventing a the
owner from re-gaining access.
parent 7a533749
......@@ -41,6 +41,13 @@ class SamlProvider < ApplicationRecord
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
......
......@@ -8,7 +8,25 @@ module EE
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
---
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
saml_provider
user
end
trait :group_owner do
after(:create) do |identity, evaluator|
identity.saml_provider.group.add_owner(identity.user)
end
end
end
......@@ -213,4 +213,46 @@ describe SamlProvider do
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
......@@ -10,6 +10,49 @@ describe IdentityProviderPolicy do
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
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