Commit 37913698 authored by Felipe Artur's avatar Felipe Artur Committed by Sean McGivern

Check if user can read issue before being assigned

parent 174655c4
...@@ -97,9 +97,9 @@ module Issuable ...@@ -97,9 +97,9 @@ module Issuable
def update_assignee_cache_counts def update_assignee_cache_counts
# make sure we flush the cache for both the old *and* new assignees(if they exist) # make sure we flush the cache for both the old *and* new assignees(if they exist)
previous_assignee = User.find_by_id(assignee_id_was) previous_assignee = User.find_by_id(assignee_id_was) if assignee_id_was
previous_assignee.try(:update_cache_counts) previous_assignee.update_cache_counts if previous_assignee
assignee.try(:update_cache_counts) assignee.update_cache_counts if assignee
end end
# We want to use optimistic lock for cases when only title or description are involved # We want to use optimistic lock for cases when only title or description are involved
......
...@@ -44,14 +44,10 @@ class IssuableBaseService < BaseService ...@@ -44,14 +44,10 @@ class IssuableBaseService < BaseService
SystemNoteService.change_time_spent(issuable, issuable.project, current_user) SystemNoteService.change_time_spent(issuable, issuable.project, current_user)
end end
def filter_params(issuable_ability_name = :issue) def filter_params(issuable)
filter_assignee ability_name = :"admin_#{issuable.to_ability_name}"
filter_milestone
filter_labels
ability = :"admin_#{issuable_ability_name}" unless can?(current_user, ability_name, project)
unless can?(current_user, ability, project)
params.delete(:milestone_id) params.delete(:milestone_id)
params.delete(:labels) params.delete(:labels)
params.delete(:add_label_ids) params.delete(:add_label_ids)
...@@ -60,14 +56,35 @@ class IssuableBaseService < BaseService ...@@ -60,14 +56,35 @@ class IssuableBaseService < BaseService
params.delete(:assignee_id) params.delete(:assignee_id)
params.delete(:due_date) params.delete(:due_date)
end end
filter_assignee(issuable)
filter_milestone
filter_labels
end end
def filter_assignee def filter_assignee(issuable)
if params[:assignee_id] == IssuableFinder::NONE return unless params[:assignee_id].present?
params[:assignee_id] = ''
assignee_id = params[:assignee_id]
if assignee_id.to_s == IssuableFinder::NONE
params[:assignee_id] = ""
else
params.delete(:assignee_id) unless assignee_can_read?(issuable, assignee_id)
end end
end end
def assignee_can_read?(issuable, assignee_id)
new_assignee = User.find_by_id(assignee_id)
return false unless new_assignee.present?
ability_name = :"read_#{issuable.to_ability_name}"
resource = issuable.persisted? ? issuable : project
can?(new_assignee, ability_name, resource)
end
def filter_milestone def filter_milestone
milestone_id = params[:milestone_id] milestone_id = params[:milestone_id]
return unless milestone_id return unless milestone_id
...@@ -146,7 +163,7 @@ class IssuableBaseService < BaseService ...@@ -146,7 +163,7 @@ class IssuableBaseService < BaseService
def create(issuable) def create(issuable)
merge_slash_commands_into_params!(issuable) merge_slash_commands_into_params!(issuable)
filter_params filter_params(issuable)
change_time_spent(issuable) change_time_spent(issuable)
params.delete(:state_event) params.delete(:state_event)
...@@ -190,7 +207,7 @@ class IssuableBaseService < BaseService ...@@ -190,7 +207,7 @@ class IssuableBaseService < BaseService
change_subscription(issuable) change_subscription(issuable)
change_todo(issuable) change_todo(issuable)
time_spent = change_time_spent(issuable) time_spent = change_time_spent(issuable)
filter_params filter_params(issuable)
old_labels = issuable.labels.to_a old_labels = issuable.labels.to_a
old_mentioned_users = issuable.mentioned_users.to_a old_mentioned_users = issuable.mentioned_users.to_a
......
...@@ -17,10 +17,6 @@ module Issues ...@@ -17,10 +17,6 @@ module Issues
private private
def filter_params
super(:issue)
end
def execute_hooks(issue, action = 'open') def execute_hooks(issue, action = 'open')
issue_data = hook_data(issue, action) issue_data = hook_data(issue, action)
hooks_scope = issue.confidential? ? :confidential_issue_hooks : :issue_hooks hooks_scope = issue.confidential? ? :confidential_issue_hooks : :issue_hooks
......
...@@ -38,10 +38,6 @@ module MergeRequests ...@@ -38,10 +38,6 @@ module MergeRequests
private private
def filter_params
super(:merge_request)
end
def merge_requests_for(branch) def merge_requests_for(branch)
origin_merge_requests = @project.origin_merge_requests origin_merge_requests = @project.origin_merge_requests
.opened.where(source_branch: branch).to_a .opened.where(source_branch: branch).to_a
......
--- ---
title: Fix issuable assignee update bug when previous assignee is null title: Check if user can read project before being assigned to issue
merge_request: merge_request:
author: author:
...@@ -44,21 +44,40 @@ describe Issue, "Issuable" do ...@@ -44,21 +44,40 @@ describe Issue, "Issuable" do
it { expect(described_class).to respond_to(:assigned) } it { expect(described_class).to respond_to(:assigned) }
end end
describe "after_save" do describe "before_save" do
describe "#update_cache_counts" do describe "#update_cache_counts" do
context "when previous assignee exists" do context "when previous assignee exists" do
it "user updates cache counts" do before do
assignee = create(:user)
issue.project.team << [assignee, :developer]
issue.update(assignee: assignee)
end
it "updates cache counts for new assignee" do
user = create(:user)
expect(user).to receive(:update_cache_counts) expect(user).to receive(:update_cache_counts)
issue.update(assignee: user) issue.update(assignee: user)
end end
it "updates cache counts for previous assignee" do
old_assignee = issue.assignee
allow(User).to receive(:find_by_id).with(old_assignee.id).and_return(old_assignee)
expect(old_assignee).to receive(:update_cache_counts)
issue.update(assignee: nil)
end
end end
context "when previous assignee does not exist" do context "when previous assignee does not exist" do
it "does not raise error" do before{ issue.update(assignee: nil) }
issue.update(assignee_id: "")
expect { issue.update(assignee_id: user) }.not_to raise_error it "updates cache count for the new assignee" do
expect_any_instance_of(User).to receive(:update_cache_counts)
issue.update(assignee: user)
end end
end end
end end
......
...@@ -52,7 +52,10 @@ describe Issuable::BulkUpdateService, services: true do ...@@ -52,7 +52,10 @@ describe Issuable::BulkUpdateService, services: true do
context 'when the new assignee ID is a valid user' do context 'when the new assignee ID is a valid user' do
it 'succeeds' do it 'succeeds' do
result = bulk_update(issue, assignee_id: create(:user).id) new_assignee = create(:user)
project.team << [new_assignee, :developer]
result = bulk_update(issue, assignee_id: new_assignee.id)
expect(result[:success]).to be_truthy expect(result[:success]).to be_truthy
expect(result[:count]).to eq(1) expect(result[:count]).to eq(1)
...@@ -60,15 +63,16 @@ describe Issuable::BulkUpdateService, services: true do ...@@ -60,15 +63,16 @@ describe Issuable::BulkUpdateService, services: true do
it 'updates the assignee to the use ID passed' do it 'updates the assignee to the use ID passed' do
assignee = create(:user) assignee = create(:user)
project.team << [assignee, :developer]
expect { bulk_update(issue, assignee_id: assignee.id) } expect { bulk_update(issue, assignee_id: assignee.id) }
.to change { issue.reload.assignee }.from(user).to(assignee) .to change { issue.reload.assignee }.from(user).to(assignee)
end end
end end
context 'when the new assignee ID is -1' do context "when the new assignee ID is #{IssuableFinder::NONE}" do
it 'unassigns the issues' do it "unassigns the issues" do
expect { bulk_update(issue, assignee_id: -1) } expect { bulk_update(issue, assignee_id: IssuableFinder::NONE) }
.to change { issue.reload.assignee }.to(nil) .to change { issue.reload.assignee }.to(nil)
end end
end end
......
...@@ -135,6 +135,8 @@ describe Issues::CreateService, services: true do ...@@ -135,6 +135,8 @@ describe Issues::CreateService, services: true do
end end
end end
it_behaves_like 'issuable create service'
it_behaves_like 'new issuable record that supports slash commands' it_behaves_like 'new issuable record that supports slash commands'
context 'for a merge request' do context 'for a merge request' do
......
...@@ -142,6 +142,17 @@ describe Issues::UpdateService, services: true do ...@@ -142,6 +142,17 @@ describe Issues::UpdateService, services: true do
update_issue(confidential: true) update_issue(confidential: true)
end end
it 'does not update assignee_id with unauthorized users' do
project.update(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
update_issue(confidential: true)
non_member = create(:user)
original_assignee = issue.assignee
update_issue(assignee_id: non_member.id)
expect(issue.reload.assignee_id).to eq(original_assignee.id)
end
end end
context 'todos' do context 'todos' do
......
...@@ -84,6 +84,8 @@ describe MergeRequests::CreateService, services: true do ...@@ -84,6 +84,8 @@ describe MergeRequests::CreateService, services: true do
end end
end end
it_behaves_like 'issuable create service'
context 'while saving references to issues that the created merge request closes' do context 'while saving references to issues that the created merge request closes' do
let(:first_issue) { create(:issue, project: project) } let(:first_issue) { create(:issue, project: project) }
let(:second_issue) { create(:issue, project: project) } let(:second_issue) { create(:issue, project: project) }
......
...@@ -5,6 +5,8 @@ describe Notes::SlashCommandsService, services: true do ...@@ -5,6 +5,8 @@ describe Notes::SlashCommandsService, services: true do
let(:project) { create(:empty_project) } let(:project) { create(:empty_project) }
let(:master) { create(:user).tap { |u| project.team << [u, :master] } } let(:master) { create(:user).tap { |u| project.team << [u, :master] } }
let(:assignee) { create(:user) } let(:assignee) { create(:user) }
before { project.team << [assignee, :master] }
end end
shared_examples 'note on noteable that does not support slash commands' do shared_examples 'note on noteable that does not support slash commands' do
......
shared_examples 'issuable create service' do
context 'asssignee_id' do
let(:assignee) { create(:user) }
before { project.team << [user, :master] }
it 'removes assignee_id when user id is invalid' do
opts = { title: 'Title', description: 'Description', assignee_id: -1 }
issuable = described_class.new(project, user, opts).execute
expect(issuable.assignee_id).to be_nil
end
it 'removes assignee_id when user id is 0' do
opts = { title: 'Title', description: 'Description', assignee_id: 0 }
issuable = described_class.new(project, user, opts).execute
expect(issuable.assignee_id).to be_nil
end
it 'saves assignee when user id is valid' do
project.team << [assignee, :master]
opts = { title: 'Title', description: 'Description', assignee_id: assignee.id }
issuable = described_class.new(project, user, opts).execute
expect(issuable.assignee_id).to eq(assignee.id)
end
context "when issuable feature is private" do
before do
project.project_feature.update(issues_access_level: ProjectFeature::PRIVATE)
project.project_feature.update(merge_requests_access_level: ProjectFeature::PRIVATE)
end
levels = [Gitlab::VisibilityLevel::INTERNAL, Gitlab::VisibilityLevel::PUBLIC]
levels.each do |level|
it "removes not authorized assignee when project is #{Gitlab::VisibilityLevel.level_name(level)}" do
project.update(visibility_level: level)
opts = { title: 'Title', description: 'Description', assignee_id: assignee.id }
issuable = described_class.new(project, user, opts).execute
expect(issuable.assignee_id).to be_nil
end
end
end
end
end
...@@ -11,6 +11,8 @@ shared_examples 'new issuable record that supports slash commands' do ...@@ -11,6 +11,8 @@ shared_examples 'new issuable record that supports slash commands' do
let(:params) { base_params.merge(defined?(default_params) ? default_params : {}).merge(example_params) } let(:params) { base_params.merge(defined?(default_params) ? default_params : {}).merge(example_params) }
let(:issuable) { described_class.new(project, user, params).execute } let(:issuable) { described_class.new(project, user, params).execute }
before { project.team << [assignee, :master ] }
context 'with labels in command only' do context 'with labels in command only' do
let(:example_params) do let(:example_params) do
{ {
...@@ -55,7 +57,7 @@ shared_examples 'new issuable record that supports slash commands' do ...@@ -55,7 +57,7 @@ shared_examples 'new issuable record that supports slash commands' do
context 'with assignee and milestone in params and command' do context 'with assignee and milestone in params and command' do
let(:example_params) do let(:example_params) do
{ {
assignee: build_stubbed(:user), assignee: create(:user),
milestone_id: double(:milestone), milestone_id: double(:milestone),
description: %(/assign @#{assignee.username}\n/milestone %"#{milestone.name}") description: %(/assign @#{assignee.username}\n/milestone %"#{milestone.name}")
} }
......
shared_examples 'issuable update service' do shared_examples 'issuable update service' do
def update_issuable(opts)
described_class.new(project, user, opts).execute(open_issuable)
end
context 'changing state' do context 'changing state' do
before { expect(project).to receive(:execute_hooks).once } before { expect(project).to receive(:execute_hooks).once }
...@@ -14,4 +18,52 @@ shared_examples 'issuable update service' do ...@@ -14,4 +18,52 @@ shared_examples 'issuable update service' do
end end
end end
end end
context 'asssignee_id' do
it 'does not update assignee when assignee_id is invalid' do
open_issuable.update(assignee_id: user.id)
update_issuable(assignee_id: -1)
expect(open_issuable.reload.assignee).to eq(user)
end
it 'unassigns assignee when user id is 0' do
open_issuable.update(assignee_id: user.id)
update_issuable(assignee_id: 0)
expect(open_issuable.assignee_id).to be_nil
end
it 'saves assignee when user id is valid' do
update_issuable(assignee_id: user.id)
expect(open_issuable.assignee_id).to eq(user.id)
end
it 'does not update assignee_id when user cannot read issue' do
non_member = create(:user)
original_assignee = open_issuable.assignee
update_issuable(assignee_id: non_member.id)
expect(open_issuable.assignee_id).to eq(original_assignee.id)
end
context "when issuable feature is private" do
levels = [Gitlab::VisibilityLevel::INTERNAL, Gitlab::VisibilityLevel::PUBLIC]
levels.each do |level|
it "does not update with unauthorized assignee when project is #{Gitlab::VisibilityLevel.level_name(level)}" do
assignee = create(:user)
project.update(visibility_level: level)
feature_visibility_attr = :"#{open_issuable.model_name.plural}_access_level"
project.project_feature.update_attribute(feature_visibility_attr, ProjectFeature::PRIVATE)
expect{ update_issuable(assignee_id: assignee) }.not_to change{ open_issuable.assignee }
end
end
end
end
end end
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