From d8eb7e9fcb8f921f9de57888c202b379a13d23a8 Mon Sep 17 00:00:00 2001
From: mfluharty <mfluharty@gitlab.com>
Date: Wed, 16 Oct 2019 15:36:28 -0600
Subject: [PATCH 1/6] Implement Resolve Vulnerability API call

Add resolve_vulnerability ability for User.
Add POST /vulnerabilities/:id/dismiss API.
Add computed state to Findings. Update
the related finder and associations to
support that computed state.
---
 .../pipeline_vulnerabilities_finder.rb        | 19 ++++-
 ee/app/models/vulnerabilities/occurrence.rb   | 25 ++++++-
 ee/app/models/vulnerability.rb                |  1 -
 ee/app/policies/ee/project_policy.rb          |  3 +
 .../vulnerabilities/occurrence_entity.rb      |  2 +
 ee/lib/api/vulnerabilities.rb                 | 17 ++++-
 .../factories/vulnerabilities/occurrences.rb  | 24 ++++++-
 .../pipeline_vulnerabilities_finder_spec.rb   | 48 +++++++++++++
 .../models/vulnerabilities/occurrence_spec.rb | 43 +++++++++++
 ee/spec/policies/project_policy_spec.rb       | 11 ++-
 ee/spec/requests/api/vulnerabilities_spec.rb  | 72 +++++++++++++++++++
 .../api/vulnerabilities_shared_examples.rb    |  2 +-
 12 files changed, 257 insertions(+), 10 deletions(-)

diff --git a/ee/app/finders/security/pipeline_vulnerabilities_finder.rb b/ee/app/finders/security/pipeline_vulnerabilities_finder.rb
index 5f111f6681320a..4818d21737e39e 100644
--- a/ee/app/finders/security/pipeline_vulnerabilities_finder.rb
+++ b/ee/app/finders/security/pipeline_vulnerabilities_finder.rb
@@ -33,7 +33,9 @@ def execute
 
         raise ParseError, 'JSON parsing failed' if report.error.is_a?(Gitlab::Ci::Parsers::Security::Common::SecurityReportParserError)
 
-        normalized_occurrences = normalize_report_occurrences(report.occurrences)
+        normalized_occurrences = normalize_report_occurrences(
+          report.occurrences,
+          vulnerabilities_by_finding_fingerprint(type, report))
         filtered_occurrences = filter(normalized_occurrences)
 
         occurrences.concat(filtered_occurrences)
@@ -48,12 +50,25 @@ def pipeline_reports
       pipeline&.security_reports&.reports
     end
 
-    def normalize_report_occurrences(report_occurrences)
+    def vulnerabilities_by_finding_fingerprint(report_type, report)
+      Vulnerabilities::Occurrence
+        .with_vulnerabilities_for_state(
+          project: pipeline.project,
+          report_type: report_type,
+          project_fingerprints: report.occurrences.map(&:project_fingerprint))
+       .each_with_object({}) do |occurrence, hash|
+        hash[occurrence.project_fingerprint] = occurrence.vulnerability
+      end
+    end
+
+    def normalize_report_occurrences(report_occurrences, vulnerabilities)
       report_occurrences.map do |report_occurrence|
         occurrence_hash = report_occurrence.to_hash
           .except(:compare_key, :identifiers, :location, :scanner)
 
         occurrence = Vulnerabilities::Occurrence.new(occurrence_hash)
+        # assigning Vulnerabilities to Findings to enable the computed state
+        occurrence.vulnerability = vulnerabilities[occurrence.project_fingerprint]
 
         occurrence.project = pipeline.project
         occurrence.build_scanner(report_occurrence.scanner.to_hash)
diff --git a/ee/app/models/vulnerabilities/occurrence.rb b/ee/app/models/vulnerabilities/occurrence.rb
index aef4a8a1b3844a..c2194ec11f883e 100644
--- a/ee/app/models/vulnerabilities/occurrence.rb
+++ b/ee/app/models/vulnerabilities/occurrence.rb
@@ -18,7 +18,7 @@ class Occurrence < ApplicationRecord
     belongs_to :project
     belongs_to :scanner, class_name: 'Vulnerabilities::Scanner'
     belongs_to :primary_identifier, class_name: 'Vulnerabilities::Identifier', inverse_of: :primary_occurrences
-    belongs_to :vulnerability, inverse_of: :findings
+    belongs_to :vulnerability, -> { select('vulnerabilities.id, vulnerabilities.state') }, inverse_of: :findings
 
     has_many :occurrence_identifiers, class_name: 'Vulnerabilities::OccurrenceIdentifier'
     has_many :identifiers, through: :occurrence_identifiers, class_name: 'Vulnerabilities::Identifier'
