Fix removing todos for users without access

7 files changed, 295 insertions, 79 deletions

diff --git a/app/services/todos/destroy/entity_leave_service.rb b/app/services/todos/destroy/entity_leave_service.rb
index b1c4eb95e87..ef6add352e7 100644
--- a/app/services/todos/destroy/entity_leave_service.rb
+++ b/app/services/todos/destroy/entity_leave_service.rb
@@ -2,65 +2,99 @@ module Todos
   module Destroy
     class EntityLeaveService < ::Todos::Destroy::BaseService
       extend ::Gitlab::Utils::Override
+      include Gitlab::Utils::StrongMemoize
 
-      attr_reader :user_id, :entity
+      attr_reader :user, :entity
 
       def initialize(user_id, entity_id, entity_type)
         unless %w(Group Project).include?(entity_type)
           raise ArgumentError.new("#{entity_type} is not an entity user can leave")
         end
 
-        @user_id = user_id
+        @user = User.find_by(id: user_id)
         @entity = entity_type.constantize.find_by(id: entity_id)
       end
 
-      private
+      def execute
+        return unless entity && user
+
+        # if at least reporter, all entities including confidential issues can be accessed
+        return if main_group_reporter?
+
+        remove_confidential_issue_todos
 
-      override :todos
-      def todos
         if entity.private?
-          Todo.where('(project_id IN (?) OR group_id IN (?)) AND user_id = ?', project_ids, group_ids, user_id)
+          remove_project_todos
+          remove_group_todos
         else
-          project_ids.each do |project_id|
-            TodosDestroyer::PrivateFeaturesWorker.perform_async(project_id, user_id)
-          end
+          enqueue_private_features_worker
+        end
+      end
 
-          Todo.where(
-            target_id: confidential_issues.select(:id), target_type: Issue, user_id: user_id
-          )
+      private
+
+      def enqueue_private_features_worker
+        project_ids.each do |project_id|
+          TodosDestroyer::PrivateFeaturesWorker.perform_async(project_id, user.id)
         end
       end
 
+      def remove_confidential_issue_todos
+        Todo.where(
+          target_id: confidential_issues.select(:id), target_type: Issue, user_id: user.id
+        ).delete_all
+      end
+
+      def remove_project_todos
+        Todo.where(project_id: non_authorized_projects, user_id: user.id).delete_all
+      end
+
+      def remove_group_todos
+        Todo.where(group_id: non_authorized_groups, user_id: user.id).delete_all
+      end
+
       override :project_ids
       def project_ids
-        case entity
-        when Project
-          [entity.id]
-        when Namespace
-          Project.select(:id).where(namespace_id: group_ids)
-        end
+        condition = case entity
+                    when Project
+                      { id: entity.id }
+                    when Namespace
+                      { namespace_id: non_member_groups }
+                    end
+
+        Project.where(condition).select(:id)
       end
 
-      def group_ids
-        case entity
-        when Project
-          []
-        when Namespace
-          entity.self_and_descendants.select(:id)
-        end
+      def non_authorized_projects
+        project_ids.where('id NOT IN (?)', user.authorized_projects.select(:id))
       end
 
-      override :todos_to_remove?
-      def todos_to_remove?
-        # if an entity is provided we want to check always at least private features
-        !!entity
+      def non_authorized_groups
+        return [] unless entity.is_a?(Namespace)
+
+        entity.self_and_descendants.select(:id)
+          .where('id NOT IN (?)', GroupsFinder.new(user).execute.select(:id))
+      end
+
+      def non_member_groups
+        entity.self_and_descendants.select(:id)
+          .where('id NOT IN (?)', user.membership_groups.select(:id))
+      end
+
+      def main_group_reporter?
+        return unless entity.is_a?(Namespace)
+
+        entity.member?(User.find(user.id), Gitlab::Access::REPORTER)
       end
 
       def confidential_issues
