diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb index 53e1b4f583724fe1da0d94feab565aee2b0a0016..f4396a766f82c807c3c8ad22edda4d69b63b3d92 100644 --- a/ee/app/models/ee/user.rb +++ b/ee/app/models/ee/user.rb @@ -185,7 +185,13 @@ def find_by_smartcard_identity(certificate_subject, certificate_issuer) # the given SAML Provider def limit_to_saml_provider(saml_provider_id) if saml_provider_id - joins(:identities).where(identities: { saml_provider_id: saml_provider_id }) + group_id = SamlProvider.select(:group_id).find(saml_provider_id).group_id + saml_users = joins(:identities).where(identities: { saml_provider_id: saml_provider_id }) + service_accounts = + service_account.joins(:user_detail).where(user_detail: { provisioned_by_group_id: group_id }) + + union = ::Gitlab::SQL::Union.new([saml_users, service_accounts]).to_sql + ::User.from("(#{union}) #{::User.table_name}") else all end diff --git a/ee/lib/gitlab/auth/group_saml/membership_enforcer.rb b/ee/lib/gitlab/auth/group_saml/membership_enforcer.rb index 1e96f469ccc26322ac1470239f80ff65adfe59c1..939360105404a0ddf25531021cca355192558a53 100644 --- a/ee/lib/gitlab/auth/group_saml/membership_enforcer.rb +++ b/ee/lib/gitlab/auth/group_saml/membership_enforcer.rb @@ -11,6 +11,7 @@ def initialize(group) def can_add_user?(user) return true unless root_group.saml_provider&.enforced_sso? return true if user.project_bot? + return true if user.service_account? && user_provisioned_by_group?(user) return false if inactive_scim_identity_for_group?(user) GroupSamlIdentityFinder.new(user: user).find_linked(group: root_group) @@ -27,6 +28,10 @@ def inactive_scim_identity_for_group?(user) inactive_identities = scim_identities.reject(&:active?) inactive_identities.any? end + + def user_provisioned_by_group?(user) + user.provisioned_by_group_id == root_group.id + end end end end diff --git a/ee/spec/finders/users_finder_spec.rb b/ee/spec/finders/users_finder_spec.rb index a151cfd4ce619ba121e61023ca980a53a8b0970b..9deeb8c1a83d0a22e981a3e10802c8189716b9ab 100644 --- a/ee/spec/finders/users_finder_spec.rb +++ b/ee/spec/finders/users_finder_spec.rb @@ -31,21 +31,32 @@ let_it_be(:saml_provider) { create(:saml_provider, group: group, enabled: true, enforced_sso: true) } let_it_be(:saml_user) { create(:user) } let_it_be(:non_saml_user) { create(:user) } + let_it_be(:instance_service_account) { create(:service_account) } + let_it_be(:service_account) { create(:service_account, provisioned_by_group: group) } + let_it_be(:other_service_account) { create(:service_account, provisioned_by_group: create(:group)) } before do create(:identity, provider: 'group_saml1', saml_provider_id: saml_provider.id, user: saml_user) end - it 'returns all users by default' do + it 'returns all users' do users = described_class.new(user).execute - expect(users).to contain_exactly(saml_user, non_saml_user, *normal_users, *users_visible_to_admin) + expect(users).to contain_exactly( + saml_user, + non_saml_user, + instance_service_account, + service_account, + other_service_account, + *normal_users, + *users_visible_to_admin + ) end - it 'returns only saml users from the provided saml_provider_id' do + it 'returns saml users and service accounts for the SAML provider and associated group' do users = described_class.new(user, saml_provider_id: saml_provider.id).execute - expect(users).to contain_exactly(saml_user) + expect(users).to contain_exactly(saml_user, service_account) end end diff --git a/ee/spec/lib/gitlab/auth/group_saml/membership_enforcer_spec.rb b/ee/spec/lib/gitlab/auth/group_saml/membership_enforcer_spec.rb index 127ddb9bcb85ed8533f7544aeaa351e89d182706..9eb1facd58f92b61c2bfbbbe799ee946e8d48e17 100644 --- a/ee/spec/lib/gitlab/auth/group_saml/membership_enforcer_spec.rb +++ b/ee/spec/lib/gitlab/auth/group_saml/membership_enforcer_spec.rb @@ -38,4 +38,20 @@ expect(described_class.new(group).can_add_user?(project_bot)).to be_truthy end + + context 'when the user is a service account' do + let_it_be(:service_account) { create(:service_account) } + + it 'allows adding a service account provisioned by the root group' do + service_account.update!(provisioned_by_group: group) + + expect(described_class.new(group).can_add_user?(service_account)).to be_truthy + end + + it 'does not allow adding a service account provisioned by another root group' do + service_account.update!(provisioned_by_group: create(:group)) + + expect(described_class.new(group).can_add_user?(service_account)).to be_falsey + end + end end diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb index 6686bb061489f781f2917179ee10250828f25f22..5d06244b75ab5403e57480ad350153054a174482 100644 --- a/ee/spec/models/ee/user_spec.rb +++ b/ee/spec/models/ee/user_spec.rb @@ -914,24 +914,37 @@ describe '.limit_to_saml_provider' do let_it_be(:user1) { create(:user) } let_it_be(:user2) { create(:user) } + let_it_be(:service_account) { create(:service_account) } + let_it_be(:group) { create(:group) } + let_it_be(:saml_provider) { create(:saml_provider, group: group) } it 'returns all users when SAML provider is nil' do rel = described_class.limit_to_saml_provider(nil) - expect(rel).to include(user1, user2) + expect(rel).to include(user1, user2, service_account) end - it 'returns only the users who have an identity that belongs to the given SAML provider' do - create(:user) - group = create(:group) - saml_provider = create(:saml_provider, group: group) - create(:identity, saml_provider: saml_provider, user: user1) - create(:identity, saml_provider: saml_provider, user: user2) - create(:identity, user: create(:user)) + context 'when users have an identity belonging to the given SAML provider' do + before_all do + create(:user) + create(:identity, saml_provider: saml_provider, user: user1) + create(:identity, saml_provider: saml_provider, user: user2) + create(:identity, user: create(:user)) + end - rel = described_class.limit_to_saml_provider(saml_provider.id) + it 'does not return service accounts not provisioned by the group' do + rel = described_class.limit_to_saml_provider(saml_provider.id) - expect(rel).to contain_exactly(user1, user2) + expect(rel).to contain_exactly(user1, user2) + end + + it 'returns only the users who have an identity that belongs to the given SAML provider and service accounts' do + service_account.update!(provisioned_by_group: group) + + rel = described_class.limit_to_saml_provider(saml_provider.id) + + expect(rel).to contain_exactly(user1, user2, service_account) + end end end diff --git a/ee/spec/support/shared_examples/models/member_shared_examples.rb b/ee/spec/support/shared_examples/models/member_shared_examples.rb index f94c6058eb625b2731a9f9f94630e80d26744888..4c04e6e002b251fc7759c52204e4a5fe0d91c6c9 100644 --- a/ee/spec/support/shared_examples/models/member_shared_examples.rb +++ b/ee/spec/support/shared_examples/models/member_shared_examples.rb @@ -32,6 +32,12 @@ expect(member).to be_valid end + it 'allows adding a service account as a member' do + member = entity.add_member(create(:service_account, provisioned_by_group: group), Member::DEVELOPER) + + expect(member).to be_valid + end + context 'subgroups' do let!(:subgroup) { create(:group, parent: group) }