Merge branch 'security-todos_not_redacted_for_guests' into 'master'

[master] Security todos not redacted for guests

See merge request gitlab/gitlabhq!2697

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

app/services/groups/update_service.rb

@@ -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
 

app/services/issues/update_service.rb

@@ -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
 

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

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

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

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')

changelogs/unreleased/security-todos_not_redacted_for_guests.yml

+---
+title: Delete confidential todos for user when downgraded to Guest
+merge_request:
+author:
+type: security

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.

spec/services/groups/update_service_spec.rb

@@ -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

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

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

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

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 }