-        assigned_ids = IssueAssignee.select(:issue_id).where(user_id: user_id)
+        assigned_ids = IssueAssignee.select(:issue_id).where(user_id: user.id)
+        authorized_reporter_projects = user
+          .authorized_projects(Gitlab::Access::REPORTER).select(:id)
 
         Issue.where(project_id: project_ids, confidential: true)
-          .where('author_id != ?', user_id)
+          .where('project_id NOT IN(?)', authorized_reporter_projects)
+          .where('author_id != ?', user.id)
           .where('id NOT IN (?)', assigned_ids)
       end
     end
diff --git a/app/services/todos/destroy/group_private_service.rb b/app/services/todos/destroy/group_private_service.rb
index 1b00bbe91e1..d13fa7a6516 100644
--- a/app/services/todos/destroy/group_private_service.rb
+++ b/app/services/todos/destroy/group_private_service.rb
@@ -18,7 +18,7 @@ module Todos
 
       override :authorized_users
       def authorized_users
-        GroupMember.select(:user_id).where(source: group.id)
+        group.direct_and_indirect_users.select(:id)
       end
 
       override :todos_to_remove?
diff --git a/app/services/todos/destroy/project_private_service.rb b/app/services/todos/destroy/project_private_service.rb
index 171933e7cbc..315a0c33398 100644
--- a/app/services/todos/destroy/project_private_service.rb
+++ b/app/services/todos/destroy/project_private_service.rb
@@ -13,7 +13,7 @@ module Todos
 
       override :todos
       def todos
-        Todo.where(project_id: project_ids)
+        Todo.where(project_id: project.id)
       end
 
       override :project_ids
diff --git a/spec/services/todos/destroy/confidential_issue_service_spec.rb b/spec/services/todos/destroy/confidential_issue_service_spec.rb
index 54d1d7e83f1..3294f7509aa 100644
--- a/spec/services/todos/destroy/confidential_issue_service_spec.rb
+++ b/spec/services/todos/destroy/confidential_issue_service_spec.rb
@@ -29,12 +29,8 @@ describe Todos::Destroy::ConfidentialIssueService do
         issue.update!(confidential: true)
       end
 
-      it 'removes issue todos for a user who is not a project member' do
+      it 'removes issue todos for users who can not access the confidential issue' do
         expect { subject }.to change { Todo.count }.from(6).to(4)
-
-        expect(user.todos).to match_array([todo_another_non_member])
-        expect(author.todos).to match_array([todo_issue_author])
-        expect(project_member.todos).to match_array([todo_issue_member])
       end
     end
 
diff --git a/spec/services/todos/destroy/entity_leave_service_spec.rb b/spec/services/todos/destroy/entity_leave_service_spec.rb
index c554db5e024..8cb91e7c1b9 100644
--- a/spec/services/todos/destroy/entity_leave_service_spec.rb
+++ b/spec/services/todos/destroy/entity_leave_service_spec.rb
@@ -5,7 +5,7 @@ describe Todos::Destroy::EntityLeaveService do
   let(:project) { create(:project, group: group) }
   let(:user)    { create(:user) }
   let(:user2)   { create(:user) }
-  let(:issue)   { create(:issue, project: project) }
+  let(:issue)   { create(:issue, project: project, confidential: true) }
   let(:mr)      { create(:merge_request, source_project: project) }
 
   let!(:todo_mr_user)     { create(:todo, user: user, target: mr, project: project) }
@@ -25,17 +25,20 @@ describe Todos::Destroy::EntityLeaveService do
           expect(user.todos).to match_array([todo_group_user])
           expect(user2.todos).to match_array([todo_issue_user2, todo_group_user2])
         end
-      end
 
-      context 'when project is not private' do
-        before do
-          group.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
-          project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
+        context 'when the user is member of the project' do
+          before do
+            project.add_developer(user)
+          end
+
+          it 'does not remove any todos' do
+            expect { subject }.not_to change { Todo.count }
+          end
         end
 
-        context 'when a user is not an author of confidential issue' do
+        context 'when the user is a project guest' do
           before do
