From be339468192c656bf9de0bb77d7e487f338902bf Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Tue, 21 May 2019 16:20:27 -0300 Subject: Delete unauthorized Todos when project is private Delete Todos for guest users when project visibility level is updated to private. --- app/models/todo.rb | 3 ++ app/services/projects/update_service.rb | 1 + app/services/todos/destroy/base_service.rb | 2 +- .../todos/destroy/confidential_issue_service.rb | 35 ++++++++++--- .../todos_destroyer/confidential_issue_worker.rb | 4 +- changelogs/unreleased/issue_49897.yml | 5 ++ spec/services/projects/update_service_spec.rb | 1 + .../destroy/confidential_issue_service_spec.rb | 58 +++++++++++++++------- .../confidential_issue_worker_spec.rb | 13 +++-- 9 files changed, 91 insertions(+), 31 deletions(-) create mode 100644 changelogs/unreleased/issue_49897.yml diff --git a/app/models/todo.rb b/app/models/todo.rb index 5dcc3e9945a..f1fc5e599eb 100644 --- a/app/models/todo.rb +++ b/app/models/todo.rb @@ -38,7 +38,9 @@ class Todo < ApplicationRecord self end }, polymorphic: true, touch: true # rubocop:disable Cop/PolymorphicAssociations + belongs_to :user + belongs_to :issue, -> { where("target_type = 'Issue'") }, foreign_key: :target_id delegate :name, :email, to: :author, prefix: true, allow_nil: true @@ -59,6 +61,7 @@ class Todo < ApplicationRecord scope :for_target, -> (id) { where(target_id: id) } scope :for_commit, -> (id) { where(commit_id: id) } scope :with_api_entity_associations, -> { preload(:target, :author, :note, group: :route, project: [:route, { namespace: :route }]) } + scope :joins_issue_and_assignees, -> { left_joins(issue: :assignees) } state_machine :state, initial: :pending do event :done do diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb index dfa7bd20254..2bc04470342 100644 --- a/app/services/projects/update_service.rb +++ b/app/services/projects/update_service.rb @@ -64,6 +64,7 @@ module Projects if project.previous_changes.include?(:visibility_level) && project.private? # don't enqueue immediately to prevent todos removal in case of a mistake + TodosDestroyer::ConfidentialIssueWorker.perform_in(Todo::WAIT_FOR_DELETE, nil, project.id) TodosDestroyer::ProjectPrivateWorker.perform_in(Todo::WAIT_FOR_DELETE, project.id) elsif (project_changed_feature_keys & todos_features_changes).present? TodosDestroyer::PrivateFeaturesWorker.perform_in(Todo::WAIT_FOR_DELETE, project.id) diff --git a/app/services/todos/destroy/base_service.rb b/app/services/todos/destroy/base_service.rb index f3f1dbb5698..7378f10e7c4 100644 --- a/app/services/todos/destroy/base_service.rb +++ b/app/services/todos/destroy/base_service.rb @@ -13,7 +13,7 @@ module Todos # rubocop: disable CodeReuse/ActiveRecord def without_authorized(items) - items.where('user_id NOT IN (?)', authorized_users) + items.where('todos.user_id NOT IN (?)', authorized_users) end # rubocop: enable CodeReuse/ActiveRecord diff --git a/app/services/todos/destroy/confidential_issue_service.rb b/app/services/todos/destroy/confidential_issue_service.rb index 6276e332448..6cdd8c16894 100644 --- a/app/services/todos/destroy/confidential_issue_service.rb +++ b/app/services/todos/destroy/confidential_issue_service.rb @@ -2,36 +2,55 @@ module Todos module Destroy + # Service class for deleting todos that belongs to confidential issues. + # It deletes todos for users that are not at least reporters, issue author or assignee. + # + # Accepts issue_id or project_id as argument. + # When issue_id is passed it deletes matching todos for one confidential issue. + # When project_id is passed it deletes matching todos for all confidential issues of the project. class ConfidentialIssueService < ::Todos::Destroy::BaseService extend ::Gitlab::Utils::Override - attr_reader :issue + attr_reader :issues # rubocop: disable CodeReuse/ActiveRecord - def initialize(issue_id) - @issue = Issue.find_by(id: issue_id) + def initialize(issue_id: nil, project_id: nil) + @issues = + if issue_id + Issue.where(id: issue_id) + elsif project_id + project_confidential_issues(project_id) + end end # rubocop: enable CodeReuse/ActiveRecord private + def project_confidential_issues(project_id) + project = Project.find(project_id) + + project.issues.confidential_only + end + override :todos # rubocop: disable CodeReuse/ActiveRecord def todos - Todo.where(target: issue) - .where('user_id != ?', issue.author_id) - .where('user_id NOT IN (?)', issue.assignees.select(:id)) + Todo.joins_issue_and_assignees + .where(target: issues) + .where('issues.confidential = ?', true) + .where('todos.user_id != issues.author_id') + .where('todos.user_id != issue_assignees.user_id') end # rubocop: enable CodeReuse/ActiveRecord override :todos_to_remove? def todos_to_remove? - issue&.confidential? + issues&.any?(&:confidential?) end override :project_ids def project_ids - issue.project_id + issues&.distinct&.select(:project_id) end override :authorized_users diff --git a/app/workers/todos_destroyer/confidential_issue_worker.rb b/app/workers/todos_destroyer/confidential_issue_worker.rb index 481fde8c83d..240a5f98ad5 100644 --- a/app/workers/todos_destroyer/confidential_issue_worker.rb +++ b/app/workers/todos_destroyer/confidential_issue_worker.rb @@ -5,8 +5,8 @@ module TodosDestroyer include ApplicationWorker include TodosDestroyerQueue - def perform(issue_id) - ::Todos::Destroy::ConfidentialIssueService.new(issue_id).execute + def perform(issue_id = nil, project_id = nil) + ::Todos::Destroy::ConfidentialIssueService.new(issue_id: issue_id, project_id: project_id).execute end end end diff --git a/changelogs/unreleased/issue_49897.yml b/changelogs/unreleased/issue_49897.yml new file mode 100644 index 00000000000..b630b5143c6 --- /dev/null +++ b/changelogs/unreleased/issue_49897.yml @@ -0,0 +1,5 @@ +--- +title: Delete unauthorized Todos when project is made private +merge_request: 28560 +author: +type: fixed diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index 5ad30b58511..1dcfb739eb6 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -45,6 +45,7 @@ describe Projects::UpdateService do it 'updates the project to private' do expect(TodosDestroyer::ProjectPrivateWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, project.id) + expect(TodosDestroyer::ConfidentialIssueWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, nil, project.id) result = update_project(project, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE) diff --git a/spec/services/todos/destroy/confidential_issue_service_spec.rb b/spec/services/todos/destroy/confidential_issue_service_spec.rb index 78b6744b426..9f7e656f7d3 100644 --- a/spec/services/todos/destroy/confidential_issue_service_spec.rb +++ b/spec/services/todos/destroy/confidential_issue_service_spec.rb @@ -9,36 +9,60 @@ describe Todos::Destroy::ConfidentialIssueService do let(:assignee) { create(:user) } let(:guest) { create(:user) } let(:project_member) { create(:user) } - let(:issue) { create(:issue, project: project, author: author, assignees: [assignee]) } - - let!(:todo_issue_non_member) { create(:todo, user: user, target: issue, project: project) } - let!(:todo_issue_member) { create(:todo, user: project_member, target: issue, project: project) } - let!(:todo_issue_author) { create(:todo, user: author, target: issue, project: project) } - let!(:todo_issue_asignee) { create(:todo, user: assignee, target: issue, project: project) } - let!(:todo_issue_guest) { create(:todo, user: guest, target: issue, project: project) } - let!(:todo_another_non_member) { create(:todo, user: user, project: project) } + let(:issue_1) { create(:issue, :confidential, project: project, author: author, assignees: [assignee]) } describe '#execute' do before do project.add_developer(project_member) project.add_guest(guest) + + # todos not to be deleted + create(:todo, user: project_member, target: issue_1, project: project) + create(:todo, user: author, target: issue_1, project: project) + create(:todo, user: assignee, target: issue_1, project: project) + create(:todo, user: user, project: project) + # Todos to be deleted + create(:todo, user: guest, target: issue_1, project: project) + create(:todo, user: user, target: issue_1, project: project) end - subject { described_class.new(issue.id).execute } + subject { described_class.new(issue_id: issue_1.id).execute } - context 'when provided issue is confidential' do - before do - issue.update!(confidential: true) + context 'when issue_id parameter is present' do + context 'when provided issue is confidential' do + it 'removes issue todos for users who can not access the confidential issue' do + expect { subject }.to change { Todo.count }.from(6).to(4) + end end - it 'removes issue todos for users who can not access the confidential issue' do - expect { subject }.to change { Todo.count }.from(6).to(4) + context 'when provided issue is not confidential' do + it 'does not remove any todos' do + issue_1.update(confidential: false) + + expect { subject }.not_to change { Todo.count } + end end end - context 'when provided issue is not confidential' do - it 'does not remove any todos' do - expect { subject }.not_to change { Todo.count } + context 'when project_id parameter is present' do + subject { described_class.new(issue_id: nil, project_id: project.id).execute } + + it 'removes issues todos for users that cannot access confidential issues' do + issue_2 = create(:issue, :confidential, project: project) + issue_3 = create(:issue, :confidential, project: project, author: author, assignees: [assignee]) + issue_4 = create(:issue, project: project) + # Todos not to be deleted + create(:todo, user: guest, target: issue_1, project: project) + create(:todo, user: assignee, target: issue_1, project: project) + create(:todo, user: project_member, target: issue_2, project: project) + create(:todo, user: author, target: issue_3, project: project) + create(:todo, user: user, target: issue_4, project: project) + create(:todo, user: user, project: project) + # Todos to be deleted + create(:todo, user: user, target: issue_1, project: project) + create(:todo, user: guest, target: issue_2, project: project) + + expect { subject }.to change { Todo.count }.from(14).to(10) end end end diff --git a/spec/workers/todos_destroyer/confidential_issue_worker_spec.rb b/spec/workers/todos_destroyer/confidential_issue_worker_spec.rb index 18876b71615..0907e2768ba 100644 --- a/spec/workers/todos_destroyer/confidential_issue_worker_spec.rb +++ b/spec/workers/todos_destroyer/confidential_issue_worker_spec.rb @@ -3,12 +3,19 @@ require 'spec_helper' describe TodosDestroyer::ConfidentialIssueWorker do - it "calls the Todos::Destroy::ConfidentialIssueService with the params it was given" do - service = double + let(:service) { double } - expect(::Todos::Destroy::ConfidentialIssueService).to receive(:new).with(100).and_return(service) + it "calls the Todos::Destroy::ConfidentialIssueService with issue_id parameter" do + expect(::Todos::Destroy::ConfidentialIssueService).to receive(:new).with(issue_id: 100, project_id: nil).and_return(service) expect(service).to receive(:execute) described_class.new.perform(100) end + + it "calls the Todos::Destroy::ConfidentialIssueService with project_id parameter" do + expect(::Todos::Destroy::ConfidentialIssueService).to receive(:new).with(issue_id: nil, project_id: 100).and_return(service) + expect(service).to receive(:execute) + + described_class.new.perform(nil, 100) + end end -- cgit v1.2.3