Commit f2aa6c92 authored by Felipe Artur's avatar Felipe Artur

Return error when moving issues between not authorized lists

Return error when not authorized to move issues between
assignees lists on issue boards.

Changelog: fixed
parent 7946d895
...@@ -56,11 +56,11 @@ module Mutations ...@@ -56,11 +56,11 @@ module Mutations
issue = authorized_find!(project_path: project_path, iid: iid) issue = authorized_find!(project_path: project_path, iid: iid)
move_params = { id: issue.id, board_id: board.id }.merge(move_arguments(args)) move_params = { id: issue.id, board_id: board.id }.merge(move_arguments(args))
move_issue(board, issue, move_params) result = move_issue(board, issue, move_params)
{ {
issue: issue.reset, issue: issue.reset,
errors: issue.errors.full_messages errors: error_for(result)
} }
end end
...@@ -79,6 +79,12 @@ module Mutations ...@@ -79,6 +79,12 @@ module Mutations
def move_arguments(args) def move_arguments(args)
args.slice(:from_list_id, :to_list_id, :move_after_id, :move_before_id) args.slice(:from_list_id, :to_list_id, :move_after_id, :move_before_id)
end end
def error_for(result)
return [] unless result.error?
[result.message]
end
end end
end end
end end
......
...@@ -4,13 +4,23 @@ module Boards ...@@ -4,13 +4,23 @@ module Boards
class BaseItemMoveService < Boards::BaseService class BaseItemMoveService < Boards::BaseService
def execute(issuable) def execute(issuable)
issuable_modification_params = issuable_params(issuable) issuable_modification_params = issuable_params(issuable)
return false if issuable_modification_params.empty? return if issuable_modification_params.empty?
move_single_issuable(issuable, issuable_modification_params) return unless move_single_issuable(issuable, issuable_modification_params)
success(issuable)
end end
private private
def success(issuable)
ServiceResponse.success(payload: { issuable: issuable })
end
def error(issuable, message)
ServiceResponse.error(payload: { issuable: issuable }, message: message)
end
def issuable_params(issuable) def issuable_params(issuable)
attrs = {} attrs = {}
......
...@@ -20,11 +20,11 @@ module EE ...@@ -20,11 +20,11 @@ module EE
def move_issue(board, issue, move_params) def move_issue(board, issue, move_params)
super super
rescue ::Issues::BaseService::EpicAssignmentError => e rescue ::Issues::BaseService::EpicAssignmentError => e
issue.errors.add(:epic_issue, e.message) ServiceResponse.error(message: e.message)
rescue ::Gitlab::Access::AccessDeniedError rescue ::Gitlab::Access::AccessDeniedError
issue.errors.add(:base, 'You are not allowed to move the issue') ServiceResponse.error(message: 'You are not allowed to move the issue')
rescue ActiveRecord::RecordNotFound rescue ActiveRecord::RecordNotFound
issue.errors.add(:base, 'Resource not found') ServiceResponse.error(message: 'Resource not found')
end end
override :move_arguments override :move_arguments
......
...@@ -6,6 +6,15 @@ module EE ...@@ -6,6 +6,15 @@ module EE
module MoveService module MoveService
extend ::Gitlab::Utils::Override extend ::Gitlab::Utils::Override
override :execute
def execute(issue)
unless assignee_authorized_for?(issue)
return error(issue, 'Not authorized to assign issue to list user')
end
super
end
override :issuable_params override :issuable_params
def issuable_params(issue) def issuable_params(issue)
args = super args = super
...@@ -20,6 +29,12 @@ module EE ...@@ -20,6 +29,12 @@ module EE
args.merge(list_movement_args(issue)) args.merge(list_movement_args(issue))
end end
def assignee_authorized_for?(issue)
return true unless moving_to_list&.assignee?
Ability.allowed?(moving_to_list.user, :read_issue, issue)
end
def both_are_list_type?(type) def both_are_list_type?(type)
return false unless moving_from_list.list_type == type return false unless moving_from_list.list_type == type
......
...@@ -54,5 +54,19 @@ RSpec.describe Mutations::Boards::Issues::IssueMoveList do ...@@ -54,5 +54,19 @@ RSpec.describe Mutations::Boards::Issues::IssueMoveList do
expect(issue1.relative_position).to eq(3) expect(issue1.relative_position).to eq(3)
end end
end end
context 'when user cannot be assigned to issue' do
before do
stub_licensed_features(board_assignee_lists: true)
end
it 'returns error on result' do
params[:to_list_id] = create(:user_list, board: board, position: 2).id
result = mutation.resolve(**params)
expect(result[:errors]).to eq(['Not authorized to assign issue to list user'])
end
end
end end
end end
...@@ -259,6 +259,17 @@ RSpec.describe Boards::Issues::MoveService, services: true do ...@@ -259,6 +259,17 @@ RSpec.describe Boards::Issues::MoveService, services: true do
expect(issue.assignees).to contain_exactly(user) expect(issue.assignees).to contain_exactly(user)
expect(issue.milestone).to eq(milestone1) expect(issue.milestone).to eq(milestone1)
end end
context 'when cannot assign to target list user' do
it 'returns error' do
random_list = create(:user_list, board: board1, position: 2)
params = { board_id: board1.id, from_list_id: user_list1.id, to_list_id: random_list.id }
result = described_class.new(parent, user, params).execute(issue)
expect(result[:status]).to eq(:error)
end
end
end end
end end
......
...@@ -100,8 +100,8 @@ RSpec.shared_examples 'issues move service' do |group| ...@@ -100,8 +100,8 @@ RSpec.shared_examples 'issues move service' do |group|
create(:labeled_issue, project: project, labels: [bug, development], assignees: [assignee]) create(:labeled_issue, project: project, labels: [bug, development], assignees: [assignee])
end end
it 'returns false' do it 'returns nil' do
expect(described_class.new(parent, user, params).execute(issue)).to eq false expect(described_class.new(parent, user, params).execute(issue)).to be_nil
end end
it 'keeps issues labels' do it 'keeps issues labels' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment