From 3cccd102ba543e02725d247893729e5c73b38295 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 20 Apr 2022 10:00:54 +0000 Subject: Add latest changes from gitlab-org/gitlab@14-10-stable-ee --- app/services/members/create_service.rb | 63 ++++++++++++++++------- app/services/members/creator_service.rb | 64 +++++++++++++++++++----- app/services/members/groups/creator_service.rb | 12 ++--- app/services/members/invite_service.rb | 55 ++++++++++++++------ app/services/members/projects/creator_service.rb | 25 ++++++--- 5 files changed, 161 insertions(+), 58 deletions(-) (limited to 'app/services/members') diff --git a/app/services/members/create_service.rb b/app/services/members/create_service.rb index 758fa2e67f1..8f7b63c32c8 100644 --- a/app/services/members/create_service.rb +++ b/app/services/members/create_service.rb @@ -14,8 +14,9 @@ module Members super @errors = [] - @invites = invites_from_params&.split(',')&.uniq&.flatten + @invites = invites_from_params @source = params[:source] + @tasks_to_be_done_members = [] end def execute @@ -25,6 +26,7 @@ module Members validate_invitable! add_members + create_tasks_to_be_done enqueue_onboarding_progress_action publish_event! @@ -40,10 +42,13 @@ module Members private - attr_reader :source, :errors, :invites, :member_created_namespace_id, :members + attr_reader :source, :errors, :invites, :member_created_namespace_id, :members, + :tasks_to_be_done_members, :member_created_member_task_id def invites_from_params - params[:user_ids] + return params[:user_ids] if params[:user_ids].is_a?(Array) + + params[:user_ids]&.to_s&.split(',')&.uniq&.flatten || [] end def validate_invite_source! @@ -74,33 +79,45 @@ module Members ) members.each { |member| process_result(member) } - - create_tasks_to_be_done end def process_result(member) - if member.invalid? - add_error_for_member(member) + existing_errors = member.errors.full_messages + + # calling invalid? clears any errors that were added outside of the + # rails validation process + if member.invalid? || existing_errors.present? + add_error_for_member(member, existing_errors) else after_execute(member: member) @member_created_namespace_id ||= member.namespace_id end end - def add_error_for_member(member) + # overridden + def add_error_for_member(member, existing_errors) prefix = "#{member.user.username}: " if member.user.present? - errors << "#{prefix}#{member.errors.full_messages.to_sentence}" + errors << "#{prefix}#{all_member_errors(member, existing_errors).to_sentence}" + end + + def all_member_errors(member, existing_errors) + existing_errors.concat(member.errors.full_messages).uniq end def after_execute(member:) super + build_tasks_to_be_done_members(member) track_invite_source(member) end def track_invite_source(member) - Gitlab::Tracking.event(self.class.name, 'create_member', label: invite_source, property: tracking_property(member), user: current_user) + Gitlab::Tracking.event(self.class.name, + 'create_member', + label: invite_source, + property: tracking_property(member), + user: current_user) end def invite_source @@ -114,16 +131,28 @@ module Members member.invite? ? 'net_new_user' : 'existing_user' end - def create_tasks_to_be_done - return if params[:tasks_to_be_done].blank? || params[:tasks_project_id].blank? - - valid_members = members.select { |member| member.valid? && member.member_task.valid? } - return unless valid_members.present? + def build_tasks_to_be_done_members(member) + return unless tasks_to_be_done?(member) + @tasks_to_be_done_members << member # We can take the first `member_task` here, since all tasks will have the same attributes needed # for the `TasksToBeDone::CreateWorker`, ie. `project` and `tasks_to_be_done`. - member_task = valid_members[0].member_task - TasksToBeDone::CreateWorker.perform_async(member_task.id, current_user.id, valid_members.map(&:user_id)) + @member_created_member_task_id ||= member.member_task.id + end + + def tasks_to_be_done?(member) + return false if params[:tasks_to_be_done].blank? || params[:tasks_project_id].blank? + + # Only create task issues for existing users. Tasks for new users are created when they signup. + member.member_task&.valid? && member.user.present? + end + + def create_tasks_to_be_done + return unless member_created_member_task_id # signal if there is any work to be done here + + TasksToBeDone::CreateWorker.perform_async(member_created_member_task_id, + current_user.id, + tasks_to_be_done_members.map(&:user_id)) end def user_limit diff --git a/app/services/members/creator_service.rb b/app/services/members/creator_service.rb index fcce32ead94..321658ac9c5 100644 --- a/app/services/members/creator_service.rb +++ b/app/services/members/creator_service.rb @@ -4,15 +4,13 @@ module Members # This class serves as more of an app-wide way we add/create members # All roads to add members should take this path. class CreatorService - include Gitlab::Experiment::Dsl - class << self def parsed_access_level(access_level) access_levels.fetch(access_level) { access_level.to_i } end def access_levels - raise NotImplementedError + Gitlab::Access.sym_options_with_owner end end @@ -25,7 +23,7 @@ module Members def execute find_or_build_member - update_member + commit_member create_member_task member @@ -33,23 +31,39 @@ module Members private + delegate :new_record?, to: :member attr_reader :source, :user, :access_level, :member, :args - def update_member - return unless can_update_member? - + def assign_member_attributes member.attributes = member_attributes + end - if member.request? - approve_request + def commit_member + if can_commit_member? + assign_member_attributes + commit_changes else - member.save + add_commit_error end end - def can_update_member? + def can_commit_member? # There is no current user for bulk actions, in which case anything is allowed - !current_user # inheriting classes will add more logic + return true if skip_authorization? + + if new_record? + can_create_new_member? + else + can_update_existing_member? + end + end + + def can_create_new_member? + raise NotImplementedError + end + + def can_update_existing_member? + raise NotImplementedError end # Populates the attributes of a member. @@ -64,6 +78,14 @@ module Members } end + def commit_changes + if member.request? + approve_request + else + member.save + end + end + def create_member_task return unless member.persisted? return if member_task_attributes.value?(nil) @@ -93,6 +115,20 @@ module Members args[:current_user] end + def skip_authorization? + !current_user + end + + def add_commit_error + msg = if new_record? + _('not authorized to create member') + else + _('not authorized to update member') + end + + member.errors.add(:base, msg) + end + def find_or_build_member @user = parse_user_param @@ -101,6 +137,8 @@ module Members else source.members.build(invite_email: user) end + + @member.blocking_refresh = args[:blocking_refresh] end # This method is used to find users that have been entered into the "Add members" field. @@ -114,7 +152,7 @@ module Members User.find_by(id: user) # rubocop:todo CodeReuse/ActiveRecord else # must be an email or at least we'll consider it one - User.find_by_any_email(user) || user + source.users_by_emails([user])[user] || user end end diff --git a/app/services/members/groups/creator_service.rb b/app/services/members/groups/creator_service.rb index df4d3f59d3b..a6f0daa99aa 100644 --- a/app/services/members/groups/creator_service.rb +++ b/app/services/members/groups/creator_service.rb @@ -3,14 +3,14 @@ module Members module Groups class CreatorService < Members::CreatorService - def self.access_levels - Gitlab::Access.sym_options_with_owner - end - private - def can_update_member? - super || current_user.can?(:update_group_member, member) + def can_create_new_member? + current_user.can?(:admin_group_member, member.group) + end + + def can_update_existing_member? + current_user.can?(:update_group_member, member) end end end diff --git a/app/services/members/invite_service.rb b/app/services/members/invite_service.rb index 85acb720f0f..1bf209ab79d 100644 --- a/app/services/members/invite_service.rb +++ b/app/services/members/invite_service.rb @@ -7,6 +7,8 @@ module Members def initialize(*args) super + @invites += parsed_emails + @errors = {} end @@ -14,38 +16,63 @@ module Members alias_method :formatted_errors, :errors - def invites_from_params - params[:email] + def parsed_emails + # can't put this in the initializer since `invites_from_params` is called in super class + # and needs it + @parsed_emails ||= (formatted_param(params[:email]) || []) + end + + def formatted_param(parameter) + parameter&.split(',')&.uniq&.flatten end def validate_invitable! super + return if params[:email].blank? + # we need the below due to add_users hitting Members::CreatorService.parse_users_list and ignoring invalid emails # ideally we wouldn't need this, but we can't really change the add_users method - valid, invalid = invites.partition { |email| Member.valid_email?(email) } - @invites = valid + invalid_emails.each { |email| errors[email] = s_('AddMember|Invite email is invalid') } + end + + def invalid_emails + parsed_emails.each_with_object([]) do |email, invalid| + next if Member.valid_email?(email) - invalid.each { |email| errors[email] = s_('AddMember|Invite email is invalid') } + invalid << email + @invites.delete(email) + end end override :blank_invites_message def blank_invites_message - s_('AddMember|Emails cannot be blank') + s_('AddMember|Invites cannot be blank') end override :add_error_for_member - def add_error_for_member(member) - errors[invite_email(member)] = member.errors.full_messages.to_sentence + def add_error_for_member(member, existing_errors) + errors[invited_object(member)] = all_member_errors(member, existing_errors).to_sentence end - override :create_tasks_to_be_done - def create_tasks_to_be_done - # Only create task issues for existing users. Tasks for new users are created when they signup. - end + def invited_object(member) + return member.invite_email if member.invite_email - def invite_email(member) - member.invite_email || member.user.email + # There is a case where someone was invited by email, but the `user` record exists. + # The member record returned will not have an invite_email attribute defined since + # the CreatorService finds `user` record sometimes by email. + # At that point we lose the info of whether this invite was done by `user` or by email. + # Here we will give preference to check invites by user_id first. + # There is also a case where a user could be invited by their email and + # at the same time via the API in the same request. + # This would would mean the same user is invited as user_id and email. + # However, that isn't as likely from the UI at least since the token generator checks + # for that case and doesn't allow email being used if the user exists as a record already. + if member.user_id.to_s.in?(invites) + member.user.username + else + member.user.all_emails.detect { |email| email.in?(invites) } + end end end end diff --git a/app/services/members/projects/creator_service.rb b/app/services/members/projects/creator_service.rb index 4dba81acf73..d92fe60c54a 100644 --- a/app/services/members/projects/creator_service.rb +++ b/app/services/members/projects/creator_service.rb @@ -3,19 +3,28 @@ module Members module Projects class CreatorService < Members::CreatorService - def self.access_levels - Gitlab::Access.sym_options_with_owner - end - private - def can_update_member? - super || current_user.can?(:update_project_member, member) || adding_a_new_owner? + def can_create_new_member? + # order is important here! + # The `admin_project_member` check has side-effects that causes projects not be created if this area is hit + # during project creation. + # Call that triggers is current_user.can?(:admin_project_member, member.project) + # I tracked back to base_policy.rb admin check and specifically in + # Gitlab::Auth::CurrentUserMode.new(@user).admin_mode? call. + # This calls user.admin? and that specific call causes issues with project creation in + # spec/requests/api/projects_spec.rb specs and others, mostly around project creation. + # https://gitlab.com/gitlab-org/gitlab/-/issues/358931 for investigation + adding_the_creator_as_owner_in_a_personal_project? || current_user.can?(:admin_project_member, member.project) + end + + def can_update_existing_member? + current_user.can?(:update_project_member, member) end - def adding_a_new_owner? + def adding_the_creator_as_owner_in_a_personal_project? # this condition is reached during testing setup a lot due to use of `.add_user` - member.owner? && member.new_record? + member.project.personal_namespace_holder?(member.user) end end end -- cgit v1.2.3