Merge branch 'mc/feature/audit-variable-changes-project' into 'master'

Audit variable changes in projects

See merge request gitlab-org/gitlab!38928

app/controllers/projects/variables_controller.rb

@@ -12,7 +12,12 @@ class Projects::VariablesController < Projects::ApplicationController
   end
 
   def update
-    if @project.update(variables_params)
+    update_result = Ci::ChangeVariablesService.new(
+      container: @project, current_user: current_user,
+      params: variables_params
+    ).execute
+
+    if update_result
       respond_to do |format|
         format.json { render_variables }
       end

app/services/ci/change_variable_service.rb

@@ -20,7 +20,12 @@ module Ci
     private
 
     def variable
-      container.variables.find_by!(params[:variable_params].slice(:key)) # rubocop:disable CodeReuse/ActiveRecord
+      params[:variable] || find_variable
+    end
+
+    def find_variable
+      identifier = params[:variable_params].slice(:id).presence || params[:variable_params].slice(:key)
+      container.variables.find_by!(identifier) # rubocop:disable CodeReuse/ActiveRecord
     end
   end
 end

ee/app/services/ci/audit_variable_change_service.rb

@@ -23,6 +23,12 @@ module Ci
 
     def log_audit_event(action, variable)
       case variable.class.to_s
+      when ::Ci::Variable.to_s
+        ::AuditEventService.new(
+          current_user,
+          container,
+          action: action
+        ).for_project_variable(variable.key).security_event
       when ::Ci::GroupVariable.to_s
         ::AuditEventService.new(
           current_user,

ee/app/services/ee/audit_event_service.rb

@@ -170,6 +170,15 @@ module EE
       for_custom_model('project', @entity.full_path)
     end
 
+    # Builds the @details attribute for project variable
+    #
+    # This uses the [Ci::ProjectVariable] @entity as the target object being audited
+    #
+    # @return [AuditEventService]
+    def for_project_variable(project_variable_key)
+      for_custom_model('ci_variable', project_variable_key)
+    end
+
     # Builds the @details attribute for group
     #
     # This uses the [Group] @entity as the target object being audited

ee/changelogs/unreleased/mc-feature-audit-variable-changes-project.yml

+---
+title: Audit variable changes for project variables.
+merge_request: 38928
+author:
+type: changed

ee/spec/controllers/ee/projects/variables_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Projects::VariablesController do
+  let(:project) { create(:project) }
+  let(:user) { create(:user) }
+
+  before do
+    sign_in(user)
+    project.add_maintainer(user)
+  end
+
+  describe 'PATCH #update' do
+    subject(:patch_update) do
+      patch :update,
+        params: {
+        namespace_id: project.namespace.to_param,
+        project_id: project,
+        variables_attributes: [variable_attributes]
+      },
+      format: :json
+    end
+
+    before do
+      stub_licensed_features(extended_audit_events: true)
+    end
+
+    context 'when creating variable' do
+      let(:variable_attributes) do
+        { key: 'new_key',
+          secret_value: 'dummy_value',
+          protected: 'false',
+          environment_scope: '*' }
+      end
+
+      it 'logs audit event' do
+        expect { patch_update }.to change(AuditEvent, :count).from(0).to(1)
+      end
+
+      it 'logs variable creation' do
+        patch_update
+
+        audit_event = AuditEvent.last.present
+
+        expect(audit_event.action).to eq('Added ci variable')
+        expect(audit_event.target).to eq('new_key')
+      end
+    end
+
+    context 'when updating variable protection' do
+      let(:variable) { create(:ci_variable, project: project, protected: false) }
+
+      let(:variable_attributes) do
+        { id: variable.id,
+          protected: 'true' }
+      end
+
+      it 'logs audit event' do
+        expect { patch_update }.to change(AuditEvent, :count).from(0).to(1)
+      end
+
+      it 'logs variable protection update' do
+        patch_update
+
+        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)
+      end
+    end
+
+    context 'when destroying variable' do
+      let(:variable) { create(:ci_variable, project: project) }
+
+      let(:variable_attributes) do
+        { key: variable.key,
+          _destroy: 'true' }
+      end
+
+      it 'logs audit event' do
+        expect { patch_update }.to change(AuditEvent, :count).from(0).to(1)
+      end
+
+      it 'logs variable destruction' do
+        patch_update
+
+        audit_event = AuditEvent.last.present
+
+        expect(audit_event.action).to eq('Removed ci variable')
+        expect(audit_event.target).to eq(variable.key)
+      end
+    end
+  end
+end

