Merge branch 'security-todos_not_redacted_for_guests' into 'master'

[master] Security todos not redacted for guests

See merge request gitlab/gitlabhq!2697

7 files changed, 25 insertions, 11 deletions

diff --git a/app/models/todo.rb b/app/models/todo.rb
index 7b64615f699..d9b86d941b6 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 31d3c844ad5..de78a3f7b27 100644
--- a/app/services/groups/update_service.rb
+++ b/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
 
diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb
index a1d0cc0e568..e992d682c79 100644
--- a/app/services/issues/update_service.rb
+++ b/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
 
diff --git a/app/services/members/base_service.rb b/app/services/members/base_service.rb
index d734571f835..e78affff797 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 93e48fc0199..dd1b9680ece 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')