From 490388aa861ef9e1a1779f88653e2706739f6fcf Mon Sep 17 00:00:00 2001 From: huzaifaiftikhar1 Date: Wed, 3 Nov 2021 19:02:42 +0530 Subject: [PATCH 1/2] Add group SAML configuration changes to group audit events Changelog: added EE: true --- doc/administration/audit_events.md | 11 +++++++ .../groups/saml_group_links_controller.rb | 15 ++++++++- .../group_saml/saml_provider/base_service.rb | 31 +++++++++++++++++++ .../saml_provider/create_service.rb | 4 +++ .../saml_provider/update_service.rb | 3 ++ .../saml_group_links_controller_spec.rb | 25 ++++++++++++++- .../saml_provider/create_service_spec.rb | 2 ++ .../saml_provider/update_service_spec.rb | 1 + .../base_service_shared_examples.rb | 16 ++++++++++ 9 files changed, 106 insertions(+), 2 deletions(-) diff --git a/doc/administration/audit_events.md b/doc/administration/audit_events.md index 8fbf06343a59f8..ddc8e8cb270fb2 100644 --- a/doc/administration/audit_events.md +++ b/doc/administration/audit_events.md @@ -73,6 +73,17 @@ From there, you can see the following actions: - Group changed visibility. - User was added to group and with which [permissions](../user/permissions.md). - User sign-in via [Group SAML](../user/group/saml_sso/index.md). +- [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/8071) in GitLab 14.5, changes to the following + [group SAML](../user/group/saml_sso/index.md) configuration: + - Enabled status. + - Enforcing SSO-only authentication for web activity. + - Enforcing SSO-only authentication for Git and Dependency Proxy activity. + - Enforcing users to have dedicated group-managed accounts. + - Prohibiting outer forks. + - Identity provider SSO URL. + - Certificate fingerprint. + - Default membership role. + - SSO-SAML group sync configuration. - Permissions changes of a user assigned to a group. - Removed user from group. - Project repository imported into group. diff --git a/ee/app/controllers/groups/saml_group_links_controller.rb b/ee/app/controllers/groups/saml_group_links_controller.rb index 3e1bffcd24be89..5fcd98f1ac3af0 100644 --- a/ee/app/controllers/groups/saml_group_links_controller.rb +++ b/ee/app/controllers/groups/saml_group_links_controller.rb @@ -13,6 +13,7 @@ def create if group_link.save flash[:notice] = s_('GroupSAML|New SAML group link saved.') + create_audit_event('saml_group_links_created', group, "SAML group links created. Group Name - #{group_link.saml_group_name}, Access Level - #{group_link.access_level}") else flash[:alert] = alert(group_link.errors.full_messages.join(', ')) end @@ -21,7 +22,9 @@ def create end def destroy - group.saml_group_links.find(params[:id]).destroy + saml_group_link = group.saml_group_links.find(params[:id]) + saml_group_link.destroy + create_audit_event('saml_group_links_removed', group, "SAML group links removed. Group Name - #{saml_group_link.saml_group_name}") redirect_to group_saml_group_links_path(@group), status: :found, notice: s_('GroupSAML|SAML group link was successfully removed.') end @@ -39,5 +42,15 @@ def saml_group_link_params def alert(error_message) s_('GroupSAML|Could not create SAML group link: %{errors}.') % { errors: error_message } end + + def create_audit_event(name, group, message) + ::Gitlab::Audit::Auditor.audit( + name: name, + author: current_user, + scope: group, + target: group, + message: message + ) + end end end diff --git a/ee/app/services/group_saml/saml_provider/base_service.rb b/ee/app/services/group_saml/saml_provider/base_service.rb index a07da222dcf44c..241e0382c1b8dc 100644 --- a/ee/app/services/group_saml/saml_provider/base_service.rb +++ b/ee/app/services/group_saml/saml_provider/base_service.rb @@ -25,6 +25,20 @@ def execute require_linked_saml_to_enable_group_managed! end end + + if saml_provider.previous_changes.present? + ::Gitlab::Audit::Auditor.audit( + name: audit_name, + author: current_user, + scope: saml_provider.group, + target: saml_provider.group, + message: message + ) + end + end + + def audit_name + "group_saml_provider" end private @@ -40,6 +54,23 @@ def add_error!(message) raise ActiveRecord::Rollback end + + def message + audit_logs_allowlist = %w[enabled certificate_fingerprint sso_url enforced_sso enforced_group_managed_accounts prohibited_outer_forks default_membership_role git_check_enforced] + change_text = saml_provider + .previous_changes + .map do |k, v| + next unless audit_logs_allowlist.include?(k) + + if v[0].nil? + "#{k} changed to #{v[1]}. " + else + "#{k} changed from #{v[0]} to #{v[1]}. " + end + end.join + + "Group SAML SSO configuration changed: #{change_text}" + end end end end diff --git a/ee/app/services/group_saml/saml_provider/create_service.rb b/ee/app/services/group_saml/saml_provider/create_service.rb index c4ee7f66abbe8c..70df48a1f38d63 100644 --- a/ee/app/services/group_saml/saml_provider/create_service.rb +++ b/ee/app/services/group_saml/saml_provider/create_service.rb @@ -9,6 +9,10 @@ def initialize(current_user, group, params:) @group = group super(current_user, group.build_saml_provider, params: params) end + + def audit_name + "#{super}_create" + end end end end diff --git a/ee/app/services/group_saml/saml_provider/update_service.rb b/ee/app/services/group_saml/saml_provider/update_service.rb index 3fe6e2e98fec09..87576650a03606 100644 --- a/ee/app/services/group_saml/saml_provider/update_service.rb +++ b/ee/app/services/group_saml/saml_provider/update_service.rb @@ -5,6 +5,9 @@ module GroupSaml module SamlProvider class UpdateService < BaseService + def audit_name + "#{super}_update" + end end end end diff --git a/ee/spec/controllers/groups/saml_group_links_controller_spec.rb b/ee/spec/controllers/groups/saml_group_links_controller_spec.rb index d82a352fa955f2..c8ec1d6a6abdfe 100644 --- a/ee/spec/controllers/groups/saml_group_links_controller_spec.rb +++ b/ee/spec/controllers/groups/saml_group_links_controller_spec.rb @@ -60,13 +60,25 @@ let_it_be(:saml_provider) { create(:saml_provider, group: group, enabled: true) } context 'with valid parameters' do - let_it_be(:params) { route_params.merge(saml_group_link: { access_level: 'Reporter', saml_group_name: generate(:saml_group_name) }) } + let_it_be(:saml_group_name) { generate(:saml_group_name) } + let_it_be(:params) { route_params.merge(saml_group_link: { access_level: 'Reporter', saml_group_name: saml_group_name }) } it 'responds with success' do + expect(::Gitlab::Audit::Auditor) + .to receive(:audit).with( + hash_including( + { name: "saml_group_links_created", + author: user, + scope: group, + target: group, + message: "SAML group links created. Group Name - #{saml_group_name}, Access Level - Reporter" }) + ).and_call_original + call_action expect(response).to have_gitlab_http_status(:found) expect(flash[:notice]).to include('New SAML group link saved.') + expect(AuditEvent.last.details[:custom_message]).to eq("SAML group links created. Group Name - #{saml_group_name}, Access Level - Reporter") end it 'creates the group link' do @@ -102,10 +114,21 @@ let_it_be(:params) { route_params } it 'responds with success' do + expect(::Gitlab::Audit::Auditor) + .to receive(:audit).with( + hash_including( + { name: "saml_group_links_removed", + author: user, + scope: group, + target: group, + message: "SAML group links removed. Group Name - #{group_link.saml_group_name}" }) + ).and_call_original + call_action expect(response).to have_gitlab_http_status(:found) expect(flash[:notice]).to include('SAML group link was successfully removed.') + expect(AuditEvent.last.details[:custom_message]).to eq("SAML group links removed. Group Name - #{group_link.saml_group_name}") end it 'removes the group link' do diff --git a/ee/spec/services/group_saml/saml_provider/create_service_spec.rb b/ee/spec/services/group_saml/saml_provider/create_service_spec.rb index 0180c20bc14147..ec7623b772808e 100644 --- a/ee/spec/services/group_saml/saml_provider/create_service_spec.rb +++ b/ee/spec/services/group_saml/saml_provider/create_service_spec.rb @@ -8,5 +8,7 @@ let(:group) { create :group } + let(:audit_event_name) { 'group_saml_provider_create' } + include_examples 'base SamlProvider service' end diff --git a/ee/spec/services/group_saml/saml_provider/update_service_spec.rb b/ee/spec/services/group_saml/saml_provider/update_service_spec.rb index 1a0c524dac86b3..aaf6f1d3365789 100644 --- a/ee/spec/services/group_saml/saml_provider/update_service_spec.rb +++ b/ee/spec/services/group_saml/saml_provider/update_service_spec.rb @@ -11,6 +11,7 @@ end let(:group) { saml_provider.group } + let(:audit_event_name) { 'group_saml_provider_update' } include_examples 'base SamlProvider service' include_examples 'SamlProvider service toggles Group Managed Accounts' diff --git a/ee/spec/support/shared_examples/services/group_saml/saml_provider/base_service_shared_examples.rb b/ee/spec/support/shared_examples/services/group_saml/saml_provider/base_service_shared_examples.rb index 244c65dc9f6d44..6580b6d99e7ebb 100644 --- a/ee/spec/support/shared_examples/services/group_saml/saml_provider/base_service_shared_examples.rb +++ b/ee/spec/support/shared_examples/services/group_saml/saml_provider/base_service_shared_examples.rb @@ -17,6 +17,15 @@ end it 'updates SAML provider with given params' do + expect(::Gitlab::Audit::Auditor) + .to receive(:audit).with( + hash_including( + { name: audit_event_name, + author: current_user, + scope: group, + target: group }) + ).and_call_original + expect do service.execute group.reload @@ -24,6 +33,13 @@ .and change { group.saml_provider&.certificate_fingerprint }.to(fingerprint) .and change { group.saml_provider&.enabled? }.to(true) .and change { group.saml_provider&.enforced_sso? }.to(true) + .and change { AuditEvent.count }.by(1) + + expect(AuditEvent.last.details[:custom_message]) + .to match(%r{enabled changed([\w\s]*)to true}) + .and match(%r{enforced_sso changed([\w\s]*)to true}) + .and match(%r{https:\/\/test}) + .and match(%r{#{fingerprint}}) end end -- GitLab From d7ad2b5b8526f90cd583d2ad25d9ef47d005ca27 Mon Sep 17 00:00:00 2001 From: Max Woolf Date: Mon, 15 Nov 2021 13:29:45 +0000 Subject: [PATCH 2/2] Create one audit event per SAML config change --- .../group_saml/saml_provider/base_service.rb | 35 ++++++++++--------- .../base_service_shared_examples.rb | 10 ++---- 2 files changed, 20 insertions(+), 25 deletions(-) diff --git a/ee/app/services/group_saml/saml_provider/base_service.rb b/ee/app/services/group_saml/saml_provider/base_service.rb index 241e0382c1b8dc..03644aded97bf8 100644 --- a/ee/app/services/group_saml/saml_provider/base_service.rb +++ b/ee/app/services/group_saml/saml_provider/base_service.rb @@ -9,6 +9,10 @@ class BaseService delegate :group, to: :saml_provider + AUDIT_LOG_ALLOWLIST = %w[ + enabled certificate_fingerprint sso_url enforced_sso enforced_group_managed_accounts prohibited_outer_forks default_membership_role git_check_enforced + ].freeze + def initialize(current_user, saml_provider, params:) @saml_provider = saml_provider @current_user = current_user @@ -26,14 +30,18 @@ def execute end end - if saml_provider.previous_changes.present? - ::Gitlab::Audit::Auditor.audit( + saml_provider.previous_changes.each do |attribute, changes| + next unless AUDIT_LOG_ALLOWLIST.include?(attribute) + + audit_context = { name: audit_name, author: current_user, scope: saml_provider.group, target: saml_provider.group, - message: message - ) + message: message(attribute, changes) + } + + ::Gitlab::Audit::Auditor.audit(audit_context) end end @@ -55,19 +63,12 @@ def add_error!(message) raise ActiveRecord::Rollback end - def message - audit_logs_allowlist = %w[enabled certificate_fingerprint sso_url enforced_sso enforced_group_managed_accounts prohibited_outer_forks default_membership_role git_check_enforced] - change_text = saml_provider - .previous_changes - .map do |k, v| - next unless audit_logs_allowlist.include?(k) - - if v[0].nil? - "#{k} changed to #{v[1]}. " - else - "#{k} changed from #{v[0]} to #{v[1]}. " - end - end.join + def message(attribute, changes) + change_text = if changes[0].nil? + "#{attribute} changed to #{changes[1]}. " + else + "#{attribute} changed from #{changes[0]} to #{changes[1]}. " + end "Group SAML SSO configuration changed: #{change_text}" end diff --git a/ee/spec/support/shared_examples/services/group_saml/saml_provider/base_service_shared_examples.rb b/ee/spec/support/shared_examples/services/group_saml/saml_provider/base_service_shared_examples.rb index 6580b6d99e7ebb..169c0b51e64e30 100644 --- a/ee/spec/support/shared_examples/services/group_saml/saml_provider/base_service_shared_examples.rb +++ b/ee/spec/support/shared_examples/services/group_saml/saml_provider/base_service_shared_examples.rb @@ -24,7 +24,7 @@ author: current_user, scope: group, target: group }) - ).and_call_original + ).exactly(4).times.and_call_original expect do service.execute @@ -33,13 +33,7 @@ .and change { group.saml_provider&.certificate_fingerprint }.to(fingerprint) .and change { group.saml_provider&.enabled? }.to(true) .and change { group.saml_provider&.enforced_sso? }.to(true) - .and change { AuditEvent.count }.by(1) - - expect(AuditEvent.last.details[:custom_message]) - .to match(%r{enabled changed([\w\s]*)to true}) - .and match(%r{enforced_sso changed([\w\s]*)to true}) - .and match(%r{https:\/\/test}) - .and match(%r{#{fingerprint}}) + .and change { AuditEvent.count }.by(4) end end -- GitLab