From cb846a97fc8675bf187ea0a610a3925f84922642 Mon Sep 17 00:00:00 2001 From: Ash McKenzie Date: Wed, 14 Aug 2024 16:20:05 +1000 Subject: [PATCH 1/4] Streamline #execute logic --- .../projects/update_mirror_service.rb | 41 +++++++++---------- .../projects/update_mirror_service_spec.rb | 18 +++----- 2 files changed, 25 insertions(+), 34 deletions(-) diff --git a/ee/app/services/projects/update_mirror_service.rb b/ee/app/services/projects/update_mirror_service.rb index cb04452640c9e2..805aa0bce406e1 100644 --- a/ee/app/services/projects/update_mirror_service.rb +++ b/ee/app/services/projects/update_mirror_service.rb @@ -8,24 +8,11 @@ class UpdateMirrorService < BaseService UpdateError = Class.new(Error) def execute - if project.import_url && - Gitlab::HTTP_V2::UrlBlocker.blocked_url?( - normalized_url(project.import_url), - schemes: Project::VALID_MIRROR_PROTOCOLS, - allow_localhost: Gitlab::CurrentSettings.allow_local_requests_from_web_hooks_and_services?, - allow_local_network: Gitlab::CurrentSettings.allow_local_requests_from_web_hooks_and_services?, - deny_all_requests_except_allowed: Gitlab::CurrentSettings.deny_all_requests_except_allowed?, - outbound_local_requests_allowlist: Gitlab::CurrentSettings.outbound_local_requests_whitelist) # rubocop:disable Naming/InclusiveLanguage -- existing setting - return error("The import URL is invalid.") - end + return unless project.import_url - unless can?(current_user, :access_git) - return error('The mirror user is not allowed to perform any git operations.') - end - - unless project.mirror? - return success - end + return error("The import URL is invalid.") if import_url_invalid? + return error('The mirror user is not allowed to perform any git operations.') unless can?(current_user, :access_git) + return success unless project.mirror? # This should be an error, but to prevent the mirroring # from being disabled when moving between shards @@ -33,9 +20,7 @@ def execute # Ref: https://gitlab.com/gitlab-org/gitlab/merge_requests/19182 return success if project.repository_read_only? - unless can?(current_user, :push_code_to_protected_branches, project) - return error("The mirror user is not allowed to push code to all branches on this project.") - end + return error("The mirror user is not allowed to push code to all branches on this project.") unless can?(current_user, :push_code_to_protected_branches, project) checksum_before = project.repository.checksum @@ -47,7 +32,7 @@ def execute # Updating LFS objects is expensive since it requires scanning for blobs with pointers. # Let's skip this if the repository hasn't changed. - update_lfs_objects if project.repository.checksum != checksum_before + update_lfs_objects if update_lfs_objects?(checksum_before) # Running git fetch in the repository creates loose objects in the same # way running git push *to* the repository does, so ensure we run regular @@ -61,6 +46,20 @@ def execute private + def update_lfs_objects?(checksum) + project.lfs_enabled? && project.repository.checksum != checksum + end + + def import_url_invalid? + Gitlab::HTTP_V2::UrlBlocker.blocked_url?( + normalized_url(project.import_url), + schemes: Project::VALID_MIRROR_PROTOCOLS, + allow_localhost: Gitlab::CurrentSettings.allow_local_requests_from_web_hooks_and_services?, + allow_local_network: Gitlab::CurrentSettings.allow_local_requests_from_web_hooks_and_services?, + deny_all_requests_except_allowed: Gitlab::CurrentSettings.deny_all_requests_except_allowed?, + outbound_local_requests_allowlist: Gitlab::CurrentSettings.outbound_local_requests_whitelist) # rubocop:disable Naming/InclusiveLanguage -- existing setting + end + def normalized_url(url) strong_memoize_with(:normalized_url, url) do CGI.unescape(Gitlab::UrlSanitizer.sanitize(url)) diff --git a/ee/spec/services/projects/update_mirror_service_spec.rb b/ee/spec/services/projects/update_mirror_service_spec.rb index e141d26029e0a9..4824941b843eba 100644 --- a/ee/spec/services/projects/update_mirror_service_spec.rb +++ b/ee/spec/services/projects/update_mirror_service_spec.rb @@ -3,6 +3,7 @@ require 'spec_helper' RSpec.describe Projects::UpdateMirrorService, feature_category: :source_code_management do + let(:lfs_enabled) { false } let(:project) do create(:project, :repository, :mirror, import_url: Project::UNKNOWN_IMPORT_URL, only_mirror_protected_branches: false) end @@ -10,7 +11,7 @@ subject(:service) { described_class.new(project, project.first_owner) } before do - allow(project).to receive(:lfs_enabled?).and_return(false) + allow(project).to receive(:lfs_enabled?).and_return(lfs_enabled) end describe "#execute" do @@ -43,8 +44,6 @@ expect(svc).to receive(:execute) end - expect(Gitlab::Metrics::Lfs).to receive_message_chain(:update_objects_error_rate, :increment).with(error: false, labels: {}) - service.execute end @@ -59,8 +58,6 @@ it "returns success when updated succeeds" do stub_fetch_mirror(project) - expect(Gitlab::Metrics::Lfs).to receive_message_chain(:update_objects_error_rate, :increment).with(error: false, labels: {}) - result = service.execute expect(result[:status]).to eq(:success) @@ -411,9 +408,7 @@ def create_file(repository) context 'updating LFS objects' do context 'when repository does not change' do - before do - allow(project).to receive(:lfs_enabled?).and_return(true) - end + let(:lfs_enabled) { true } it 'does not attempt to update LFS objects' do expect(Projects::LfsPointers::LfsImportService).not_to receive(:new) @@ -429,19 +424,16 @@ def create_file(repository) context 'when Lfs is disabled in the project' do it 'does not update LFS objects' do - allow(project).to receive(:lfs_enabled?).and_return(false) expect(Projects::LfsPointers::LfsObjectDownloadListService).not_to receive(:new) - expect(Gitlab::Metrics::Lfs).to receive_message_chain(:update_objects_error_rate, :increment).with(error: false, labels: {}) + expect(Gitlab::Metrics::Lfs).not_to receive(:update_objects_error_rate) service.execute end end context 'when Lfs is enabled in the project' do - before do - allow(project).to receive(:lfs_enabled?).and_return(true) - end + let(:lfs_enabled) { true } it 'updates LFS objects' do expect(Projects::LfsPointers::LfsImportService).to receive(:new).and_call_original -- GitLab From a93510919494165f91c48e7c7545dae64c7b7e4e Mon Sep 17 00:00:00 2001 From: Ash McKenzie Date: Fri, 16 Aug 2024 16:34:31 +1000 Subject: [PATCH 2/4] Update Lfs strings to LFS --- ee/spec/services/projects/update_mirror_service_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ee/spec/services/projects/update_mirror_service_spec.rb b/ee/spec/services/projects/update_mirror_service_spec.rb index 4824941b843eba..5377915928097d 100644 --- a/ee/spec/services/projects/update_mirror_service_spec.rb +++ b/ee/spec/services/projects/update_mirror_service_spec.rb @@ -422,7 +422,7 @@ def create_file(repository) stub_fetch_mirror(project) end - context 'when Lfs is disabled in the project' do + context 'when LFS is disabled in the project' do it 'does not update LFS objects' do expect(Projects::LfsPointers::LfsObjectDownloadListService).not_to receive(:new) @@ -432,7 +432,7 @@ def create_file(repository) end end - context 'when Lfs is enabled in the project' do + context 'when LFS is enabled in the project' do let(:lfs_enabled) { true } it 'updates LFS objects' do @@ -446,7 +446,7 @@ def create_file(repository) service.execute end - context 'when Lfs import fails' do + context 'when LFS import fails' do let(:error_message) { 'error_message' } before do -- GitLab From 7eff30986bfb9f8cb7b8157aacf5148bc927ef7e Mon Sep 17 00:00:00 2001 From: Ash McKenzie Date: Fri, 16 Aug 2024 16:35:06 +1000 Subject: [PATCH 3/4] Move import URL check into import_url_invalid? --- ee/app/services/projects/update_mirror_service.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/ee/app/services/projects/update_mirror_service.rb b/ee/app/services/projects/update_mirror_service.rb index 805aa0bce406e1..3574186c151487 100644 --- a/ee/app/services/projects/update_mirror_service.rb +++ b/ee/app/services/projects/update_mirror_service.rb @@ -8,8 +8,6 @@ class UpdateMirrorService < BaseService UpdateError = Class.new(Error) def execute - return unless project.import_url - return error("The import URL is invalid.") if import_url_invalid? return error('The mirror user is not allowed to perform any git operations.') unless can?(current_user, :access_git) return success unless project.mirror? @@ -51,7 +49,7 @@ def update_lfs_objects?(checksum) end def import_url_invalid? - Gitlab::HTTP_V2::UrlBlocker.blocked_url?( + project.import_url && Gitlab::HTTP_V2::UrlBlocker.blocked_url?( normalized_url(project.import_url), schemes: Project::VALID_MIRROR_PROTOCOLS, allow_localhost: Gitlab::CurrentSettings.allow_local_requests_from_web_hooks_and_services?, -- GitLab From 570ee839c3bd0bd28027c91232885b9c80304ede Mon Sep 17 00:00:00 2001 From: Ash McKenzie Date: Tue, 27 Aug 2024 12:11:14 +1000 Subject: [PATCH 4/4] Use expanded format to appease RuboCop --- ee/app/services/projects/update_mirror_service.rb | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/ee/app/services/projects/update_mirror_service.rb b/ee/app/services/projects/update_mirror_service.rb index 3574186c151487..291b2488cfc07a 100644 --- a/ee/app/services/projects/update_mirror_service.rb +++ b/ee/app/services/projects/update_mirror_service.rb @@ -9,7 +9,11 @@ class UpdateMirrorService < BaseService def execute return error("The import URL is invalid.") if import_url_invalid? - return error('The mirror user is not allowed to perform any git operations.') unless can?(current_user, :access_git) + + unless can?(current_user, :access_git) + return error('The mirror user is not allowed to perform any git operations.') + end + return success unless project.mirror? # This should be an error, but to prevent the mirroring @@ -18,7 +22,9 @@ def execute # Ref: https://gitlab.com/gitlab-org/gitlab/merge_requests/19182 return success if project.repository_read_only? - return error("The mirror user is not allowed to push code to all branches on this project.") unless can?(current_user, :push_code_to_protected_branches, project) + unless can?(current_user, :push_code_to_protected_branches, project) + return error("The mirror user is not allowed to push code to all branches on this project.") + end checksum_before = project.repository.checksum -- GitLab