From eaa1ff8a55f982cb952d5dd1f286a12774c252d5 Mon Sep 17 00:00:00 2001 From: Drew Blessing Date: Tue, 11 Jul 2023 12:56:24 -0500 Subject: [PATCH 1/3] Allow Service Account to be added to SSO-enforced group Service Accounts cannot sign-in interactively, and they're meant to be used regardless of SSO enforcement settings. This change allows a Service Account to be added as a member even when SSO is enforced. EE: true Changelog: fixed --- ee/lib/gitlab/auth/group_saml/membership_enforcer.rb | 2 +- .../lib/gitlab/auth/group_saml/membership_enforcer_spec.rb | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/ee/lib/gitlab/auth/group_saml/membership_enforcer.rb b/ee/lib/gitlab/auth/group_saml/membership_enforcer.rb index 1e96f469ccc263..0b89a80c453367 100644 --- a/ee/lib/gitlab/auth/group_saml/membership_enforcer.rb +++ b/ee/lib/gitlab/auth/group_saml/membership_enforcer.rb @@ -10,7 +10,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.project_bot? || user.service_account? return false if inactive_scim_identity_for_group?(user) GroupSamlIdentityFinder.new(user: user).find_linked(group: root_group) 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 127ddb9bcb85ed..3343f3d8a6bc6a 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,10 @@ expect(described_class.new(group).can_add_user?(project_bot)).to be_truthy end + + it 'allows adding a service account as a member' do + service_account = create(:service_account) + + expect(described_class.new(group).can_add_user?(service_account)).to be_truthy + end end -- GitLab From 3f8c6b094473b7c7543c43a3948a436ff8a8fed8 Mon Sep 17 00:00:00 2001 From: Drew Blessing Date: Wed, 6 Sep 2023 14:08:14 -0500 Subject: [PATCH 2/3] Allow user search to include both service accts and SSO users Changelog: changed EE: true --- ee/app/models/ee/user.rb | 12 ++++- .../auth/group_saml/membership_enforcer.rb | 9 +++- ee/spec/finders/users_finder_spec.rb | 17 +++++-- .../group_saml/membership_enforcer_spec.rb | 20 +++++++-- ee/spec/models/ee/user_spec.rb | 44 ++++++++++++++----- .../models/member_shared_examples.rb | 6 +++ 6 files changed, 89 insertions(+), 19 deletions(-) diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb index 53e1b4f583724f..440729653ae475 100644 --- a/ee/app/models/ee/user.rb +++ b/ee/app/models/ee/user.rb @@ -146,6 +146,10 @@ module User where(id: ::PersonalAccessToken.with_invalid_expires_at(expiration_date).select(:user_id)) end + scope :where_provisioned_by_group_id, ->(group_id) do + joins(:user_detail).where(user_detail: { provisioned_by_group_id: group_id }) + end + scope :with_scim_identities_by_extern_uid, ->(extern_uid) { joins(:scim_identities).merge(ScimIdentity.with_extern_uid(extern_uid)) } accepts_nested_attributes_for :namespace @@ -185,7 +189,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 0b89a80c453367..eec98e16f24c12 100644 --- a/ee/lib/gitlab/auth/group_saml/membership_enforcer.rb +++ b/ee/lib/gitlab/auth/group_saml/membership_enforcer.rb @@ -10,7 +10,8 @@ def initialize(group) def can_add_user?(user) return true unless root_group.saml_provider&.enforced_sso? - return true if user.project_bot? || user.service_account? + 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,12 @@ def inactive_scim_identity_for_group?(user) inactive_identities = scim_identities.reject(&:active?) inactive_identities.any? end + + # Group service accounts are provisioned by a root group + # Instance/self-managed service accounts have nil provisioning group + def user_provisioned_by_group?(user) + user.provisioned_by_group_id == root_group.id || user.provisioned_by_group_id.nil? + end end end end diff --git a/ee/spec/finders/users_finder_spec.rb b/ee/spec/finders/users_finder_spec.rb index a151cfd4ce619b..1e6ca6cf7f96ce 100644 --- a/ee/spec/finders/users_finder_spec.rb +++ b/ee/spec/finders/users_finder_spec.rb @@ -31,21 +31,30 @@ 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(: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 except service accounts' 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, + 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 3343f3d8a6bc6a..6991c8c67eadee 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 @@ -39,9 +39,23 @@ expect(described_class.new(group).can_add_user?(project_bot)).to be_truthy end - it 'allows adding a service account as a member' do - service_account = create(:service_account) + context 'when the user is a service account' do + let_it_be(:service_account) { create(:service_account) } - expect(described_class.new(group).can_add_user?(service_account)).to be_truthy + it 'allows adding a service account as a member' do + expect(described_class.new(group).can_add_user?(service_account)).to be_truthy + end + + it 'allows adding a service account provisioned by the root group' do + service_account.update!(provisioned_by_group_id: 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 6686bb061489f7..a3dfcb8367badb 100644 --- a/ee/spec/models/ee/user_spec.rb +++ b/ee/spec/models/ee/user_spec.rb @@ -269,6 +269,17 @@ ) end end + + describe '.where_provisioned_by_group_id' do + let_it_be(:group) { create(:group) } + let_it_be(:user_provisioned) { create(:user, provisioned_by_group: group) } + let_it_be(:user_provisioned_other_group) { create(:user, provisioned_by_group: create(:group)) } + let_it_be(:user_not_provisioned) { create(:user) } + + it 'returns users that were provisioned by the group ID' do + expect(described_class.where_provisioned_by_group_id(group.id)).to match_array([user_provisioned]) + end + end end describe 'after_create' do @@ -914,24 +925,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 + + it 'does not return service accounts not provisioned by the group' do + rel = described_class.limit_to_saml_provider(saml_provider.id) - rel = described_class.limit_to_saml_provider(saml_provider.id) + expect(rel).to contain_exactly(user1, user2) + end - expect(rel).to contain_exactly(user1, user2) + 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 f94c6058eb625b..56f1a55399dec1 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 addding a service account as a member' do + member = entity.add_member(create(:service_account), Member::DEVELOPER) + + expect(member).to be_valid + end + context 'subgroups' do let!(:subgroup) { create(:group, parent: group) } -- GitLab From 861dd6d502c2b1b3027663e5ff87bea5064de11d Mon Sep 17 00:00:00 2001 From: Drew Blessing Date: Mon, 11 Sep 2023 09:46:01 -0500 Subject: [PATCH 3/3] Review fixes --- ee/app/models/ee/user.rb | 4 ---- ee/lib/gitlab/auth/group_saml/membership_enforcer.rb | 4 +--- ee/spec/finders/users_finder_spec.rb | 4 +++- .../auth/group_saml/membership_enforcer_spec.rb | 6 +----- ee/spec/models/ee/user_spec.rb | 11 ----------- .../shared_examples/models/member_shared_examples.rb | 4 ++-- 6 files changed, 7 insertions(+), 26 deletions(-) diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb index 440729653ae475..f4396a766f82c8 100644 --- a/ee/app/models/ee/user.rb +++ b/ee/app/models/ee/user.rb @@ -146,10 +146,6 @@ module User where(id: ::PersonalAccessToken.with_invalid_expires_at(expiration_date).select(:user_id)) end - scope :where_provisioned_by_group_id, ->(group_id) do - joins(:user_detail).where(user_detail: { provisioned_by_group_id: group_id }) - end - scope :with_scim_identities_by_extern_uid, ->(extern_uid) { joins(:scim_identities).merge(ScimIdentity.with_extern_uid(extern_uid)) } accepts_nested_attributes_for :namespace diff --git a/ee/lib/gitlab/auth/group_saml/membership_enforcer.rb b/ee/lib/gitlab/auth/group_saml/membership_enforcer.rb index eec98e16f24c12..939360105404a0 100644 --- a/ee/lib/gitlab/auth/group_saml/membership_enforcer.rb +++ b/ee/lib/gitlab/auth/group_saml/membership_enforcer.rb @@ -29,10 +29,8 @@ def inactive_scim_identity_for_group?(user) inactive_identities.any? end - # Group service accounts are provisioned by a root group - # Instance/self-managed service accounts have nil provisioning group def user_provisioned_by_group?(user) - user.provisioned_by_group_id == root_group.id || user.provisioned_by_group_id.nil? + user.provisioned_by_group_id == root_group.id end end end diff --git a/ee/spec/finders/users_finder_spec.rb b/ee/spec/finders/users_finder_spec.rb index 1e6ca6cf7f96ce..9deeb8c1a83d0a 100644 --- a/ee/spec/finders/users_finder_spec.rb +++ b/ee/spec/finders/users_finder_spec.rb @@ -31,6 +31,7 @@ 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)) } @@ -38,12 +39,13 @@ create(:identity, provider: 'group_saml1', saml_provider_id: saml_provider.id, user: saml_user) end - it 'returns all users except service accounts' do + it 'returns all users' do users = described_class.new(user).execute expect(users).to contain_exactly( saml_user, non_saml_user, + instance_service_account, service_account, other_service_account, *normal_users, 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 6991c8c67eadee..9eb1facd58f92b 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 @@ -42,12 +42,8 @@ context 'when the user is a service account' do let_it_be(:service_account) { create(:service_account) } - it 'allows adding a service account as a member' do - expect(described_class.new(group).can_add_user?(service_account)).to be_truthy - end - it 'allows adding a service account provisioned by the root group' do - service_account.update!(provisioned_by_group_id: group) + service_account.update!(provisioned_by_group: group) expect(described_class.new(group).can_add_user?(service_account)).to be_truthy end diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb index a3dfcb8367badb..5d06244b75ab54 100644 --- a/ee/spec/models/ee/user_spec.rb +++ b/ee/spec/models/ee/user_spec.rb @@ -269,17 +269,6 @@ ) end end - - describe '.where_provisioned_by_group_id' do - let_it_be(:group) { create(:group) } - let_it_be(:user_provisioned) { create(:user, provisioned_by_group: group) } - let_it_be(:user_provisioned_other_group) { create(:user, provisioned_by_group: create(:group)) } - let_it_be(:user_not_provisioned) { create(:user) } - - it 'returns users that were provisioned by the group ID' do - expect(described_class.where_provisioned_by_group_id(group.id)).to match_array([user_provisioned]) - end - end end describe 'after_create' do 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 56f1a55399dec1..4c04e6e002b251 100644 --- a/ee/spec/support/shared_examples/models/member_shared_examples.rb +++ b/ee/spec/support/shared_examples/models/member_shared_examples.rb @@ -32,8 +32,8 @@ expect(member).to be_valid end - it 'allows addding a service account as a member' do - member = entity.add_member(create(:service_account), Member::DEVELOPER) + 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 -- GitLab