Merge branch 'security-todos-redact-guests' into 'master'

Security: Redact confidential todos when membership changed to guest

See merge request gitlab-org/security/gitlab!900

app/services/members/base_service.rb

@@ -7,6 +7,11 @@ module Members
     def initialize(current_user = nil, params = {})
       @current_user = current_user
       @params = params
+
+      # could be a string, force to an integer, part of fix
+      # https://gitlab.com/gitlab-org/gitlab/-/issues/219496
+      # Allow the ArgumentError to be raised if it can't be converted to an integer.
+      @params[:access_level] = Integer(@params[:access_level]) if @params[:access_level]
     end
 
     def after_execute(args)

app/services/todos/destroy/entity_leave_service.rb

@@ -52,7 +52,14 @@ module Todos
 
       # rubocop: disable CodeReuse/ActiveRecord
       def remove_project_todos
-        Todo.where(project_id: non_authorized_projects, user_id: user.id).delete_all
+        # Issues are viewable by guests (even in private projects), so remove those todos
+        # from projects without guest access
+        Todo.where(project_id: non_authorized_guest_projects, user_id: user.id)
+          .delete_all
+
+        # MRs require reporter access, so remove those todos that are not authorized
+        Todo.where(project_id: non_authorized_reporter_projects, target_type: MergeRequest.name, user_id: user.id)
+          .delete_all
       end
       # rubocop: enable CodeReuse/ActiveRecord
 
@@ -68,7 +75,7 @@ module Todos
                     when Project
                       { id: entity.id }
                     when Namespace
-                      { namespace_id: non_member_groups }
+                      { namespace_id: non_authorized_reporter_groups }
                     end
 
         Project.where(condition)
@@ -76,8 +83,32 @@ module Todos
       # rubocop: enable CodeReuse/ActiveRecord
 
       # rubocop: disable CodeReuse/ActiveRecord
-      def non_authorized_projects
-        projects.where('id NOT IN (?)', user.authorized_projects.select(:id))
+      def authorized_reporter_projects
+        user.authorized_projects(Gitlab::Access::REPORTER).select(:id)
+      end
+      # rubocop: enable CodeReuse/ActiveRecord
+
+      # rubocop: disable CodeReuse/ActiveRecord
+      def authorized_guest_projects
+        user.authorized_projects(Gitlab::Access::GUEST).select(:id)
+      end
+      # rubocop: enable CodeReuse/ActiveRecord
+
+      # rubocop: disable CodeReuse/ActiveRecord
+      def non_authorized_reporter_projects
+        projects.where('id NOT IN (?)', authorized_reporter_projects)
+      end
+      # rubocop: enable CodeReuse/ActiveRecord
+
+      # rubocop: disable CodeReuse/ActiveRecord
+      def non_authorized_guest_projects
+        projects.where('id NOT IN (?)', authorized_guest_projects)
+      end
+      # rubocop: enable CodeReuse/ActiveRecord
+
+      # rubocop: disable CodeReuse/ActiveRecord
+      def authorized_reporter_groups
+        GroupsFinder.new(user, min_access_level: Gitlab::Access::REPORTER).execute.select(:id)
       end
       # rubocop: enable CodeReuse/ActiveRecord
 
@@ -91,9 +122,9 @@ module Todos
       # rubocop: enable CodeReuse/ActiveRecord
 
       # rubocop: disable CodeReuse/ActiveRecord
-      def non_member_groups
+      def non_authorized_reporter_groups
         entity.self_and_descendants.select(:id)
-          .where('id NOT IN (?)', user.membership_groups.select(:id))
+          .where('id NOT IN (?)', authorized_reporter_groups)
       end
       # rubocop: enable CodeReuse/ActiveRecord
 
@@ -106,8 +137,6 @@ module Todos
       # rubocop: disable CodeReuse/ActiveRecord
       def confidential_issues
         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: projects, confidential: true)
           .where('project_id NOT IN(?)', authorized_reporter_projects)

changelogs/unreleased/security-todos-redact-guests.yml

+---
+title: Fix redaction of confidential Todos
+merge_request:
+author:
+type: security

spec/services/members/update_service_spec.rb

@@ -31,17 +31,35 @@ RSpec.describe Members::UpdateService do
     end
 
     context 'when member is downgraded to guest' do
-      let(:params) do
-        { access_level: Gitlab::Access::GUEST }
+      shared_examples 'schedules to delete confidential todos' do
+        it 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
+
+      context 'with Gitlab::Access::GUEST level as a string' do
+        let(:params) { { access_level: Gitlab::Access::GUEST.to_s } }
+
+        it_behaves_like 'schedules to delete confidential todos'
       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
+      context 'with Gitlab::Access::GUEST level as an integer' do
+        let(:params) { { access_level: Gitlab::Access::GUEST } }
+
+        it_behaves_like 'schedules to delete confidential todos'
+      end
+    end
 
-        updated_member = described_class.new(current_user, params).execute(member, permission: permission)
+    context 'when access_level is invalid' do
+      let(:params) { { access_level: 'invalid' } }
 
-        expect(updated_member).to be_valid
-        expect(updated_member.access_level).to eq(Gitlab::Access::GUEST)
+      it 'raises an error' do
+        expect { described_class.new(current_user, params) }.to raise_error(ArgumentError, 'invalid value for Integer(): "invalid"')
       end
     end
   end

spec/services/todos/destroy/entity_leave_service_spec.rb

@@ -3,14 +3,15 @@
 require 'spec_helper'
 
 RSpec.describe Todos::Destroy::EntityLeaveService do
-  let_it_be(:group, reload: true)   { create(:group, :private) }
-  let_it_be(:project, reload: true) { create(:project, group: group) }
   let_it_be(:user, reload: true)    { create(:user) }
   let_it_be(:user2, reload: true)   { create(:user) }
-  let_it_be(:issue)                 { create(:issue, project: project) }
-  let_it_be(:issue_c)               { create(:issue, project: project, confidential: true) }
-  let_it_be(:todo_group_user)       { create(:todo, user: user, group: group) }
-  let_it_be(:todo_group_user2)      { create(:todo, user: user2, group: group) }
+
+  let(:group)   { create(:group, :private) }
+  let(:project) { create(:project, :private, group: group) }
+  let(:issue)                 { create(:issue, project: project) }
+  let(:issue_c)               { create(:issue, project: project, confidential: true) }
+  let!(:todo_group_user)       { create(:todo, user: user, group: group) }
+  let!(:todo_group_user2)      { create(:todo, user: user2, group: group) }
 
   let(:mr)                  { create(:merge_request, source_project: project) }
   let!(:todo_mr_user)       { create(:todo, user: user, target: mr, project: project) }
@@ -43,10 +44,6 @@ RSpec.describe Todos::Destroy::EntityLeaveService do
     it { removes_only_confidential_issues_todos }
   end
 
-  shared_examples 'removes confidential issues and merge request todos' do
-    it { removes_confidential_issues_and_merge_request_todos }
-  end
-
   def does_not_remove_any_todos
     expect { subject }.not_to change { Todo.count }
   end
@@ -75,8 +72,9 @@ RSpec.describe Todos::Destroy::EntityLeaveService do
     describe 'updating a Project' do
       subject { described_class.new(user.id, project.id, 'Project').execute }
 
+      # a private project in a private group is valid
       context 'when project is private' do
-        context 'when a user leaves a project' do
+        context 'when user is not a member of the project' do
           it 'removes project todos for the provided user' do
             expect { subject }.to change { Todo.count }.from(6).to(3)
 
@@ -87,27 +85,63 @@ RSpec.describe Todos::Destroy::EntityLeaveService do
 
         context 'access permissions' do
           # rubocop:disable RSpec/LeakyConstantDeclaration
