Merge branch 'security-120026-redact-notes-in-moved-confidential-issues' into 'master'

Redact notes in moved confidential issues

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

app/models/issue.rb

@@ -326,9 +326,7 @@ class Issue < ApplicationRecord
       true
     elsif project.owner == user
       true
-    elsif confidential?
-      author == user ||
-        assignees.include?(user) ||
+    elsif confidential? && !assignee_or_author?(user)
       project.team.member?(user, Gitlab::Access::REPORTER)
     else
       project.public? ||

changelogs/unreleased/security-120026-redact-notes-in-moved-confidential-issues.yml

+---
+title: Redact notes in moved confidential issues
+merge_request:
+author:
+type: security

spec/lib/banzai/filter/reference_redactor_filter_spec.rb

@@ -20,8 +20,8 @@ describe Banzai::Filter::ReferenceRedactorFilter do
   it 'skips when the skip_redaction flag is set' do
     user = create(:user)
     project = create(:project)
-
     link = reference_link(project: project.id, reference_type: 'test')
+
     doc = filter(link, current_user: user, skip_redaction: true)
 
     expect(doc.css('a').length).to eq 1
@@ -51,8 +51,8 @@ describe Banzai::Filter::ReferenceRedactorFilter do
         user = create(:user)
         project = create(:project)
         project.add_maintainer(user)
-
         link = reference_link(project: project.id, reference_type: 'test')
+
         doc = filter(link, current_user: user)
 
         expect(doc.css('a').length).to eq 1
@@ -69,8 +69,8 @@ describe Banzai::Filter::ReferenceRedactorFilter do
       it 'removes unpermitted references' do
         user = create(:user)
         project = create(:project)
-
         link = reference_link(project: project.id, reference_type: 'test')
+
         doc = filter(link, current_user: user)
 
         expect(doc.css('a').length).to eq 0
@@ -90,8 +90,8 @@ describe Banzai::Filter::ReferenceRedactorFilter do
         non_member = create(:user)
         project = create(:project, :public)
         issue = create(:issue, :confidential, project: project)
-
         link = reference_link(project: project.id, issue: issue.id, reference_type: 'issue')
+
         doc = filter(link, current_user: non_member)
 
         expect(doc.css('a').length).to eq 0
@@ -124,8 +124,8 @@ describe Banzai::Filter::ReferenceRedactorFilter do
         assignee = create(:user)
         project = create(:project, :public)
         issue = create(:issue, :confidential, project: project, assignees: [assignee])
-
         link = reference_link(project: project.id, issue: issue.id, reference_type: 'issue')
+
         doc = filter(link, current_user: assignee)
 
         expect(doc.css('a').length).to eq 1
@@ -136,8 +136,8 @@ describe Banzai::Filter::ReferenceRedactorFilter do
         project = create(:project, :public)
         project.add_developer(member)
         issue = create(:issue, :confidential, project: project)
-
         link = reference_link(project: project.id, issue: issue.id, reference_type: 'issue')
+
         doc = filter(link, current_user: member)
 
         expect(doc.css('a').length).to eq 1
@@ -147,20 +147,62 @@ describe Banzai::Filter::ReferenceRedactorFilter do
         admin = create(:admin)
         project = create(:project, :public)
         issue = create(:issue, :confidential, project: project)
-
         link = reference_link(project: project.id, issue: issue.id, reference_type: 'issue')
+
         doc = filter(link, current_user: admin)
 
         expect(doc.css('a').length).to eq 1
       end