@@ -115,6 +115,29 @@ def self.counted_by_severity
       end
     end
 
+    def self.with_vulnerabilities_for_state(project:, report_type:, project_fingerprints:)
+      Vulnerabilities::Occurrence
+        .includes(:vulnerability, :identifiers)
+        .where(
+          project: project,
+          report_type: report_type,
+          project_fingerprint: project_fingerprints
+        )
+        .select('report_type, vulnerability_id, project_fingerprint, raw_metadata') # fetching only required attributes
+    end
+
+    def state
+      return 'dismissed' if dismissal_feedback.present?
+
+      if vulnerability.nil?
+        'new'
+      elsif vulnerability.closed?
+        'resolved'
+      else
+        'confirmed'
+      end
+    end
+
     def feedback(feedback_type:)
       params = {
         project_id: project_id,
diff --git a/ee/app/models/vulnerability.rb b/ee/app/models/vulnerability.rb
index 12957adc731d5e..95f5d40f4bc847 100644
--- a/ee/app/models/vulnerability.rb
+++ b/ee/app/models/vulnerability.rb
@@ -10,7 +10,6 @@ class Vulnerability < ApplicationRecord
   belongs_to :last_edited_by, class_name: 'User'
   belongs_to :closed_by, class_name: 'User'
 
-  # TODO: temporary, remove when https://gitlab.com/gitlab-org/gitlab/merge_requests/18283 is merged and rebased onto
   has_many :findings, class_name: 'Vulnerabilities::Occurrence', inverse_of: :vulnerability
 
   enum state: { opened: 1, closed: 2 }
diff --git a/ee/app/policies/ee/project_policy.rb b/ee/app/policies/ee/project_policy.rb
index 968d3abeaa45cd..f5568d334d317f 100644
--- a/ee/app/policies/ee/project_policy.rb
+++ b/ee/app/policies/ee/project_policy.rb
@@ -151,11 +151,13 @@ module ProjectPolicy
 
       rule { can?(:developer_access) }.policy do
         enable :read_project_security_dashboard
+        enable :resolve_vulnerability
         enable :dismiss_vulnerability
       end
 
       rule { security_dashboard_feature_disabled }.policy do
         prevent :read_project_security_dashboard
+        prevent :resolve_vulnerability
         prevent :dismiss_vulnerability
       end
 
@@ -201,6 +203,7 @@ module ProjectPolicy
         enable :read_deployment
         enable :read_pages
         enable :read_project_security_dashboard
+        enable :resolve_vulnerability
         enable :dismiss_vulnerability
       end
 
diff --git a/ee/app/serializers/vulnerabilities/occurrence_entity.rb b/ee/app/serializers/vulnerabilities/occurrence_entity.rb
index bab04f95b51e98..239dd24cfd1cb3 100644
--- a/ee/app/serializers/vulnerabilities/occurrence_entity.rb
+++ b/ee/app/serializers/vulnerabilities/occurrence_entity.rb
@@ -31,6 +31,8 @@ class Vulnerabilities::OccurrenceEntity < Grape::Entity
     expose :solution
   end
 
+  expose :state
+
   expose :blob_path do |occurrence|
     occurrence.present.blob_path
   end
diff --git a/ee/lib/api/vulnerabilities.rb b/ee/lib/api/vulnerabilities.rb
index 93a19ad1bfe450..0c950ba22a88a7 100644
--- a/ee/lib/api/vulnerabilities.rb
+++ b/ee/lib/api/vulnerabilities.rb
@@ -38,6 +38,21 @@ def render_vulnerability(vulnerability)
       requires :id, type: String, desc: 'The ID of a vulnerability'
     end
     resource :vulnerabilities do
+      desc 'Resolve a vulnerability' do
+        success VulnerabilityEntity
+      end
+      post ':id/resolve' do
+        if Feature.enabled?(:first_class_vulnerabilities)
+          vulnerability = find_and_authorize_vulnerability!(:resolve_vulnerability)
+          break not_modified! if vulnerability.closed?
+
+          vulnerability.update(state: 'closed')
+          render_vulnerability(vulnerability)
+        else
+          not_found!
+        end
+      end
+
       desc 'Dismiss a vulnerability' do
         success VulnerabilityEntity
       end
@@ -61,7 +76,7 @@ def render_vulnerability(vulnerability)
       params do
         # These params have no effect for Vulnerabilities API but are required to support falling back to
         # responding with Vulnerability Findings when :first_class_vulnerabilities feature is disabled.
-        # TODO: remove usage of :vulnerability_findings_params when feature flag is removed
+        # TODO: replace :vulnerability_findings_params with just :pagination when feature flag is removed
         # https://gitlab.com/gitlab-org/gitlab/issues/33488
         use :vulnerability_findings_params
       end
diff --git a/ee/spec/factories/vulnerabilities/occurrences.rb b/ee/spec/factories/vulnerabilities/occurrences.rb
index 05e0f1cbcebf94..c2143fa69fc697 100644
--- a/ee/spec/factories/vulnerabilities/occurrences.rb
+++ b/ee/spec/factories/vulnerabilities/occurrences.rb
@@ -5,7 +5,7 @@
     Digest::SHA1.hexdigest("uuid-#{n}")[0..35]
   end
 
-  factory :vulnerabilities_occurrence, class: Vulnerabilities::Occurrence do
+  factory :vulnerabilities_occurrence, class: Vulnerabilities::Occurrence, aliases: [:vulnerabilities_finding] do
     name { 'Cipher with no integrity' }
     project
     sequence(:uuid) { generate(:vulnerability_occurrence_uuid) }
@@ -36,5 +36,27 @@
         ]
       }.to_json
     end
+
+    trait :confirmed do
+      after(:create) do |finding|
+        create(:vulnerability, :opened, project: finding.project, findings: [finding])
+      end
+    end
+
+    trait :resolved do
+      after(:create) do |finding|
+        create(:vulnerability, :closed, 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
   end
 end
diff --git a/ee/spec/finders/security/pipeline_vulnerabilities_finder_spec.rb b/ee/spec/finders/security/pipeline_vulnerabilities_finder_spec.rb
index 52a146c2125593..cd19809e270c78 100644
--- a/ee/spec/finders/security/pipeline_vulnerabilities_finder_spec.rb
+++ b/ee/spec/finders/security/pipeline_vulnerabilities_finder_spec.rb
@@ -218,6 +218,54 @@ def disable_deduplication
       end
     end
 
+    context 'when matching vulnerability records exist' do
+      before do
+        create(:vulnerabilities_finding,
+               :confirmed,
+               project: project,
+               report_type: 'sast',
+               project_fingerprint: confirmed_fingerprint)
+        create(:vulnerabilities_finding,
+               :resolved,
+               project: project,
+               report_type: 'sast',
+               project_fingerprint: resolved_fingerprint)
+        create(:vulnerabilities_finding,
+               :dismissed,
+               project: project,
+               report_type: 'sast',
+               project_fingerprint: dismissed_fingerprint)
+      end
+
+      let(:confirmed_fingerprint) do
+        Digest::SHA1.hexdigest(
+          'python/hardcoded/hardcoded-tmp.py:52865813c884a507be1f152d654245af34aba8a391626d01f1ab6d3f52ec8779:B108')
+      end
+
+      let(:resolved_fingerprint) do
+        Digest::SHA1.hexdigest(
+          'groovy/src/main/java/com/gitlab/security_products/tests/App.groovy:47:PREDICTABLE_RANDOM')
+      end
+
+      let(:dismissed_fingerprint) do
+        Digest::SHA1.hexdigest(
+          'groovy/src/main/java/com/gitlab/security_products/tests/App.groovy:41:PREDICTABLE_RANDOM')
+      end
+
+      subject { described_class.new(pipeline: pipeline, params: { report_type: %w[sast], scope: 'all' }).execute }
+
+      it 'assigns vulnerability records to findings providing them with computed state' do
+        confirmed = subject.find { |f| f.project_fingerprint == confirmed_fingerprint }
+        resolved = subject.find { |f| f.project_fingerprint == resolved_fingerprint }
+        dismissed = subject.find { |f| f.project_fingerprint == dismissed_fingerprint }
+
+        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')
+      end
+    end
+
     def read_fixture(fixture)
       JSON.parse(File.read(fixture.file.path))
     end
diff --git a/ee/spec/models/vulnerabilities/occurrence_spec.rb b/ee/spec/models/vulnerabilities/occurrence_spec.rb
index 513546e12fabee..05bffacfd3899c 100644
--- a/ee/spec/models/vulnerabilities/occurrence_spec.rb
+++ b/ee/spec/models/vulnerabilities/occurrence_spec.rb
@@ -356,4 +356,47 @@
       end
     end
   end
+
+  describe '#state' do
+    let(:new_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) }
+
+    it 'returns the expected state for a new finding' do
+      expect(new_finding.state).to eq 'new'
+    end
+
+    it 'returns the expected state for a confirmed finding' do
+      expect(confirmed_finding.state).to eq 'confirmed'
+    end
+
+    it 'returns the expected state for a resolved finding' do
+      expect(resolved_finding.state).to eq 'resolved'
+    end
+
+    it 'returns the expected state for a dismissed finding' do
+      expect(dismissed_finding.state).to eq 'dismissed'
+    end
+
+    context 'when a vulnerability present for a dismissed finding' do
+      before do
+        create(:vulnerability, project: dismissed_finding.project, findings: [dismissed_finding])
+      end
+
+      it 'still reports a dismissed state' do
+        expect(dismissed_finding.state).to eq 'dismissed'
+      end
+    end
+
+    context 'when a non-dismissal feedback present for a finding belonging to a closed vulnerability' do
+      before do
+        create(:vulnerability_feedback, :issue, project: resolved_finding.project)
+      end
+
+      it 'reports as resolved' do
+        expect(resolved_finding.state).to eq 'resolved'
+      end
+    end
+  end
 end
diff --git a/ee/spec/policies/project_policy_spec.rb b/ee/spec/policies/project_policy_spec.rb
index f6ca81eb41594a..86fd61fb648905 100644
--- a/ee/spec/policies/project_policy_spec.rb
+++ b/ee/spec/policies/project_policy_spec.rb
@@ -30,7 +30,12 @@
       %i[read_issue_link read_software_license_policy]
     end
     let(:additional_reporter_permissions) { [:admin_issue_link] }
-    let(:additional_developer_permissions) { %i[admin_vulnerability_feedback read_project_security_dashboard read_feature_flag dismiss_vulnerability] }
+    let(:additional_developer_permissions) do
+      %i[
+        admin_vulnerability_feedback read_project_security_dashboard read_feature_flag
+        resolve_vulnerability dismiss_vulnerability
+      ]
+    end
     let(:additional_maintainer_permissions) { %i[push_code_to_protected_branches admin_feature_flags_client] }
     let(:auditor_permissions) do
       %i[
@@ -41,9 +46,8 @@
         read_pipeline read_build read_commit_status read_container_image
         read_environment read_deployment read_merge_request read_pages
         create_merge_request_in award_emoji
-        read_project_security_dashboard
+        read_project_security_dashboard resolve_vulnerability dismiss_vulnerability
         read_vulnerability_feedback read_software_license_policy
-        dismiss_vulnerability
       ]
     end
 
@@ -490,6 +494,7 @@
 
         include_context 'when security dashboard feature is not available'
 
+        it { is_expected.to be_disallowed(:resolve_vulnerability) }
         it { is_expected.to be_disallowed(:dismiss_vulnerability) }
       end
     end
diff --git a/ee/spec/requests/api/vulnerabilities_spec.rb b/ee/spec/requests/api/vulnerabilities_spec.rb
index a036746eab1d07..eb0313d06fe5f3 100644
--- a/ee/spec/requests/api/vulnerabilities_spec.rb
+++ b/ee/spec/requests/api/vulnerabilities_spec.rb
@@ -147,4 +147,76 @@
       end
     end
   end
+
+  describe "POST /vulnerabilities:id/resolve" do
+    before do
+      create_list(:vulnerabilities_finding, 2, vulnerability: vulnerability)
+    end
+
+    let(:vulnerability) { project.vulnerabilities.first }
+
+    subject { post api("/vulnerabilities/#{vulnerability.id}/resolve", user) }
+
+    context 'with an authorized user with proper permissions' do
+      before do
+        project.add_developer(user)
+      end
+
+      it 'dismisses a vulnerability and its associated findings' do
+        subject
+
+        expect(response).to have_gitlab_http_status(201)
+        expect(response).to match_response_schema('vulnerability', dir: 'ee')
+
+        expect(vulnerability.reload).to be_closed
+        expect(vulnerability.findings).to all have_attributes(state: 'resolved')
+      end
+
+      context 'when the vulnerability is already resolved' do
+        let(:vulnerability) { create(:vulnerability, :closed, project: project) }
+
+        it 'responds with 304 Not Modified response' do
+          subject
+
+          expect(response).to have_gitlab_http_status(304)
+        end
+      end
+
+      context 'and when security dashboard feature is not available' do
+        before do
+          stub_licensed_features(security_dashboard: false)
+        end
+
+        it 'responds with 403 Forbidden' do
+          subject
+
+          expect(response).to have_gitlab_http_status(403)
+        end
+      end
+    end
+
+    context 'when user does not have permissions to create a dismissal feedback' do
+      before do
+        project.add_reporter(user)
+      end
+
+      it 'responds with 403 Forbidden' do
+        subject
+
+        expect(response).to have_gitlab_http_status(403)
+      end
+    end
+
+    context 'when first-class vulnerabilities feature is disabled' do
+      before do
+        stub_feature_flags(first_class_vulnerabilities: false)
+      end
+
+      it 'responds with 404 Not Found' do
+        subject
+
+        expect(response).to have_gitlab_http_status(404)
+      end
+    end
+  end
 end
diff --git a/ee/spec/support/shared_examples/requests/api/vulnerabilities_shared_examples.rb b/ee/spec/support/shared_examples/requests/api/vulnerabilities_shared_examples.rb
index 02e3b404d36287..beb7f25af0c961 100644
--- a/ee/spec/support/shared_examples/requests/api/vulnerabilities_shared_examples.rb
+++ b/ee/spec/support/shared_examples/requests/api/vulnerabilities_shared_examples.rb
@@ -108,7 +108,7 @@
         get api(project_vulnerabilities_path, user), params: { report_type: 'dependency_scanning' }
       end.count
 
-      expect { get api(project_vulnerabilities_path, user) }.not_to exceed_query_limit(control_count)
+      expect { get api(project_vulnerabilities_path, user) }.not_to exceed_query_limit(control_count).with_threshold(1)
     end
 
     describe 'filtering' do
-- 
GitLab


From 456a8d908c140f637e76d49fbfc516319e3ad221 Mon Sep 17 00:00:00 2001
From: Victor Zagorodny <vzagorodny@gitlab.com>
Date: Fri, 18 Oct 2019 14:59:09 +0300
Subject: [PATCH 2/6] Improve with_vulnerabilities_for_state query

---
 ee/app/models/vulnerabilities/occurrence.rb | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/ee/app/models/vulnerabilities/occurrence.rb b/ee/app/models/vulnerabilities/occurrence.rb
index c2194ec11f883e..0b015562ba9bad 100644
--- a/ee/app/models/vulnerabilities/occurrence.rb
+++ b/ee/app/models/vulnerabilities/occurrence.rb
@@ -18,7 +18,7 @@ class Occurrence < ApplicationRecord
     belongs_to :project
     belongs_to :scanner, class_name: 'Vulnerabilities::Scanner'
     belongs_to :primary_identifier, class_name: 'Vulnerabilities::Identifier', inverse_of: :primary_occurrences
-    belongs_to :vulnerability, -> { select('vulnerabilities.id, vulnerabilities.state') }, inverse_of: :findings
+    belongs_to :vulnerability, inverse_of: :findings
 
     has_many :occurrence_identifiers, class_name: 'Vulnerabilities::OccurrenceIdentifier'
     has_many :identifiers, through: :occurrence_identifiers, class_name: 'Vulnerabilities::Identifier'
@@ -117,13 +117,14 @@ def self.counted_by_severity
 
     def self.with_vulnerabilities_for_state(project:, report_type:, project_fingerprints:)
       Vulnerabilities::Occurrence
-        .includes(:vulnerability, :identifiers)
+        .joins(:vulnerability)
         .where(
           project: project,
           report_type: report_type,
           project_fingerprint: project_fingerprints
         )
-        .select('report_type, vulnerability_id, project_fingerprint, raw_metadata') # fetching only required attributes
+        .select('report_type, vulnerability_id, project_fingerprint, raw_metadata, '\
+                'vulnerabilities.id, vulnerabilities.state') # fetching only required attributes
     end
 
     def state
-- 
GitLab


From 6e2558b1fd7bcc7feb5db3743d3e795ccffe6000 Mon Sep 17 00:00:00 2001
From: Victor Zagorodny <vzagorodny@gitlab.com>
Date: Fri, 18 Oct 2019 15:00:04 +0300
Subject: [PATCH 3/6] Update closed_by and closed_at on V. dismissal

---
 ee/app/services/vulnerabilities/dismiss_service.rb   | 10 +++++-----
 ee/spec/requests/api/vulnerabilities_spec.rb         | 12 +++++++-----
 .../services/vulnerabilities/dismiss_service_spec.rb |  8 +++++---
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/ee/app/services/vulnerabilities/dismiss_service.rb b/ee/app/services/vulnerabilities/dismiss_service.rb
index ba1d41307be071..5bf7524acc6896 100644
--- a/ee/app/services/vulnerabilities/dismiss_service.rb
+++ b/ee/app/services/vulnerabilities/dismiss_service.rb
@@ -6,14 +6,14 @@ class DismissService
 
     FindingsDismissResult = Struct.new(:ok?, :finding, :message)
 
-    def initialize(current_user, vulnerability)
-      @current_user = current_user
+    def initialize(user, vulnerability)
+      @user = user
       @vulnerability = vulnerability
       @project = vulnerability.project
     end
 
     def execute
-      raise Gitlab::Access::AccessDeniedError unless can?(@current_user, :dismiss_vulnerability, @project)
+      raise Gitlab::Access::AccessDeniedError unless can?(@user, :dismiss_vulnerability, @project)
 
       @vulnerability.transaction do
         result = dismiss_findings
@@ -23,7 +23,7 @@ def execute
           raise ActiveRecord::Rollback
         end
 
-        @vulnerability.update(state: 'closed')
+        @vulnerability.update(state: :closed, closed_by: @user, closed_at: Time.zone.now)
       end
 
       @vulnerability
@@ -32,7 +32,7 @@ def execute
     private
 
     def feedback_service_for(finding)
-      VulnerabilityFeedback::CreateService.new(@project, @current_user, feedback_params_for(finding))
+      VulnerabilityFeedback::CreateService.new(@project, @user, feedback_params_for(finding))
     end
 
     def feedback_params_for(finding)
diff --git a/ee/spec/requests/api/vulnerabilities_spec.rb b/ee/spec/requests/api/vulnerabilities_spec.rb
index eb0313d06fe5f3..6d54f3e273d1ac 100644
--- a/ee/spec/requests/api/vulnerabilities_spec.rb
+++ b/ee/spec/requests/api/vulnerabilities_spec.rb
@@ -61,13 +61,15 @@
       end
 
       it 'dismisses a vulnerability and its associated findings' do
-        subject
+        Timecop.freeze do
+          subject
 
-        expect(response).to have_gitlab_http_status(201)
-        expect(response).to match_response_schema('vulnerability', dir: 'ee')
+          expect(response).to have_gitlab_http_status(201)
+          expect(response).to match_response_schema('vulnerability', dir: 'ee')
 
-        expect(vulnerability.reload).to be_closed
-        expect(vulnerability.findings).to all have_vulnerability_dismissal_feedback
+          expect(vulnerability.reload).to have_attributes(state: 'closed', closed_by: user, closed_at: Time.zone.now)
+          expect(vulnerability.findings).to all have_vulnerability_dismissal_feedback
+        end
       end
 
       context 'when there is a dismissal error' do
diff --git a/ee/spec/services/vulnerabilities/dismiss_service_spec.rb b/ee/spec/services/vulnerabilities/dismiss_service_spec.rb
index e70fc493050d90..99d3d8abbe8cb6 100644
--- a/ee/spec/services/vulnerabilities/dismiss_service_spec.rb
+++ b/ee/spec/services/vulnerabilities/dismiss_service_spec.rb
@@ -20,10 +20,12 @@
     end
 
     it 'dismisses a vulnerability and its associated findings' do
-      subject
+      Timecop.freeze do
+        subject
 
-      expect(vulnerability.reload).to be_closed
-      expect(vulnerability.findings).to all have_vulnerability_dismissal_feedback
+        expect(vulnerability.reload).to have_attributes(state: 'closed', closed_by: user, closed_at: Time.zone.now)
+        expect(vulnerability.findings).to all have_vulnerability_dismissal_feedback
+      end
     end
 
     context 'when there is a finding dismissal error' do
-- 
GitLab


From f099020cec07b88056556a7c40a5ce99fa1fb4c5 Mon Sep 17 00:00:00 2001
From: Victor Zagorodny <vzagorodny@gitlab.com>
Date: Fri, 18 Oct 2019 15:01:30 +0300
Subject: [PATCH 4/6] Add ResolveService for state, closed_by, closed_at

Add ResolveService to process the "resolve" op
over a Vulnerability and use this service in the
POST /vulnerabilites/:id/resolve API operation
---
 .../vulnerabilities/resolve_service.rb        | 20 ++++++++
 ee/lib/api/vulnerabilities.rb                 |  2 +-
 ee/spec/requests/api/vulnerabilities_spec.rb  | 16 +++---
 .../vulnerabilities/resolve_service_spec.rb   | 50 +++++++++++++++++++
 4 files changed, 80 insertions(+), 8 deletions(-)
 create mode 100644 ee/app/services/vulnerabilities/resolve_service.rb
 create mode 100644 ee/spec/services/vulnerabilities/resolve_service_spec.rb