ee/spec/requests/api/variables_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe API::Variables do
+  let(:user) { create(:user) }
+  let(:project) { create(:project) }
+
+  before do
+    project.add_maintainer(user)
+
+    stub_licensed_features(extended_audit_events: true)
+  end
+
+  describe 'POST /projects/:id/variables' do
+    subject(:post_create) do
+      post api("/projects/#{project.id}/variables", user), params: { key: 'new_variable', value: 'secret_value', protected: true }
+    end
+
+    it 'logs audit event' do
+      expect { post_create }.to change(AuditEvent, :count).from(0).to(1)
+    end
+
+    it 'logs variable creation' do
+      post_create
+
+      audit_event = AuditEvent.last.present
+
+      expect(audit_event.action).to eq('Added ci variable')
+      expect(audit_event.target).to eq('new_variable')
+    end
+  end
+
+  describe 'PUT /projects/:id/variables/:key' do
+    let(:variable) { create(:ci_variable, project: project, protected: false) }
+
+    subject(:put_update) do
+      put api("/projects/#{project.id}/variables/#{variable.key}", user), params: { protected: true }
+    end
+
+    it 'logs audit event' do
+      expect { put_update }.to change(AuditEvent, :count).from(0).to(1)
+    end
+
+    it 'logs variable protection update' do
+      put_update
+
+      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)
+    end
+  end
+
+  describe 'DELETE /projects/:id/variables/:key' do
+    let(:variable) { create(:ci_variable, project: project, protected: false) }
+
+    subject(:delete_destroy) do
+      delete api("/projects/#{project.id}/variables/#{variable.key}", user)
+    end
+
+    it 'logs audit event' do
+      expect { delete_destroy }.to change(AuditEvent, :count).from(0).to(1)
+    end
+
+    it 'logs variable destruction' do
+      delete_destroy
+
+      audit_event = AuditEvent.last.present
+
+      expect(audit_event.action).to eq('Removed ci variable')
+      expect(audit_event.target).to eq(variable.key)
+    end
+  end
+end

lib/api/variables.rb

@@ -67,10 +67,11 @@ module API
         optional :environment_scope, type: String, desc: 'The environment_scope of the variable'
       end
       post ':id/variables' do
-        variable_params = declared_params(include_missing: false)
-        variable_params = filter_variable_parameters(variable_params)
-
-        variable = user_project.variables.create(variable_params)
+        variable = ::Ci::ChangeVariableService.new(
+          container: user_project,
+          current_user: current_user,
+          params: { action: :create, variable_params: filter_variable_parameters(declared_params(include_missing: false)) }
+        ).execute
 
         if variable.valid?
           present variable, with: Entities::Ci::Variable
@@ -96,10 +97,17 @@ module API
         variable = find_variable(params)
         not_found!('Variable') unless variable
 
-        variable_params = declared_params(include_missing: false).except(:key, :filter)
-        variable_params = filter_variable_parameters(variable_params)
+        variable_params = filter_variable_parameters(
+          declared_params(include_missing: false)
+            .except(:key, :filter)
+        )
+        variable = ::Ci::ChangeVariableService.new(
+          container: user_project,
+          current_user: current_user,
+          params: { action: :update, variable: variable, variable_params: variable_params }
+        ).execute
 
-        if variable.update(variable_params)
+        if variable.valid?
           present variable, with: Entities::Ci::Variable
         else
           render_validation_error!(variable)
@@ -119,8 +127,11 @@ module API
         variable = find_variable(params)
         not_found!('Variable') unless variable
 
-        # Variables don't have a timestamp. Therefore, destroy unconditionally.
-        variable.destroy
+        ::Ci::ChangeVariableService.new(
+          container: user_project,
+          current_user: current_user,
+          params: { action: :destroy, variable: variable }
+        ).execute
 
         no_content!
       end