+
+      context "when a confidential issue is moved from a public project to a private one" do
+        let(:public_project) { create(:project, :public) }
+        let(:private_project) { create(:project, :private) }
+
+        it 'removes references for author' do
+          author = create(:user)
+          issue = create(:issue, :confidential, project: public_project, author: author)
+          issue.update!(project: private_project) # move issue to private project
+          link = reference_link(project: private_project.id, issue: issue.id, reference_type: 'issue')
+
+          doc = filter(link, current_user: author)
+
+          expect(doc.css('a').length).to eq 0
+        end
+
+        it 'removes references for assignee' do
+          assignee = create(:user)
+          issue = create(:issue, :confidential, project: public_project, assignees: [assignee])
+          issue.update!(project: private_project) # move issue to private project
+          link = reference_link(project: private_project.id, issue: issue.id, reference_type: 'issue')
+
+          doc = filter(link, current_user: assignee)
+
+          expect(doc.css('a').length).to eq 0
+        end
+
+        it 'allows references for project members' do
+          member = create(:user)
+          project = create(:project, :public)
+          project_2 = create(:project, :private)
+          project.add_developer(member)
+          project_2.add_developer(member)
+          issue = create(:issue, :confidential, project: project)
+          issue.update!(project: project_2) # move issue to private project
+          link = reference_link(project: project_2.id, issue: issue.id, reference_type: 'issue')
+
+          doc = filter(link, current_user: member)
+
+          expect(doc.css('a').length).to eq 1
+        end
+      end
     end
 
     it 'allows references for non confidential issues' do
       user = create(:user)
       project = create(:project, :public)
       issue = create(:issue, project: project)
-
       link = reference_link(project: project.id, issue: issue.id, reference_type: 'issue')
+
       doc = filter(link, current_user: user)
 
       expect(doc.css('a').length).to eq 1
@@ -172,8 +214,8 @@ describe Banzai::Filter::ReferenceRedactorFilter do
       it 'removes unpermitted Group references' do
         user = create(:user)
         group = create(:group, :private)
-
         link = reference_link(group: group.id, reference_type: 'user')
+
         doc = filter(link, current_user: user)
 
         expect(doc.css('a').length).to eq 0
@@ -183,8 +225,8 @@ describe Banzai::Filter::ReferenceRedactorFilter do
         user = create(:user)
         group = create(:group, :private)
         group.add_developer(user)
-
         link = reference_link(group: group.id, reference_type: 'user')
+
         doc = filter(link, current_user: user)
 
         expect(doc.css('a').length).to eq 1
@@ -200,8 +242,8 @@ describe Banzai::Filter::ReferenceRedactorFilter do
     context 'with data-user' do
       it 'allows any User reference' do
         user = create(:user)
-
         link = reference_link(user: user.id, reference_type: 'user')
+
         doc = filter(link)
 
         expect(doc.css('a').length).to eq 1

spec/models/issue_spec.rb

@@ -529,89 +529,137 @@ describe Issue do
   end
 
   describe '#visible_to_user?' do
+    let(:project) { build(:project) }
+    let(:issue)   { build(:issue, project: project) }
+    let(:user)    { create(:user) }
+
+    subject { issue.visible_to_user?(user) }
+
+    context 'with a project' do
+      it 'returns false when feature is disabled' do
+        project.project_feature.update_attribute(:issues_access_level, ProjectFeature::DISABLED)
+
+        is_expected.to eq(false)
+      end
+
+      it 'returns false when restricted for members' do
+        project.project_feature.update_attribute(:issues_access_level, ProjectFeature::PRIVATE)
+
+        is_expected.to eq(false)
+      end
+    end
+
     context 'without a user' do
-      let(:issue) { build(:issue) }
+      let(:user) { nil }
 
       it 'returns true when the issue is publicly visible' do
         expect(issue).to receive(:publicly_visible?).and_return(true)
 
-        expect(issue.visible_to_user?).to eq(true)
+        is_expected.to eq(true)
       end
 
       it 'returns false when the issue is not publicly visible' do
         expect(issue).to receive(:publicly_visible?).and_return(false)
 
-        expect(issue.visible_to_user?).to eq(false)
+        is_expected.to eq(false)
       end
     end
 
     context 'with a user' do
-      let(:user) { create(:user) }
-      let(:issue) { build(:issue) }
-
-      it 'returns true when the issue is readable' do
-        expect(issue).to receive(:readable_by?).with(user).and_return(true)
+      shared_examples 'issue readable by user' do
+        it { is_expected.to eq(true) }
+      end
 
-        expect(issue.visible_to_user?(user)).to eq(true)
+      shared_examples 'issue not readable by user' do
+        it { is_expected.to eq(false) }
       end
 
-      it 'returns false when the issue is not readable' do
-        expect(issue).to receive(:readable_by?).with(user).and_return(false)
+      shared_examples 'confidential issue readable by user' do
+        specify do
+          issue.confidential = true
 
-        expect(issue.visible_to_user?(user)).to eq(false)
+          is_expected.to eq(true)
+        end
       end
 
-      it 'returns false when feature is disabled' do
-        expect(issue).not_to receive(:readable_by?)
+      shared_examples 'confidential issue not readable by user' do
+        specify do
+          issue.confidential = true
+
+          is_expected.to eq(false)
+        end
+      end
 
-        issue.project.project_feature.update_attribute(:issues_access_level, ProjectFeature::DISABLED)
+      context 'with an admin user' do
+        let(:user) { build(:admin) }
 
-        expect(issue.visible_to_user?(user)).to eq(false)
+        it_behaves_like 'issue readable by user'
+        it_behaves_like 'confidential issue readable by user'
       end
 
-      it 'returns false when restricted for members' do
-        expect(issue).not_to receive(:readable_by?)
+      context 'with an owner' do
+        before do
+          project.add_maintainer(user)
+        end
 
-        issue.project.project_feature.update_attribute(:issues_access_level, ProjectFeature::PRIVATE)
+        it_behaves_like 'issue readable by user'
+        it_behaves_like 'confidential issue readable by user'
+      end
 
-        expect(issue.visible_to_user?(user)).to eq(false)
+      context 'with a reporter user' do
+        before do
+          project.add_reporter(user)
         end
+
+        it_behaves_like 'issue readable by user'
+        it_behaves_like 'confidential issue readable by user'
       end
 
-    describe 'with a regular user that is not a team member' do
-      let(:user) { create(:user) }
+      context 'with a guest user' do
+        before do
+          project.add_guest(user)
+        end
 
-      context 'using a public project' do
-        let(:project) { create(:project, :public) }
+        it_behaves_like 'issue readable by user'
+        it_behaves_like 'confidential issue not readable by user'
 
-        it 'returns true for a regular issue' do
-          issue = build(:issue, project: project)
+        context 'when user is an assignee' do
+          before do
+            issue.update!(assignees: [user])
+          end
 
-          expect(issue.visible_to_user?(user)).to eq(true)
+          it_behaves_like 'issue readable by user'
+          it_behaves_like 'confidential issue readable by user'
         end
 
-        it 'returns false for a confidential issue' do
-          issue = build(:issue, project: project, confidential: true)
+        context 'when user is the author' do
+          before do
+            issue.update!(author: user)
+          end
 
-          expect(issue.visible_to_user?(user)).to eq(false)
+          it_behaves_like 'issue readable by user'
+          it_behaves_like 'confidential issue readable by user'
         end
       end
 
-      context 'using an internal project' do
-        let(:project) { create(:project, :internal) }
-
-        context 'using an internal user' do
-          it 'returns true for a regular issue' do
-            issue = build(:issue, project: project)
+      context 'with a user that is not a member' do
+        context 'using a public project' do
+          let(:project) { build(:project, :public) }
 
-            expect(issue.visible_to_user?(user)).to eq(true)
+          it_behaves_like 'issue readable by user'
+          it_behaves_like 'confidential issue not readable by user'
         end
 
-          it 'returns false for a confidential issue' do
-            issue = build(:issue, :confidential, project: project)
+        context 'using an internal project' do
+          let(:project) { build(:project, :internal) }
 
-            expect(issue.visible_to_user?(user)).to eq(false)
+          context 'using an internal user' do
+            before do
+              allow(user).to receive(:external?).and_return(false)
             end
+
+            it_behaves_like 'issue readable by user'
+            it_behaves_like 'confidential issue not readable by user'
           end
 
           context 'using an external user' do
@@ -619,132 +667,120 @@ describe Issue do
               allow(user).to receive(:external?).and_return(true)
             end
 
-          it 'returns false for a regular issue' do
-            issue = build(:issue, project: project)
-
-            expect(issue.visible_to_user?(user)).to eq(false)
+            it_behaves_like 'issue not readable by user'
+            it_behaves_like 'confidential issue not readable by user'
+          end
         end
 
-          it 'returns false for a confidential issue' do
-            issue = build(:issue, :confidential, project: project)
-
-            expect(issue.visible_to_user?(user)).to eq(false)
+        context 'using an external user' do
+          before do
+            allow(user).to receive(:external?).and_return(true)
           end
+
+          it_behaves_like 'issue not readable by user'
+          it_behaves_like 'confidential issue not readable by user'
         end
       end
 
-      context 'using a private project' do
-        let(:project) { create(:project, :private) }
-
-        it 'returns false for a regular issue' do
-          issue = build(:issue, project: project)
-
-          expect(issue.visible_to_user?(user)).to eq(false)
+      context 'with an external authentication service' do
+        before do
+          enable_external_authorization_service_check
         end
 
-        it 'returns false for a confidential issue' do
-          issue = build(:issue, :confidential, project: project)
+        it 'is `false` when an external authorization service is enabled' do
+          issue = build(:issue, project: build(:project, :public))
 
-          expect(issue.visible_to_user?(user)).to eq(false)
+          expect(issue).not_to be_visible_to_user
         end
 
-        context 'when the user is the project owner' do
-          before do
-            project.add_maintainer(user)
+        it 'checks the external service to determine if an issue is readable by a user' do
+          project = build(:project, :public,
+                          external_authorization_classification_label: 'a-label')
+          issue = build(:issue, project: project)
+          user = build(:user)
+
+          expect(::Gitlab::ExternalAuthorization).to receive(:access_allowed?).with(user, 'a-label') { false }
+          expect(issue.visible_to_user?(user)).to be_falsy
         end
 
-          it 'returns true for a regular issue' do
+        it 'does not check the external service if a user does not have access to the project' do
+          project = build(:project, :private,
+                          external_authorization_classification_label: 'a-label')
           issue = build(:issue, project: project)
+          user = build(:user)
 
-            expect(issue.visible_to_user?(user)).to eq(true)
+          expect(::Gitlab::ExternalAuthorization).not_to receive(:access_allowed?)
+          expect(issue.visible_to_user?(user)).to be_falsy
         end
 
-          it 'returns true for a confidential issue' do
-            issue = build(:issue, :confidential, project: project)
+        it 'does not check the external webservice for admins' do
+          issue = build(:issue)
+          user = build(:admin)
 
-            expect(issue.visible_to_user?(user)).to eq(true)
-          end
-        end
+          expect(::Gitlab::ExternalAuthorization).not_to receive(:access_allowed?)
+
+          issue.visible_to_user?(user)
         end
       end
 
-    context 'with a regular user that is a team member' do
-      let(:user) { create(:user) }
-      let(:project) { create(:project, :public) }
+      context 'when issue is moved to a private project' do
+        let(:private_project) { build(:project, :private)}
 
-      context 'using a public project' do
         before do
-          project.add_developer(user)
+          issue.update(project: private_project) # move issue to private project
         end
 
-        it 'returns true for a regular issue' do
-          issue = build(:issue, project: project)
-
-          expect(issue.visible_to_user?(user)).to eq(true)
+        shared_examples 'issue visible if user has guest access' do
+          context 'when user is not a member' do
+            it_behaves_like 'issue not readable by user'
+            it_behaves_like 'confidential issue not readable by user'
           end
 
-        it 'returns true for a confidential issue' do
-          issue = build(:issue, :confidential, project: project)
+          context 'when user is a guest' do
+            before do
+              private_project.add_guest(user)
+            end
 
-          expect(issue.visible_to_user?(user)).to eq(true)
+            it_behaves_like 'issue readable by user'
+            it_behaves_like 'confidential issue readable by user'
           end
         end
 
-      context 'using an internal project' do
-        let(:project) { create(:project, :internal) }
-
+        context 'when user is the author of the original issue' do
           before do
-          project.add_developer(user)
+            issue.update!(author: user)
           end
 
-        it 'returns true for a regular issue' do
-          issue = build(:issue, project: project)
-
-          expect(issue.visible_to_user?(user)).to eq(true)
+          it_behaves_like 'issue visible if user has guest access'
         end
 
-        it 'returns true for a confidential issue' do
-          issue = build(:issue, :confidential, project: project)
-
-          expect(issue.visible_to_user?(user)).to eq(true)
-        end
+        context 'when user is an assignee in the original issue' do
+          before do
+            issue.update!(assignees: [user])
           end
 
-      context 'using a private project' do
-        let(:project) { create(:project, :private) }
+          it_behaves_like 'issue visible if user has guest access'
+        end
 
