Commit 027b5b06 authored by Andreas Brandl's avatar Andreas Brandl

Merge branch '34417-add-resolved-state-to-vulns' into 'master'

Add 'resolved' state and resolution tracking to Vulnerabilities

See merge request gitlab-org/gitlab!19498
parents 6fcef956 573cbeb1
# frozen_string_literal: true
class AddResolvedAttributesToVulnerabilities < ActiveRecord::Migration[5.2]
DOWNTIME = false
def up
add_column :vulnerabilities, :resolved_by_id, :bigint
add_column :vulnerabilities, :resolved_at, :datetime_with_timezone
end
def down
remove_column :vulnerabilities, :resolved_at
remove_column :vulnerabilities, :resolved_by_id
end
end
# frozen_string_literal: true
class AddForeignKeyOnResolvedByIdToVulnerabilities < ActiveRecord::Migration[5.2]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
disable_ddl_transaction!
def up
add_concurrent_index :vulnerabilities, :resolved_by_id
add_concurrent_foreign_key :vulnerabilities, :users, column: :resolved_by_id, on_delete: :nullify
end
def down
remove_foreign_key :vulnerabilities, column: :resolved_by_id
remove_concurrent_index :vulnerabilities, :resolved_by_id
end
end
# frozen_string_literal: true
class SetResolvedStateOnVulnerabilities < ActiveRecord::Migration[5.2]
DOWNTIME = false
def up
execute <<~SQL
-- selecting IDs for all non-orphan Findings that either have no feedback or it's a non-dismissal feedback
WITH resolved_vulnerability_ids AS (
SELECT DISTINCT vulnerability_id AS id
FROM vulnerability_occurrences
LEFT JOIN vulnerability_feedback ON vulnerability_feedback.project_fingerprint = ENCODE(vulnerability_occurrences.project_fingerprint::bytea, 'HEX')
WHERE vulnerability_id IS NOT NULL
AND (vulnerability_feedback.id IS NULL OR vulnerability_feedback.feedback_type <> 0)
)
UPDATE vulnerabilities
SET state = 3, resolved_by_id = closed_by_id, resolved_at = NOW()
FROM resolved_vulnerability_ids
WHERE vulnerabilities.id IN (resolved_vulnerability_ids.id)
AND state = 2 -- only 'closed' Vulnerabilities become 'resolved'
SQL
end
def down
execute <<~SQL
UPDATE vulnerabilities
SET state = 2, resolved_by_id = NULL, resolved_at = NULL -- state = 'closed'
WHERE state = 3 -- 'resolved'
SQL
end
end
......@@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema.define(version: 2019_11_12_232338) do
ActiveRecord::Schema.define(version: 2019_11_14_173624) do
# These are extensions that must be enabled in order to support this database
enable_extension "pg_trgm"
......@@ -3952,6 +3952,8 @@ ActiveRecord::Schema.define(version: 2019_11_12_232338) do
t.boolean "severity_overridden", default: false
t.integer "confidence", limit: 2, null: false
t.boolean "confidence_overridden", default: false
t.bigint "resolved_by_id"
t.datetime_with_timezone "resolved_at"
t.integer "report_type", limit: 2, null: false
t.integer "cached_markdown_version"
t.index ["author_id"], name: "index_vulnerabilities_on_author_id"
......@@ -3961,6 +3963,7 @@ ActiveRecord::Schema.define(version: 2019_11_12_232338) do
t.index ["last_edited_by_id"], name: "index_vulnerabilities_on_last_edited_by_id"
t.index ["milestone_id"], name: "index_vulnerabilities_on_milestone_id"
t.index ["project_id"], name: "index_vulnerabilities_on_project_id"
t.index ["resolved_by_id"], name: "index_vulnerabilities_on_resolved_by_id"
t.index ["start_date_sourcing_milestone_id"], name: "index_vulnerabilities_on_start_date_sourcing_milestone_id"
t.index ["updated_by_id"], name: "index_vulnerabilities_on_updated_by_id"
end
......@@ -4518,6 +4521,7 @@ ActiveRecord::Schema.define(version: 2019_11_12_232338) do
add_foreign_key "vulnerabilities", "users", column: "author_id", name: "fk_b1de915a15", on_delete: :nullify
add_foreign_key "vulnerabilities", "users", column: "closed_by_id", name: "fk_cf5c60acbf", on_delete: :nullify
add_foreign_key "vulnerabilities", "users", column: "last_edited_by_id", name: "fk_1302949740", on_delete: :nullify
add_foreign_key "vulnerabilities", "users", column: "resolved_by_id", name: "fk_76bc5f5455", on_delete: :nullify
add_foreign_key "vulnerabilities", "users", column: "updated_by_id", name: "fk_7ac31eacb9", on_delete: :nullify
add_foreign_key "vulnerability_feedback", "ci_pipelines", column: "pipeline_id", on_delete: :nullify
add_foreign_key "vulnerability_feedback", "issues", on_delete: :nullify
......
......@@ -138,9 +138,11 @@ module Vulnerabilities
return 'dismissed' if dismissal_feedback.present?
if vulnerability.nil?
'new'
elsif vulnerability.closed?
'opened'
elsif vulnerability.resolved?
'resolved'
elsif vulnerability.closed? # fail-safe check for cases when dismissal feedback was lost or was not created
'dismissed'
else
'confirmed'
end
......
......@@ -19,11 +19,12 @@ class Vulnerability < ApplicationRecord
belongs_to :author, class_name: 'User' # keep this association named 'author' for correct work of markdown cache
belongs_to :updated_by, class_name: 'User'
belongs_to :last_edited_by, class_name: 'User'
belongs_to :resolved_by, class_name: 'User'
belongs_to :closed_by, class_name: 'User'
has_many :findings, class_name: 'Vulnerabilities::Occurrence', inverse_of: :vulnerability
enum state: { opened: 1, closed: 2 }
enum state: { opened: 1, closed: 2, resolved: 3 }
enum severity: Vulnerabilities::Occurrence::SEVERITY_LEVELS, _prefix: :severity
enum confidence: Vulnerabilities::Occurrence::CONFIDENCE_LEVELS, _prefix: :confidence
enum report_type: Vulnerabilities::Occurrence::REPORT_TYPES
......
......@@ -23,7 +23,7 @@ module Vulnerabilities
raise ActiveRecord::Rollback
end
@vulnerability.update(state: :closed, closed_by: @user, closed_at: Time.zone.now)
@vulnerability.update(state: :closed, closed_by: @user, closed_at: Time.current)
end
@vulnerability
......
......@@ -13,7 +13,7 @@ module Vulnerabilities
raise Gitlab::Access::AccessDeniedError unless can?(@user, :resolve_vulnerability, @vulnerability.project)
@vulnerability.tap do |vulnerability|
vulnerability.update(state: :closed, closed_by: @user, closed_at: Time.zone.now)
vulnerability.update(state: :resolved, resolved_by: @user, resolved_at: Time.current)
end
end
end
......
......@@ -43,7 +43,7 @@ module API
end
post ':id/resolve' do
vulnerability = find_and_authorize_vulnerability!(:resolve_vulnerability)
break not_modified! if vulnerability.closed?
break not_modified! if vulnerability.resolved?
vulnerability = ::Vulnerabilities::ResolveService.new(current_user, vulnerability).execute
render_vulnerability(vulnerability)
......
......@@ -915,6 +915,7 @@ module EE
expose :author_id
expose :updated_by_id
expose :last_edited_by_id
expose :resolved_by_id
expose :closed_by_id
expose :start_date
......@@ -923,6 +924,7 @@ module EE
expose :created_at
expose :updated_at
expose :last_edited_at
expose :resolved_at
expose :closed_at
end
end
......
......@@ -14,9 +14,14 @@ FactoryBot.define do
state { :opened }
end
trait :resolved do
state { :resolved }
resolved_at { Time.current }
end
trait :closed do
state { :closed }
closed_at { Time.now }
closed_at { Time.current }
end
trait :with_findings do
......
......@@ -45,18 +45,26 @@ FactoryBot.define do
trait :resolved do
after(:create) do |finding|
create(:vulnerability, :closed, project: finding.project, findings: [finding])
create(:vulnerability, :resolved, project: finding.project, findings: [finding])
end
end
trait :dismissed do
after(:create) do |finding|
create(:vulnerability, :closed, project: finding.project, findings: [finding])
create(:vulnerability_feedback,
:dismissal,
project: finding.project,
project_fingerprint: finding.project_fingerprint)
end
end
trait :with_issue_feedback do
after(:create) do |finding|
create(:vulnerability_feedback,
:issue,
project: finding.project,
project_fingerprint: finding.project_fingerprint)
end
end
end
end
......@@ -286,7 +286,7 @@ describe Security::PipelineVulnerabilitiesFinder do
expect(confirmed.state).to eq 'confirmed'
expect(resolved.state).to eq 'resolved'
expect(dismissed.state).to eq 'dismissed'
expect(subject - [confirmed, resolved, dismissed]).to all have_attributes(state: 'new')
expect(subject - [confirmed, resolved, dismissed]).to all have_attributes(state: 'opened')
end
end
......
......@@ -6,7 +6,7 @@
"type": "string"
},
"description": { "type": ["string", "null"] },
"state": { "type": "string", "enum": ["opened", "closed"] },
"state": { "type": "string", "enum": ["opened", "resolved", "closed"] },
"severity": {
"type": "string",
"enum": ["undefined", "info", "unknown", "low", "medium", "high", "critical"]
......@@ -37,12 +37,14 @@
"author_id": { "type": "integer" },
"updated_by_id": { "type": ["integer", "null"] },
"last_edited_by_id": { "type": ["integer", "null"] },
"resolved_by_id": { "type": ["integer", "null"] },
"closed_by_id": { "type": ["integer", "null"] },
"start_date": { "type": ["date", "null"] },
"due_date": { "type": ["date", "null"] },
"created_at": { "type": "date" },
"updated_at": { "type": "date" },
"last_edited_at": { "type": "date" },
"resolved_at": { "type": "date" },
"closed_at": { "type": "date" }
}
}
......@@ -55,6 +55,10 @@
{ "type": "null" },
{ "$ref": "../vulnerability_feedback.json" }
]},
"state" : {
"type": "string",
"enum": ["opened", "confirmed", "resolved", "dismissed"]
},
"description": { "type": ["string", "null"] },
"solution": { "type": ["string", "null"] },
"location" : {
......
# frozen_string_literal: true
require 'spec_helper'
require Rails.root.join('db', 'post_migrate', '20191114173624_set_resolved_state_on_vulnerabilities.rb')
describe SetResolvedStateOnVulnerabilities, :migration do
PACK_FORMAT = 'H*'
let(:confidence_levels) do
{ undefined: 0, ignore: 1, unknown: 2, experimental: 3, low: 4, medium: 5, high: 6, confirmed: 7 }
end
let(:severity_levels) { { undefined: 0, info: 1, unknown: 2, low: 4, medium: 5, high: 6, critical: 7 } }
let(:states) { { opened: 1, closed: 2, resolved: 3 } }
let(:report_types) { { sast: 0, dependency_scanning: 1, container_scanning: 2, dast: 3 } }
let(:feedback_types) { { dismissal: 0, issue: 1 } }
let(:users) { table(:users) }
let(:namespaces) { table(:namespaces) }
let(:projects) { table(:projects) }
let(:scanners) { table(:vulnerability_scanners) }
let(:identifiers) { table(:vulnerability_identifiers) }
let(:findings) { table(:vulnerability_occurrences) }
let(:vulnerabilities) { table(:vulnerabilities) }
let(:issues) { table(:issues) }
let(:feedbacks) { table(:vulnerability_feedback) }
let(:vulnerability_open_id) { 1 }
let(:vulnerability_resolved_id) { 2 }
let(:vulnerability_dismissed_id) { 3 }
let(:vulnerability_orphan_id) { 4 }
let(:vulnerability_non_dismissal_feedback_id) { 5 }
let(:closer_id) { 2 }
def fingerprint
hash = String.new('', capacity: 40)
40.times { hash << rand(16).to_s(16) }
hash
end
def bin_fingerprint
[fingerprint].pack(PACK_FORMAT)
end
def bin_to_str_fingerprint(bin_data)
bin_data.unpack1(PACK_FORMAT)
end
before do
author = users.create!(id: 1, email: 'author@example.com', projects_limit: 10)
closer = users.create!(id: closer_id, email: 'closer@example.com', projects_limit: 10)
namespace = namespaces.create!(id: 1, name: 'namespace_1', path: 'namespace_1', owner_id: author.id)
project = projects.create!(id: 1, creator_id: author.id, namespace_id: namespace.id)
vulnerabilities_common_attrs = { project_id: project.id, author_id: author.id, severity: severity_levels[:high],
confidence: confidence_levels[:medium], report_type: report_types[:sast] }
vulnerability_open = vulnerabilities.create!(
id: vulnerability_open_id, state: states[:opened], title: 'finding_open', title_html: 'finding_open', **vulnerabilities_common_attrs)
vulnerability_resolved = vulnerabilities.create!(
id: vulnerability_resolved_id, state: states[:closed], title: 'finding_resolved', title_html: 'finding_resolved', closed_by_id: closer.id,
**vulnerabilities_common_attrs)
vulnerability_dismissed = vulnerabilities.create!(
id: vulnerability_dismissed_id, state: states[:closed], title: 'finding_dismissed', title_html: 'finding_dismissed',
closed_by_id: closer.id, **vulnerabilities_common_attrs)
vulnerabilities.create!(
id: vulnerability_orphan_id, state: states[:closed], title: 'vulnerability_orphan', title_html: 'vulnerability_orphan',
closed_by_id: closer.id, **vulnerabilities_common_attrs)
vulnerability_non_dismissal_feedback = vulnerabilities.create!(
id: vulnerability_non_dismissal_feedback_id, state: states[:closed], title: 'finding_non_dismissal_feedback',
title_html: 'finding_non_dismissal_feedback', closed_by_id: closer.id, **vulnerabilities_common_attrs)
scanner = scanners.create!(id: 1, project_id: project.id, name: 'scanner', external_id: 'SCANNER_ID')
identifiers_common_attrs = { project_id: project.id, external_type: 'SECURITY_ID' }
identifier_1 = identifiers.create!(
id: 1, fingerprint: '1111111111111111111111111111111111111111', external_id: 'SECURITY_1',
name: 'SECURITY_IDENTIFIER 1', **identifiers_common_attrs)
identifier_2 = identifiers.create!(
id: 2, fingerprint: '2222222222222222222222222222222222222222', external_id: 'SECURITY_2',
name: 'SECURITY_IDENTIFIER 2', **identifiers_common_attrs)
identifier_3 = identifiers.create!(
id: 3, fingerprint: '3333333333333333333333333333333333333333', external_id: 'SECURITY_3',
name: 'SECURITY_IDENTIFIER 3', **identifiers_common_attrs)
identifier_4 = identifiers.create!(
id: 4, fingerprint: '4444444444444444444444444444444444444444', external_id: 'SECURITY_4',
name: 'SECURITY_IDENTIFIER 4', **identifiers_common_attrs)
findings_common_attrs = { project_id: project.id, scanner_id: scanner.id, severity: severity_levels[:high],
confidence: confidence_levels[:medium], metadata_version: 'sast:1.0', raw_metadata: '{}' }
findings.create!(
id: 1, report_type: report_types[:sast], name: 'finding_confirmed', primary_identifier_id: identifier_1.id,
uuid: fingerprint[0..35], project_fingerprint: bin_fingerprint, location_fingerprint: bin_fingerprint,
vulnerability_id: vulnerability_open.id, **findings_common_attrs)
findings.create!(
id: 2, report_type: report_types[:dependency_scanning], name: 'finding_resolved',
primary_identifier_id: identifier_2.id, uuid: fingerprint[0..35], project_fingerprint: bin_fingerprint,
location_fingerprint: bin_fingerprint, vulnerability_id: vulnerability_resolved.id, **findings_common_attrs)
finding_dismissed = findings.create!(
id: 3, report_type: report_types[:container_scanning], name: 'finding_dismissed',
primary_identifier_id: identifier_3.id, uuid: fingerprint[0..35], project_fingerprint: bin_fingerprint,
location_fingerprint: bin_fingerprint, vulnerability_id: vulnerability_dismissed.id, **findings_common_attrs)
finding_non_dismissal_feedback = findings.create!(
id: 4, report_type: report_types[:dast], name: 'finding_non_dismissal_feedback',
primary_identifier_id: identifier_4.id, uuid: fingerprint[0..35], project_fingerprint: bin_fingerprint,
location_fingerprint: bin_fingerprint, vulnerability_id: vulnerability_non_dismissal_feedback.id,
**findings_common_attrs)
issue = issues.create!(id: 1, title: 'Fix the vulnerability', project_id: project.id, author_id: author.id)
feedbacks.create!(
id: 1, project_id: project.id, author_id: author.id, category: finding_dismissed.report_type,
feedback_type: feedback_types[:dismissal],
project_fingerprint: bin_to_str_fingerprint(finding_dismissed.project_fingerprint))
feedbacks.create!(
id: 2, project_id: project.id, author_id: author.id, category: finding_non_dismissal_feedback.report_type,
feedback_type: feedback_types[:issue], issue_id: issue.id,
project_fingerprint: bin_to_str_fingerprint(finding_non_dismissal_feedback.project_fingerprint))
end
def find(id)
vulnerabilities.find(id)
end
describe '#up' do
it 'sets "resolved" state only for resolved vulnerabilities' do
Timecop.freeze do
migrate!
expect(find(vulnerability_open_id)).to have_attributes(state: states[:opened], resolved_by_id: nil, resolved_at: nil)
expect(find(vulnerability_resolved_id)).to(
have_attributes(state: states[:resolved], resolved_by_id: closer_id, resolved_at: be_like_time(Time.current)))
expect(find(vulnerability_dismissed_id)).to have_attributes(state: states[:closed], resolved_by_id: nil, resolved_at: nil)
expect(find(vulnerability_orphan_id)).to have_attributes(state: states[:closed], resolved_by_id: nil, resolved_at: nil)
expect(find(vulnerability_non_dismissal_feedback_id)).to(
have_attributes(state: states[:resolved], resolved_by_id: closer_id, resolved_at: be_like_time(Time.current)))
end
end
end
describe '#down' do
it 'rolls back the migration correctly' do
expect(find(vulnerability_open_id)).to have_attributes(state: states[:opened], resolved_by_id: nil, resolved_at: nil)
expect(find(vulnerability_resolved_id)).to have_attributes(state: states[:closed], resolved_by_id: nil, resolved_at: nil)
expect(find(vulnerability_dismissed_id)).to have_attributes(state: states[:closed], resolved_by_id: nil, resolved_at: nil)
expect(find(vulnerability_orphan_id)).to have_attributes(state: states[:closed], resolved_by_id: nil, resolved_at: nil)
expect(find(vulnerability_non_dismissal_feedback_id)).to(
have_attributes(state: states[:closed], resolved_by_id: nil, resolved_at: nil))
migrate!
schema_migrate_down!
expect(find(vulnerability_open_id)).to have_attributes(state: states[:opened], resolved_by_id: nil, resolved_at: nil)
expect(find(vulnerability_resolved_id)).to have_attributes(state: states[:closed], resolved_by_id: nil, resolved_at: nil)
expect(find(vulnerability_dismissed_id)).to have_attributes(state: states[:closed], resolved_by_id: nil, resolved_at: nil)
expect(find(vulnerability_orphan_id)).to have_attributes(state: states[:closed], resolved_by_id: nil, resolved_at: nil)
expect(find(vulnerability_non_dismissal_feedback_id)).to(
have_attributes(state: states[:closed], resolved_by_id: nil, resolved_at: nil))
end
end
end
......@@ -437,13 +437,18 @@ describe Vulnerabilities::Occurrence do
end
describe '#state' do
let(:new_finding) { create(:vulnerabilities_finding) }
before do
create(:vulnerability, :closed, project: finding_with_issue.project, findings: [finding_with_issue])
end
let(:unresolved_finding) { create(:vulnerabilities_finding) }
let(:confirmed_finding) { create(:vulnerabilities_finding, :confirmed) }
let(:resolved_finding) { create(:vulnerabilities_finding, :resolved) }
let(:dismissed_finding) { create(:vulnerabilities_finding, :dismissed) }
let(:finding_with_issue) { create(:vulnerabilities_finding, :with_issue_feedback) }
it 'returns the expected state for a new finding' do
expect(new_finding.state).to eq 'new'
it 'returns the expected state for a unresolved finding' do
expect(unresolved_finding.state).to eq 'opened'
end
it 'returns the expected state for a confirmed finding' do
......
......@@ -3,27 +3,11 @@
require 'spec_helper'
describe Vulnerability do
let(:state_values) do
{ opened: 1, closed: 2 }
end
let(:severity_values) do
{ undefined: 0,
info: 1,
unknown: 2,
low: 4,
medium: 5,
high: 6,
critical: 7 }
end
let(:state_values) { { opened: 1, closed: 2, resolved: 3 } }
let(:severity_values) { { undefined: 0, info: 1, unknown: 2, low: 4, medium: 5, high: 6, critical: 7 } }
let(:confidence_values) do
{ undefined: 0,
ignore: 1,
unknown: 2,
experimental: 3,
low: 4,
medium: 5,
high: 6,
confirmed: 7 }
{ undefined: 0, ignore: 1, unknown: 2, experimental: 3, low: 4, medium: 5, high: 6, confirmed: 7 }
end
let(:report_types) do
{ sast: 0,
......@@ -47,6 +31,7 @@ describe Vulnerability do
it { is_expected.to belong_to(:author).class_name('User') }
it { is_expected.to belong_to(:updated_by).class_name('User') }
it { is_expected.to belong_to(:last_edited_by).class_name('User') }
it { is_expected.to belong_to(:resolved_by).class_name('User') }
it { is_expected.to belong_to(:closed_by).class_name('User') }
it { is_expected.to have_many(:findings).class_name('Vulnerabilities::Occurrence').dependent(false) }
......
......@@ -178,7 +178,7 @@ describe API::Vulnerabilities do
expect(response).to match_response_schema('public_api/v4/vulnerability', dir: 'ee')
expect(vulnerability.reload).to(
have_attributes(state: 'closed', closed_by: user, closed_at: be_like_time(Time.zone.now)))
have_attributes(state: 'closed', closed_by: user, closed_at: be_like_time(Time.current)))
expect(vulnerability.findings).to all have_vulnerability_dismissal_feedback
end
end
......@@ -262,13 +262,13 @@ describe API::Vulnerabilities do
expect(response).to match_response_schema('public_api/v4/vulnerability', dir: 'ee')
expect(vulnerability.reload).to(
have_attributes(state: 'closed', closed_by: user, closed_at: be_like_time(Time.zone.now)))
have_attributes(state: 'resolved', resolved_by: user, resolved_at: be_like_time(Time.current)))
expect(vulnerability.findings).to all have_attributes(state: 'resolved')
end
end
context 'when the vulnerability is already resolved' do
let(:vulnerability) { create(:vulnerability, :closed, project: project) }
let(:vulnerability) { create(:vulnerability, :resolved, project: project) }
it 'responds with 304 Not Modified response' do
resolve_vulnerability
......
......@@ -26,7 +26,7 @@ describe Vulnerabilities::DismissService do
dismiss_vulnerability
expect(vulnerability.reload).to(
have_attributes(state: 'closed', closed_by: user, closed_at: be_like_time(Time.zone.now)))
have_attributes(state: 'closed', closed_by: user, closed_at: be_like_time(Time.current)))
expect(vulnerability.findings).to all have_vulnerability_dismissal_feedback
end
end
......
......@@ -26,7 +26,7 @@ describe Vulnerabilities::ResolveService do
resolve_vulnerability
expect(vulnerability.reload).to(
have_attributes(state: 'closed', closed_by: user, closed_at: be_like_time(Time.zone.now)))
have_attributes(state: 'resolved', resolved_by: user, resolved_at: be_like_time(Time.current)))
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