summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/models/concerns/issuable.rb6
-rw-r--r--app/services/issuable_base_service.rb41
-rw-r--r--app/services/issues/base_service.rb4
-rw-r--r--app/services/merge_requests/base_service.rb4
-rw-r--r--changelogs/unreleased/issue_22664.yml2
-rw-r--r--spec/models/concerns/issuable_spec.rb29
-rw-r--r--spec/services/issuable/bulk_update_service_spec.rb12
-rw-r--r--spec/services/issues/create_service_spec.rb2
-rw-r--r--spec/services/issues/update_service_spec.rb11
-rw-r--r--spec/services/merge_requests/create_service_spec.rb2
-rw-r--r--spec/services/notes/slash_commands_service_spec.rb2
-rw-r--r--spec/support/services/issuable_create_service_shared_examples.rb52
-rw-r--r--spec/support/services/issuable_create_service_slash_commands_shared_examples.rb4
-rw-r--r--spec/support/services/issuable_update_service_shared_examples.rb52
14 files changed, 189 insertions, 34 deletions
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index ecbfb625c5e..5e63825bf99 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -93,9 +93,9 @@ module Issuable
def update_assignee_cache_counts
# 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.try(:update_cache_counts)
- assignee.try(:update_cache_counts)
+ previous_assignee = User.find_by_id(assignee_id_was) if assignee_id_was
+ previous_assignee.update_cache_counts if previous_assignee
+ assignee.update_cache_counts if assignee
end
# We want to use optimistic lock for cases when only title or description are involved
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index ab3d2a9a0cd..4ce5fd993d9 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -36,14 +36,10 @@ class IssuableBaseService < BaseService
end
end
- def filter_params(issuable_ability_name = :issue)
- filter_assignee
- filter_milestone
- filter_labels
+ def filter_params(issuable)
+ ability_name = :"admin_#{issuable.to_ability_name}"
- ability = :"admin_#{issuable_ability_name}"
-
- unless can?(current_user, ability, project)
+ unless can?(current_user, ability_name, project)
params.delete(:milestone_id)
params.delete(:labels)
params.delete(:add_label_ids)
@@ -52,14 +48,35 @@ class IssuableBaseService < BaseService
params.delete(:assignee_id)
params.delete(:due_date)
end
+
+ filter_assignee(issuable)
+ filter_milestone
+ filter_labels
end
- def filter_assignee
- if params[:assignee_id] == IssuableFinder::NONE
- params[:assignee_id] = ''
+ def filter_assignee(issuable)
+ return unless params[:assignee_id].present?
+
+ 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
+ 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
milestone_id = params[:milestone_id]
return unless milestone_id
@@ -138,7 +155,7 @@ class IssuableBaseService < BaseService
def create(issuable)
merge_slash_commands_into_params!(issuable)
- filter_params
+ filter_params(issuable)
params.delete(:state_event)
params[:author] ||= current_user
@@ -180,7 +197,7 @@ class IssuableBaseService < BaseService
change_state(issuable)
change_subscription(issuable)
change_todo(issuable)
- filter_params
+ filter_params(issuable)
old_labels = issuable.labels.to_a
old_mentioned_users = issuable.mentioned_users.to_a
diff --git a/app/services/issues/base_service.rb b/app/services/issues/base_service.rb
index 742e834df97..35af867a098 100644
--- a/app/services/issues/base_service.rb
+++ b/app/services/issues/base_service.rb
@@ -17,10 +17,6 @@ module Issues
private
- def filter_params
- super(:issue)
- end
-
def execute_hooks(issue, action = 'open')
issue_data = hook_data(issue, action)
hooks_scope = issue.confidential? ? :confidential_issue_hooks : :issue_hooks
diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb
index 800fd39c424..70e25956dc7 100644
--- a/app/services/merge_requests/base_service.rb
+++ b/app/services/merge_requests/base_service.rb
@@ -38,10 +38,6 @@ module MergeRequests
private
- def filter_params
- super(:merge_request)
- end
-
def merge_requests_for(branch)
origin_merge_requests = @project.origin_merge_requests
.opened.where(source_branch: branch).to_a
diff --git a/changelogs/unreleased/issue_22664.yml b/changelogs/unreleased/issue_22664.yml
index 28d3e74b1f8..18a8d9ec6be 100644
--- a/changelogs/unreleased/issue_22664.yml
+++ b/changelogs/unreleased/issue_22664.yml
@@ -1,4 +1,4 @@
---
-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:
author:
diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb
index 3cc96816cb0..1078c959419 100644
--- a/spec/models/concerns/issuable_spec.rb
+++ b/spec/models/concerns/issuable_spec.rb
@@ -44,21 +44,40 @@ describe Issue, "Issuable" do
it { expect(described_class).to respond_to(:assigned) }
end
- describe "after_save" do
+ describe "before_save" do
describe "#update_cache_counts" 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)
issue.update(assignee: user)
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
context "when previous assignee does not exist" do
- it "does not raise error" do
- issue.update(assignee_id: "")
+ before{ issue.update(assignee: nil) }
- 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
diff --git a/spec/services/issuable/bulk_update_service_spec.rb b/spec/services/issuable/bulk_update_service_spec.rb
index 5f3020b6525..0475f38fe5e 100644
--- a/spec/services/issuable/bulk_update_service_spec.rb
+++ b/spec/services/issuable/bulk_update_service_spec.rb
@@ -52,7 +52,10 @@ describe Issuable::BulkUpdateService, services: true do
context 'when the new assignee ID is a valid user' 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[:count]).to eq(1)
@@ -60,15 +63,16 @@ describe Issuable::BulkUpdateService, services: true do
it 'updates the assignee to the use ID passed' do
assignee = create(:user)
+ project.team << [assignee, :developer]
expect { bulk_update(issue, assignee_id: assignee.id) }
.to change { issue.reload.assignee }.from(user).to(assignee)
end
end
- context 'when the new assignee ID is -1' do
- it 'unassigns the issues' do
- expect { bulk_update(issue, assignee_id: -1) }
+ context "when the new assignee ID is #{IssuableFinder::NONE}" do
+ it "unassigns the issues" do
+ expect { bulk_update(issue, assignee_id: IssuableFinder::NONE) }
.to change { issue.reload.assignee }.to(nil)
end
end
diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb
index 8bde61ee336..ac3834c32ff 100644
--- a/spec/services/issues/create_service_spec.rb
+++ b/spec/services/issues/create_service_spec.rb
@@ -135,6 +135,8 @@ describe Issues::CreateService, services: true do
end
end
+ it_behaves_like 'issuable create service'
+
it_behaves_like 'new issuable record that supports slash commands'
context 'for a merge request' do
diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb
index eafbea46905..d83b09fd32c 100644
--- a/spec/services/issues/update_service_spec.rb
+++ b/spec/services/issues/update_service_spec.rb
@@ -142,6 +142,17 @@ describe Issues::UpdateService, services: true do
update_issue(confidential: true)
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
context 'todos' do
diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb
index b8142889075..673c0bd6c9c 100644
--- a/spec/services/merge_requests/create_service_spec.rb
+++ b/spec/services/merge_requests/create_service_spec.rb
@@ -84,6 +84,8 @@ describe MergeRequests::CreateService, services: true do
end
end
+ it_behaves_like 'issuable create service'
+
context 'while saving references to issues that the created merge request closes' do
let(:first_issue) { create(:issue, project: project) }
let(:second_issue) { create(:issue, project: project) }
diff --git a/spec/services/notes/slash_commands_service_spec.rb b/spec/services/notes/slash_commands_service_spec.rb
index d1099884a02..960b5cd5e6f 100644
--- a/spec/services/notes/slash_commands_service_spec.rb
+++ b/spec/services/notes/slash_commands_service_spec.rb
@@ -5,6 +5,8 @@ describe Notes::SlashCommandsService, services: true do
let(:project) { create(:empty_project) }
let(:master) { create(:user).tap { |u| project.team << [u, :master] } }
let(:assignee) { create(:user) }
+
+ before { project.team << [assignee, :master] }
end
shared_examples 'note on noteable that does not support slash commands' do
diff --git a/spec/support/services/issuable_create_service_shared_examples.rb b/spec/support/services/issuable_create_service_shared_examples.rb
new file mode 100644
index 00000000000..93c0267d2db
--- /dev/null
+++ b/spec/support/services/issuable_create_service_shared_examples.rb
@@ -0,0 +1,52 @@
+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
diff --git a/spec/support/services/issuable_create_service_slash_commands_shared_examples.rb b/spec/support/services/issuable_create_service_slash_commands_shared_examples.rb
index 5f9645ed44f..dd54b0addda 100644
--- a/spec/support/services/issuable_create_service_slash_commands_shared_examples.rb
+++ b/spec/support/services/issuable_create_service_slash_commands_shared_examples.rb
@@ -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(:issuable) { described_class.new(project, user, params).execute }
+ before { project.team << [assignee, :master ] }
+
context 'with labels in command only' do
let(:example_params) 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
let(:example_params) do
{
- assignee: build_stubbed(:user),
+ assignee: create(:user),
milestone_id: double(:milestone),
description: %(/assign @#{assignee.username}\n/milestone %"#{milestone.name}")
}
diff --git a/spec/support/services/issuable_update_service_shared_examples.rb b/spec/support/services/issuable_update_service_shared_examples.rb
index a3336755773..49cea1e608c 100644
--- a/spec/support/services/issuable_update_service_shared_examples.rb
+++ b/spec/support/services/issuable_update_service_shared_examples.rb
@@ -1,4 +1,8 @@
shared_examples 'issuable update service' do
+ def update_issuable(opts)
+ described_class.new(project, user, opts).execute(open_issuable)
+ end
+
context 'changing state' do
before { expect(project).to receive(:execute_hooks).once }
@@ -14,4 +18,52 @@ shared_examples 'issuable update service' do
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