-            issue.update!(confidential: true)
+            project.add_guest(user)
           end
 
           it 'removes only confidential issues todos' do
@@ -43,24 +46,79 @@ describe Todos::Destroy::EntityLeaveService do
           end
         end
 
-        context 'when a user is an author of confidential issue' do
+        context 'when the user is member of a parent group' do
           before do
-            issue.update!(author: user, confidential: true)
+            group.add_developer(user)
           end
 
-          it 'removes only confidential issues todos' do
+          it 'does not remove any todos' do
             expect { subject }.not_to change { Todo.count }
           end
         end
 
-        context 'when a user is an assignee of confidential issue' do
+        context 'when the user is guest of a parent group' do
           before do
-            issue.update!(confidential: true)
-            issue.assignees << user
+            project.add_guest(user)
           end
 
           it 'removes only confidential issues todos' do
-            expect { subject }.not_to change { Todo.count }
+            expect { subject }.to change { Todo.count }.from(5).to(4)
+          end
+        end
+      end
+
+      context 'when project is not private' do
+        before do
+          group.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
+          project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
+        end
+
+        context 'confidential issues' do
+          context 'when a user is not an author of confidential issue' do
+            it 'removes only confidential issues todos' do
+              expect { subject }.to change { Todo.count }.from(5).to(4)
+            end
+          end
+
+          context 'when a user is an author of confidential issue' do
+            before do
+              issue.update!(author: user)
+            end
+
+            it 'does not remove any todos' do
+              expect { subject }.not_to change { Todo.count }
+            end
+          end
+
+          context 'when a user is an assignee of confidential issue' do
+            before do
+              issue.assignees << user
+            end
+
+            it 'does not remove any todos' do
+              expect { subject }.not_to change { Todo.count }
+            end
+          end
+
+          context 'when a user is a project guest' do
+            before do
+              project.add_guest(user)
+            end
+
+            it 'removes only confidential issues todos' do
+              expect { subject }.to change { Todo.count }.from(5).to(4)
+            end
+          end
+
+          context 'when a user is a project guest but group developer' do
+            before do
+              project.add_guest(user)
+              group.add_developer(user)
+            end
+
+            it 'does not remove any todos' do
+              expect { subject }.not_to change { Todo.count }
+            end
           end
         end
 
@@ -89,37 +147,128 @@ describe Todos::Destroy::EntityLeaveService do
           expect(user2.todos).to match_array([todo_issue_user2, todo_group_user2])
         end
 
+        context 'when the user is member of the group' do
+          before do
+            group.add_developer(user)
+          end
+
+          it 'does not remove any todos' do
+            expect { subject }.not_to change { Todo.count }
+          end
+        end
+
+        context 'when the user is member of the group project but not the group' do
+          before do
+            project.add_developer(user)
+          end
+
+          it 'does not remove any todos' do
+            expect { subject }.not_to change { Todo.count }
+          end
+        end
+
         context 'with nested groups', :nested_groups do
           let(:subgroup) { create(:group, :private, parent: group) }
+          let(:subgroup2) { create(:group, :private, parent: group) }
           let(:subproject) { create(:project, group: subgroup) }
+          let(:subproject2) { create(:project, group: subgroup2) }
 
           let!(:todo_subproject_user)  { create(:todo, user: user, project: subproject) }
+          let!(:todo_subproject2_user)  { create(:todo, user: user, project: subproject2) }
           let!(:todo_subgroup_user)    { create(:todo, user: user, group: subgroup) }
+          let!(:todo_subgroup2_user)    { create(:todo, user: user, group: subgroup2) }
           let!(:todo_subproject_user2) { create(:todo, user: user2, project: subproject) }
           let!(:todo_subpgroup_user2)  { create(:todo, user: user2, group: subgroup) }
 
