Merge branch 'mc/feature/fix-audit-variable-logs' into 'master'

Audit variable logs

See merge request gitlab-org/gitlab!37945

app/controllers/groups/variables_controller.rb

@@ -15,7 +15,12 @@ module Groups
     end
 
     def update
-      if @group.update(group_variables_params)
+      update_result = Ci::ChangeVariablesService.new(
+        container: @group, current_user: current_user,
+        params: group_variables_params
+      ).execute
+
+      if update_result
         respond_to do |format|
           format.json { render_group_variables }
         end

app/services/ci/change_variable_service.rb

+# frozen_string_literal: true
+
+module Ci
+  class ChangeVariableService < BaseContainerService
+    def execute
+      case params[:action]
+      when :create
+        container.variables.create(params[:variable_params])
+      when :update
+        variable.tap do |target_variable|
+          target_variable.update(params[:variable_params].except(:key))
+        end
+      when :destroy
+        variable.tap do |target_variable|
+          target_variable.destroy
+        end
+      end
+    end
+
+    private
+
+    def variable
+      container.variables.find_by!(params[:variable_params].slice(:key)) # rubocop:disable CodeReuse/ActiveRecord
+    end
+  end
+end
+
+::Ci::ChangeVariableService.prepend_if_ee('EE::Ci::ChangeVariableService')

app/services/ci/change_variables_service.rb

+# frozen_string_literal: true
+
+module Ci
+  class ChangeVariablesService < BaseContainerService
+    def execute
+      container.update(params)
+    end
+  end
+end
+
+::Ci::ChangeVariablesService.prepend_if_ee('EE::Ci::ChangeVariablesService')

ee/app/services/ci/audit_variable_change_service.rb

+# frozen_string_literal: true
+
+module Ci
+  class AuditVariableChangeService < ::BaseContainerService
+    include ::Audit::Changes
+
+    def execute
+      return unless License.feature_available?(:extended_audit_events)
+
+      case params[:action]
+      when :create, :destroy
+        log_audit_event(params[:action], params[:variable])
+      when :update
+        audit_changes(
+          :protected,
+          as: 'variable protection', entity: container,
+          model: params[:variable], target_details: params[:variable].key
+        )
+      end
+    end
+
+    private
+
+    def log_audit_event(action, variable)
+      case variable.class.to_s
+      when ::Ci::GroupVariable.to_s
+        ::AuditEventService.new(
+          current_user,
+          container,
+          action: action
+        ).for_group_variable(variable.key).security_event
+      end
+    end
+  end
+end

ee/app/services/ee/audit_event_service.rb

@@ -179,6 +179,15 @@ module EE
       for_custom_model('group', @entity.full_path)
     end
 
+    # Builds the @details attribute for group variable
+    #
+    # This uses the [Ci::GroupVariable] @entity as the target object being audited
+    #
+    # @return [AuditEventService]
+    def for_group_variable(group_variable_key)
+      for_custom_model('ci_group_variable', group_variable_key)
+    end
+
     def enabled?
       admin_audit_log_enabled? ||
         audit_events_enabled? ||

ee/app/services/ee/ci/change_variable_service.rb

+# frozen_string_literal: true
+
+module EE
+  module Ci
+    module ChangeVariableService
+      extend ::Gitlab::Utils::Override
+
+      override :execute
+      def execute
+        super.tap do |target_variable|
+          if target_variable.valid?
+            ::Ci::AuditVariableChangeService.new(
+              container: container,
+              current_user: current_user,
+              params: { action: params[:action], variable: target_variable }
+            ).execute
+          end
+        end
+      end
+    end
+  end
+end

ee/app/services/ee/ci/change_variables_service.rb

+# frozen_string_literal: true
+
+module EE
+  module Ci
+    module ChangeVariablesService
+      extend ::Gitlab::Utils::Override
+
+      override :execute
+      def execute
+        super.tap do |result|
+          log_audit_events if result
+        end
+      end
+
+      private
+
+      def log_audit_events
+        params[:variables_attributes].each do |variable_params|
+          action = variable_action(variable_params)
+          target = target_variable(action, variable_params)
+
+          ::Ci::AuditVariableChangeService.new(
+            container: container,
+            current_user: current_user,
+            params: { action: action, variable: target }
+          ).execute
+        end
+      end
+
+      def find_variable_by_id(target_id)
+        container.variables.find { |variable| variable.id.to_s == target_id.to_s }
+      end
+
+      def find_variable_by_key(target_key)
+        container.variables.find { |variable| variable.key == target_key }
+      end
+
+      def variable_class
+        container.class.reflect_on_association(:variables).klass
+      end
+
+      def variable_action(variable_params)
+        if variable_params[:_destroy]
+          :destroy
+        elsif variable_params[:id].nil?
+          :create
+        else
+          :update
+        end
+      end
+
+      def target_variable(action, variable_params)
+        case action
+        when :create
+          find_variable_by_key(variable_params[:key])
+        when :update
+          find_variable_by_id(variable_params[:id])
+        when :destroy
+          variable_class.new(variable_params.except(:_destroy))
+        end
+      end
+    end
+  end
+end

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

+---
+title: Audit log group variable updates.
+merge_request: 37945
+author:
+type: changed

ee/spec/services/ci/audit_variable_change_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::AuditVariableChangeService do
+  subject(:execute) { service.execute }
+
+  let_it_be(:user) { create(:user) }
+  let(:group) { create(:group) }
+  let(:variable) { create(:ci_group_variable) }
+
+  let(:service) do
+    described_class.new(
+      container: group, current_user: user,
+      params: { action: action, variable: variable }
+    )
+  end
+
+  before do
+    group.variables << variable
+  end
+
+  context 'when extended audits are available' do
+    before do
+      stub_licensed_features(extended_audit_events: true)
+    end
+
+    context 'when creating variable' do
+      let(:action) { :create }
+
+      it 'logs audit event' do
+        expect { execute }.to change(AuditEvent, :count).from(0).to(1)
+      end
+
+      it 'logs variable creation' do
+        execute
+
+        audit_event = AuditEvent.last.present
+
+        expect(audit_event.action).to eq('Added ci group variable')
+        expect(audit_event.target).to eq(variable.key)
+      end
+    end
+
+    context 'when updating variable protection' do
+      let(:action) { :update }
+
+      before do
+        variable.update!(protected: true)
+      end
+
+      it 'logs audit event' do
+        expect { execute }.to change(AuditEvent, :count).from(0).to(1)
+      end
+
+      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)
+      end
+    end
+
+    context 'when destroying variable' do
+      let(:action) { :destroy }
+
+      it 'logs audit event' do
+        expect { execute }.to change(AuditEvent, :count).from(0).to(1)
+      end
+
+      it 'logs variable destruction' do
+        execute
+
+        audit_event = AuditEvent.last.present
+
+        expect(audit_event.action).to eq('Removed ci group variable')
+        expect(audit_event.target).to eq(variable.key)
+      end
+    end
+  end
+
+  context 'when extended audits are not available' do
+    before do
+      stub_licensed_features(extended_audit_events: false)
+    end
+
+    context 'when creating variable' do
+      let(:action) { :create }
+
+      it 'does not log an audit event' do
+        expect { execute }.not_to change(AuditEvent, :count).from(0)
+      end
+    end
+
+    context 'when updating variable protection' do
+      let(:action) { :update }
+
+      before do
+        variable.update!(protected: true)
+      end
+
+      it 'does not log an audit event' do
+        expect { execute }.not_to change(AuditEvent, :count).from(0)
+      end
+    end
+
+    context 'when destroying variable' do
+      let(:action) { :destroy }
+
+      it 'does not log an audit event' do
+        expect { execute }.not_to change(AuditEvent, :count).from(0)
+      end
+    end
+  end
+end

