Merge branch 'limit-alert-assignees' into 'master'

Limit alert assignment to only users who can read alerts

See merge request gitlab-org/gitlab!34681

app/assets/javascripts/alert_management/components/sidebar/sidebar_assignees.vue

@@ -25,6 +25,9 @@ export default {
   UPDATE_ALERT_ASSIGNEES_ERROR: s__(
     'AlertManagement|There was an error while updating the assignee(s) of the alert. Please try again.',
   ),
+  UPDATE_ALERT_ASSIGNEES_GRAPHQL_ERROR: s__(
+    'AlertManagement|This assignee cannot be assigned to this alert.',
+  ),
   components: {
     GlIcon,
     GlDropdown,
@@ -156,9 +159,17 @@ export default {
             projectPath: this.projectPath,
           },
         })
-        .then(() => {
+        .then(({ data: { alertSetAssignees: { errors } = [] } = {} } = {}) => {
           this.hideDropdown();
-          this.$emit('alert-refresh');
+
+          if (errors[0]) {
+            return this.$emit(
+              'alert-sidebar-error',
+              `${this.$options.UPDATE_ALERT_ASSIGNEES_GRAPHQL_ERROR} ${errors[0]}.`,
+            );
+          }
+
+          return this.$emit('alert-refresh');
         })
         .catch(() => {
           this.$emit('alert-sidebar-error', this.$options.UPDATE_ALERT_ASSIGNEES_ERROR);

app/services/alert_management/alerts/update_service.rb

@@ -19,6 +19,8 @@ module AlertManagement
         return error_no_updates if params.empty?
 
         filter_assignees
+        return error_no_assignee_permissions if unauthorized_assignees?
+
         old_assignees = alert.assignees.to_a
 
         if alert.update(params)
@@ -38,10 +40,6 @@ module AlertManagement
         current_user&.can?(:update_alert_management_alert, alert)
       end
 
-      def assignee_todo_allowed?
-        assignee&.can?(:read_alert_management_alert, alert)
-      end
-
       def todo_service
         strong_memoize(:todo_service) do
           TodoService.new
@@ -64,18 +62,20 @@ module AlertManagement
         error(_('Please provide attributes to update'))
       end
 
+      def error_no_assignee_permissions
+        error(_('Assignee has no permissions'))
+      end
+
       # ----- Assignee-related behavior ------
+      def unauthorized_assignees?
+        params[:assignees]&.any? { |user| !user.can?(:read_alert_management_alert, alert) }
+      end
+
       def filter_assignees
         return if params[:assignees].nil?
 
-        params[:assignees] = Array(assignee)
-      end
-
-      def assignee
-        strong_memoize(:assignee) do
-          # Take first assignee while multiple are not currently supported
-          params[:assignees]&.first
-        end
+        # Always take first assignee while multiple are not currently supported
+        params[:assignees] = Array(params[:assignees].first)
       end
 
       def process_assignement(old_assignees)
@@ -84,8 +84,7 @@ module AlertManagement
       end
 
       def assign_todo
-        # Remove check in follow-up issue https://gitlab.com/gitlab-org/gitlab/-/issues/222672
-        return unless assignee_todo_allowed?
+        return if alert.assignees.empty?
 
         todo_service.assign_alert(alert, current_user)
       end

changelogs/unreleased/limit-alert-assignees.yml

+---
+title: Limit alert assignment to only users who can read alerts
+merge_request: 34681
+author:
+type: fixed

locale/gitlab.pot

@@ -2011,6 +2011,9 @@ msgstr ""
 msgid "AlertManagement|There was an error while updating the status of the alert. Please try again."
 msgstr ""
 
+msgid "AlertManagement|This assignee cannot be assigned to this alert."
+msgstr ""
+
 msgid "AlertManagement|Tool"
 msgstr ""
 
@@ -3020,6 +3023,9 @@ msgid_plural "%d Assignees"
 msgstr[0] ""
 msgstr[1] ""
 
+msgid "Assignee has no permissions"
+msgstr ""
+
 msgid "Assignee lists not available with your current license"
 msgstr ""
 

spec/frontend/alert_management/components/alert_managment_sidebar_assignees_spec.js

@@ -59,7 +59,7 @@ describe('Alert Details Sidebar Assignees', () => {
   describe('updating the alert status', () => {
     const mockUpdatedMutationResult = {
       data: {
-        updateAlertStatus: {
+        alertSetAssignees: {
           errors: [],
           alert: {
             assigneeUsernames: ['root'],
@@ -125,6 +125,26 @@ describe('Alert Details Sidebar Assignees', () => {
       });
     });
 
+    it('shows an error when request contains error messages', () => {
+      wrapper.setData({ isDropdownSearching: false });
+      const errorMutationResult = {
+        data: {
+          alertSetAssignees: {
+            errors: ['There was a problem for sure.'],
+            alert: {},
+          },
+        },
+      };
+
+      jest.spyOn(wrapper.vm.$apollo, 'mutate').mockResolvedValue(errorMutationResult);
+
+      return wrapper.vm.$nextTick().then(() => {
+        const SideBarAssigneeItem = wrapper.findAll(SidebarAssignee).at(0);
+        SideBarAssigneeItem.vm.$emit('click');
+        expect(wrapper.emitted('alert-refresh')).toBeUndefined();
+      });
+    });
+
     it('stops updating and cancels loading when the request fails', () => {
       jest.spyOn(wrapper.vm.$apollo, 'mutate').mockReturnValue(Promise.reject(new Error()));
       wrapper.vm.updateAlertAssignees('root');

spec/services/alert_management/alerts/update_service_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 
 describe AlertManagement::Alerts::UpdateService do
   let_it_be(:user_with_permissions) { create(:user) }
+  let_it_be(:other_user_with_permissions) { create(:user) }
   let_it_be(:user_without_permissions) { create(:user) }
   let_it_be(:alert, reload: true) { create(:alert_management_alert) }
   let_it_be(:project) { alert.project }
@@ -15,119 +16,131 @@ describe AlertManagement::Alerts::UpdateService do
 
   before_all do
     project.add_developer(user_with_permissions)
+    project.add_developer(other_user_with_permissions)
   end
 
   describe '#execute' do
+    shared_examples 'does not add a todo' do
+      specify { expect { response }.not_to change(Todo, :count) }
+    end
+
+    shared_examples 'does not add a system note' do
+      specify { expect { response }.not_to change(Note, :count) }
+    end
+
+    shared_examples 'error response' do |message|
+      it_behaves_like 'does not add a todo'
+      it_behaves_like 'does not add a system note'
+
+      it 'has an informative message' do
+        expect(response).to be_error
+        expect(response.message).to eq(message)
+      end
+    end
+
     subject(:response) { service.execute }
 
     context 'when the current_user is nil' do
       let(:current_user) { nil }
 
-      it 'results in an error' do
-        expect(response).to be_error
-        expect(response.message).to eq('You have no permissions')
-      end
+      it_behaves_like 'error response', 'You have no permissions'
     end
 
-    context 'when user does not have permission to update alerts' do
+    context 'when current_user does not have permission to update alerts' do
       let(:current_user) { user_without_permissions }
 
-      it 'results in an error' do
-        expect(response).to be_error
-        expect(response.message).to eq('You have no permissions')
-      end
+      it_behaves_like 'error response', 'You have no permissions'
     end
 
     context 'when no parameters are included' do
-      it 'results in an error' do
-        expect(response).to be_error
-        expect(response.message).to eq('Please provide attributes to update')
-      end
+      it_behaves_like 'error response', 'Please provide attributes to update'
     end
 
-    context 'when an error occures during update' do
+    context 'when an error occurs during update' do
       let(:params) { { title: nil } }
 
-      it 'results in an error' do
-        expect { response }.not_to change { alert.reload.notes.count }
-        expect(response).to be_error
-        expect(response.message).to eq("Title can't be blank")
-      end
+      it_behaves_like 'error response', "Title can't be blank"
     end
 
     context 'when a model attribute is included without assignees' do
       let(:params) { { title: 'This is an updated alert.' } }
 
+      it_behaves_like 'does not add a todo'
+      it_behaves_like 'does not add a system note'
+
       it 'updates the attribute' do
         original_title = alert.title
 
         expect { response }.to change { alert.title }.from(original_title).to(params[:title])
         expect(response).to be_success
       end
+    end
 
-      it 'skips adding a todo' do
-        expect { response }.not_to change(Todo, :count)
+    context 'when assignees are included' do
+      shared_examples 'adds a todo' do
+        let(:assignee) { expected_assignees.first }
+
+        specify do
+          expect { response }.to change { assignee.reload.todos.count }.by(1)
+          expect(assignee.todos.last.author).to eq(current_user)
         end
       end
 
-    context 'when assignees are included' do
-      let(:params) { { assignees: [user_with_permissions] } }
+      shared_examples 'adds a system note' do
+        specify { expect { response }.to change { alert.reload.notes.count }.by(1) }
+      end
+
+      shared_examples 'successful assignment' do
+        it_behaves_like 'adds a system note'
+        it_behaves_like 'adds a todo'
 
         after do
           alert.assignees = []
         end
 
-      it 'assigns the user' do
-        expect { response }.to change { alert.reload.assignees }.from([]).to(params[:assignees])
+        specify do
+          expect { response }.to change { alert.reload.assignees }.from([]).to(expected_assignees)
           expect(response).to be_success
         end
-
-      it 'creates a system note for the assignment' do
-        expect { response }.to change { alert.reload.notes.count }.by(1)
       end
 
-      it 'adds a todo' do
-        expect { response }.to change { Todo.where(user: user_with_permissions).count }.by(1)
-      end
+      let(:expected_assignees) { params[:assignees] }
 
-      context 'when current user is not the assignee' do
-        let(:assignee_user) { create(:user) }
-        let(:params) { { assignees: [assignee_user] } }
+      context 'when the assignee is the current user' do
+        let(:params) { { assignees: [current_user] } }
 
-        it 'skips adding todo for assignee without permission to read alert' do
-          expect { response }.not_to change(Todo, :count)
+        it_behaves_like 'successful assignment'
       end
 
-        context 'when assignee has read permission' do
-          before do
-            project.add_developer(assignee_user)
+      context 'when the assignee has read permissions' do
+        let(:params) { { assignees: [other_user_with_permissions] } }
+
+        it_behaves_like 'successful assignment'
       end
 
-          it 'adds a todo' do
-            response
+      context 'when the assignee does not have read permissions' do
+        let(:params) { { assignees: [user_without_permissions] } }
 
-            expect(Todo.first.author).to eq(current_user)
+        it_behaves_like 'error response', 'Assignee has no permissions'
       end
-        end
-
-        context 'when current_user is nil' do
-          let(:current_user) { nil }
 
-          it 'skips adding todo if current_user is nil' do
-            project.add_developer(assignee_user)
+      context 'when user is already assigned' do
+        let(:params) { { assignees: [user_with_permissions] } }
 
-            expect { response }.not_to change(Todo, :count)
-          end
+        before do
+          alert.assignees << user_with_permissions
         end
+
+        it_behaves_like 'does not add a system note'
+        # TODO: We should not add another todo in this scenario
+        it_behaves_like 'adds a todo'
       end
 
       context 'with multiple users included' do
         let(:params) { { assignees: [user_with_permissions, user_without_permissions] } }
+        let(:expected_assignees) { [user_with_permissions] }
 
-        it 'assigns the first permissioned user' do
-          expect { response }.to change { alert.reload.assignees }.from([]).to([user_with_permissions])
-          expect(response).to be_success
-        end
+        it_behaves_like 'successful assignment'
       end
     end
   end