Ignore group members in user quick actions

This ensures that:

- our error messages are helpful, pointing out exactly what is wrong
- we permit things like commas and 'and'
- we never return users the current user cannot read

We also test that we cannot read mass groups like `@all`.

app/services/quick_actions/interpret_service.rb

@@ -71,21 +71,19 @@ module QuickActions
 
     # rubocop: disable CodeReuse/ActiveRecord
     def extract_users(params)
-      return [] if params.nil?
-
-      args = params.split(' ').uniq
-      users = extract_references(params, :user)
-
-      if users.empty?
-        users =
-          if params.strip == 'me'
-            [current_user]
-          else
-            User.where(username: args)
-          end
-      end
+      return [] if params.blank?
+
+      args = params.split(/\s|,/).select(&:present?).uniq - ['and']
+      users = extract_references(args.reject { _1 == 'me' }.join(' '), :user)
+      users << current_user if args.include?('me')
+      users = User.where(username: args).to_a if users.empty?
+
+      users.select! { can?(:read_user_profile, _1) }
 
-      failed_parse(format(_("Failed to find users for '%{params}'"), params: params)) if users.size < args.size
+      usernames = users.map(&:username).to_set
+      missing = args.reject { |arg| arg == 'me' || usernames.include?(arg.delete_prefix('@')) }.map { "'#{_1}'" }
+
+      failed_parse(format(_("Failed to find users for %{missing}"), missing: missing.to_sentence)) if missing.present?
 
       users
     end
@@ -194,6 +192,10 @@ module QuickActions
         user: current_user
       )
     end
+
+    def can?(ability, object)
+      Ability.allowed?(current_user, ability, object)
+    end
   end
 end
 

ee/spec/services/ee/notes/quick_actions_service_spec.rb

@@ -273,8 +273,8 @@ RSpec.describe Notes::QuickActionsService do
 
     let(:multiline_assign_reviewer_text) do
       <<~HEREDOC
-      /assign_reviewer @#{user.username}
-      /assign_reviewer @#{reviewer.username})
+      /assign_reviewer #{user.to_reference}
+      /assign_reviewer #{reviewer.to_reference}
       HEREDOC
     end
 

ee/spec/services/quick_actions/interpret_service_spec.rb

@@ -142,12 +142,12 @@ RSpec.describe QuickActions::InterpretService do
             group_members.each { group.add_developer(_1) }
           end
 
-          it 'asssigns all group members' do
+          it 'ignores group members' do
             merge_request.update!(assignee_ids: [user.id])
 
             _, updates = service.execute(command, merge_request)
 
-            expect(updates[:assignee_ids]).to match_array([user.id] + group_members.map(&:id))
+            expect(updates[:assignee_ids]).to be_nil
           end
         end
 

locale/gitlab.pot

@@ -14921,7 +14921,7 @@ msgstr ""
 msgid "Failed to find import label for Jira import."
 msgstr ""
 
-msgid "Failed to find users for '%{params}'"
+msgid "Failed to find users for %{missing}"
 msgstr ""
 
 msgid "Failed to generate export, please try again later."

spec/services/quick_actions/interpret_service_spec.rb

@@ -11,13 +11,14 @@ RSpec.describe QuickActions::InterpretService do
   let_it_be(:developer2) { create(:user) }
   let_it_be(:developer3) { create(:user) }
   let_it_be_with_reload(:issue) { create(:issue, project: project) }
-  let(:milestone) { create(:milestone, project: project, title: '9.10') }
-  let(:commit) { create(:commit, project: project) }
   let_it_be(:inprogress) { create(:label, project: project, title: 'In Progress') }
   let_it_be(:helmchart) { create(:label, project: project, title: 'Helm Chart Registry') }
   let_it_be(:bug) { create(:label, project: project, title: 'Bug') }
 
-  let(:service) { described_class.new(project, developer) }
+  let(:milestone) { create(:milestone, project: project, title: '9.10') }
+  let(:commit) { create(:commit, project: project) }
+
+  subject(:service) { described_class.new(project, developer) }
 
   before_all do
     public_project.add_developer(developer)
@@ -970,6 +971,32 @@ RSpec.describe QuickActions::InterpretService do
         it_behaves_like 'assign_reviewer command'
       end
 
+      context 'with a private user' do
+        let(:ref) { create(:user, :unconfirmed).to_reference }
+        let(:content) { "/assign_reviewer #{ref}" }
+
+        it_behaves_like 'failed command', 'a parse error' do
+          let(:match_msg) { eq "Could not apply assign_reviewer command. Failed to find users for '#{ref}'." }
+        end
+      end
+
+      context 'with a private user, bare username' do
+        let(:ref) { create(:user, :unconfirmed).username }
+        let(:content) { "/assign_reviewer #{ref}" }
+
+        it_behaves_like 'failed command', 'a parse error' do
+          let(:match_msg) { eq "Could not apply assign_reviewer command. Failed to find users for '#{ref}'." }
+        end
+      end
+
+      context 'with @all' do
+        let(:content) { "/assign_reviewer @all" }
+
+        it_behaves_like 'failed command', 'a parse error' do
+          let(:match_msg) { eq "Could not apply assign_reviewer command. Failed to find users for '@all'." }
+        end
+      end
+
       context 'with an incorrect user' do
         let(:content) { '/assign_reviewer @abcd1234' }
 
@@ -997,11 +1024,10 @@ RSpec.describe QuickActions::InterpretService do
       end
 
       context 'with extra text' do
-        let(:arg) { "@#{developer.username} do it!" }
-        let(:content) { "/assign_reviewer #{arg}" }
+        let(:content) { "/assign_reviewer #{developer.to_reference} do it!" }
 
         it_behaves_like 'failed command', 'a parse error' do
-          let(:match_msg) { eq "Could not apply assign_reviewer command. Failed to find users for '#{arg}'." }
+          let(:match_msg) { eq "Could not apply assign_reviewer command. Failed to find users for 'do' and 'it!'." }
         end
       end
     end
@@ -2451,7 +2477,8 @@ RSpec.describe QuickActions::InterpretService do
         it 'tells us why we cannot do that' do
           _, explanations = service.explain(content, merge_request)
 
-          expect(explanations).to eq ["Problem with assign command: Failed to find users for '#{arg}'."]
+          expect(explanations)
+            .to contain_exactly "Problem with assign command: Failed to find users for 'to', 'this', and 'issue'."
         end
       end
     end