ee/spec/services/ee/ci/change_variable_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::ChangeVariableService do
+  subject(:execute) { service.execute }
+
+  let_it_be(:user) { create(:user) }
+  let(:group) { create(:group) }
+  let(:audit_service_spy) { class_spy(::Ci::AuditVariableChangeService, new: spy) }
+
+  let(:service) do
+    described_class.new(
+      container: group, current_user: user,
+      params: variable_params
+    )
+  end
+
+  before do
+    stub_const('::Ci::AuditVariableChangeService', audit_service_spy)
+  end
+
+  context 'when creating a variable' do
+    let(:variable_params) { { variable_params: { key: 'new_variable', value: 'new_value' }, action: :create } }
+
+    it 'calls AuditVariableChangeService with create' do
+      execute
+
+      expect(audit_service_spy).to have_received(:new).with(
+        hash_including(
+          container: group, current_user: user,
+          params: hash_including(action: :create, variable: instance_of(::Ci::GroupVariable))
+        )
+      )
+    end
+  end
+
+  context 'when updating a variable' do
+    let(:variable) { create(:ci_group_variable) }
+    let(:variable_params) { { variable_params: { key: variable.key, protected: 'true' }, action: :update } }
+
+    before do
+      group.variables << variable
+    end
+
+    it 'calls AuditVariableChangeService with update' do
+      execute
+
+      expect(audit_service_spy).to have_received(:new).with(
+        hash_including(
+          container: group, current_user: user,
+          params: hash_including(action: :update, variable: instance_of(::Ci::GroupVariable))
+        )
+      )
+    end
+  end
+
+  context 'when destroying a variable' do
+    let(:variable) { create(:ci_group_variable, key: 'old_variable') }
+    let(:variable_params) { { variable_params: { key: variable.key }, action: :destroy } }
+
+    before do
+      group.variables << variable
+    end
+
+    it 'calls AuditVariableChangeService with destroy' do
+      execute
+
+      expect(audit_service_spy).to have_received(:new).with(
+        hash_including(
+          container: group, current_user: user,
+          params: hash_including(action: :destroy, variable: instance_of(::Ci::GroupVariable))
+        )
+      )
+    end
+  end
+end