-          it 'removes todos for the user including subprojects todos' do
-            expect { subject }.to change { Todo.count }.from(9).to(4)
+          context 'when the user is not a member of any groups/projects' do
+            it 'removes todos for the user including subprojects todos' do
+              expect { subject }.to change { Todo.count }.from(11).to(4)
+
+              expect(user.todos).to be_empty
+              expect(user2.todos)
+                .to match_array(
+                  [todo_issue_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2]
+                )
+            end
+          end
+
+          context 'when the user is member of a parent group' do
+            before do
+              parent_group = create(:group)
+              group.update!(parent: parent_group)
+              parent_group.add_developer(user)
+            end
+
+            it 'does not remove any todos' do
+              expect { subject }.not_to change { Todo.count }
+            end
+          end
+
+          context 'when the user is member of a subgroup' do
+            before do
+              subgroup.add_developer(user)
+            end
+
+            it 'does not remove group and subproject todos' do
+              expect { subject }.to change { Todo.count }.from(11).to(7)
+
+              expect(user.todos).to match_array([todo_group_user, todo_subgroup_user, todo_subproject_user])
+              expect(user2.todos)
+                .to match_array(
+                  [todo_issue_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2]
+                )
+            end
+          end
 
-            expect(user.todos).to be_empty
-            expect(user2.todos)
-              .to match_array(
-                [todo_issue_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2]
-              )
+          context 'when the user is member of a child project' do
+            before do
+              subproject.add_developer(user)
+            end
+
+            it 'does not remove subproject and group todos' do
+              expect { subject }.to change { Todo.count }.from(11).to(7)
+
+              expect(user.todos).to match_array([todo_subgroup_user, todo_group_user, todo_subproject_user])
+              expect(user2.todos)
+                .to match_array(
+                  [todo_issue_user2, todo_group_user2, todo_subproject_user2, todo_subpgroup_user2]
+                )
+            end
           end
         end
       end
 
       context 'when group is not private' do
         before do
-          issue.update!(confidential: true)
-
           group.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
           project.update!(visibility_level: Gitlab::VisibilityLevel::INTERNAL)
         end
 
-        it 'removes only confidential issues todos' do
-          expect { subject }.to change { Todo.count }.from(5).to(4)
+        context 'when user is not member' do
+          it 'removes only confidential issues todos' do
+            expect { subject }.to change { Todo.count }.from(5).to(4)
+          end
+        end
+
+        context 'when user is a project guest' do
+          before do
+            project.add_guest(user)
+          end
+
+          it 'removes only confidential issues todos' do
+            expect { subject }.to change { Todo.count }.from(5).to(4)
+          end
+        end
+
+        context 'when user is a project guest & group developer' do
+          before do
+            project.add_guest(user)
+            group.add_developer(user)
+          end
+
+          it 'does not remove any todos' do
+            expect { subject }.not_to change { Todo.count }
+          end
         end
       end
     end
diff --git a/spec/services/todos/destroy/group_private_service_spec.rb b/spec/services/todos/destroy/group_private_service_spec.rb
index 48e5a5a91eb..c4ee6ebed50 100644
--- a/spec/services/todos/destroy/group_private_service_spec.rb
+++ b/spec/services/todos/destroy/group_private_service_spec.rb
@@ -1,31 +1,62 @@
 require 'spec_helper'
 
 describe Todos::Destroy::GroupPrivateService do
-  let(:group)        { create(:group, :public) }
-  let(:user)         { create(:user) }
-  let(:group_member) { create(:user) }
+  let(:group)         { create(:group, :public) }
+  let(:project)       { create(:project, group: group) }
+  let(:user)          { create(:user) }
+  let(:group_member)  { create(:user) }
+  let(:project_member)  { create(:user) }
 
-  let!(:todo_non_member)   { create(:todo, user: user, group: group) }
-  let!(:todo_member)       { create(:todo, user: group_member, group: group) }
+  let!(:todo_non_member)         { create(:todo, user: user, group: group) }
   let!(:todo_another_non_member) { create(:todo, user: user, group: group) }
