Add more helpful error when not authorized to update

- currently it defaults to model validations, which isn't correct

Changelog: changed

.rubocop_todo/layout/line_length.yml

@@ -650,7 +650,6 @@ Layout/LineLength:
     - 'app/services/loose_foreign_keys/batch_cleaner_service.rb'
     - 'app/services/loose_foreign_keys/cleaner_service.rb'
     - 'app/services/members/approve_access_request_service.rb'
-    - 'app/services/members/create_service.rb'
     - 'app/services/members/destroy_service.rb'
     - 'app/services/members/invitation_reminder_email_service.rb'
     - 'app/services/members/mailgun/process_webhook_service.rb'

app/services/members/create_service.rb

@@ -16,6 +16,7 @@ module Members
       @errors = []
       @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,7 +42,8 @@ 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
       return params[:user_ids] if params[:user_ids].is_a?(Array)
@@ -76,13 +79,15 @@ 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
@@ -90,20 +95,29 @@ module Members
     end
 
     # overridden
-    def add_error_for_member(member)
+    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
@@ -117,17 +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?
-
-      # Only create task issues for existing users. Tasks for new users are created when they signup.
-      valid_members = members.select { |member| member.valid? && member.member_task.valid? && member.user.present? }
-      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

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
 

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

app/services/members/invite_service.rb

@@ -51,8 +51,8 @@ module Members
     end
 
     override :add_error_for_member
-    def add_error_for_member(member)
-      errors[invited_object(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
 
     def invited_object(member)

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_the_creator_as_owner_in_a_personal_project?
+      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_the_creator_as_owner_in_a_personal_project?
         # this condition is reached during testing setup a lot due to use of `.add_user`
-        member.project.personal_namespace_holder?(member.user) && member.new_record?
+        member.project.personal_namespace_holder?(member.user)
       end
     end
   end

locale/gitlab.pot

@@ -45317,6 +45317,12 @@ msgstr ""
 msgid "none"
 msgstr ""
 
+msgid "not authorized to create member"
+msgstr ""
+
+msgid "not authorized to update member"
+msgstr ""
+
 msgid "not found"
 msgstr ""
 

spec/services/members/create_service_spec.rb

@@ -143,6 +143,32 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_
     end
   end
 
+  context 'when adding a project_bot' do
+    let_it_be(:project_bot) { create(:user, :project_bot) }
+
+    let(:user_ids) { project_bot.id }
+
+    context 'when project_bot is already a member' do
+      before do
+        source.add_developer(project_bot)
+      end
+
+      it 'does not update the member' do
+        expect(execute_service[:status]).to eq(:error)
+        expect(execute_service[:message]).to eq("#{project_bot.username}: not authorized to update member")
+        expect(OnboardingProgress.completed?(source.namespace, :user_added)).to be(false)
+      end
+    end
+
+    context 'when project_bot is not already a member' do
+      it 'adds the member' do
+        expect(execute_service[:status]).to eq(:success)
+        expect(source.users).to include project_bot
+        expect(OnboardingProgress.completed?(source.namespace, :user_added)).to be(true)
+      end
+    end
+  end
+
   context 'when tracking the invite source', :snowplow do
     context 'when invite_source is not passed' do
       let(:additional_params) { {} }

spec/services/members/creator_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Members::CreatorService do
+  let_it_be(:source, reload: true) { create(:group, :public) }
+  let_it_be(:member_type) { GroupMember }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:current_user) { create(:user) }
+
+  describe '#execute' do
+    it 'raises error for new member on authorization check implementation' do
+      expect do
+        described_class.new(source, user, :maintainer, current_user: current_user).execute
+      end.to raise_error(NotImplementedError)
+    end
+
+    it 'raises error for an existing member on authorization check implementation' do
+      source.add_developer(user)
+
+      expect do
+        described_class.new(source, user, :maintainer, current_user: current_user).execute
+      end.to raise_error(NotImplementedError)
+    end
+  end
+end

spec/support/shared_examples/models/member_shared_examples.rb

@@ -88,19 +88,55 @@ RSpec.shared_examples_for "member creation" do
       expect(member).to be_persisted
     end
 
-    context 'when admin mode is enabled', :enable_admin_mode do
+    context 'when adding a project_bot' do
+      let_it_be(:project_bot) { create(:user, :project_bot) }
+
+      before_all do
+        source.add_owner(user)
+      end
+
+      context 'when project_bot is already a member' do
+        before do
+          source.add_developer(project_bot)
+        end
+
+        it 'does not update the member' do
+          member = described_class.new(source, project_bot, :maintainer, current_user: user).execute
+
+          expect(source.users.reload).to include(project_bot)
+          expect(member).to be_persisted
+          expect(member.access_level).to eq(Gitlab::Access::DEVELOPER)
+          expect(member.errors.full_messages).to include(/not authorized to update member/)
+        end
+      end
+
+      context 'when project_bot is not already a member' do
+        it 'adds the member' do
+          member = described_class.new(source, project_bot, :maintainer, current_user: user).execute
+
+          expect(source.users.reload).to include(project_bot)
+          expect(member).to be_persisted
+        end
+      end
+    end
+
+    context 'when admin mode is enabled', :enable_admin_mode, :aggregate_failures do
       it 'sets members.created_by to the given admin current_user' do
         member = described_class.new(source, user, :maintainer, current_user: admin).execute
 
+        expect(member).to be_persisted
+        expect(source.users.reload).to include(user)
         expect(member.created_by).to eq(admin)
       end
     end
 
     context 'when admin mode is disabled' do
-      it 'rejects setting members.created_by to the given admin current_user' do
+      it 'rejects setting members.created_by to the given admin current_user', :aggregate_failures do
         member = described_class.new(source, user, :maintainer, current_user: admin).execute
 
-        expect(member.created_by).to be_nil
+        expect(member).not_to be_persisted
+        expect(source.users.reload).not_to include(user)
+        expect(member.errors.full_messages).to include(/not authorized to create member/)
       end
     end
 
@@ -142,7 +178,7 @@ RSpec.shared_examples_for "member creation" do
       end
 
       context 'when called with an unknown user id' do
-        it 'adds the user as a member' do
+        it 'does not add the user as a member' do
           expect(source.users).not_to include(user)
 
           described_class.new(source, non_existing_record_id, :maintainer).execute