From 48d9998ee4ce75ae5d203cb399044b85eee9b393 Mon Sep 17 00:00:00 2001
From: Pam Artiaga <partiaga@gitlab.com>
Date: Wed, 30 Aug 2023 14:36:08 +1000
Subject: [PATCH 1/9] Log audit event when updating protected environments

Changelog: changed
MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494
EE: true

Change audit event type and message for protected environment update

Also update the specs to make it clearer which message corresponds
to which update

Update doc for new audit event type

Make rubocop happy

Refactor to remove complexity

Fix regression for EnvironmentDropdownService

Update variable name for readability

Move audit event type config to ee

Update log audit event message
---
 .../audit_event_types.md                      |   1 +
 .../protected_environments/authorizable.rb    |   3 +-
 .../environment_dropdown_service.rb           |   5 +-
 .../protected_environments/update_service.rb  | 134 +++++++++++++++++-
 .../types/protected_environment_updated.yml   |   9 ++
 ee/spec/factories/protected_environments.rb   |  52 ++++++-
 .../update_service_spec.rb                    | 109 +++++++++++++-
 7 files changed, 302 insertions(+), 11 deletions(-)
 create mode 100644 ee/config/audit_events/types/protected_environment_updated.yml

diff --git a/doc/administration/audit_event_streaming/audit_event_types.md b/doc/administration/audit_event_streaming/audit_event_types.md
index 5c27dabe378649..d87f6372dafecc 100644
--- a/doc/administration/audit_event_streaming/audit_event_types.md
+++ b/doc/administration/audit_event_streaming/audit_event_types.md
@@ -304,3 +304,4 @@ audit events to external destinations.
 | [`feature_flag_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/113453) | Triggered when a feature flag is deleted. | **{check-circle}** Yes | **{check-circle}** Yes | `feature_flags` | GitLab [15.10](https://gitlab.com/gitlab-org/gitlab/-/issues/374109) |
 | [`feature_flag_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/113453) | Triggered when a feature flag is updated. | **{check-circle}** Yes | **{check-circle}** Yes | `feature_flags` | GitLab [15.10](https://gitlab.com/gitlab-org/gitlab/-/issues/374109) |
 | [`manually_trigger_housekeeping`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/112095) | Triggered when manually triggering housekeeping via API or admin UI | **{check-circle}** Yes | **{check-circle}** Yes | `source_code_management` | GitLab [15.9](https://gitlab.com/gitlab-org/gitlab/-/issues/390761) |
+| [`protected_environment_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494) | This event is triggered when a protected environment is updated. | **{check-circle}** Yes | **{dotted-circle}** No | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
diff --git a/ee/app/models/protected_environments/authorizable.rb b/ee/app/models/protected_environments/authorizable.rb
index 035f35de8e8264..f3acef5b9ae1da 100644
--- a/ee/app/models/protected_environments/authorizable.rb
+++ b/ee/app/models/protected_environments/authorizable.rb
@@ -25,7 +25,8 @@ module Authorizable
 
     HUMAN_ACCESS_LEVELS = {
       Gitlab::Access::MAINTAINER => 'Maintainers',
-      Gitlab::Access::DEVELOPER => 'Developers + Maintainers'
+      Gitlab::Access::DEVELOPER => 'Developers + Maintainers',
+      Gitlab::Access::ADMIN => 'Admin'
     }.freeze
 
     def check_access(user)
diff --git a/ee/app/services/protected_environments/environment_dropdown_service.rb b/ee/app/services/protected_environments/environment_dropdown_service.rb
index f4f096223e32cc..bf7634e95a242b 100644
--- a/ee/app/services/protected_environments/environment_dropdown_service.rb
+++ b/ee/app/services/protected_environments/environment_dropdown_service.rb
@@ -12,7 +12,10 @@ def self.roles
     end
 
     def self.human_access_levels
-      ::ProtectedEnvironments::DeployAccessLevel::HUMAN_ACCESS_LEVELS
+      ::ProtectedEnvironments::DeployAccessLevel::HUMAN_ACCESS_LEVELS.slice(
+        Gitlab::Access::MAINTAINER,
+        Gitlab::Access::DEVELOPER
+      )
     end
   end
 end
diff --git a/ee/app/services/protected_environments/update_service.rb b/ee/app/services/protected_environments/update_service.rb
index 690e23da86028a..f950ded309fcfc 100644
--- a/ee/app/services/protected_environments/update_service.rb
+++ b/ee/app/services/protected_environments/update_service.rb
@@ -1,8 +1,140 @@
 # frozen_string_literal: true
 module ProtectedEnvironments
   class UpdateService < ProtectedEnvironments::BaseService
+    AUDITABLE_ATTRIBUTES = [:required_approval_count].freeze
+    AUTHORIZABLE_ATTRIBUTES = [:access_level, :user_id, :group_id].freeze
+
     def execute(protected_environment)
-      protected_environment.update(sanitized_params)
+      protected_environment.assign_attributes(sanitized_params)
+
+      prepared_audit_context = audit_context(protected_environment)
+
+      result = protected_environment.save
+
+      log_audit_event(prepared_audit_context) if successful?(result) # TODO: do not log if no changes
+
+      result
+    end
+
+    private
+
+    def successful?(result)
+      result == true
+    end
+
+    def log_audit_event(prepared_audit_context)
+      ::Gitlab::Audit::Auditor.audit(prepared_audit_context)
+    end
+
+    def audit_context(protected_environment)
+      {
+        name: 'protected_environment_updated',
+        author: current_user,
+        scope: container,
+        target: protected_environment,
+        message: audit_messages(protected_environment).join(' ')
+      }
+    end
+
+    def audit_messages(protected_environment)
+      messages = []
+
+      messages += changed_attributes_messages(protected_environment)
+      messages += changed_deploy_access_levels_messages(protected_environment)
+      messages += changed_approval_rules_messages(protected_environment)
+
+      messages
+    end
+
+    def changed_attributes_messages(protected_environment)
+      protected_environment.changes.slice(*AUDITABLE_ATTRIBUTES).map do |attr_name, changes|
+        "Updated #{attr_name} from #{changes.first} to #{changes.last}."
+      end
+    end
+
+    def changed_deploy_access_levels_messages(protected_environment)
+      protected_environment.deploy_access_levels.filter_map do |deploy_access_level|
+        if deploy_access_level.new_record?
+          "Added deploy access level for #{deploy_access_level.humanize}."
+        elsif deploy_access_level.marked_for_destruction?
+          "Deleted deploy access level for #{deploy_access_level.humanize}."
+        else
+          updated_deploy_access_level_message(deploy_access_level)
+        end
+      end
+    end
+
+    def updated_deploy_access_level_message(deploy_access_level)
+      changed_access_levels = changed_access_level_details(deploy_access_level)
+      return if changed_access_levels.empty?
+
+      "Updated deploy access level from #{changed_access_levels.first} to #{changed_access_levels.last}."
+    end
+
+    def changed_approval_rules_messages(protected_environment)
+      protected_environment.approval_rules.filter_map do |approval_rule|
+        if approval_rule.new_record?
+          "Added approval rule for #{approval_rule.humanize} " \
+            "with required approval count #{approval_rule.required_approvals}."
+        elsif approval_rule.marked_for_destruction?
+          "Deleted approval rule for #{approval_rule.humanize}."
+        else
+          updated_approval_rule_message(approval_rule)
+        end
+      end
+    end
+
+    def updated_approval_rule_message(approval_rule)
+      changed_access_levels = changed_access_level_details(approval_rule)
+      changed_approval_counts = approval_rule.changes[:required_approvals]
+
+      return if changed_access_levels.empty? && changed_approval_counts.nil?
+
+      if changed_access_levels.present?
+        update_approval_rule_access_level_message(approval_rule, changed_access_levels, changed_approval_counts)
+      else
+        update_approval_rule_approval_count_message(approval_rule, changed_approval_counts)
+      end
+    end
+
+    def update_approval_rule_access_level_message(approval_rule, changed_access_levels, changed_approval_counts)
+      old_approval_count = changed_approval_counts ? changed_approval_counts.first : approval_rule.required_approvals
+      new_approval_count = changed_approval_counts ? changed_approval_counts.last : approval_rule.required_approvals
+
+      "Updated approval rule for #{changed_access_levels.first} with required approval count #{old_approval_count} " \
+        "to #{approval_rule.humanize} with required approval count #{new_approval_count}."
+    end
+
+    def update_approval_rule_approval_count_message(approval_rule, changed_approval_counts)
+      "Updated approval rule for #{approval_rule.humanize} with required approval count " \
+        "from #{changed_approval_counts.first} " \
+        "to #{changed_approval_counts.last}."
+    end
+
+    def changed_access_level_details(authorizable_object)
+      changed_attributes = authorizable_object.changes.slice(*AUTHORIZABLE_ATTRIBUTES)
+      return [] if changed_attributes.empty?
+
+      old_access_level_name = nil
+      new_access_level_name = nil
+
+      changed_attributes.each do |attr_name, changes|
+        new_access_level_name = authorizable_object.humanize if changes.last.present?
+        old_access_level_name = humanize(attr_name.to_sym, changes.first) if changes.first.present?
+      end
+
+      [old_access_level_name, new_access_level_name]
+    end
+
+    def humanize(type, value)
+      case type
+      when :access_level
+        ::ProtectedEnvironments::DeployAccessLevel::HUMAN_ACCESS_LEVELS[value]
+      when :user_id
+        User.find(value).name
+      when :group_id
+        Group.find(value).name
+      end
     end
   end
 end
diff --git a/ee/config/audit_events/types/protected_environment_updated.yml b/ee/config/audit_events/types/protected_environment_updated.yml
new file mode 100644
index 00000000000000..0a8c06604c9c7a
--- /dev/null
+++ b/ee/config/audit_events/types/protected_environment_updated.yml
@@ -0,0 +1,9 @@
+---
+name: protected_environment_updated
+description: This event is triggered when a protected environment is updated.
+introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/415603
+introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494
+feature_category: environment_management
+milestone: '16.4'
+saved_to_database: true
+streamed: false
diff --git a/ee/spec/factories/protected_environments.rb b/ee/spec/factories/protected_environments.rb
index 92307b0b9bf7ca..3c81703cc7bf0a 100644
--- a/ee/spec/factories/protected_environments.rb
+++ b/ee/spec/factories/protected_environments.rb
@@ -7,15 +7,20 @@
     transient do
       authorize_user_to_deploy { nil }
       authorize_group_to_deploy { nil }
+      require_user_to_approve { nil }
     end
 
     after(:build) do |protected_environment, evaluator|
-      if user = evaluator.authorize_user_to_deploy
-        protected_environment.deploy_access_levels.new(user: user)
+      if deploy_user = evaluator.authorize_user_to_deploy
+        protected_environment.deploy_access_levels.new(user: deploy_user)
       end
 
-      if group = evaluator.authorize_group_to_deploy
-        protected_environment.deploy_access_levels.new(group: group)
+      if deploy_group = evaluator.authorize_group_to_deploy
+        protected_environment.deploy_access_levels.new(group: deploy_group)
+      end
+
+      if approve_user = evaluator.require_user_to_approve
+        protected_environment.approval_rules.new(user_id: approve_user.id, required_approvals: 1)
       end
     end
 
@@ -26,20 +31,53 @@
     end
 
     trait :admins_can_deploy do
-      after(:build) do |protected_environment|
+      transient do
+        admins_approval_count { nil }
+      end
+
+      after(:build) do |protected_environment, evaluator|
         protected_environment.deploy_access_levels.new(access_level: Gitlab::Access::ADMIN)
+
+        if evaluator.admins_approval_count
+          protected_environment.approval_rules.new(
+            access_level: Gitlab::Access::ADMIN,
+            required_approvals: evaluator.admins_approval_count
+          )
+        end
       end
     end
 
     trait :maintainers_can_deploy do
-      after(:build) do |protected_environment|
+      transient do
+        maintainers_approval_count { nil }
+      end
+
+      after(:build) do |protected_environment, evaluator|
         protected_environment.deploy_access_levels.new(access_level: Gitlab::Access::MAINTAINER)
+
+        if evaluator.maintainers_approval_count
+          protected_environment.approval_rules.new(
+            access_level: Gitlab::Access::MAINTAINER,
+            required_approvals: evaluator.maintainers_approval_count
+          )
+        end
       end
     end
 
     trait :developers_can_deploy do
-      after(:build) do |protected_environment|
+      transient do
+        developers_approval_count { nil }
+      end
+
+      after(:build) do |protected_environment, evaluator|
         protected_environment.deploy_access_levels.new(access_level: Gitlab::Access::DEVELOPER)
+
+        if evaluator.developers_approval_count
+          protected_environment.approval_rules.new(
+            access_level: Gitlab::Access::DEVELOPER,
+            required_approvals: evaluator.developers_approval_count
+          )
+        end
       end
     end
 
diff --git a/ee/spec/services/protected_environments/update_service_spec.rb b/ee/spec/services/protected_environments/update_service_spec.rb
index 0ce5ba17ed2b1d..2efffbe1f3ffb5 100644
--- a/ee/spec/services/protected_environments/update_service_spec.rb
+++ b/ee/spec/services/protected_environments/update_service_spec.rb
@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 require 'spec_helper'
 
-RSpec.describe ProtectedEnvironments::UpdateService, '#execute' do
+RSpec.describe ProtectedEnvironments::UpdateService, '#execute', feature_category: :environment_management do
   let(:project) { create(:project) }
   let(:user) { create(:user) }
   let(:maintainer_access) { Gitlab::Access::MAINTAINER }
@@ -31,6 +31,101 @@
         subject
       end.to change { ProtectedEnvironments::DeployAccessLevel.count }.from(1).to(2)
     end
+
+    describe 'change auditing' do
+      before do
+        project.project_group_links.create!(
+          group_id: invited_group.id,
+          group_access: Gitlab::Access::DEVELOPER,
+          expires_at: 1.month.from_now
+        )
+      end
+
+      let(:invited_group) { create(:group) }
+      let(:project_user) { project.users.first }
+
+      let(:protected_environment) do
+        create(
+          :protected_environment,
+          :admins_can_deploy,
+          :maintainers_can_deploy,
+          admins_approval_count: 1,
+          maintainers_approval_count: 1,
+          require_user_to_approve: project_user,
+          name: 'staging',
+          required_approval_count: 1,
+          project: project
+        )
+      end
+
+      let(:params_with_expected_audit_messages) do
+        update_params = {}
+        messages = []
+
+        update_params[:required_approval_count] = 3
+        messages << "Updated required_approval_count from 1 to 3."
+
+        update_params[:deploy_access_levels_attributes] = [].tap do |update_attrs|
+          deploy_access_for_delete = protected_environment.deploy_access_levels[0]
+          update_attrs << { id: deploy_access_for_delete.id, _destroy: true }
+          messages << "Deleted deploy access level for #{deploy_access_for_delete.humanize}."
+
+          deploy_access_for_update = protected_environment.deploy_access_levels[1]
+          update_attrs << { id: deploy_access_for_update.id, access_level: nil, user_id: project_user.id }
+          messages << "Updated deploy access level from #{deploy_access_for_update.humanize} to #{project_user.name}."
+
+          update_attrs << { access_level: Gitlab::Access::DEVELOPER }
+          messages << "Added deploy access level for #{humanize_access_level(Gitlab::Access::DEVELOPER)}."
+        end
+
+        update_params[:approval_rules_attributes] = [].tap do |update_attrs|
+          approval_rule_for_delete = protected_environment.approval_rules[0]
+          update_attrs << { id: approval_rule_for_delete.id, _destroy: true }
+          messages << "Deleted approval rule for #{approval_rule_for_delete.humanize}."
+
+          approval_rule_for_update_1 = protected_environment.approval_rules[1]
+          update_attrs << {
+            id: approval_rule_for_update_1.id,
+            access_level: nil, user_id: nil,
+            group_id: invited_group.id,
+            required_approvals: 5
+          }
+          messages << "Updated approval rule for #{approval_rule_for_update_1.humanize} " \
+                      "with required approval count 1 " \
+                      "to #{invited_group.name} with required approval count 5."
+
+          approval_rule_for_update_2 = protected_environment.approval_rules[2]
+          update_attrs << { id: approval_rule_for_update_2.id, required_approvals: 4 }
+          messages << "Updated approval rule for #{approval_rule_for_update_2.humanize} " \
+                      "with required approval count from 1 to 4."
+
+          update_attrs << { access_level: Gitlab::Access::DEVELOPER, required_approvals: 1 }
+          messages << "Added approval rule for #{humanize_access_level(Gitlab::Access::DEVELOPER)} " \
+                      "with required approval count 1."
+        end
+
+        { params: update_params, expected_audit_messages: messages }
+      end
+
+      let(:params) { params_with_expected_audit_messages[:params] }
+      let(:expected_audit_messages) { params_with_expected_audit_messages[:expected_audit_messages] }
+
+      let(:expected_audit_context) do
+        {
+          name: 'protected_environment_updated',
+          author: user,
+          scope: project,
+          target: protected_environment,
+          message: expected_audit_messages.join(' ')
+        }
+      end
+
+      it 'stores and logs the audit event' do
+        expect(::Gitlab::Audit::Auditor).to receive(:audit).with(expected_audit_context)
+
+        subject
+      end
+    end
   end
 
   context 'with invalid params' do
@@ -52,6 +147,14 @@
 
       it_behaves_like 'invalid multiple deployment access levels'
     end
+
+    describe 'change auditing' do
+      it 'does not log any audit events' do
+        expect(::Gitlab::Audit::Auditor).not_to receive(:audit).with(name: 'protected_environment_updated')
+
+        subject
+      end
+    end
   end
 
   context 'deploy access level by group' do
@@ -72,4 +175,8 @@
 
     it_behaves_like 'valid protected environment user'
   end
+
+  def humanize_access_level(access_level)
+    ::ProtectedEnvironments::DeployAccessLevel::HUMAN_ACCESS_LEVELS[access_level]
+  end
 end
-- 
GitLab


From 3b83552c97dd6d1eb6cf595d800169d1b744efb6 Mon Sep 17 00:00:00 2001
From: Pam Artiaga <partiaga@gitlab.com>
Date: Tue, 12 Sep 2023 13:21:34 +1000
Subject: [PATCH 2/9] Add new audit event types

Use separate audit event types for value object changes

Add new audit events to doc
---
 .../audit_event_types.md                      |   8 +-
 .../protected_environments/update_service.rb  | 114 +++++---
 ...tected_environment_approval_rule_added.yml |   9 +
 ...cted_environment_approval_rule_deleted.yml |   9 +
 ...cted_environment_approval_rule_updated.yml |   9 +
 ..._environment_deploy_access_level_added.yml |   9 +
 ...nvironment_deploy_access_level_deleted.yml |   9 +
 ...nvironment_deploy_access_level_updated.yml |   9 +
 .../update_service_spec.rb                    | 272 +++++++++++-------
 9 files changed, 306 insertions(+), 142 deletions(-)
 create mode 100644 ee/config/audit_events/types/protected_environment_approval_rule_added.yml
 create mode 100644 ee/config/audit_events/types/protected_environment_approval_rule_deleted.yml
 create mode 100644 ee/config/audit_events/types/protected_environment_approval_rule_updated.yml
 create mode 100644 ee/config/audit_events/types/protected_environment_deploy_access_level_added.yml
 create mode 100644 ee/config/audit_events/types/protected_environment_deploy_access_level_deleted.yml
 create mode 100644 ee/config/audit_events/types/protected_environment_deploy_access_level_updated.yml

diff --git a/doc/administration/audit_event_streaming/audit_event_types.md b/doc/administration/audit_event_streaming/audit_event_types.md
index d87f6372dafecc..efcfb3305b1ed3 100644
--- a/doc/administration/audit_event_streaming/audit_event_types.md
+++ b/doc/administration/audit_event_streaming/audit_event_types.md
@@ -246,6 +246,13 @@ audit events to external destinations.
 | [`protected_branch_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/92074) | Triggered when a protected branch is created | **{check-circle}** Yes | **{check-circle}** Yes | `source_code_management` | GitLab [15.2](https://gitlab.com/gitlab-org/gitlab/-/issues/363091) |
 | [`protected_branch_removed`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/92074) | Triggered when a protected branch is removed | **{check-circle}** Yes | **{check-circle}** Yes | `source_code_management` | GitLab [15.2](https://gitlab.com/gitlab-org/gitlab/-/issues/363091) |
 | [`protected_branch_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/107530) | Event triggered on the setting for protected branches is update | **{check-circle}** Yes | **{check-circle}** Yes | `source_code_management` | GitLab [15.8](https://gitlab.com/gitlab-org/gitlab/-/issues/369318) |
+| [`protected_environment_approval_rule_added`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494) | This event is triggered when an approval rule is added to a protected environment. | **{check-circle}** Yes | **{dotted-circle}** No | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
+| [`protected_environment_approval_rule_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494) | This event is triggered when an approval rule is removed from a protected environment. | **{check-circle}** Yes | **{dotted-circle}** No | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
+| [`protected_environment_approval_rule_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494) | This event is triggered when an approval rule of a protected environment is updated. | **{check-circle}** Yes | **{dotted-circle}** No | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
+| [`protected_environment_deploy_access_level_added`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494) | This event is triggered when a deploy access level is added to a protected environment. | **{check-circle}** Yes | **{dotted-circle}** No | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
+| [`protected_environment_deploy_access_level_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494) | This event is triggered when a deploy access level is removed from a protected environment. | **{check-circle}** Yes | **{dotted-circle}** No | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
+| [`protected_environment_deploy_access_level_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494) | This event is triggered when a deploy access level of a protected environment is updated | **{check-circle}** Yes | **{dotted-circle}** No | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
+| [`protected_environment_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494) | This event is triggered when a protected environment is updated. | **{check-circle}** Yes | **{dotted-circle}** No | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
 | [`registration_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/123080) | Event triggered when a user registers for instance access | **{check-circle}** Yes | **{check-circle}** Yes | `compliance_management` | GitLab [16.3](https://gitlab.com/gitlab-org/gitlab/-/issues/374107) |
 | [`release_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/111080) | Event triggered when a release is created | **{check-circle}** Yes | **{check-circle}** Yes | `compliance_management` | GitLab [15.9](https://gitlab.com/gitlab-org/gitlab/-/issues/374111) |
 | [`release_deleted_audit_event`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/111080) | Event triggered when a release is deleted | **{check-circle}** Yes | **{check-circle}** Yes | `compliance_management` | GitLab [15.9](https://gitlab.com/gitlab-org/gitlab/-/issues/374111) |
@@ -304,4 +311,3 @@ audit events to external destinations.
 | [`feature_flag_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/113453) | Triggered when a feature flag is deleted. | **{check-circle}** Yes | **{check-circle}** Yes | `feature_flags` | GitLab [15.10](https://gitlab.com/gitlab-org/gitlab/-/issues/374109) |
 | [`feature_flag_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/113453) | Triggered when a feature flag is updated. | **{check-circle}** Yes | **{check-circle}** Yes | `feature_flags` | GitLab [15.10](https://gitlab.com/gitlab-org/gitlab/-/issues/374109) |
 | [`manually_trigger_housekeeping`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/112095) | Triggered when manually triggering housekeeping via API or admin UI | **{check-circle}** Yes | **{check-circle}** Yes | `source_code_management` | GitLab [15.9](https://gitlab.com/gitlab-org/gitlab/-/issues/390761) |
-| [`protected_environment_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494) | This event is triggered when a protected environment is updated. | **{check-circle}** Yes | **{dotted-circle}** No | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
diff --git a/ee/app/services/protected_environments/update_service.rb b/ee/app/services/protected_environments/update_service.rb
index f950ded309fcfc..221e5fa46ed6bd 100644
--- a/ee/app/services/protected_environments/update_service.rb
+++ b/ee/app/services/protected_environments/update_service.rb
@@ -7,11 +7,11 @@ class UpdateService < ProtectedEnvironments::BaseService
     def execute(protected_environment)
       protected_environment.assign_attributes(sanitized_params)
 
-      prepared_audit_context = audit_context(protected_environment)
+      prepared_audit_contexts = audit_contexts(protected_environment)
 
       result = protected_environment.save
 
-      log_audit_event(prepared_audit_context) if successful?(result) # TODO: do not log if no changes
+      log_audit_event(prepared_audit_contexts) if successful?(result)
 
       result
     end
@@ -22,68 +22,96 @@ def successful?(result)
       result == true
     end
 
-    def log_audit_event(prepared_audit_context)
-      ::Gitlab::Audit::Auditor.audit(prepared_audit_context)
+    def log_audit_event(prepared_audit_contexts)
+      prepared_audit_contexts.each do |audit_context|
+        ::Gitlab::Audit::Auditor.audit(audit_context)
+      end
     end
 
-    def audit_context(protected_environment)
-      {
-        name: 'protected_environment_updated',
-        author: current_user,
-        scope: container,
-        target: protected_environment,
-        message: audit_messages(protected_environment).join(' ')
-      }
+    def audit_contexts(protected_environment)
+      events = []
+
+      events << changed_attributes_audit_context(protected_environment)
+      events += changed_deploy_access_levels_audit_contexts(protected_environment)
+      events += changed_approval_rules_audit_contexts(protected_environment)
+
+      events.compact
     end
 
-    def audit_messages(protected_environment)
-      messages = []
+    def changed_attributes_audit_context(protected_environment)
+      return nil if (protected_environment.changes.symbolize_keys.keys & AUDITABLE_ATTRIBUTES).empty?
 
-      messages += changed_attributes_messages(protected_environment)
-      messages += changed_deploy_access_levels_messages(protected_environment)
-      messages += changed_approval_rules_messages(protected_environment)
+      changed_attributes_message(protected_environment)
 
-      messages
+      build_audit_context(
+        protected_environment: protected_environment,
+        event_name: 'protected_environment_updated',
+        message: changed_attributes_message(protected_environment)
+      )
     end
 
-    def changed_attributes_messages(protected_environment)
+    def changed_attributes_message(protected_environment)
       protected_environment.changes.slice(*AUDITABLE_ATTRIBUTES).map do |attr_name, changes|
         "Updated #{attr_name} from #{changes.first} to #{changes.last}."
-      end
+      end.join(' ')
     end
 
-    def changed_deploy_access_levels_messages(protected_environment)
+    def changed_deploy_access_levels_audit_contexts(protected_environment)
       protected_environment.deploy_access_levels.filter_map do |deploy_access_level|
         if deploy_access_level.new_record?
-          "Added deploy access level for #{deploy_access_level.humanize}."
+          build_audit_context(
+            protected_environment: protected_environment,
+            event_name: 'protected_environment_deploy_access_level_added',
+            message: "Added deploy access level #{deploy_access_level.humanize}."
+          )
         elsif deploy_access_level.marked_for_destruction?
-          "Deleted deploy access level for #{deploy_access_level.humanize}."
-        else
-          updated_deploy_access_level_message(deploy_access_level)
+          build_audit_context(
+            protected_environment: protected_environment,
+            event_name: 'protected_environment_deploy_access_level_deleted',
+            message: "Deleted deploy access level #{deploy_access_level.humanize}."
+          )
+        elsif (deploy_access_level.changes.symbolize_keys.keys & AUTHORIZABLE_ATTRIBUTES).present?
+          build_audit_context(
+            protected_environment: protected_environment,
+            event_name: 'protected_environment_deploy_access_level_updated',
+            message: updated_deploy_access_level_message(deploy_access_level)
+          )
         end
       end
     end
 
-    def updated_deploy_access_level_message(deploy_access_level)
-      changed_access_levels = changed_access_level_details(deploy_access_level)
-      return if changed_access_levels.empty?
-
-      "Updated deploy access level from #{changed_access_levels.first} to #{changed_access_levels.last}."
-    end
-
-    def changed_approval_rules_messages(protected_environment)
+    def changed_approval_rules_audit_contexts(protected_environment)
       protected_environment.approval_rules.filter_map do |approval_rule|
         if approval_rule.new_record?
-          "Added approval rule for #{approval_rule.humanize} " \
-            "with required approval count #{approval_rule.required_approvals}."
+          build_audit_context(
+            protected_environment: protected_environment,
+            event_name: 'protected_environment_approval_rule_added',
+            message: "Added approval rule for #{approval_rule.humanize} " \
+                     "with required approval count #{approval_rule.required_approvals}."
+          )
         elsif approval_rule.marked_for_destruction?
-          "Deleted approval rule for #{approval_rule.humanize}."
-        else
-          updated_approval_rule_message(approval_rule)
+          build_audit_context(
+            protected_environment: protected_environment,
+            event_name: 'protected_environment_approval_rule_deleted',
+            message: "Deleted approval rule for #{approval_rule.humanize}."
+          )
+        elsif (approval_rule.changes.symbolize_keys.keys & (AUTHORIZABLE_ATTRIBUTES+[:required_approvals])).present?
+          build_audit_context(
+            protected_environment: protected_environment,
+            event_name: 'protected_environment_approval_rule_updated',
+            message: updated_approval_rule_message(approval_rule)
+          )
         end
       end
     end
 
+    def updated_deploy_access_level_message(deploy_access_level)
+      changed_access_levels = changed_access_level_details(deploy_access_level)
+      return if changed_access_levels.empty?
+
+      "Changed deploy access level from #{changed_access_levels.first} to #{changed_access_levels.last}."
+    end
+
     def updated_approval_rule_message(approval_rule)
       changed_access_levels = changed_access_level_details(approval_rule)
       changed_approval_counts = approval_rule.changes[:required_approvals]
@@ -126,6 +154,16 @@ def changed_access_level_details(authorizable_object)
       [old_access_level_name, new_access_level_name]
     end
 
+    def build_audit_context(protected_environment:, event_name:, message:)
+      {
+        name: event_name,
+        author: current_user,
+        scope: container,
+        target: protected_environment,
+        message: message
+      }
+    end
+
     def humanize(type, value)
       case type
       when :access_level
diff --git a/ee/config/audit_events/types/protected_environment_approval_rule_added.yml b/ee/config/audit_events/types/protected_environment_approval_rule_added.yml
new file mode 100644
index 00000000000000..0acd0e11e2ea47
--- /dev/null
+++ b/ee/config/audit_events/types/protected_environment_approval_rule_added.yml
@@ -0,0 +1,9 @@
+---
+name: protected_environment_approval_rule_added
+description: This event is triggered when an approval rule is added to a protected environment.
+introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/415603
+introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494
+feature_category: environment_management
+milestone: '16.4'
+saved_to_database: true
+streamed: false
diff --git a/ee/config/audit_events/types/protected_environment_approval_rule_deleted.yml b/ee/config/audit_events/types/protected_environment_approval_rule_deleted.yml
new file mode 100644
index 00000000000000..0014743f085a62
--- /dev/null
+++ b/ee/config/audit_events/types/protected_environment_approval_rule_deleted.yml
@@ -0,0 +1,9 @@
+---
+name: protected_environment_approval_rule_deleted
+description: This event is triggered when an approval rule is removed from a protected environment.
+introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/415603
+introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494
+feature_category: environment_management
+milestone: '16.4'
+saved_to_database: true
+streamed: false
diff --git a/ee/config/audit_events/types/protected_environment_approval_rule_updated.yml b/ee/config/audit_events/types/protected_environment_approval_rule_updated.yml
new file mode 100644
index 00000000000000..90a4b7cb07d113
--- /dev/null
+++ b/ee/config/audit_events/types/protected_environment_approval_rule_updated.yml
@@ -0,0 +1,9 @@
+---
+name: protected_environment_approval_rule_updated
+description: This event is triggered when an approval rule of a protected environment is updated.
+introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/415603
+introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494
+feature_category: environment_management
+milestone: '16.4'
+saved_to_database: true
+streamed: false
diff --git a/ee/config/audit_events/types/protected_environment_deploy_access_level_added.yml b/ee/config/audit_events/types/protected_environment_deploy_access_level_added.yml
new file mode 100644
index 00000000000000..700e18d1b4336a
--- /dev/null
+++ b/ee/config/audit_events/types/protected_environment_deploy_access_level_added.yml
@@ -0,0 +1,9 @@
+---
+name: protected_environment_deploy_access_level_added
+description: This event is triggered when a deploy access level is added to a protected environment.
+introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/415603
+introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494
+feature_category: environment_management
+milestone: '16.4'
+saved_to_database: true
+streamed: false
diff --git a/ee/config/audit_events/types/protected_environment_deploy_access_level_deleted.yml b/ee/config/audit_events/types/protected_environment_deploy_access_level_deleted.yml
new file mode 100644
index 00000000000000..a60e73584a7900
--- /dev/null
+++ b/ee/config/audit_events/types/protected_environment_deploy_access_level_deleted.yml
@@ -0,0 +1,9 @@
+---
+name: protected_environment_deploy_access_level_deleted
+description: This event is triggered when a deploy access level is removed from a protected environment.
+introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/415603
+introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494
+feature_category: environment_management
+milestone: '16.4'
+saved_to_database: true
+streamed: false
diff --git a/ee/config/audit_events/types/protected_environment_deploy_access_level_updated.yml b/ee/config/audit_events/types/protected_environment_deploy_access_level_updated.yml
new file mode 100644
index 00000000000000..9139a6bf5b79f5
--- /dev/null
+++ b/ee/config/audit_events/types/protected_environment_deploy_access_level_updated.yml
@@ -0,0 +1,9 @@
+---
+name: protected_environment_deploy_access_level_updated
+description: This event is triggered when a deploy access level of a protected environment is updated
+introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/415603
+introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494
+feature_category: environment_management
+milestone: '16.4'
+saved_to_database: true
+streamed: false
diff --git a/ee/spec/services/protected_environments/update_service_spec.rb b/ee/spec/services/protected_environments/update_service_spec.rb
index 2efffbe1f3ffb5..eea8bb54daae88 100644
--- a/ee/spec/services/protected_environments/update_service_spec.rb
+++ b/ee/spec/services/protected_environments/update_service_spec.rb
@@ -31,101 +31,6 @@
         subject
       end.to change { ProtectedEnvironments::DeployAccessLevel.count }.from(1).to(2)
     end
-
-    describe 'change auditing' do
-      before do
-        project.project_group_links.create!(
-          group_id: invited_group.id,
-          group_access: Gitlab::Access::DEVELOPER,
-          expires_at: 1.month.from_now
-        )
-      end
-
-      let(:invited_group) { create(:group) }
-      let(:project_user) { project.users.first }
-
-      let(:protected_environment) do
-        create(
-          :protected_environment,
-          :admins_can_deploy,
-          :maintainers_can_deploy,
-          admins_approval_count: 1,
-          maintainers_approval_count: 1,
-          require_user_to_approve: project_user,
-          name: 'staging',
-          required_approval_count: 1,
-          project: project
-        )
-      end
-
-      let(:params_with_expected_audit_messages) do
-        update_params = {}
-        messages = []
-
-        update_params[:required_approval_count] = 3
-        messages << "Updated required_approval_count from 1 to 3."
-
-        update_params[:deploy_access_levels_attributes] = [].tap do |update_attrs|
-          deploy_access_for_delete = protected_environment.deploy_access_levels[0]
-          update_attrs << { id: deploy_access_for_delete.id, _destroy: true }
-          messages << "Deleted deploy access level for #{deploy_access_for_delete.humanize}."
-
-          deploy_access_for_update = protected_environment.deploy_access_levels[1]
-          update_attrs << { id: deploy_access_for_update.id, access_level: nil, user_id: project_user.id }
-          messages << "Updated deploy access level from #{deploy_access_for_update.humanize} to #{project_user.name}."
-
-          update_attrs << { access_level: Gitlab::Access::DEVELOPER }
-          messages << "Added deploy access level for #{humanize_access_level(Gitlab::Access::DEVELOPER)}."
-        end
-
-        update_params[:approval_rules_attributes] = [].tap do |update_attrs|
-          approval_rule_for_delete = protected_environment.approval_rules[0]
-          update_attrs << { id: approval_rule_for_delete.id, _destroy: true }
-          messages << "Deleted approval rule for #{approval_rule_for_delete.humanize}."
-
-          approval_rule_for_update_1 = protected_environment.approval_rules[1]
-          update_attrs << {
-            id: approval_rule_for_update_1.id,
-            access_level: nil, user_id: nil,
-            group_id: invited_group.id,
-            required_approvals: 5
-          }
-          messages << "Updated approval rule for #{approval_rule_for_update_1.humanize} " \
-                      "with required approval count 1 " \
-                      "to #{invited_group.name} with required approval count 5."
-
-          approval_rule_for_update_2 = protected_environment.approval_rules[2]
-          update_attrs << { id: approval_rule_for_update_2.id, required_approvals: 4 }
-          messages << "Updated approval rule for #{approval_rule_for_update_2.humanize} " \
-                      "with required approval count from 1 to 4."
-
-          update_attrs << { access_level: Gitlab::Access::DEVELOPER, required_approvals: 1 }
-          messages << "Added approval rule for #{humanize_access_level(Gitlab::Access::DEVELOPER)} " \
-                      "with required approval count 1."
-        end
-
-        { params: update_params, expected_audit_messages: messages }
-      end
-
-      let(:params) { params_with_expected_audit_messages[:params] }
-      let(:expected_audit_messages) { params_with_expected_audit_messages[:expected_audit_messages] }
-
-      let(:expected_audit_context) do
-        {
-          name: 'protected_environment_updated',
-          author: user,
-          scope: project,
-          target: protected_environment,
-          message: expected_audit_messages.join(' ')
-        }
-      end
-
-      it 'stores and logs the audit event' do
-        expect(::Gitlab::Audit::Auditor).to receive(:audit).with(expected_audit_context)
-
-        subject
-      end
-    end
   end
 
   context 'with invalid params' do
@@ -147,14 +52,6 @@
 
       it_behaves_like 'invalid multiple deployment access levels'
     end
-
-    describe 'change auditing' do
-      it 'does not log any audit events' do
-        expect(::Gitlab::Audit::Auditor).not_to receive(:audit).with(name: 'protected_environment_updated')
-
-        subject
-      end
-    end
   end
 
   context 'deploy access level by group' do
@@ -176,6 +73,175 @@
     it_behaves_like 'valid protected environment user'
   end
 
+  describe 'auditing changes' do
+    before do
+      project.project_group_links.create!(
+        group_id: invited_group.id,
+        group_access: Gitlab::Access::DEVELOPER,
+        expires_at: 1.month.from_now
+      )
+
+      allow(::Gitlab::Audit::Auditor).to receive(:audit)
+    end
+
+    let(:invited_group) { create(:group) }
+    let(:project_user) { project.users.first }
+
+    let(:protected_environment) do
+      create(
+        :protected_environment,
+        :admins_can_deploy,
+        :maintainers_can_deploy,
+        admins_approval_count: 1,
+        maintainers_approval_count: 1,
+        require_user_to_approve: project_user,
+        name: 'staging',
+        required_approval_count: 1,
+        project: project
+      )
+    end
+
+    let!(:deploy_access_for_delete) { protected_environment.deploy_access_levels[0] }
+    let!(:deploy_access_for_update) { protected_environment.deploy_access_levels[1] }
+    let!(:approval_rule_for_delete) { protected_environment.approval_rules[0] }
+    let!(:approval_rule_for_update_1) { protected_environment.approval_rules[1] }
+    let!(:approval_rule_for_update_2) { protected_environment.approval_rules[2] }
+
+    let(:params) do
+      {
+        required_approval_count: 3,
+        deploy_access_levels_attributes: [
+          { id: deploy_access_for_delete.id, _destroy: true },
+          { id: deploy_access_for_update.id, access_level: nil, user_id: project_user.id },
+          { access_level: Gitlab::Access::DEVELOPER }
+        ],
+        approval_rules_attributes: [
+          { id: approval_rule_for_delete.id, _destroy: true },
+          {
+            id: approval_rule_for_update_1.id,
+            access_level: nil, user_id: nil,
+            group_id: invited_group.id,
+            required_approvals: 5
+          },
+          { id: approval_rule_for_update_2.id, required_approvals: 4 },
+          { access_level: Gitlab::Access::DEVELOPER, required_approvals: 1 }
+        ]
+      }
+    end
+
+    it 'stores and logs the audit event for the protected_environment update' do
+      subject
+
+      expected_audit_context = {
+        name: 'protected_environment_updated',
+        author: user,
+        scope: project,
+        target: protected_environment.reload,
+        message: "Updated required_approval_count from 1 to 3."
+      }
+
+      expect(::Gitlab::Audit::Auditor).to have_received(:audit).with(expected_audit_context)
+    end
+
+    it 'stores and logs the audit events for the changes in deploy_access_levels', :aggregate_failures do
+      deploy_access_for_update_old_access_level = deploy_access_for_update.humanize
+
+      subject
+
+      expected_audit_context = {
+        author: user,
+        scope: project,
+        target: protected_environment.reload
+      }
+
+      expect(::Gitlab::Audit::Auditor).to have_received(:audit).with(
+        expected_audit_context.merge(
+          name: 'protected_environment_deploy_access_level_deleted',
+          message: "Deleted deploy access level #{deploy_access_for_delete.humanize}."
+        )
+      )
+      expect(::Gitlab::Audit::Auditor).to have_received(:audit).with(
+        expected_audit_context.merge(
+          name: 'protected_environment_deploy_access_level_updated',
+          message: "Changed deploy access level from #{deploy_access_for_update_old_access_level} to #{project_user.name}."
+        )
+      )
+      expect(::Gitlab::Audit::Auditor).to have_received(:audit).with(
+        expected_audit_context.merge(
+          name: 'protected_environment_deploy_access_level_added',
+          message: "Added deploy access level #{humanize_access_level(Gitlab::Access::DEVELOPER)}."
+        )
+      )
+    end
+
+    it 'stores and logs the audit events for the changes in approval_rules' do
+      approval_rule_for_update_1_old_access_level = approval_rule_for_update_1.humanize
+      approval_rule_for_update_2_old_access_level = approval_rule_for_update_2.humanize
+
+      subject
+
+      expected_audit_context = {
+        author: user,
+        scope: project,
+        target: protected_environment.reload
+      }
+
+      expect(::Gitlab::Audit::Auditor).to have_received(:audit).with(
+        expected_audit_context.merge(
+          name: 'protected_environment_approval_rule_deleted',
+          message: "Deleted approval rule for #{approval_rule_for_delete.humanize}."
+        )
+      )
+      expect(::Gitlab::Audit::Auditor).to have_received(:audit).with(
+        expected_audit_context.merge(
+          name: 'protected_environment_approval_rule_updated',
+          message: "Updated approval rule for #{approval_rule_for_update_1_old_access_level} " \
+                   "with required approval count 1 " \
+                   "to #{invited_group.name} with required approval count 5."
+        )
+      )
+      expect(::Gitlab::Audit::Auditor).to have_received(:audit).with(
+        expected_audit_context.merge(
+          name: 'protected_environment_approval_rule_updated',
+          message: "Updated approval rule for #{approval_rule_for_update_2_old_access_level} " \
+                   "with required approval count from 1 to 4."
+        )
+      )
+      expect(::Gitlab::Audit::Auditor).to have_received(:audit).with(
+        expected_audit_context.merge(
+          name: 'protected_environment_approval_rule_added',
+          message: "Added approval rule for #{humanize_access_level(Gitlab::Access::DEVELOPER)} " \
+                   "with required approval count 1."
+        )
+      )
+    end
+
+    context 'when there are updates to non-auditable attributes only' do
+      let(:params) do
+        {
+          deploy_access_levels_attributes: [
+            { id: deploy_access_for_update.id, group_inheritance_type: 1 }
+          ],
+          approval_rules_attributes: [
+            { id: approval_rule_for_update_2.id, group_inheritance_type: 1 }
+          ]
+        }
+      end
+
+      it 'does not log any audit event' do
+        expect(::Gitlab::Audit::Auditor).not_to receive(:audit).with(hash_including(name: 'protected_environment_updated'))
+        expect(::Gitlab::Audit::Auditor).not_to receive(:audit).with(hash_including(name: 'protected_environment_deploy_access_level_added'))
+        expect(::Gitlab::Audit::Auditor).not_to receive(:audit).with(hash_including(name: 'protected_environment_deploy_access_level_updated'))
+        expect(::Gitlab::Audit::Auditor).not_to receive(:audit).with(hash_including(name: 'protected_environment_deploy_access_level_deleted'))
+        expect(::Gitlab::Audit::Auditor).not_to receive(:audit).with(hash_including(name: 'protected_environment_approval_rule_added'))
+        expect(::Gitlab::Audit::Auditor).not_to receive(:audit).with(hash_including(name: 'protected_environment_approval_rule_updated'))
+        expect(::Gitlab::Audit::Auditor).not_to receive(:audit).with(hash_including(name: 'protected_environment_approval_rule_deleted'))
+
+        subject
+      end
+    end
+  end
+
   def humanize_access_level(access_level)
     ::ProtectedEnvironments::DeployAccessLevel::HUMAN_ACCESS_LEVELS[access_level]
   end
-- 
GitLab


From 910661518c587db545ae5da8869c4cc9092968c6 Mon Sep 17 00:00:00 2001
From: Pam Artiaga <partiaga@gitlab.com>
Date: Tue, 12 Sep 2023 14:30:47 +1000
Subject: [PATCH 3/9] Extract rule changes auditing to another class

Add spec for rule changes auditor

Make update messages more readable

Make rubocop happy

Use let_it_be instead of let
---
 .../authorization_rule_changes_auditor.rb     | 143 +++++++++++++++++
 .../protected_environments/update_service.rb  | 151 +++---------------
 ...authorization_rule_changes_auditor_spec.rb | 135 ++++++++++++++++
 .../update_service_spec.rb                    |  41 +++--
 4 files changed, 324 insertions(+), 146 deletions(-)
 create mode 100644 ee/app/services/protected_environments/authorization_rule_changes_auditor.rb
 create mode 100644 ee/spec/services/protected_environments/authorization_rule_changes_auditor_spec.rb

diff --git a/ee/app/services/protected_environments/authorization_rule_changes_auditor.rb b/ee/app/services/protected_environments/authorization_rule_changes_auditor.rb
new file mode 100644
index 00000000000000..05e28c076f426f
--- /dev/null
+++ b/ee/app/services/protected_environments/authorization_rule_changes_auditor.rb
@@ -0,0 +1,143 @@
+# frozen_string_literal: true
+
+module ProtectedEnvironments
+  class AuthorizationRuleChangesAuditor
+    AUTHORIZABLE_ATTRIBUTES = [:access_level, :user_id, :group_id].freeze
+
+    def initialize(author:, scope:, protected_environment:)
+      @author = author
+      @scope = scope
+      @protected_environment = protected_environment
+    end
+
+    def build_audit_contexts
+      (
+        changed_deploy_access_levels_audit_contexts +
+        changed_approval_rules_audit_contexts
+      ).compact
+    end
+
+    private
+
+    attr_reader :author, :scope, :protected_environment
+
+    def changed_deploy_access_levels_audit_contexts
+      protected_environment.deploy_access_levels.filter_map do |deploy_access_level|
+        if deploy_access_level.new_record?
+          build_audit_context(
+            event_name: 'protected_environment_deploy_access_level_added',
+            message: "Added deploy access level #{deploy_access_level.humanize}."
+          )
+        elsif deploy_access_level.marked_for_destruction?
+          build_audit_context(
+            event_name: 'protected_environment_deploy_access_level_deleted',
+            message: "Deleted deploy access level #{deploy_access_level.humanize}."
+          )
+        elsif deploy_access_level_has_changes?(deploy_access_level)
+          build_audit_context(
+            event_name: 'protected_environment_deploy_access_level_updated',
+            message: updated_deploy_access_level_message(deploy_access_level)
+          )
+        end
+      end
+    end
+
+    def deploy_access_level_has_changes?(deploy_access_level)
+      (deploy_access_level.changes.symbolize_keys.keys & AUTHORIZABLE_ATTRIBUTES).present?
+    end
+
+    def updated_deploy_access_level_message(deploy_access_level)
+      changed_access_levels = changed_access_level_details(deploy_access_level)
+      "Changed deploy access level from #{changed_access_levels[:old_access_level]} " \
+        "to #{changed_access_levels[:new_access_level]}."
+    end
+
+    def changed_approval_rules_audit_contexts
+      protected_environment.approval_rules.filter_map do |approval_rule|
+        if approval_rule.new_record?
+          build_audit_context(
+            event_name: 'protected_environment_approval_rule_added',
+            message: "Added approval rule for #{approval_rule.humanize} " \
+                     "with required approval count #{approval_rule.required_approvals}."
+          )
+        elsif approval_rule.marked_for_destruction?
+          build_audit_context(
+            event_name: 'protected_environment_approval_rule_deleted',
+            message: "Deleted approval rule for #{approval_rule.humanize}."
+          )
+        elsif approval_rule_has_changes?(approval_rule)
+          build_audit_context(
+            event_name: 'protected_environment_approval_rule_updated',
+            message: updated_approval_rule_message(approval_rule)
+          )
+        end
+      end
+    end
+
+    def approval_rule_has_changes?(approval_rule)
+      auditable_attributes = AUTHORIZABLE_ATTRIBUTES + [:required_approvals]
+      (approval_rule.changes.symbolize_keys.keys & auditable_attributes).present?
+    end
+
+    def updated_approval_rule_message(approval_rule)
+      changed_access_levels = changed_access_level_details(approval_rule)
+      changed_approval_counts = approval_rule.changes[:required_approvals]
+
+      if changed_access_levels.present?
+        update_approval_rule_access_level_message(approval_rule, changed_access_levels, changed_approval_counts)
+      else
+        update_approval_rule_approval_count_message(approval_rule, changed_approval_counts)
+      end
+    end
+
+    def update_approval_rule_access_level_message(approval_rule, changed_access_levels, changed_approval_counts)
+      old_access_level_approval_count = changed_approval_counts&.first || approval_rule.required_approvals
+      new_access_level_approval_count = approval_rule.required_approvals
+
+      "Updated approval rule for #{changed_access_levels[:old_access_level]} " \
+        "with required approval count #{old_access_level_approval_count} " \
+        "to #{changed_access_levels[:new_access_level]} " \
+        "with required approval count #{new_access_level_approval_count}."
+    end
+
+    def update_approval_rule_approval_count_message(approval_rule, changed_approval_counts)
+      "Updated approval rule for #{approval_rule.humanize} " \
+        "with required approval count " \
+        "from #{changed_approval_counts.first} " \
+        "to #{changed_approval_counts.last}."
+    end
+
+    def changed_access_level_details(authorizable_object)
+      changes = authorizable_object.changes.slice(*AUTHORIZABLE_ATTRIBUTES)
+      return unless changes.present?
+
+      old_access_level = changes.detect { |_, changed_values| changed_values.first.present? }
+
+      {
+        old_access_level: humanize(old_access_level[0], old_access_level[1].first),
+        new_access_level: authorizable_object.humanize
+      }
+    end
+
+    def build_audit_context(event_name:, message:)
+      {
+        name: event_name,
+        author: author,
+        scope: scope,
+        target: protected_environment,
+        message: message
+      }
+    end
+
+    def humanize(type, value)
+      case type.to_sym
+      when :access_level
+        ::ProtectedEnvironments::DeployAccessLevel::HUMAN_ACCESS_LEVELS[value]
+      when :user_id
+        User.find(value).name
+      when :group_id
+        Group.find(value).name
+      end
+    end
+  end
+end
diff --git a/ee/app/services/protected_environments/update_service.rb b/ee/app/services/protected_environments/update_service.rb
index 221e5fa46ed6bd..6eb92e2078f0c5 100644
--- a/ee/app/services/protected_environments/update_service.rb
+++ b/ee/app/services/protected_environments/update_service.rb
@@ -2,12 +2,11 @@
 module ProtectedEnvironments
   class UpdateService < ProtectedEnvironments::BaseService
     AUDITABLE_ATTRIBUTES = [:required_approval_count].freeze
-    AUTHORIZABLE_ATTRIBUTES = [:access_level, :user_id, :group_id].freeze
 
     def execute(protected_environment)
       protected_environment.assign_attributes(sanitized_params)
 
-      prepared_audit_contexts = audit_contexts(protected_environment)
+      prepared_audit_contexts = prepare_audit_contexts(protected_environment)
 
       result = protected_environment.save
 
@@ -28,151 +27,37 @@ def log_audit_event(prepared_audit_contexts)
       end
     end
 
-    def audit_contexts(protected_environment)
-      events = []
-
-      events << changed_attributes_audit_context(protected_environment)
-      events += changed_deploy_access_levels_audit_contexts(protected_environment)
-      events += changed_approval_rules_audit_contexts(protected_environment)
-
-      events.compact
+    def prepare_audit_contexts(protected_environment)
+      (
+        changed_attributes_audit_context(protected_environment) +
+        rule_changes_audit_contexts(protected_environment)
+      ).compact
     end
 
-    def changed_attributes_audit_context(protected_environment)
-      return nil if (protected_environment.changes.symbolize_keys.keys & AUDITABLE_ATTRIBUTES).empty?
-
-      changed_attributes_message(protected_environment)
-
-      build_audit_context(
-        protected_environment: protected_environment,
-        event_name: 'protected_environment_updated',
-        message: changed_attributes_message(protected_environment)
+    def rule_changes_audit_contexts(protected_environment)
+      rule_changes_auditor = ::ProtectedEnvironments::AuthorizationRuleChangesAuditor.new(
+        author: current_user,
+        scope: container,
+        protected_environment: protected_environment
       )
-    end
-
-    def changed_attributes_message(protected_environment)
-      protected_environment.changes.slice(*AUDITABLE_ATTRIBUTES).map do |attr_name, changes|
-        "Updated #{attr_name} from #{changes.first} to #{changes.last}."
-      end.join(' ')
-    end
-
-    def changed_deploy_access_levels_audit_contexts(protected_environment)
-      protected_environment.deploy_access_levels.filter_map do |deploy_access_level|
-        if deploy_access_level.new_record?
-          build_audit_context(
-            protected_environment: protected_environment,
-            event_name: 'protected_environment_deploy_access_level_added',
-            message: "Added deploy access level #{deploy_access_level.humanize}."
-          )
-        elsif deploy_access_level.marked_for_destruction?
-          build_audit_context(
-            protected_environment: protected_environment,
-            event_name: 'protected_environment_deploy_access_level_deleted',
-            message: "Deleted deploy access level #{deploy_access_level.humanize}."
-          )
-        elsif (deploy_access_level.changes.symbolize_keys.keys & AUTHORIZABLE_ATTRIBUTES).present?
-          build_audit_context(
-            protected_environment: protected_environment,
-            event_name: 'protected_environment_deploy_access_level_updated',
-            message: updated_deploy_access_level_message(deploy_access_level)
-          )
-        end
-      end
-    end
-
-    def changed_approval_rules_audit_contexts(protected_environment)
-      protected_environment.approval_rules.filter_map do |approval_rule|
-        if approval_rule.new_record?
-          build_audit_context(
-            protected_environment: protected_environment,
-            event_name: 'protected_environment_approval_rule_added',
-            message: "Added approval rule for #{approval_rule.humanize} " \
-                     "with required approval count #{approval_rule.required_approvals}."
-          )
-        elsif approval_rule.marked_for_destruction?
-          build_audit_context(
-            protected_environment: protected_environment,
-            event_name: 'protected_environment_approval_rule_deleted',
-            message: "Deleted approval rule for #{approval_rule.humanize}."
-          )
-        elsif (approval_rule.changes.symbolize_keys.keys & (AUTHORIZABLE_ATTRIBUTES+[:required_approvals])).present?
-          build_audit_context(
-            protected_environment: protected_environment,
-            event_name: 'protected_environment_approval_rule_updated',
-            message: updated_approval_rule_message(approval_rule)
-          )
-        end
-      end
-    end
-
-    def updated_deploy_access_level_message(deploy_access_level)
-      changed_access_levels = changed_access_level_details(deploy_access_level)
-      return if changed_access_levels.empty?
 
-      "Changed deploy access level from #{changed_access_levels.first} to #{changed_access_levels.last}."
+      rule_changes_auditor.build_audit_contexts
     end
 
-    def updated_approval_rule_message(approval_rule)
-      changed_access_levels = changed_access_level_details(approval_rule)
-      changed_approval_counts = approval_rule.changes[:required_approvals]
-
-      return if changed_access_levels.empty? && changed_approval_counts.nil?
-
-      if changed_access_levels.present?
-        update_approval_rule_access_level_message(approval_rule, changed_access_levels, changed_approval_counts)
-      else
-        update_approval_rule_approval_count_message(approval_rule, changed_approval_counts)
-      end
-    end
-
-    def update_approval_rule_access_level_message(approval_rule, changed_access_levels, changed_approval_counts)
-      old_approval_count = changed_approval_counts ? changed_approval_counts.first : approval_rule.required_approvals
-      new_approval_count = changed_approval_counts ? changed_approval_counts.last : approval_rule.required_approvals
-
-      "Updated approval rule for #{changed_access_levels.first} with required approval count #{old_approval_count} " \
-        "to #{approval_rule.humanize} with required approval count #{new_approval_count}."
-    end
-
-    def update_approval_rule_approval_count_message(approval_rule, changed_approval_counts)
-      "Updated approval rule for #{approval_rule.humanize} with required approval count " \
-        "from #{changed_approval_counts.first} " \
-        "to #{changed_approval_counts.last}."
-    end
-
-    def changed_access_level_details(authorizable_object)
-      changed_attributes = authorizable_object.changes.slice(*AUTHORIZABLE_ATTRIBUTES)
-      return [] if changed_attributes.empty?
-
-      old_access_level_name = nil
-      new_access_level_name = nil
-
-      changed_attributes.each do |attr_name, changes|
-        new_access_level_name = authorizable_object.humanize if changes.last.present?
-        old_access_level_name = humanize(attr_name.to_sym, changes.first) if changes.first.present?
+    def changed_attributes_audit_context(protected_environment)
+      protected_environment.changes.slice(*AUDITABLE_ATTRIBUTES).map do |attr_name, changes|
+        build_audit_context(protected_environment, attr_name, changes)
       end
-
-      [old_access_level_name, new_access_level_name]
     end
 
-    def build_audit_context(protected_environment:, event_name:, message:)
+    def build_audit_context(protected_environment, attribute_name, changes)
       {
-        name: event_name,
+        name: 'protected_environment_updated',
         author: current_user,
         scope: container,
         target: protected_environment,
-        message: message
+        message: "Updated #{attribute_name} from #{changes.first} to #{changes.last}."
       }
     end
-
-    def humanize(type, value)
-      case type
-      when :access_level
-        ::ProtectedEnvironments::DeployAccessLevel::HUMAN_ACCESS_LEVELS[value]
-      when :user_id
-        User.find(value).name
-      when :group_id
-        Group.find(value).name
-      end
-    end
   end
 end
diff --git a/ee/spec/services/protected_environments/authorization_rule_changes_auditor_spec.rb b/ee/spec/services/protected_environments/authorization_rule_changes_auditor_spec.rb
new file mode 100644
index 00000000000000..f7712b4377bfe1
--- /dev/null
+++ b/ee/spec/services/protected_environments/authorization_rule_changes_auditor_spec.rb
@@ -0,0 +1,135 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ProtectedEnvironments::AuthorizationRuleChangesAuditor, '#build_audit_contexts', feature_category: :environment_management do
+  let_it_be(:project) { create(:project) }
+  let(:user) { create(:user) }
+  let(:invited_group) { create(:group) }
+  let(:project_user) { project.users.first }
+
+  let(:protected_environment) do
+    create(
+      :protected_environment,
+      :admins_can_deploy,
+      :maintainers_can_deploy,
+      admins_approval_count: 1,
+      maintainers_approval_count: 1,
+      require_user_to_approve: project_user,
+      name: 'staging',
+      required_approval_count: 1,
+      project: project
+    )
+  end
+
+  let(:deploy_access_for_delete) { protected_environment.deploy_access_levels[0] }
+  let(:deploy_access_for_update) { protected_environment.deploy_access_levels[1] }
+  let(:approval_rule_for_delete) { protected_environment.approval_rules[0] }
+  let(:approval_rule_for_update_1) { protected_environment.approval_rules[1] }
+  let(:approval_rule_for_update_2) { protected_environment.approval_rules[2] }
+
+  let(:params) do
+    {
+      deploy_access_levels_attributes: [
+        { id: deploy_access_for_delete.id, _destroy: true },
+        { id: deploy_access_for_update.id, access_level: nil, user_id: project_user.id },
+        { access_level: Gitlab::Access::DEVELOPER }
+      ],
+      approval_rules_attributes: [
+        { id: approval_rule_for_delete.id, _destroy: true },
+        {
+          id: approval_rule_for_update_1.id,
+          access_level: nil, user_id: nil,
+          group_id: invited_group.id,
+          required_approvals: 5
+        },
+        { id: approval_rule_for_update_2.id, required_approvals: 4 },
+        { access_level: Gitlab::Access::DEVELOPER, required_approvals: 1 }
+      ]
+    }
+  end
+
+  before do
+    project.project_group_links.create!(
+      group_id: invited_group.id,
+      group_access: Gitlab::Access::DEVELOPER,
+      expires_at: 1.month.from_now
+    )
+  end
+
+  subject do
+    protected_environment.assign_attributes(params)
+    described_class.new(author: user, scope: project, protected_environment: protected_environment).build_audit_contexts
+  end
+
+  it 'returns the correct audit contexts for deploy access levels changes' do
+    expected_audit_contexts = [
+      {
+        name: 'protected_environment_deploy_access_level_deleted',
+        author: user,
+        scope: project,
+        target: protected_environment,
+        message: "Deleted deploy access level #{deploy_access_for_delete.humanize}."
+      },
+      {
+        name: 'protected_environment_deploy_access_level_updated',
+        author: user,
+        scope: project,
+        target: protected_environment,
+        message: "Changed deploy access level from #{deploy_access_for_update.humanize} to #{project_user.name}."
+      },
+      {
+        name: 'protected_environment_deploy_access_level_added',
+        author: user,
+        scope: project,
+        target: protected_environment,
+        message: "Added deploy access level #{humanize_access_level(Gitlab::Access::DEVELOPER)}."
+      }
+    ]
+
+    expect(subject).to include(*expected_audit_contexts)
+  end
+
+  it 'returns the correct audit contexts for approval rules changes' do
+    expected_audit_contexts = [
+      {
+        name: 'protected_environment_approval_rule_deleted',
+        author: user,
+        scope: project,
+        target: protected_environment,
+        message: "Deleted approval rule for #{approval_rule_for_delete.humanize}."
+      },
+      {
+        name: 'protected_environment_approval_rule_updated',
+        author: user,
+        scope: project,
+        target: protected_environment,
+        message: "Updated approval rule for #{approval_rule_for_update_1.humanize} " \
+                 "with required approval count 1 " \
+                 "to #{invited_group.name} with required approval count 5."
+      },
+      {
+        name: 'protected_environment_approval_rule_updated',
+        author: user,
+        scope: project,
+        target: protected_environment,
+        message: "Updated approval rule for #{approval_rule_for_update_2.humanize} " \
+                 "with required approval count from 1 to 4."
+      },
+      {
+        name: 'protected_environment_approval_rule_added',
+        author: user,
+        scope: project,
+        target: protected_environment,
+        message: "Added approval rule for #{humanize_access_level(Gitlab::Access::DEVELOPER)} " \
+                 "with required approval count 1."
+      }
+    ]
+
+    expect(subject).to include(*expected_audit_contexts)
+  end
+
+  def humanize_access_level(access_level)
+    ::ProtectedEnvironments::DeployAccessLevel::HUMAN_ACCESS_LEVELS[access_level]
+  end
+end
diff --git a/ee/spec/services/protected_environments/update_service_spec.rb b/ee/spec/services/protected_environments/update_service_spec.rb
index eea8bb54daae88..1041e4bc562412 100644
--- a/ee/spec/services/protected_environments/update_service_spec.rb
+++ b/ee/spec/services/protected_environments/update_service_spec.rb
@@ -101,11 +101,11 @@
       )
     end
 
-    let!(:deploy_access_for_delete) { protected_environment.deploy_access_levels[0] }
-    let!(:deploy_access_for_update) { protected_environment.deploy_access_levels[1] }
-    let!(:approval_rule_for_delete) { protected_environment.approval_rules[0] }
-    let!(:approval_rule_for_update_1) { protected_environment.approval_rules[1] }
-    let!(:approval_rule_for_update_2) { protected_environment.approval_rules[2] }
+    let(:deploy_access_for_delete) { protected_environment.deploy_access_levels[0] }
+    let(:deploy_access_for_update) { protected_environment.deploy_access_levels[1] }
+    let(:approval_rule_for_delete) { protected_environment.approval_rules[0] }
+    let(:approval_rule_for_update_1) { protected_environment.approval_rules[1] }
+    let(:approval_rule_for_update_2) { protected_environment.approval_rules[2] }
 
     let(:params) do
       {
@@ -163,7 +163,8 @@
       expect(::Gitlab::Audit::Auditor).to have_received(:audit).with(
         expected_audit_context.merge(
           name: 'protected_environment_deploy_access_level_updated',
-          message: "Changed deploy access level from #{deploy_access_for_update_old_access_level} to #{project_user.name}."
+          message: "Changed deploy access level from " \
+                   "#{deploy_access_for_update_old_access_level} to #{project_user.name}."
         )
       )
       expect(::Gitlab::Audit::Auditor).to have_received(:audit).with(
@@ -229,13 +230,27 @@
       end
 
       it 'does not log any audit event' do
-        expect(::Gitlab::Audit::Auditor).not_to receive(:audit).with(hash_including(name: 'protected_environment_updated'))
-        expect(::Gitlab::Audit::Auditor).not_to receive(:audit).with(hash_including(name: 'protected_environment_deploy_access_level_added'))
-        expect(::Gitlab::Audit::Auditor).not_to receive(:audit).with(hash_including(name: 'protected_environment_deploy_access_level_updated'))
-        expect(::Gitlab::Audit::Auditor).not_to receive(:audit).with(hash_including(name: 'protected_environment_deploy_access_level_deleted'))
-        expect(::Gitlab::Audit::Auditor).not_to receive(:audit).with(hash_including(name: 'protected_environment_approval_rule_added'))
-        expect(::Gitlab::Audit::Auditor).not_to receive(:audit).with(hash_including(name: 'protected_environment_approval_rule_updated'))
-        expect(::Gitlab::Audit::Auditor).not_to receive(:audit).with(hash_including(name: 'protected_environment_approval_rule_deleted'))
+        expect(::Gitlab::Audit::Auditor).not_to receive(:audit).with(
+          name: 'protected_environment_updated'
+        )
+        expect(::Gitlab::Audit::Auditor).not_to receive(:audit).with(
+          name: 'protected_environment_deploy_access_level_added'
+        )
+        expect(::Gitlab::Audit::Auditor).not_to receive(:audit).with(
+          name: 'protected_environment_deploy_access_level_updated'
+        )
+        expect(::Gitlab::Audit::Auditor).not_to receive(:audit).with(
+          name: 'protected_environment_deploy_access_level_deleted'
+        )
+        expect(::Gitlab::Audit::Auditor).not_to receive(:audit).with(
+          name: 'protected_environment_approval_rule_added'
+        )
+        expect(::Gitlab::Audit::Auditor).not_to receive(:audit).with(
+          name: 'protected_environment_approval_rule_updated'
+        )
+        expect(::Gitlab::Audit::Auditor).not_to receive(:audit).with(
+          name: 'protected_environment_approval_rule_deleted'
+        )
 
         subject
       end
-- 
GitLab


From 5717022c8d36c2a59f81db20a7c7443d5daab8db Mon Sep 17 00:00:00 2001
From: Pam Artiaga <partiaga@gitlab.com>
Date: Wed, 13 Sep 2023 16:58:31 +1000
Subject: [PATCH 4/9] Make sure protected_environment events are streamed

---
 .../audit_event_streaming/audit_event_types.md     | 14 +++++++-------
 .../protected_environment_approval_rule_added.yml  |  2 +-
 ...protected_environment_approval_rule_deleted.yml |  2 +-
 ...protected_environment_approval_rule_updated.yml |  2 +-
 ...ected_environment_deploy_access_level_added.yml |  2 +-
 ...ted_environment_deploy_access_level_deleted.yml |  2 +-
 ...ted_environment_deploy_access_level_updated.yml |  2 +-
 .../types/protected_environment_updated.yml        |  2 +-
 8 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/doc/administration/audit_event_streaming/audit_event_types.md b/doc/administration/audit_event_streaming/audit_event_types.md
index efcfb3305b1ed3..d6d8b9cd2af456 100644
--- a/doc/administration/audit_event_streaming/audit_event_types.md
+++ b/doc/administration/audit_event_streaming/audit_event_types.md
@@ -246,13 +246,13 @@ audit events to external destinations.
 | [`protected_branch_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/92074) | Triggered when a protected branch is created | **{check-circle}** Yes | **{check-circle}** Yes | `source_code_management` | GitLab [15.2](https://gitlab.com/gitlab-org/gitlab/-/issues/363091) |
 | [`protected_branch_removed`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/92074) | Triggered when a protected branch is removed | **{check-circle}** Yes | **{check-circle}** Yes | `source_code_management` | GitLab [15.2](https://gitlab.com/gitlab-org/gitlab/-/issues/363091) |
 | [`protected_branch_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/107530) | Event triggered on the setting for protected branches is update | **{check-circle}** Yes | **{check-circle}** Yes | `source_code_management` | GitLab [15.8](https://gitlab.com/gitlab-org/gitlab/-/issues/369318) |
-| [`protected_environment_approval_rule_added`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494) | This event is triggered when an approval rule is added to a protected environment. | **{check-circle}** Yes | **{dotted-circle}** No | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
-| [`protected_environment_approval_rule_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494) | This event is triggered when an approval rule is removed from a protected environment. | **{check-circle}** Yes | **{dotted-circle}** No | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
-| [`protected_environment_approval_rule_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494) | This event is triggered when an approval rule of a protected environment is updated. | **{check-circle}** Yes | **{dotted-circle}** No | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
-| [`protected_environment_deploy_access_level_added`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494) | This event is triggered when a deploy access level is added to a protected environment. | **{check-circle}** Yes | **{dotted-circle}** No | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
-| [`protected_environment_deploy_access_level_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494) | This event is triggered when a deploy access level is removed from a protected environment. | **{check-circle}** Yes | **{dotted-circle}** No | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
-| [`protected_environment_deploy_access_level_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494) | This event is triggered when a deploy access level of a protected environment is updated | **{check-circle}** Yes | **{dotted-circle}** No | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
-| [`protected_environment_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494) | This event is triggered when a protected environment is updated. | **{check-circle}** Yes | **{dotted-circle}** No | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
+| [`protected_environment_approval_rule_added`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494) | This event is triggered when an approval rule is added to a protected environment. | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
+| [`protected_environment_approval_rule_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494) | This event is triggered when an approval rule is removed from a protected environment. | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
+| [`protected_environment_approval_rule_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494) | This event is triggered when an approval rule of a protected environment is updated. | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
+| [`protected_environment_deploy_access_level_added`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494) | This event is triggered when a deploy access level is added to a protected environment. | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
+| [`protected_environment_deploy_access_level_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494) | This event is triggered when a deploy access level is removed from a protected environment. | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
+| [`protected_environment_deploy_access_level_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494) | This event is triggered when a deploy access level of a protected environment is updated | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
+| [`protected_environment_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494) | This event is triggered when a protected environment is updated. | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
 | [`registration_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/123080) | Event triggered when a user registers for instance access | **{check-circle}** Yes | **{check-circle}** Yes | `compliance_management` | GitLab [16.3](https://gitlab.com/gitlab-org/gitlab/-/issues/374107) |
 | [`release_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/111080) | Event triggered when a release is created | **{check-circle}** Yes | **{check-circle}** Yes | `compliance_management` | GitLab [15.9](https://gitlab.com/gitlab-org/gitlab/-/issues/374111) |
 | [`release_deleted_audit_event`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/111080) | Event triggered when a release is deleted | **{check-circle}** Yes | **{check-circle}** Yes | `compliance_management` | GitLab [15.9](https://gitlab.com/gitlab-org/gitlab/-/issues/374111) |
diff --git a/ee/config/audit_events/types/protected_environment_approval_rule_added.yml b/ee/config/audit_events/types/protected_environment_approval_rule_added.yml
index 0acd0e11e2ea47..528db32b645e32 100644
--- a/ee/config/audit_events/types/protected_environment_approval_rule_added.yml
+++ b/ee/config/audit_events/types/protected_environment_approval_rule_added.yml
@@ -6,4 +6,4 @@ introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494
 feature_category: environment_management
 milestone: '16.4'
 saved_to_database: true
-streamed: false
+streamed: true
diff --git a/ee/config/audit_events/types/protected_environment_approval_rule_deleted.yml b/ee/config/audit_events/types/protected_environment_approval_rule_deleted.yml
index 0014743f085a62..bf84041cd60fe0 100644
--- a/ee/config/audit_events/types/protected_environment_approval_rule_deleted.yml
+++ b/ee/config/audit_events/types/protected_environment_approval_rule_deleted.yml
@@ -6,4 +6,4 @@ introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494
 feature_category: environment_management
 milestone: '16.4'
 saved_to_database: true
-streamed: false
+streamed: true
diff --git a/ee/config/audit_events/types/protected_environment_approval_rule_updated.yml b/ee/config/audit_events/types/protected_environment_approval_rule_updated.yml
index 90a4b7cb07d113..7b46e5edc6a9e8 100644
--- a/ee/config/audit_events/types/protected_environment_approval_rule_updated.yml
+++ b/ee/config/audit_events/types/protected_environment_approval_rule_updated.yml
@@ -6,4 +6,4 @@ introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494
 feature_category: environment_management
 milestone: '16.4'
 saved_to_database: true
-streamed: false
+streamed: true
diff --git a/ee/config/audit_events/types/protected_environment_deploy_access_level_added.yml b/ee/config/audit_events/types/protected_environment_deploy_access_level_added.yml
index 700e18d1b4336a..246c3be6824a2c 100644
--- a/ee/config/audit_events/types/protected_environment_deploy_access_level_added.yml
+++ b/ee/config/audit_events/types/protected_environment_deploy_access_level_added.yml
@@ -6,4 +6,4 @@ introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494
 feature_category: environment_management
 milestone: '16.4'
 saved_to_database: true
-streamed: false
+streamed: true
diff --git a/ee/config/audit_events/types/protected_environment_deploy_access_level_deleted.yml b/ee/config/audit_events/types/protected_environment_deploy_access_level_deleted.yml
index a60e73584a7900..b5c444cf16732f 100644
--- a/ee/config/audit_events/types/protected_environment_deploy_access_level_deleted.yml
+++ b/ee/config/audit_events/types/protected_environment_deploy_access_level_deleted.yml
@@ -6,4 +6,4 @@ introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494
 feature_category: environment_management
 milestone: '16.4'
 saved_to_database: true
-streamed: false
+streamed: true
diff --git a/ee/config/audit_events/types/protected_environment_deploy_access_level_updated.yml b/ee/config/audit_events/types/protected_environment_deploy_access_level_updated.yml
index 9139a6bf5b79f5..1617dfee34b625 100644
--- a/ee/config/audit_events/types/protected_environment_deploy_access_level_updated.yml
+++ b/ee/config/audit_events/types/protected_environment_deploy_access_level_updated.yml
@@ -6,4 +6,4 @@ introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494
 feature_category: environment_management
 milestone: '16.4'
 saved_to_database: true
-streamed: false
+streamed: true
diff --git a/ee/config/audit_events/types/protected_environment_updated.yml b/ee/config/audit_events/types/protected_environment_updated.yml
index 0a8c06604c9c7a..b805c2ecf4f459 100644
--- a/ee/config/audit_events/types/protected_environment_updated.yml
+++ b/ee/config/audit_events/types/protected_environment_updated.yml
@@ -6,4 +6,4 @@ introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494
 feature_category: environment_management
 milestone: '16.4'
 saved_to_database: true
-streamed: false
+streamed: true
-- 
GitLab


From 09827c2297062679b19c45f832930afd872ddbf3 Mon Sep 17 00:00:00 2001
From: Huzaifa Iftikhar <hiftikhar@gitlab.com>
Date: Fri, 15 Sep 2023 04:49:41 +0000
Subject: [PATCH 5/9] Update according to MR suggestions

Use changes instead of previous_changes

Update linked MR in audit_event types

Update audit event types docs

Do not use admin role in specs
---
 .../audit_event_types.md                      | 14 ++--
 .../protected_environments/authorizable.rb    |  3 +-
 .../authorization_rule_changes_auditor.rb     | 15 +++--
 .../environment_dropdown_service.rb           |  5 +-
 ...tected_environment_approval_rule_added.yml |  2 +-
 ...cted_environment_approval_rule_deleted.yml |  2 +-
 ...cted_environment_approval_rule_updated.yml |  2 +-
 ..._environment_deploy_access_level_added.yml |  2 +-
 ...nvironment_deploy_access_level_deleted.yml |  2 +-
 ...nvironment_deploy_access_level_updated.yml |  2 +-
 .../types/protected_environment_updated.yml   |  2 +-
 ee/spec/factories/protected_environments.rb   | 67 +++++--------------
 ...authorization_rule_changes_auditor_spec.rb | 38 +++++++----
 .../update_service_spec.rb                    | 48 +++++++------
 14 files changed, 93 insertions(+), 111 deletions(-)

diff --git a/doc/administration/audit_event_streaming/audit_event_types.md b/doc/administration/audit_event_streaming/audit_event_types.md
index d6d8b9cd2af456..44381061ef35f7 100644
--- a/doc/administration/audit_event_streaming/audit_event_types.md
+++ b/doc/administration/audit_event_streaming/audit_event_types.md
@@ -246,13 +246,13 @@ audit events to external destinations.
 | [`protected_branch_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/92074) | Triggered when a protected branch is created | **{check-circle}** Yes | **{check-circle}** Yes | `source_code_management` | GitLab [15.2](https://gitlab.com/gitlab-org/gitlab/-/issues/363091) |
 | [`protected_branch_removed`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/92074) | Triggered when a protected branch is removed | **{check-circle}** Yes | **{check-circle}** Yes | `source_code_management` | GitLab [15.2](https://gitlab.com/gitlab-org/gitlab/-/issues/363091) |
 | [`protected_branch_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/107530) | Event triggered on the setting for protected branches is update | **{check-circle}** Yes | **{check-circle}** Yes | `source_code_management` | GitLab [15.8](https://gitlab.com/gitlab-org/gitlab/-/issues/369318) |
-| [`protected_environment_approval_rule_added`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494) | This event is triggered when an approval rule is added to a protected environment. | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
-| [`protected_environment_approval_rule_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494) | This event is triggered when an approval rule is removed from a protected environment. | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
-| [`protected_environment_approval_rule_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494) | This event is triggered when an approval rule of a protected environment is updated. | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
-| [`protected_environment_deploy_access_level_added`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494) | This event is triggered when a deploy access level is added to a protected environment. | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
-| [`protected_environment_deploy_access_level_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494) | This event is triggered when a deploy access level is removed from a protected environment. | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
-| [`protected_environment_deploy_access_level_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494) | This event is triggered when a deploy access level of a protected environment is updated | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
-| [`protected_environment_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494) | This event is triggered when a protected environment is updated. | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
+| [`protected_environment_approval_rule_added`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484) | This event is triggered when an approval rule is added to a protected environment. | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
+| [`protected_environment_approval_rule_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484) | This event is triggered when an approval rule is removed from a protected environment. | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
+| [`protected_environment_approval_rule_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484) | This event is triggered when an approval rule of a protected environment is updated. | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
+| [`protected_environment_deploy_access_level_added`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484) | This event is triggered when a deploy access level is added to a protected environment. | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
+| [`protected_environment_deploy_access_level_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484) | This event is triggered when a deploy access level is removed from a protected environment. | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
+| [`protected_environment_deploy_access_level_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484) | This event is triggered when a deploy access level of a protected environment is updated | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
+| [`protected_environment_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484) | This event is triggered when a protected environment is updated. | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
 | [`registration_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/123080) | Event triggered when a user registers for instance access | **{check-circle}** Yes | **{check-circle}** Yes | `compliance_management` | GitLab [16.3](https://gitlab.com/gitlab-org/gitlab/-/issues/374107) |
 | [`release_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/111080) | Event triggered when a release is created | **{check-circle}** Yes | **{check-circle}** Yes | `compliance_management` | GitLab [15.9](https://gitlab.com/gitlab-org/gitlab/-/issues/374111) |
 | [`release_deleted_audit_event`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/111080) | Event triggered when a release is deleted | **{check-circle}** Yes | **{check-circle}** Yes | `compliance_management` | GitLab [15.9](https://gitlab.com/gitlab-org/gitlab/-/issues/374111) |
diff --git a/ee/app/models/protected_environments/authorizable.rb b/ee/app/models/protected_environments/authorizable.rb
index f3acef5b9ae1da..035f35de8e8264 100644
--- a/ee/app/models/protected_environments/authorizable.rb
+++ b/ee/app/models/protected_environments/authorizable.rb
@@ -25,8 +25,7 @@ module Authorizable
 
     HUMAN_ACCESS_LEVELS = {
       Gitlab::Access::MAINTAINER => 'Maintainers',
-      Gitlab::Access::DEVELOPER => 'Developers + Maintainers',
-      Gitlab::Access::ADMIN => 'Admin'
+      Gitlab::Access::DEVELOPER => 'Developers + Maintainers'
     }.freeze
 
     def check_access(user)
diff --git a/ee/app/services/protected_environments/authorization_rule_changes_auditor.rb b/ee/app/services/protected_environments/authorization_rule_changes_auditor.rb
index 05e28c076f426f..087180d7258da7 100644
--- a/ee/app/services/protected_environments/authorization_rule_changes_auditor.rb
+++ b/ee/app/services/protected_environments/authorization_rule_changes_auditor.rb
@@ -43,7 +43,7 @@ def changed_deploy_access_levels_audit_contexts
     end
 
     def deploy_access_level_has_changes?(deploy_access_level)
-      (deploy_access_level.changes.symbolize_keys.keys & AUTHORIZABLE_ATTRIBUTES).present?
+      deploy_access_level.changes.keys.any? { |key| AUTHORIZABLE_ATTRIBUTES.include?(key.to_sym) }
     end
 
     def updated_deploy_access_level_message(deploy_access_level)
@@ -76,7 +76,7 @@ def changed_approval_rules_audit_contexts
 
     def approval_rule_has_changes?(approval_rule)
       auditable_attributes = AUTHORIZABLE_ATTRIBUTES + [:required_approvals]
-      (approval_rule.changes.symbolize_keys.keys & auditable_attributes).present?
+      approval_rule.changes.keys.any? { |key| auditable_attributes.include?(key.to_sym) }
     end
 
     def updated_approval_rule_message(approval_rule)
@@ -112,10 +112,11 @@ def changed_access_level_details(authorizable_object)
       return unless changes.present?
 
       old_access_level = changes.detect { |_, changed_values| changed_values.first.present? }
+      new_access_level = changes.detect { |_, changed_values| changed_values.last.present? }
 
       {
-        old_access_level: humanize(old_access_level[0], old_access_level[1].first),
-        new_access_level: authorizable_object.humanize
+        old_access_level: humanize(type: old_access_level[0], value: old_access_level[1].first),
+        new_access_level: humanize(type: new_access_level[0], value: new_access_level[1].last)
       }
     end
 
@@ -129,14 +130,14 @@ def build_audit_context(event_name:, message:)
       }
     end
 
-    def humanize(type, value)
+    def humanize(type:, value:)
       case type.to_sym
       when :access_level
         ::ProtectedEnvironments::DeployAccessLevel::HUMAN_ACCESS_LEVELS[value]
       when :user_id
-        User.find(value).name
+        "user with ID #{value}"
       when :group_id
-        Group.find(value).name
+        "group with ID #{value}"
       end
     end
   end
diff --git a/ee/app/services/protected_environments/environment_dropdown_service.rb b/ee/app/services/protected_environments/environment_dropdown_service.rb
index bf7634e95a242b..f4f096223e32cc 100644
--- a/ee/app/services/protected_environments/environment_dropdown_service.rb
+++ b/ee/app/services/protected_environments/environment_dropdown_service.rb
@@ -12,10 +12,7 @@ def self.roles
     end
 
     def self.human_access_levels
-      ::ProtectedEnvironments::DeployAccessLevel::HUMAN_ACCESS_LEVELS.slice(
-        Gitlab::Access::MAINTAINER,
-        Gitlab::Access::DEVELOPER
-      )
+      ::ProtectedEnvironments::DeployAccessLevel::HUMAN_ACCESS_LEVELS
     end
   end
 end
diff --git a/ee/config/audit_events/types/protected_environment_approval_rule_added.yml b/ee/config/audit_events/types/protected_environment_approval_rule_added.yml
index 528db32b645e32..ec842b492ac167 100644
--- a/ee/config/audit_events/types/protected_environment_approval_rule_added.yml
+++ b/ee/config/audit_events/types/protected_environment_approval_rule_added.yml
@@ -2,7 +2,7 @@
 name: protected_environment_approval_rule_added
 description: This event is triggered when an approval rule is added to a protected environment.
 introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/415603
-introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494
+introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484
 feature_category: environment_management
 milestone: '16.4'
 saved_to_database: true
diff --git a/ee/config/audit_events/types/protected_environment_approval_rule_deleted.yml b/ee/config/audit_events/types/protected_environment_approval_rule_deleted.yml
index bf84041cd60fe0..db8a02f2171b36 100644
--- a/ee/config/audit_events/types/protected_environment_approval_rule_deleted.yml
+++ b/ee/config/audit_events/types/protected_environment_approval_rule_deleted.yml
@@ -2,7 +2,7 @@
 name: protected_environment_approval_rule_deleted
 description: This event is triggered when an approval rule is removed from a protected environment.
 introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/415603
-introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494
+introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484
 feature_category: environment_management
 milestone: '16.4'
 saved_to_database: true
diff --git a/ee/config/audit_events/types/protected_environment_approval_rule_updated.yml b/ee/config/audit_events/types/protected_environment_approval_rule_updated.yml
index 7b46e5edc6a9e8..dc89f5f682b41d 100644
--- a/ee/config/audit_events/types/protected_environment_approval_rule_updated.yml
+++ b/ee/config/audit_events/types/protected_environment_approval_rule_updated.yml
@@ -2,7 +2,7 @@
 name: protected_environment_approval_rule_updated
 description: This event is triggered when an approval rule of a protected environment is updated.
 introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/415603
-introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494
+introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484
 feature_category: environment_management
 milestone: '16.4'
 saved_to_database: true
diff --git a/ee/config/audit_events/types/protected_environment_deploy_access_level_added.yml b/ee/config/audit_events/types/protected_environment_deploy_access_level_added.yml
index 246c3be6824a2c..54482841045832 100644
--- a/ee/config/audit_events/types/protected_environment_deploy_access_level_added.yml
+++ b/ee/config/audit_events/types/protected_environment_deploy_access_level_added.yml
@@ -2,7 +2,7 @@
 name: protected_environment_deploy_access_level_added
 description: This event is triggered when a deploy access level is added to a protected environment.
 introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/415603
-introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494
+introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484
 feature_category: environment_management
 milestone: '16.4'
 saved_to_database: true
diff --git a/ee/config/audit_events/types/protected_environment_deploy_access_level_deleted.yml b/ee/config/audit_events/types/protected_environment_deploy_access_level_deleted.yml
index b5c444cf16732f..6d57ef86d31d39 100644
--- a/ee/config/audit_events/types/protected_environment_deploy_access_level_deleted.yml
+++ b/ee/config/audit_events/types/protected_environment_deploy_access_level_deleted.yml
@@ -2,7 +2,7 @@
 name: protected_environment_deploy_access_level_deleted
 description: This event is triggered when a deploy access level is removed from a protected environment.
 introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/415603
-introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494
+introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484
 feature_category: environment_management
 milestone: '16.4'
 saved_to_database: true
diff --git a/ee/config/audit_events/types/protected_environment_deploy_access_level_updated.yml b/ee/config/audit_events/types/protected_environment_deploy_access_level_updated.yml
index 1617dfee34b625..f75edfe9e5bed5 100644
--- a/ee/config/audit_events/types/protected_environment_deploy_access_level_updated.yml
+++ b/ee/config/audit_events/types/protected_environment_deploy_access_level_updated.yml
@@ -2,7 +2,7 @@
 name: protected_environment_deploy_access_level_updated
 description: This event is triggered when a deploy access level of a protected environment is updated
 introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/415603
-introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494
+introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484
 feature_category: environment_management
 milestone: '16.4'
 saved_to_database: true
diff --git a/ee/config/audit_events/types/protected_environment_updated.yml b/ee/config/audit_events/types/protected_environment_updated.yml
index b805c2ecf4f459..045efeddb454f6 100644
--- a/ee/config/audit_events/types/protected_environment_updated.yml
+++ b/ee/config/audit_events/types/protected_environment_updated.yml
@@ -2,7 +2,7 @@
 name: protected_environment_updated
 description: This event is triggered when a protected environment is updated.
 introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/415603
-introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130494
+introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484
 feature_category: environment_management
 milestone: '16.4'
 saved_to_database: true
diff --git a/ee/spec/factories/protected_environments.rb b/ee/spec/factories/protected_environments.rb
index 3c81703cc7bf0a..5aacb44ce56809 100644
--- a/ee/spec/factories/protected_environments.rb
+++ b/ee/spec/factories/protected_environments.rb
@@ -7,7 +7,9 @@
     transient do
       authorize_user_to_deploy { nil }
       authorize_group_to_deploy { nil }
-      require_user_to_approve { nil }
+      authorize_roles_to_deploy { [] }
+      require_users_to_approve { [] }
+      require_roles_to_approve { [] }
     end
 
     after(:build) do |protected_environment, evaluator|
@@ -19,65 +21,28 @@
         protected_environment.deploy_access_levels.new(group: deploy_group)
       end
 
-      if approve_user = evaluator.require_user_to_approve
-        protected_environment.approval_rules.new(user_id: approve_user.id, required_approvals: 1)
-      end
-    end
-
-    before(:create) do |protected_environment, evaluator|
-      if protected_environment.deploy_access_levels.empty?
-        protected_environment.deploy_access_levels.new(user: create(:user))
-      end
-    end
-
-    trait :admins_can_deploy do
-      transient do
-        admins_approval_count { nil }
-      end
-
-      after(:build) do |protected_environment, evaluator|
-        protected_environment.deploy_access_levels.new(access_level: Gitlab::Access::ADMIN)
-
-        if evaluator.admins_approval_count
-          protected_environment.approval_rules.new(
-            access_level: Gitlab::Access::ADMIN,
-            required_approvals: evaluator.admins_approval_count
-          )
+      if (roles = evaluator.authorize_roles_to_deploy).present?
+        roles.each do |role|
+          protected_environment.deploy_access_levels.new(access_level: role)
         end
       end
-    end
 
-    trait :maintainers_can_deploy do
-      transient do
-        maintainers_approval_count { nil }
+      if (approve_users = evaluator.require_users_to_approve).present?
+        approve_users.each do |approve_user|
+          protected_environment.approval_rules.new(user_id: approve_user.id, required_approvals: 1)
+        end
       end
 
-      after(:build) do |protected_environment, evaluator|
-        protected_environment.deploy_access_levels.new(access_level: Gitlab::Access::MAINTAINER)
-
-        if evaluator.maintainers_approval_count
-          protected_environment.approval_rules.new(
-            access_level: Gitlab::Access::MAINTAINER,
-            required_approvals: evaluator.maintainers_approval_count
-          )
+      if (approve_roles = evaluator.require_roles_to_approve).present?
+        approve_roles.each do |approve_role|
+          protected_environment.approval_rules.new(access_level: approve_role, required_approvals: 1)
         end
       end
     end
 
-    trait :developers_can_deploy do
-      transient do
-        developers_approval_count { nil }
-      end
-
-      after(:build) do |protected_environment, evaluator|
-        protected_environment.deploy_access_levels.new(access_level: Gitlab::Access::DEVELOPER)
-
-        if evaluator.developers_approval_count
-          protected_environment.approval_rules.new(
-            access_level: Gitlab::Access::DEVELOPER,
-            required_approvals: evaluator.developers_approval_count
-          )
-        end
+    before(:create) do |protected_environment, evaluator|
+      if protected_environment.deploy_access_levels.empty?
+        protected_environment.deploy_access_levels.new(user: create(:user))
       end
     end
 
diff --git a/ee/spec/services/protected_environments/authorization_rule_changes_auditor_spec.rb b/ee/spec/services/protected_environments/authorization_rule_changes_auditor_spec.rb
index f7712b4377bfe1..7991daf1640c74 100644
--- a/ee/spec/services/protected_environments/authorization_rule_changes_auditor_spec.rb
+++ b/ee/spec/services/protected_environments/authorization_rule_changes_auditor_spec.rb
@@ -7,26 +7,33 @@
   let(:user) { create(:user) }
   let(:invited_group) { create(:group) }
   let(:project_user) { project.users.first }
+  let(:admin_user) { create(:user, :admin) }
 
   let(:protected_environment) do
     create(
       :protected_environment,
-      :admins_can_deploy,
-      :maintainers_can_deploy,
-      admins_approval_count: 1,
-      maintainers_approval_count: 1,
-      require_user_to_approve: project_user,
+      authorize_user_to_deploy: admin_user,
+      authorize_roles_to_deploy: [Gitlab::Access::MAINTAINER],
+      require_users_to_approve: [admin_user, project_user],
+      require_roles_to_approve: [Gitlab::Access::MAINTAINER],
       name: 'staging',
       required_approval_count: 1,
       project: project
     )
   end
 
-  let(:deploy_access_for_delete) { protected_environment.deploy_access_levels[0] }
-  let(:deploy_access_for_update) { protected_environment.deploy_access_levels[1] }
-  let(:approval_rule_for_delete) { protected_environment.approval_rules[0] }
-  let(:approval_rule_for_update_1) { protected_environment.approval_rules[1] }
-  let(:approval_rule_for_update_2) { protected_environment.approval_rules[2] }
+  let(:deploy_access_levels) { protected_environment.deploy_access_levels }
+  let(:deploy_access_for_delete) { find_authorization_rule(deploy_access_levels, :user_id, admin_user.id) }
+  let(:deploy_access_for_update) do
+    find_authorization_rule(deploy_access_levels, :access_level, Gitlab::Access::MAINTAINER)
+  end
+
+  let(:approval_rules) { protected_environment.approval_rules }
+  let(:approval_rule_for_delete) { find_authorization_rule(approval_rules, :user_id, admin_user.id) }
+  let(:approval_rule_for_update_1) { find_authorization_rule(approval_rules, :user_id, project_user.id) }
+  let(:approval_rule_for_update_2) do
+    find_authorization_rule(approval_rules, :access_level, Gitlab::Access::MAINTAINER)
+  end
 
   let(:params) do
     {
@@ -76,7 +83,8 @@
         author: user,
         scope: project,
         target: protected_environment,
-        message: "Changed deploy access level from #{deploy_access_for_update.humanize} to #{project_user.name}."
+        message: "Changed deploy access level from #{deploy_access_for_update.humanize} " \
+                 "to user with ID #{project_user.id}."
       },
       {
         name: 'protected_environment_deploy_access_level_added',
@@ -104,9 +112,9 @@
         author: user,
         scope: project,
         target: protected_environment,
-        message: "Updated approval rule for #{approval_rule_for_update_1.humanize} " \
+        message: "Updated approval rule for user with ID #{project_user.id} " \
                  "with required approval count 1 " \
-                 "to #{invited_group.name} with required approval count 5."
+                 "to group with ID #{invited_group.id} with required approval count 5."
       },
       {
         name: 'protected_environment_approval_rule_updated',
@@ -132,4 +140,8 @@
   def humanize_access_level(access_level)
     ::ProtectedEnvironments::DeployAccessLevel::HUMAN_ACCESS_LEVELS[access_level]
   end
+
+  def find_authorization_rule(authorization_rules, attr_name, attr_value)
+    authorization_rules.detect { |ar| ar.send(attr_name) == attr_value }
+  end
 end
diff --git a/ee/spec/services/protected_environments/update_service_spec.rb b/ee/spec/services/protected_environments/update_service_spec.rb
index 1041e4bc562412..6485658016387a 100644
--- a/ee/spec/services/protected_environments/update_service_spec.rb
+++ b/ee/spec/services/protected_environments/update_service_spec.rb
@@ -84,28 +84,36 @@
       allow(::Gitlab::Audit::Auditor).to receive(:audit)
     end
 
+    let(:admin_user) { create(:user, :admin) }
+
     let(:invited_group) { create(:group) }
     let(:project_user) { project.users.first }
 
     let(:protected_environment) do
       create(
         :protected_environment,
-        :admins_can_deploy,
-        :maintainers_can_deploy,
-        admins_approval_count: 1,
-        maintainers_approval_count: 1,
-        require_user_to_approve: project_user,
+        authorize_user_to_deploy: admin_user,
+        authorize_roles_to_deploy: [Gitlab::Access::MAINTAINER],
+        require_users_to_approve: [admin_user, project_user],
+        require_roles_to_approve: [Gitlab::Access::MAINTAINER],
         name: 'staging',
         required_approval_count: 1,
         project: project
       )
     end
 
-    let(:deploy_access_for_delete) { protected_environment.deploy_access_levels[0] }
-    let(:deploy_access_for_update) { protected_environment.deploy_access_levels[1] }
-    let(:approval_rule_for_delete) { protected_environment.approval_rules[0] }
-    let(:approval_rule_for_update_1) { protected_environment.approval_rules[1] }
-    let(:approval_rule_for_update_2) { protected_environment.approval_rules[2] }
+    let(:deploy_access_levels) { protected_environment.deploy_access_levels }
+    let(:deploy_access_for_delete) { find_authorization_rule(deploy_access_levels, :user_id, admin_user.id) }
+    let(:deploy_access_for_update) do
+      find_authorization_rule(deploy_access_levels, :access_level, Gitlab::Access::MAINTAINER)
+    end
+
+    let(:approval_rules) { protected_environment.approval_rules }
+    let(:approval_rule_for_delete) { find_authorization_rule(approval_rules, :user_id, admin_user.id) }
+    let(:approval_rule_for_update_1) { find_authorization_rule(approval_rules, :user_id, project_user.id) }
+    let(:approval_rule_for_update_2) do
+      find_authorization_rule(approval_rules, :access_level, Gitlab::Access::MAINTAINER)
+    end
 
     let(:params) do
       {
@@ -144,8 +152,6 @@
     end
 
     it 'stores and logs the audit events for the changes in deploy_access_levels', :aggregate_failures do
-      deploy_access_for_update_old_access_level = deploy_access_for_update.humanize
-
       subject
 
       expected_audit_context = {
@@ -163,8 +169,9 @@
       expect(::Gitlab::Audit::Auditor).to have_received(:audit).with(
         expected_audit_context.merge(
           name: 'protected_environment_deploy_access_level_updated',
-          message: "Changed deploy access level from " \
-                   "#{deploy_access_for_update_old_access_level} to #{project_user.name}."
+          message: "Changed deploy access level " \
+                   "from #{humanize_access_level(Gitlab::Access::MAINTAINER)} " \
+                   "to user with ID #{project_user.id}."
         )
       )
       expect(::Gitlab::Audit::Auditor).to have_received(:audit).with(
@@ -176,9 +183,6 @@
     end
 
     it 'stores and logs the audit events for the changes in approval_rules' do
-      approval_rule_for_update_1_old_access_level = approval_rule_for_update_1.humanize
-      approval_rule_for_update_2_old_access_level = approval_rule_for_update_2.humanize
-
       subject
 
       expected_audit_context = {
@@ -196,15 +200,15 @@
       expect(::Gitlab::Audit::Auditor).to have_received(:audit).with(
         expected_audit_context.merge(
           name: 'protected_environment_approval_rule_updated',
-          message: "Updated approval rule for #{approval_rule_for_update_1_old_access_level} " \
+          message: "Updated approval rule for user with ID #{project_user.id} " \
                    "with required approval count 1 " \
-                   "to #{invited_group.name} with required approval count 5."
+                   "to group with ID #{invited_group.id} with required approval count 5."
         )
       )
       expect(::Gitlab::Audit::Auditor).to have_received(:audit).with(
         expected_audit_context.merge(
           name: 'protected_environment_approval_rule_updated',
-          message: "Updated approval rule for #{approval_rule_for_update_2_old_access_level} " \
+          message: "Updated approval rule for #{humanize_access_level(Gitlab::Access::MAINTAINER)} " \
                    "with required approval count from 1 to 4."
         )
       )
@@ -260,4 +264,8 @@
   def humanize_access_level(access_level)
     ::ProtectedEnvironments::DeployAccessLevel::HUMAN_ACCESS_LEVELS[access_level]
   end
+
+  def find_authorization_rule(authorization_rules, attr_name, attr_value)
+    authorization_rules.detect { |ar| ar.send(attr_name) == attr_value }
+  end
 end
-- 
GitLab


From d2e3d6883dfb81b54f297e000ed9b92583ca073e Mon Sep 17 00:00:00 2001
From: Pam Artiaga <partiaga@gitlab.com>
Date: Fri, 15 Sep 2023 17:06:33 +1000
Subject: [PATCH 6/9] Only prepare audit contexts after successful save

Move auditor class under Audit module

Remove unused methods

Add comment to explain changed access levels

Use audit_changes method

Fix specs, update accdg to suggestions
---
 .../authorization_rule_changes_auditor.rb     | 144 ------------
 .../protected_environments/update_service.rb  |  73 +++---
 ...ment_authorization_rule_changes_auditor.rb | 211 ++++++++++++++++++
 ee/spec/factories/protected_environments.rb   |  38 ++--
 ...uthorization_rule_changes_auditor_spec.rb} | 118 +++++-----
 .../update_service_spec.rb                    |  12 +-
 6 files changed, 347 insertions(+), 249 deletions(-)
 delete mode 100644 ee/app/services/protected_environments/authorization_rule_changes_auditor.rb
 create mode 100644 ee/lib/audit/protected_environment_authorization_rule_changes_auditor.rb
 rename ee/spec/{services/protected_environments/authorization_rule_changes_auditor_spec.rb => lib/audit/protected_environment_authorization_rule_changes_auditor_spec.rb} (62%)

diff --git a/ee/app/services/protected_environments/authorization_rule_changes_auditor.rb b/ee/app/services/protected_environments/authorization_rule_changes_auditor.rb
deleted file mode 100644
index 087180d7258da7..00000000000000
--- a/ee/app/services/protected_environments/authorization_rule_changes_auditor.rb
+++ /dev/null
@@ -1,144 +0,0 @@
-# frozen_string_literal: true
-
-module ProtectedEnvironments
-  class AuthorizationRuleChangesAuditor
-    AUTHORIZABLE_ATTRIBUTES = [:access_level, :user_id, :group_id].freeze
-
-    def initialize(author:, scope:, protected_environment:)
-      @author = author
-      @scope = scope
-      @protected_environment = protected_environment
-    end
-
-    def build_audit_contexts
-      (
-        changed_deploy_access_levels_audit_contexts +
-        changed_approval_rules_audit_contexts
-      ).compact
-    end
-
-    private
-
-    attr_reader :author, :scope, :protected_environment
-
-    def changed_deploy_access_levels_audit_contexts
-      protected_environment.deploy_access_levels.filter_map do |deploy_access_level|
-        if deploy_access_level.new_record?
-          build_audit_context(
-            event_name: 'protected_environment_deploy_access_level_added',
-            message: "Added deploy access level #{deploy_access_level.humanize}."
-          )
-        elsif deploy_access_level.marked_for_destruction?
-          build_audit_context(
-            event_name: 'protected_environment_deploy_access_level_deleted',
-            message: "Deleted deploy access level #{deploy_access_level.humanize}."
-          )
-        elsif deploy_access_level_has_changes?(deploy_access_level)
-          build_audit_context(
-            event_name: 'protected_environment_deploy_access_level_updated',
-            message: updated_deploy_access_level_message(deploy_access_level)
-          )
-        end
-      end
-    end
-
-    def deploy_access_level_has_changes?(deploy_access_level)
-      deploy_access_level.changes.keys.any? { |key| AUTHORIZABLE_ATTRIBUTES.include?(key.to_sym) }
-    end
-
-    def updated_deploy_access_level_message(deploy_access_level)
-      changed_access_levels = changed_access_level_details(deploy_access_level)
-      "Changed deploy access level from #{changed_access_levels[:old_access_level]} " \
-        "to #{changed_access_levels[:new_access_level]}."
-    end
-
-    def changed_approval_rules_audit_contexts
-      protected_environment.approval_rules.filter_map do |approval_rule|
-        if approval_rule.new_record?
-          build_audit_context(
-            event_name: 'protected_environment_approval_rule_added',
-            message: "Added approval rule for #{approval_rule.humanize} " \
-                     "with required approval count #{approval_rule.required_approvals}."
-          )
-        elsif approval_rule.marked_for_destruction?
-          build_audit_context(
-            event_name: 'protected_environment_approval_rule_deleted',
-            message: "Deleted approval rule for #{approval_rule.humanize}."
-          )
-        elsif approval_rule_has_changes?(approval_rule)
-          build_audit_context(
-            event_name: 'protected_environment_approval_rule_updated',
-            message: updated_approval_rule_message(approval_rule)
-          )
-        end
-      end
-    end
-
-    def approval_rule_has_changes?(approval_rule)
-      auditable_attributes = AUTHORIZABLE_ATTRIBUTES + [:required_approvals]
-      approval_rule.changes.keys.any? { |key| auditable_attributes.include?(key.to_sym) }
-    end
-
-    def updated_approval_rule_message(approval_rule)
-      changed_access_levels = changed_access_level_details(approval_rule)
-      changed_approval_counts = approval_rule.changes[:required_approvals]
-
-      if changed_access_levels.present?
-        update_approval_rule_access_level_message(approval_rule, changed_access_levels, changed_approval_counts)
-      else
-        update_approval_rule_approval_count_message(approval_rule, changed_approval_counts)
-      end
-    end
-
-    def update_approval_rule_access_level_message(approval_rule, changed_access_levels, changed_approval_counts)
-      old_access_level_approval_count = changed_approval_counts&.first || approval_rule.required_approvals
-      new_access_level_approval_count = approval_rule.required_approvals
-
-      "Updated approval rule for #{changed_access_levels[:old_access_level]} " \
-        "with required approval count #{old_access_level_approval_count} " \
-        "to #{changed_access_levels[:new_access_level]} " \
-        "with required approval count #{new_access_level_approval_count}."
-    end
-
-    def update_approval_rule_approval_count_message(approval_rule, changed_approval_counts)
-      "Updated approval rule for #{approval_rule.humanize} " \
-        "with required approval count " \
-        "from #{changed_approval_counts.first} " \
-        "to #{changed_approval_counts.last}."
-    end
-
-    def changed_access_level_details(authorizable_object)
-      changes = authorizable_object.changes.slice(*AUTHORIZABLE_ATTRIBUTES)
-      return unless changes.present?
-
-      old_access_level = changes.detect { |_, changed_values| changed_values.first.present? }
-      new_access_level = changes.detect { |_, changed_values| changed_values.last.present? }
-
-      {
-        old_access_level: humanize(type: old_access_level[0], value: old_access_level[1].first),
-        new_access_level: humanize(type: new_access_level[0], value: new_access_level[1].last)
-      }
-    end
-
-    def build_audit_context(event_name:, message:)
-      {
-        name: event_name,
-        author: author,
-        scope: scope,
-        target: protected_environment,
-        message: message
-      }
-    end
-
-    def humanize(type:, value:)
-      case type.to_sym
-      when :access_level
-        ::ProtectedEnvironments::DeployAccessLevel::HUMAN_ACCESS_LEVELS[value]
-      when :user_id
-        "user with ID #{value}"
-      when :group_id
-        "group with ID #{value}"
-      end
-    end
-  end
-end
diff --git a/ee/app/services/protected_environments/update_service.rb b/ee/app/services/protected_environments/update_service.rb
index 6eb92e2078f0c5..21e0307ffabbc5 100644
--- a/ee/app/services/protected_environments/update_service.rb
+++ b/ee/app/services/protected_environments/update_service.rb
@@ -1,18 +1,23 @@
 # frozen_string_literal: true
 module ProtectedEnvironments
   class UpdateService < ProtectedEnvironments::BaseService
+    include ::Audit::Changes
+
     AUDITABLE_ATTRIBUTES = [:required_approval_count].freeze
 
     def execute(protected_environment)
-      protected_environment.assign_attributes(sanitized_params)
-
-      prepared_audit_contexts = prepare_audit_contexts(protected_environment)
-
-      result = protected_environment.save
+      deploy_access_levels_before_save = protected_environment.deploy_access_levels.to_a
+      approval_rules_before_save = protected_environment.approval_rules.to_a
 
-      log_audit_event(prepared_audit_contexts) if successful?(result)
-
-      result
+      protected_environment.update(sanitized_params).tap do |is_updated|
+        if is_updated
+          log_audit_events(
+            protected_environment: protected_environment,
+            previous_deploy_access_levels: deploy_access_levels_before_save,
+            previous_approval_rules: approval_rules_before_save
+          )
+        end
+      end
     end
 
     private
@@ -21,43 +26,39 @@ def successful?(result)
       result == true
     end
 
-    def log_audit_event(prepared_audit_contexts)
-      prepared_audit_contexts.each do |audit_context|
-        ::Gitlab::Audit::Auditor.audit(audit_context)
-      end
-    end
-
-    def prepare_audit_contexts(protected_environment)
-      (
-        changed_attributes_audit_context(protected_environment) +
-        rule_changes_audit_contexts(protected_environment)
-      ).compact
-    end
+    def log_audit_events(protected_environment:, previous_deploy_access_levels:, previous_approval_rules:)
+      audit_changed_attributes(protected_environment)
 
-    def rule_changes_audit_contexts(protected_environment)
-      rule_changes_auditor = ::ProtectedEnvironments::AuthorizationRuleChangesAuditor.new(
-        author: current_user,
-        scope: container,
-        protected_environment: protected_environment
+      audit_authorization_rule_changes(
+        protected_environment: protected_environment,
+        previous_deploy_access_levels: previous_deploy_access_levels,
+        previous_approval_rules: previous_approval_rules
       )
-
-      rule_changes_auditor.build_audit_contexts
     end
 
-    def changed_attributes_audit_context(protected_environment)
-      protected_environment.changes.slice(*AUDITABLE_ATTRIBUTES).map do |attr_name, changes|
-        build_audit_context(protected_environment, attr_name, changes)
+    def audit_changed_attributes(protected_environment)
+      AUDITABLE_ATTRIBUTES.each do |attr_name|
+        audit_changes(
+          attr_name,
+          entity: container,
+          model: protected_environment,
+          event_type: 'protected_environment_updated'
+        )
       end
     end
 
-    def build_audit_context(protected_environment, attribute_name, changes)
-      {
-        name: 'protected_environment_updated',
+    def audit_authorization_rule_changes(
+      protected_environment:,
+      previous_deploy_access_levels:,
+      previous_approval_rules:
+    )
+      ::Audit::ProtectedEnvironmentAuthorizationRuleChangesAuditor.new(
         author: current_user,
         scope: container,
-        target: protected_environment,
-        message: "Updated #{attribute_name} from #{changes.first} to #{changes.last}."
-      }
+        protected_environment: protected_environment,
+        previous_deploy_access_levels: previous_deploy_access_levels,
+        previous_approval_rules: previous_approval_rules
+      ).audit
     end
   end
 end
diff --git a/ee/lib/audit/protected_environment_authorization_rule_changes_auditor.rb b/ee/lib/audit/protected_environment_authorization_rule_changes_auditor.rb
new file mode 100644
index 00000000000000..68a8e90a076877
--- /dev/null
+++ b/ee/lib/audit/protected_environment_authorization_rule_changes_auditor.rb
@@ -0,0 +1,211 @@
+# frozen_string_literal: true
+
+module Audit
+  class ProtectedEnvironmentAuthorizationRuleChangesAuditor
+    AUTHORIZABLE_ATTRIBUTES = [:access_level, :user_id, :group_id].freeze
+
+    def initialize(author:, scope:, protected_environment:, previous_deploy_access_levels:, previous_approval_rules:)
+      @author = author
+      @scope = scope
+      @protected_environment = protected_environment
+      @previous_deploy_access_levels = previous_deploy_access_levels
+      @previous_approval_rules = previous_approval_rules
+    end
+
+    def audit
+      audit_changed_deploy_access_levels
+      audit_changed_approval_rules
+    end
+
+    private
+
+    attr_reader :author, :scope, :protected_environment, :previous_deploy_access_levels, :previous_approval_rules
+
+    def audit_changed_deploy_access_levels
+      audit_deleted_deploy_access_levels
+      audit_updated_deploy_access_levels
+      audit_created_deploy_access_levels
+    end
+
+    def audit_deleted_deploy_access_levels
+      current_deploy_access_levels_ids = protected_environment.deploy_access_levels.map(&:id)
+      deleted_deploy_access_levels = previous_deploy_access_levels.select do |dal|
+        current_deploy_access_levels_ids.exclude?(dal.id)
+      end
+
+      deleted_deploy_access_levels.each do |deploy_access_level|
+        audit_event(
+          event_name: 'protected_environment_deploy_access_level_deleted',
+          message: "Deleted deploy access level #{deploy_access_level.humanize}."
+        )
+      end
+    end
+
+    def audit_created_deploy_access_levels
+      previous_deploy_access_levels_ids = previous_deploy_access_levels.map(&:id)
+      new_deploy_access_levels = protected_environment.deploy_access_levels.select do |dal|
+        previous_deploy_access_levels_ids.exclude?(dal.id)
+      end
+
+      new_deploy_access_levels.each do |deploy_access_level|
+        audit_event(
+          event_name: 'protected_environment_deploy_access_level_added',
+          message: "Added deploy access level #{deploy_access_level.humanize}."
+        )
+      end
+    end
+
+    def audit_updated_deploy_access_levels
+      protected_environment.deploy_access_levels.each do |deploy_access_level|
+        next unless deploy_access_level_has_changes?(deploy_access_level)
+
+        audit_event(
+          event_name: 'protected_environment_deploy_access_level_updated',
+          message: updated_deploy_access_level_message(deploy_access_level)
+        )
+      end
+    end
+
+    def audit_changed_approval_rules
+      audit_deleted_approval_rules
+      audit_updated_approval_rules
+      audit_created_approval_rules
+    end
+
+    def audit_deleted_approval_rules
+      current_approval_rules_ids = protected_environment.approval_rules.map(&:id)
+      deleted_approval_rules = previous_approval_rules.select do |dal|
+        current_approval_rules_ids.exclude?(dal.id)
+      end
+
+      deleted_approval_rules.each do |approval_rule|
+        audit_event(
+          event_name: 'protected_environment_approval_rule_deleted',
+          message: "Deleted approval rule for #{approval_rule.humanize}."
+        )
+      end
+    end
+
+    def audit_created_approval_rules
+      previous_approval_rules_ids = previous_approval_rules.map(&:id)
+      new_approval_rules = protected_environment.approval_rules.select do |dal|
+        previous_approval_rules_ids.exclude?(dal.id)
+      end
+
+      new_approval_rules.each do |approval_rule|
+        audit_event(
+          event_name: 'protected_environment_approval_rule_added',
+          message: "Added approval rule for #{approval_rule.humanize} " \
+                   "with required approval count #{approval_rule.required_approvals}."
+        )
+      end
+    end
+
+    def audit_updated_approval_rules
+      protected_environment.approval_rules.each do |approval_rule|
+        next unless approval_rule_has_changes?(approval_rule)
+
+        audit_event(
+          event_name: 'protected_environment_approval_rule_updated',
+          message: updated_approval_rule_message(approval_rule)
+        )
+      end
+    end
+
+    def deploy_access_level_has_changes?(deploy_access_level)
+      return false if deploy_access_level.previous_changes.keys.any? { |key| key.to_sym == :id }
+
+      deploy_access_level.previous_changes.keys.any? { |key| AUTHORIZABLE_ATTRIBUTES.include?(key.to_sym) }
+    end
+
+    def updated_deploy_access_level_message(deploy_access_level)
+      changed_access_levels = changed_access_level_details(deploy_access_level)
+      "Changed deploy access level from #{changed_access_levels[:old_access_level]} " \
+        "to #{changed_access_levels[:new_access_level]}."
+    end
+
+    def approval_rule_has_changes?(approval_rule)
+      return false if approval_rule.previous_changes.keys.any? { |key| key.to_sym == :id }
+
+      auditable_attributes = AUTHORIZABLE_ATTRIBUTES + [:required_approvals]
+      approval_rule.previous_changes.keys.any? { |key| auditable_attributes.include?(key.to_sym) }
+    end
+
+    def updated_approval_rule_message(approval_rule)
+      changed_access_levels = changed_access_level_details(approval_rule)
+      changed_approval_counts = approval_rule.previous_changes[:required_approvals]
+
+      if changed_access_levels.present?
+        update_approval_rule_access_level_message(approval_rule, changed_access_levels, changed_approval_counts)
+      else
+        update_approval_rule_approval_count_message(approval_rule, changed_approval_counts)
+      end
+    end
+
+    def update_approval_rule_access_level_message(approval_rule, changed_access_levels, changed_approval_counts)
+      old_access_level_approval_count = changed_approval_counts&.first || approval_rule.required_approvals
+      new_access_level_approval_count = approval_rule.required_approvals
+
+      "Updated approval rule for #{changed_access_levels[:old_access_level]} " \
+        "with required approval count #{old_access_level_approval_count} " \
+        "to #{changed_access_levels[:new_access_level]} " \
+        "with required approval count #{new_access_level_approval_count}."
+    end
+
+    def update_approval_rule_approval_count_message(approval_rule, changed_approval_counts)
+      "Updated approval rule for #{approval_rule.humanize} " \
+        "with required approval count " \
+        "from #{changed_approval_counts.first} " \
+        "to #{changed_approval_counts.last}."
+    end
+
+    def changed_access_level_details(authorizable_object)
+      # The AUTHORIZABLE_ATTRIBUTES (:access_level, :user_id, :group_id) are mutually exclusive
+      # This means that an authorizable_object (deploy_access_level or approval_rule)
+      # can only have one of :access_level, :user_id, or :group_id present
+      # A "change" can mean either of the following:
+      #   - the value of the same attribute is changed, e.g.: user_id=1 to user_id=2
+      #   - the value of one attribute becomes nil, and the value of another attribute is set,
+      #     e.g.: { user_id: 1, access_level: nil } to { user_id: nil, access_level: 40 }
+      changes = authorizable_object.previous_changes.slice(*AUTHORIZABLE_ATTRIBUTES)
+      return unless changes.present?
+
+      # changes are formatted as such: { attr_name: [old_value, new_value] }
+      # the old access level is determined by which attribute has the old_value present
+      # the new access level is determined by which attribute has the new_value present
+      old_access_level = changes.detect { |_, changed_values| changed_values.first.present? }
+      new_access_level = changes.detect { |_, changed_values| changed_values.last.present? }
+
+      {
+        old_access_level: humanize(type: old_access_level[0], value: old_access_level[1].first),
+        new_access_level: humanize(type: new_access_level[0], value: new_access_level[1].last)
+      }
+    end
+
+    def build_audit_context(event_name:, message:)
+      {
+        name: event_name,
+        author: author,
+        scope: scope,
+        target: protected_environment,
+        message: message
+      }
+    end
+
+    def humanize(type:, value:)
+      case type.to_sym
+      when :access_level
+        ::ProtectedEnvironments::DeployAccessLevel::HUMAN_ACCESS_LEVELS[value]
+      when :user_id
+        "user with ID #{value}"
+      when :group_id
+        "group with ID #{value}"
+      end
+    end
+
+    def audit_event(event_name:, message:)
+      audit_context = build_audit_context(event_name: event_name, message: message)
+      ::Gitlab::Audit::Auditor.audit(audit_context)
+    end
+  end
+end
diff --git a/ee/spec/factories/protected_environments.rb b/ee/spec/factories/protected_environments.rb
index 5aacb44ce56809..a2804f15cda58e 100644
--- a/ee/spec/factories/protected_environments.rb
+++ b/ee/spec/factories/protected_environments.rb
@@ -7,9 +7,7 @@
     transient do
       authorize_user_to_deploy { nil }
       authorize_group_to_deploy { nil }
-      authorize_roles_to_deploy { [] }
       require_users_to_approve { [] }
-      require_roles_to_approve { [] }
     end
 
     after(:build) do |protected_environment, evaluator|
@@ -21,23 +19,11 @@
         protected_environment.deploy_access_levels.new(group: deploy_group)
       end
 
-      if (roles = evaluator.authorize_roles_to_deploy).present?
-        roles.each do |role|
-          protected_environment.deploy_access_levels.new(access_level: role)
-        end
-      end
-
       if (approve_users = evaluator.require_users_to_approve).present?
         approve_users.each do |approve_user|
           protected_environment.approval_rules.new(user_id: approve_user.id, required_approvals: 1)
         end
       end
-
-      if (approve_roles = evaluator.require_roles_to_approve).present?
-        approve_roles.each do |approve_role|
-          protected_environment.approval_rules.new(access_level: approve_role, required_approvals: 1)
-        end
-      end
     end
 
     before(:create) do |protected_environment, evaluator|
@@ -46,6 +32,30 @@
       end
     end
 
+    trait :admins_can_deploy do
+      after(:build) do |protected_environment|
+        protected_environment.deploy_access_levels.new(access_level: Gitlab::Access::ADMIN)
+      end
+    end
+
+    trait :maintainers_can_deploy do
+      after(:build) do |protected_environment|
+        protected_environment.deploy_access_levels.new(access_level: Gitlab::Access::MAINTAINER)
+      end
+    end
+
+    trait :developers_can_deploy do
+      after(:build) do |protected_environment|
+        protected_environment.deploy_access_levels.new(access_level: Gitlab::Access::DEVELOPER)
+      end
+    end
+
+    trait :maintainers_can_approve do
+      after(:build) do |protected_environment|
+        protected_environment.approval_rules.new(access_level: Gitlab::Access::MAINTAINER, required_approvals: 1)
+      end
+    end
+
     trait :production do
       name { 'production' }
     end
diff --git a/ee/spec/services/protected_environments/authorization_rule_changes_auditor_spec.rb b/ee/spec/lib/audit/protected_environment_authorization_rule_changes_auditor_spec.rb
similarity index 62%
rename from ee/spec/services/protected_environments/authorization_rule_changes_auditor_spec.rb
rename to ee/spec/lib/audit/protected_environment_authorization_rule_changes_auditor_spec.rb
index 7991daf1640c74..6981ed5a739069 100644
--- a/ee/spec/services/protected_environments/authorization_rule_changes_auditor_spec.rb
+++ b/ee/spec/lib/audit/protected_environment_authorization_rule_changes_auditor_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe ProtectedEnvironments::AuthorizationRuleChangesAuditor, '#build_audit_contexts', feature_category: :environment_management do
+RSpec.describe Audit::ProtectedEnvironmentAuthorizationRuleChangesAuditor, '#audit', feature_category: :environment_management do
   let_it_be(:project) { create(:project) }
   let(:user) { create(:user) }
   let(:invited_group) { create(:group) }
@@ -12,10 +12,10 @@
   let(:protected_environment) do
     create(
       :protected_environment,
+      :maintainers_can_deploy,
+      :maintainers_can_approve,
       authorize_user_to_deploy: admin_user,
-      authorize_roles_to_deploy: [Gitlab::Access::MAINTAINER],
       require_users_to_approve: [admin_user, project_user],
-      require_roles_to_approve: [Gitlab::Access::MAINTAINER],
       name: 'staging',
       required_approval_count: 1,
       project: project
@@ -62,79 +62,93 @@
       group_access: Gitlab::Access::DEVELOPER,
       expires_at: 1.month.from_now
     )
+
+    allow(::Gitlab::Audit::Auditor).to receive(:audit)
   end
 
   subject do
-    protected_environment.assign_attributes(params)
-    described_class.new(author: user, scope: project, protected_environment: protected_environment).build_audit_contexts
+    previous_deploy_access_levels = protected_environment.deploy_access_levels.to_a
+    previous_approval_rules = protected_environment.approval_rules.to_a
+
+    protected_environment.update!(params)
+
+    described_class.new(
+      author: user,
+      scope: project,
+      protected_environment: protected_environment,
+      previous_deploy_access_levels: previous_deploy_access_levels,
+      previous_approval_rules: previous_approval_rules
+    ).audit
   end
 
-  it 'returns the correct audit contexts for deploy access levels changes' do
-    expected_audit_contexts = [
-      {
+  it 'stores and logs the audit events for the changes in deploy_access_levels', :aggregate_failures do
+    subject
+
+    expected_audit_context = {
+      author: user,
+      scope: project,
+      target: protected_environment.reload
+    }
+
+    expect(::Gitlab::Audit::Auditor).to have_received(:audit).with(
+      expected_audit_context.merge(
         name: 'protected_environment_deploy_access_level_deleted',
-        author: user,
-        scope: project,
-        target: protected_environment,
         message: "Deleted deploy access level #{deploy_access_for_delete.humanize}."
-      },
-      {
+      )
+    )
+    expect(::Gitlab::Audit::Auditor).to have_received(:audit).with(
+      expected_audit_context.merge(
         name: 'protected_environment_deploy_access_level_updated',
-        author: user,
-        scope: project,
-        target: protected_environment,
-        message: "Changed deploy access level from #{deploy_access_for_update.humanize} " \
+        message: "Changed deploy access level " \
+                 "from #{humanize_access_level(Gitlab::Access::MAINTAINER)} " \
                  "to user with ID #{project_user.id}."
-      },
-      {
+      )
+    )
+    expect(::Gitlab::Audit::Auditor).to have_received(:audit).with(
+      expected_audit_context.merge(
         name: 'protected_environment_deploy_access_level_added',
-        author: user,
-        scope: project,
-        target: protected_environment,
         message: "Added deploy access level #{humanize_access_level(Gitlab::Access::DEVELOPER)}."
-      }
-    ]
-
-    expect(subject).to include(*expected_audit_contexts)
+      )
+    )
   end
 
-  it 'returns the correct audit contexts for approval rules changes' do
-    expected_audit_contexts = [
-      {
+  it 'stores and logs the audit events for the changes in approval_rules', :aggregate_failures do
+    subject
+
+    expected_audit_context = {
+      author: user,
+      scope: project,
+      target: protected_environment.reload
+    }
+
+    expect(::Gitlab::Audit::Auditor).to have_received(:audit).with(
+      expected_audit_context.merge(
         name: 'protected_environment_approval_rule_deleted',
-        author: user,
-        scope: project,
-        target: protected_environment,
         message: "Deleted approval rule for #{approval_rule_for_delete.humanize}."
-      },
-      {
+      )
+    )
+    expect(::Gitlab::Audit::Auditor).to have_received(:audit).with(
+      expected_audit_context.merge(
         name: 'protected_environment_approval_rule_updated',
-        author: user,
-        scope: project,
-        target: protected_environment,
         message: "Updated approval rule for user with ID #{project_user.id} " \
                  "with required approval count 1 " \
                  "to group with ID #{invited_group.id} with required approval count 5."
-      },
-      {
+      )
+    )
+    expect(::Gitlab::Audit::Auditor).to have_received(:audit).with(
+      expected_audit_context.merge(
         name: 'protected_environment_approval_rule_updated',
-        author: user,
-        scope: project,
-        target: protected_environment,
-        message: "Updated approval rule for #{approval_rule_for_update_2.humanize} " \
+        message: "Updated approval rule for #{humanize_access_level(Gitlab::Access::MAINTAINER)} " \
                  "with required approval count from 1 to 4."
-      },
-      {
+      )
+    )
+    expect(::Gitlab::Audit::Auditor).to have_received(:audit).with(
+      expected_audit_context.merge(
         name: 'protected_environment_approval_rule_added',
-        author: user,
-        scope: project,
-        target: protected_environment,
         message: "Added approval rule for #{humanize_access_level(Gitlab::Access::DEVELOPER)} " \
                  "with required approval count 1."
-      }
-    ]
-
-    expect(subject).to include(*expected_audit_contexts)
+      )
+    )
   end
 
   def humanize_access_level(access_level)
diff --git a/ee/spec/services/protected_environments/update_service_spec.rb b/ee/spec/services/protected_environments/update_service_spec.rb
index 6485658016387a..000ea75ec3673b 100644
--- a/ee/spec/services/protected_environments/update_service_spec.rb
+++ b/ee/spec/services/protected_environments/update_service_spec.rb
@@ -92,10 +92,10 @@
     let(:protected_environment) do
       create(
         :protected_environment,
+        :maintainers_can_deploy,
+        :maintainers_can_approve,
         authorize_user_to_deploy: admin_user,
-        authorize_roles_to_deploy: [Gitlab::Access::MAINTAINER],
         require_users_to_approve: [admin_user, project_user],
-        require_roles_to_approve: [Gitlab::Access::MAINTAINER],
         name: 'staging',
         required_approval_count: 1,
         project: project
@@ -145,7 +145,13 @@
         author: user,
         scope: project,
         target: protected_environment.reload,
-        message: "Updated required_approval_count from 1 to 3."
+        message: "Changed required_approval_count from 1 to 3",
+        target_details: nil,
+        additional_details: {
+          change: :required_approval_count,
+          from: 1,
+          to: 3
+        }
       }
 
       expect(::Gitlab::Audit::Auditor).to have_received(:audit).with(expected_audit_context)
-- 
GitLab


From 56509bff53d32e2aadaf6da80cd134c2022a63a3 Mon Sep 17 00:00:00 2001
From: Pam Artiaga <partiaga@gitlab.com>
Date: Wed, 20 Sep 2023 11:58:14 +1000
Subject: [PATCH 7/9] Update milestone for audit event types

---
 .../audit_event_streaming/audit_event_types.md     | 14 +++++++-------
 .../protected_environment_approval_rule_added.yml  |  2 +-
 ...protected_environment_approval_rule_deleted.yml |  2 +-
 ...protected_environment_approval_rule_updated.yml |  2 +-
 ...ected_environment_deploy_access_level_added.yml |  2 +-
 ...ted_environment_deploy_access_level_deleted.yml |  2 +-
 ...ted_environment_deploy_access_level_updated.yml |  2 +-
 .../types/protected_environment_updated.yml        |  2 +-
 8 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/doc/administration/audit_event_streaming/audit_event_types.md b/doc/administration/audit_event_streaming/audit_event_types.md
index 44381061ef35f7..d3f123a18508e8 100644
--- a/doc/administration/audit_event_streaming/audit_event_types.md
+++ b/doc/administration/audit_event_streaming/audit_event_types.md
@@ -246,13 +246,13 @@ audit events to external destinations.
 | [`protected_branch_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/92074) | Triggered when a protected branch is created | **{check-circle}** Yes | **{check-circle}** Yes | `source_code_management` | GitLab [15.2](https://gitlab.com/gitlab-org/gitlab/-/issues/363091) |
 | [`protected_branch_removed`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/92074) | Triggered when a protected branch is removed | **{check-circle}** Yes | **{check-circle}** Yes | `source_code_management` | GitLab [15.2](https://gitlab.com/gitlab-org/gitlab/-/issues/363091) |
 | [`protected_branch_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/107530) | Event triggered on the setting for protected branches is update | **{check-circle}** Yes | **{check-circle}** Yes | `source_code_management` | GitLab [15.8](https://gitlab.com/gitlab-org/gitlab/-/issues/369318) |
-| [`protected_environment_approval_rule_added`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484) | This event is triggered when an approval rule is added to a protected environment. | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
-| [`protected_environment_approval_rule_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484) | This event is triggered when an approval rule is removed from a protected environment. | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
-| [`protected_environment_approval_rule_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484) | This event is triggered when an approval rule of a protected environment is updated. | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
-| [`protected_environment_deploy_access_level_added`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484) | This event is triggered when a deploy access level is added to a protected environment. | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
-| [`protected_environment_deploy_access_level_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484) | This event is triggered when a deploy access level is removed from a protected environment. | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
-| [`protected_environment_deploy_access_level_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484) | This event is triggered when a deploy access level of a protected environment is updated | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
-| [`protected_environment_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484) | This event is triggered when a protected environment is updated. | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.4](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
+| [`protected_environment_approval_rule_added`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484) | This event is triggered when an approval rule is added to a protected environment. | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.5](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
+| [`protected_environment_approval_rule_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484) | This event is triggered when an approval rule is removed from a protected environment. | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.5](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
+| [`protected_environment_approval_rule_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484) | This event is triggered when an approval rule of a protected environment is updated. | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.5](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
+| [`protected_environment_deploy_access_level_added`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484) | This event is triggered when a deploy access level is added to a protected environment. | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.5](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
+| [`protected_environment_deploy_access_level_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484) | This event is triggered when a deploy access level is removed from a protected environment. | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.5](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
+| [`protected_environment_deploy_access_level_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484) | This event is triggered when a deploy access level of a protected environment is updated | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.5](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
+| [`protected_environment_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484) | This event is triggered when a protected environment is updated. | **{check-circle}** Yes | **{check-circle}** Yes | `environment_management` | GitLab [16.5](https://gitlab.com/gitlab-org/gitlab/-/issues/415603) |
 | [`registration_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/123080) | Event triggered when a user registers for instance access | **{check-circle}** Yes | **{check-circle}** Yes | `compliance_management` | GitLab [16.3](https://gitlab.com/gitlab-org/gitlab/-/issues/374107) |
 | [`release_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/111080) | Event triggered when a release is created | **{check-circle}** Yes | **{check-circle}** Yes | `compliance_management` | GitLab [15.9](https://gitlab.com/gitlab-org/gitlab/-/issues/374111) |
 | [`release_deleted_audit_event`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/111080) | Event triggered when a release is deleted | **{check-circle}** Yes | **{check-circle}** Yes | `compliance_management` | GitLab [15.9](https://gitlab.com/gitlab-org/gitlab/-/issues/374111) |
diff --git a/ee/config/audit_events/types/protected_environment_approval_rule_added.yml b/ee/config/audit_events/types/protected_environment_approval_rule_added.yml
index ec842b492ac167..2ee3e9d79b63e1 100644
--- a/ee/config/audit_events/types/protected_environment_approval_rule_added.yml
+++ b/ee/config/audit_events/types/protected_environment_approval_rule_added.yml
@@ -4,6 +4,6 @@ description: This event is triggered when an approval rule is added to a protect
 introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/415603
 introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484
 feature_category: environment_management
-milestone: '16.4'
+milestone: '16.5'
 saved_to_database: true
 streamed: true
diff --git a/ee/config/audit_events/types/protected_environment_approval_rule_deleted.yml b/ee/config/audit_events/types/protected_environment_approval_rule_deleted.yml
index db8a02f2171b36..de43153ffb6c22 100644
--- a/ee/config/audit_events/types/protected_environment_approval_rule_deleted.yml
+++ b/ee/config/audit_events/types/protected_environment_approval_rule_deleted.yml
@@ -4,6 +4,6 @@ description: This event is triggered when an approval rule is removed from a pro
 introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/415603
 introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484
 feature_category: environment_management
-milestone: '16.4'
+milestone: '16.5'
 saved_to_database: true
 streamed: true
diff --git a/ee/config/audit_events/types/protected_environment_approval_rule_updated.yml b/ee/config/audit_events/types/protected_environment_approval_rule_updated.yml
index dc89f5f682b41d..5573b98b9a1d19 100644
--- a/ee/config/audit_events/types/protected_environment_approval_rule_updated.yml
+++ b/ee/config/audit_events/types/protected_environment_approval_rule_updated.yml
@@ -4,6 +4,6 @@ description: This event is triggered when an approval rule of a protected enviro
 introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/415603
 introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484
 feature_category: environment_management
-milestone: '16.4'
+milestone: '16.5'
 saved_to_database: true
 streamed: true
diff --git a/ee/config/audit_events/types/protected_environment_deploy_access_level_added.yml b/ee/config/audit_events/types/protected_environment_deploy_access_level_added.yml
index 54482841045832..c6c5290bf9d24d 100644
--- a/ee/config/audit_events/types/protected_environment_deploy_access_level_added.yml
+++ b/ee/config/audit_events/types/protected_environment_deploy_access_level_added.yml
@@ -4,6 +4,6 @@ description: This event is triggered when a deploy access level is added to a pr
 introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/415603
 introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484
 feature_category: environment_management
-milestone: '16.4'
+milestone: '16.5'
 saved_to_database: true
 streamed: true
diff --git a/ee/config/audit_events/types/protected_environment_deploy_access_level_deleted.yml b/ee/config/audit_events/types/protected_environment_deploy_access_level_deleted.yml
index 6d57ef86d31d39..0581e9c2db29f3 100644
--- a/ee/config/audit_events/types/protected_environment_deploy_access_level_deleted.yml
+++ b/ee/config/audit_events/types/protected_environment_deploy_access_level_deleted.yml
@@ -4,6 +4,6 @@ description: This event is triggered when a deploy access level is removed from
 introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/415603
 introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484
 feature_category: environment_management
-milestone: '16.4'
+milestone: '16.5'
 saved_to_database: true
 streamed: true
diff --git a/ee/config/audit_events/types/protected_environment_deploy_access_level_updated.yml b/ee/config/audit_events/types/protected_environment_deploy_access_level_updated.yml
index f75edfe9e5bed5..79bc38f6abd173 100644
--- a/ee/config/audit_events/types/protected_environment_deploy_access_level_updated.yml
+++ b/ee/config/audit_events/types/protected_environment_deploy_access_level_updated.yml
@@ -4,6 +4,6 @@ description: This event is triggered when a deploy access level of a protected e
 introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/415603
 introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484
 feature_category: environment_management
-milestone: '16.4'
+milestone: '16.5'
 saved_to_database: true
 streamed: true
diff --git a/ee/config/audit_events/types/protected_environment_updated.yml b/ee/config/audit_events/types/protected_environment_updated.yml
index 045efeddb454f6..fde6fc1cd6285e 100644
--- a/ee/config/audit_events/types/protected_environment_updated.yml
+++ b/ee/config/audit_events/types/protected_environment_updated.yml
@@ -4,6 +4,6 @@ description: This event is triggered when a protected environment is updated.
 introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/415603
 introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131484
 feature_category: environment_management
-milestone: '16.4'
+milestone: '16.5'
 saved_to_database: true
 streamed: true
-- 
GitLab


From 6865210a2dae0414a3f3ef8f5ab8f2c174dc00b1 Mon Sep 17 00:00:00 2001
From: Pam Artiaga <partiaga@gitlab.com>
Date: Wed, 20 Sep 2023 14:21:47 +1000
Subject: [PATCH 8/9] Only query records for deletion

---
 .../protected_environments/update_service.rb  |  42 +++++--
 ...ment_authorization_rule_changes_auditor.rb | 115 ++++++------------
 ...authorization_rule_changes_auditor_spec.rb |   7 +-
 3 files changed, 69 insertions(+), 95 deletions(-)

diff --git a/ee/app/services/protected_environments/update_service.rb b/ee/app/services/protected_environments/update_service.rb
index 21e0307ffabbc5..c0421c98755670 100644
--- a/ee/app/services/protected_environments/update_service.rb
+++ b/ee/app/services/protected_environments/update_service.rb
@@ -6,15 +6,19 @@ class UpdateService < ProtectedEnvironments::BaseService
     AUDITABLE_ATTRIBUTES = [:required_approval_count].freeze
 
     def execute(protected_environment)
-      deploy_access_levels_before_save = protected_environment.deploy_access_levels.to_a
-      approval_rules_before_save = protected_environment.approval_rules.to_a
+      # before updating the `protected_environment`, we need to query upfront
+      # the `deploy_access_levels` and `approval_rules` records that are marked for destruction
+      # since `protected_environment.deploy_access_levels` and `protected_environment.approval_rules`
+      # will no longer include these records after update
+      deploy_access_levels_for_destruction = find_deploy_access_levels_for_destruction(protected_environment)
+      approval_rules_for_destruction = find_approval_rules_for_destruction(protected_environment)
 
       protected_environment.update(sanitized_params).tap do |is_updated|
         if is_updated
           log_audit_events(
             protected_environment: protected_environment,
-            previous_deploy_access_levels: deploy_access_levels_before_save,
-            previous_approval_rules: approval_rules_before_save
+            deleted_deploy_access_levels: deploy_access_levels_for_destruction,
+            deleted_approval_rules: approval_rules_for_destruction
           )
         end
       end
@@ -22,17 +26,29 @@ def execute(protected_environment)
 
     private
 
-    def successful?(result)
-      result == true
+    # rubocop: disable CodeReuse/ActiveRecord
+    def find_deploy_access_levels_for_destruction(protected_environment)
+      return [] unless sanitized_params[:deploy_access_levels_attributes].present?
+
+      ids = sanitized_params[:deploy_access_levels_attributes].select { |dal| dal[:_destroy] }.pluck(:id)
+      protected_environment.deploy_access_levels.where(id: ids).to_a
+    end
+
+    def find_approval_rules_for_destruction(protected_environment)
+      return [] unless sanitized_params[:approval_rules_attributes].present?
+
+      ids = sanitized_params[:approval_rules_attributes].select { |ar| ar[:_destroy] }.pluck(:id)
+      protected_environment.approval_rules.where(id: ids).to_a
     end
+    # rubocop: enable CodeReuse/ActiveRecord
 
-    def log_audit_events(protected_environment:, previous_deploy_access_levels:, previous_approval_rules:)
+    def log_audit_events(protected_environment:, deleted_deploy_access_levels:, deleted_approval_rules:)
       audit_changed_attributes(protected_environment)
 
       audit_authorization_rule_changes(
         protected_environment: protected_environment,
-        previous_deploy_access_levels: previous_deploy_access_levels,
-        previous_approval_rules: previous_approval_rules
+        deleted_deploy_access_levels: deleted_deploy_access_levels,
+        deleted_approval_rules: deleted_approval_rules
       )
     end
 
@@ -49,15 +65,15 @@ def audit_changed_attributes(protected_environment)
 
     def audit_authorization_rule_changes(
       protected_environment:,
-      previous_deploy_access_levels:,
-      previous_approval_rules:
+      deleted_deploy_access_levels:,
+      deleted_approval_rules:
     )
       ::Audit::ProtectedEnvironmentAuthorizationRuleChangesAuditor.new(
         author: current_user,
         scope: container,
         protected_environment: protected_environment,
-        previous_deploy_access_levels: previous_deploy_access_levels,
-        previous_approval_rules: previous_approval_rules
+        deleted_deploy_access_levels: deleted_deploy_access_levels,
+        deleted_approval_rules: deleted_approval_rules
       ).audit
     end
   end
diff --git a/ee/lib/audit/protected_environment_authorization_rule_changes_auditor.rb b/ee/lib/audit/protected_environment_authorization_rule_changes_auditor.rb
index 68a8e90a076877..5eb0ef29297cae 100644
--- a/ee/lib/audit/protected_environment_authorization_rule_changes_auditor.rb
+++ b/ee/lib/audit/protected_environment_authorization_rule_changes_auditor.rb
@@ -4,12 +4,12 @@ module Audit
   class ProtectedEnvironmentAuthorizationRuleChangesAuditor
     AUTHORIZABLE_ATTRIBUTES = [:access_level, :user_id, :group_id].freeze
 
-    def initialize(author:, scope:, protected_environment:, previous_deploy_access_levels:, previous_approval_rules:)
+    def initialize(author:, scope:, protected_environment:, deleted_deploy_access_levels:, deleted_approval_rules:)
       @author = author
       @scope = scope
       @protected_environment = protected_environment
-      @previous_deploy_access_levels = previous_deploy_access_levels
-      @previous_approval_rules = previous_approval_rules
+      @deleted_deploy_access_levels = deleted_deploy_access_levels
+      @deleted_approval_rules = deleted_approval_rules
     end
 
     def audit
@@ -19,20 +19,14 @@ def audit
 
     private
 
-    attr_reader :author, :scope, :protected_environment, :previous_deploy_access_levels, :previous_approval_rules
+    attr_reader :author, :scope, :protected_environment, :deleted_deploy_access_levels, :deleted_approval_rules
 
     def audit_changed_deploy_access_levels
       audit_deleted_deploy_access_levels
-      audit_updated_deploy_access_levels
-      audit_created_deploy_access_levels
+      audit_created_and_updated_deploy_access_levels
     end
 
     def audit_deleted_deploy_access_levels
-      current_deploy_access_levels_ids = protected_environment.deploy_access_levels.map(&:id)
-      deleted_deploy_access_levels = previous_deploy_access_levels.select do |dal|
-        current_deploy_access_levels_ids.exclude?(dal.id)
-      end
-
       deleted_deploy_access_levels.each do |deploy_access_level|
         audit_event(
           event_name: 'protected_environment_deploy_access_level_deleted',
@@ -41,43 +35,28 @@ def audit_deleted_deploy_access_levels
       end
     end
 
-    def audit_created_deploy_access_levels
-      previous_deploy_access_levels_ids = previous_deploy_access_levels.map(&:id)
-      new_deploy_access_levels = protected_environment.deploy_access_levels.select do |dal|
-        previous_deploy_access_levels_ids.exclude?(dal.id)
-      end
-
-      new_deploy_access_levels.each do |deploy_access_level|
-        audit_event(
-          event_name: 'protected_environment_deploy_access_level_added',
-          message: "Added deploy access level #{deploy_access_level.humanize}."
-        )
-      end
-    end
-
-    def audit_updated_deploy_access_levels
+    def audit_created_and_updated_deploy_access_levels
       protected_environment.deploy_access_levels.each do |deploy_access_level|
-        next unless deploy_access_level_has_changes?(deploy_access_level)
-
-        audit_event(
-          event_name: 'protected_environment_deploy_access_level_updated',
-          message: updated_deploy_access_level_message(deploy_access_level)
-        )
+        if deploy_access_level.previously_new_record?
+          audit_event(
+            event_name: 'protected_environment_deploy_access_level_added',
+            message: "Added deploy access level #{deploy_access_level.humanize}."
+          )
+        elsif deploy_access_level_has_changes?(deploy_access_level)
+          audit_event(
+            event_name: 'protected_environment_deploy_access_level_updated',
+            message: updated_deploy_access_level_message(deploy_access_level)
+          )
+        end
       end
     end
 
     def audit_changed_approval_rules
       audit_deleted_approval_rules
-      audit_updated_approval_rules
-      audit_created_approval_rules
+      audit_created_and_updated_approval_rules
     end
 
     def audit_deleted_approval_rules
-      current_approval_rules_ids = protected_environment.approval_rules.map(&:id)
-      deleted_approval_rules = previous_approval_rules.select do |dal|
-        current_approval_rules_ids.exclude?(dal.id)
-      end
-
       deleted_approval_rules.each do |approval_rule|
         audit_event(
           event_name: 'protected_environment_approval_rule_deleted',
@@ -86,35 +65,24 @@ def audit_deleted_approval_rules
       end
     end
 
-    def audit_created_approval_rules
-      previous_approval_rules_ids = previous_approval_rules.map(&:id)
-      new_approval_rules = protected_environment.approval_rules.select do |dal|
-        previous_approval_rules_ids.exclude?(dal.id)
-      end
-
-      new_approval_rules.each do |approval_rule|
-        audit_event(
-          event_name: 'protected_environment_approval_rule_added',
-          message: "Added approval rule for #{approval_rule.humanize} " \
-                   "with required approval count #{approval_rule.required_approvals}."
-        )
-      end
-    end
-
-    def audit_updated_approval_rules
+    def audit_created_and_updated_approval_rules
       protected_environment.approval_rules.each do |approval_rule|
-        next unless approval_rule_has_changes?(approval_rule)
-
-        audit_event(
-          event_name: 'protected_environment_approval_rule_updated',
-          message: updated_approval_rule_message(approval_rule)
-        )
+        if approval_rule.previously_new_record?
+          audit_event(
+            event_name: 'protected_environment_approval_rule_added',
+            message: "Added approval rule for #{approval_rule.humanize} " \
+                     "with required approval count #{approval_rule.required_approvals}."
+          )
+        elsif approval_rule_has_changes?(approval_rule)
+          audit_event(
+            event_name: 'protected_environment_approval_rule_updated',
+            message: updated_approval_rule_message(approval_rule)
+          )
+        end
       end
     end
 
     def deploy_access_level_has_changes?(deploy_access_level)
-      return false if deploy_access_level.previous_changes.keys.any? { |key| key.to_sym == :id }
-
       deploy_access_level.previous_changes.keys.any? { |key| AUTHORIZABLE_ATTRIBUTES.include?(key.to_sym) }
     end
 
@@ -125,8 +93,6 @@ def updated_deploy_access_level_message(deploy_access_level)
     end
 
     def approval_rule_has_changes?(approval_rule)
-      return false if approval_rule.previous_changes.keys.any? { |key| key.to_sym == :id }
-
       auditable_attributes = AUTHORIZABLE_ATTRIBUTES + [:required_approvals]
       approval_rule.previous_changes.keys.any? { |key| auditable_attributes.include?(key.to_sym) }
     end
@@ -182,16 +148,6 @@ def changed_access_level_details(authorizable_object)
       }
     end
 
-    def build_audit_context(event_name:, message:)
-      {
-        name: event_name,
-        author: author,
-        scope: scope,
-        target: protected_environment,
-        message: message
-      }
-    end
-
     def humanize(type:, value:)
       case type.to_sym
       when :access_level
@@ -204,8 +160,13 @@ def humanize(type:, value:)
     end
 
     def audit_event(event_name:, message:)
-      audit_context = build_audit_context(event_name: event_name, message: message)
-      ::Gitlab::Audit::Auditor.audit(audit_context)
+      ::Gitlab::Audit::Auditor.audit(
+        name: event_name,
+        author: author,
+        scope: scope,
+        target: protected_environment,
+        message: message
+      )
     end
   end
 end
diff --git a/ee/spec/lib/audit/protected_environment_authorization_rule_changes_auditor_spec.rb b/ee/spec/lib/audit/protected_environment_authorization_rule_changes_auditor_spec.rb
index 6981ed5a739069..d77c2225746f7b 100644
--- a/ee/spec/lib/audit/protected_environment_authorization_rule_changes_auditor_spec.rb
+++ b/ee/spec/lib/audit/protected_environment_authorization_rule_changes_auditor_spec.rb
@@ -67,17 +67,14 @@
   end
 
   subject do
-    previous_deploy_access_levels = protected_environment.deploy_access_levels.to_a
-    previous_approval_rules = protected_environment.approval_rules.to_a
-
     protected_environment.update!(params)
 
     described_class.new(
       author: user,
       scope: project,
       protected_environment: protected_environment,
-      previous_deploy_access_levels: previous_deploy_access_levels,
-      previous_approval_rules: previous_approval_rules
+      deleted_deploy_access_levels: [deploy_access_for_delete],
+      deleted_approval_rules: [approval_rule_for_delete]
     ).audit
   end
 
-- 
GitLab


From a23fbae78d75911b27b47804e672697afac161e6 Mon Sep 17 00:00:00 2001
From: Pam Artiaga <partiaga@gitlab.com>
Date: Thu, 21 Sep 2023 12:00:26 +1000
Subject: [PATCH 9/9] Avoid CodeReuse/ActiveRecord rubocop violation

---
 .../services/protected_environments/update_service.rb  | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/ee/app/services/protected_environments/update_service.rb b/ee/app/services/protected_environments/update_service.rb
index c0421c98755670..ea010f97401aee 100644
--- a/ee/app/services/protected_environments/update_service.rb
+++ b/ee/app/services/protected_environments/update_service.rb
@@ -26,21 +26,19 @@ def execute(protected_environment)
 
     private
 
-    # rubocop: disable CodeReuse/ActiveRecord
     def find_deploy_access_levels_for_destruction(protected_environment)
       return [] unless sanitized_params[:deploy_access_levels_attributes].present?
 
-      ids = sanitized_params[:deploy_access_levels_attributes].select { |dal| dal[:_destroy] }.pluck(:id)
-      protected_environment.deploy_access_levels.where(id: ids).to_a
+      ids = sanitized_params[:deploy_access_levels_attributes].filter_map { |dal| dal[:id] if dal[:_destroy] }
+      protected_environment.deploy_access_levels.id_in(ids).to_a
     end
 
     def find_approval_rules_for_destruction(protected_environment)
       return [] unless sanitized_params[:approval_rules_attributes].present?
 
-      ids = sanitized_params[:approval_rules_attributes].select { |ar| ar[:_destroy] }.pluck(:id)
-      protected_environment.approval_rules.where(id: ids).to_a
+      ids = sanitized_params[:approval_rules_attributes].filter_map { |ar| ar[:id] if ar[:_destroy] }
+      protected_environment.approval_rules.id_in(ids).to_a
     end
-    # rubocop: enable CodeReuse/ActiveRecord
 
     def log_audit_events(protected_environment:, deleted_deploy_access_levels:, deleted_approval_rules:)
       audit_changed_attributes(protected_environment)
-- 
GitLab