ee/spec/services/ee/ci/change_variables_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::ChangeVariablesService do
+  subject(:execute) { service.execute }
+
+  let_it_be(:user) { create(:user) }
+  let(:group) { create(:group) }
+  let(:audit_service_spy) { class_spy(Ci::AuditVariableChangeService, new: spy) }
+
+  let(:service) do
+    described_class.new(
+      container: group, current_user: user,
+      params: variable_params
+    )
+  end
+
+  before do
+    stub_const('Ci::AuditVariableChangeService', audit_service_spy)
+  end
+
+  context 'when creating a variable' do
+    let(:variable_params) { { variables_attributes: [{ key: 'new_variable', value: 'new_value' }] } }
+
+    it 'calls AuditVariableChangeService with create' do
+      execute
+
+      expect(audit_service_spy).to have_received(:new).with(
+        hash_including(
+          container: group, current_user: user,
+          params: hash_including(action: :create, variable: instance_of(::Ci::GroupVariable))
+        )
+      )
+    end
+  end
+
+  context 'when updating a variable' do
+    let(:variable) { create(:ci_group_variable) }
+    let(:variable_params) { { variables_attributes: [{ id: variable.id }.merge(variable_changes)] } }
+
+    before do
+      group.variables << variable
+    end
+
+    context 'when update succeeds' do
+      let(:variable_changes) { { protected: 'true' } }
+
+      it 'calls AuditVariableChangeService with update' do
+        execute
+
+        expect(audit_service_spy).to have_received(:new).with(
+          hash_including(
+            container: group, current_user: user,
+            params: hash_including(action: :update, variable: instance_of(::Ci::GroupVariable))
+          )
+        )
+      end
+    end
+
+    context 'when update fails' do
+      let(:variable_changes) { { value: 'shrt', masked: 'true' } }
+
+      it 'does not call AuditVariableChangeService' do
+        execute
+
+        expect(audit_service_spy).not_to have_received(:new)
+      end
+    end
+  end
+
+  context 'when destroying a variable' do
+    let(:variable) { create(:ci_group_variable, key: 'old_variable') }
+    let(:variable_params) { { variables_attributes: [{ id: variable.id, key: variable.key, _destroy: 'true' }] } }
+
+    before do
+      group.variables << variable
+    end
+
+    it 'calls AuditVariableChangeService with destroy' do
+      execute
+
+      expect(audit_service_spy).to have_received(:new).with(
+        hash_including(
+          container: group, current_user: user,
+          params: hash_including(action: :destroy, variable: instance_of(::Ci::GroupVariable))
+        )
+      )
+    end
+  end
+
+  context 'when making multiple changes' do
+    let(:update_variable) { create(:ci_group_variable) }
+    let(:delete_variable) { create(:ci_group_variable, key: 'old_variable') }
+
+    let(:variable_params) do
+      {
+        variables_attributes: [
+          { key: 'new_variable', value: 'new_value' },
+          { id: update_variable.id, protected: 'true' },
+          { id: delete_variable.id, key: delete_variable.key, _destroy: 'true' }
+        ]
+      }
+    end
+
+    before do
+      group.variables << update_variable
+      group.variables << delete_variable
+    end
+
+    it 'calls AuditVariableChangeService with create' do
+      execute
+
+      expect(audit_service_spy).to have_received(:new).with(
+        hash_including(
+          container: group, current_user: user,
+          params: hash_including(action: :create, variable: instance_of(::Ci::GroupVariable))
+        )
+      )
+    end
+
+    it 'calls AuditVariableChangeService with update' do
+      execute
+
+      expect(audit_service_spy).to have_received(:new).with(
+        hash_including(
+          container: group, current_user: user,
+          params: hash_including(action: :update, variable: instance_of(::Ci::GroupVariable))
+        )
+      )
+    end
+
+    it 'calls AuditVariableChangeService with destroy' do
+      execute
+
+      expect(audit_service_spy).to have_received(:new).with(
+        hash_including(
+          container: group, current_user: user,
+          params: hash_including(action: :destroy, variable: instance_of(::Ci::GroupVariable))
+        )
+      )
+    end
+  end
+end

lib/api/group_variables.rb

@@ -51,9 +51,11 @@ module API
         optional :variable_type, type: String, values: ::Ci::GroupVariable.variable_types.keys, desc: 'The type of variable, must be one of env_var or file. Defaults to env_var'
       end
       post ':id/variables' do
-        variable_params = declared_params(include_missing: false)
-
-        variable = user_group.variables.create(variable_params)
+        variable = ::Ci::ChangeVariableService.new(
+          container: user_group,
+          current_user: current_user,
+          params: { action: :create, variable_params: declared_params(include_missing: false) }
+        ).execute
 
         if variable.valid?
           present variable, with: Entities::Variable
@@ -74,17 +76,19 @@ module API
       end
       # rubocop: disable CodeReuse/ActiveRecord
       put ':id/variables/:key' do
-        variable = user_group.variables.find_by(key: params[:key])
-
-        break not_found!('GroupVariable') unless variable
-
-        variable_params = declared_params(include_missing: false).except(:key)
+        variable = ::Ci::ChangeVariableService.new(
+          container: user_group,
+          current_user: current_user,
+          params: { action: :update, variable_params: declared_params(include_missing: false) }
+        ).execute
 
-        if variable.update(variable_params)
+        if variable.valid?
           present variable, with: Entities::Variable
         else
           render_validation_error!(variable)
         end
+      rescue ::ActiveRecord::RecordNotFound
+        not_found!('GroupVariable')
       end
       # rubocop: enable CodeReuse/ActiveRecord
 
