Replace Vulnerabilities::Feedback.only_valid_feedback without joining ci

This method joins between `ci_*` and non `ci_*` tables and as such we
need to replace this logic with something that does not do this join
before we move `ci_*` tables to a different database.

This method `Vulnerabilities::Feedback.only_valid_feedback` was
introduced originally in
https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/117 to
solve a security vulnerability
https://gitlab.com/gitlab-org/gitlab/-/issues/24932. The basic issue was
that we had a controller that allowed you to create a
`Vulnerability::Feedback` and set the `pipeline_id` of that. The problem
is that for a window of time a user could have set the `pipeline_id` to
be from a different project. We also had another controller that would
present the `path` for a pipeline and as such you could learn the path
of private projects by guessing the `pipeline_id` from other projects.

The original fix used a `join ci_pipelines` to filter out any
`Vulnerabilities::Feedback` that happened to be associated to another
project. We can't do this join anymore. It seemed simpler to change the
security fix to focus on not rendering the `pipeline` if the
`project_id` was belonging to a different project. So instead we just
don't return `pipeline` if the `project_id` is not what it should be
(ie. they hacked the `pipeline_id` to be a different project). This may
present some issue in the frontend but this data should only appear if
the user in the project deliberately created this hack data so we
shouldn't be concerned about the UX impact here.

It's also possible this fix improves performance for this endpoint as it
doesn't need to do a redundant join.

Since this fixes the problem in a slightly different way it also
required us to change the test slightly to assert the `pipeline` is
filtered out now rather than the whole `Feedback` object.

ee/app/controllers/projects/vulnerability_feedback_controller.rb

@@ -17,9 +17,6 @@ class Projects::VulnerabilityFeedbackController < Projects::ApplicationControlle
     # TODO: Move to finder or list service
     @vulnerability_feedback = @project.vulnerability_feedback.with_associations
 
-    # TODO remove once filtered data has been cleaned
-    @vulnerability_feedback = @vulnerability_feedback.only_valid_feedback
-
     if params[:category].present?
       @vulnerability_feedback = @vulnerability_feedback
         .with_category(params[:category])

ee/app/models/vulnerabilities/feedback.rb

@@ -38,13 +38,6 @@ module Vulnerabilities
     after_save :touch_pipeline, if: :for_dismissal?
     after_destroy :touch_pipeline, if: :for_dismissal?
 
-    # TODO remove once filtered data has been cleaned
-    def self.only_valid_feedback
-      pipeline = Ci::Pipeline.arel_table
-      feedback = arel_table
-      joins(:pipeline).where(pipeline[:project_id].eq(feedback[:project_id]))
-    end
-
     def self.find_or_init_for(feedback_params)
       validate_enums(feedback_params)
 

ee/app/serializers/vulnerabilities/feedback_entity.rb

@@ -14,7 +14,8 @@ class Vulnerabilities::FeedbackEntity < Grape::Entity
   end
   expose :finding_uuid
 
-  expose :pipeline, if: -> (feedback, _) { feedback.pipeline.present? } do
+  # TODO: Remove project_id comparison once bad/invalid data has been deleted https://gitlab.com/gitlab-org/gitlab/-/issues/218837
+  expose :pipeline, if: -> (feedback, _) { feedback.pipeline.present? && feedback.pipeline.project_id == feedback.project_id } do
     expose :id do |feedback|
       feedback.pipeline.id
     end

ee/spec/controllers/projects/vulnerability_feedback_controller_spec.rb

@@ -42,8 +42,8 @@ RSpec.describe Projects::VulnerabilityFeedbackController do
         expect(json_response.length).to eq 5
       end
 
-      # TODO remove once filtered data has been cleaned
-      context 'with invalid vulnerability_feedback' do
+      # TODO: Remove once bad/invalid data has been deleted https://gitlab.com/gitlab-org/gitlab/-/issues/218837
+      context 'when the pipeline has been set to another project' do
         let!(:vuln_feedback_in_other_proj) do
           feedback = build(
             :vulnerability_feedback,
@@ -52,14 +52,20 @@ RSpec.describe Projects::VulnerabilityFeedbackController do
             pipeline: create(:ci_pipeline)
           )
 
+          # Simulating vulnerable data that was in the DB before we introduced
+          # the validation preventing
           feedback.save!(validate: false)
+
+          feedback
         end
 
-        it 'ignores feedback in other project' do
+        it 'does not present the pipeline' do
           list_feedbacks
 
           expect(response).to match_response_schema('vulnerability_feedback_list', dir: 'ee')
-          expect(json_response.length).to eq 5
+          expect(json_response.length).to eq 6
+          feedback_with_invalid_pipeline_response = json_response.find { |r| r['id'] == vuln_feedback_in_other_proj.id }
+          expect(feedback_with_invalid_pipeline_response['pipeline']).to be_nil
         end
       end
 

ee/spec/models/vulnerabilities/feedback_spec.rb

@@ -189,26 +189,6 @@ RSpec.describe Vulnerabilities::Feedback do
     end
   end
 
-  # TODO remove once filtered data has been cleaned
-  describe '::only_valid_feedback' do
-    let(:project) { create(:project) }
-    let(:pipeline) { create(:ci_pipeline, project: project) }
-
-    let!(:feedback) { create(:vulnerability_feedback, :dismissal, :sast, project: project, pipeline: pipeline) }
-    let!(:invalid_feedback) do
-      feedback = build(:vulnerability_feedback, project: project, pipeline: create(:ci_pipeline))
-
-      feedback.save(validate: false)
-    end
-
-    it 'filters out invalid feedback' do
-      feedback_records = described_class.only_valid_feedback
-
-      expect(feedback_records.length).to eq 1
-      expect(feedback_records.first).to eq feedback
-    end
-  end
-
   describe '#has_comment?' do
     let_it_be(:project) { create(:project) }