Merge branch 'sf/feature/GL8070-audit-variable-changes' into 'master'

Audit CI variable changes

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131882



Merged-by: Stan Hu <stanhu@gmail.com>
Approved-by: Heinrich Lee Yu <heinrich@gitlab.com>
Approved-by: Aaron Huntsman <ahuntsman@gitlab.com>
Approved-by: Stan Hu <stanhu@gmail.com>
Co-authored-by: Sam Figueroa <sfigueroa@gitlab.com>

app/models/ci/instance_variable.rb

@@ -47,5 +47,9 @@ def cached_data
         end
       end
     end
+
+    def audit_details
+      key
+    end
   end
 end

doc/administration/audit_event_streaming/audit_event_types.md

@@ -180,6 +180,9 @@ Audit event types belong to the following product categories.
 | [`ci_group_variable_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/91983) | Triggered when a CI variable is created at a group level| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.2](https://gitlab.com/gitlab-org/gitlab/-/issues/363090) |
 | [`ci_group_variable_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/91983) | Triggered when a group's CI variable is deleted| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.2](https://gitlab.com/gitlab-org/gitlab/-/issues/363090) |
 | [`ci_group_variable_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/91983) | Triggered when a group's CI variable is updated| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.2](https://gitlab.com/gitlab-org/gitlab/-/issues/363090) |
+| [`ci_instance_variable_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131882) | When an instance level CI variable is created| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.5](https://gitlab.com/gitlab-org/gitlab/-/issues/8070) |
+| [`ci_instance_variable_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131882) | When an instance level CI varialbe is deleted| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.5](https://gitlab.com/gitlab-org/gitlab/-/issues/8070) |
+| [`ci_instance_variable_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131882) | When an instance level CI variable is changed| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.5](https://gitlab.com/gitlab-org/gitlab/-/issues/8070) |
 | [`ci_variable_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/91983) | Triggered when a CI variable is created at a project level| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.2](https://gitlab.com/gitlab-org/gitlab/-/issues/363090) |
 | [`ci_variable_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/91983) | Triggered when a project's CI variable is deleted| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.2](https://gitlab.com/gitlab-org/gitlab/-/issues/363090) |
 | [`ci_variable_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/91983) | Triggered when a project's CI variable is updated| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.2](https://gitlab.com/gitlab-org/gitlab/-/issues/363090) |

ee/app/services/ci/audit_variable_change_service.rb

@@ -4,7 +4,12 @@ module Ci
   class AuditVariableChangeService < ::BaseContainerService
     include ::Audit::Changes
-    AUDITABLE_VARIABLE_CLASSES = [::Ci::Variable, ::Ci::GroupVariable].freeze
+    AUDITABLE_VARIABLE_CLASSES = [::Ci::Variable, ::Ci::GroupVariable, ::Ci::InstanceVariable].freeze
+    TEXT_COLUMNS = %i[key value].freeze
+    SECURITY_COLUMNS = {
+      protected: 'variable protection',
+      masked: 'variable masking'
+    }.freeze
     def execute
       return unless container.feature_available?(:audit_events)
@@ -14,16 +19,46 @@ def execute
       when :create, :destroy
         log_audit_event(params[:action], params[:variable])
       when :update
+        audit_update(params[:variable])
+      end
+    end
+
+    private
+
+    def audit_update(variable)
+      SECURITY_COLUMNS.each do |column, as_text|
         audit_changes(
-          :protected,
-          as: 'variable protection', entity: container,
-          model: params[:variable], target_details: params[:variable].key,
-          event_type: event_type_name(params[:variable], params[:action])
+          column,
+          as: as_text,
+          entity: container,
+          model: variable,
+          target_details: variable.key,
+          event_type: event_type_name(variable, :update)
+        )
+      end
+
+      TEXT_COLUMNS.compact.each do |column|
+        # instance variables don't have a :value column
+        next if column == :value && variable.is_a?(::Ci::InstanceVariable)
+
+        audit_changes(
+          column,
+          entity: container,
+          model: variable,
+          target_details: variable.key,
+          event_type: event_type_name(variable, :update),
+          skip_changes: skip_changes?(variable, column)
         )
       end
     end
-    private
+    def skip_changes?(variable, column)
+      return false unless column == :value
+      # do not include masked values in audit, if masking or unmasking
+      return true if variable.masked?
+
+      variable.masked_changed? && variable.masked_change.any?
+    end
     def log_audit_event(action, variable)
       audit_context = {

ee/config/audit_events/types/ci_instance_variable_created.yml

+---
+name: ci_instance_variable_created
+description: When an instance level CI variable is created
+introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/8070
+introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131882
+feature_category: continuous_integration
+milestone: '16.5'
+saved_to_database: true
+streamed: true

ee/config/audit_events/types/ci_instance_variable_deleted.yml

+---
+name: ci_instance_variable_deleted
+description: When an instance level CI varialbe is deleted
+introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/8070
+introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131882
+feature_category: continuous_integration
+milestone: '16.5'
+saved_to_database: true
+streamed: true

ee/config/audit_events/types/ci_instance_variable_updated.yml

+---
+name: ci_instance_variable_updated
+description: When an instance level CI variable is changed
+introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/8070
+introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131882
+feature_category: continuous_integration
+milestone: '16.5'
+saved_to_database: true
+streamed: true

ee/spec/services/ci/audit_variable_change_service_spec.rb

@@ -8,6 +8,7 @@
   let_it_be(:user) { create(:user) }
   let(:group) { create(:group) }
+  let(:instance_variable) { create(:ci_instance_variable, key: 'CI_DEBUG_TRACE', value: true) }
   let(:group_variable) { create(:ci_group_variable, group: group) }
   let(:destination) { create(:external_audit_event_destination, group: group) }
   let(:project) { create(:project, group: group) }
@@ -27,13 +28,13 @@
       expect { execute }.to change(AuditEvent, :count).from(0).to(1)
     end
-    it 'logs variable group creation' do
+    it 'logs (project/group/instance) variable creation' do
       execute
-      audit_event = AuditEvent.last.present
-      expect(audit_event.action).to eq(message)
-      expect(audit_event.target).to eq(variable.key)
+      audit_event = AuditEvent.last.presence
+      expect(audit_event.details[:custom_message]).to eq(message)
+      expect(audit_event.details[:target_details]).to eq(variable.key)
     end
     it_behaves_like 'sends correct event type in audit event stream' do
@@ -55,10 +56,10 @@
     it 'logs variable protection update' do
       execute
-      audit_event = AuditEvent.last.present
-      expect(audit_event.action).to eq('Changed variable protection from false to true')
-      expect(audit_event.target).to eq(variable.key)
+      audit_event = AuditEvent.last.presence
+      expect(audit_event.details[:custom_message]).to eq('Changed variable protection from false to true')
+      expect(audit_event.details[:target_details]).to eq(variable.key)
     end
     it_behaves_like 'sends correct event type in audit event stream' do
@@ -66,6 +67,47 @@
     end
   end
+  shared_examples 'audit value change' do
+    let(:action) { :update }
+
+    context 'when updated masked' do
+      before do
+        variable.masked = true
+        variable.save!
+      end
+
+      it 'logs audit event' do
+        expect { execute }.to change(AuditEvent, :count).from(0).to(1)
+      end
+
+      it 'logs variable masked update' do
+        execute
+
+        audit_event = AuditEvent.last.presence
+
+        expect(audit_event.details[:custom_message]).to eq('Changed variable masking from false to true')
+        expect(audit_event.details[:target_details]).to eq(variable.key)
+      end
+
+      it_behaves_like 'sends correct event type in audit event stream' do
+        let_it_be(:event_type) { event_type }
+      end
+    end
+
+    context 'when masked is and was false' do
+      it 'audit with from and to of the value' do
+        variable.masked = false
+        variable.value = 'A'
+        variable.save!
+        variable.reload
+
+        variable.value = 'B'
+
+        expect { execute }.not_to change(AuditEvent, :count)
+      end
+    end
+  end
+
   shared_examples 'no audit events are created' do
     context 'when creating variable' do
       let(:action) { :create }
@@ -106,7 +148,7 @@
     it 'logs variable destruction' do
       execute
-      audit_event = AuditEvent.last.present
+      audit_event = AuditEvent.last.presence
       expect(audit_event.action).to eq(message)
       expect(audit_event.target).to eq(variable.key)
@@ -124,55 +166,72 @@
       group.external_audit_event_destinations.create!(destination_url: 'http://example.com')
     end
-    context 'when creating group variable' do
-      it_behaves_like 'audit creation' do
-        let(:variable) { group_variable }
-        let_it_be(:message) { 'Added ci group variable' }
-        let_it_be(:event_type) { "ci_group_variable_created" }
+    context 'with instance variables' do
+      let(:variable) { instance_variable }
+      context 'when creating instance variable' do
+        it_behaves_like 'audit creation' do
+          let_it_be(:message) { 'Added ci instance variable' }
+          let_it_be(:event_type) { "ci_instance_variable_created" }
+        end
       end
-    end
-
-    context 'when updating group variable protection' do
-      it_behaves_like 'audit when updating variable protection' do
-        let(:variable) { group_variable }
-        let_it_be(:event_type) { "ci_group_variable_updated" }
+      context 'when updating instance variable protection' do
+        it_behaves_like 'audit when updating variable protection' do
+          let_it_be(:event_type) { "ci_instance_variable_updated" }
+        end
       end
     end
-    context 'when deleting group variable' do
-      it_behaves_like 'audit when updating variable protection' do
-        let(:variable) { group_variable }
-        let_it_be(:message) { 'Removed ci group variable' }
+    context 'with group variables' do
+      let(:variable) { group_variable }
+      it_behaves_like 'audit value change' do
         let_it_be(:event_type) { "ci_group_variable_updated" }
       end
-    end
-    context 'when creating project variable' do
-      it_behaves_like 'audit creation' do
-        let(:variable) { project_variable }
-        let_it_be(:message) { 'Added ci variable' }
-        let_it_be(:event_type) { "ci_variable_created" }
+      context 'when creating group variable' do
+        it_behaves_like 'audit creation' do
+          let_it_be(:message) { 'Added ci group variable' }
+          let_it_be(:event_type) { "ci_group_variable_created" }
+        end
+      end
+
+      context 'when updating group variable protection' do
+        it_behaves_like 'audit when updating variable protection' do
+          let_it_be(:event_type) { "ci_group_variable_updated" }
+        end
+      end
+      context 'when deleting group variable' do
+        it_behaves_like 'audit when updating variable protection' do
+          let_it_be(:message) { 'Removed ci group variable' }
+          let_it_be(:event_type) { "ci_group_variable_updated" }
+        end
       end
     end
-    context 'when updating project variable protection' do
-      it_behaves_like 'audit when updating variable protection' do
-        let(:variable) { project_variable }
-        let_it_be(:event_type) { "ci_variable_updated" }
+    context 'with project variables' do
+      let(:variable) { project_variable }
+      context 'when creating project variable' do
+        it_behaves_like 'audit creation' do
+          let_it_be(:message) { 'Added ci variable' }
+          let_it_be(:event_type) { "ci_variable_created" }
+        end
       end
-    end
-    context 'when deleting project variable' do
-      it_behaves_like 'audit when updating variable protection' do
-        let(:variable) { project_variable }
-        let_it_be(:message) { 'Removed ci variable' }
-        let_it_be(:event_type) { "ci_variable_updated" }
+      context 'when updating project variable protection' do
+        it_behaves_like 'audit when updating variable protection' do
+          let_it_be(:event_type) { "ci_variable_updated" }
+        end
+      end
+      context 'when deleting project variable' do
+        it_behaves_like 'audit when updating variable protection' do
+          let_it_be(:message) { 'Removed ci variable' }
+          let_it_be(:event_type) { "ci_variable_updated" }
+        end
       end
     end
   end