Commit be339468 authored by Felipe Artur's avatar Felipe Artur

Delete unauthorized Todos when project is private

Delete Todos for guest users when project visibility
level is updated to private.
parent c9da4375
......@@ -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
......
......@@ -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)
......
......@@ -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
......
......@@ -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
......
......@@ -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
---
title: Delete unauthorized Todos when project is made private
merge_request: 28560
author:
type: fixed
......@@ -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)
......
......@@ -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
......
......@@ -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
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