diff --git a/app/controllers/profiles/gpg_keys_controller.rb b/app/controllers/profiles/gpg_keys_controller.rb index 7f04927f517d27b678af2e98dc304e2ba259c642..9e16d195b00ed2f6002401b2ae9f6a28f8692ffc 100644 --- a/app/controllers/profiles/gpg_keys_controller.rb +++ b/app/controllers/profiles/gpg_keys_controller.rb @@ -22,7 +22,7 @@ def create end def destroy - @gpg_key.destroy + GpgKeys::DestroyService.new(current_user).execute(@gpg_key) respond_to do |format| format.html { redirect_to profile_gpg_keys_url, status: :found } diff --git a/app/services/gpg_keys/create_service.rb b/app/services/gpg_keys/create_service.rb index e41444b2a82c55c79a3dd0dbe720bae1044d80cf..ab8b12732d7755aca45f05887a344cf84f8bb4ae 100644 --- a/app/services/gpg_keys/create_service.rb +++ b/app/services/gpg_keys/create_service.rb @@ -3,9 +3,17 @@ module GpgKeys class CreateService < Keys::BaseService def execute - key = user.gpg_keys.create(params) + key = create(params) notification_service.new_gpg_key(key) if key.persisted? key end + + private + + def create(params) + user.gpg_keys.create(params) + end end end + +GpgKeys::CreateService.prepend_mod diff --git a/app/services/gpg_keys/destroy_service.rb b/app/services/gpg_keys/destroy_service.rb index cecbfe266112b0f660f4bfdc871ad91bbe1385d6..2e82509897e2357b0b57f0c633fd5a0880320ac1 100644 --- a/app/services/gpg_keys/destroy_service.rb +++ b/app/services/gpg_keys/destroy_service.rb @@ -7,3 +7,5 @@ def execute(key) end end end + +GpgKeys::DestroyService.prepend_mod diff --git a/doc/administration/audit_events.md b/doc/administration/audit_events.md index ae921d4822a44fc1d143d2befbcf930966c5bcaa..7a871caf658b4d5fd1bb08805c2ea7c4c24f6a70 100644 --- a/doc/administration/audit_events.md +++ b/doc/administration/audit_events.md @@ -166,6 +166,7 @@ The following user actions are recorded: - A failed attempt to create or revoke a user's personal access token ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/276921) in GitLab 13.6) - Administrator added or removed ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/323905) in GitLab 14.1) - Removed SSH key ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/220127) in GitLab 14.1) +- Added or removed GPG key ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/220127) in GitLab 14.1) Instance events can also be accessed via the [Instance Audit Events API](../api/audit_events.md#instance-audit-events). diff --git a/ee/app/models/concerns/auditable.rb b/ee/app/models/concerns/auditable.rb index d5e66565a40ff3df2be351b941142cb8e33afa54..187dd0a097d086f9eef8a25754b2893b5ec1813d 100644 --- a/ee/app/models/concerns/auditable.rb +++ b/ee/app/models/concerns/auditable.rb @@ -6,4 +6,8 @@ def push_audit_event(event) ::Gitlab::Audit::EventQueue.push(event) end + + def audit_details + raise NotImplementedError, "#{self.class} does not implement #{__method__}" + end end diff --git a/ee/app/models/ee/gpg_key.rb b/ee/app/models/ee/gpg_key.rb index c2edd6b7fb223865dbe24c8660095c567b5c7c44..c48adecac5101bcfaf85ed9e36393345c6e686f8 100644 --- a/ee/app/models/ee/gpg_key.rb +++ b/ee/app/models/ee/gpg_key.rb @@ -4,9 +4,15 @@ module EE module GpgKey extend ActiveSupport::Concern + include Auditable + prepended do scope :preload_users, -> { preload(:user) } scope :for_user, -> (user) { where(user: user) } end + + def audit_details + user.name + end end end diff --git a/ee/app/models/ee/key.rb b/ee/app/models/ee/key.rb index c3ba5f47c20bd7b5ee0a993ffc3e7b790a804fdc..c03943ba9a9b84edc9097154b14658fb9a7e5d57 100644 --- a/ee/app/models/ee/key.rb +++ b/ee/app/models/ee/key.rb @@ -4,6 +4,8 @@ module EE module Key extend ActiveSupport::Concern + include Auditable + prepended do include UsageStatistics @@ -41,5 +43,9 @@ def enforce_ssh_key_expiration_feature_available? License.feature_available?(:enforce_ssh_key_expiration) end end + + def audit_details + title + end end end diff --git a/ee/app/services/ee/gpg_keys/create_service.rb b/ee/app/services/ee/gpg_keys/create_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..c7ce188f3d010315e451ce8e830e8ee3558cf891 --- /dev/null +++ b/ee/app/services/ee/gpg_keys/create_service.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +module EE + module GpgKeys + module CreateService + extend ::Gitlab::Utils::Override + + override :create + def create(params) + super.tap do |key| + next unless key.persisted? + + audit_context = { + name: 'add_gpg_key', + author: user, + scope: key.user, + target: key, + message: 'Added GPG key' + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + end + end + end + end +end diff --git a/ee/app/services/ee/gpg_keys/destroy_service.rb b/ee/app/services/ee/gpg_keys/destroy_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..12789d055ca10e571697f6243929eb4557344bad --- /dev/null +++ b/ee/app/services/ee/gpg_keys/destroy_service.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +module EE + module GpgKeys + module DestroyService + extend ::Gitlab::Utils::Override + + override :execute + def execute(key) + super.tap do |destroyed| + next unless destroyed + + audit_context = { + name: 'remove_gpg_key', + author: user, + scope: key.user, + target: key, + message: 'Removed GPG key' + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + end + end + end + end +end diff --git a/ee/lib/gitlab/audit/target.rb b/ee/lib/gitlab/audit/target.rb index 7bfd2994915a445702138db42b731afc6ce6baf9..b9cb54aece8c4836fdded4dd91b4696a64840ff0 100644 --- a/ee/lib/gitlab/audit/target.rb +++ b/ee/lib/gitlab/audit/target.rb @@ -14,7 +14,7 @@ def type end def details - @object.try(:name) || @object.try(:title) || 'unknown' + @object.try(:name) || @object.try(:audit_details) || 'unknown' end end end diff --git a/ee/spec/lib/gitlab/audit/target_spec.rb b/ee/spec/lib/gitlab/audit/target_spec.rb index d4441d11576d076e95f0856a9c63a7cda8733941..67b814cd10ce40fa2a6138e19f0a4661375851b2 100644 --- a/ee/spec/lib/gitlab/audit/target_spec.rb +++ b/ee/spec/lib/gitlab/audit/target_spec.rb @@ -26,7 +26,7 @@ describe '#details' do using RSpec::Parameterized::TableSyntax - where(:name, :title, :details) do + where(:name, :audit_details, :details) do 'jackie' | 'wanderer' | 'jackie' 'jackie' | nil | 'jackie' nil | 'wanderer' | 'wanderer' @@ -35,7 +35,7 @@ before do allow(object).to receive(:name).and_return(name) if name - allow(object).to receive(:title).and_return(title) if title + allow(object).to receive(:audit_details).and_return(audit_details) if audit_details end with_them do diff --git a/ee/spec/models/concerns/auditable_spec.rb b/ee/spec/models/concerns/auditable_spec.rb index ea31e3ccfbdd0bcbdbe59483683261a4f54db02c..d56f100190812589a5f89974c58ae9a71b085e82 100644 --- a/ee/spec/models/concerns/auditable_spec.rb +++ b/ee/spec/models/concerns/auditable_spec.rb @@ -34,4 +34,10 @@ end end end + + describe '#audit_details' do + it 'raises error to prompt for implementation' do + expect { instance.audit_details }.to raise_error(/does not implement audit_details/) + end + end end diff --git a/ee/spec/models/ee/gpg_key_spec.rb b/ee/spec/models/ee/gpg_key_spec.rb index 19e1c827227b561d76fb36c11d2e02ff4f471aa7..b182e44bdd3e4c0a6854d13199aa2e37828fc53f 100644 --- a/ee/spec/models/ee/gpg_key_spec.rb +++ b/ee/spec/models/ee/gpg_key_spec.rb @@ -12,4 +12,10 @@ it { is_expected.to contain_exactly(gpg_key) } end + + describe '#audit_details' do + it "equals to the user's name" do + expect(gpg_key.audit_details).to eq(user.name) + end + end end diff --git a/ee/spec/models/ee/key_spec.rb b/ee/spec/models/ee/key_spec.rb index 09150e7fae10acded17b09c37541bd6565b1e721..d7ff2dc95f6b8305b6e6e1533f5c3fd38018c778 100644 --- a/ee/spec/models/ee/key_spec.rb +++ b/ee/spec/models/ee/key_spec.rb @@ -72,4 +72,11 @@ end end end + + describe '#audit_details' do + it 'equals to the title' do + key = build(:personal_key) + expect(key.audit_details).to eq(key.title) + end + end end diff --git a/ee/spec/services/ee/gpg_keys/create_service_spec.rb b/ee/spec/services/ee/gpg_keys/create_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..67d9aaec19cf307a5bfef33a86d6dfad6fc8997e --- /dev/null +++ b/ee/spec/services/ee/gpg_keys/create_service_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe GpgKeys::CreateService do + let_it_be(:user) { create(:user) } + + let(:params) { attributes_for(:gpg_key) } + + subject { described_class.new(user, params) } + + it 'creates an audit event', :aggregate_failures do + expect { subject.execute }.to change(GpgKey, :count).by(1) + .and change(AuditEvent, :count).by(1) + + key = user.gpg_keys.last + + expect(AuditEvent.last).to have_attributes( + author: user, + entity_id: key.user.id, + target_id: key.id, + target_type: key.class.name, + target_details: key.user.name, + details: include(custom_message: 'Added GPG key') + ) + end + + it 'returns the correct value' do + expect(subject.execute).to eq(GpgKey.last) + end + + context 'when create operation fails' do + let(:params) { attributes_for(:gpg_key).except(:key) } + + it 'does not create an audit event' do + expect { subject.execute }.not_to change(AuditEvent, :count) + end + + it 'returns the correct value' do + expect(subject.execute).to be_a_new(GpgKey) + end + end +end diff --git a/ee/spec/services/ee/gpg_keys/destroy_service_spec.rb b/ee/spec/services/ee/gpg_keys/destroy_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..52b23cdfe1583e76bd77df42663a0fcaafc76712 --- /dev/null +++ b/ee/spec/services/ee/gpg_keys/destroy_service_spec.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe GpgKeys::DestroyService do + let_it_be(:user) { create(:user) } + + subject { described_class.new(user) } + + it 'creates an audit event', :aggregate_failures do + key = create(:personal_key) + + expect { subject.execute(key) }.to change(AuditEvent, :count).by(1) + + expect(AuditEvent.last).to have_attributes( + author: user, + entity_id: key.user.id, + target_id: key.id, + target_type: key.class.name, + target_details: key.title, + details: include(custom_message: 'Removed GPG key') + ) + end + + it 'returns the correct value' do + key = build(:gpg_key) + allow(key).to receive(:destroy).and_return(true) + + expect(subject.execute(key)).to eq(true) + end + + context 'when destroy operation fails' do + let(:key) { build(:gpg_key) } + + before do + allow(key).to receive(:destroy).and_return(false) + end + + it 'does not create an audit event' do + expect { subject.execute(key) }.not_to change(AuditEvent, :count) + end + + it 'returns the correct value' do + expect(subject.execute(key)).to eq(false) + end + end +end