diff --git a/ee/app/services/vulnerabilities/resolve_service.rb b/ee/app/services/vulnerabilities/resolve_service.rb
new file mode 100644
index 00000000000000..16adea5f091938
--- /dev/null
+++ b/ee/app/services/vulnerabilities/resolve_service.rb
@@ -0,0 +1,20 @@
+# frozen_string_literal: true
+
+module Vulnerabilities
+  class ResolveService
+    include Gitlab::Allowable
+
+    def initialize(user, vulnerability)
+      @user = user
+      @vulnerability = vulnerability
+    end
+
+    def execute
+      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)
+      end
+    end
+  end
+end
diff --git a/ee/lib/api/vulnerabilities.rb b/ee/lib/api/vulnerabilities.rb
index 0c950ba22a88a7..ec381915c21f5b 100644
--- a/ee/lib/api/vulnerabilities.rb
+++ b/ee/lib/api/vulnerabilities.rb
@@ -46,7 +46,7 @@ def render_vulnerability(vulnerability)
           vulnerability = find_and_authorize_vulnerability!(:resolve_vulnerability)
           break not_modified! if vulnerability.closed?
 
-          vulnerability.update(state: 'closed')
+          vulnerability = ::Vulnerabilities::ResolveService.new(current_user, vulnerability).execute
           render_vulnerability(vulnerability)
         else
           not_found!
diff --git a/ee/spec/requests/api/vulnerabilities_spec.rb b/ee/spec/requests/api/vulnerabilities_spec.rb
index 6d54f3e273d1ac..e92aaf6469d85e 100644
--- a/ee/spec/requests/api/vulnerabilities_spec.rb
+++ b/ee/spec/requests/api/vulnerabilities_spec.rb
@@ -164,14 +164,16 @@
         project.add_developer(user)
       end
 
-      it 'dismisses a vulnerability and its associated findings' do
-        subject
+      it 'resolves a vulnerability and its associated findings' do
+        Timecop.freeze do
+          subject
 
-        expect(response).to have_gitlab_http_status(201)
-        expect(response).to match_response_schema('vulnerability', dir: 'ee')
+          expect(response).to have_gitlab_http_status(201)
+          expect(response).to match_response_schema('vulnerability', dir: 'ee')
 
-        expect(vulnerability.reload).to be_closed
-        expect(vulnerability.findings).to all have_attributes(state: 'resolved')
+          expect(vulnerability.reload).to have_attributes(state: 'closed', closed_by: user, closed_at: Time.zone.now)
+          expect(vulnerability.findings).to all have_attributes(state: 'resolved')
+        end
       end
 
       context 'when the vulnerability is already resolved' do
@@ -197,7 +199,7 @@
       end
     end
 
-    context 'when user does not have permissions to create a dismissal feedback' do
+    context 'when user does not have permissions to resolve a vulnerability' do
       before do
         project.add_reporter(user)
       end
diff --git a/ee/spec/services/vulnerabilities/resolve_service_spec.rb b/ee/spec/services/vulnerabilities/resolve_service_spec.rb
new file mode 100644
index 00000000000000..c2140ec0fd8780
--- /dev/null
+++ b/ee/spec/services/vulnerabilities/resolve_service_spec.rb
@@ -0,0 +1,50 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Vulnerabilities::ResolveService do
+  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, project: project) }
+  let(:service) { described_class.new(user, vulnerability) }
+
+  subject { service.execute }
+
+  context 'with an authorized user with proper permissions' do
+    before do
+      project.add_developer(user)
+    end
+
+    it 'resolves a vulnerability' do
+      Timecop.freeze do
+        subject
+
+        expect(vulnerability.reload).to have_attributes(state: 'closed', closed_by: user, closed_at: Time.zone.now)
+      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 { subject }.to raise_error(Gitlab::Access::AccessDeniedError)
+      end
+    end
+  end
+
+  context 'when user does not have rights to dismiss a vulnerability' do
+    before do
+      project.add_reporter(user)
+    end
+
+    it 'raises an "access denied" error' do
+      expect { subject }.to raise_error(Gitlab::Access::AccessDeniedError)
+    end
+  end
+end
-- 
GitLab


From fc47cc5b8cb3d7534fbd57a2569287cb6b20f03c Mon Sep 17 00:00:00 2001
From: Victor Zagorodny <vzagorodny@gitlab.com>
Date: Fri, 18 Oct 2019 15:01:53 +0300
Subject: [PATCH 5/6] Add note on with_threshold usage reasoning

