Skip to content
Snippets Groups Projects
Commit f74fab12 authored by Felipe Cardozo's avatar Felipe Cardozo :three: Committed by GitLab Release Tools Bot
Browse files

Remove todos from confidential notes when user loses access

Merge branch 'security-remove_non_readable_todos' into 'master'

See merge request gitlab-org/security/gitlab!2573

Changelog: security
parent d2c11b9b
No related branches found
No related tags found
No related merge requests found
......@@ -74,6 +74,7 @@ class Todo < ApplicationRecord
scope :for_commit, -> (id) { where(commit_id: id) }
scope :with_entity_associations, -> { preload(:target, :author, :note, group: :route, project: [:route, { namespace: [:route, :owner] }]) }
scope :joins_issue_and_assignees, -> { left_joins(issue: :assignees) }
scope :for_internal_notes, -> { joins(:note).where(note: { confidential: true }) }
enum resolved_by_action: { system_done: 0, api_all_done: 1, api_done: 2, mark_all_done: 3, mark_done: 4 }, _prefix: :resolved_by
......
......@@ -41,11 +41,20 @@ def enqueue_private_features_worker
end
def remove_confidential_resource_todos
# Deletes todos for confidential issues
Todo
.for_target(confidential_issues.select(:id))
.for_type(Issue.name)
.for_user(user)
.delete_all
# Deletes todos for internal notes on unauthorized projects
Todo
.for_type(Issue.name)
.for_internal_notes
.for_project(non_authorized_reporter_projects) # Only Reporter+ can read internal notes
.for_user(user)
.delete_all
end
def remove_project_todos
......
# frozen_string_literal: true
class AddIndexWithTargetTypeToTodos < Gitlab::Database::Migration[2.0]
disable_ddl_transaction!
INDEX_FOR_PROJECTS_NAME = 'index_requirements_project_id_user_id_id_and_target_type'
INDEX_FOR_TARGET_TYPE_NAME = 'index_requirements_user_id_and_target_type'
def up
add_concurrent_index :todos, [:project_id, :user_id, :id, :target_type], name: INDEX_FOR_PROJECTS_NAME
add_concurrent_index :todos, [:user_id, :target_type], name: INDEX_FOR_TARGET_TYPE_NAME
end
def down
remove_concurrent_index_by_name :todos, INDEX_FOR_PROJECTS_NAME
remove_concurrent_index_by_name :todos, INDEX_FOR_TARGET_TYPE_NAME
end
end
# frozen_string_literal: true
class RemoveDeprecatedIndexesFromTodos < Gitlab::Database::Migration[2.0]
disable_ddl_transaction!
PROJECTS_INDEX = 'index_todos_on_project_id_and_user_id_and_id'
USERS_INDEX = 'index_todos_on_user_id'
# These indexes are deprecated in favor of two new ones
# added in a previous migration:
#
# * index_requirements_project_id_user_id_target_type_and_id
# * index_requirements_user_id_and_target_type
def up
remove_concurrent_index_by_name :todos, PROJECTS_INDEX
remove_concurrent_index_by_name :todos, USERS_INDEX
end
def down
add_concurrent_index :todos, [:project_id, :user_id, :id], name: PROJECTS_INDEX
add_concurrent_index :todos, :user_id, name: USERS_INDEX
end
end
f6638435457f57f5c566e107de4f4557a1d87b5dd27acc9e5345999197d18e6e
\ No newline at end of file
ff9ad44a43be82867da8e0f51e68a2284065cab6b2eb7cf6496108dce1cdd657
\ No newline at end of file
......@@ -29506,6 +29506,10 @@ CREATE INDEX index_requirements_on_title_trigram ON requirements USING gin (titl
 
CREATE INDEX index_requirements_on_updated_at ON requirements USING btree (updated_at);
 
CREATE INDEX index_requirements_project_id_user_id_id_and_target_type ON todos USING btree (project_id, user_id, id, target_type);
CREATE INDEX index_requirements_user_id_and_target_type ON todos USING btree (user_id, target_type);
CREATE INDEX index_resource_iteration_events_on_issue_id ON resource_iteration_events USING btree (issue_id);
 
CREATE INDEX index_resource_iteration_events_on_iteration_id ON resource_iteration_events USING btree (iteration_id);
......@@ -29824,12 +29828,8 @@ CREATE INDEX index_todos_on_note_id ON todos USING btree (note_id);
 
CREATE INDEX index_todos_on_project_id_and_id ON todos USING btree (project_id, id);
 
CREATE INDEX index_todos_on_project_id_and_user_id_and_id ON todos USING btree (project_id, user_id, id);
CREATE INDEX index_todos_on_target_type_and_target_id ON todos USING btree (target_type, target_id);
 
CREATE INDEX index_todos_on_user_id ON todos USING btree (user_id);
CREATE INDEX index_todos_on_user_id_and_id_done ON todos USING btree (user_id, id) WHERE ((state)::text = 'done'::text);
 
CREATE INDEX index_todos_on_user_id_and_id_pending ON todos USING btree (user_id, id) WHERE ((state)::text = 'pending'::text);
......@@ -13,11 +13,20 @@ def remove_confidential_resource_todos
return unless entity.is_a?(Namespace)
# Deletes todos for confidential epics
::Todo
.for_target(confidential_epics.select(:id))
.for_type(::Epic.name)
.for_user(user)
.delete_all
# Deletes todos for internal notes on unauthorized groups
::Todo
.for_type(::Epic.name)
.for_internal_notes
.for_group(non_authorized_reporter_groups) # Only reporter+ can read internal notes
.for_user(user)
.delete_all
end
private
......
......@@ -12,18 +12,22 @@
let!(:todo1) { create(:todo, target: epic1, user: user, group: subgroup) }
let!(:todo2) { create(:todo, target: epic2, user: user, group: subgroup) }
let(:internal_note) { create(:note, noteable: epic2, confidential: true ) }
let!(:todo_for_internal_note) do
create(:todo, user: user, target: epic2, group: subgroup, note: internal_note)
end
describe '#execute' do
subject { described_class.new(user.id, subgroup.id, 'Group').execute }
shared_examples 'removes only confidential epics todos' do
it 'removes todos targeting confidential epics in the group' do
expect { subject }.to change { Todo.count }.by(-1)
shared_examples 'removes confidential epics and internal notes todos' do
it 'removes todos targeting confidential epics and internal notes in the group' do
expect { subject }.to change { Todo.count }.by(-2)
expect(user.reload.todos.ids).to match_array(todo2.id)
end
end
it_behaves_like 'removes only confidential epics todos'
it_behaves_like 'removes confidential epics and internal notes todos'
context 'when user is still member of ancestor group' do
before do
......@@ -57,7 +61,7 @@
subgroup.add_guest(user)
end
it_behaves_like 'removes only confidential epics todos'
it_behaves_like 'removes confidential epics and internal notes todos'
end
end
end
......@@ -495,4 +495,14 @@
it { is_expected.to contain_exactly(user1.id, user2.id) }
end
describe '.for_internal_notes' do
it 'returns todos created from internal notes' do
internal_note = create(:note, confidential: true )
todo = create(:todo, note: internal_note)
create(:todo)
expect(described_class.for_internal_notes).to contain_exactly(todo)
end
end
end
......@@ -3,21 +3,24 @@
require 'spec_helper'
RSpec.describe Todos::Destroy::EntityLeaveService do
let_it_be(:user, reload: true) { create(:user) }
let_it_be(:user2, reload: true) { create(:user) }
let(:group) { create(:group, :private) }
let(:project) { create(:project, :private, group: group) }
let(:issue) { create(:issue, project: project) }
let(:issue_c) { create(:issue, project: project, confidential: true) }
let!(:todo_group_user) { create(:todo, user: user, group: group) }
let!(:todo_group_user2) { create(:todo, user: user2, group: group) }
let(:mr) { create(:merge_request, source_project: project) }
let!(:todo_mr_user) { create(:todo, user: user, target: mr, project: project) }
let!(:todo_issue_user) { create(:todo, user: user, target: issue, project: project) }
let!(:todo_issue_c_user) { create(:todo, user: user, target: issue_c, project: project) }
let_it_be(:user, reload: true) { create(:user) }
let_it_be(:user2, reload: true) { create(:user) }
let_it_be_with_refind(:group) { create(:group, :private) }
let_it_be(:project) { create(:project, :private, group: group) }
let(:issue) { create(:issue, project: project) }
let(:issue_c) { create(:issue, project: project, confidential: true) }
let!(:todo_group_user) { create(:todo, user: user, group: group) }
let!(:todo_group_user2) { create(:todo, user: user2, group: group) }
let(:mr) { create(:merge_request, source_project: project) }
let!(:todo_mr_user) { create(:todo, user: user, target: mr, project: project) }
let!(:todo_issue_user) { create(:todo, user: user, target: issue, project: project) }
let!(:todo_issue_c_user) { create(:todo, user: user, target: issue_c, project: project) }
let!(:todo_issue_c_user2) { create(:todo, user: user2, target: issue_c, project: project) }
let(:internal_note) { create(:note, noteable: issue, project: project, confidential: true ) }
let!(:todo_for_internal_note) do
create(:todo, user: user, target: issue, project: project, note: internal_note)
end
shared_examples 'using different access permissions' do
before do
......@@ -34,20 +37,28 @@
it { does_not_remove_any_todos }
end
shared_examples 'removes only confidential issues todos' do
it { removes_only_confidential_issues_todos }
shared_examples 'removes confidential issues and internal notes todos' do
it { removes_confidential_issues_and_internal_notes_todos }
end
shared_examples 'removes only internal notes todos' do
it { removes_only_internal_notes_todos }
end
def does_not_remove_any_todos
expect { subject }.not_to change { Todo.count }
end
def removes_only_confidential_issues_todos
expect { subject }.to change { Todo.count }.from(6).to(5)
def removes_only_internal_notes_todos
expect { subject }.to change { Todo.count }.from(7).to(6)
end
def removes_confidential_issues_and_internal_notes_todos
expect { subject }.to change { Todo.count }.from(7).to(5)
end
def removes_confidential_issues_and_merge_request_todos
expect { subject }.to change { Todo.count }.from(6).to(4)
def removes_confidential_issues_and_internal_notes_and_merge_request_todos
expect { subject }.to change { Todo.count }.from(7).to(4)
expect(user.todos).to match_array([todo_issue_user, todo_group_user])
end
......@@ -70,7 +81,7 @@ def set_access(object, user, access_name)
context 'when project is private' do
context 'when user is not a member of the project' do
it 'removes project todos for the provided user' do
expect { subject }.to change { Todo.count }.from(6).to(3)
expect { subject }.to change { Todo.count }.from(7).to(3)
expect(user.todos).to match_array([todo_group_user])
expect(user2.todos).to match_array([todo_issue_c_user2, todo_group_user2])
......@@ -81,11 +92,11 @@ def set_access(object, user, access_name)
where(:group_access, :project_access, :method_name) do
[
[nil, :reporter, :does_not_remove_any_todos],
[nil, :guest, :removes_confidential_issues_and_merge_request_todos],
[nil, :guest, :removes_confidential_issues_and_internal_notes_and_merge_request_todos],
[:reporter, nil, :does_not_remove_any_todos],
[:guest, nil, :removes_confidential_issues_and_merge_request_todos],
[:guest, nil, :removes_confidential_issues_and_internal_notes_and_merge_request_todos],
[:guest, :reporter, :does_not_remove_any_todos],
[:guest, :guest, :removes_confidential_issues_and_merge_request_todos]
[:guest, :guest, :removes_confidential_issues_and_internal_notes_and_merge_request_todos]
]
end
......@@ -97,11 +108,12 @@ def set_access(object, user, access_name)
# a private project in an internal/public group is valid
context 'when project is private in an internal/public group' do
let(:group) { create(:group, :internal) }
let_it_be(:group) { create(:group, :internal) }
let_it_be(:project) { create(:project, :private, group: group) }
context 'when user is not a member of the project' do
it 'removes project todos for the provided user' do
expect { subject }.to change { Todo.count }.from(6).to(3)
expect { subject }.to change { Todo.count }.from(7).to(3)
expect(user.todos).to match_array([todo_group_user])
expect(user2.todos).to match_array([todo_issue_c_user2, todo_group_user2])
......@@ -112,11 +124,11 @@ def set_access(object, user, access_name)
where(:group_access, :project_access, :method_name) do
[
[nil, :reporter, :does_not_remove_any_todos],
[nil, :guest, :removes_confidential_issues_and_merge_request_todos],
[nil, :guest, :removes_confidential_issues_and_internal_notes_and_merge_request_todos],
[:reporter, nil, :does_not_remove_any_todos],
[:guest, nil, :removes_confidential_issues_and_merge_request_todos],
[:guest, nil, :removes_confidential_issues_and_internal_notes_and_merge_request_todos],
[:guest, :reporter, :does_not_remove_any_todos],
[:guest, :guest, :removes_confidential_issues_and_merge_request_todos]
[:guest, :guest, :removes_confidential_issues_and_internal_notes_and_merge_request_todos]
]
end
......@@ -142,7 +154,7 @@ def set_access(object, user, access_name)
context 'confidential issues' do
context 'when a user is not an author of confidential issue' do
it_behaves_like 'removes only confidential issues todos'
it_behaves_like 'removes confidential issues and internal notes todos'
end
context 'when a user is an author of confidential issue' do
......@@ -150,7 +162,7 @@ def set_access(object, user, access_name)
issue_c.update!(author: user)
end
it_behaves_like 'does not remove any todos'
it_behaves_like 'removes only internal notes todos'
end
context 'when a user is an assignee of confidential issue' do
......@@ -158,18 +170,18 @@ def set_access(object, user, access_name)
issue_c.assignees << user
end
it_behaves_like 'does not remove any todos'
it_behaves_like 'removes only internal notes todos'
end
context 'access permissions' do
where(:group_access, :project_access, :method_name) do
[
[nil, :reporter, :does_not_remove_any_todos],
[nil, :guest, :removes_only_confidential_issues_todos],
[nil, :guest, :removes_confidential_issues_and_internal_notes_todos],
[:reporter, nil, :does_not_remove_any_todos],
[:guest, nil, :removes_only_confidential_issues_todos],
[:guest, nil, :removes_confidential_issues_and_internal_notes_todos],
[:guest, :reporter, :does_not_remove_any_todos],
[:guest, :guest, :removes_only_confidential_issues_todos]
[:guest, :guest, :removes_confidential_issues_and_internal_notes_todos]
]
end
......@@ -186,7 +198,7 @@ def set_access(object, user, access_name)
end
it 'removes only users issue todos' do
expect { subject }.to change { Todo.count }.from(6).to(5)
expect { subject }.to change { Todo.count }.from(7).to(5)
end
end
end
......@@ -199,7 +211,7 @@ def set_access(object, user, access_name)
context 'when group is private' do
context 'when a user leaves a group' do
it 'removes group and subproject todos for the user' do
expect { subject }.to change { Todo.count }.from(6).to(2)
expect { subject }.to change { Todo.count }.from(7).to(2)
expect(user.todos).to be_empty
expect(user2.todos).to match_array([todo_issue_c_user2, todo_group_user2])
......@@ -210,11 +222,11 @@ def set_access(object, user, access_name)
where(:group_access, :project_access, :method_name) do
[
[nil, :reporter, :does_not_remove_any_todos],
[nil, :guest, :removes_confidential_issues_and_merge_request_todos],
[nil, :guest, :removes_confidential_issues_and_internal_notes_and_merge_request_todos],
[:reporter, nil, :does_not_remove_any_todos],
[:guest, nil, :removes_confidential_issues_and_merge_request_todos],
[:guest, nil, :removes_confidential_issues_and_internal_notes_and_merge_request_todos],
[:guest, :reporter, :does_not_remove_any_todos],
[:guest, :guest, :removes_confidential_issues_and_merge_request_todos]
[:guest, :guest, :removes_confidential_issues_and_internal_notes_and_merge_request_todos]
]
end
......@@ -224,12 +236,12 @@ def set_access(object, user, access_name)
end
context 'with nested groups' do
let(:parent_group) { create(:group, :public) }
let(:parent_subgroup) { create(:group)}
let(:subgroup) { create(:group, :private, parent: group) }
let(:subgroup2) { create(:group, :private, parent: group) }
let(:subproject) { create(:project, group: subgroup) }
let(:subproject2) { create(:project, group: subgroup2) }
let_it_be_with_refind(:parent_group) { create(:group, :public) }
let_it_be_with_refind(:parent_subgroup) { create(:group) }
let_it_be(:subgroup) { create(:group, :private, parent: group) }
let_it_be(:subgroup2) { create(:group, :private, parent: group) }
let_it_be(:subproject) { create(:project, group: subgroup) }
let_it_be(:subproject2) { create(:project, group: subgroup2) }
let!(:todo_subproject_user) { create(:todo, user: user, project: subproject) }
let!(:todo_subproject2_user) { create(:todo, user: user, project: subproject2) }
......@@ -238,6 +250,10 @@ def set_access(object, user, access_name)
let!(:todo_subproject_user2) { create(:todo, user: user2, project: subproject) }
let!(:todo_subpgroup_user2) { create(:todo, user: user2, group: subgroup) }
let!(:todo_parent_group_user) { create(:todo, user: user, group: parent_group) }
let(:subproject_internal_note) { create(:note, noteable: issue, project: project, confidential: true ) }
let!(:todo_for_internal_subproject_note) do
create(:todo, user: user, target: issue, project: project, note: subproject_internal_note)
end
before do
group.update!(parent: parent_group)
......@@ -245,7 +261,7 @@ def set_access(object, user, access_name)
context 'when the user is not a member of any groups/projects' do
it 'removes todos for the user including subprojects todos' do
expect { subject }.to change { Todo.count }.from(13).to(5)
expect { subject }.to change { Todo.count }.from(15).to(5)
expect(user.todos).to eq([todo_parent_group_user])
expect(user2.todos)
......@@ -269,7 +285,7 @@ def set_access(object, user, access_name)
end
it 'does not remove group and subproject todos' do
expect { subject }.to change { Todo.count }.from(13).to(8)
expect { subject }.to change { Todo.count }.from(15).to(8)
expect(user.todos)
.to match_array(
......@@ -288,7 +304,7 @@ def set_access(object, user, access_name)
end
it 'does not remove subproject and group todos' do
expect { subject }.to change { Todo.count }.from(13).to(8)
expect { subject }.to change { Todo.count }.from(15).to(8)
expect(user.todos)
.to match_array(
......@@ -319,13 +335,13 @@ def set_access(object, user, access_name)
context 'access permissions' do
where(:group_access, :project_access, :method_name) do
[
[nil, nil, :removes_only_confidential_issues_todos],
[nil, nil, :removes_confidential_issues_and_internal_notes_todos],
[nil, :reporter, :does_not_remove_any_todos],
[nil, :guest, :removes_only_confidential_issues_todos],
[nil, :guest, :removes_confidential_issues_and_internal_notes_todos],
[:reporter, nil, :does_not_remove_any_todos],
[:guest, nil, :removes_only_confidential_issues_todos],
[:guest, nil, :removes_confidential_issues_and_internal_notes_todos],
[:guest, :reporter, :does_not_remove_any_todos],
[:guest, :guest, :removes_only_confidential_issues_todos]
[:guest, :guest, :removes_confidential_issues_and_internal_notes_todos]
]
end
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment