Merge branch '373808-guard-state-transition' into 'master'

Prevent vulnerability state transition creation when there is no state change

See merge request !99142



Merged-by: Doug Stull <dstull@gitlab.com>
Approved-by: Gregory Havenga <11164960-ghavenga@users.noreply.gitlab.com>
Approved-by: Doug Stull <dstull@gitlab.com>
Co-authored-by: Subashis <schakraborty@gitlab.com>

ee/app/services/vulnerabilities/confirm_service.rb

@@ -7,23 +7,25 @@ class ConfirmService < BaseService
     def execute
       raise Gitlab::Access::AccessDeniedError unless authorized?
 
+      unless @vulnerability.confirmed?
         ApplicationRecord.transaction do
           Vulnerabilities::StateTransition.create!(
             vulnerability: @vulnerability,
             from_state: @vulnerability.state,
-          to_state: Vulnerability.states[:confirmed]
+            to_state: :confirmed
           )
 
           if Feature.enabled?(:deprecate_vulnerabilities_feedback, @vulnerability.project)
-          update_vulnerability_with(state: Vulnerability.states[:confirmed], confirmed_by: @user,
+            update_vulnerability_with(state: :confirmed, confirmed_by: @user,
                                       confirmed_at: Time.current)
           else
-          update_vulnerability_with(state: Vulnerability.states[:confirmed], confirmed_by: @user,
+            update_vulnerability_with(state: :confirmed, confirmed_by: @user,
                                       confirmed_at: Time.current) do
               DestroyDismissalFeedbackService.new(@user, @vulnerability).execute
             end
           end
         end
+      end
 
       @vulnerability
     end

ee/app/services/vulnerabilities/dismiss_service.rb

@@ -16,11 +16,12 @@ def initialize(current_user, vulnerability, comment = nil, dismissal_reason = ni
     def execute
       raise Gitlab::Access::AccessDeniedError unless authorized?
 
-      update_vulnerability_with(state: Vulnerability.states[:dismissed], dismissed_by: @user, dismissed_at: Time.current) do
-        Vulnerabilities::StateTransition.create(
+      unless @vulnerability.dismissed?
+        update_vulnerability_with(state: :dismissed, dismissed_by: @user, dismissed_at: Time.current) do
+          Vulnerabilities::StateTransition.create!(
             vulnerability: @vulnerability,
             from_state: @vulnerability.state,
-          to_state: Vulnerability.states[:dismissed],
+            to_state: :dismissed,
             comment: @comment,
             dismissal_reason: @dismissal_reason
           )
@@ -34,6 +35,7 @@ def execute
             end
           end
         end
+      end
 
       @vulnerability
     end

ee/app/services/vulnerabilities/resolve_service.rb

@@ -7,21 +7,23 @@ class ResolveService < BaseService
     def execute
       raise Gitlab::Access::AccessDeniedError unless authorized?
 
+      unless @vulnerability.resolved?
         ApplicationRecord.transaction do
           Vulnerabilities::StateTransition.create!(
             vulnerability: @vulnerability,
             from_state: @vulnerability.state,
-          to_state: Vulnerability.states[:resolved]
+            to_state: :resolved
           )
 
           if Feature.enabled?(:deprecate_vulnerabilities_feedback, @vulnerability.project)
-          update_vulnerability_with(state: Vulnerability.states[:resolved], resolved_by: @user, resolved_at: Time.current)
+            update_vulnerability_with(state: :resolved, resolved_by: @user, resolved_at: Time.current)
           else
-          update_vulnerability_with(state: Vulnerability.states[:resolved], resolved_by: @user, resolved_at: Time.current) do
+            update_vulnerability_with(state: :resolved, resolved_by: @user, resolved_at: Time.current) do
               DestroyDismissalFeedbackService.new(@user, @vulnerability).execute
             end
           end
         end
+      end
 
       @vulnerability
     end

ee/app/services/vulnerabilities/revert_to_detected_service.rb

@@ -9,21 +9,23 @@ class RevertToDetectedService < BaseService
     def execute
       raise Gitlab::Access::AccessDeniedError unless authorized?
 
+      unless @vulnerability.detected?
         ApplicationRecord.transaction do
           Vulnerabilities::StateTransition.create!(
             vulnerability: @vulnerability,
             from_state: @vulnerability.state,
-          to_state: Vulnerability.states[:detected]
+            to_state: :detected
           )
 
           if Feature.enabled?(:deprecate_vulnerabilities_feedback, @vulnerability.project)
-          update_vulnerability_with(state: Vulnerability.states[:detected], **REVERT_PARAMS)
+            update_vulnerability_with(state: :detected, **REVERT_PARAMS)
           else
-          update_vulnerability_with(state: Vulnerability.states[:detected], **REVERT_PARAMS) do
+            update_vulnerability_with(state: :detected, **REVERT_PARAMS) do
               DestroyDismissalFeedbackService.new(@user, @vulnerability).execute
             end
           end
         end
+      end
 
       @vulnerability
     end

ee/spec/services/vulnerabilities/confirm_service_spec.rb

@@ -22,6 +22,7 @@
       project.add_developer(user)
     end
 
+    context 'when vulnerability state is different from the requested state' do
       it_behaves_like 'calls vulnerability statistics utility services in order'
 
       context 'when feature flag deprecate_vulnerabilities_feedback is disabled' do
@@ -71,6 +72,14 @@
         end
       end
     end
+  end
+
+  context 'when vulnerability state is not different from the requested state' do
+    let(:state) { :confirmed }
+    let(:action) { confirm_vulnerability }
+
+    it_behaves_like 'does not create state transition for same state'
+  end
 
   describe 'permissions' do
     context 'when admin mode is enabled', :enable_admin_mode do

ee/spec/services/vulnerabilities/dismiss_service_spec.rb

@@ -37,6 +37,7 @@
     end
   end
 
+  context 'when vulnerability state is different from the requested state' do
     context 'when deprecate_vulnerabilities_feedback is enabled' do
       before do
         stub_feature_flags(deprecate_vulnerabilities_feedback: true)
@@ -241,6 +242,14 @@
         end
       end
     end
+  end
+
+  context 'when vulnerability state is not different from the requested state' do
+    let(:state) { :dismissed }
+    let(:action) { dismiss_vulnerability }
+
+    it_behaves_like 'does not create state transition for same state'
+  end
 
   describe 'permissions' do
     context 'when admin mode is enabled', :enable_admin_mode do

ee/spec/services/vulnerabilities/resolve_service_spec.rb

@@ -17,6 +17,7 @@
 
   subject(:resolve_vulnerability) { service.execute }
 
+  context 'when vulnerability state is different from the requested state' do
     context 'with an authorized user with proper permissions' do
       before do
         project.add_developer(user)
@@ -71,6 +72,14 @@
         end
       end
     end
+  end
+
+  context 'when vulnerability state is not different from the requested state' do
+    let(:state) { :resolved }
+    let(:action) { resolve_vulnerability }
+
+    it_behaves_like 'does not create state transition for same state'
+  end
 
   describe 'permissions' do
     context 'when admin mode is enabled', :enable_admin_mode do

ee/spec/services/vulnerabilities/revert_to_detected_service_spec.rb

@@ -49,6 +49,7 @@
       project.add_developer(user)
     end
 
+    context 'when vulnerability state is different from the requested state' do
       context 'when vulnerability is dismissed' do
         let(:vulnerability) { create(:vulnerability, :dismissed, :with_findings, project: project) }
 
@@ -101,6 +102,14 @@
         end
       end
     end
+  end
+
+  context 'when vulnerability state is not different from the requested state' do
+    let(:state) { :detected }
+    let(:action) { revert_vulnerability_to_detected }
+
+    it_behaves_like 'does not create state transition for same state'
+  end
 
   describe 'permissions' do
     context 'when admin mode is enabled', :enable_admin_mode do

ee/spec/support/shared_examples/services/vulnerabilities/does_not_create_state_transition_for_same_state.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'does not create state transition for same state' do
+  context 'when vulnerability state is not different from the requested state' do
+    let(:vulnerability) { create(:vulnerability, state, :with_findings, project: project) }
+
+    context 'with an authorized user with proper permissions' do
+      before do
+        project.add_developer(user)
+      end
+
+      it 'does not create a state transition entry' do
+        expect { action }.not_to change(Vulnerabilities::StateTransition, :count)
+      end
+    end
+  end
+end