+        context 'when user is not the author or an assignee in original issue' do
+          context 'when user is a guest' do
             before do
-          project.add_developer(user)
+              private_project.add_guest(user)
             end
 
-        it 'returns true for a regular issue' do
-          issue = build(:issue, project: project)
-
-          expect(issue.visible_to_user?(user)).to eq(true)
+            it_behaves_like 'issue readable by user'
+            it_behaves_like 'confidential issue not readable by user'
           end
 
-        it 'returns true for a confidential issue' do
-          issue = build(:issue, :confidential, project: project)
-
-          expect(issue.visible_to_user?(user)).to eq(true)
-        end
-      end
+          context 'when user is a reporter' do
+            before do
+              private_project.add_reporter(user)
             end
 
-    context 'with an admin user' do
-      let(:project) { create(:project) }
-      let(:user) { create(:admin) }
-
-      it 'returns true for a regular issue' do
-        issue = build(:issue, project: project)
-
-        expect(issue.visible_to_user?(user)).to eq(true)
+            it_behaves_like 'issue readable by user'
+            it_behaves_like 'confidential issue readable by user'
+          end
         end
-
-      it 'returns true for a confidential issue' do
-        issue = build(:issue, :confidential, project: project)
-
-        expect(issue.visible_to_user?(user)).to eq(true)
       end
     end
   end
@@ -868,49 +904,6 @@ describe Issue do
     subject { create(:issue, updated_at: 1.hour.ago) }
   end
 
-  context 'when an external authentication service' do
-    before do
-      enable_external_authorization_service_check
-    end
-
-    describe '#visible_to_user?' do
-      it 'is `false` when an external authorization service is enabled' do
-        issue = build(:issue, project: build(:project, :public))
-
-        expect(issue).not_to be_visible_to_user
-      end
-
-      it 'checks the external service to determine if an issue is readable by a user' do
-        project = build(:project, :public,
-                        external_authorization_classification_label: 'a-label')
-        issue = build(:issue, project: project)
-        user = build(:user)
-
-        expect(::Gitlab::ExternalAuthorization).to receive(:access_allowed?).with(user, 'a-label') { false }
-        expect(issue.visible_to_user?(user)).to be_falsy
-      end
-
-      it 'does not check the external service if a user does not have access to the project' do
-        project = build(:project, :private,
-                        external_authorization_classification_label: 'a-label')
-        issue = build(:issue, project: project)
-        user = build(:user)
-
-        expect(::Gitlab::ExternalAuthorization).not_to receive(:access_allowed?)
-        expect(issue.visible_to_user?(user)).to be_falsy
-      end
-
-      it 'does not check the external webservice for admins' do
-        issue = build(:issue)
-        user = build(:admin)
-
-        expect(::Gitlab::ExternalAuthorization).not_to receive(:access_allowed?)
-
-        issue.visible_to_user?(user)
-      end
-    end
-  end
-
   describe "#labels_hook_attrs" do
     let(:label) { create(:label) }
     let(:issue) { create(:labeled_issue, labels: [label]) }

spec/policies/issue_policy_spec.rb

@@ -103,12 +103,24 @@ describe IssuePolicy do
         expect(permissions(author, confidential_issue_no_assignee)).to be_disallowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue)
       end
 
+      it 'does not allow issue author to read or update confidential issue moved to an private project' do
+        confidential_issue.project = build(:project, :private)
+
+        expect(permissions(author, confidential_issue)).to be_disallowed(:read_issue, :read_issue_iid, :update_issue)
+      end
+
       it 'allows issue assignees to read and update their confidential issues' do
         expect(permissions(assignee, confidential_issue)).to be_allowed(:read_issue, :read_issue_iid, :update_issue)
         expect(permissions(assignee, confidential_issue)).to be_disallowed(:admin_issue)
 
         expect(permissions(assignee, confidential_issue_no_assignee)).to be_disallowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue)
       end
+
+      it 'does not allow issue assignees to read or update confidential issue moved to an private project' do
+        confidential_issue.project = build(:project, :private)
+
+        expect(permissions(assignee, confidential_issue)).to be_disallowed(:read_issue, :read_issue_iid, :update_issue)
+      end
     end
   end