diff --git a/ee/app/models/ee/member.rb b/ee/app/models/ee/member.rb index a1eb347c1fa21b0f1e325100e34465c80e6d60c4..f33f4fcfa995381b7425419b2112a03519c46dd2 100644 --- a/ee/app/models/ee/member.rb +++ b/ee/app/models/ee/member.rb @@ -161,14 +161,7 @@ def last_owner? end def has_capacity_left? - return true unless source.root_ancestor.apply_user_cap? - return true if any_existing_active_membership? - - !source.root_ancestor.user_limit_reached? - end - - def any_existing_active_membership? - user && ::Member.in_hierarchy(source).with_user(user).with_state(:active).any? + source.root_ancestor.capacity_left_for_user?(user) end end end diff --git a/ee/app/models/ee/namespace.rb b/ee/app/models/ee/namespace.rb index 30c1468e4f8741a87ae8dcc03622832cae553aa7..dd4750928b86de2ca233456047dfe88644bcf23d 100644 --- a/ee/app/models/ee/namespace.rb +++ b/ee/app/models/ee/namespace.rb @@ -468,6 +468,13 @@ def user_limit_reached?(use_cache: false) free_user_cap.reached_limit? end + def capacity_left_for_user?(user) + return true unless apply_user_cap? + return true if ::Member.in_hierarchy(root_ancestor).with_user(user).with_state(:active).exists? + + !user_limit_reached? + end + def free_plan_user_ids strong_memoize(:free_plan_user_ids) do billed_users.pluck(:id) diff --git a/ee/app/services/members/activate_service.rb b/ee/app/services/members/activate_service.rb index 51446bd37f77f31b538988ad7cd24e988f3b24cf..e16b98953bed8c0ea38c333e78110005f532e32c 100644 --- a/ee/app/services/members/activate_service.rb +++ b/ee/app/services/members/activate_service.rb @@ -26,14 +26,10 @@ def execute return error(_('No group provided')) unless group return error(_('You do not have permission to approve a member'), :forbidden) unless allowed? return error(_('You can only approve an indivdual user, member, or all members')) unless valid_params? + return error(_('You cannot approve all pending members on a free plan')) if activate_all && group.free_plan? + return error(_('There is no seat left to activate the member')) unless has_capacity_left? - if activate_memberships - log_event - - success - else - error(_('No memberships found'), :bad_request) - end + activate_memberships end private @@ -45,16 +41,25 @@ def valid_params? end def activate_memberships - memberships_found = false memberships = activate_all ? awaiting_memberships : scoped_memberships + affected_user_ids = Set.new + memberships.find_each do |member| - memberships_found = true + member.update_columns(state: ::Member::STATE_ACTIVE, updated_at: Time.current) - member.activate + affected_user_ids.add(member.user_id) end - memberships_found + if !affected_user_ids.empty? + UserProjectAccessChangedService.new(affected_user_ids.to_a).execute(blocking: false) + + log_audit_event unless activate_all + log_event + success + else + error(_('No memberships found'), :bad_request) + end end # rubocop: disable CodeReuse/ActiveRecord @@ -77,6 +82,12 @@ def allowed? can?(current_user, :admin_group_member, group) end + def has_capacity_left? + return true if activate_all && !group.free_plan? + + group.root_ancestor.capacity_left_for_user?(user || member.user) + end + def log_event log_params = { group: group.id, @@ -88,8 +99,20 @@ def log_event params[:user] = user.id if user end end - Gitlab::AppLogger.info(log_params) end + + def log_audit_event + target = user || member&.user + return unless target + + ::Gitlab::Audit::Auditor.audit( + name: 'change_membership_state', + author: current_user, + scope: group, + target: target, + message: 'Changed the membership state to active' + ) + end end end diff --git a/ee/app/services/members/await_service.rb b/ee/app/services/members/await_service.rb index 69dcaa04062100b58830f5864cb1a2754d6f9d19..e83bd411f368625686ab00257e894cfde2280ed9 100644 --- a/ee/app/services/members/await_service.rb +++ b/ee/app/services/members/await_service.rb @@ -15,6 +15,8 @@ def execute return error(_('No group provided')) unless group return error(_('No user provided')) unless user return error(_('You do not have permission to set a member awaiting')) unless allowed? + return error(_('The last owner cannot be set to awaiting')) if group.last_owner?(user) + return error(_('You cannot set yourself to awaiting')) if current_user == user set_memberships_to_awaiting end @@ -24,14 +26,14 @@ def execute attr_reader :group, :current_user, :user def set_memberships_to_awaiting - memberships_found = false + # rubocop: disable CodeReuse/ActiveRecord + affected_memberships = Member.where(id: memberships) + .update_all(state: ::Member::STATE_AWAITING, updated_at: Time.current) + # rubocop: enable CodeReuse/ActiveRecord - memberships.find_each do |member| - memberships_found = true - member.wait - end + if affected_memberships > 0 + UserProjectAccessChangedService.new(user.id).execute(blocking: false) - if memberships_found log_audit_event ServiceResponse.success else diff --git a/ee/spec/models/ee/namespace_spec.rb b/ee/spec/models/ee/namespace_spec.rb index 0db58be1e442a2edba6a10533160f41466cd109a..11ce5d874c7859becbcf1a0b6bd55176d14268db 100644 --- a/ee/spec/models/ee/namespace_spec.rb +++ b/ee/spec/models/ee/namespace_spec.rb @@ -1826,6 +1826,39 @@ end end + describe '#capacity_left_for_user?' do + let(:namespace) { create(:group) } + let(:user) { create(:user) } + + where(:apply_user_cap, :user_limit_reached, :existing_membership, :result) do + false | false | false | true + false | false | true | true + false | true | true | true + true | false | false | true + true | false | true | true + true | true | true | true + true | true | false | false + end + + subject { namespace.capacity_left_for_user?(user) } + + with_them do + before do + create(:group_member, group: namespace, user: user) if existing_membership + + free_user_cap = instance_double( + Namespaces::FreeUserCap::Standard, + enforce_cap?: apply_user_cap, + reached_limit?: user_limit_reached + ) + + allow(namespace).to receive(:free_user_cap).and_return(free_user_cap) + end + + it { is_expected.to eq(result) } + end + end + describe '#has_free_or_no_subscription?', :saas do it 'returns true with a free plan' do namespace = create(:group_with_plan, plan: :free_plan) diff --git a/ee/spec/services/members/activate_service_spec.rb b/ee/spec/services/members/activate_service_spec.rb index b89e608e2b56d5e3ac12215ba17712be125c284a..d33e7fd398ddd6e535ede25060dede0bdafe2e83 100644 --- a/ee/spec/services/members/activate_service_spec.rb +++ b/ee/spec/services/members/activate_service_spec.rb @@ -3,6 +3,150 @@ require 'spec_helper' RSpec.describe Members::ActivateService do + shared_examples 'handles free user cap' do + context 'check if free user cap has been reached', :saas do + let_it_be_with_reload(:root_group) { create(:group_with_plan, plan: :free_plan) } + let_it_be_with_reload(:sub_group) { create(:group, parent: root_group) } + let_it_be_with_reload(:project) { create(:project, namespace: root_group) } + + before do + allow(group).to receive(:user_cap_available?).and_return(false) + stub_ee_application_setting(should_check_namespace_plan: true) + end + + context 'when the :free_user_cap feature flag is disabled' do + before do + stub_feature_flags(free_user_cap: false) + end + + it_behaves_like 'successful member activation' do + let(:member) { create(:group_member, :awaiting, group: group, user: user) } + end + end + + context 'when the :free_user_cap feature flag is enabled' do + before do + stub_feature_flags(free_user_cap: true) + end + + context 'when the free user cap has not been reached' do + it_behaves_like 'successful member activation' do + let(:member) { create(:group_member, :awaiting, group: root_group, user: user) } + end + + it_behaves_like 'successful member activation' do + let(:member) { create(:group_member, :awaiting, group: sub_group, user: user) } + end + + it_behaves_like 'successful member activation' do + let(:member) { create(:project_member, :awaiting, project: project, user: user) } + end + end + + context 'when the free user cap has been reached' do + before do + stub_const('::Namespaces::FreeUserCap::FREE_USER_LIMIT', 1) + end + + context 'when group member' do + let(:member) { create(:group_member, :awaiting, group: root_group, user: user) } + + it 'keeps the member awaiting' do + expect(member).to be_awaiting + + result = execute + + expect(result[:status]).to eq :error + expect(result[:message]).to eq 'There is no seat left to activate the member' + expect(member.reload).to be_awaiting + end + end + + context 'when sub-group member' do + let(:member) { create(:group_member, :awaiting, group: sub_group, user: user) } + + it 'keeps the member awaiting' do + expect(member).to be_awaiting + + result = execute + + expect(result[:status]).to eq :error + expect(result[:message]).to eq 'There is no seat left to activate the member' + expect(member.reload).to be_awaiting + end + end + + context 'when project member' do + let(:member) { create(:project_member, :awaiting, project: project, user: user) } + + it 'keeps the member awaiting' do + expect(member).to be_awaiting + + result = execute + + expect(result[:status]).to eq :error + expect(result[:message]).to eq 'There is no seat left to activate the member' + expect(member.reload).to be_awaiting + end + end + + context 'when there is already an active membership' do + before do + stub_const('::Namespaces::FreeUserCap::FREE_USER_LIMIT', 2) + end + + context 'when active group membership' do + let(:member) { create(:group_member, :awaiting, group: sub_group, user: user) } + + before do + create(:group_member, :active, group: group, user: user) + end + + it 'sets the group member to active' do + expect(member).to be_awaiting + + execute + + expect(member.reload).to be_active + end + end + + context 'when active project membership' do + let(:member) { create(:group_member, :awaiting, group: group, user: user) } + + before do + create(:project_member, :active, project: project, user: user) + end + + it 'sets the group member to active' do + expect(member).to be_awaiting + + execute + + expect(member.reload).to be_active + end + end + end + end + end + end + end + + # There is a bug where member records are not valid when the membership to the sub-group + # has a lower access level than the membership to the parent group. + # https://gitlab.com/gitlab-org/gitlab/-/issues/362091 + shared_examples 'when user has multiple memberships with invalid access levels' do + let_it_be(:member) { create(:group_member, :awaiting, :developer, group: sub_group, user: user) } + let_it_be(:parent_membership) { create(:group_member, :awaiting, :maintainer, group: root_group, user: user) } + + it 'activates all memberships' do + execute + + expect(member.reload).to be_active + expect(parent_membership.reload).to be_active + end + end + describe '#execute' do let_it_be(:current_user) { create(:user) } let_it_be(:user) { create(:user) } @@ -41,9 +185,18 @@ expect(member.awaiting?).to be true end - it 'activates the member' do + it 'activates the member and sets updated_at', :freeze_time do expect(execute[:status]).to eq :success expect(member.reload.active?).to be true + expect(member.updated_at).to eq(Time.current) + end + + it 'calls UserProjectAccessChangedService' do + expect_next_instance_of(UserProjectAccessChangedService, [user.id]) do |service| + expect(service).to receive(:execute).with(blocking: false) + end + + execute end it 'logs the approval in application logs' do @@ -63,6 +216,16 @@ execute end + + it 'tracks an audit event' do + execute + + audit_event = AuditEvent.find_by(author_id: current_user) + expect(audit_event.author).to eq(current_user) + expect(audit_event.entity).to eq(group) + expect(audit_event.target_id).to eq(user.id) + expect(audit_event.details[:custom_message]).to eq('Changed the membership state to active') + end end context 'when authorized' do @@ -149,6 +312,20 @@ end end end + + context 'when member is not member of the group' do + let_it_be(:member) { create(:group_member, :awaiting, group: create(:group), user: user) } + + it 'returns an error' do + result = execute + + expect(result[:status]).to eq :error + expect(result[:message]).to eq 'No memberships found' + end + end + + it_behaves_like 'handles free user cap' + it_behaves_like 'when user has multiple memberships with invalid access levels' end context 'when activating a user' do @@ -195,6 +372,20 @@ expect(sub_member.reload.active?).to be true end end + + context 'when user is not member of the group' do + let_it_be(:member) { create(:group_member, :awaiting, group: create(:group), user: user) } + + it 'returns an error' do + result = execute + + expect(result[:status]).to eq :error + expect(result[:message]).to eq 'No memberships found' + end + end + + it_behaves_like 'handles free user cap' + it_behaves_like 'when user has multiple memberships with invalid access levels' end context 'when activating all awaiting members' do @@ -240,6 +431,39 @@ execute end + + it 'calls UserProjectAccessChangedService' do + double = instance_double(UserProjectAccessChangedService, :execute) + user_ids = [group_members, sub_group_members, project_members].flatten.map { |m| m.user_id } + + expect(UserProjectAccessChangedService).to receive(:new).with(match_array(user_ids)).and_return(double) + expect(double).to receive(:execute).with(blocking: false) + + execute + end + + context 'when on saas', :saas do + context 'when group is a group with paid plan' do + let_it_be_with_reload(:root_group) { create(:group_with_plan, plan: :premium_plan) } + + it 'is successful' do + result = execute + + expect(result[:status]).to eq :success + end + end + + context 'when group is a group with a free plan' do + let_it_be_with_reload(:root_group) { create(:group_with_plan, plan: :free_plan) } + + it 'returns an error' do + result = execute + + expect(result[:status]).to eq :error + expect(result[:message]).to eq 'You cannot approve all pending members on a free plan' + end + end + end end end end diff --git a/ee/spec/services/members/await_service_spec.rb b/ee/spec/services/members/await_service_spec.rb index 7c93a9788c4c6578e1373a0d731299491b5a233f..0e0b2a76e27fa301963556074590b9a7ae09527a 100644 --- a/ee/spec/services/members/await_service_spec.rb +++ b/ee/spec/services/members/await_service_spec.rb @@ -33,10 +33,19 @@ expect(member).to be_active end - it 'sets the member state to awaiting', :aggregate_failures do + it 'sets the member state to awaiting and sets updated_at', :aggregate_failures, :freeze_time do expect(execute).to be_success expect(member.reload).to be_awaiting + expect(member.updated_at).to eq(Time.current) + end + + it 'calls UserProjectAccessChangedService' do + expect_next_instance_of(UserProjectAccessChangedService, user.id) do |service| + expect(service).to receive(:execute).with(blocking: false) + end + + execute end it 'tracks an audit event' do @@ -71,50 +80,96 @@ group.add_owner(current_user) end - context 'when member of the root group' do - it_behaves_like 'succesfully sets member to be awaiting' do - let(:member) { create(:group_member, :active, group: root_group, user: user) } + context 'when not the last owner' do + let_it_be(:owner) { create(:group_member, :owner, group: root_group)} + + context 'when member of the root group' do + it_behaves_like 'succesfully sets member to be awaiting' do + let(:member) { create(:group_member, :active, group: root_group, user: user) } + end end - end - context 'when member of a sub-group' do - it_behaves_like 'succesfully sets member to be awaiting' do - let(:member) { create(:group_member, :active, group: sub_group, user: user) } + context 'when member of a sub-group' do + it_behaves_like 'succesfully sets member to be awaiting' do + let(:member) { create(:group_member, :active, group: sub_group, user: user) } + end end - end - context 'when member is an awaiting member of a project' do - it_behaves_like 'succesfully sets member to be awaiting' do - let(:member) { create(:project_member, :active, project: project, user: user) } + context 'when member is an awaiting member of a project' do + it_behaves_like 'succesfully sets member to be awaiting' do + let(:member) { create(:project_member, :active, project: project, user: user) } + end end - end - context 'when there are multiple member records in the hierarchy' do - let_it_be(:root_member) { create(:group_member, :active, :developer, group: root_group, user: user) } - let_it_be(:sub_member) { create(:group_member, :active, :maintainer, group: sub_group, user: user) } - let_it_be(:project_member) { create(:project_member, :active, :maintainer, project: project, user: user) } + context 'when there are multiple member records in the hierarchy' do + let_it_be(:root_member) { create(:group_member, :active, :developer, group: root_group, user: user) } + let_it_be(:sub_member) { create(:group_member, :active, :maintainer, group: sub_group, user: user) } + let_it_be(:project_member) { create(:project_member, :active, :maintainer, project: project, user: user) } - it 'sets them all to awaiting', :aggregate_failures do - expect(execute).to be_success + it 'sets them all to awaiting', :aggregate_failures do + expect(execute).to be_success - expect(root_member.reload).to be_awaiting - expect(sub_member.reload).to be_awaiting - expect(project_member.reload).to be_awaiting + expect(root_member.reload).to be_awaiting + expect(sub_member.reload).to be_awaiting + expect(project_member.reload).to be_awaiting + end end - end - context 'when there are no active memberships' do - let_it_be(:root_member) { create(:group_member, :awaiting, :developer, group: root_group, user: user) } + context 'when there are no active memberships' do + let_it_be(:root_member) { create(:group_member, :awaiting, :developer, group: root_group, user: user) } - it_behaves_like 'returns an error', 'No memberships found' - end + it_behaves_like 'returns an error', 'No memberships found' + end - it 'does not affect other memberships' do - other_member = create(:group_member, :awaiting, group: root_group, user: create(:user)) + it 'does not affect other memberships' do + other_member = create(:group_member, :awaiting, group: root_group, user: create(:user)) - execute + execute + + expect(other_member.reload).to be_awaiting + end + + context 'when current_user is the same user' do + let_it_be(:current_user) { user } + + before do + group.add_owner(user) + end + + it_behaves_like 'returns an error', 'You cannot set yourself to awaiting' + end + + context 'when user is not member of the group' do + let_it_be(:member) { create(:group_member, :active, group: create(:group), user: user) } + + it 'returns an error' do + result = execute + + expect(result[:status]).to eq :error + expect(result[:message]).to eq 'No memberships found' + end + end + + # There is a bug where member records are not valid when the membership to the sub-group + # has a lower access level than the membership to the parent group. + # https://gitlab.com/gitlab-org/gitlab/-/issues/362091 + context 'when user has multiple memberships with invalid access levels' do + let_it_be(:sub_membership) { create(:group_member, :active, :developer, group: sub_group, user: user) } + let_it_be(:parent_membership) { create(:group_member, :active, :maintainer, group: root_group, user: user) } + + it 'sets all memberships to be awaiting' do + execute + + expect(sub_membership.reload).to be_awaiting + expect(parent_membership.reload).to be_awaiting + end + end + end + + context 'when user is the last owner' do + let_it_be(:user) { current_user } - expect(other_member.reload).to be_awaiting + it_behaves_like 'returns an error', 'The last owner cannot be set to awaiting' end end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 6c3fa19e63024366efa52644ff48b56ec3c94727..265841e971fa35a3ab5ad8cfef65fc6f701c4f33 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -37929,6 +37929,9 @@ msgstr "" msgid "The issue was successfully promoted to an epic. Redirecting to epic..." msgstr "" +msgid "The last owner cannot be set to awaiting" +msgstr "" + msgid "The latest artifacts created by jobs in the most recent successful pipeline will be stored." msgstr "" @@ -38301,6 +38304,9 @@ msgstr "" msgid "There is no data available. Please change your selection." msgstr "" +msgid "There is no seat left to activate the member" +msgstr "" + msgid "There is no table data available." msgstr "" @@ -43439,6 +43445,9 @@ msgstr "" msgid "You cannot access the raw file. Please wait a minute." msgstr "" +msgid "You cannot approve all pending members on a free plan" +msgstr "" + msgid "You cannot approve your own deployment." msgstr "" @@ -43463,6 +43472,9 @@ msgstr "" msgid "You cannot rename an environment after it's created." msgstr "" +msgid "You cannot set yourself to awaiting" +msgstr "" + msgid "You cannot write to a read-only secondary GitLab Geo instance. Please use %{link_to_primary_node} instead." msgstr ""