Commit ba2240ea authored by Robert Speicher's avatar Robert Speicher

Merge branch 'issue_49897' into 'master'

Delete unauthorized Todos when project is private

Closes #49897

See merge request gitlab-org/gitlab-ce!28560
parents 42b75038 be339468
...@@ -38,7 +38,9 @@ class Todo < ApplicationRecord ...@@ -38,7 +38,9 @@ class Todo < ApplicationRecord
self self
end end
}, polymorphic: true, touch: true # rubocop:disable Cop/PolymorphicAssociations }, polymorphic: true, touch: true # rubocop:disable Cop/PolymorphicAssociations
belongs_to :user belongs_to :user
belongs_to :issue, -> { where("target_type = 'Issue'") }, foreign_key: :target_id
delegate :name, :email, to: :author, prefix: true, allow_nil: true delegate :name, :email, to: :author, prefix: true, allow_nil: true
...@@ -59,6 +61,7 @@ class Todo < ApplicationRecord ...@@ -59,6 +61,7 @@ class Todo < ApplicationRecord
scope :for_target, -> (id) { where(target_id: id) } scope :for_target, -> (id) { where(target_id: id) }
scope :for_commit, -> (id) { where(commit_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 :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 state_machine :state, initial: :pending do
event :done do event :done do
......
...@@ -64,6 +64,7 @@ module Projects ...@@ -64,6 +64,7 @@ module Projects
if project.previous_changes.include?(:visibility_level) && project.private? if project.previous_changes.include?(:visibility_level) && project.private?
# don't enqueue immediately to prevent todos removal in case of a mistake # 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) TodosDestroyer::ProjectPrivateWorker.perform_in(Todo::WAIT_FOR_DELETE, project.id)
elsif (project_changed_feature_keys & todos_features_changes).present? elsif (project_changed_feature_keys & todos_features_changes).present?
TodosDestroyer::PrivateFeaturesWorker.perform_in(Todo::WAIT_FOR_DELETE, project.id) TodosDestroyer::PrivateFeaturesWorker.perform_in(Todo::WAIT_FOR_DELETE, project.id)
......
...@@ -13,7 +13,7 @@ module Todos ...@@ -13,7 +13,7 @@ module Todos
# rubocop: disable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord
def without_authorized(items) def without_authorized(items)
items.where('user_id NOT IN (?)', authorized_users) items.where('todos.user_id NOT IN (?)', authorized_users)
end end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
......
...@@ -2,36 +2,55 @@ ...@@ -2,36 +2,55 @@
module Todos module Todos
module Destroy 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 class ConfidentialIssueService < ::Todos::Destroy::BaseService
extend ::Gitlab::Utils::Override extend ::Gitlab::Utils::Override
attr_reader :issue attr_reader :issues
# rubocop: disable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord
def initialize(issue_id) def initialize(issue_id: nil, project_id: nil)
@issue = Issue.find_by(id: issue_id) @issues =
if issue_id
Issue.where(id: issue_id)
elsif project_id
project_confidential_issues(project_id)
end
end end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
private private
def project_confidential_issues(project_id)
project = Project.find(project_id)
project.issues.confidential_only
end
override :todos override :todos
# rubocop: disable CodeReuse/ActiveRecord # rubocop: disable CodeReuse/ActiveRecord
def todos def todos
Todo.where(target: issue) Todo.joins_issue_and_assignees
.where('user_id != ?', issue.author_id) .where(target: issues)
.where('user_id NOT IN (?)', issue.assignees.select(:id)) .where('issues.confidential = ?', true)
.where('todos.user_id != issues.author_id')
.where('todos.user_id != issue_assignees.user_id')
end end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
override :todos_to_remove? override :todos_to_remove?
def todos_to_remove? def todos_to_remove?
issue&.confidential? issues&.any?(&:confidential?)
end end
override :project_ids override :project_ids
def project_ids def project_ids
issue.project_id issues&.distinct&.select(:project_id)
end end
override :authorized_users override :authorized_users
......
...@@ -5,8 +5,8 @@ module TodosDestroyer ...@@ -5,8 +5,8 @@ module TodosDestroyer
include ApplicationWorker include ApplicationWorker
include TodosDestroyerQueue include TodosDestroyerQueue
def perform(issue_id) def perform(issue_id = nil, project_id = nil)
::Todos::Destroy::ConfidentialIssueService.new(issue_id).execute ::Todos::Destroy::ConfidentialIssueService.new(issue_id: issue_id, project_id: project_id).execute
end end
end end
end end
---
title: Delete unauthorized Todos when project is made private
merge_request: 28560
author:
type: fixed
...@@ -45,6 +45,7 @@ describe Projects::UpdateService do ...@@ -45,6 +45,7 @@ describe Projects::UpdateService do
it 'updates the project to private' do it 'updates the project to private' do
expect(TodosDestroyer::ProjectPrivateWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, project.id) 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) result = update_project(project, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE)
......
...@@ -9,28 +9,27 @@ describe Todos::Destroy::ConfidentialIssueService do ...@@ -9,28 +9,27 @@ describe Todos::Destroy::ConfidentialIssueService do
let(:assignee) { create(:user) } let(:assignee) { create(:user) }
let(:guest) { create(:user) } let(:guest) { create(:user) }
let(:project_member) { create(:user) } let(:project_member) { create(:user) }
let(:issue) { create(:issue, project: project, author: author, assignees: [assignee]) } let(:issue_1) { create(:issue, :confidential, 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) }
describe '#execute' do describe '#execute' do
before do before do
project.add_developer(project_member) project.add_developer(project_member)
project.add_guest(guest) 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 end
subject { described_class.new(issue.id).execute } subject { described_class.new(issue_id: issue_1.id).execute }
context 'when issue_id parameter is present' do
context 'when provided issue is confidential' do context 'when provided issue is confidential' do
before do
issue.update!(confidential: true)
end
it 'removes issue todos for users who can not access the confidential issue' 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) expect { subject }.to change { Todo.count }.from(6).to(4)
end end
...@@ -38,8 +37,33 @@ describe Todos::Destroy::ConfidentialIssueService do ...@@ -38,8 +37,33 @@ describe Todos::Destroy::ConfidentialIssueService do
context 'when provided issue is not confidential' do context 'when provided issue is not confidential' do
it 'does not remove any todos' do it 'does not remove any todos' do
issue_1.update(confidential: false)
expect { subject }.not_to change { Todo.count } expect { subject }.not_to change { Todo.count }
end end
end end
end end
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
end end
...@@ -3,12 +3,19 @@ ...@@ -3,12 +3,19 @@
require 'spec_helper' require 'spec_helper'
describe TodosDestroyer::ConfidentialIssueWorker do describe TodosDestroyer::ConfidentialIssueWorker do
it "calls the Todos::Destroy::ConfidentialIssueService with the params it was given" do let(:service) { double }
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) expect(service).to receive(:execute)
described_class.new.perform(100) described_class.new.perform(100)
end 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 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