From 0dfbe948532f6a02658734890d5cb2ee6742dfca Mon Sep 17 00:00:00 2001 From: moaz-khalifa Date: Thu, 16 Jan 2025 15:29:19 +0100 Subject: [PATCH 1/6] Send audit event for packages creation --- .../packages/create_package_service.rb | 31 +++- .../debian/process_package_file_service.rb | 6 + .../packages/helm/process_file_service.rb | 6 + .../npm/process_package_file_service.rb | 7 + .../nuget/create_or_update_package_service.rb | 11 +- .../update_package_from_metadata_service.rb | 19 ++- .../packages/rubygems/process_gem_service.rb | 6 + .../package_registry_package_published.yml | 10 ++ doc/user/compliance/audit_event_types.md | 6 + .../ee/packages/create_package_service.rb | 16 ++ .../debian/process_package_file_service.rb | 18 +++ .../ee/packages/helm/process_file_service.rb | 18 +++ .../npm/process_package_file_service.rb | 18 +++ .../update_package_from_metadata_service.rb | 18 +++ .../packages/rubygems/process_gem_service.rb | 18 +++ .../packages/create_audit_event_service.rb | 68 +++++++++ .../wip/package_registry_audit_events.yml | 9 ++ .../packages/create_package_service_spec.rb | 65 ++++++++ .../process_package_file_service_spec.rb | 21 +++ .../helm/process_file_service_spec.rb | 19 +++ .../npm/process_package_file_service_spec.rb | 27 ++++ ...date_package_from_metadata_service_spec.rb | 19 +++ .../rubygems/process_gem_service_spec.rb | 18 +++ .../create_audit_event_service_spec.rb | 143 ++++++++++++++++++ 24 files changed, 578 insertions(+), 19 deletions(-) create mode 100644 config/audit_events/types/package_registry_package_published.yml create mode 100644 ee/app/services/ee/packages/create_package_service.rb create mode 100644 ee/app/services/ee/packages/debian/process_package_file_service.rb create mode 100644 ee/app/services/ee/packages/helm/process_file_service.rb create mode 100644 ee/app/services/ee/packages/npm/process_package_file_service.rb create mode 100644 ee/app/services/ee/packages/nuget/update_package_from_metadata_service.rb create mode 100644 ee/app/services/ee/packages/rubygems/process_gem_service.rb create mode 100644 ee/app/services/packages/create_audit_event_service.rb create mode 100644 ee/config/feature_flags/wip/package_registry_audit_events.yml create mode 100644 ee/spec/services/ee/packages/create_package_service_spec.rb create mode 100644 ee/spec/services/ee/packages/debian/process_package_file_service_spec.rb create mode 100644 ee/spec/services/ee/packages/helm/process_file_service_spec.rb create mode 100644 ee/spec/services/ee/packages/npm/process_package_file_service_spec.rb create mode 100644 ee/spec/services/ee/packages/nuget/update_package_from_metadata_service_spec.rb create mode 100644 ee/spec/services/ee/packages/rubygems/process_gem_service_spec.rb create mode 100644 ee/spec/services/packages/create_audit_event_service_spec.rb diff --git a/app/services/packages/create_package_service.rb b/app/services/packages/create_package_service.rb index 792c51232b5ae8..aab90a8acf65e5 100644 --- a/app/services/packages/create_package_service.rb +++ b/app/services/packages/create_package_service.rb @@ -5,10 +5,11 @@ class CreatePackageService < BaseService protected def find_or_create_package!(package_type, name: params[:name], version: params[:version]) + created = false # safe_find_or_create_by! was originally called here. # We merely switched to `find_or_create_by!` # rubocop: disable CodeReuse/ActiveRecord - project + package = project .packages .with_package_type(package_type) .not_pending_destruction @@ -17,17 +18,27 @@ def find_or_create_package!(package_type, name: params[:name], version: params[: package.creator = package_creator add_build_info(package) + + created = true end # rubocop: enable CodeReuse/ActiveRecord + + send_audit_event(package) if created + + package end def create_package!(package_type, attrs = {}) - project - .packages - .with_package_type(package_type) - .create!(package_attrs(attrs)) do |package| - add_build_info(package) - end + package = project + .packages + .with_package_type(package_type) + .create!(package_attrs(attrs)) do |package| + add_build_info(package) + end + + send_audit_event(package) + + package end def can_create_package? @@ -67,5 +78,11 @@ def add_build_info(package) package.build_infos.new(pipeline: params[:build].pipeline) end end + + def send_audit_event(package) + # defined in EE + end end end + +Packages::CreatePackageService.prepend_mod diff --git a/app/services/packages/debian/process_package_file_service.rb b/app/services/packages/debian/process_package_file_service.rb index a1bcc0a6f7338c..9fe463bd86382e 100644 --- a/app/services/packages/debian/process_package_file_service.rb +++ b/app/services/packages/debian/process_package_file_service.rb @@ -26,6 +26,7 @@ def execute try_obtain_lease do distribution.transaction do rename_package_and_set_version + send_audit_event update_package update_files_metadata if changes_file? update_file_metadata @@ -207,6 +208,11 @@ def lease_key def lease_timeout DEFAULT_LEASE_TIMEOUT end + + # defined in EE + def send_audit_event; end end end end + +Packages::Debian::ProcessPackageFileService.prepend_mod diff --git a/app/services/packages/helm/process_file_service.rb b/app/services/packages/helm/process_file_service.rb index 219f3d8c781129..265ea258fb7e46 100644 --- a/app/services/packages/helm/process_file_service.rb +++ b/app/services/packages/helm/process_file_service.rb @@ -20,6 +20,7 @@ def execute try_obtain_lease do temp_package.transaction do rename_package_and_set_version + send_audit_event rename_package_file_and_set_metadata cleanup_temp_package end @@ -90,6 +91,11 @@ def lease_key def lease_timeout DEFAULT_LEASE_TIMEOUT end + + # defined in EE + def send_audit_event; end end end end + +Packages::Helm::ProcessFileService.prepend_mod diff --git a/app/services/packages/npm/process_package_file_service.rb b/app/services/packages/npm/process_package_file_service.rb index 0cff80a82c9998..28451d0c04ca64 100644 --- a/app/services/packages/npm/process_package_file_service.rb +++ b/app/services/packages/npm/process_package_file_service.rb @@ -25,6 +25,8 @@ def execute package.default! + send_audit_event + ::Packages::Npm::CreateMetadataCacheWorker.perform_async(package.project_id, package.name) ServiceResponse.success @@ -70,6 +72,11 @@ def path_for(entry) rescue ::Gem::Package::TarInvalidError entry.header.name end + + # defined in EE + def send_audit_event; end end end end + +Packages::Npm::ProcessPackageFileService.prepend_mod diff --git a/app/services/packages/nuget/create_or_update_package_service.rb b/app/services/packages/nuget/create_or_update_package_service.rb index de01757c25ce2e..73adbab6204c49 100644 --- a/app/services/packages/nuget/create_or_update_package_service.rb +++ b/app/services/packages/nuget/create_or_update_package_service.rb @@ -2,7 +2,7 @@ module Packages module Nuget - class CreateOrUpdatePackageService < BaseService + class CreateOrUpdatePackageService < ::Packages::CreatePackageService include ::Gitlab::Utils::StrongMemoize include ExclusiveLeaseGuard @@ -77,6 +77,8 @@ def update_tags end def create_build_infos + return unless existing_package + target_package.create_build_infos!(params[:build]) end @@ -94,12 +96,7 @@ def target_package strong_memoize_attr :target_package def create_new_package - ::Packages::Nuget::Package.create!( - name: metadata[:package_name], - version: metadata[:package_version], - project: project, - creator: current_user.is_a?(User) ? current_user : nil - ) + create_package!(:nuget, name: metadata[:package_name], version: metadata[:package_version]) end def package_filename diff --git a/app/services/packages/nuget/update_package_from_metadata_service.rb b/app/services/packages/nuget/update_package_from_metadata_service.rb index 811b4355f1047e..0c53be382f88b9 100644 --- a/app/services/packages/nuget/update_package_from_metadata_service.rb +++ b/app/services/packages/nuget/update_package_from_metadata_service.rb @@ -15,6 +15,8 @@ class UpdatePackageFromMetadataService InvalidMetadataError = ZipError = DuplicatePackageError = Class.new(StandardError) + delegate :package, to: :@package_file, private: true + def initialize(package_file, package_zip_file) @package_file = package_file @package_zip_file = package_zip_file @@ -43,12 +45,12 @@ def execute def process_package_update package_to_destroy = nil - target_package = @package_file.package + target_package = package if existing_package raise DuplicatePackageError, "A package '#{package_name}' with version '#{package_version}' already exists" unless symbol_package? || duplicates_allowed? - package_to_destroy = @package_file.package + package_to_destroy = package target_package = existing_package else if symbol_package? @@ -101,15 +103,17 @@ def valid_metadata? end def update_linked_package - @package_file.package.update!( + package.update!( name: package_name, version: package_version, status: :default ) - ::Packages::Nuget::CreateDependencyService.new(@package_file.package, package_dependencies) + send_audit_event + + ::Packages::Nuget::CreateDependencyService.new(package, package_dependencies) .execute - @package_file.package + package end def existing_package @@ -177,6 +181,11 @@ def lease_key def lease_timeout DEFAULT_LEASE_TIMEOUT end + + # defined in EE + def send_audit_event; end end end end + +Packages::Nuget::UpdatePackageFromMetadataService.prepend_mod diff --git a/app/services/packages/rubygems/process_gem_service.rb b/app/services/packages/rubygems/process_gem_service.rb index 07545495f1bf8d..f0b7c63fcae769 100644 --- a/app/services/packages/rubygems/process_gem_service.rb +++ b/app/services/packages/rubygems/process_gem_service.rb @@ -35,6 +35,7 @@ def process_gem try_obtain_lease do package.transaction do rename_package_and_set_version + send_audit_event rename_package_file ::Packages::Rubygems::MetadataExtractionService.new(package, gemspec).execute ::Packages::Rubygems::CreateGemspecService.new(package, gemspec).execute @@ -120,6 +121,11 @@ def lease_key def lease_timeout DEFAULT_LEASE_TIMEOUT end + + # defined in EE + def send_audit_event; end end end end + +Packages::Rubygems::ProcessGemService.prepend_mod diff --git a/config/audit_events/types/package_registry_package_published.yml b/config/audit_events/types/package_registry_package_published.yml new file mode 100644 index 00000000000000..48a8e917d1d927 --- /dev/null +++ b/config/audit_events/types/package_registry_package_published.yml @@ -0,0 +1,10 @@ +--- +name: package_registry_package_published +description: A package was published to GitLab package registry. Available only when the feature flag `package_registry_audit_events` is enabled. +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/329588 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/178181 +feature_category: package_registry +milestone: "17.9" +saved_to_database: true +streamed: true +scope: [Project, Group] diff --git a/doc/user/compliance/audit_event_types.md b/doc/user/compliance/audit_event_types.md index 1c26e9254c470b..5c9cca7515ebb3 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -443,6 +443,12 @@ Audit event types belong to the following product categories. |:----------|:---------------------|:------------------|:--------------|:------| | [`experiment_features_enabled_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/118222) | Enabling experiment AI features setting is toggled | **{check-circle}** Yes | GitLab [16.0](https://gitlab.com/gitlab-org/gitlab/-/issues/404856/) | Group | +### Package registry + +| Type name | Event triggered when | Saved to database | Introduced in | Scope | +|:----------|:---------------------|:------------------|:--------------|:------| +| [`package_registry_package_published`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/178181) | A package was published to GitLab package registry. Available only when the feature flag `package_registry_audit_events` is enabled. | **{check-circle}** Yes | GitLab [17.9](https://gitlab.com/gitlab-org/gitlab/-/issues/329588) | Project, Group | + ### Permissions | Type name | Event triggered when | Saved to database | Introduced in | Scope | diff --git a/ee/app/services/ee/packages/create_package_service.rb b/ee/app/services/ee/packages/create_package_service.rb new file mode 100644 index 00000000000000..a1a0a65ed318c3 --- /dev/null +++ b/ee/app/services/ee/packages/create_package_service.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +module EE + module Packages + module CreatePackageService + extend ::Gitlab::Utils::Override + + private + + override :send_audit_event + def send_audit_event(package) + ::Packages::CreateAuditEventService.new(package, current_user:).execute + end + end + end +end diff --git a/ee/app/services/ee/packages/debian/process_package_file_service.rb b/ee/app/services/ee/packages/debian/process_package_file_service.rb new file mode 100644 index 00000000000000..761249e8fbe048 --- /dev/null +++ b/ee/app/services/ee/packages/debian/process_package_file_service.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module EE + module Packages + module Debian + module ProcessPackageFileService + extend ::Gitlab::Utils::Override + + private + + override :send_audit_event + def send_audit_event + ::Packages::CreateAuditEventService.new(package).execute + end + end + end + end +end diff --git a/ee/app/services/ee/packages/helm/process_file_service.rb b/ee/app/services/ee/packages/helm/process_file_service.rb new file mode 100644 index 00000000000000..cc9e873aa9b50f --- /dev/null +++ b/ee/app/services/ee/packages/helm/process_file_service.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module EE + module Packages + module Helm + module ProcessFileService + extend ::Gitlab::Utils::Override + + private + + override :send_audit_event + def send_audit_event + ::Packages::CreateAuditEventService.new(package).execute + end + end + end + end +end diff --git a/ee/app/services/ee/packages/npm/process_package_file_service.rb b/ee/app/services/ee/packages/npm/process_package_file_service.rb new file mode 100644 index 00000000000000..f5a1f983c1fdad --- /dev/null +++ b/ee/app/services/ee/packages/npm/process_package_file_service.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module EE + module Packages + module Npm + module ProcessPackageFileService + extend ::Gitlab::Utils::Override + + private + + override :send_audit_event + def send_audit_event + ::Packages::CreateAuditEventService.new(package).execute + end + end + end + end +end diff --git a/ee/app/services/ee/packages/nuget/update_package_from_metadata_service.rb b/ee/app/services/ee/packages/nuget/update_package_from_metadata_service.rb new file mode 100644 index 00000000000000..72dfd05a49221e --- /dev/null +++ b/ee/app/services/ee/packages/nuget/update_package_from_metadata_service.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module EE + module Packages + module Nuget + module UpdatePackageFromMetadataService + extend ::Gitlab::Utils::Override + + private + + override :send_audit_event + def send_audit_event + ::Packages::CreateAuditEventService.new(package).execute + end + end + end + end +end diff --git a/ee/app/services/ee/packages/rubygems/process_gem_service.rb b/ee/app/services/ee/packages/rubygems/process_gem_service.rb new file mode 100644 index 00000000000000..9ee8919923d146 --- /dev/null +++ b/ee/app/services/ee/packages/rubygems/process_gem_service.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module EE + module Packages + module Rubygems + module ProcessGemService + extend ::Gitlab::Utils::Override + + private + + override :send_audit_event + def send_audit_event + ::Packages::CreateAuditEventService.new(package).execute + end + end + end + end +end diff --git a/ee/app/services/packages/create_audit_event_service.rb b/ee/app/services/packages/create_audit_event_service.rb new file mode 100644 index 00000000000000..2c4383845276f2 --- /dev/null +++ b/ee/app/services/packages/create_audit_event_service.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true + +module Packages + class CreateAuditEventService + FEATURE_FLAG_DISABLED_ERROR = ServiceResponse.error(message: 'Feature flag is not enabled').freeze + INVALID_STATE_ERROR = ServiceResponse.error(message: 'Package is not in the default state').freeze + + delegate :project, to: :package, private: true + + def initialize(package, current_user: nil, event_name: 'package_registry_package_published') + @package = package + @current_user = current_user + @event_name = event_name + end + + def execute + return FEATURE_FLAG_DISABLED_ERROR if Feature.disabled?(:package_registry_audit_events, package.project) + return INVALID_STATE_ERROR unless package.default? + + ::Gitlab::Audit::Auditor.audit(audit_context) + + ServiceResponse.success + end + + private + + attr_reader :package, :current_user, :event_name + + def audit_context + { + name: event_name, + author: author, + scope: project.group || project, + target: package, + target_details: target_details, + message: "#{package.package_type} package published", + additional_details: { auth_token_type: } + } + end + + def author + current_user || package.creator || ::Gitlab::Audit::DeployTokenAuthor.new + end + + def target_details + "#{project.full_path}/#{package.name}-#{package.version}" + end + + def auth_token_type + ::Current.token_info&.dig(:token_type) || token_type_from_current_user || token_type_from_package_creator + end + + def token_type_from_current_user + return unless current_user + + return 'DeployToken' if current_user.is_a?(DeployToken) + return 'CiJobToken' if current_user.from_ci_job_token? + + 'PersonalAccessToken' + end + + def token_type_from_package_creator + return 'DeployToken' unless package.creator + + 'PersonalAccessToken or CiJobToken' + end + end +end diff --git a/ee/config/feature_flags/wip/package_registry_audit_events.yml b/ee/config/feature_flags/wip/package_registry_audit_events.yml new file mode 100644 index 00000000000000..a2793d787c00de --- /dev/null +++ b/ee/config/feature_flags/wip/package_registry_audit_events.yml @@ -0,0 +1,9 @@ +--- +name: package_registry_audit_events +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/329588 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/178181 +rollout_issue_url: +milestone: '17.9' +group: group::package registry +type: wip +default_enabled: false diff --git a/ee/spec/services/ee/packages/create_package_service_spec.rb b/ee/spec/services/ee/packages/create_package_service_spec.rb new file mode 100644 index 00000000000000..d8a328cf8a1f6c --- /dev/null +++ b/ee/spec/services/ee/packages/create_package_service_spec.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::CreatePackageService, feature_category: :package_registry do + let_it_be(:project) { create(:project) } + let_it_be(:user) { create(:user) } + + let(:params) { { name: 'package-name', version: '1.0.0' } } + let(:service) { subclass.new(project, user, params) } + + describe '#execute' do + subject(:execute) { service.execute } + + context 'when calling find_or_create_package!' do + let(:subclass) do + Class.new(described_class) do + def execute + find_or_create_package!(:maven) + end + end + end + + context 'when package is found' do + before do + create(:maven_package, project: project, name: params[:name], version: params[:version]) + end + + it 'does not call the CreateAuditEventService' do + expect(::Packages::CreateAuditEventService).not_to receive(:new) + + execute + end + end + + context 'when package is created' do + it 'calls the CreateAuditEventService' do + expect(::Packages::CreateAuditEventService).to receive(:new) + .with(instance_of(Packages::Package), current_user: user) + .and_call_original + + execute + end + end + end + + context 'when calling create_package!' do + let(:subclass) do + Class.new(described_class) do + def execute + create_package!(:maven) + end + end + end + + it 'calls the CreateAuditEventService' do + expect(::Packages::CreateAuditEventService).to receive(:new) + .with(instance_of(Packages::Package), current_user: user) + .and_call_original + + execute + end + end + end +end diff --git a/ee/spec/services/ee/packages/debian/process_package_file_service_spec.rb b/ee/spec/services/ee/packages/debian/process_package_file_service_spec.rb new file mode 100644 index 00000000000000..c415b78d1e81eb --- /dev/null +++ b/ee/spec/services/ee/packages/debian/process_package_file_service_spec.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::Debian::ProcessPackageFileService, feature_category: :package_registry do + let_it_be(:distribution) { create(:debian_project_distribution, :with_file, suite: 'unstable') } + let_it_be(:package) { create(:debian_temporary_with_files, project: distribution.project) } + + let(:package_file) { package.package_files.with_file_name('libsample0_1.2.3~alpha2_amd64.deb').first } + let(:distribution_name) { distribution.codename } + let(:component_name) { 'main' } + let(:service) { described_class.new(package_file, distribution_name, component_name) } + + describe '#execute' do + it 'calls the CreateAuditEventService' do + expect(::Packages::CreateAuditEventService).to receive(:new).with(package).and_call_original + + service.execute + end + end +end diff --git a/ee/spec/services/ee/packages/helm/process_file_service_spec.rb b/ee/spec/services/ee/packages/helm/process_file_service_spec.rb new file mode 100644 index 00000000000000..8f500bd9f67528 --- /dev/null +++ b/ee/spec/services/ee/packages/helm/process_file_service_spec.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::Helm::ProcessFileService, feature_category: :package_registry do + let_it_be(:package) { create(:helm_package, :processing, without_package_files: true) } + let_it_be(:package_file) { create(:helm_package_file, without_loaded_metadatum: true, package: package) } + + let(:channel) { 'stable' } + let(:service) { described_class.new(channel, package_file) } + + describe '#execute' do + it 'calls the CreateAuditEventService' do + expect(::Packages::CreateAuditEventService).to receive(:new).with(package).and_call_original + + service.execute + end + end +end diff --git a/ee/spec/services/ee/packages/npm/process_package_file_service_spec.rb b/ee/spec/services/ee/packages/npm/process_package_file_service_spec.rb new file mode 100644 index 00000000000000..b111000a843267 --- /dev/null +++ b/ee/spec/services/ee/packages/npm/process_package_file_service_spec.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::Npm::ProcessPackageFileService, feature_category: :package_registry do + let_it_be(:package) { build(:npm_package, :processing, id: 1) } + let_it_be(:package_file) do + build(:package_file, :npm, file_fixture: expand_fixture_path('packages/npm/package-1.3.7.tgz'), package: package) + end + + let(:service) { described_class.new(package_file) } + + describe '#execute' do + before do + allow_next_instance_of(::Packages::Npm::CheckManifestCoherenceService, package, + instance_of(Gem::Package::TarReader::Entry)) do |service| + allow(service).to receive(:execute).and_return(ServiceResponse.success) + end + end + + it 'calls the CreateAuditEventService' do + expect(::Packages::CreateAuditEventService).to receive(:new).with(package).and_call_original + + service.execute + end + end +end diff --git a/ee/spec/services/ee/packages/nuget/update_package_from_metadata_service_spec.rb b/ee/spec/services/ee/packages/nuget/update_package_from_metadata_service_spec.rb new file mode 100644 index 00000000000000..423d925eafd25c --- /dev/null +++ b/ee/spec/services/ee/packages/nuget/update_package_from_metadata_service_spec.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::Nuget::UpdatePackageFromMetadataService, feature_category: :package_registry do + let_it_be(:package) { create(:nuget_package, :processing) } + let(:package_file) { package.reload.package_files.first } + let(:package_zip_file) { Zip::File.new(package_file.file) } # rubocop:disable Performance/Rubyzip -- for testing purpose only + + let(:service) { described_class.new(package_file, package_zip_file) } + + describe '#execute' do + it 'calls the CreateAuditEventService' do + expect(::Packages::CreateAuditEventService).to receive(:new).with(package).and_call_original + + service.execute + end + end +end diff --git a/ee/spec/services/ee/packages/rubygems/process_gem_service_spec.rb b/ee/spec/services/ee/packages/rubygems/process_gem_service_spec.rb new file mode 100644 index 00000000000000..c5c25a8077de9d --- /dev/null +++ b/ee/spec/services/ee/packages/rubygems/process_gem_service_spec.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::Rubygems::ProcessGemService, feature_category: :package_registry do + let_it_be(:package) { create(:rubygems_package, :processing) } + + let(:package_file) { create(:package_file, :unprocessed_gem, package: package) } + let(:service) { described_class.new(package_file) } + + describe '#execute' do + it 'calls the CreateAuditEventService' do + expect(::Packages::CreateAuditEventService).to receive(:new).with(package).and_call_original + + service.execute + end + end +end diff --git a/ee/spec/services/packages/create_audit_event_service_spec.rb b/ee/spec/services/packages/create_audit_event_service_spec.rb new file mode 100644 index 00000000000000..ab34d934dc70b8 --- /dev/null +++ b/ee/spec/services/packages/create_audit_event_service_spec.rb @@ -0,0 +1,143 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::CreateAuditEventService, feature_category: :package_registry do + let_it_be(:project) { build_stubbed(:project, group: build_stubbed(:group)) } + let_it_be(:user) { build_stubbed(:user) } + let_it_be(:package) { build_stubbed(:package, project: project, creator: user) } + let_it_be(:deploy_token) { build_stubbed(:deploy_token) } + + let(:current_user) { user } + let(:service) { described_class.new(package, current_user: current_user) } + + describe '#execute' do + subject(:execute) { service.execute } + + include_examples 'audit event logging' do + let(:operation) { execute } + let(:event_type) { 'package_registry_package_published' } + let(:fail_condition!) { allow(package).to receive(:default?).and_return(false) } + let(:attributes) do + { + author_id: current_user.id, + entity_id: project.group.id, + entity_type: 'Group', + details: { + author_name: current_user.name, + event_name: 'package_registry_package_published', + target_id: package.id, + target_type: package.class.name, + target_details: "#{project.full_path}/#{package.name}-#{package.version}", + author_class: user.class.name, + custom_message: "#{package.package_type} package published", + auth_token_type: 'PersonalAccessToken' + } + } + end + end + + context 'when feature flag is disabled' do + before do + stub_feature_flags(package_registry_audit_events: false) + end + + it { is_expected.to eq(described_class::FEATURE_FLAG_DISABLED_ERROR) } + end + + context 'when project does not belong to a group' do + before do + project.group = nil + end + + it 'uses project as scope' do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with( + hash_including(scope: project) + ) + + execute + end + end + + context 'for auth token type detection' do + context 'when Current.token_info is present' do + before do + allow(::Current).to receive(:token_info).and_return({ token_type: 'SomeToken' }) + end + + it 'uses token type from Current' do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with( + hash_including(additional_details: { auth_token_type: 'SomeToken' }) + ) + + execute + end + end + + context 'with current_user' do + it 'sets auth_token_type as PersonalAccessToken' do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with( + hash_including(additional_details: { auth_token_type: 'PersonalAccessToken' }) + ) + + execute + end + + context 'when current user is a deploy token' do + let(:current_user) { deploy_token } + + it 'sets auth_token_type as DeployToken' do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with( + hash_including(additional_details: { auth_token_type: 'DeployToken' }) + ) + + execute + end + end + + context 'when current user is a CiJobToken' do + before do + allow(current_user).to receive(:from_ci_job_token?).and_return(true) + end + + it 'sets auth_token_type as CiJobToken' do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with( + hash_including(additional_details: { auth_token_type: 'CiJobToken' }) + ) + + execute + end + end + end + + context 'without current_user' do + let(:current_user) { nil } + + it 'sets auth_token_type as PersonalAccessToken or CiJobToken' do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with( + hash_including(additional_details: { auth_token_type: 'PersonalAccessToken or CiJobToken' }) + ) + + execute + end + + context 'when package has no creator' do + before do + allow(package).to receive(:creator).and_return(nil) + end + + it 'uses DeployTokenAuthor as author' do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with( + hash_including( + author: kind_of(::Gitlab::Audit::DeployTokenAuthor), + additional_details: { auth_token_type: 'DeployToken' } + ) + ) + + execute + end + end + end + end + end +end -- GitLab From 349a080eefe33278499639e740030625677944e1 Mon Sep 17 00:00:00 2001 From: moaz-khalifa Date: Mon, 20 Jan 2025 10:35:38 +0100 Subject: [PATCH 2/6] Apply backend reviewer feedback --- app/services/packages/create_package_service.rb | 5 +---- ee/app/services/packages/create_audit_event_service.rb | 4 ++-- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/app/services/packages/create_package_service.rb b/app/services/packages/create_package_service.rb index aab90a8acf65e5..4407b322112500 100644 --- a/app/services/packages/create_package_service.rb +++ b/app/services/packages/create_package_service.rb @@ -5,7 +5,6 @@ class CreatePackageService < BaseService protected def find_or_create_package!(package_type, name: params[:name], version: params[:version]) - created = false # safe_find_or_create_by! was originally called here. # We merely switched to `find_or_create_by!` # rubocop: disable CodeReuse/ActiveRecord @@ -18,12 +17,10 @@ def find_or_create_package!(package_type, name: params[:name], version: params[: package.creator = package_creator add_build_info(package) - - created = true end # rubocop: enable CodeReuse/ActiveRecord - send_audit_event(package) if created + send_audit_event(package) if package.previously_new_record? package end diff --git a/ee/app/services/packages/create_audit_event_service.rb b/ee/app/services/packages/create_audit_event_service.rb index 2c4383845276f2..16dc50e77732ba 100644 --- a/ee/app/services/packages/create_audit_event_service.rb +++ b/ee/app/services/packages/create_audit_event_service.rb @@ -17,9 +17,9 @@ def execute return FEATURE_FLAG_DISABLED_ERROR if Feature.disabled?(:package_registry_audit_events, package.project) return INVALID_STATE_ERROR unless package.default? - ::Gitlab::Audit::Auditor.audit(audit_context) + audit_event = ::Gitlab::Audit::Auditor.audit(audit_context) - ServiceResponse.success + ServiceResponse.success(payload: { audit_event: }) end private -- GitLab From 4e578645a53875dc3d18268aa14a2dc3a13efb82 Mon Sep 17 00:00:00 2001 From: moaz-khalifa Date: Mon, 20 Jan 2025 13:52:00 +0100 Subject: [PATCH 3/6] Move event yml to ee --- .../audit_events/types/package_registry_package_published.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename {config => ee/config}/audit_events/types/package_registry_package_published.yml (96%) diff --git a/config/audit_events/types/package_registry_package_published.yml b/ee/config/audit_events/types/package_registry_package_published.yml similarity index 96% rename from config/audit_events/types/package_registry_package_published.yml rename to ee/config/audit_events/types/package_registry_package_published.yml index 48a8e917d1d927..d6ee0ebfd1d52c 100644 --- a/config/audit_events/types/package_registry_package_published.yml +++ b/ee/config/audit_events/types/package_registry_package_published.yml @@ -4,7 +4,7 @@ description: A package was published to GitLab package registry. Available only introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/329588 introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/178181 feature_category: package_registry -milestone: "17.9" +milestone: '17.9' saved_to_database: true streamed: true scope: [Project, Group] -- GitLab From 37ae796930efe3395f08a460662a8e29b78808b9 Mon Sep 17 00:00:00 2001 From: moaz-khalifa Date: Mon, 20 Jan 2025 20:38:45 +0100 Subject: [PATCH 4/6] Avoid creating event in transaction --- app/models/packages/package.rb | 1 + .../packages/create_package_service.rb | 26 +++++------------- .../debian/process_package_file_service.rb | 6 ++--- .../packages/helm/process_file_service.rb | 6 ++--- .../npm/process_package_file_service.rb | 5 ---- .../update_package_from_metadata_service.rb | 5 ---- .../packages/rubygems/process_gem_service.rb | 4 --- .../ee/packages/create_package_service.rb | 27 ++++++++++++++++--- .../debian/process_package_file_service.rb | 10 +++---- .../ee/packages/helm/process_file_service.rb | 10 +++---- .../npm/process_package_file_service.rb | 10 +++---- .../update_package_from_metadata_service.rb | 10 ++++--- .../packages/rubygems/process_gem_service.rb | 10 +++---- .../packages/create_package_service_spec.rb | 22 ++++++++------- .../process_package_file_service_spec.rb | 6 +++-- .../helm/process_file_service_spec.rb | 6 +++-- .../npm/process_package_file_service_spec.rb | 15 ++++++----- ...date_package_from_metadata_service_spec.rb | 8 +++--- .../rubygems/process_gem_service_spec.rb | 8 +++--- spec/models/packages/package_spec.rb | 1 + .../process_package_file_service_spec.rb | 5 ++++ .../helm/process_file_service_spec.rb | 22 +++++++++++++++ 22 files changed, 130 insertions(+), 93 deletions(-) diff --git a/app/models/packages/package.rb b/app/models/packages/package.rb index 56193d97cb3d29..f3b3ec54359c2a 100644 --- a/app/models/packages/package.rb +++ b/app/models/packages/package.rb @@ -8,6 +8,7 @@ class Packages::Package < ApplicationRecord include Packages::Installable include Packages::Downloadable include EnumInheritance + include AfterCommitQueue DISPLAYABLE_STATUSES = [:default, :error, :deprecated].freeze INSTALLABLE_STATUSES = [:default, :hidden, :deprecated].freeze diff --git a/app/services/packages/create_package_service.rb b/app/services/packages/create_package_service.rb index 4407b322112500..15e5b6e5672ebb 100644 --- a/app/services/packages/create_package_service.rb +++ b/app/services/packages/create_package_service.rb @@ -8,7 +8,7 @@ def find_or_create_package!(package_type, name: params[:name], version: params[: # safe_find_or_create_by! was originally called here. # We merely switched to `find_or_create_by!` # rubocop: disable CodeReuse/ActiveRecord - package = project + project .packages .with_package_type(package_type) .not_pending_destruction @@ -19,23 +19,15 @@ def find_or_create_package!(package_type, name: params[:name], version: params[: add_build_info(package) end # rubocop: enable CodeReuse/ActiveRecord - - send_audit_event(package) if package.previously_new_record? - - package end def create_package!(package_type, attrs = {}) - package = project - .packages - .with_package_type(package_type) - .create!(package_attrs(attrs)) do |package| - add_build_info(package) - end - - send_audit_event(package) - - package + project + .packages + .with_package_type(package_type) + .create!(package_attrs(attrs)) do |package| + add_build_info(package) + end end def can_create_package? @@ -75,10 +67,6 @@ def add_build_info(package) package.build_infos.new(pipeline: params[:build].pipeline) end end - - def send_audit_event(package) - # defined in EE - end end end diff --git a/app/services/packages/debian/process_package_file_service.rb b/app/services/packages/debian/process_package_file_service.rb index 9fe463bd86382e..fbe05d1fee214e 100644 --- a/app/services/packages/debian/process_package_file_service.rb +++ b/app/services/packages/debian/process_package_file_service.rb @@ -26,7 +26,6 @@ def execute try_obtain_lease do distribution.transaction do rename_package_and_set_version - send_audit_event update_package update_files_metadata if changes_file? update_file_metadata @@ -34,6 +33,8 @@ def execute end ::Packages::Debian::GenerateDistributionWorker.perform_async(:project, package.distribution.id) + + ServiceResponse.success end end @@ -208,9 +209,6 @@ def lease_key def lease_timeout DEFAULT_LEASE_TIMEOUT end - - # defined in EE - def send_audit_event; end end end end diff --git a/app/services/packages/helm/process_file_service.rb b/app/services/packages/helm/process_file_service.rb index 265ea258fb7e46..c28e585a996131 100644 --- a/app/services/packages/helm/process_file_service.rb +++ b/app/services/packages/helm/process_file_service.rb @@ -20,10 +20,11 @@ def execute try_obtain_lease do temp_package.transaction do rename_package_and_set_version - send_audit_event rename_package_file_and_set_metadata cleanup_temp_package end + + ServiceResponse.success end end @@ -91,9 +92,6 @@ def lease_key def lease_timeout DEFAULT_LEASE_TIMEOUT end - - # defined in EE - def send_audit_event; end end end end diff --git a/app/services/packages/npm/process_package_file_service.rb b/app/services/packages/npm/process_package_file_service.rb index 28451d0c04ca64..0a87b0f9395052 100644 --- a/app/services/packages/npm/process_package_file_service.rb +++ b/app/services/packages/npm/process_package_file_service.rb @@ -25,8 +25,6 @@ def execute package.default! - send_audit_event - ::Packages::Npm::CreateMetadataCacheWorker.perform_async(package.project_id, package.name) ServiceResponse.success @@ -72,9 +70,6 @@ def path_for(entry) rescue ::Gem::Package::TarInvalidError entry.header.name end - - # defined in EE - def send_audit_event; end end end end diff --git a/app/services/packages/nuget/update_package_from_metadata_service.rb b/app/services/packages/nuget/update_package_from_metadata_service.rb index 0c53be382f88b9..285141a7525173 100644 --- a/app/services/packages/nuget/update_package_from_metadata_service.rb +++ b/app/services/packages/nuget/update_package_from_metadata_service.rb @@ -109,8 +109,6 @@ def update_linked_package status: :default ) - send_audit_event - ::Packages::Nuget::CreateDependencyService.new(package, package_dependencies) .execute package @@ -181,9 +179,6 @@ def lease_key def lease_timeout DEFAULT_LEASE_TIMEOUT end - - # defined in EE - def send_audit_event; end end end end diff --git a/app/services/packages/rubygems/process_gem_service.rb b/app/services/packages/rubygems/process_gem_service.rb index f0b7c63fcae769..2ed975f63121f3 100644 --- a/app/services/packages/rubygems/process_gem_service.rb +++ b/app/services/packages/rubygems/process_gem_service.rb @@ -35,7 +35,6 @@ def process_gem try_obtain_lease do package.transaction do rename_package_and_set_version - send_audit_event rename_package_file ::Packages::Rubygems::MetadataExtractionService.new(package, gemspec).execute ::Packages::Rubygems::CreateGemspecService.new(package, gemspec).execute @@ -121,9 +120,6 @@ def lease_key def lease_timeout DEFAULT_LEASE_TIMEOUT end - - # defined in EE - def send_audit_event; end end end end diff --git a/ee/app/services/ee/packages/create_package_service.rb b/ee/app/services/ee/packages/create_package_service.rb index a1a0a65ed318c3..c565a07106fed7 100644 --- a/ee/app/services/ee/packages/create_package_service.rb +++ b/ee/app/services/ee/packages/create_package_service.rb @@ -5,11 +5,30 @@ module Packages module CreatePackageService extend ::Gitlab::Utils::Override - private + protected - override :send_audit_event - def send_audit_event(package) - ::Packages::CreateAuditEventService.new(package, current_user:).execute + override :find_or_create_package! + def find_or_create_package!(package_type, name: params[:name], version: params[:version]) + super.tap do |package| + user = current_user + + package.run_after_commit_or_now do + if package.previously_new_record? + ::Packages::CreateAuditEventService.new(package, current_user: user).execute + end + end + end + end + + override :create_package! + def create_package!(package_type, attrs = {}) + super.tap do |package| + user = current_user + + package.run_after_commit_or_now do + ::Packages::CreateAuditEventService.new(package, current_user: user).execute + end + end end end end diff --git a/ee/app/services/ee/packages/debian/process_package_file_service.rb b/ee/app/services/ee/packages/debian/process_package_file_service.rb index 761249e8fbe048..5ae41f06783e04 100644 --- a/ee/app/services/ee/packages/debian/process_package_file_service.rb +++ b/ee/app/services/ee/packages/debian/process_package_file_service.rb @@ -6,11 +6,11 @@ module Debian module ProcessPackageFileService extend ::Gitlab::Utils::Override - private - - override :send_audit_event - def send_audit_event - ::Packages::CreateAuditEventService.new(package).execute + override :execute + def execute + super.tap do |response| + ::Packages::CreateAuditEventService.new(package).execute if response&.success? + end end end end diff --git a/ee/app/services/ee/packages/helm/process_file_service.rb b/ee/app/services/ee/packages/helm/process_file_service.rb index cc9e873aa9b50f..0d48efe1206559 100644 --- a/ee/app/services/ee/packages/helm/process_file_service.rb +++ b/ee/app/services/ee/packages/helm/process_file_service.rb @@ -6,11 +6,11 @@ module Helm module ProcessFileService extend ::Gitlab::Utils::Override - private - - override :send_audit_event - def send_audit_event - ::Packages::CreateAuditEventService.new(package).execute + override :execute + def execute + super.tap do |response| + ::Packages::CreateAuditEventService.new(package).execute if response&.success? + end end end end diff --git a/ee/app/services/ee/packages/npm/process_package_file_service.rb b/ee/app/services/ee/packages/npm/process_package_file_service.rb index f5a1f983c1fdad..4ebba81159bbb2 100644 --- a/ee/app/services/ee/packages/npm/process_package_file_service.rb +++ b/ee/app/services/ee/packages/npm/process_package_file_service.rb @@ -6,11 +6,11 @@ module Npm module ProcessPackageFileService extend ::Gitlab::Utils::Override - private - - override :send_audit_event - def send_audit_event - ::Packages::CreateAuditEventService.new(package).execute + override :execute + def execute + super.tap do |response| + ::Packages::CreateAuditEventService.new(package).execute if response.success? + end end end end diff --git a/ee/app/services/ee/packages/nuget/update_package_from_metadata_service.rb b/ee/app/services/ee/packages/nuget/update_package_from_metadata_service.rb index 72dfd05a49221e..24f376fb266784 100644 --- a/ee/app/services/ee/packages/nuget/update_package_from_metadata_service.rb +++ b/ee/app/services/ee/packages/nuget/update_package_from_metadata_service.rb @@ -8,9 +8,13 @@ module UpdatePackageFromMetadataService private - override :send_audit_event - def send_audit_event - ::Packages::CreateAuditEventService.new(package).execute + override :update_linked_package + def update_linked_package + super.tap do |package| + package.run_after_commit do + ::Packages::CreateAuditEventService.new(package).execute if package.default? + end + end end end end diff --git a/ee/app/services/ee/packages/rubygems/process_gem_service.rb b/ee/app/services/ee/packages/rubygems/process_gem_service.rb index 9ee8919923d146..7159a57aebbd0e 100644 --- a/ee/app/services/ee/packages/rubygems/process_gem_service.rb +++ b/ee/app/services/ee/packages/rubygems/process_gem_service.rb @@ -6,11 +6,11 @@ module Rubygems module ProcessGemService extend ::Gitlab::Utils::Override - private - - override :send_audit_event - def send_audit_event - ::Packages::CreateAuditEventService.new(package).execute + override :execute + def execute + super.tap do |response| + ::Packages::CreateAuditEventService.new(package).execute if response.success? + end end end end diff --git a/ee/spec/services/ee/packages/create_package_service_spec.rb b/ee/spec/services/ee/packages/create_package_service_spec.rb index d8a328cf8a1f6c..9ed7a6d3ffea4d 100644 --- a/ee/spec/services/ee/packages/create_package_service_spec.rb +++ b/ee/spec/services/ee/packages/create_package_service_spec.rb @@ -26,7 +26,7 @@ def execute create(:maven_package, project: project, name: params[:name], version: params[:version]) end - it 'does not call the CreateAuditEventService' do + it 'does not execute the CreateAuditEventService' do expect(::Packages::CreateAuditEventService).not_to receive(:new) execute @@ -34,10 +34,12 @@ def execute end context 'when package is created' do - it 'calls the CreateAuditEventService' do - expect(::Packages::CreateAuditEventService).to receive(:new) - .with(instance_of(Packages::Package), current_user: user) - .and_call_original + it 'executes the CreateAuditEventService' do + expect_next_instance_of( + ::Packages::CreateAuditEventService, instance_of(Packages::Package), current_user: user + ) do |service| + expect(service).to receive(:execute) + end execute end @@ -53,10 +55,12 @@ def execute end end - it 'calls the CreateAuditEventService' do - expect(::Packages::CreateAuditEventService).to receive(:new) - .with(instance_of(Packages::Package), current_user: user) - .and_call_original + it 'executes the CreateAuditEventService' do + expect_next_instance_of( + ::Packages::CreateAuditEventService, instance_of(Packages::Package), current_user: user + ) do |service| + expect(service).to receive(:execute) + end execute end diff --git a/ee/spec/services/ee/packages/debian/process_package_file_service_spec.rb b/ee/spec/services/ee/packages/debian/process_package_file_service_spec.rb index c415b78d1e81eb..53fbace4510f39 100644 --- a/ee/spec/services/ee/packages/debian/process_package_file_service_spec.rb +++ b/ee/spec/services/ee/packages/debian/process_package_file_service_spec.rb @@ -12,8 +12,10 @@ let(:service) { described_class.new(package_file, distribution_name, component_name) } describe '#execute' do - it 'calls the CreateAuditEventService' do - expect(::Packages::CreateAuditEventService).to receive(:new).with(package).and_call_original + it 'executes the CreateAuditEventService' do + expect_next_instance_of(::Packages::CreateAuditEventService, package) do |service| + expect(service).to receive(:execute) + end service.execute end diff --git a/ee/spec/services/ee/packages/helm/process_file_service_spec.rb b/ee/spec/services/ee/packages/helm/process_file_service_spec.rb index 8f500bd9f67528..e8a5d499d5dc9a 100644 --- a/ee/spec/services/ee/packages/helm/process_file_service_spec.rb +++ b/ee/spec/services/ee/packages/helm/process_file_service_spec.rb @@ -10,8 +10,10 @@ let(:service) { described_class.new(channel, package_file) } describe '#execute' do - it 'calls the CreateAuditEventService' do - expect(::Packages::CreateAuditEventService).to receive(:new).with(package).and_call_original + it 'executes the CreateAuditEventService' do + expect_next_instance_of(::Packages::CreateAuditEventService, package) do |service| + expect(service).to receive(:execute) + end service.execute end diff --git a/ee/spec/services/ee/packages/npm/process_package_file_service_spec.rb b/ee/spec/services/ee/packages/npm/process_package_file_service_spec.rb index b111000a843267..452d3270ff7757 100644 --- a/ee/spec/services/ee/packages/npm/process_package_file_service_spec.rb +++ b/ee/spec/services/ee/packages/npm/process_package_file_service_spec.rb @@ -12,14 +12,17 @@ describe '#execute' do before do - allow_next_instance_of(::Packages::Npm::CheckManifestCoherenceService, package, - instance_of(Gem::Package::TarReader::Entry)) do |service| - allow(service).to receive(:execute).and_return(ServiceResponse.success) - end + allow_next_instance_of( + ::Packages::Npm::CheckManifestCoherenceService, package, instance_of(Gem::Package::TarReader::Entry) + ) do |service| + allow(service).to receive(:execute).and_return(ServiceResponse.success) + end end - it 'calls the CreateAuditEventService' do - expect(::Packages::CreateAuditEventService).to receive(:new).with(package).and_call_original + it 'executes the CreateAuditEventService' do + expect_next_instance_of(::Packages::CreateAuditEventService, package) do |service| + expect(service).to receive(:execute) + end service.execute end diff --git a/ee/spec/services/ee/packages/nuget/update_package_from_metadata_service_spec.rb b/ee/spec/services/ee/packages/nuget/update_package_from_metadata_service_spec.rb index 423d925eafd25c..7dffce1be6f941 100644 --- a/ee/spec/services/ee/packages/nuget/update_package_from_metadata_service_spec.rb +++ b/ee/spec/services/ee/packages/nuget/update_package_from_metadata_service_spec.rb @@ -5,13 +5,15 @@ RSpec.describe Packages::Nuget::UpdatePackageFromMetadataService, feature_category: :package_registry do let_it_be(:package) { create(:nuget_package, :processing) } let(:package_file) { package.reload.package_files.first } - let(:package_zip_file) { Zip::File.new(package_file.file) } # rubocop:disable Performance/Rubyzip -- for testing purpose only + let(:package_zip_file) { Zip::File.new(package_file.file) } # rubocop:disable Performance/Rubyzip -- only for testing purposes let(:service) { described_class.new(package_file, package_zip_file) } describe '#execute' do - it 'calls the CreateAuditEventService' do - expect(::Packages::CreateAuditEventService).to receive(:new).with(package).and_call_original + it 'executes the CreateAuditEventService' do + expect_next_instance_of(::Packages::CreateAuditEventService, package) do |service| + expect(service).to receive(:execute) + end service.execute end diff --git a/ee/spec/services/ee/packages/rubygems/process_gem_service_spec.rb b/ee/spec/services/ee/packages/rubygems/process_gem_service_spec.rb index c5c25a8077de9d..b92d1e65a09373 100644 --- a/ee/spec/services/ee/packages/rubygems/process_gem_service_spec.rb +++ b/ee/spec/services/ee/packages/rubygems/process_gem_service_spec.rb @@ -4,13 +4,15 @@ RSpec.describe Packages::Rubygems::ProcessGemService, feature_category: :package_registry do let_it_be(:package) { create(:rubygems_package, :processing) } + let_it_be(:package_file) { create(:package_file, :unprocessed_gem, package: package) } - let(:package_file) { create(:package_file, :unprocessed_gem, package: package) } let(:service) { described_class.new(package_file) } describe '#execute' do - it 'calls the CreateAuditEventService' do - expect(::Packages::CreateAuditEventService).to receive(:new).with(package).and_call_original + it 'executes the CreateAuditEventService' do + expect_next_instance_of(::Packages::CreateAuditEventService, package) do |service| + expect(service).to receive(:execute) + end service.execute end diff --git a/spec/models/packages/package_spec.rb b/spec/models/packages/package_spec.rb index e8a209a72875c7..ae9db783e91b17 100644 --- a/spec/models/packages/package_spec.rb +++ b/spec/models/packages/package_spec.rb @@ -9,6 +9,7 @@ it_behaves_like 'having unique enum values' it { is_expected.to be_a Packages::Downloadable } + it { is_expected.to be_a AfterCommitQueue } describe 'relationships' do it { is_expected.to belong_to(:project) } diff --git a/spec/services/packages/debian/process_package_file_service_spec.rb b/spec/services/packages/debian/process_package_file_service_spec.rb index 5176b0681902cc..a7bcc5a19e9b08 100644 --- a/spec/services/packages/debian/process_package_file_service_spec.rb +++ b/spec/services/packages/debian/process_package_file_service_spec.rb @@ -114,6 +114,7 @@ .and change { package.publication }.from(nil) .and change { debian_file_metadatum.file_type }.from('unknown').to('changes') .and not_change { debian_file_metadatum.component } + expect(subject).to be_success end end @@ -132,6 +133,7 @@ .and change { package.publication }.from(nil) .and change { debian_file_metadatum.file_type }.from('unknown').to(expected_file_type) .and change { debian_file_metadatum.component }.from(nil).to(component_name) + expect(subject).to be_success end end @@ -248,6 +250,7 @@ expect { package.reload } .to raise_error(ActiveRecord::RecordNotFound) + expect(subject).to be_success end end @@ -359,6 +362,8 @@ expect { package.reload } .to raise_error(ActiveRecord::RecordNotFound) + + expect(subject).to be_success end end diff --git a/spec/services/packages/helm/process_file_service_spec.rb b/spec/services/packages/helm/process_file_service_spec.rb index a1f53e8756c08f..5ba838d3771327 100644 --- a/spec/services/packages/helm/process_file_service_spec.rb +++ b/spec/services/packages/helm/process_file_service_spec.rb @@ -19,6 +19,8 @@ end describe '#execute' do + include ExclusiveLeaseHelpers + subject(:execute) { service.execute } context 'without a file' do @@ -52,6 +54,7 @@ expect(package_file.helm_file_metadatum.channel).to eq(channel) expect(package_file.helm_file_metadatum.metadata).to eq(expected) + expect(execute).to be_success end context 'marked as pending_destruction' do @@ -82,6 +85,7 @@ expect(package_file.helm_file_metadatum.channel).to eq(channel) expect(package_file.helm_file_metadatum.metadata).to eq(expected) + expect(execute).to be_success end end @@ -116,5 +120,23 @@ it { expect { execute }.to raise_error(Packages::Helm::ExtractFileMetadataService::ExtractionError, 'Error while parsing Chart.yaml: (): did not find expected node content while parsing a flow node at line 2 column 1') } end + + context 'with exclusive lease guard' do + let(:lease_key) { service.send(:lease_key) } + + it 'obtains a lease to create or update the package' do + expect_to_obtain_exclusive_lease(lease_key) + + execute + end + + context 'when the lease is already taken' do + before do + stub_exclusive_lease_taken(lease_key) + end + + specify { expect(execute).to be_nil } + end + end end end -- GitLab From 72381925c33aefb007fe979459f43dd6498a8d6f Mon Sep 17 00:00:00 2001 From: moaz-khalifa Date: Fri, 24 Jan 2025 20:20:34 +0100 Subject: [PATCH 5/6] Use model callback instead of services to send audit events --- app/models/packages/package.rb | 3 +- .../packages/create_package_service.rb | 2 - .../debian/process_package_file_service.rb | 4 - .../packages/helm/process_file_service.rb | 4 - .../npm/process_package_file_service.rb | 2 - .../nuget/create_or_update_package_service.rb | 11 ++- .../update_package_from_metadata_service.rb | 14 ++- .../packages/rubygems/process_gem_service.rb | 2 - ee/app/models/ee/packages/package.rb | 23 +++++ .../ee/packages/create_package_service.rb | 35 -------- .../debian/process_package_file_service.rb | 18 ---- .../ee/packages/helm/process_file_service.rb | 18 ---- .../npm/process_package_file_service.rb | 18 ---- .../update_package_from_metadata_service.rb | 22 ----- .../packages/rubygems/process_gem_service.rb | 18 ---- .../packages/create_audit_event_service.rb | 33 +++---- ee/spec/models/ee/packages/package_spec.rb | 89 +++++++++++++++++++ .../packages/create_package_service_spec.rb | 69 -------------- .../process_package_file_service_spec.rb | 23 ----- .../helm/process_file_service_spec.rb | 21 ----- .../npm/process_package_file_service_spec.rb | 30 ------- ...date_package_from_metadata_service_spec.rb | 21 ----- .../rubygems/process_gem_service_spec.rb | 20 ----- .../create_audit_event_service_spec.rb | 65 +++++--------- spec/models/packages/package_spec.rb | 1 - .../ml/destroy_model_version_service_spec.rb | 2 +- .../process_package_file_service_spec.rb | 5 -- .../helm/process_file_service_spec.rb | 22 ----- 28 files changed, 157 insertions(+), 438 deletions(-) create mode 100644 ee/app/models/ee/packages/package.rb delete mode 100644 ee/app/services/ee/packages/create_package_service.rb delete mode 100644 ee/app/services/ee/packages/debian/process_package_file_service.rb delete mode 100644 ee/app/services/ee/packages/helm/process_file_service.rb delete mode 100644 ee/app/services/ee/packages/npm/process_package_file_service.rb delete mode 100644 ee/app/services/ee/packages/nuget/update_package_from_metadata_service.rb delete mode 100644 ee/app/services/ee/packages/rubygems/process_gem_service.rb create mode 100644 ee/spec/models/ee/packages/package_spec.rb delete mode 100644 ee/spec/services/ee/packages/create_package_service_spec.rb delete mode 100644 ee/spec/services/ee/packages/debian/process_package_file_service_spec.rb delete mode 100644 ee/spec/services/ee/packages/helm/process_file_service_spec.rb delete mode 100644 ee/spec/services/ee/packages/npm/process_package_file_service_spec.rb delete mode 100644 ee/spec/services/ee/packages/nuget/update_package_from_metadata_service_spec.rb delete mode 100644 ee/spec/services/ee/packages/rubygems/process_gem_service_spec.rb diff --git a/app/models/packages/package.rb b/app/models/packages/package.rb index f3b3ec54359c2a..01a1cf5cd2e828 100644 --- a/app/models/packages/package.rb +++ b/app/models/packages/package.rb @@ -8,7 +8,6 @@ class Packages::Package < ApplicationRecord include Packages::Installable include Packages::Downloadable include EnumInheritance - include AfterCommitQueue DISPLAYABLE_STATUSES = [:default, :error, :deprecated].freeze INSTALLABLE_STATUSES = [:default, :hidden, :deprecated].freeze @@ -310,3 +309,5 @@ def prevent_concurrent_inserts connection.execute("SELECT pg_advisory_xact_lock(#{lock_expression})") end end + +Packages::Package.prepend_mod diff --git a/app/services/packages/create_package_service.rb b/app/services/packages/create_package_service.rb index 15e5b6e5672ebb..792c51232b5ae8 100644 --- a/app/services/packages/create_package_service.rb +++ b/app/services/packages/create_package_service.rb @@ -69,5 +69,3 @@ def add_build_info(package) end end end - -Packages::CreatePackageService.prepend_mod diff --git a/app/services/packages/debian/process_package_file_service.rb b/app/services/packages/debian/process_package_file_service.rb index fbe05d1fee214e..a1bcc0a6f7338c 100644 --- a/app/services/packages/debian/process_package_file_service.rb +++ b/app/services/packages/debian/process_package_file_service.rb @@ -33,8 +33,6 @@ def execute end ::Packages::Debian::GenerateDistributionWorker.perform_async(:project, package.distribution.id) - - ServiceResponse.success end end @@ -212,5 +210,3 @@ def lease_timeout end end end - -Packages::Debian::ProcessPackageFileService.prepend_mod diff --git a/app/services/packages/helm/process_file_service.rb b/app/services/packages/helm/process_file_service.rb index c28e585a996131..219f3d8c781129 100644 --- a/app/services/packages/helm/process_file_service.rb +++ b/app/services/packages/helm/process_file_service.rb @@ -23,8 +23,6 @@ def execute rename_package_file_and_set_metadata cleanup_temp_package end - - ServiceResponse.success end end @@ -95,5 +93,3 @@ def lease_timeout end end end - -Packages::Helm::ProcessFileService.prepend_mod diff --git a/app/services/packages/npm/process_package_file_service.rb b/app/services/packages/npm/process_package_file_service.rb index 0a87b0f9395052..0cff80a82c9998 100644 --- a/app/services/packages/npm/process_package_file_service.rb +++ b/app/services/packages/npm/process_package_file_service.rb @@ -73,5 +73,3 @@ def path_for(entry) end end end - -Packages::Npm::ProcessPackageFileService.prepend_mod diff --git a/app/services/packages/nuget/create_or_update_package_service.rb b/app/services/packages/nuget/create_or_update_package_service.rb index 73adbab6204c49..de01757c25ce2e 100644 --- a/app/services/packages/nuget/create_or_update_package_service.rb +++ b/app/services/packages/nuget/create_or_update_package_service.rb @@ -2,7 +2,7 @@ module Packages module Nuget - class CreateOrUpdatePackageService < ::Packages::CreatePackageService + class CreateOrUpdatePackageService < BaseService include ::Gitlab::Utils::StrongMemoize include ExclusiveLeaseGuard @@ -77,8 +77,6 @@ def update_tags end def create_build_infos - return unless existing_package - target_package.create_build_infos!(params[:build]) end @@ -96,7 +94,12 @@ def target_package strong_memoize_attr :target_package def create_new_package - create_package!(:nuget, name: metadata[:package_name], version: metadata[:package_version]) + ::Packages::Nuget::Package.create!( + name: metadata[:package_name], + version: metadata[:package_version], + project: project, + creator: current_user.is_a?(User) ? current_user : nil + ) end def package_filename diff --git a/app/services/packages/nuget/update_package_from_metadata_service.rb b/app/services/packages/nuget/update_package_from_metadata_service.rb index 285141a7525173..811b4355f1047e 100644 --- a/app/services/packages/nuget/update_package_from_metadata_service.rb +++ b/app/services/packages/nuget/update_package_from_metadata_service.rb @@ -15,8 +15,6 @@ class UpdatePackageFromMetadataService InvalidMetadataError = ZipError = DuplicatePackageError = Class.new(StandardError) - delegate :package, to: :@package_file, private: true - def initialize(package_file, package_zip_file) @package_file = package_file @package_zip_file = package_zip_file @@ -45,12 +43,12 @@ def execute def process_package_update package_to_destroy = nil - target_package = package + target_package = @package_file.package if existing_package raise DuplicatePackageError, "A package '#{package_name}' with version '#{package_version}' already exists" unless symbol_package? || duplicates_allowed? - package_to_destroy = package + package_to_destroy = @package_file.package target_package = existing_package else if symbol_package? @@ -103,15 +101,15 @@ def valid_metadata? end def update_linked_package - package.update!( + @package_file.package.update!( name: package_name, version: package_version, status: :default ) - ::Packages::Nuget::CreateDependencyService.new(package, package_dependencies) + ::Packages::Nuget::CreateDependencyService.new(@package_file.package, package_dependencies) .execute - package + @package_file.package end def existing_package @@ -182,5 +180,3 @@ def lease_timeout end end end - -Packages::Nuget::UpdatePackageFromMetadataService.prepend_mod diff --git a/app/services/packages/rubygems/process_gem_service.rb b/app/services/packages/rubygems/process_gem_service.rb index 2ed975f63121f3..07545495f1bf8d 100644 --- a/app/services/packages/rubygems/process_gem_service.rb +++ b/app/services/packages/rubygems/process_gem_service.rb @@ -123,5 +123,3 @@ def lease_timeout end end end - -Packages::Rubygems::ProcessGemService.prepend_mod diff --git a/ee/app/models/ee/packages/package.rb b/ee/app/models/ee/packages/package.rb new file mode 100644 index 00000000000000..06dac560f23f94 --- /dev/null +++ b/ee/app/models/ee/packages/package.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module EE + module Packages + module Package + extend ActiveSupport::Concern + + prepended do + after_commit :create_audit_event, on: %i[create update] + + private + + def create_audit_event + return unless default? + return unless previously_new_record? || saved_change_to_status? + return if saved_change_to_status? && saved_change_to_status != self.class.statuses.invert.values_at(2, 0) + + ::Packages::CreateAuditEventService.new(self).execute + end + end + end + end +end diff --git a/ee/app/services/ee/packages/create_package_service.rb b/ee/app/services/ee/packages/create_package_service.rb deleted file mode 100644 index c565a07106fed7..00000000000000 --- a/ee/app/services/ee/packages/create_package_service.rb +++ /dev/null @@ -1,35 +0,0 @@ -# frozen_string_literal: true - -module EE - module Packages - module CreatePackageService - extend ::Gitlab::Utils::Override - - protected - - override :find_or_create_package! - def find_or_create_package!(package_type, name: params[:name], version: params[:version]) - super.tap do |package| - user = current_user - - package.run_after_commit_or_now do - if package.previously_new_record? - ::Packages::CreateAuditEventService.new(package, current_user: user).execute - end - end - end - end - - override :create_package! - def create_package!(package_type, attrs = {}) - super.tap do |package| - user = current_user - - package.run_after_commit_or_now do - ::Packages::CreateAuditEventService.new(package, current_user: user).execute - end - end - end - end - end -end diff --git a/ee/app/services/ee/packages/debian/process_package_file_service.rb b/ee/app/services/ee/packages/debian/process_package_file_service.rb deleted file mode 100644 index 5ae41f06783e04..00000000000000 --- a/ee/app/services/ee/packages/debian/process_package_file_service.rb +++ /dev/null @@ -1,18 +0,0 @@ -# frozen_string_literal: true - -module EE - module Packages - module Debian - module ProcessPackageFileService - extend ::Gitlab::Utils::Override - - override :execute - def execute - super.tap do |response| - ::Packages::CreateAuditEventService.new(package).execute if response&.success? - end - end - end - end - end -end diff --git a/ee/app/services/ee/packages/helm/process_file_service.rb b/ee/app/services/ee/packages/helm/process_file_service.rb deleted file mode 100644 index 0d48efe1206559..00000000000000 --- a/ee/app/services/ee/packages/helm/process_file_service.rb +++ /dev/null @@ -1,18 +0,0 @@ -# frozen_string_literal: true - -module EE - module Packages - module Helm - module ProcessFileService - extend ::Gitlab::Utils::Override - - override :execute - def execute - super.tap do |response| - ::Packages::CreateAuditEventService.new(package).execute if response&.success? - end - end - end - end - end -end diff --git a/ee/app/services/ee/packages/npm/process_package_file_service.rb b/ee/app/services/ee/packages/npm/process_package_file_service.rb deleted file mode 100644 index 4ebba81159bbb2..00000000000000 --- a/ee/app/services/ee/packages/npm/process_package_file_service.rb +++ /dev/null @@ -1,18 +0,0 @@ -# frozen_string_literal: true - -module EE - module Packages - module Npm - module ProcessPackageFileService - extend ::Gitlab::Utils::Override - - override :execute - def execute - super.tap do |response| - ::Packages::CreateAuditEventService.new(package).execute if response.success? - end - end - end - end - end -end diff --git a/ee/app/services/ee/packages/nuget/update_package_from_metadata_service.rb b/ee/app/services/ee/packages/nuget/update_package_from_metadata_service.rb deleted file mode 100644 index 24f376fb266784..00000000000000 --- a/ee/app/services/ee/packages/nuget/update_package_from_metadata_service.rb +++ /dev/null @@ -1,22 +0,0 @@ -# frozen_string_literal: true - -module EE - module Packages - module Nuget - module UpdatePackageFromMetadataService - extend ::Gitlab::Utils::Override - - private - - override :update_linked_package - def update_linked_package - super.tap do |package| - package.run_after_commit do - ::Packages::CreateAuditEventService.new(package).execute if package.default? - end - end - end - end - end - end -end diff --git a/ee/app/services/ee/packages/rubygems/process_gem_service.rb b/ee/app/services/ee/packages/rubygems/process_gem_service.rb deleted file mode 100644 index 7159a57aebbd0e..00000000000000 --- a/ee/app/services/ee/packages/rubygems/process_gem_service.rb +++ /dev/null @@ -1,18 +0,0 @@ -# frozen_string_literal: true - -module EE - module Packages - module Rubygems - module ProcessGemService - extend ::Gitlab::Utils::Override - - override :execute - def execute - super.tap do |response| - ::Packages::CreateAuditEventService.new(package).execute if response.success? - end - end - end - end - end -end diff --git a/ee/app/services/packages/create_audit_event_service.rb b/ee/app/services/packages/create_audit_event_service.rb index 16dc50e77732ba..2c4386f61d7c5b 100644 --- a/ee/app/services/packages/create_audit_event_service.rb +++ b/ee/app/services/packages/create_audit_event_service.rb @@ -3,19 +3,18 @@ module Packages class CreateAuditEventService FEATURE_FLAG_DISABLED_ERROR = ServiceResponse.error(message: 'Feature flag is not enabled').freeze - INVALID_STATE_ERROR = ServiceResponse.error(message: 'Package is not in the default state').freeze + MVN_VERSIONLESS_PKG_ERROR = ServiceResponse.error(message: "Maven versionless packages aren't audited").freeze - delegate :project, to: :package, private: true + delegate :project, :creator, to: :package, private: true - def initialize(package, current_user: nil, event_name: 'package_registry_package_published') + def initialize(package, event_name: 'package_registry_package_published') @package = package - @current_user = current_user @event_name = event_name end def execute - return FEATURE_FLAG_DISABLED_ERROR if Feature.disabled?(:package_registry_audit_events, package.project) - return INVALID_STATE_ERROR unless package.default? + return FEATURE_FLAG_DISABLED_ERROR if ::Feature.disabled?(:package_registry_audit_events, project) + return MVN_VERSIONLESS_PKG_ERROR if package.maven? && package.version.nil? audit_event = ::Gitlab::Audit::Auditor.audit(audit_context) @@ -24,12 +23,12 @@ def execute private - attr_reader :package, :current_user, :event_name + attr_reader :package, :event_name def audit_context { name: event_name, - author: author, + author: creator || ::Gitlab::Audit::DeployTokenAuthor.new, scope: project.group || project, target: package, target_details: target_details, @@ -38,29 +37,17 @@ def audit_context } end - def author - current_user || package.creator || ::Gitlab::Audit::DeployTokenAuthor.new - end - def target_details "#{project.full_path}/#{package.name}-#{package.version}" end def auth_token_type - ::Current.token_info&.dig(:token_type) || token_type_from_current_user || token_type_from_package_creator - end - - def token_type_from_current_user - return unless current_user - - return 'DeployToken' if current_user.is_a?(DeployToken) - return 'CiJobToken' if current_user.from_ci_job_token? - - 'PersonalAccessToken' + ::Current.token_info&.dig(:token_type) || token_type_from_package_creator end def token_type_from_package_creator - return 'DeployToken' unless package.creator + return 'DeployToken' unless creator + return 'CiJobToken' if creator.from_ci_job_token? 'PersonalAccessToken or CiJobToken' end diff --git a/ee/spec/models/ee/packages/package_spec.rb b/ee/spec/models/ee/packages/package_spec.rb new file mode 100644 index 00000000000000..3d9075afd28820 --- /dev/null +++ b/ee/spec/models/ee/packages/package_spec.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Packages::Package, type: :model, feature_category: :package_registry do + describe '#create_audit_event callback' do + before do + allow(::Packages::CreateAuditEventService).to receive(:new).and_call_original + end + + shared_examples 'creates audit event' do + it 'calls CreateAuditEventService' do + subject + + expect(::Packages::CreateAuditEventService).to have_received(:new).with(package) + end + end + + shared_examples 'does not create audit event' do + it 'does not call CreateAuditEventService' do + subject + + expect(::Packages::CreateAuditEventService).not_to have_received(:new).with(package) + end + end + + context 'on create' do + let(:package) { build(:generic_package) } + + subject { package.save! } + + context 'with default status' do + it_behaves_like 'creates audit event' + end + + context 'with non-default status' do + before do + package.status = :processing + end + + it_behaves_like 'does not create audit event' + end + end + + context 'on update' do + using RSpec::Parameterized::TableSyntax + + let_it_be(:package) { create(:generic_package) } + + context 'when changing status' do + subject { package.public_send(:"#{to_status}!") } + + where(:from_status, :to_status, :creates_event) do + :default | :pending_destruction | false + :default | :hidden | false + :hidden | :default | false + :processing | :default | true + end + + before do + package.public_send(:"#{from_status}!") + end + + with_them do + it_behaves_like params[:creates_event] ? 'creates audit event' : 'does not create audit event' + end + end + + context 'when updating attributes' do + subject { package.update!(updates) } + + where(:scenario, :updates, :initial_status, :creates_event) do + 'name only' | { name: 'new_name' } | :default | false + 'name and default to hidden' | { name: 'new_name', status: :hidden } | :default | false + 'name and processing to default' | { name: 'new_name', status: :default } | :processing | true + 'name and hidden to default' | { name: 'new_name', status: :default } | :hidden | false + end + + before do + package.public_send(:"#{initial_status}!") + end + + with_them do + it_behaves_like params[:creates_event] ? 'creates audit event' : 'does not create audit event' + end + end + end + end +end diff --git a/ee/spec/services/ee/packages/create_package_service_spec.rb b/ee/spec/services/ee/packages/create_package_service_spec.rb deleted file mode 100644 index 9ed7a6d3ffea4d..00000000000000 --- a/ee/spec/services/ee/packages/create_package_service_spec.rb +++ /dev/null @@ -1,69 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Packages::CreatePackageService, feature_category: :package_registry do - let_it_be(:project) { create(:project) } - let_it_be(:user) { create(:user) } - - let(:params) { { name: 'package-name', version: '1.0.0' } } - let(:service) { subclass.new(project, user, params) } - - describe '#execute' do - subject(:execute) { service.execute } - - context 'when calling find_or_create_package!' do - let(:subclass) do - Class.new(described_class) do - def execute - find_or_create_package!(:maven) - end - end - end - - context 'when package is found' do - before do - create(:maven_package, project: project, name: params[:name], version: params[:version]) - end - - it 'does not execute the CreateAuditEventService' do - expect(::Packages::CreateAuditEventService).not_to receive(:new) - - execute - end - end - - context 'when package is created' do - it 'executes the CreateAuditEventService' do - expect_next_instance_of( - ::Packages::CreateAuditEventService, instance_of(Packages::Package), current_user: user - ) do |service| - expect(service).to receive(:execute) - end - - execute - end - end - end - - context 'when calling create_package!' do - let(:subclass) do - Class.new(described_class) do - def execute - create_package!(:maven) - end - end - end - - it 'executes the CreateAuditEventService' do - expect_next_instance_of( - ::Packages::CreateAuditEventService, instance_of(Packages::Package), current_user: user - ) do |service| - expect(service).to receive(:execute) - end - - execute - end - end - end -end diff --git a/ee/spec/services/ee/packages/debian/process_package_file_service_spec.rb b/ee/spec/services/ee/packages/debian/process_package_file_service_spec.rb deleted file mode 100644 index 53fbace4510f39..00000000000000 --- a/ee/spec/services/ee/packages/debian/process_package_file_service_spec.rb +++ /dev/null @@ -1,23 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Packages::Debian::ProcessPackageFileService, feature_category: :package_registry do - let_it_be(:distribution) { create(:debian_project_distribution, :with_file, suite: 'unstable') } - let_it_be(:package) { create(:debian_temporary_with_files, project: distribution.project) } - - let(:package_file) { package.package_files.with_file_name('libsample0_1.2.3~alpha2_amd64.deb').first } - let(:distribution_name) { distribution.codename } - let(:component_name) { 'main' } - let(:service) { described_class.new(package_file, distribution_name, component_name) } - - describe '#execute' do - it 'executes the CreateAuditEventService' do - expect_next_instance_of(::Packages::CreateAuditEventService, package) do |service| - expect(service).to receive(:execute) - end - - service.execute - end - end -end diff --git a/ee/spec/services/ee/packages/helm/process_file_service_spec.rb b/ee/spec/services/ee/packages/helm/process_file_service_spec.rb deleted file mode 100644 index e8a5d499d5dc9a..00000000000000 --- a/ee/spec/services/ee/packages/helm/process_file_service_spec.rb +++ /dev/null @@ -1,21 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Packages::Helm::ProcessFileService, feature_category: :package_registry do - let_it_be(:package) { create(:helm_package, :processing, without_package_files: true) } - let_it_be(:package_file) { create(:helm_package_file, without_loaded_metadatum: true, package: package) } - - let(:channel) { 'stable' } - let(:service) { described_class.new(channel, package_file) } - - describe '#execute' do - it 'executes the CreateAuditEventService' do - expect_next_instance_of(::Packages::CreateAuditEventService, package) do |service| - expect(service).to receive(:execute) - end - - service.execute - end - end -end diff --git a/ee/spec/services/ee/packages/npm/process_package_file_service_spec.rb b/ee/spec/services/ee/packages/npm/process_package_file_service_spec.rb deleted file mode 100644 index 452d3270ff7757..00000000000000 --- a/ee/spec/services/ee/packages/npm/process_package_file_service_spec.rb +++ /dev/null @@ -1,30 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Packages::Npm::ProcessPackageFileService, feature_category: :package_registry do - let_it_be(:package) { build(:npm_package, :processing, id: 1) } - let_it_be(:package_file) do - build(:package_file, :npm, file_fixture: expand_fixture_path('packages/npm/package-1.3.7.tgz'), package: package) - end - - let(:service) { described_class.new(package_file) } - - describe '#execute' do - before do - allow_next_instance_of( - ::Packages::Npm::CheckManifestCoherenceService, package, instance_of(Gem::Package::TarReader::Entry) - ) do |service| - allow(service).to receive(:execute).and_return(ServiceResponse.success) - end - end - - it 'executes the CreateAuditEventService' do - expect_next_instance_of(::Packages::CreateAuditEventService, package) do |service| - expect(service).to receive(:execute) - end - - service.execute - end - end -end diff --git a/ee/spec/services/ee/packages/nuget/update_package_from_metadata_service_spec.rb b/ee/spec/services/ee/packages/nuget/update_package_from_metadata_service_spec.rb deleted file mode 100644 index 7dffce1be6f941..00000000000000 --- a/ee/spec/services/ee/packages/nuget/update_package_from_metadata_service_spec.rb +++ /dev/null @@ -1,21 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Packages::Nuget::UpdatePackageFromMetadataService, feature_category: :package_registry do - let_it_be(:package) { create(:nuget_package, :processing) } - let(:package_file) { package.reload.package_files.first } - let(:package_zip_file) { Zip::File.new(package_file.file) } # rubocop:disable Performance/Rubyzip -- only for testing purposes - - let(:service) { described_class.new(package_file, package_zip_file) } - - describe '#execute' do - it 'executes the CreateAuditEventService' do - expect_next_instance_of(::Packages::CreateAuditEventService, package) do |service| - expect(service).to receive(:execute) - end - - service.execute - end - end -end diff --git a/ee/spec/services/ee/packages/rubygems/process_gem_service_spec.rb b/ee/spec/services/ee/packages/rubygems/process_gem_service_spec.rb deleted file mode 100644 index b92d1e65a09373..00000000000000 --- a/ee/spec/services/ee/packages/rubygems/process_gem_service_spec.rb +++ /dev/null @@ -1,20 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Packages::Rubygems::ProcessGemService, feature_category: :package_registry do - let_it_be(:package) { create(:rubygems_package, :processing) } - let_it_be(:package_file) { create(:package_file, :unprocessed_gem, package: package) } - - let(:service) { described_class.new(package_file) } - - describe '#execute' do - it 'executes the CreateAuditEventService' do - expect_next_instance_of(::Packages::CreateAuditEventService, package) do |service| - expect(service).to receive(:execute) - end - - service.execute - end - end -end diff --git a/ee/spec/services/packages/create_audit_event_service_spec.rb b/ee/spec/services/packages/create_audit_event_service_spec.rb index ab34d934dc70b8..a5e199c50b9fa2 100644 --- a/ee/spec/services/packages/create_audit_event_service_spec.rb +++ b/ee/spec/services/packages/create_audit_event_service_spec.rb @@ -8,8 +8,7 @@ let_it_be(:package) { build_stubbed(:package, project: project, creator: user) } let_it_be(:deploy_token) { build_stubbed(:deploy_token) } - let(:current_user) { user } - let(:service) { described_class.new(package, current_user: current_user) } + let(:service) { described_class.new(package) } describe '#execute' do subject(:execute) { service.execute } @@ -17,21 +16,21 @@ include_examples 'audit event logging' do let(:operation) { execute } let(:event_type) { 'package_registry_package_published' } - let(:fail_condition!) { allow(package).to receive(:default?).and_return(false) } + let(:fail_condition!) { allow(package).to receive_messages(maven?: true, version: nil) } let(:attributes) do { - author_id: current_user.id, + author_id: user.id, entity_id: project.group.id, entity_type: 'Group', details: { - author_name: current_user.name, + author_name: user.name, event_name: 'package_registry_package_published', target_id: package.id, target_type: package.class.name, target_details: "#{project.full_path}/#{package.name}-#{package.version}", author_class: user.class.name, custom_message: "#{package.package_type} package published", - auth_token_type: 'PersonalAccessToken' + auth_token_type: 'PersonalAccessToken or CiJobToken' } } end @@ -74,45 +73,24 @@ end end - context 'with current_user' do - it 'sets auth_token_type as PersonalAccessToken' do - expect(::Gitlab::Audit::Auditor).to receive(:audit).with( - hash_including(additional_details: { auth_token_type: 'PersonalAccessToken' }) - ) - - execute - end - - context 'when current user is a deploy token' do - let(:current_user) { deploy_token } - - it 'sets auth_token_type as DeployToken' do - expect(::Gitlab::Audit::Auditor).to receive(:audit).with( - hash_including(additional_details: { auth_token_type: 'DeployToken' }) - ) - - execute - end + context 'when package has no creator' do + before do + allow(package).to receive(:creator).and_return(nil) end - context 'when current user is a CiJobToken' do - before do - allow(current_user).to receive(:from_ci_job_token?).and_return(true) - end - - it 'sets auth_token_type as CiJobToken' do - expect(::Gitlab::Audit::Auditor).to receive(:audit).with( - hash_including(additional_details: { auth_token_type: 'CiJobToken' }) + it 'uses DeployTokenAuthor as author' do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with( + hash_including( + author: kind_of(::Gitlab::Audit::DeployTokenAuthor), + additional_details: { auth_token_type: 'DeployToken' } ) + ) - execute - end + execute end end - context 'without current_user' do - let(:current_user) { nil } - + context 'when package has a creator' do it 'sets auth_token_type as PersonalAccessToken or CiJobToken' do expect(::Gitlab::Audit::Auditor).to receive(:audit).with( hash_including(additional_details: { auth_token_type: 'PersonalAccessToken or CiJobToken' }) @@ -121,17 +99,14 @@ execute end - context 'when package has no creator' do + context 'when user is from ci job token' do before do - allow(package).to receive(:creator).and_return(nil) + allow(user).to receive(:from_ci_job_token?).and_return(true) end - it 'uses DeployTokenAuthor as author' do + it 'sets auth_token_type as CiJobToken' do expect(::Gitlab::Audit::Auditor).to receive(:audit).with( - hash_including( - author: kind_of(::Gitlab::Audit::DeployTokenAuthor), - additional_details: { auth_token_type: 'DeployToken' } - ) + hash_including(additional_details: { auth_token_type: 'CiJobToken' }) ) execute diff --git a/spec/models/packages/package_spec.rb b/spec/models/packages/package_spec.rb index ae9db783e91b17..e8a209a72875c7 100644 --- a/spec/models/packages/package_spec.rb +++ b/spec/models/packages/package_spec.rb @@ -9,7 +9,6 @@ it_behaves_like 'having unique enum values' it { is_expected.to be_a Packages::Downloadable } - it { is_expected.to be_a AfterCommitQueue } describe 'relationships' do it { is_expected.to belong_to(:project) } diff --git a/spec/services/ml/destroy_model_version_service_spec.rb b/spec/services/ml/destroy_model_version_service_spec.rb index f384eb81761ca3..9a207504916051 100644 --- a/spec/services/ml/destroy_model_version_service_spec.rb +++ b/spec/services/ml/destroy_model_version_service_spec.rb @@ -53,7 +53,7 @@ it 'does not delete the model version' do is_expected.to be_error.and have_attributes(message: "You don't have access to this package") expect(Ml::ModelVersion.find_by(id: model_version.id)).to eq(model_version) - expect(Gitlab::Audit::Auditor).not_to have_received(:audit) + expect(Gitlab::Audit::Auditor).not_to have_received(:audit).with(audit_event) end end end diff --git a/spec/services/packages/debian/process_package_file_service_spec.rb b/spec/services/packages/debian/process_package_file_service_spec.rb index a7bcc5a19e9b08..5176b0681902cc 100644 --- a/spec/services/packages/debian/process_package_file_service_spec.rb +++ b/spec/services/packages/debian/process_package_file_service_spec.rb @@ -114,7 +114,6 @@ .and change { package.publication }.from(nil) .and change { debian_file_metadatum.file_type }.from('unknown').to('changes') .and not_change { debian_file_metadatum.component } - expect(subject).to be_success end end @@ -133,7 +132,6 @@ .and change { package.publication }.from(nil) .and change { debian_file_metadatum.file_type }.from('unknown').to(expected_file_type) .and change { debian_file_metadatum.component }.from(nil).to(component_name) - expect(subject).to be_success end end @@ -250,7 +248,6 @@ expect { package.reload } .to raise_error(ActiveRecord::RecordNotFound) - expect(subject).to be_success end end @@ -362,8 +359,6 @@ expect { package.reload } .to raise_error(ActiveRecord::RecordNotFound) - - expect(subject).to be_success end end diff --git a/spec/services/packages/helm/process_file_service_spec.rb b/spec/services/packages/helm/process_file_service_spec.rb index 5ba838d3771327..a1f53e8756c08f 100644 --- a/spec/services/packages/helm/process_file_service_spec.rb +++ b/spec/services/packages/helm/process_file_service_spec.rb @@ -19,8 +19,6 @@ end describe '#execute' do - include ExclusiveLeaseHelpers - subject(:execute) { service.execute } context 'without a file' do @@ -54,7 +52,6 @@ expect(package_file.helm_file_metadatum.channel).to eq(channel) expect(package_file.helm_file_metadatum.metadata).to eq(expected) - expect(execute).to be_success end context 'marked as pending_destruction' do @@ -85,7 +82,6 @@ expect(package_file.helm_file_metadatum.channel).to eq(channel) expect(package_file.helm_file_metadatum.metadata).to eq(expected) - expect(execute).to be_success end end @@ -120,23 +116,5 @@ it { expect { execute }.to raise_error(Packages::Helm::ExtractFileMetadataService::ExtractionError, 'Error while parsing Chart.yaml: (): did not find expected node content while parsing a flow node at line 2 column 1') } end - - context 'with exclusive lease guard' do - let(:lease_key) { service.send(:lease_key) } - - it 'obtains a lease to create or update the package' do - expect_to_obtain_exclusive_lease(lease_key) - - execute - end - - context 'when the lease is already taken' do - before do - stub_exclusive_lease_taken(lease_key) - end - - specify { expect(execute).to be_nil } - end - end end end -- GitLab From b477fe4f69ecbad1ffdcd65ce707ed7434f8a107 Mon Sep 17 00:00:00 2001 From: moaz-khalifa Date: Tue, 28 Jan 2025 15:57:59 +0100 Subject: [PATCH 6/6] Address maintainer feedback --- ee/app/models/ee/packages/package.rb | 5 ++++- .../packages/create_audit_event_service.rb | 8 +++----- ee/spec/models/ee/packages/package_spec.rb | 6 ++++++ .../packages/create_audit_event_service_spec.rb | 14 +++----------- 4 files changed, 16 insertions(+), 17 deletions(-) diff --git a/ee/app/models/ee/packages/package.rb b/ee/app/models/ee/packages/package.rb index 06dac560f23f94..ec4bcfd25b63a5 100644 --- a/ee/app/models/ee/packages/package.rb +++ b/ee/app/models/ee/packages/package.rb @@ -5,6 +5,8 @@ module Packages module Package extend ActiveSupport::Concern + PROCESSING_TO_DEFAULT = ::Packages::Package.statuses.invert.values_at(2, 0).freeze + prepended do after_commit :create_audit_event, on: %i[create update] @@ -12,8 +14,9 @@ module Package def create_audit_event return unless default? + return if maven? && version.nil? return unless previously_new_record? || saved_change_to_status? - return if saved_change_to_status? && saved_change_to_status != self.class.statuses.invert.values_at(2, 0) + return if saved_change_to_status? && saved_change_to_status != PROCESSING_TO_DEFAULT ::Packages::CreateAuditEventService.new(self).execute end diff --git a/ee/app/services/packages/create_audit_event_service.rb b/ee/app/services/packages/create_audit_event_service.rb index 2c4386f61d7c5b..6923081ce3d64d 100644 --- a/ee/app/services/packages/create_audit_event_service.rb +++ b/ee/app/services/packages/create_audit_event_service.rb @@ -3,7 +3,6 @@ module Packages class CreateAuditEventService FEATURE_FLAG_DISABLED_ERROR = ServiceResponse.error(message: 'Feature flag is not enabled').freeze - MVN_VERSIONLESS_PKG_ERROR = ServiceResponse.error(message: "Maven versionless packages aren't audited").freeze delegate :project, :creator, to: :package, private: true @@ -14,11 +13,10 @@ def initialize(package, event_name: 'package_registry_package_published') def execute return FEATURE_FLAG_DISABLED_ERROR if ::Feature.disabled?(:package_registry_audit_events, project) - return MVN_VERSIONLESS_PKG_ERROR if package.maven? && package.version.nil? - audit_event = ::Gitlab::Audit::Auditor.audit(audit_context) + ::Gitlab::Audit::Auditor.audit(audit_context) - ServiceResponse.success(payload: { audit_event: }) + ServiceResponse.success end private @@ -32,7 +30,7 @@ def audit_context scope: project.group || project, target: package, target_details: target_details, - message: "#{package.package_type} package published", + message: "#{package.package_type.humanize} package published", additional_details: { auth_token_type: } } end diff --git a/ee/spec/models/ee/packages/package_spec.rb b/ee/spec/models/ee/packages/package_spec.rb index 3d9075afd28820..ec14970028a0c7 100644 --- a/ee/spec/models/ee/packages/package_spec.rb +++ b/ee/spec/models/ee/packages/package_spec.rb @@ -40,6 +40,12 @@ it_behaves_like 'does not create audit event' end + + context 'with versionless maven package' do + subject { create(:maven_package, version: nil) } + + it_behaves_like 'does not create audit event' + end end context 'on update' do diff --git a/ee/spec/services/packages/create_audit_event_service_spec.rb b/ee/spec/services/packages/create_audit_event_service_spec.rb index a5e199c50b9fa2..29ef77c3b865df 100644 --- a/ee/spec/services/packages/create_audit_event_service_spec.rb +++ b/ee/spec/services/packages/create_audit_event_service_spec.rb @@ -5,7 +5,7 @@ RSpec.describe Packages::CreateAuditEventService, feature_category: :package_registry do let_it_be(:project) { build_stubbed(:project, group: build_stubbed(:group)) } let_it_be(:user) { build_stubbed(:user) } - let_it_be(:package) { build_stubbed(:package, project: project, creator: user) } + let_it_be(:package) { build_stubbed(:generic_package, project: project, creator: user) } let_it_be(:deploy_token) { build_stubbed(:deploy_token) } let(:service) { described_class.new(package) } @@ -16,7 +16,7 @@ include_examples 'audit event logging' do let(:operation) { execute } let(:event_type) { 'package_registry_package_published' } - let(:fail_condition!) { allow(package).to receive_messages(maven?: true, version: nil) } + let(:fail_condition!) { stub_feature_flags(package_registry_audit_events: false) } let(:attributes) do { author_id: user.id, @@ -29,21 +29,13 @@ target_type: package.class.name, target_details: "#{project.full_path}/#{package.name}-#{package.version}", author_class: user.class.name, - custom_message: "#{package.package_type} package published", + custom_message: "#{package.package_type.humanize} package published", auth_token_type: 'PersonalAccessToken or CiJobToken' } } end end - context 'when feature flag is disabled' do - before do - stub_feature_flags(package_registry_audit_events: false) - end - - it { is_expected.to eq(described_class::FEATURE_FLAG_DISABLED_ERROR) } - end - context 'when project does not belong to a group' do before do project.group = nil -- GitLab