Merge branch 'ee-10161-hide-issuable-urls-on-vulnerability-feedback' into 'master'

EE 10161 hide issuable urls on vulnerability feedback

Closes #10161

See merge request gitlab-org/gitlab-ee!14928

ee/app/assets/javascripts/vue_shared/security_reports/components/modal.vue

@@ -100,9 +100,15 @@ export default {
     issueFeedback() {
       return this.vulnerability && this.vulnerability.issue_feedback;
     },
+    canReadIssueFeedback() {
+      return this.issueFeedback && this.issueFeedback.issue_url;
+    },
     mergeRequestFeedback() {
       return this.vulnerability && this.vulnerability.merge_request_feedback;
     },
+    canReadMergeRequestFeedback() {
+      return this.mergeRequestFeedback && this.mergeRequestFeedback.merge_request_path;
+    },
     dismissalFeedback() {
       return (
         this.vulnerability &&
@@ -204,7 +210,7 @@ export default {
         :vulnerability-feedback-help-path="vulnerabilityFeedbackHelpPath"
       />
 
-      <ul v-if="issueFeedback || mergeRequestFeedback" class="notes card my-4">
+      <ul v-if="canReadIssueFeedback || canReadMergeRequestFeedback" class="notes card my-4">
         <li v-if="issueFeedback" class="note">
           <div class="card-body">
             <issue-note :feedback="issueFeedback" :project="project" />

ee/app/serializers/vulnerabilities/feedback_entity.rb

@@ -27,7 +27,7 @@ class Vulnerabilities::FeedbackEntity < Grape::Entity
     feedback.issue.iid
   end
 
-  expose :issue_url, if: -> (feedback, _) { feedback.issue.present? } do |feedback|
+  expose :issue_url, if: -> (_, _) { can_read_issue? } do |feedback|
     project_issue_url(feedback.project, feedback.issue)
   end
 
@@ -35,7 +35,7 @@ class Vulnerabilities::FeedbackEntity < Grape::Entity
     feedback.merge_request.iid
   end
 
-  expose :merge_request_path, if: -> (feedback, _) { feedback.merge_request.present? } do |feedback|
+  expose :merge_request_path, if: -> (_, _) { can_read_merge_request? } do |feedback|
     project_merge_request_path(feedback.project, feedback.merge_request)
   end
 
@@ -59,4 +59,12 @@ class Vulnerabilities::FeedbackEntity < Grape::Entity
   def can_destroy_feedback?
     can?(request.current_user, :destroy_vulnerability_feedback, feedback)
   end
+
+  def can_read_issue?
+    feedback.issue.present? && can?(request.current_user, :read_issue, feedback.issue)
+  end
+
+  def can_read_merge_request?
+    feedback.merge_request.present? && can?(request.current_user, :read_merge_request, feedback.merge_request)
+  end
 end

ee/spec/frontend/vue_shared/security_reports/components/modal_spec.js

@@ -128,6 +128,82 @@ describe('Security Reports modal', () => {
     });
   });
 
+  describe('related issue read access', () => {
+    describe('with permission to read', () => {
+      beforeEach(() => {
+        const propsData = {
+          modal: createState().modal,
+        };
+
+        propsData.modal.vulnerability.issue_feedback = {
+          issue_url: 'http://issue.url',
+        };
+        wrapper = shallowMount(Component, { propsData });
+      });
+
+      it('displays a link to the issue', () => {
+        const notes = wrapper.find('.notes');
+        expect(notes.exists()).toBe(true);
+      });
+    });
+
+    describe('without permission to read', () => {
+      beforeEach(() => {
+        const propsData = {
+          modal: createState().modal,
+        };
+
+        propsData.modal.vulnerability.issue_feedback = {
+          issue_url: null,
+        };
+        wrapper = shallowMount(Component, { propsData });
+      });
+
+      it('hides the link to the issue', () => {
+        const notes = wrapper.find('.notes');
+        expect(notes.exists()).toBe(false);
+      });
+    });
+  });
+
+  describe('related merge request read access', () => {
+    describe('with permission to read', () => {
+      beforeEach(() => {
+        const propsData = {
+          modal: createState().modal,
+        };
+
+        propsData.modal.vulnerability.merge_request_feedback = {
+          merge_request_path: 'http://mr.url',
+        };
+        wrapper = shallowMount(Component, { propsData });
+      });
+
+      it('displays a link to the merge request', () => {
+        const notes = wrapper.find('.notes');
+        expect(notes.exists()).toBe(true);
+      });
+    });
+
+    describe('without permission to read', () => {
+      beforeEach(() => {
+        const propsData = {
+          modal: createState().modal,
+        };
+
+        propsData.modal.vulnerability.merge_request_feedback = {
+          merge_request_path: null,
+        };
+        wrapper = shallowMount(Component, { propsData });
+      });
+
+      it('hides the link to the merge request', () => {
+        const notes = wrapper.find('.notes');
+        expect(notes.exists()).toBe(false);
+      });
+    });
+  });
+
   describe('with a resolved issue', () => {
     beforeEach(() => {
       const propsData = {

ee/spec/serializers/vulnerabilities/feedback_entity_spec.rb

@@ -22,9 +22,26 @@ describe Vulnerabilities::FeedbackEntity do
     context 'feedback type is issue' do
       let(:feedback) { build(:vulnerability_feedback, :issue, project: project) }
 
-      it 'exposes issue information' do
-        is_expected.to include(:issue_iid)
-        is_expected.to include(:issue_url)
+      context 'when issue is present' do
+        it 'exposes the issue iid' do
+          is_expected.to include(:issue_iid)
+        end
+
+        context 'when user can view issues' do
+          before do
+            project.add_developer(user)
+          end
+
+          it 'exposes issue url' do
+            is_expected.to include(:issue_url)
+          end
+        end
+
+        context 'when user cannot view issues' do
+          it 'does not expose issue url' do
+            is_expected.not_to include(:issue_url)
+          end
+        end
       end
 
       context 'when issue is not present' do
@@ -50,9 +67,26 @@ describe Vulnerabilities::FeedbackEntity do
     context 'feedback type is merge_request' do
       let(:feedback) { build(:vulnerability_feedback, :merge_request, project: project) }
 
-      it 'exposes merge request information' do
-        is_expected.to include(:merge_request_iid)
-        is_expected.to include(:merge_request_path)
+      context 'when merge request is present' do
+        it 'exposes the merge request iid' do
+          is_expected.to include(:merge_request_iid)
+        end
+
+        context 'when user can view merge requests' do
+          before do
+            project.add_developer(user)
+          end
+
+          it 'exposes merge request url' do
+            is_expected.to include(:merge_request_path)
+          end
+        end
+
+        context 'when user cannot view merge requests' do
+          it 'does not expose merge request url' do
+            is_expected.not_to include(:merge_request_path)
+          end
+        end
       end
 
       context 'when merge request is not present' do
@@ -116,40 +150,4 @@ describe Vulnerabilities::FeedbackEntity do
       expect(subject[:comment_details]).to include(:comment_author)
     end
   end
-
-  context 'when issue is present' do
-    let(:feedback) { build(:vulnerability_feedback, :issue ) }
-
-    it 'exposes issue information' do
-      is_expected.to include(:issue_iid)
-      is_expected.to include(:issue_url)
-    end
-  end
-
-  context 'when issue is not present' do
-    let(:feedback) { build(:vulnerability_feedback, feedback_type: 'issue', issue: nil) }
-
-    it 'does not expose issue information' do
-      is_expected.not_to include(:issue_iid)
-      is_expected.not_to include(:issue_url)
-    end
-  end
-
-  context 'when merge request is present' do
-    let(:feedback) { build(:vulnerability_feedback, :merge_request ) }
-
-    it 'exposes merge request information' do
-      is_expected.to include(:merge_request_iid)
-      is_expected.to include(:merge_request_path)
-    end
-  end
-
-  context 'when merge request is not present' do
-    let(:feedback) { build(:vulnerability_feedback, feedback_type: 'merge_request', merge_request: nil) }
-
-    it 'does not expose merge request information' do
-      is_expected.not_to include(:merge_request_iid)
-      is_expected.not_to include(:merge_request_path)
-    end
-  end
 end