Commit 1f6cd7d3 authored by John Jarvis's avatar John Jarvis
Browse files

Merge branch 'security-todos_not_redacted_for_guests-11-5' into 'security-11-5'

[11.5] Delete confidential issue todos for guests

See merge request gitlab/gitlabhq!2723
parents 30d2af94 d488b4e4
......@@ -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 @@ def execute
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
......
......@@ -38,7 +38,7 @@ def handle_changes(issue, options)
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 @@ def action_member_permission(action, member)
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 @@ def execute(member, skip_authorization: false)
notification_service.decline_access_request(member)
end
enqeue_delete_todos(member)
enqueue_delete_todos(member)
after_execute(member: member)
......@@ -24,12 +24,6 @@ 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
......
......@@ -10,9 +10,18 @@ def execute(member, permission: :update)
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 @@ 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)
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 @@
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 @@ def update_issue(opts)
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 @@
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 @@
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 @@
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 @@
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