Commit 1653f7b1 authored by Felipe Artur's avatar Felipe Artur

Delete confidential issue todos for guests

Fix leaking information of confidential issues on TODOs
when user is downgraded to guest access.
parent cc735352
......@@ -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
......
......@@ -31,7 +31,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
......
......@@ -44,7 +44,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
......
......@@ -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
......@@ -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
......
......@@ -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
......@@ -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')
......
---
title: Delete confidential todos for user when downgraded to Guest
merge_request:
author:
type: security
......@@ -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.
......
......@@ -56,7 +56,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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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 }
......
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