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

[11.4] Delete confidential issue todos for guests

See merge request gitlab/gitlabhq!2724

14 files changed, 55 insertions, 16 deletions

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 }