diff --git a/doc/user/compliance/audit_event_types.md b/doc/user/compliance/audit_event_types.md index 487e48a818658649476b413b2f34d6c128e56ed7..30c0dd7808ce9efb459fc2185013c1fafb516f00 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -455,6 +455,8 @@ Audit event types belong to the following product categories. | Type name | Event triggered when | Saved to database | Introduced in | Scope | |:----------|:---------------------|:------------------|:--------------|:------| +| [`admin_role_assigned_to_user`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/186570) | A custom admin role is assigned to a user | {{< icon name="check-circle" >}} Yes | GitLab [18.0](https://gitlab.com/gitlab-org/gitlab/-/issues/507958) | User | +| [`admin_role_unassigned_from_user`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/186570) | A custom admin role is unassigned from a user | {{< icon name="check-circle" >}} Yes | GitLab [18.0](https://gitlab.com/gitlab-org/gitlab/-/issues/507958) | User | | [`member_role_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/137087) | A custom role is created | {{< icon name="check-circle" >}} Yes | GitLab [16.7](https://gitlab.com/gitlab-org/gitlab/-/issues/388934) | Group, Instance | | [`member_role_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/141630) | A custom role is deleted | {{< icon name="check-circle" >}} Yes | GitLab [16.9](https://gitlab.com/gitlab-org/gitlab/-/issues/437672) | Group, Instance | | [`member_role_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/141630) | A custom role is updated | {{< icon name="check-circle" >}} Yes | GitLab [16.9](https://gitlab.com/gitlab-org/gitlab/-/issues/437672) | Group, Instance | diff --git a/ee/app/models/users/user_member_role.rb b/ee/app/models/users/user_member_role.rb index f70d5c59e9e39d78608b53af2252696eeba6596b..7a2d95d7eb3c7c34451a022de3df769b05f09552 100644 --- a/ee/app/models/users/user_member_role.rb +++ b/ee/app/models/users/user_member_role.rb @@ -9,5 +9,11 @@ class UserMemberRole < ApplicationRecord validates :member_role, presence: true validates :user, presence: true, uniqueness: true + + def self.create_or_update(user:, member_role:) + find_or_initialize_by(user: user).tap do |record| + record.update(member_role: member_role) + end + end end end diff --git a/ee/app/services/users/member_roles/assign_service.rb b/ee/app/services/users/member_roles/assign_service.rb index cbd0badc3fb204d8fbab7d083c3271e27f95fcbb..cc3315eb6adb92952cdbdf874a341b7fdb530798 100644 --- a/ee/app/services/users/member_roles/assign_service.rb +++ b/ee/app/services/users/member_roles/assign_service.rb @@ -3,64 +3,92 @@ module Users module MemberRoles class AssignService < BaseService - attr_accessor :current_user, :params + attr_accessor :current_user, :user_to_be_assigned, :admin_role def initialize(current_user, params = {}) @current_user = current_user - @params = params + + @user_to_be_assigned = params[:user] + @admin_role = params[:member_role] end def execute - unless current_user.can?(:admin_member_role) - return ServiceResponse.error(message: 'Forbidden', reason: :forbidden) - end - - unless valid_member_role_param? - return ServiceResponse.error( - message: 'Only admin custom roles can be assigned directly to a user.', - reason: :forbidden - ) - end + return error('custom_roles licensed feature must be available') unless License.feature_available?(:custom_roles) unless Feature.enabled?(:custom_admin_roles, :instance) - return ServiceResponse.error(message: 'Not yet available', reason: :forbidden) + return error('Feature flag `custom_admin_roles` is not enabled for the instance') end - assign + return error('Forbidden') unless current_user.can?(:admin_member_role) + + return error('Only admin custom roles can be assigned directly to a user.') unless admin_related_role? + + assign_or_unassign_admin_role end private - def valid_member_role_param? - return true unless params[:member_role] + def admin_related_role? + return true if admin_role.blank? - params[:member_role].admin_related_role? + admin_role.admin_related_role? end - def assign - user_member_role = if params[:member_role] - handle_assignment - else - existing_user_member_role.destroy! if existing_user_member_role + def assign_or_unassign_admin_role + # if admin role is present -> create or update database record + # if admin role is nil -> that means we are unassigning admin role from user, + # hence destroy any existing records + admin_role ? create_or_update_record : destroy_record + end - nil - end + def create_or_update_record + record = Users::UserMemberRole.create_or_update(user: user_to_be_assigned, member_role: admin_role) - ServiceResponse.success(payload: { user_member_role: user_member_role }) + if record.valid? + log_audit_event( + action: 'admin_role_assigned_to_user', + admin_role: admin_role + ) + success(record) + else + error(record.errors.full_messages.join(', ')) + end end - def handle_assignment - if existing_user_member_role - existing_user_member_role.tap do |user_role| - user_role.update!(member_role: params[:member_role]) - end + def destroy_record + record = Users::UserMemberRole.find_by_user_id(user_to_be_assigned.id) + + return success(nil) unless record + + if record.destroy + log_audit_event( + action: 'admin_role_unassigned_from_user', + admin_role: record.member_role + ) + success(nil) else - Users::UserMemberRole.create(params) + error(record.errors.full_messages.join(', ')) end end - def existing_user_member_role - @existing_user_member_role ||= Users::UserMemberRole.find_by_user_id(params[:user].id) + def log_audit_event(action:, admin_role:) + audit_context = { + name: action, + author: current_user, + scope: user_to_be_assigned, + target: admin_role, + message: action.humanize + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + end + + def success(record) + ::ServiceResponse.success(payload: { user_member_role: record }) + end + + def error(message) + ::ServiceResponse.error(message: message) end end end diff --git a/ee/config/audit_events/types/admin_role_assigned_to_user.yml b/ee/config/audit_events/types/admin_role_assigned_to_user.yml new file mode 100644 index 0000000000000000000000000000000000000000..6887bff266783e30a43da40cb55a108de95fa833 --- /dev/null +++ b/ee/config/audit_events/types/admin_role_assigned_to_user.yml @@ -0,0 +1,10 @@ +--- +name: admin_role_assigned_to_user +description: A custom admin role is assigned to a user +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/507958 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/186570 +feature_category: permissions +milestone: "18.0" +saved_to_database: true +streamed: true +scope: [User] diff --git a/ee/config/audit_events/types/admin_role_unassigned_from_user.yml b/ee/config/audit_events/types/admin_role_unassigned_from_user.yml new file mode 100644 index 0000000000000000000000000000000000000000..a72e72305b0d436307762d8d191e9096b353bc96 --- /dev/null +++ b/ee/config/audit_events/types/admin_role_unassigned_from_user.yml @@ -0,0 +1,10 @@ +--- +name: admin_role_unassigned_from_user +description: A custom admin role is unassigned from a user +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/507958 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/186570 +feature_category: permissions +milestone: "18.0" +saved_to_database: true +streamed: true +scope: [User] diff --git a/ee/spec/models/users/user_member_role_spec.rb b/ee/spec/models/users/user_member_role_spec.rb index bc914815163a5889c363eab1cab0b44a8f69ba85..3a00d64c91cf819930d98f89baf8f6c50272e785 100644 --- a/ee/spec/models/users/user_member_role_spec.rb +++ b/ee/spec/models/users/user_member_role_spec.rb @@ -15,4 +15,37 @@ it { is_expected.to validate_presence_of(:user) } it { is_expected.to validate_uniqueness_of(:user) } end + + describe '.create_or_update' do + let_it_be(:user) { create(:user) } + let_it_be(:admin_role) { create(:member_role, :admin, name: 'Admin role') } + + subject(:create_or_update) do + described_class.create_or_update(user: user, member_role: admin_role) + end + + context 'when user member role record does not exist' do + it 'creates the record' do + expect { create_or_update }.to change { described_class.count }.by(1) + + result = described_class.last + + expect(result.user).to eq(user) + expect(result.member_role).to eq(admin_role) + end + end + + context 'when user member role record exists' do + let_it_be(:user_member_role) { create(:user_member_role, user: user) } + + it 'updates the record' do + create_or_update + + result = user_member_role.reload + + expect(result.user).to eq(user) + expect(result.member_role).to eq(admin_role) + end + end + end end diff --git a/ee/spec/services/users/member_roles/assign_service_spec.rb b/ee/spec/services/users/member_roles/assign_service_spec.rb index a069965a0a4ae797bb0dc15160f374aa8327e964..256843512a42a96045cfc396cb442f2a55254297 100644 --- a/ee/spec/services/users/member_roles/assign_service_spec.rb +++ b/ee/spec/services/users/member_roles/assign_service_spec.rb @@ -3,12 +3,12 @@ require 'spec_helper' RSpec.describe Users::MemberRoles::AssignService, feature_category: :permissions do - let_it_be(:member_role) { create(:member_role, :admin) } + let_it_be(:admin_role_1) { create(:member_role, :admin, name: 'Admin role 1') } let_it_be(:user) { create(:user) } let_it_be_with_reload(:current_user) { create(:admin) } - let(:member_role_param) { member_role } + let(:member_role_param) { admin_role_1 } let(:params) { { user: user, member_role: member_role_param } } subject(:assign_member_role) { described_class.new(current_user, params).execute } @@ -24,94 +24,212 @@ it 'returns an error' do expect(assign_member_role).to be_error + expect(assign_member_role.message).to include('Forbidden') end end context 'when current user is an admin', :enable_admin_mode do - context 'when custom_admin_roles FF is disabled' do + context 'when `custom_admin_roles` feature-flag is disabled' do before do stub_feature_flags(custom_admin_roles: false) end it 'returns an error' do expect(assign_member_role).to be_error + expect(assign_member_role.message).to include( + 'Feature flag `custom_admin_roles` is not enabled for the instance' + ) end end - context 'when custom_admin_roles FF is enabled' do + context 'when custom_roles feature is not available' do + before do + stub_licensed_features(custom_roles: false) + end + + it 'returns an error' do + expect(assign_member_role).to be_error + expect(assign_member_role.message).to include('custom_roles licensed feature must be available') + end + end + + context 'when `custom_admin_roles` feature-flag is enabled' do context 'when member_role param is present' do - context 'when no admin member role is assigned to the user' do - it 'creates a new user member role relation' do - expect { assign_member_role }.to change { Users::UserMemberRole.count }.by(1) + context 'when no admin role is assigned to the user' do + it 'creates a new record' do + expect { assign_member_role }.to change { ::Users::UserMemberRole.count }.by(1) end - it 'returns success' do - expect(assign_member_role).to be_success + it 'assigns the correct member role id' do + expect { assign_member_role }.to change { + user.user_member_roles.first&.member_role_id + }.from(nil).to(admin_role_1.id) end - end - context 'when the user has another admin member role assigned' do - let_it_be(:user_member_role) { create(:user_member_role, member_role: member_role, user: user) } - let_it_be(:new_member_role) { create(:member_role, :admin) } + it 'returns success' do + response = assign_member_role - let(:member_role_param) { new_member_role } + expect(response).to be_success + expect(response.payload[:user_member_role]).to eq(::Users::UserMemberRole.last) + end - it 'does not create a new user member role relation' do - expect { assign_member_role }.not_to change { Users::UserMemberRole.count } + include_examples 'audit event logging' do + let(:licensed_features_to_stub) { { custom_roles: true } } + let(:operation) { assign_member_role } + + let(:fail_condition!) do + allow_next_instance_of(::Users::UserMemberRole) do |record| + allow(record).to receive(:valid?).and_return(false) + end + end + + let(:attributes) do + { + author_id: current_user.id, + entity_id: user.id, + entity_type: 'User', + details: { + event_name: 'admin_role_assigned_to_user', + author_name: current_user.name, + author_class: 'User', + target_id: admin_role_1.id, + target_type: 'MemberRole', + target_details: admin_role_1.name, + custom_message: 'Admin role assigned to user' + } + } + end end + end + + context 'when the user has another admin role assigned' do + let_it_be(:admin_role_2) { create(:member_role, :admin, name: 'Admin role 2') } + let_it_be(:user_member_role) { create(:user_member_role, member_role: admin_role_1, user: user) } - it 'updates the member role assigned to the user' do + let(:member_role_param) { admin_role_2 } + + it 'updates the record' do expect { assign_member_role }.to change { user_member_role.reload.member_role } - .from(member_role).to(new_member_role) + .from(admin_role_1).to(admin_role_2) end it 'returns success' do - expect(assign_member_role).to be_success + response = assign_member_role + + expect(response).to be_success + expect(response.payload[:user_member_role]).to eq(user_member_role) + end + + include_examples 'audit event logging' do + let(:licensed_features_to_stub) { { custom_roles: true } } + let(:operation) { assign_member_role } + + let(:fail_condition!) do + allow_next_found_instance_of(::Users::UserMemberRole) do |record| + allow(record).to receive(:valid?).and_return(false) + end + end + + let(:attributes) do + { + author_id: current_user.id, + entity_id: user.id, + entity_type: 'User', + details: { + event_name: 'admin_role_assigned_to_user', + author_name: current_user.name, + author_class: 'User', + target_id: admin_role_2.id, + target_type: 'MemberRole', + target_details: admin_role_2.name, + custom_message: 'Admin role assigned to user' + } + } + end end end end - context 'when member_role param is null' do - let_it_be(:other_user_member_role) { create(:user_member_role, member_role: member_role) } - + context 'when member_role param is not provided' do let(:member_role_param) { nil } - context 'when user member role relation exists for the user' do - let_it_be(:user_member_role) { create(:user_member_role, member_role: member_role, user: user) } + context 'when a user member role already exists' do + let_it_be(:user_member_role) { create(:user_member_role, member_role: admin_role_1, user: user) } - it 'deletes the existing user member role relation' do - expect { assign_member_role }.to change { Users::UserMemberRole.count }.by(-1) + it 'deletes the existing record' do + expect { assign_member_role }.to change { ::Users::UserMemberRole.count }.by(-1) + end - expect { user_member_role.reload }.to raise_error(ActiveRecord::RecordNotFound) + it 'assigns the correct value' do + expect { assign_member_role }.to change { user.reload.user_member_roles }.to([]) end it 'returns success' do - expect(assign_member_role).to be_success + response = assign_member_role + + expect(response).to be_success + expect(response.payload[:user_member_role]).to be_nil end - end - context 'when user member role relation does not exist for the user' do - it 'does not delete any user member role relation' do - expect { assign_member_role }.not_to change { Users::UserMemberRole.count } + include_examples 'audit event logging' do + let(:licensed_features_to_stub) { { custom_roles: true } } + let(:operation) { assign_member_role } + + let(:fail_condition!) do + allow_next_found_instance_of(::Users::UserMemberRole) do |record| + allow(record).to receive(:destroy).and_return(false) + end + end + + let(:attributes) do + { + author_id: current_user.id, + entity_id: user.id, + entity_type: 'User', + details: { + event_name: 'admin_role_unassigned_from_user', + author_name: current_user.name, + author_class: 'User', + target_id: admin_role_1.id, + target_type: 'MemberRole', + target_details: admin_role_1.name, + custom_message: 'Admin role unassigned from user' + } + } + end + end + end - expect(other_user_member_role.reload).not_to be_nil + context 'when a user member role does not exist' do + it 'does not delete any records' do + expect { assign_member_role }.not_to change { ::Users::UserMemberRole.count } end it 'returns success' do - expect(assign_member_role).to be_success + response = assign_member_role + + expect(response).to be_success + expect(response.payload[:user_member_role]).to be_nil + end + + it 'does not log an audit event' do + expect(Gitlab::Audit::Auditor).not_to receive(:audit) end end end - context 'when the provided custom role is not an admin role' do - let_it_be(:member_role) { create(:member_role) } + context 'when the provided member role is not an admin role' do + let_it_be(:member_role) { create(:member_role, name: 'Standard role') } + + let(:member_role_param) { member_role } it 'does not create a new user member role relation' do - expect { assign_member_role }.not_to change { Users::UserMemberRole.count } + expect { assign_member_role }.not_to change { ::Users::UserMemberRole.count } end it 'returns error' do expect(assign_member_role).to be_error + expect(assign_member_role.message).to include('Only admin custom roles can be assigned directly to a user.') end end end