-          PROJECT_PRIVATE_ACCESS_TABLE =
+          PRIVATE_PROJECT_PRIVATE_GROUP_ACCESS_TABLE =
             lambda do |_|
               [
                 # :group_access, :project_access, :c_todos, :mr_todos, :method
                 [nil,            :reporter,       :keep,    :keep,     :does_not_remove_any_todos],
-                [nil,            :guest,          :delete,  :keep,     :removes_only_confidential_issues_todos],
+                [nil,            :guest,          :delete,  :delete,   :removes_confidential_issues_and_merge_request_todos],
+                [:reporter,      nil,             :keep,    :keep,     :does_not_remove_any_todos],
+                [:guest,         nil,             :delete,  :delete,   :removes_confidential_issues_and_merge_request_todos],
+                [:guest,         :reporter,       :keep,    :keep,     :does_not_remove_any_todos],
+                [:guest,         :guest,          :delete,  :delete,   :removes_confidential_issues_and_merge_request_todos]
+              ]
+            end
+          # rubocop:enable RSpec/LeakyConstantDeclaration
+
+          it_behaves_like 'using different access permissions', PRIVATE_PROJECT_PRIVATE_GROUP_ACCESS_TABLE
+        end
+      end
+
+      # a private project in an internal/public group is valid
+      context 'when project is private in an internal/public group' do
+        let(:group) { create(:group, :internal) }
+
+        context 'when user is not a member of the project' do
+          it 'removes project todos for the provided user' do
+            expect { subject }.to change { Todo.count }.from(6).to(3)
+
+            expect(user.todos).to match_array([todo_group_user])
+            expect(user2.todos).to match_array([todo_issue_c_user2, todo_group_user2])
+          end
+        end
+
+        context 'access permissions' do
+          # rubocop:disable RSpec/LeakyConstantDeclaration
+          PRIVATE_PROJECT_INTERNAL_GROUP_ACCESS_TABLE =
+            lambda do |_|
+              [
+                # :group_access, :project_access, :c_todos, :mr_todos, :method
+                [nil,            :reporter,       :keep,    :keep,     :does_not_remove_any_todos],
+                [nil,            :guest,          :delete,  :delete,   :removes_confidential_issues_and_merge_request_todos],
                 [:reporter,      nil,             :keep,    :keep,     :does_not_remove_any_todos],
-                [:guest,         nil,             :delete,  :keep,     :removes_only_confidential_issues_todos]
+                [:guest,         nil,             :delete,  :delete,   :removes_confidential_issues_and_merge_request_todos],
+                [:guest,         :reporter,       :keep,    :keep,     :does_not_remove_any_todos],
+                [:guest,         :guest,          :delete,  :delete,   :removes_confidential_issues_and_merge_request_todos]
               ]
             end
           # rubocop:enable RSpec/LeakyConstantDeclaration
 
-          it_behaves_like 'using different access permissions', PROJECT_PRIVATE_ACCESS_TABLE
+          it_behaves_like 'using different access permissions', PRIVATE_PROJECT_INTERNAL_GROUP_ACCESS_TABLE
         end
       end
 
+      # an internal project in an internal/public group is valid
       context 'when project is not private' do
-        let_it_be(:group, reload: true)   { create(:group, :internal) }
-        let_it_be(:project, reload: true) { create(:project, :internal, group: group) }
-        let_it_be(:issue, reload: true)   { create(:issue, project: project) }
-        let_it_be(:issue_c, reload: true) { create(:issue, project: project, confidential: true) }
+        let(:group)   { create(:group, :internal) }
+        let(:project) { create(:project, :internal, group: group) }
+        let(:issue)   { create(:issue, project: project) }
+        let(:issue_c) { create(:issue, project: project, confidential: true) }
 
         it 'enqueues the PrivateFeaturesWorker' do
           expect(TodosDestroyer::PrivateFeaturesWorker)
@@ -139,7 +173,7 @@ RSpec.describe Todos::Destroy::EntityLeaveService do
 
           context 'access permissions' do
             # rubocop:disable RSpec/LeakyConstantDeclaration
-            PROJECT_NOT_PRIVATE_ACCESS_TABLE =
+            INTERNAL_PROJECT_INTERNAL_GROUP_ACCESS_TABLE =
               lambda do |_|
                 [
                   # :group_access, :project_access, :c_todos, :mr_todos, :method
@@ -147,12 +181,13 @@ RSpec.describe Todos::Destroy::EntityLeaveService do
                   [nil,            :guest,          :delete,  :keep,     :removes_only_confidential_issues_todos],
                   [:reporter,      nil,             :keep,    :keep,     :does_not_remove_any_todos],
                   [:guest,         nil,             :delete,  :keep,     :removes_only_confidential_issues_todos],
-                  [:reporter,      :guest,          :keep,    :keep,     :does_not_remove_any_todos]
+                  [:guest,         :reporter,       :keep,    :keep,     :does_not_remove_any_todos],
+                  [:guest,         :guest,          :delete,  :keep,     :removes_only_confidential_issues_todos]
                 ]
               end
             # rubocop:enable RSpec/LeakyConstantDeclaration
 
