diff options
| -rw-r--r-- | app/models/concerns/issuable.rb | 6 | ||||
| -rw-r--r-- | app/services/issuable_base_service.rb | 41 | ||||
| -rw-r--r-- | app/services/issues/base_service.rb | 4 | ||||
| -rw-r--r-- | app/services/merge_requests/base_service.rb | 4 | ||||
| -rw-r--r-- | changelogs/unreleased/issue_22664.yml | 2 | ||||
| -rw-r--r-- | spec/models/concerns/issuable_spec.rb | 29 | ||||
| -rw-r--r-- | spec/services/issuable/bulk_update_service_spec.rb | 12 | ||||
| -rw-r--r-- | spec/services/issues/create_service_spec.rb | 2 | ||||
| -rw-r--r-- | spec/services/issues/update_service_spec.rb | 11 | ||||
| -rw-r--r-- | spec/services/merge_requests/create_service_spec.rb | 2 | ||||
| -rw-r--r-- | spec/services/notes/slash_commands_service_spec.rb | 2 | ||||
| -rw-r--r-- | spec/support/services/issuable_create_service_shared_examples.rb | 52 | ||||
| -rw-r--r-- | spec/support/services/issuable_create_service_slash_commands_shared_examples.rb | 4 | ||||
| -rw-r--r-- | spec/support/services/issuable_update_service_shared_examples.rb | 52 |
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 |