Commit be6ec6a5 authored by Jarka Kadlecová's avatar Jarka Kadlecová
Browse files

Delete todos when users loses target read permissions

parent 4e0b0760def9
......@@ -37,6 +37,8 @@ def handle_changes(issue, options)
end
if issue.previous_changes.include?('confidential')
# don't enqueue immediately to prevent todos removal in case of a mistake
TodosDestroyer::ConfidentialIssueWorker.perform_in(1.hour, issue.id) if issue.confidential?
create_confidentiality_note(issue)
end
......
......@@ -15,6 +15,8 @@ def execute(member, skip_authorization: false)
notification_service.decline_access_request(member)
end
enqeue_delete_todos(member)
after_execute(member: member)
member
......@@ -22,6 +24,12 @@ def execute(member, skip_authorization: false)
private
def enqeue_delete_todos(member)
type = member.is_a?(GroupMember) ? 'Group' : 'Project'
# don't enqueue immediately to prevent todos removal in case of a mistake
TodosDestroyer::EntityLeaveWorker.perform_in(1.hour, member.user_id, member.source_id, type)
end
def can_destroy_member?(member)
can?(current_user, destroy_member_permission(member), member)
end
......
......@@ -25,13 +25,7 @@ def execute
return validation_failed! if project.errors.any?
if project.update(params.except(:default_branch))
if project.previous_changes.include?('path')
project.rename_repo
else
system_hook_service.execute_hooks_for(project, :update)
end
update_pages_config if changing_pages_https_only?
after_update
success
else
......@@ -47,6 +41,21 @@ def run_auto_devops_pipeline?
private
def after_update
if project.previous_changes.include?(:visibility_level) && project.private?
# don't enqueue immediately to prevent todos removal in case of a mistake
TodosDestroyer::ProjectPrivateWorker.perform_in(1.hour, project.id)
end
if project.previous_changes.include?('path')
project.rename_repo
else
system_hook_service.execute_hooks_for(project, :update)
end
update_pages_config if changing_pages_https_only?
end
def validation_failed!
model_errors = project.errors.full_messages.to_sentence
error_message = model_errors.presence || 'Project could not be updated!'
......
module Todos
module Destroy
class BaseService
def execute
return unless todos_to_remove?
without_authorized(todos).delete_all
end
private
def without_authorized(items)
items.where('user_id NOT IN (?)', authorized_users)
end
def authorized_users
ProjectAuthorization.select(:user_id).where(project_id: project_ids)
end
def todos
# overridden in subclasses
end
def project_ids
# overridden in subclasses
end
def todos_to_remove?
# overridden in subclasses
end
end
end
end
module Todos
module Destroy
class ConfidentialIssueService < ::Todos::Destroy::BaseService
extend ::Gitlab::Utils::Override
attr_reader :issue
def initialize(issue_id)
@issue = Issue.find_by(id: issue_id)
end
private
override :todos
def todos
Todo.where(target: issue)
end
override :todos_to_remove?
def todos_to_remove?
issue&.confidential?
end
override :project_ids
def project_ids
issue.project_id
end
end
end
end
module Todos
module Destroy
class EntityLeaveService < ::Todos::Destroy::BaseService
extend ::Gitlab::Utils::Override
attr_reader :user_id, :entity
def initialize(user_id, entity_id, entity_type)
unless %w(Group Project).include?(entity_type)
raise ArgumentError.new("#{entity_type} is not an entity user can leave")
end
@user_id = user_id
@entity = entity_type.constantize.find_by(id: entity_id)
end
private
override :todos
def todos
if entity.private?
Todo.where(project_id: project_ids, user_id: user_id)
else
Todo.where(target_id: confidential_issues.select(:id), target_type: Issue)
end
end
override :project_ids
def project_ids
if entity.is_a?(Project)
entity.id
else
Project.select(:id).where(namespace_id: entity.self_and_descendants.select(:id))
end
end
override :todos_to_remove?
def todos_to_remove?
return unless entity
entity.private? || confidential_issues.count > 0
end
def confidential_issues
Issue.where(project_id: project_ids, confidential: true)
end
end
end
end
module Todos
module Destroy
class ProjectPrivateService < ::Todos::Destroy::BaseService
extend ::Gitlab::Utils::Override
attr_reader :project
def initialize(project_id)
@project = Project.find_by(id: project_id)
end
private
override :todos
def todos
Todo.where(project_id: project_ids)
end
override :project_ids
def project_ids
project.id
end
override :todos_to_remove?
def todos_to_remove?
project&.private?
end
end
end
end
......@@ -73,6 +73,10 @@
- repository_check:repository_check_batch
- repository_check:repository_check_single_repository
- todos_destroyer:todos_destroyer_confidential_issue
- todos_destroyer:todos_destroyer_entity_leave
- todos_destroyer:todos_destroyer_project_private
- default
- mailers # ActionMailer::DeliveryJob.queue_name
......
# frozen_string_literal: true
##
# Concern for setting Sidekiq settings for the various Todos Destroyers.
#
module TodosDestroyerQueue
extend ActiveSupport::Concern
included do
queue_namespace :todos_destroyer
end
end
module TodosDestroyer
class ConfidentialIssueWorker
include ApplicationWorker
include TodosDestroyerQueue
def perform(issue_id)
::Todos::Destroy::ConfidentialIssueService.new(issue_id).execute
end
end
end
module TodosDestroyer
class EntityLeaveWorker
include ApplicationWorker
include TodosDestroyerQueue
def perform(user_id, entity_id, entity_type)
::Todos::Destroy::EntityLeaveService.new(user_id, entity_id, entity_type).execute
end
end
end
module TodosDestroyer
class ProjectPrivateWorker
include ApplicationWorker
include TodosDestroyerQueue
def perform(project_id)
::Todos::Destroy::ProjectPrivateService.new(project_id).execute
end
end
end
---
title: Delete todos when user loses access to read the target
merge_request: 20665
author:
type: other
......@@ -45,6 +45,7 @@
- [github_import_advance_stage, 1]
- [project_service, 1]
- [delete_user, 1]
- [todos_destroyer, 1]
- [delete_merged_branches, 1]
- [authorized_projects, 1]
- [expire_build_instance_artifacts, 1]
......
......@@ -55,6 +55,8 @@ def update_issue(opts)
end
it 'updates the issue with the given params' do
expect(TodosDestroyer::ConfidentialIssueWorker).not_to receive(:perform_in)
update_issue(opts)
expect(issue).to be_valid
......@@ -74,6 +76,21 @@ def update_issue(opts)
.to change { project.open_issues_count }.from(1).to(0)
end
it 'enqueues ConfidentialIssueWorker when an issue is made confidential' do
expect(TodosDestroyer::ConfidentialIssueWorker).to receive(:perform_in).with(1.hour, issue.id)
update_issue(confidential: true)
end
it 'does not enqueue ConfidentialIssueWorker when an issue is made non confidential' do
# set confidentiality to true before the actual update
issue.update!(confidential: true)
expect(TodosDestroyer::ConfidentialIssueWorker).not_to receive(:perform_in)
update_issue(confidential: false)
end
it 'updates open issue counter for assignees when issue is reassigned' do
update_issue(assignee_ids: [user2.id])
......
......@@ -20,6 +20,11 @@
end
shared_examples 'a service destroying a member' do
before do
type = member.is_a?(GroupMember) ? 'Group' : 'Project'
expect(TodosDestroyer::EntityLeaveWorker).to receive(:perform_in).with(1.hour, member.user_id, member.source_id, type)
end
it 'destroys the member' do
expect { described_class.new(current_user).execute(member, opts) }.to change { member.source.members_and_requesters.count }.by(-1)
end
......
......@@ -15,6 +15,8 @@
context 'when changing visibility level' do
context 'when visibility_level is INTERNAL' do
it 'updates the project to internal' do
expect(TodosDestroyer::ProjectPrivateWorker).not_to receive(:perform_in)
result = update_project(project, user, visibility_level: Gitlab::VisibilityLevel::INTERNAL)
expect(result).to eq({ status: :success })
......@@ -24,12 +26,30 @@
context 'when visibility_level is PUBLIC' do
it 'updates the project to public' do
expect(TodosDestroyer::ProjectPrivateWorker).not_to receive(:perform_in)
result = update_project(project, user, visibility_level: Gitlab::VisibilityLevel::PUBLIC)
expect(result).to eq({ status: :success })
expect(project).to be_public
end
end
context 'when visibility_level is PRIVATE' do
before do
project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
end
it 'updates the project to private' do
expect(TodosDestroyer::ProjectPrivateWorker).to receive(:perform_in).with(1.hour, project.id)
result = update_project(project, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE)
expect(result).to eq({ status: :success })
expect(project).to be_private
end
end
context 'when visibility levels are restricted to PUBLIC only' do
before do
stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC])
......@@ -38,6 +58,7 @@
context 'when visibility_level is INTERNAL' do
it 'updates the project to internal' do
result = update_project(project, user, visibility_level: Gitlab::VisibilityLevel::INTERNAL)
expect(result).to eq({ status: :success })
expect(project).to be_internal
end
......@@ -54,6 +75,7 @@
context 'when updated by an admin' do
it 'updates the project to public' do
result = update_project(project, admin, visibility_level: Gitlab::VisibilityLevel::PUBLIC)
expect(result).to eq({ status: :success })
expect(project).to be_public
end
......
require 'spec_helper'
describe Todos::Destroy::ConfidentialIssueService do
let(:project) { create(:project, :public) }
let(:user) { create(:user) }
let(:project_member) { create(:user) }
let(:issue) { create(:issue, project: project) }
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_another_non_member) { create(:todo, user: user, project: project) }
describe '#execute' do
before do
project.add_developer(project_member)
end
subject { described_class.new(issue.id).execute }
context 'when provided issue is confidential' do
before do
issue.update!(confidential: true)
end
it 'removes issue todos for a user who is not a project member' do
expect { subject }.to change { Todo.count }.from(3).to(2)
expect(user.todos).to match_array([todo_another_non_member])
expect(project_member.todos).to match_array([todo_issue_member])
end
end
context 'when provided issue is not confidential' do
it 'does not remove any todos' do
expect { subject }.not_to change { Todo.count }
end
end
end
end
require 'spec_helper'
describe Todos::Destroy::EntityLeaveService do
let(:group) { create(:group, :private) }
let(:project) { create(:project, group: group) }
let(:user) { create(:user) }
let(:project_member) { create(:user) }
let(:issue) { create(:issue, :confidential, project: project) }
let!(:todo_non_member) { create(:todo, user: user, project: project) }
let!(:todo_conf_issue_non_member) { create(:todo, user: user, target: issue, project: project) }
let!(:todo_conf_issue_member) { create(:todo, user: project_member, target: issue, project: project) }
describe '#execute' do
before do
project.add_developer(project_member)
end
context 'when a user leaves a project' do
subject { described_class.new(user.id, project.id, 'Project').execute }
context 'when project is private' do
it 'removes todos for a user who is not a member' do
expect { subject }.to change { Todo.count }.from(3).to(1)
expect(user.todos).to be_empty
expect(project_member.todos).to match_array([todo_conf_issue_member])
end
end
context 'when project is not private' do
before do
group.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
end
it 'removes only confidential issues todos' do
expect { subject }.to change { Todo.count }.from(3).to(2)
end
end
end
context 'when a user leaves a group' do
subject { described_class.new(user.id, group.id, 'Group').execute }
context 'when group is private' do
it 'removes todos for a user who is not a member' do
expect { subject }.to change { Todo.count }.from(3).to(1)
expect(user.todos).to be_empty
expect(project_member.todos).to match_array([todo_conf_issue_member])
end
context 'with nested groups', :nested_groups do
let(:subgroup) { create(:group, :private, parent: group) }
let(:subproject) { create(:project, group: subgroup) }
let!(:todo_subproject_non_member) { create(:todo, user: user, project: subproject) }
let!(:todo_subproject_member) { create(:todo, user: project_member, project: subproject) }
it 'removes todos for a user who is not a member' do
expect { subject }.to change { Todo.count }.from(5).to(2)
expect(user.todos).to be_empty
expect(project_member.todos)
.to match_array([todo_conf_issue_member, todo_subproject_member])
end
end
end
context 'when group is not private' do
before do
group.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
end
it 'removes only confidential issues todos' do
expect { subject }.to change { Todo.count }.from(3).to(2)
end
end
end
context 'when entity type is not valid' do
it 'raises an exception' do
expect { described_class.new(user.id, group.id, 'GroupWrongly').execute }
.to raise_error(ArgumentError)
end
end
context 'when entity was not found' do
it 'does not remove any todos' do
expect { described_class.new(user.id, 999999, 'Group').execute }
.not_to change { Todo.count }
end
end
end
end
require 'spec_helper'
describe Todos::Destroy::ProjectPrivateService do
let(:project) { create(:project, :public) }
let(:user) { create(:user) }
let(:project_member) { create(:user) }
let!(:todo_issue_non_member) { create(:todo, user: user, project: project) }
let!(:todo_issue_member) { create(:todo, user: project_member, project: project) }
let!(:todo_another_non_member) { create(:todo, user: user, project: project) }
describe '#execute' do
before do
project.add_developer(project_member)
end
subject { described_class.new(project.id).execute }
context 'when a project set to private' do
before do
project.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
end
it 'removes issue todos for a user who is not a member' do
expect { subject }.to change { Todo.count }.from(3).to(1)
expect(user.todos).to be_empty
expect(project_member.todos).to match_array([todo_issue_member])
end
end
context 'when project is not private' do
it 'does not remove any todos' do
expect { subject }.not_to change { Todo.count }
end
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