-            it_behaves_like 'using different access permissions', PROJECT_NOT_PRIVATE_ACCESS_TABLE
+            it_behaves_like 'using different access permissions', INTERNAL_PROJECT_INTERNAL_GROUP_ACCESS_TABLE
           end
         end
 
@@ -185,17 +220,21 @@ RSpec.describe Todos::Destroy::EntityLeaveService do
 
         context 'access permissions' do
           # rubocop:disable RSpec/LeakyConstantDeclaration
-          GROUP_PRIVATE_ACCESS_TABLE =
+          PRIVATE_GROUP_PRIVATE_PROJECT_ACCESS_TABLE =
             lambda do |_|
               [
                 # :group_access, :project_access, :c_todos, :mr_todos, :method
                 [nil,            :reporter,       :keep,    :keep,     :does_not_remove_any_todos],
-                [:reporter,      nil,             :keep,    :keep,     :does_not_remove_any_todos]
+                [nil,            :guest,          :delete,  :delete,   :removes_confidential_issues_and_merge_request_todos],
+                [:reporter,      nil,             :keep,    :keep,     :does_not_remove_any_todos],
+                [:guest,         nil,             :delete,  :delete,   :removes_confidential_issues_and_merge_request_todos],
+                [:guest,         :reporter,       :keep,    :keep,     :does_not_remove_any_todos],
+                [:guest,         :guest,          :delete,  :delete,   :removes_confidential_issues_and_merge_request_todos]
               ]
             end
           # rubocop:enable RSpec/LeakyConstantDeclaration
 
-          it_behaves_like 'using different access permissions', GROUP_PRIVATE_ACCESS_TABLE
+          it_behaves_like 'using different access permissions', PRIVATE_GROUP_PRIVATE_PROJECT_ACCESS_TABLE
         end
 
         context 'with nested groups' do
@@ -268,10 +307,10 @@ RSpec.describe Todos::Destroy::EntityLeaveService do
       end
 
       context 'when group is not private' do
-        let_it_be(:group, reload: true)   { create(:group, :internal) }
-        let_it_be(:project, reload: true) { create(:project, :internal, group: group) }
-        let_it_be(:issue, reload: true)   { create(:issue, project: project) }
-        let_it_be(:issue_c, reload: true) { create(:issue, project: project, confidential: true) }
+        let(:group)   { create(:group, :internal) }
+        let(:project) { create(:project, :internal, group: group) }
+        let(:issue)   { create(:issue, project: project) }
+        let(:issue_c) { create(:issue, project: project, confidential: true) }
 
         it 'enqueues the PrivateFeaturesWorker' do
           expect(TodosDestroyer::PrivateFeaturesWorker)
@@ -282,18 +321,22 @@ RSpec.describe Todos::Destroy::EntityLeaveService do
 
         context 'access permissions' do
           # rubocop:disable RSpec/LeakyConstantDeclaration
-          GROUP_NOT_PRIVATE_ACCESS_TABLE =
+          INTERNAL_GROUP_INTERNAL_PROJECT_ACCESS_TABLE =
             lambda do |_|
               [
                 # :group_access, :project_access, :c_todos, :mr_todos, :method
                 [nil,            nil,             :delete,  :keep,     :removes_only_confidential_issues_todos],
+                [nil,            :reporter,       :keep,    :keep,     :does_not_remove_any_todos],
                 [nil,            :guest,          :delete,  :keep,     :removes_only_confidential_issues_todos],
-                [:reporter,      :guest,          :keep,    :keep,     :does_not_remove_any_todos]
+                [:reporter,      nil,             :keep,    :keep,     :does_not_remove_any_todos],
+                [:guest,         nil,             :delete,  :keep,     :removes_only_confidential_issues_todos],
+                [:guest,         :reporter,       :keep,    :keep,     :does_not_remove_any_todos],
+                [:guest,         :guest,          :delete,  :keep,     :removes_only_confidential_issues_todos]
               ]
             end
           # rubocop:enable RSpec/LeakyConstantDeclaration
 
-          it_behaves_like 'using different access permissions', GROUP_NOT_PRIVATE_ACCESS_TABLE
+          it_behaves_like 'using different access permissions', INTERNAL_GROUP_INTERNAL_PROJECT_ACCESS_TABLE
         end
       end
     end