+  let!(:todo_group_member)       { create(:todo, user: group_member, group: group) }
+  let!(:todo_project_member)     { create(:todo, user: project_member, group: group) }
 
   describe '#execute' do
     before do
       group.add_developer(group_member)
+      project.add_developer(project_member)
     end
 
     subject { described_class.new(group.id).execute }
 
-    context 'when a project set to private' do
+    context 'when a group set to private' do
       before do
         group.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
       end
 
-      it 'removes todos for a user who is not a member' do
-        expect { subject }.to change { Todo.count }.from(3).to(1)
+      it 'removes todos only for users who are not group users' do
+        expect { subject }.to change { Todo.count }.from(4).to(2)
 
         expect(user.todos).to be_empty
-        expect(group_member.todos).to match_array([todo_member])
+        expect(group_member.todos).to match_array([todo_group_member])
+        expect(project_member.todos).to match_array([todo_project_member])
+      end
+
+      context 'with nested groups' do
+        let(:parent_group) { create(:group) }
+        let(:subgroup)     { create(:group, :private, parent: group) }
+        let(:subproject)   { create(:project, group: subgroup) }
+
+        let(:parent_member)  { create(:user) }
+        let(:subgroup_member)  { create(:user) }
+        let(:subgproject_member)  { create(:user) }
+
+        let!(:todo_parent_member)     { create(:todo, user: parent_member, group: group) }
+        let!(:todo_subgroup_member)   { create(:todo, user: subgroup_member, group: group) }
+        let!(:todo_subproject_member) { create(:todo, user: subgproject_member, group: group) }
+
+        before do
+          group.update!(parent: parent_group)
+
+          parent_group.add_developer(parent_member)
+          subgroup.add_developer(subgroup_member)
+          subproject.add_developer(subgproject_member)
+        end
+
+        it 'removes todos only for users who are not group users' do
+          expect { subject }.to change { Todo.count }.from(7).to(5)
+        end
       end
     end
 
diff --git a/spec/services/todos/destroy/project_private_service_spec.rb b/spec/services/todos/destroy/project_private_service_spec.rb
index badf3f913a5..9ebc0231795 100644
--- a/spec/services/todos/destroy/project_private_service_spec.rb
+++ b/spec/services/todos/destroy/project_private_service_spec.rb
@@ -1,17 +1,22 @@
 require 'spec_helper'
 
 describe Todos::Destroy::ProjectPrivateService do
-  let(:project)        { create(:project, :public) }
+  let(:group)          { create(:group, :public) }
+  let(:project)        { create(:project, :public, group: group) }
   let(:user)           { create(:user) }
   let(:project_member) { create(:user) }
+  let(:group_member)   { create(:user) }
 
-  let!(:todo_issue_non_member)   { create(:todo, user: user, project: project) }
-  let!(:todo_issue_member)       { create(:todo, user: project_member, project: project) }
-  let!(:todo_another_non_member) { create(:todo, user: user, project: project) }
+  let!(:todo_non_member)   { create(:todo, user: user, project: project) }
+  let!(:todo2_non_member)  { create(:todo, user: user, project: project) }
+  let!(:todo_member)       { create(:todo, user: project_member, project: project) }
+  let!(:todo_member)       { create(:todo, user: project_member, project: project) }
+  let!(:todo_group_member) { create(:todo, user: group_member, project: project) }
 
   describe '#execute' do
     before do
       project.add_developer(project_member)
+      group.add_developer(group_member)
     end
 
     subject { described_class.new(project.id).execute }
@@ -22,10 +27,11 @@ describe Todos::Destroy::ProjectPrivateService do
       end
 
       it 'removes issue todos for a user who is not a member' do
-        expect { subject }.to change { Todo.count }.from(3).to(1)
+        expect { subject }.to change { Todo.count }.from(4).to(2)
 
         expect(user.todos).to be_empty
-        expect(project_member.todos).to match_array([todo_issue_member])
+        expect(project_member.todos).to match_array([todo_member])
+        expect(group_member.todos).to match_array([todo_group_member])
       end
     end