From 59cdc44650c2198250419a7a128af2ddb566b143 Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Tue, 11 Dec 2018 16:15:10 -0200 Subject: Delete confidential issue todos for guests Fix leaking information of confidential issues on TODOs when user is downgraded to guest access. --- app/models/todo.rb | 5 +++++ app/services/groups/update_service.rb | 2 +- app/services/issues/update_service.rb | 2 +- app/services/members/base_service.rb | 6 ++++++ app/services/members/destroy_service.rb | 8 +------- app/services/members/update_service.rb | 9 +++++++++ app/services/projects/update_service.rb | 4 ++-- .../security-todos_not_redacted_for_guests.yml | 5 +++++ doc/workflow/todos.md | 3 +++ spec/services/groups/update_service_spec.rb | 2 +- spec/services/issues/update_service_spec.rb | 2 +- spec/services/members/destroy_service_spec.rb | 2 +- spec/services/members/update_service_spec.rb | 17 +++++++++++++++++ spec/services/projects/update_service_spec.rb | 4 ++-- 14 files changed, 55 insertions(+), 16 deletions(-) create mode 100644 changelogs/unreleased/security-todos_not_redacted_for_guests.yml diff --git a/app/models/todo.rb b/app/models/todo.rb index 265fb932f7c..acd87a6d173 100644 --- a/app/models/todo.rb +++ b/app/models/todo.rb @@ -4,6 +4,11 @@ class Todo < ActiveRecord::Base include Sortable include FromUnion + # Time to wait for todos being removed when not visible for user anymore. + # Prevents TODOs being removed by mistake, for example, removing access from a user + # and giving it back again. + WAIT_FOR_DELETE = 1.hour + ASSIGNED = 1 MENTIONED = 2 BUILD_FAILED = 3 diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb index fe47aa2f140..dd7b30639b0 100644 --- a/app/services/groups/update_service.rb +++ b/app/services/groups/update_service.rb @@ -29,7 +29,7 @@ module Groups def after_update if group.previous_changes.include?(:visibility_level) && group.private? # don't enqueue immediately to prevent todos removal in case of a mistake - TodosDestroyer::GroupPrivateWorker.perform_in(1.hour, group.id) + TodosDestroyer::GroupPrivateWorker.perform_in(Todo::WAIT_FOR_DELETE, group.id) end end diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index b54b0bf6ef6..27013dc21ad 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -38,7 +38,7 @@ module Issues 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? + TodosDestroyer::ConfidentialIssueWorker.perform_in(Todo::WAIT_FOR_DELETE, issue.id) if issue.confidential? create_confidentiality_note(issue) end diff --git a/app/services/members/base_service.rb b/app/services/members/base_service.rb index 8248f1441d7..db719132ca9 100644 --- a/app/services/members/base_service.rb +++ b/app/services/members/base_service.rb @@ -47,5 +47,11 @@ module Members raise "Unknown action '#{action}' on #{member}!" end end + + def enqueue_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(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, type) + end end end diff --git a/app/services/members/destroy_service.rb b/app/services/members/destroy_service.rb index c186a5971dc..ae0c644e6c0 100644 --- a/app/services/members/destroy_service.rb +++ b/app/services/members/destroy_service.rb @@ -15,7 +15,7 @@ module Members notification_service.decline_access_request(member) end - enqeue_delete_todos(member) + enqueue_delete_todos(member) after_execute(member: member) @@ -24,12 +24,6 @@ module Members 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 diff --git a/app/services/members/update_service.rb b/app/services/members/update_service.rb index 1f5618dae53..ff8d5c1d8c9 100644 --- a/app/services/members/update_service.rb +++ b/app/services/members/update_service.rb @@ -10,9 +10,18 @@ module Members if member.update(params) after_execute(action: permission, old_access_level: old_access_level, member: member) + + # Deletes only confidential issues todos for guests + enqueue_delete_todos(member) if downgrading_to_guest? end member end + + private + + def downgrading_to_guest? + params[:access_level] == Gitlab::Access::GUEST + end end end diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb index f25a4e30938..6063392da0a 100644 --- a/app/services/projects/update_service.rb +++ b/app/services/projects/update_service.rb @@ -61,9 +61,9 @@ 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::ProjectPrivateWorker.perform_in(1.hour, 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(1.hour, project.id) + TodosDestroyer::PrivateFeaturesWorker.perform_in(Todo::WAIT_FOR_DELETE, project.id) end if project.previous_changes.include?('path') diff --git a/changelogs/unreleased/security-todos_not_redacted_for_guests.yml b/changelogs/unreleased/security-todos_not_redacted_for_guests.yml new file mode 100644 index 00000000000..be0ae9a7193 --- /dev/null +++ b/changelogs/unreleased/security-todos_not_redacted_for_guests.yml @@ -0,0 +1,5 @@ +--- +title: Delete confidential todos for user when downgraded to Guest +merge_request: +author: +type: security diff --git a/doc/workflow/todos.md b/doc/workflow/todos.md index f94d592d0db..830f17aa7f2 100644 --- a/doc/workflow/todos.md +++ b/doc/workflow/todos.md @@ -35,6 +35,9 @@ A Todo appears in your Todos dashboard when: - the author, or - have set it to automatically merge once pipeline succeeds. +NOTE: **Note:** +When an user no longer has access to a resource related to a Todo like an issue, merge request, project or group the related Todos, for security reasons, gets deleted within the next hour. The delete is delayed to prevent data loss in case user got their access revoked by mistake. + ### Directly addressed Todos > [Introduced][ce-7926] in GitLab 9.0. diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb index 7c5c7409cc1..3e9ae68a0cb 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -50,7 +50,7 @@ describe Groups::UpdateService do create(:project, :private, group: internal_group) expect(TodosDestroyer::GroupPrivateWorker).to receive(:perform_in) - .with(1.hour, internal_group.id) + .with(Todo::WAIT_FOR_DELETE, internal_group.id) end it "changes permission level to private" do diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 07aa8449a66..5f7e9fd320e 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -77,7 +77,7 @@ describe Issues::UpdateService, :mailer do end it 'enqueues ConfidentialIssueWorker when an issue is made confidential' do - expect(TodosDestroyer::ConfidentialIssueWorker).to receive(:perform_in).with(1.hour, issue.id) + expect(TodosDestroyer::ConfidentialIssueWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, issue.id) update_issue(confidential: true) end diff --git a/spec/services/members/destroy_service_spec.rb b/spec/services/members/destroy_service_spec.rb index 0a5220c7c61..5aa7165e135 100644 --- a/spec/services/members/destroy_service_spec.rb +++ b/spec/services/members/destroy_service_spec.rb @@ -22,7 +22,7 @@ describe Members::DestroyService do 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) + expect(TodosDestroyer::EntityLeaveWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, type) end it 'destroys the member' do diff --git a/spec/services/members/update_service_spec.rb b/spec/services/members/update_service_spec.rb index 6d19a95ffeb..599ed39ca37 100644 --- a/spec/services/members/update_service_spec.rb +++ b/spec/services/members/update_service_spec.rb @@ -20,11 +20,28 @@ describe Members::UpdateService do shared_examples 'a service updating a member' do it 'updates the member' do + expect(TodosDestroyer::EntityLeaveWorker).not_to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, source.class.name) + updated_member = described_class.new(current_user, params).execute(member, permission: permission) expect(updated_member).to be_valid expect(updated_member.access_level).to eq(Gitlab::Access::MAINTAINER) end + + context 'when member is downgraded to guest' do + let(:params) do + { access_level: Gitlab::Access::GUEST } + end + + it 'schedules to delete confidential todos' do + expect(TodosDestroyer::EntityLeaveWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, source.class.name).once + + updated_member = described_class.new(current_user, params).execute(member, permission: permission) + + expect(updated_member).to be_valid + expect(updated_member.access_level).to eq(Gitlab::Access::GUEST) + end + end end before do diff --git a/spec/services/projects/update_service_spec.rb b/spec/services/projects/update_service_spec.rb index d58ff2cedc0..8adfc63222e 100644 --- a/spec/services/projects/update_service_spec.rb +++ b/spec/services/projects/update_service_spec.rb @@ -41,7 +41,7 @@ describe Projects::UpdateService do end it 'updates the project to private' do - expect(TodosDestroyer::ProjectPrivateWorker).to receive(:perform_in).with(1.hour, project.id) + expect(TodosDestroyer::ProjectPrivateWorker).to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, project.id) result = update_project(project, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE) @@ -191,7 +191,7 @@ describe Projects::UpdateService do context 'when changing feature visibility to private' do it 'updates the visibility correctly' do expect(TodosDestroyer::PrivateFeaturesWorker) - .to receive(:perform_in).with(1.hour, project.id) + .to receive(:perform_in).with(Todo::WAIT_FOR_DELETE, project.id) result = update_project(project, user, project_feature_attributes: { issues_access_level: ProjectFeature::PRIVATE } -- cgit v1.2.1