Commit 8090f3da authored by Michał Zając's avatar Michał Zając

Find or initialize Scanners using project_id

Add `project_id` as argument to `find_or_initialize_by` so the scanners
are created in the correct project if they don't exist within that
project already.

This prevents the following scenario:

1. No scanner called `gitlab-manual-vulnerability-report` exists in any
   project.
2. Project A creates a vulnerability manually.
3. `gitlab-manual-vulnerability-report` scanner gets created within
   project A.
4. Project B creates a vulnerability manually.
5. Project B users don't see any scanner attached to the finding because
   it's actually attached to the scanner created in step 3 and they
   don't have access to that scanner.

Fixing this 100% will require a data migration which will be performed
later on.

See https://gitlab.com/gitlab-org/gitlab/-/issues/355802

Changelog: fixed
EE: true
parent 64fea52b
......@@ -65,10 +65,9 @@ module Vulnerabilities
# In the GraphQL mutation mutation arguments we want to respect the security scanner schema:
# https://gitlab.com/gitlab-org/security-products/security-report-schemas/-/blob/master/src/security-report-format.json#L339
# So the id provided to the mutation is actually external_id in our database
Vulnerabilities::Scanner.find_or_initialize_by(external_id: scanner_hash[:id]) do |s|
Vulnerabilities::Scanner.find_or_initialize_by(external_id: scanner_hash[:id], project_id: @project.id) do |s|
s.name = scanner_hash[:name]
s.vendor = scanner_hash.dig(:vendor, :name).to_s
s.project = @project
end
end
# rubocop: enable CodeReuse/ActiveRecord
......
......@@ -10,6 +10,7 @@ RSpec.describe Vulnerabilities::ManuallyCreateService do
let_it_be(:user) { create(:user) }
let(:project) { create(:project) } # cannot use let_it_be here: caching causes problems with permission-related tests
let(:different_project) { create(:project) }
subject { described_class.new(project, user, params: params).execute }
......@@ -108,10 +109,21 @@ RSpec.describe Vulnerabilities::ManuallyCreateService do
end
context 'when Scanner already exists' do
let!(:scanner) { create(:vulnerabilities_scanner, external_id: scanner_attributes[:id]) }
let!(:scanner) { create(:vulnerabilities_scanner, external_id: scanner_attributes[:id], project: project) }
it 'does not create a new Scanner' do
expect { subject }.to change(Vulnerabilities::Scanner, :count).by(0)
expect(vulnerability.finding.scanner_id).to eq(scanner.id)
end
end
# See https://gitlab.com/gitlab-org/gitlab/-/issues/355802#note_874700035
context 'when Scanner with the same name exists in a different project' do
let!(:scanner) { create(:vulnerabilities_scanner, external_id: scanner_attributes[:id], project: different_project) }
it 'creates a new Scanner in the correct project', :aggregate_failures do
expect { subject }.to change(Vulnerabilities::Scanner, :count).by(1)
expect(vulnerability.finding.scanner_id).not_to eq(scanner.id)
end
end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment