From a2ec5c408808b82c5e75645f59b2fd3b94796bd1 Mon Sep 17 00:00:00 2001
From: Andrew Evans <aevans@gitlab.com>
Date: Wed, 17 Jan 2024 18:00:56 +0000
Subject: [PATCH] Service accounts can be added to LDAP-synced groups
See [425947](https://gitlab.com/gitlab-org/gitlab/-/issues/425947) for
details.
Allows adding service account users to groups even when LDAP sync is
enabled. To do this, we make some major changes:
- Add a new permission: `admin_service_account_members` ; this way we
can check for group admin permissions even when those have been
mostly disabled by setting the group to LDAP syncing
- Switch which permission we check in the service layer when creating
new `Member` objects for groups <-> service account users
- Remove multiple layers of duplicative permission checking from the API
and upper-level services
The new permission is only valid when the `service_accounts` licensed
feature is enabled.
Added some additional error handling to ensure there are no changes in
the API status codes or object returns.
Changelog: changed
EE: true
---
app/services/members/create_service.rb | 23 +++----
app/services/members/creator_service.rb | 4 +-
app/services/members/destroy_service.rb | 7 ++-
.../auth/ldap/ldap-troubleshooting.md | 6 ++
ee/app/policies/ee/group_policy.rb | 5 ++
ee/app/services/ee/members/destroy_service.rb | 9 +++
.../ee/members/groups/creator_service.rb | 9 +++
ee/spec/policies/group_policy_spec.rb | 51 +++++++++++++++
ee/spec/requests/api/members_spec.rb | 62 +++++++++++++++++++
.../ee/members/destroy_service_spec.rb | 20 ++++++
.../ee/members/groups/creator_service_spec.rb | 22 +++++++
lib/api/helpers/members_helpers.rb | 28 ++++++---
lib/api/members.rb | 2 -
spec/requests/api/members_spec.rb | 18 ++++++
spec/services/members/create_service_spec.rb | 16 ++++-
.../models/member_shared_examples.rb | 2 +
.../requests/api/members_shared_examples.rb | 2 +-
17 files changed, 255 insertions(+), 31 deletions(-)
diff --git a/app/services/members/create_service.rb b/app/services/members/create_service.rb
index b453098e27ac..ec9a4f9f4a67 100644
--- a/app/services/members/create_service.rb
+++ b/app/services/members/create_service.rb
@@ -8,18 +8,19 @@ class CreateService < Members::BaseService
DEFAULT_INVITE_LIMIT = 100
- attr_reader :membership_locked
+ attr_reader :membership_locked, :http_status
def initialize(*args)
super
@errors = []
+ @http_status = nil
@invites = invites_from_params
@source = params[:source]
end
def execute
- raise Gitlab::Access::AccessDeniedError unless can?(current_user, create_member_permission(source), source)
+ validate_source_type!
if adding_at_least_one_owner && cannot_assign_owner_responsibilities_to_member_in_project?
raise Gitlab::Access::AccessDeniedError
@@ -65,6 +66,10 @@ def invites_from_params
params[:user_id].to_s.split(',').uniq
end
+ def validate_source_type!
+ raise "Unknown source type: #{source.class}!" unless source.is_a?(Group) || source.is_a?(Project)
+ end
+
def validate_invite_source!
raise ArgumentError, s_('AddMember|No invite source provided.') unless invite_source.present?
end
@@ -102,6 +107,7 @@ def create_params
end
def process_result(member)
+ @http_status = :unauthorized if member.errors.added? :base, :unauthorized
existing_errors = member.errors.full_messages
# calling invalid? clears any errors that were added outside of the
@@ -174,7 +180,7 @@ def at_least_one_member_created?
def result
if errors.any?
- error(formatted_errors)
+ error(formatted_errors, http_status)
else
success
end
@@ -194,17 +200,6 @@ def publish_event!
})
)
end
-
- def create_member_permission(source)
- case source
- when Group
- :admin_group_member
- when Project
- :admin_project_member
- else
- raise "Unknown source type: #{source.class}!"
- end
- end
end
end
diff --git a/app/services/members/creator_service.rb b/app/services/members/creator_service.rb
index d7bf073d8e9f..57159c14b3b7 100644
--- a/app/services/members/creator_service.rb
+++ b/app/services/members/creator_service.rb
@@ -176,7 +176,7 @@ def can_commit_member?
end
end
- # overridden in Members::Groups::CreatorService
+ # overridden in EE:Members::Groups::CreatorService
def member_role_too_high?
false
end
@@ -243,7 +243,7 @@ def add_commit_error
_('not authorized to update member')
end
- member.errors.add(:base, msg)
+ member.errors.add(:base, :unauthorized, message: msg)
end
def add_member_role_error
diff --git a/app/services/members/destroy_service.rb b/app/services/members/destroy_service.rb
index d4cc60c6de00..14cdb83b4e5d 100644
--- a/app/services/members/destroy_service.rb
+++ b/app/services/members/destroy_service.rb
@@ -185,7 +185,7 @@ def destroying_member_with_owner_access_level?(member)
def destroy_member_permission(member)
case member
when GroupMember
- :destroy_group_member
+ destroy_group_member_permission(member)
when ProjectMember
:destroy_project_member
else
@@ -193,6 +193,11 @@ def destroy_member_permission(member)
end
end
+ # overridden in EE::Members::DestroyService
+ def destroy_group_member_permission(_member)
+ :destroy_group_member
+ end
+
def destroy_bot_member_permission(member)
raise "Unsupported bot member type: #{member}" unless member.is_a?(ProjectMember)
diff --git a/doc/administration/auth/ldap/ldap-troubleshooting.md b/doc/administration/auth/ldap/ldap-troubleshooting.md
index eb1ee2034697..39546345355e 100644
--- a/doc/administration/auth/ldap/ldap-troubleshooting.md
+++ b/doc/administration/auth/ldap/ldap-troubleshooting.md
@@ -393,6 +393,12 @@ the rails console.
UIDs here should match the 'Identifier' from the LDAP identity checked earlier. If it doesn't,
the user does not appear to be in the LDAP group.
+#### Cannot add service account user to group when LDAP sync is enabled
+
+When LDAP sync is enabled for a group, you cannot use the "invite" dialog to invite new group members.
+
+To resolve this issue in GitLab 16.8 and later, you can invite service accounts to and remove them from a group using the [group members API endpoints](../../../api/members.md#add-a-member-to-a-group-or-project).
+
#### Administrator privileges not granted
When [Administrator sync](ldap_synchronization.md#administrator-sync) has been configured
diff --git a/ee/app/policies/ee/group_policy.rb b/ee/app/policies/ee/group_policy.rb
index cec0a34eb05e..960b24b43cd2 100644
--- a/ee/app/policies/ee/group_policy.rb
+++ b/ee/app/policies/ee/group_policy.rb
@@ -462,6 +462,7 @@ module GroupPolicy
rule { (admin | owner) & service_accounts_available }.policy do
enable :admin_service_accounts
+ enable :admin_service_account_member
end
rule { memberships_locked_to_saml & saml_group_links_enabled & ~admin }.policy do
@@ -522,6 +523,10 @@ module GroupPolicy
enable :destroy_group_member
end
+ rule { custom_roles_allowed & role_enables_admin_group_member & service_accounts_available }.policy do
+ enable :admin_service_account_member
+ end
+
rule { custom_roles_allowed & role_enables_manage_group_access_tokens & resource_access_token_feature_available }.policy do
enable :read_resource_access_tokens
enable :destroy_resource_access_tokens
diff --git a/ee/app/services/ee/members/destroy_service.rb b/ee/app/services/ee/members/destroy_service.rb
index c5724276f6e7..945bdfabb8e6 100644
--- a/ee/app/services/ee/members/destroy_service.rb
+++ b/ee/app/services/ee/members/destroy_service.rb
@@ -118,6 +118,15 @@ def destroy_data_related_to_member(member, skip_subresources, skip_saml_identity
::User.clear_group_with_ai_available_cache(member.user.id)
end
+ override :destroy_group_member_permission
+ def destroy_group_member_permission(member)
+ if member.user&.service_account?
+ :admin_service_account_member
+ else
+ super(member)
+ end
+ end
+
def enqueue_cleanup_add_on_seat_assignments(member)
namespace = member.source.root_ancestor
diff --git a/ee/app/services/ee/members/groups/creator_service.rb b/ee/app/services/ee/members/groups/creator_service.rb
index e06760a8799d..4e2de871c5d9 100644
--- a/ee/app/services/ee/members/groups/creator_service.rb
+++ b/ee/app/services/ee/members/groups/creator_service.rb
@@ -30,6 +30,15 @@ def member_attributes
attributes.merge(member_role_id: member_role_id)
end
+ override :can_create_new_member?
+ def can_create_new_member?
+ if member.user&.service_account?
+ current_user.can?(:admin_service_account_member, member.group)
+ else
+ current_user.can?(:admin_group_member, member.group)
+ end
+ end
+
def ignore_user_limits
args[:ignore_user_limits]
end
diff --git a/ee/spec/policies/group_policy_spec.rb b/ee/spec/policies/group_policy_spec.rb
index 51e4edd6558e..4438455c70c3 100644
--- a/ee/spec/policies/group_policy_spec.rb
+++ b/ee/spec/policies/group_policy_spec.rb
@@ -1339,6 +1339,14 @@ def stub_group_saml_config(enabled)
it { is_expected.not_to be_allowed(:admin_group_member) }
it { is_expected.not_to be_allowed(:override_group_member) }
it { is_expected.not_to be_allowed(:update_group_member) }
+
+ context 'and service_accounts feature is enabled' do
+ before do
+ stub_licensed_features(service_accounts: true)
+ end
+
+ it { is_expected.to be_allowed(:admin_service_account_member) }
+ end
end
end
end
@@ -2832,6 +2840,7 @@ def expect_private_group_permissions_as_if_non_member
let(:current_user) { owner }
it { is_expected.to be_disallowed(:admin_service_accounts) }
+ it { is_expected.to be_disallowed(:admin_service_account_member) }
end
context 'when feature is enabled' do
@@ -2843,12 +2852,14 @@ def expect_private_group_permissions_as_if_non_member
let(:current_user) { maintainer }
it { is_expected.to be_disallowed(:admin_service_accounts) }
+ it { is_expected.to be_disallowed(:admin_service_account_member) }
end
context 'when the user is an owner' do
let(:current_user) { owner }
it { is_expected.to be_allowed(:admin_service_accounts) }
+ it { is_expected.to be_allowed(:admin_service_account_member) }
end
context 'when the user is an instance admin' do
@@ -2856,10 +2867,12 @@ def expect_private_group_permissions_as_if_non_member
context 'when admin mode is enabled', :enable_admin_mode do
it { is_expected.to be_allowed(:admin_service_accounts) }
+ it { is_expected.to be_allowed(:admin_service_account_member) }
end
context 'when admin mode is not enabled' do
it { is_expected.to be_disallowed(:admin_service_accounts) }
+ it { is_expected.to be_disallowed(:admin_service_account_member) }
end
end
end
@@ -3058,6 +3071,44 @@ def create_member_role(member, abilities = member_role_abilities)
let(:allowed_abilities) { [:admin_group_member] }
it_behaves_like 'custom roles abilities'
+
+ context 'admin_service_account_member' do
+ let_it_be(:guest) { create(:user) }
+ let_it_be(:group) { create(:group) }
+
+ let_it_be(:group_member_guest) do
+ create(
+ :group_member,
+ user: guest,
+ source: group,
+ access_level: Gitlab::Access::GUEST
+ )
+ end
+
+ let(:role) do
+ create(
+ :member_role,
+ :guest,
+ namespace: group,
+ admin_group_member: true
+ )
+ end
+
+ before do
+ role.members << group_member_guest
+ stub_licensed_features(custom_roles: true)
+ end
+
+ it { is_expected.to be_disallowed(:admin_service_account_member) }
+
+ context 'when service accounts feature enabled' do
+ before do
+ stub_licensed_features(custom_roles: true, service_accounts: true)
+ end
+
+ it { is_expected.to be_allowed(:admin_service_account_member) }
+ end
+ end
end
context 'for a member role with manage_group_access_tokens true' do
diff --git a/ee/spec/requests/api/members_spec.rb b/ee/spec/requests/api/members_spec.rb
index 430734f85023..c6e54f918fa3 100644
--- a/ee/spec/requests/api/members_spec.rb
+++ b/ee/spec/requests/api/members_spec.rb
@@ -1273,6 +1273,68 @@
expect(response).to have_gitlab_http_status(:forbidden)
end
+
+ context 'when user to be added is a service account' do
+ let_it_be(:service_account) { create(:service_account) }
+
+ it 'does not allow adding the account to the group' do
+ post api("/groups/#{group.id}/members", owner),
+ params: { user_id: service_account.id, access_level: Member::DEVELOPER }
+
+ expect(response).to have_gitlab_http_status(:forbidden)
+ end
+
+ context 'and the service_account feature is enabled' do
+ before do
+ stub_licensed_features(service_accounts: true)
+ end
+
+ it 'adds the service account to the group' do
+ expect do
+ post api("/groups/#{group.id}/members", owner),
+ params: { user_id: service_account.id, access_level: Member::DEVELOPER }
+ end.to change { Member.count }.by(1)
+ end
+ end
+ end
+ end
+
+ describe 'DELETE /groups/:id/members/:user_id' do
+ let(:service_account) { create(:user, :service_account) }
+ let!(:service_account_member) do
+ create(:group_member, :developer, group: group, user: service_account, ldap: false)
+ end
+
+ it 'fails for LDAP-managed group member' do
+ expect do
+ delete api("/groups/#{group.id}/members/#{ldap_developer.id}", owner)
+ end.not_to change { Member.count }
+
+ expect(response).to have_gitlab_http_status(:forbidden)
+ end
+
+ it 'fails when group member is a service account' do
+ expect do
+ delete api("/groups/#{group.id}/members/#{service_account.id}", owner)
+ end.not_to change { Member.count }
+
+ expect(response).to have_gitlab_http_status(:forbidden)
+ end
+
+ context 'when service_accounts feature is enabled' do
+ before do
+ stub_licensed_features(service_accounts: true)
+ end
+
+ it 'succeeds when group member is a service account' do
+ expect do
+ delete api("/groups/#{group.id}/members/#{service_account.id}", owner)
+ end.to change { Member.count }.by(-1)
+
+ expect(response).to have_gitlab_http_status(:no_content)
+ expect(response.body).to be_empty
+ end
+ end
end
describe 'POST /groups/:id/members/:user_id/override' do
diff --git a/ee/spec/services/ee/members/destroy_service_spec.rb b/ee/spec/services/ee/members/destroy_service_spec.rb
index eb52e8f1a77e..082144c4c82f 100644
--- a/ee/spec/services/ee/members/destroy_service_spec.rb
+++ b/ee/spec/services/ee/members/destroy_service_spec.rb
@@ -319,6 +319,26 @@
end
end
end
+
+ context 'when removing a service account group member' do
+ subject(:destroy_service) { described_class.new(current_user).execute(member) }
+
+ let(:member_user) { create(:user, :service_account) }
+
+ it 'raises AccessDeniedError when :service_accounts feature unavailable' do
+ expect { subject }.to raise_error(Gitlab::Access::AccessDeniedError)
+ end
+
+ context 'when :service_accounts feature is enabled' do
+ before do
+ stub_licensed_features(service_accounts: true)
+ end
+
+ it 'removes the service account member' do
+ expect { subject }.to change { member.source.members_and_requesters.count }.by(-1)
+ end
+ end
+ end
end
context 'when current user is not present' do # ie, when the system initiates the destroy
diff --git a/ee/spec/services/ee/members/groups/creator_service_spec.rb b/ee/spec/services/ee/members/groups/creator_service_spec.rb
index cc6cd90c1d4e..fd02ee9f873d 100644
--- a/ee/spec/services/ee/members/groups/creator_service_spec.rb
+++ b/ee/spec/services/ee/members/groups/creator_service_spec.rb
@@ -200,5 +200,27 @@
end
end
end
+
+ context 'when adding a service_account member' do
+ let_it_be(:user) { create(:user, :service_account) }
+ let_it_be(:source) { create(:group) }
+ let_it_be(:group_owner) { create(:user) }
+
+ before_all do
+ source.add_owner(group_owner)
+ end
+
+ before do
+ allow(group_owner).to receive(:can?).and_return(false)
+ allow(group_owner).to receive(:can?).with(:admin_service_account_member, anything).and_return(true)
+ end
+
+ it 'checks the appropriate permission' do
+ member = described_class.add_member(source, user, :maintainer, current_user: group_owner)
+
+ expect(member).to be_a GroupMember
+ expect(member).to be_persisted
+ end
+ end
end
end
diff --git a/lib/api/helpers/members_helpers.rb b/lib/api/helpers/members_helpers.rb
index 1a23dcd0d3c0..6d1cd9d8cd9d 100644
--- a/lib/api/helpers/members_helpers.rb
+++ b/lib/api/helpers/members_helpers.rb
@@ -85,20 +85,30 @@ def add_single_member_by_user_id(create_service_params)
user_id = create_service_params[:user_id]
user = User.find_by(id: user_id) # rubocop: disable CodeReuse/ActiveRecord
- if user
- conflict!('Member already exists') if member_already_exists?(source, user_id)
+ not_found!('User') unless user
- instance = ::Members::CreateService.new(current_user, create_service_params)
- instance.execute
+ conflict!('Member already exists') if member_already_exists?(source, user_id)
- not_allowed! if instance.membership_locked # This currently can only be reached in EE if group membership is locked
+ instance = ::Members::CreateService.new(current_user, create_service_params)
+ result = instance.execute
- member = instance.single_member
- render_validation_error!(member) if member.invalid?
+ # This currently can only be reached in EE if group membership is locked
+ not_allowed! if instance.membership_locked
- present_members(member)
+ if result[:status] == :error && result[:http_status] == :unauthorized
+ raise Gitlab::Access::AccessDeniedError
+ end
+
+ # prefer responding with model validations, if present
+ member = instance.single_member
+ render_validation_error!(member) if member.invalid?
+
+ # if errors occurred besides model validations or authorization failures,
+ # render those appropriately
+ if result[:status] == :error
+ render_structured_api_error!(result, :bad_request)
else
- not_found!('User')
+ present_members(member)
end
end
diff --git a/lib/api/members.rb b/lib/api/members.rb
index 908733d4aa12..e4bd29640cd6 100644
--- a/lib/api/members.rb
+++ b/lib/api/members.rb
@@ -119,8 +119,6 @@ class Members < ::API::Base
post ":id/members", feature_category: feature_category do
source = find_source(source_type, params[:id])
- authorize_admin_source_member!(source_type, source)
-
create_service_params = params.merge(source: source)
if add_multiple_members?(params[:user_id].to_s)
diff --git a/spec/requests/api/members_spec.rb b/spec/requests/api/members_spec.rb
index 7fc58140fb6b..01a8ca055a5a 100644
--- a/spec/requests/api/members_spec.rb
+++ b/spec/requests/api/members_spec.rb
@@ -346,6 +346,24 @@
expect(json_response['access_level']).to eq(Member::DEVELOPER)
end
+ it 'returns the error message if there was an error adding the member to the group' do
+ error_message = 'Test CreateService Error Message'
+ allow_next_instance_of(::Members::CreateService) do |service|
+ expect(service).to receive(:execute).and_return(status: :error, message: error_message)
+ allow(service).to receive(:single_member).and_return(
+ instance_double(Member, invalid?: false)
+ )
+ end
+
+ expect do
+ post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
+ params: { user_id: stranger.id, access_level: Member::DEVELOPER }
+ end.not_to change { source.members.count }
+ expect(response).to have_gitlab_http_status(:bad_request)
+ expect(json_response['status']).to eq('error')
+ expect(json_response['message']).to eq(error_message)
+ end
+
context 'with invite_source considerations', :snowplow do
let(:params) { { user_id: stranger.id, access_level: Member::DEVELOPER } }
diff --git a/spec/services/members/create_service_spec.rb b/spec/services/members/create_service_spec.rb
index c08b40e9528d..f019e7f40466 100644
--- a/spec/services/members/create_service_spec.rb
+++ b/spec/services/members/create_service_spec.rb
@@ -31,8 +31,10 @@
context 'when the current user does not have permission to create members' do
let(:current_user) { create(:user) }
- it 'raises a Gitlab::Access::AccessDeniedError' do
- expect { execute_service }.to raise_error(Gitlab::Access::AccessDeniedError)
+ it 'returns an unauthorized http_status' do
+ expect(execute_service[:status]).to eq(:error)
+ # this is expected by API::Helpers::MembersHelpers#add_single_member_by_user_id
+ expect(execute_service[:http_status]).to eq(:unauthorized)
end
context 'when a project maintainer attempts to add owners' do
@@ -56,6 +58,15 @@
end
end
+ context 'when trying to create a Membership with invalid params' do
+ let(:additional_params) { Hash[invite_source: '_invite_source_', expires_at: 3.days.ago] }
+
+ it 'returns an error response' do
+ expect(execute_service[:status]).to eq(:error)
+ expect(execute_service[:http_status]).to be_nil
+ end
+ end
+
context 'when passing valid parameters' do
it 'adds a user to members' do
expect(execute_service[:status]).to eq(:success)
@@ -251,6 +262,7 @@
it 'does not update the member' do
expect(execute_service[:status]).to eq(:error)
+ expect(execute_service[:http_status]).to eq(:unauthorized)
expect(execute_service[:message]).to eq("#{project_bot.username}: not authorized to update member")
expect(Onboarding::Progress.completed?(source.namespace, :user_added)).to be(false)
end
diff --git a/spec/support/shared_examples/models/member_shared_examples.rb b/spec/support/shared_examples/models/member_shared_examples.rb
index 01d6642e814f..43b60fad67d6 100644
--- a/spec/support/shared_examples/models/member_shared_examples.rb
+++ b/spec/support/shared_examples/models/member_shared_examples.rb
@@ -139,6 +139,7 @@
expect(source.reload).to have_user(project_bot)
expect(member).to be_persisted
expect(member.access_level).to eq(Gitlab::Access::DEVELOPER)
+ expect(member.errors.added?(:base, :unauthorized)).to eq(true)
expect(member.errors.full_messages).to include(/not authorized to update member/)
end
end
@@ -169,6 +170,7 @@
expect(member).not_to be_persisted
expect(source).not_to have_user(user)
+ expect(member.errors.added?(:base, :unauthorized)).to eq(true)
expect(member.errors.full_messages).to include(/not authorized to create member/)
end
end
diff --git a/spec/support/shared_examples/requests/api/members_shared_examples.rb b/spec/support/shared_examples/requests/api/members_shared_examples.rb
index 9136f60eb934..135a984f8f9c 100644
--- a/spec/support/shared_examples/requests/api/members_shared_examples.rb
+++ b/spec/support/shared_examples/requests/api/members_shared_examples.rb
@@ -14,7 +14,7 @@
RSpec.shared_examples 'a 403 response when user does not have rights to manage members of a specific access level' do
it 'returns 403' do
- route
+ expect { route }.not_to change { Member.count }
expect(response).to have_gitlab_http_status(:forbidden)
end
--
GitLab