@@ -96,10 +100,17 @@ module API
       end
       # rubocop: disable CodeReuse/ActiveRecord
       delete ':id/variables/:key' do
-        variable = user_group.variables.find_by(key: params[:key])
-        not_found!('GroupVariable') unless variable
-
-        destroy_conditionally!(variable)
+        variable = user_group.variables.find_by!(key: params[:key])
+
+        destroy_conditionally!(variable) do |target_variable|
+          ::Ci::ChangeVariableService.new(
+            container: user_group,
+            current_user: current_user,
+            params: { action: :destroy, variable_params: declared_params(include_missing: false) }
+          ).execute
+        end
+      rescue ::ActiveRecord::RecordNotFound
+        not_found!('GroupVariable')
       end
       # rubocop: enable CodeReuse/ActiveRecord
     end

spec/requests/api/group_variables_spec.rb

@@ -169,6 +169,14 @@ RSpec.describe API::GroupVariables do
 
         expect(response).to have_gitlab_http_status(:not_found)
       end
+
+      it 'responds with 400 if the update fails' do
+        put api("/groups/#{group.id}/variables/#{variable.key}", user), params: { value: 'shrt', masked: true }
+
+        expect(response).to have_gitlab_http_status(:bad_request)
+        expect(variable.reload.masked).to eq(false)
+        expect(json_response['message']).to eq('value' => ['is invalid'])
+      end
     end
 
     context 'authorized user with invalid permissions' do

spec/services/ci/change_variable_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::ChangeVariableService do
+  let(:service) { described_class.new(container: group, current_user: user, params: params) }
+
+  let_it_be(:user) { create(:user) }
+  let(:group) { create(:group) }
+
+  describe '#execute' do
+    subject(:execute) { service.execute }
+
+    context 'when creating a variable' do
+      let(:params) { { variable_params: { key: 'new_variable', value: 'variable_value' }, action: :create } }
+
+      it 'persists a variable' do
+        expect { execute }.to change(Ci::GroupVariable, :count).from(0).to(1)
+      end
+    end
+
+    context 'when updating a variable' do
+      let!(:variable) { create(:ci_group_variable, value: 'old_value') }
+      let(:params) { { variable_params: { key: variable.key, value: 'new_value' }, action: :update } }
+
+      before do
+        group.variables << variable
+      end
+
+      it 'updates a variable' do
+        expect { execute }.to change { variable.reload.value }.from('old_value').to('new_value')
+      end
+
+      context 'when the variable does not exist' do
+        before do
+          variable.destroy!
+        end
+
+        it 'raises a record not found error' do
+          expect { execute }.to raise_error(::ActiveRecord::RecordNotFound)
+        end
+      end
+    end
+
+    context 'when destroying a variable' do
+      let!(:variable) { create(:ci_group_variable) }
+      let(:params) { { variable_params: { key: variable.key }, action: :destroy } }
+
+      before do
+        group.variables << variable
+      end
+
+      it 'destroys a variable' do
+        expect { execute }.to change { Ci::GroupVariable.exists?(variable.id) }.from(true).to(false)
+      end
+
+      context 'when the variable does not exist' do
+        before do
+          variable.destroy!
+        end
+
+        it 'raises a record not found error' do
+          expect { execute }.to raise_error(::ActiveRecord::RecordNotFound)
+        end
+      end
+    end
+  end
+end

spec/services/ci/change_variables_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::ChangeVariablesService do
+  let(:service) { described_class.new(container: group, current_user: user, params: params) }
+
+  let_it_be(:user) { create(:user) }
+  let(:group) { spy(:group, variables: []) }
+  let(:params) { { variables_attributes: [{ key: 'new_variable', value: 'variable_value' }] } }
+
+  describe '#execute' do
+    subject(:execute) { service.execute }
+
+    it 'delegates to ActiveRecord update' do
+      execute
+
+      expect(group).to have_received(:update).with(params)
+    end
+  end
+end