Merge branch '195361-api-call-to-confirm-vulnerability' into 'master'

Resolve "API call to confirm Vulnerability"

See merge request gitlab-org/gitlab!23099

db/migrate/20200203173508_add_confirmed_attributes_to_vulnerabilities.rb

+# frozen_string_literal: true
+
+class AddConfirmedAttributesToVulnerabilities < ActiveRecord::Migration[5.2]
+  DOWNTIME = false
+
+  def change
+    add_column :vulnerabilities, :confirmed_by_id, :bigint
+    add_column :vulnerabilities, :confirmed_at, :datetime_with_timezone
+  end
+end

db/migrate/20200203183508_add_index_for_vulnerability_confirmed_by.rb

+# frozen_string_literal: true
+
+class AddIndexForVulnerabilityConfirmedBy < ActiveRecord::Migration[5.2]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :vulnerabilities, :confirmed_by_id
+    add_concurrent_foreign_key :vulnerabilities, :users, column: :confirmed_by_id, on_delete: :nullify
+  end
+
+  def down
+    remove_foreign_key :vulnerabilities, column: :confirmed_by_id
+    remove_concurrent_index :vulnerabilities, :confirmed_by_id
+  end
+end

db/schema.rb

@@ -4356,8 +4356,11 @@ ActiveRecord::Schema.define(version: 2020_02_13_204737) do
     t.datetime_with_timezone "resolved_at"
     t.integer "report_type", limit: 2, null: false
     t.integer "cached_markdown_version"
+    t.bigint "confirmed_by_id"
+    t.datetime_with_timezone "confirmed_at"
     t.index ["author_id"], name: "index_vulnerabilities_on_author_id"
     t.index ["closed_by_id"], name: "index_vulnerabilities_on_closed_by_id"
+    t.index ["confirmed_by_id"], name: "index_vulnerabilities_on_confirmed_by_id"
     t.index ["due_date_sourcing_milestone_id"], name: "index_vulnerabilities_on_due_date_sourcing_milestone_id"
     t.index ["epic_id"], name: "index_vulnerabilities_on_epic_id"
     t.index ["last_edited_by_id"], name: "index_vulnerabilities_on_last_edited_by_id"
@@ -5014,6 +5017,7 @@ ActiveRecord::Schema.define(version: 2020_02_13_204737) do
   add_foreign_key "vulnerabilities", "projects", name: "fk_efb96ab1e2", on_delete: :cascade
   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: "confirmed_by_id", name: "fk_959d40ad0a", 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

ee/app/models/vulnerability.rb

@@ -21,6 +21,7 @@ class Vulnerability < ApplicationRecord
   belongs_to :last_edited_by, class_name: 'User'
   belongs_to :resolved_by, class_name: 'User'
   belongs_to :closed_by, class_name: 'User'
+  belongs_to :confirmed_by, class_name: 'User'
 
   has_many :findings, class_name: 'Vulnerabilities::Occurrence', inverse_of: :vulnerability
   has_many :issue_links, class_name: 'Vulnerabilities::IssueLink', inverse_of: :vulnerability
@@ -31,7 +32,7 @@ class Vulnerability < ApplicationRecord
     end
   end
 
-  enum state: { detected: 1, dismissed: 2, resolved: 3 }
+  enum state: { detected: 1, dismissed: 2, resolved: 3, confirmed: 4 }
   enum severity: Vulnerabilities::Occurrence::SEVERITY_LEVELS, _prefix: :severity
   enum confidence: Vulnerabilities::Occurrence::CONFIDENCE_LEVELS, _prefix: :confidence
   enum report_type: Vulnerabilities::Occurrence::REPORT_TYPES

ee/app/services/vulnerabilities/confirm_service.rb

+# frozen_string_literal: true
+
+module Vulnerabilities
+  class ConfirmService
+    include Gitlab::Allowable
+
+    def initialize(user, vulnerability)
+      @user = user
+      @vulnerability = vulnerability
+    end
+
+    def execute
+      raise Gitlab::Access::AccessDeniedError unless can?(@user, :admin_vulnerability, @vulnerability.project)
+
+      @vulnerability.tap do |vulnerability|
+        vulnerability.update(state: Vulnerability.states[:confirmed], confirmed_by: @user, confirmed_at: Time.current)
+      end
+    end
+  end
+end

ee/app/services/vulnerabilities/create_service.rb

@@ -35,7 +35,7 @@ module Vulnerabilities
         author: @author,
         project: @project,
         title: finding.name,
-        state: :detected,
+        state: Vulnerability.states[:detected],
         severity: finding.severity,
         severity_overridden: false,
         confidence: finding.confidence,

ee/app/services/vulnerabilities/dismiss_service.rb

@@ -23,7 +23,7 @@ module Vulnerabilities
           raise ActiveRecord::Rollback
         end
 
-        @vulnerability.update(state: :dismissed, closed_by: @user, closed_at: Time.current)
+        @vulnerability.update(state: Vulnerability.states[:dismissed], closed_by: @user, closed_at: Time.current)
       end
 
       @vulnerability

ee/app/services/vulnerabilities/resolve_service.rb

@@ -13,7 +13,7 @@ module Vulnerabilities
       raise Gitlab::Access::AccessDeniedError unless can?(@user, :admin_vulnerability, @vulnerability.project)
 
       @vulnerability.tap do |vulnerability|
-        vulnerability.update(state: :resolved, resolved_by: @user, resolved_at: Time.current)
+        vulnerability.update(state: Vulnerability.states[:resolved], resolved_by: @user, resolved_at: Time.current)
       end
     end
   end

ee/changelogs/unreleased/23099-api-call-to-confirm-vulnerability.yml

+---
+title: Add API route to confirm a vulnerability
+merge_request: 23099
+author:
+type: added

ee/lib/api/vulnerabilities.rb

@@ -59,6 +59,17 @@ module API
         vulnerability = ::Vulnerabilities::DismissService.new(current_user, vulnerability).execute
         render_vulnerability(vulnerability)
       end
+
+      desc 'Confirm a vulnerability' do
+        success EE::API::Entities::Vulnerability
+      end
+      post ':id/confirm' do
+        vulnerability = find_and_authorize_vulnerability!(:admin_vulnerability)
+        break not_modified! if vulnerability.confirmed?
+
+        vulnerability = ::Vulnerabilities::ConfirmService.new(current_user, vulnerability).execute
+        render_vulnerability(vulnerability)
+      end
     end
 
     params do

ee/spec/factories/vulnerabilities.rb

@@ -11,19 +11,24 @@ FactoryBot.define do
     report_type { :sast }
 
     trait :detected do
-      state { :detected }
+      state { Vulnerability.states[:detected] }
     end
 
     trait :resolved do
-      state { :resolved }
+      state { Vulnerability.states[:resolved] }
       resolved_at { Time.current }
     end
 
     trait :dismissed do
-      state { :dismissed }
+      state { Vulnerability.states[:dismissed] }
       closed_at { Time.current }
     end
 
+    trait :confirmed do
+      state { Vulnerability.states[:confirmed] }
+      confirmed_at { Time.current }
+    end
+
     trait :with_findings do
       after(:build) do |vulnerability|
         vulnerability.findings = build_list(

ee/spec/fixtures/api/schemas/public_api/v4/vulnerability.json

@@ -6,7 +6,7 @@
       "type": "string"
     },
     "description": { "type": ["string", "null"] },
-    "state": { "type": "string", "enum": ["detected", "resolved", "dismissed"] },
+    "state": { "type": "string", "enum": ["detected", "resolved", "dismissed", "confirmed"] },
     "severity": {
       "type": "string",
       "enum": ["undefined", "info", "unknown", "low", "medium", "high", "critical"]

ee/spec/models/vulnerability_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 describe Vulnerability do
-  let(:state_values) { { detected: 1, dismissed: 2, resolved: 3 } }
+  let(:state_values) { { detected: 1, dismissed: 2, resolved: 3, confirmed: 4 } }
   let(:severity_values) { { undefined: 0, info: 1, unknown: 2, low: 4, medium: 5, high: 6, critical: 7 } }
 
   let(:confidence_values) do
@@ -21,6 +21,8 @@ describe Vulnerability do
   it { is_expected.to define_enum_for(:confidence).with_values(confidence_values).with_prefix(:confidence) }
   it { is_expected.to define_enum_for(:report_type).with_values(report_types) }
 
+  it_behaves_like 'having unique enum values'
+
   describe 'associations' do
     subject { build(:vulnerability) }
 
@@ -35,6 +37,7 @@ describe Vulnerability do
     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 belong_to(:confirmed_by).class_name('User') }
 
     it { is_expected.to have_many(:findings).class_name('Vulnerabilities::Occurrence').dependent(false) }
   end

ee/spec/requests/api/vulnerabilities_spec.rb

@@ -318,4 +318,61 @@ describe API::Vulnerabilities do
       it { expect { resolve_vulnerability }.to be_denied_for(:anonymous) }
     end
   end
+
+  describe 'POST /vulnerabilities/:id/confirm' do
+    before do
+      create_list(:vulnerabilities_finding, 2, vulnerability: vulnerability)
+    end
+
+    let_it_be(:project) { create(:project, :with_vulnerabilities) }
+    let(:vulnerability) { project.vulnerabilities.first }
+    let(:vulnerability_id) { vulnerability.id }
+
+    subject(:confirm_vulnerability) { post api("/vulnerabilities/#{vulnerability_id}/confirm", user) }
+
+    context 'with an authorized user with proper permissions' do
+      before do
+        project.add_developer(user)
+      end
+
+      it 'confirms a vulnerability and its associated findings' do
+        Timecop.freeze do
+          confirm_vulnerability
+
+          expect(response).to have_gitlab_http_status(201)
+          expect(response).to match_response_schema('public_api/v4/vulnerability', dir: 'ee')
+
+          expect(vulnerability.reload).to(
+            have_attributes(state: 'confirmed', confirmed_by: user, confirmed_at: be_like_time(Time.current)))
+          expect(vulnerability.findings).to all have_attributes(state: 'confirmed')
+        end
+      end
+
+      it_behaves_like 'responds with "not found" for an unknown vulnerability ID'
+
+      context 'when the vulnerability is already confirmed' do
+        let(:vulnerability) { create(:vulnerability, :confirmed, project: project) }
+
+        it 'responds with 304 Not Modified response' do
+          confirm_vulnerability
+
+          expect(response).to have_gitlab_http_status(304)
+        end
+      end
+
+      it_behaves_like 'forbids access to vulnerability API endpoint in case of disabled features'
+    end
+
+    describe 'permissions' do
+      it { expect { confirm_vulnerability }.to be_allowed_for(:admin) }
+      it { expect { confirm_vulnerability }.to be_allowed_for(:owner).of(project) }
+      it { expect { confirm_vulnerability }.to be_allowed_for(:maintainer).of(project) }
+      it { expect { confirm_vulnerability }.to be_allowed_for(:developer).of(project) }
+
+      it { expect { confirm_vulnerability }.to be_denied_for(:auditor) }
+      it { expect { confirm_vulnerability }.to be_denied_for(:reporter).of(project) }
+      it { expect { confirm_vulnerability }.to be_denied_for(:guest).of(project) }
+      it { expect { confirm_vulnerability }.to be_denied_for(:anonymous) }
+    end
+  end
 end

ee/spec/services/vulnerabilities/confirm_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Vulnerabilities::ConfirmService do
+  include AccessMatchersGeneric
+
+  before do
+    stub_licensed_features(security_dashboard: true)
+  end
+
+  let_it_be(:user) { create(:user) }
+  let(:project) { create(:project) } # cannot use let_it_be here: caching causes problems with permission-related tests
+  let(:vulnerability) { create(:vulnerability, :with_findings, project: project) }
+  let(:service) { described_class.new(user, vulnerability) }
+
+  subject(:confirm_vulnerability) { service.execute }
+
+  context 'with an authorized user with proper permissions' do
+    before do
+      project.add_developer(user)
+    end
+
+    it 'confirms a vulnerability' do
+      Timecop.freeze do
+        confirm_vulnerability
+
+        expect(vulnerability.reload).to(
+          have_attributes(state: 'confirmed', confirmed_by: user, confirmed_at: be_like_time(Time.current)))
+      end
+    end
+
+    context 'when security dashboard feature is disabled' do
+      before do
+        stub_licensed_features(security_dashboard: false)
+      end
+
+      it 'raises an "access denied" error' do
+        expect { confirm_vulnerability }.to raise_error(Gitlab::Access::AccessDeniedError)
+      end
+    end
+  end
+
+  describe 'permissions' do
+    it { expect { confirm_vulnerability }.to be_allowed_for(:admin) }
+    it { expect { confirm_vulnerability }.to be_allowed_for(:owner).of(project) }
+    it { expect { confirm_vulnerability }.to be_allowed_for(:maintainer).of(project) }
+    it { expect { confirm_vulnerability }.to be_allowed_for(:developer).of(project) }
+
+    it { expect { confirm_vulnerability }.to be_denied_for(:auditor) }
+    it { expect { confirm_vulnerability }.to be_denied_for(:reporter).of(project) }
+    it { expect { confirm_vulnerability }.to be_denied_for(:guest).of(project) }
+    it { expect { confirm_vulnerability }.to be_denied_for(:anonymous) }
+  end
+end

spec/db/schema_spec.rb

@@ -55,7 +55,7 @@ describe 'Database schema' do
     members: %w[source_id created_by_id],
     merge_requests: %w[last_edited_by_id state_id],
     namespaces: %w[owner_id parent_id],
-    notes: %w[author_id commit_id noteable_id updated_by_id resolved_by_id discussion_id],
+    notes: %w[author_id commit_id noteable_id updated_by_id resolved_by_id confirmed_by_id discussion_id],
     notification_settings: %w[source_id],
     oauth_access_grants: %w[resource_owner_id application_id],
     oauth_access_tokens: %w[resource_owner_id application_id],