---
 .../requests/api/vulnerabilities_shared_examples.rb             | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ee/spec/support/shared_examples/requests/api/vulnerabilities_shared_examples.rb b/ee/spec/support/shared_examples/requests/api/vulnerabilities_shared_examples.rb
index beb7f25af0c961..1f9282e365ff22 100644
--- a/ee/spec/support/shared_examples/requests/api/vulnerabilities_shared_examples.rb
+++ b/ee/spec/support/shared_examples/requests/api/vulnerabilities_shared_examples.rb
@@ -108,6 +108,8 @@
         get api(project_vulnerabilities_path, user), params: { report_type: 'dependency_scanning' }
       end.count
 
+      # Threshold is required for the extra query performed in Security::PipelineVulnerabilitiesFinder to load
+      # the Vulnerabilities providing computed states for the associated Vulnerability::Occurrences
       expect { get api(project_vulnerabilities_path, user) }.not_to exceed_query_limit(control_count).with_threshold(1)
     end
 
-- 
GitLab


From 6749a8cfdadb9d6729e38eb0e674bf229de27b05 Mon Sep 17 00:00:00 2001
From: Victor Zagorodny <vzagorodny@gitlab.com>
Date: Tue, 22 Oct 2019 10:15:58 +0300
Subject: [PATCH 6/6] Change time tests to use be_like_time matcher

---
 ee/spec/requests/api/vulnerabilities_spec.rb             | 6 ++++--
 ee/spec/services/vulnerabilities/dismiss_service_spec.rb | 3 ++-
 ee/spec/services/vulnerabilities/resolve_service_spec.rb | 3 ++-
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/ee/spec/requests/api/vulnerabilities_spec.rb b/ee/spec/requests/api/vulnerabilities_spec.rb
index e92aaf6469d85e..0a4e82d77851ee 100644
--- a/ee/spec/requests/api/vulnerabilities_spec.rb
+++ b/ee/spec/requests/api/vulnerabilities_spec.rb
@@ -67,7 +67,8 @@
           expect(response).to have_gitlab_http_status(201)
           expect(response).to match_response_schema('vulnerability', dir: 'ee')
 
-          expect(vulnerability.reload).to have_attributes(state: 'closed', closed_by: user, closed_at: Time.zone.now)
+          expect(vulnerability.reload).to(
+            have_attributes(state: 'closed', closed_by: user, closed_at: be_like_time(Time.zone.now)))
           expect(vulnerability.findings).to all have_vulnerability_dismissal_feedback
         end
       end
@@ -171,7 +172,8 @@
           expect(response).to have_gitlab_http_status(201)
           expect(response).to match_response_schema('vulnerability', dir: 'ee')
 
-          expect(vulnerability.reload).to have_attributes(state: 'closed', closed_by: user, closed_at: Time.zone.now)
+          expect(vulnerability.reload).to(
+            have_attributes(state: 'closed', closed_by: user, closed_at: be_like_time(Time.zone.now)))
           expect(vulnerability.findings).to all have_attributes(state: 'resolved')
         end
       end
diff --git a/ee/spec/services/vulnerabilities/dismiss_service_spec.rb b/ee/spec/services/vulnerabilities/dismiss_service_spec.rb
index 99d3d8abbe8cb6..9452116f8dd0c9 100644
--- a/ee/spec/services/vulnerabilities/dismiss_service_spec.rb
+++ b/ee/spec/services/vulnerabilities/dismiss_service_spec.rb
@@ -23,7 +23,8 @@
       Timecop.freeze do
         subject
 
-        expect(vulnerability.reload).to have_attributes(state: 'closed', closed_by: user, closed_at: Time.zone.now)
+        expect(vulnerability.reload).to(
+          have_attributes(state: 'closed', closed_by: user, closed_at: be_like_time(Time.zone.now)))
         expect(vulnerability.findings).to all have_vulnerability_dismissal_feedback
       end
     end
diff --git a/ee/spec/services/vulnerabilities/resolve_service_spec.rb b/ee/spec/services/vulnerabilities/resolve_service_spec.rb
index c2140ec0fd8780..db628cf6b6b2b0 100644
--- a/ee/spec/services/vulnerabilities/resolve_service_spec.rb
+++ b/ee/spec/services/vulnerabilities/resolve_service_spec.rb
@@ -23,7 +23,8 @@
       Timecop.freeze do
         subject
 
-        expect(vulnerability.reload).to have_attributes(state: 'closed', closed_by: user, closed_at: Time.zone.now)
+        expect(vulnerability.reload).to(
+          have_attributes(state: 'closed', closed_by: user, closed_at: be_like_time(Time.zone.now)))
       end
     end
 
-- 
GitLab