From 6d43e46437279dc5a576f21c89ceb38325e052a0 Mon Sep 17 00:00:00 2001 From: Nicolas Dular Date: Wed, 18 May 2022 12:43:32 +0200 Subject: [PATCH 1/2] Fix Members Activate and Await Service It's possible for membership records to be invalid when the inherited `access_level` is higher. When using the state machine to transition between membership states, it would validate the records and therefore fail to change the state. To prevent this bug from happening we skip the validation by using `update_all`. To still guarantee valid data, we moved the validation and restrictions to the `Members::ActivateService` and `Members::AwaitService`. * We now call `UserProjectAccessChangedService` when setting a member to be `awaiting`, which runs otherwise as an `after_commit` hook. * We check the free user cap limits when activating a user. * We prevent the last owner to be set to awaiting. * We don't allow a user to set themselves to be `awaiting` anymore. --- ee/app/models/ee/member.rb | 9 +- ee/app/models/ee/namespace.rb | 7 + ee/app/services/members/activate_service.rb | 32 ++- ee/app/services/members/await_service.rb | 14 +- ee/spec/models/ee/namespace_spec.rb | 33 +++ .../services/members/activate_service_spec.rb | 213 ++++++++++++++++++ .../services/members/await_service_spec.rb | 114 +++++++--- locale/gitlab.pot | 12 + 8 files changed, 379 insertions(+), 55 deletions(-) diff --git a/ee/app/models/ee/member.rb b/ee/app/models/ee/member.rb index a1eb347c1fa21b..f33f4fcfa99538 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 30c1468e4f8741..dd4750928b86de 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 51446bd37f77f3..b0a6f5c85e84e4 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,24 @@ 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) - 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_event + success + else + error(_('No memberships found'), :bad_request) + end end # rubocop: disable CodeReuse/ActiveRecord @@ -77,6 +81,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, diff --git a/ee/app/services/members/await_service.rb b/ee/app/services/members/await_service.rb index 69dcaa04062100..af5ff64b757e44 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) + # 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 0db58be1e442a2..11ce5d874c7859 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 b89e608e2b56d5..1961e80c1640a9 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) } @@ -46,6 +190,14 @@ expect(member.reload.active?).to be true 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 expected_params = { message: "Group member access approved", @@ -149,6 +301,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 +361,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 +420,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 7c93a9788c4c65..38f2c1490a7971 100644 --- a/ee/spec/services/members/await_service_spec.rb +++ b/ee/spec/services/members/await_service_spec.rb @@ -39,6 +39,14 @@ expect(member.reload).to be_awaiting 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 execute @@ -71,50 +79,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 6c3fa19e630243..265841e971fa35 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 "" -- GitLab From 1661bd6e1eac201e84f126cc22ee2a8f7e518929 Mon Sep 17 00:00:00 2001 From: Nicolas Dular Date: Tue, 31 May 2022 11:14:51 +0200 Subject: [PATCH 2/2] Set updated_at and create audit_event on activate * Sets `updated_at` manually as it would not get triggered by `updated_all` or `update_columns`. * Creates an audit event when activating a single user. --- ee/app/services/members/activate_service.rb | 17 +++++++++++++++-- ee/app/services/members/await_service.rb | 2 +- .../services/members/activate_service_spec.rb | 13 ++++++++++++- ee/spec/services/members/await_service_spec.rb | 3 ++- 4 files changed, 30 insertions(+), 5 deletions(-) diff --git a/ee/app/services/members/activate_service.rb b/ee/app/services/members/activate_service.rb index b0a6f5c85e84e4..e16b98953bed8c 100644 --- a/ee/app/services/members/activate_service.rb +++ b/ee/app/services/members/activate_service.rb @@ -46,7 +46,7 @@ def activate_memberships affected_user_ids = Set.new memberships.find_each do |member| - member.update_columns(state: ::Member::STATE_ACTIVE) + member.update_columns(state: ::Member::STATE_ACTIVE, updated_at: Time.current) affected_user_ids.add(member.user_id) end @@ -54,6 +54,7 @@ def activate_memberships 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 @@ -98,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 af5ff64b757e44..e83bd411f36862 100644 --- a/ee/app/services/members/await_service.rb +++ b/ee/app/services/members/await_service.rb @@ -28,7 +28,7 @@ def execute def set_memberships_to_awaiting # rubocop: disable CodeReuse/ActiveRecord affected_memberships = Member.where(id: memberships) - .update_all(state: ::Member::STATE_AWAITING) + .update_all(state: ::Member::STATE_AWAITING, updated_at: Time.current) # rubocop: enable CodeReuse/ActiveRecord if affected_memberships > 0 diff --git a/ee/spec/services/members/activate_service_spec.rb b/ee/spec/services/members/activate_service_spec.rb index 1961e80c1640a9..d33e7fd398ddd6 100644 --- a/ee/spec/services/members/activate_service_spec.rb +++ b/ee/spec/services/members/activate_service_spec.rb @@ -185,9 +185,10 @@ 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 @@ -215,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 diff --git a/ee/spec/services/members/await_service_spec.rb b/ee/spec/services/members/await_service_spec.rb index 38f2c1490a7971..0e0b2a76e27fa3 100644 --- a/ee/spec/services/members/await_service_spec.rb +++ b/ee/spec/services/members/await_service_spec.rb @@ -33,10 +33,11 @@ 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 -- GitLab