diff --git a/doc/administration/audit_events.md b/doc/administration/audit_events.md index 98fe05e2805a90c129d070b13679ec472294ff0e..214a1f3a009654d423339d11192caacad9c358da 100644 --- a/doc/administration/audit_events.md +++ b/doc/administration/audit_events.md @@ -114,6 +114,7 @@ From there, you can see the following actions: - Instance administrator started or stopped impersonation of a group member. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/300961) in GitLab 14.8. - Group deploy token was successfully created, revoked, or deleted. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/353452) in GitLab 14.9. - Failed attempt to create a group deploy token. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/353452) in GitLab 14.9. +- [IP restrictions](../user/group/index.md#restrict-group-access-by-ip-address) changed. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/358986) in GitLab 15.0 Group events can also be accessed via the [Group Audit Events API](../api/audit_events.md#group-audit-events) diff --git a/ee/app/services/ee/groups/update_service.rb b/ee/app/services/ee/groups/update_service.rb index 78e20cad4f38128bac0d31570ac7d06ec73ef1e3..ef90b3d10837d37fd64f0ad13f138487db74a1ee 100644 --- a/ee/app/services/ee/groups/update_service.rb +++ b/ee/app/services/ee/groups/update_service.rb @@ -19,7 +19,7 @@ def execute return false if group.errors.present? - super.tap { |success| log_audit_event if success } + super.tap { |success| log_audit_events if success } end private @@ -103,7 +103,10 @@ def handle_ip_restriction_update return if comma_separated_ranges.nil? - IpRestrictions::UpdateService.new(group, comma_separated_ranges).execute + # rubocop:disable Gitlab/ModuleWithInstanceVariables + @ip_restriction_update_service = IpRestrictions::UpdateService.new(current_user, group, comma_separated_ranges) + @ip_restriction_update_service.execute + # rubocop:enable Gitlab/ModuleWithInstanceVariables end def handle_allowed_email_domains_update @@ -119,7 +122,9 @@ def allowed_settings_params @allowed_settings_params ||= super + EE_SETTINGS_PARAMS end - def log_audit_event + def log_audit_events + @ip_restriction_update_service&.log_audit_event # rubocop:disable Gitlab/ModuleWithInstanceVariables + EE::Audit::GroupChangesAuditor.new(current_user, group).execute end end diff --git a/ee/app/services/ee/ip_restrictions/update_service.rb b/ee/app/services/ee/ip_restrictions/update_service.rb index 7d02cd707111a6f96c6d4a640f8cd3f38adc82b2..e24784d3cac71b19658830e61b633b3bb372d916 100644 --- a/ee/app/services/ee/ip_restrictions/update_service.rb +++ b/ee/app/services/ee/ip_restrictions/update_service.rb @@ -10,19 +10,41 @@ class UpdateService # marks the records that exist for the group right now, but are not in `comma_separated_ranges` for deletion. # builds new ip_restriction records that do not exist for the group right now, but exists in `comma_separated_ranges` - def initialize(group, comma_separated_ranges) + def initialize(current_user, group, comma_separated_ranges) + @current_user = current_user @group = group @comma_separated_ranges = comma_separated_ranges end def execute + @old_ranges = existing_ranges + mark_deleted_ranges_for_destruction build_new_ip_restriction_records + + @new_ranges = existing_ranges + end + + # This method is public because this service doesn't save anything to DB. + # Save happens when parent group is saved in EE:Groups::UpdateService + def log_audit_event + raise "Nothing to log, the service must be executed first." unless @old_ranges && @new_ranges + + return unless group.licensed_feature_available?(:extended_audit_events) + return if @old_ranges.sort == @new_ranges.sort + + ::Gitlab::Audit::Auditor.audit( + name: 'ip_restrictions_changed', + message: "Group IP restrictions updated from '#{@old_ranges.join(',')}' to '#{@new_ranges.join(',')}'", + target: group, + scope: group, + author: current_user + ) end private - attr_reader :group, :comma_separated_ranges + attr_reader :group, :current_user, :comma_separated_ranges def mark_deleted_ranges_for_destruction return unless ranges_to_be_deleted.present? @@ -55,7 +77,7 @@ def ranges_to_be_created end def existing_ranges - group.ip_restrictions.map(&:range) + group.ip_restrictions.reject(&:marked_for_destruction?).map(&:range) end def current_ranges diff --git a/ee/spec/services/ee/ip_restrictions/update_service_spec.rb b/ee/spec/services/ee/ip_restrictions/update_service_spec.rb index d880bad90fa77593a3a8ecd35434dd95265151fb..05320e69e0a6a42a4c45db5517b5b40730efc159 100644 --- a/ee/spec/services/ee/ip_restrictions/update_service_spec.rb +++ b/ee/spec/services/ee/ip_restrictions/update_service_spec.rb @@ -3,15 +3,17 @@ require 'spec_helper' RSpec.describe EE::IpRestrictions::UpdateService do - let(:group) { create(:group) } + subject(:service) { described_class.new(current_user, group, comma_separated_ranges) } - subject { described_class.new(group, comma_separated_ranges).execute } + let(:group) { create(:group) } + let(:current_user) { build(:user) } + let(:comma_separated_ranges) { '192.168.0.0/24,10.0.0.0/8' } describe '#execute' do + subject { service.execute } + context 'for a group that has no ip restriction' do context 'with valid IP subnets' do - let(:comma_separated_ranges) { '192.168.0.0/24,10.0.0.0/8' } - it 'builds new ip_restriction records' do subject @@ -109,4 +111,63 @@ end end end + + describe '#log_audit_event' do + before do + stub_licensed_features(extended_audit_events: true) + end + + context 'when new ranges are different from old ranges' do + it "logs ip_restrictions_changed event" do + subject.execute + + expect(Gitlab::Audit::Auditor).to receive(:audit).with( + name: 'ip_restrictions_changed', + message: "Group IP restrictions updated from '' to '#{comma_separated_ranges}'", + target: group, + scope: group, + author: current_user + ).and_call_original + + subject.log_audit_event + end + + context "when license doesn't allow auditing" do + before do + stub_licensed_features(extended_audit_events: false) + end + + it "doesn't log any events" do + subject.execute + + expect(Gitlab::Audit::Auditor).not_to receive(:audit) + + subject.log_audit_event + end + end + end + + context 'when new ranges are the same as old ranges' do + before do + comma_separated_ranges.split(',').reverse_each do |range| + create(:ip_restriction, group: group, range: range) + end + end + + it "doesn't log any events" do + subject.execute + + expect(Gitlab::Audit::Auditor).not_to receive(:audit) + + subject.log_audit_event + end + end + + context 'when log is called without prior execute' do + it 'raises an error' do + expect { subject.log_audit_event } + .to raise_error("Nothing to log, the service must be executed first.") + end + end + end end diff --git a/ee/spec/services/groups/update_service_spec.rb b/ee/spec/services/groups/update_service_spec.rb index 4c6e279ee56883d392d65d2b02e808f6849acda3..232d3047cc010eedd296e1a62a95ab53aa124751 100644 --- a/ee/spec/services/groups/update_service_spec.rb +++ b/ee/spec/services/groups/update_service_spec.rb @@ -21,11 +21,11 @@ } end - describe '#visibility' do - before do - group.add_owner(user) - end + before do + group.add_owner(user) + end + describe '#visibility' do include_examples 'audit event logging' do let(:operation) do update_group(group, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE) @@ -46,6 +46,32 @@ end end end + + describe 'ip restrictions' do + context 'when IP restrictions were changed' do + before do + group.add_owner(user) + end + + include_examples 'audit event logging' do + let(:operation) do + update_group(group, user, ip_restriction_ranges: '192.168.0.0/24,10.0.0.0/8' ) + end + + let(:fail_condition!) do + allow(group).to receive(:save).and_return(false) + end + + let(:attributes) do + audit_event_params.tap do |param| + param[:details].merge!( + custom_message: "Group IP restrictions updated from '' to '192.168.0.0/24,10.0.0.0/8'" + ) + end + end + end + end + end end describe 'changing file_template_project_id' do