Merge branch '435644-extract-dependency-proxy-logic' into 'master'

Extract common parts of the Maven dependency proxy

See merge request !144324



Merged-by: David Fernandez <dfernandez@gitlab.com>
Approved-by: Moaz Khalifa <mkhalifa@gitlab.com>
Approved-by: Subashis Chakraborty <schakraborty@gitlab.com>
Approved-by: Eugenia Grieff <egrieff@gitlab.com>
Reviewed-by: David Fernandez <dfernandez@gitlab.com>
Reviewed-by: Moaz Khalifa <mkhalifa@gitlab.com>

ee/app/services/dependency_proxy/packages/maven/verify_package_file_etag_service.rb

-# frozen_string_literal: true
-
-module DependencyProxy
-  module Packages
-    module Maven
-      class VerifyPackageFileEtagService
-        include ::Gitlab::Utils::StrongMemoize
-
-        REGISTRY_NOT_AVAILABLE_ERROR_CODE = 503
-        REGISTRY_NOT_AVAILABLE_MESSAGE = 'External registry is not available'
-        TIMEOUT = 5
-
-        def initialize(remote_url:, package_file:)
-          @remote_url = remote_url
-          @package_file = package_file
-        end
-
-        def execute
-          return ServiceResponse.error(message: 'invalid arguments', reason: :invalid_arguments) unless valid?
-          return error_with_response_code unless response.success?
-          return ServiceResponse.error(message: 'no etag from external registry', reason: :no_etag) if etag.blank?
-          return ServiceResponse.success if etag_match?
-
-          ServiceResponse.error(
-            message: "etag from external registry doesn't match any known digests",
-            reason: :wrong_etag
-          )
-        rescue *::Gitlab::HTTP::HTTP_ERRORS
-          error_with_response_code(
-            code: REGISTRY_NOT_AVAILABLE_ERROR_CODE,
-            message: REGISTRY_NOT_AVAILABLE_MESSAGE
-          )
-        end
-
-        private
-
-        attr_reader :remote_url, :package_file
-
-        def response
-          ::Gitlab::HTTP.head(remote_url, follow_redirects: true, timeout: TIMEOUT)
-        end
-        strong_memoize_attr :response
-
-        def etag_match?
-          return false unless sanitized_etag
-
-          # Remote registries can have different ways to return the ETag field:
-          # GitLab: md5
-          # Maven Central: md5
-          # Artifactory: sha1
-          # Github: No ETag field
-          # Sonatype Nexus: custom string with the sha1. Example {SHA1{e9702caacd0b915b0495f6d191371d61c379cd1c}}.
-          #
-          # We thus need to check the etag field in different ways.
-
-          # Check if the Etag is exactly one of the digests.
-          return true if %i[md5 sha1 sha256].any? { |digest| sanitized_etag == package_file["file_#{digest}"] }
-          return true if %i[sha1].any? { |digest| sanitized_etag.include?(package_file["file_#{digest}"]) }
-
-          false
-        end
-
-        def sanitized_etag
-          return unless etag.present?
-
-          etag.delete('"')
-        end
-        strong_memoize_attr :sanitized_etag
-
-        def etag
-          strong_memoize_with(:etag, response) do
-            response.headers['etag']
-          end
-        end
-
-        def valid?
-          remote_url.present? && package_file
-        end
-
-        def error_with_response_code(code: response.code, message: nil)
-          message ||= "Received #{code} from external registry"
-          Gitlab::AppLogger.error(
-            service_class: self.class.to_s,
-            project_id: package_file.package&.project_id,
-            message: message
-          )
-
-          ServiceResponse.error(message: message, reason: :response_error_code)
-        end
-      end
-    end
-  end
-end

ee/app/services/dependency_proxy/packages/verify_package_file_etag_service.rb

+# frozen_string_literal: true
+
+module DependencyProxy
+  module Packages
+    class VerifyPackageFileEtagService
+      include ::Gitlab::Utils::StrongMemoize
+
+      REGISTRY_NOT_AVAILABLE_ERROR_CODE = 503
+      REGISTRY_NOT_AVAILABLE_MESSAGE = 'External registry is not available'
+      TIMEOUT = 5
+
+      def initialize(remote_url:, package_file:, headers: {})
+        @remote_url = remote_url
+        @package_file = package_file
+        @headers = headers
+      end
+
+      def execute
+        return ServiceResponse.error(message: 'invalid arguments', reason: :invalid_arguments) unless valid?
+        return error_with_response_code unless response.success?
+        return ServiceResponse.error(message: 'no etag from external registry', reason: :no_etag) if etag.blank?
+        return ServiceResponse.success if etag_match?
+
+        ServiceResponse.error(
+          message: "etag from external registry doesn't match any known digests",
+          reason: :wrong_etag
+        )
+      rescue *::Gitlab::HTTP::HTTP_ERRORS
+        error_with_response_code(
+          code: REGISTRY_NOT_AVAILABLE_ERROR_CODE,
+          message: REGISTRY_NOT_AVAILABLE_MESSAGE
+        )
+      end
+
+      private
+
+      attr_reader :remote_url, :package_file, :headers
+
+      def response
+        ::Gitlab::HTTP.head(remote_url, headers: headers, follow_redirects: true, timeout: TIMEOUT)
+      end
+      strong_memoize_attr :response
+
+      def etag_match?
+        return false unless sanitized_etag
+
+        # Remote registries can have different ways to return the ETag field:
+        # GitLab: md5
+        # Maven Central: md5
+        # Artifactory: sha1
+        # Github: No ETag field
+        # Sonatype Nexus: custom string with the sha1. Example {SHA1{e9702caacd0b915b0495f6d191371d61c379cd1c}}.
+        #
+        # We thus need to check the etag field in different ways.
+
+        return true if %i[md5 sha1 sha256].any? { |digest| sanitized_etag == package_file["file_#{digest}"] }
+        return true if %i[sha1].any? { |digest| sanitized_etag.include?(package_file["file_#{digest}"]) }
+
+        false
+      end
+
+      def sanitized_etag
+        return unless etag.present?
+
+        etag.delete('"')
+      end
+      strong_memoize_attr :sanitized_etag
+
+      def etag
+        response.headers['etag']
+      end
+      strong_memoize_attr :etag
+
+      def valid?
+        remote_url.present? && package_file
+      end
+
+      def error_with_response_code(code: response.code, message: nil)
+        message ||= "Received #{code} from external registry"
+        Gitlab::AppLogger.error(
+          service_class: self.class.to_s,
+          project_id: package_file.project_id,
+          message: message
+        )
+
+        ServiceResponse.error(message: message, reason: :response_error_code)
+      end
+    end
+  end
+end

ee/lib/api/concerns/dependency_proxy/packages_helpers.rb

+# frozen_string_literal: true
+
+module API
+  module Concerns
+    module DependencyProxy
+      module PackagesHelpers
+        extend ActiveSupport::Concern
+        include Gitlab::ClassAttributes
+
+        TIMEOUTS = {
+          open: 10,
+          read: 10
+        }.freeze
+
+        RESPONSE_STATUSES = {
+          error: :bad_gateway,
+          timeout: :gateway_timeout
+        }.freeze
+
+        CALLBACKS_CLASS = Struct.new(:skip_upload, :before_respond_with)
+
+        included do
+          helpers ::API::Helpers::PackagesHelpers
+
+          feature_category :package_registry
+          urgency :low
+
+          helpers do
+            include ::Gitlab::Utils::StrongMemoize
+
+            def dependency_proxy_setting
+              setting = project.dependency_proxy_packages_setting
+              external_registry_url_field = "#{package_format}_external_registry_url".to_sym
+              return unless setting.enabled && setting[external_registry_url_field]
+
+              return setting if can?(current_user, :read_package, setting)
+
+              # guest users can have :read_project but not :read_package
+              forbidden! if can?(current_user, :read_project, project)
+            end
+            strong_memoize_attr :dependency_proxy_setting
+
+            def destroy_package_file(package_file)
+              return unless package_file
+
+              ::Packages::MarkPackageFilesForDestructionService.new(
+                ::Packages::PackageFile.id_in(package_file.id)
+              ).execute
+            end
+
+            def respond_with(package_file:)
+              result = ::DependencyProxy::Packages::VerifyPackageFileEtagService.new(
+                remote_url: remote_package_file_url,
+                package_file: package_file
+              ).execute
+
+              if result.success? || (result.error? && result.reason != :wrong_etag)
+                track_file_pulled_event(from_cache: true)
+                present_carrierwave_file_with_head_support!(package_file)
+              elsif can?(current_user, :destroy_package, dependency_proxy_setting) &&
+                  can?(current_user, :create_package, dependency_proxy_setting)
+                destroy_package_file(package_file) if package_file
+
+                send_and_upload_remote_url
+              else
+                send_remote_url
+              end
+            end
+
+            def send_remote_url
+              send_workhorse_headers(
+                Gitlab::Workhorse.send_url(
+                  remote_package_file_url,
+                  headers: remote_url_headers,
+                  allow_redirects: true,
+                  timeouts: TIMEOUTS,
+                  response_statuses: RESPONSE_STATUSES
+                )
+              )
+            end
+
+            def track_file_pulled_event(from_cache: false)
+              event_name = from_cache ? tracking_event_name(from: :cache) : tracking_event_name(from: :external)
+
+              # we can't send deploy tokens to #track_event
+              user = current_user if current_user.is_a?(User)
+
+              ::Gitlab::InternalEvents.track_event(event_name, user: user, project: project)
+            end
+
+            def tracking_event_name(from:)
+              "dependency_proxy_packages_#{package_format}_file_pulled_from_#{from}"
+            end
+
+            def send_and_upload_remote_url
+              upload_config = {
+                method: 'PUT',
+                url: upload_url,
+                headers: upload_headers
+              }
+
+              send_workhorse_headers(
+                Gitlab::Workhorse.send_dependency(
+                  remote_url_headers,
+                  remote_package_file_url,
+                  upload_config: upload_config
+                )
+              )
+            end
+
+            def send_workhorse_headers(headers)
+              track_file_pulled_event(from_cache: false)
+              header(*headers)
+              env['api.format'] = :binary
+              status :ok
+              body ''
+            end
+
+            def handle(package_file)
+              callbacks = CALLBACKS_CLASS.new
+
+              yield callbacks if block_given?
+
+              if package_file
+                handle_existing_file(package_file: package_file, callbacks: callbacks)
+              else
+                handle_new_file(callbacks: callbacks)
+              end
+            end
+
+            def handle_existing_file(package_file:, callbacks:)
+              result = callbacks.before_respond_with&.call
+              return result unless result.blank?
+
+              respond_with(package_file: package_file)
+            end
+
+            def handle_new_file(callbacks:)
+              if can?(current_user, :create_package, dependency_proxy_setting) && !callbacks.skip_upload&.call
+                send_and_upload_remote_url
+              else
+                send_remote_url
+              end
+            end
+
+            def upload_headers
+              {}
+            end
+
+            def remote_url_headers
+              {}
+            end
+
+            def package_format
+              options[:for].name.demodulize.underscore
+            end
+
+            def wrap_error_response
+              yield
+            end
+          end
+
+          after_validation do
+            require_packages_enabled!
+            require_dependency_proxy_enabled!
+
+            wrap_error_response { not_found! } unless dependency_proxy_setting
+
+            unless project.licensed_feature_available?(:dependency_proxy_for_packages)
+              wrap_error_response { forbidden! }
+            end
+          end
+        end
+      end
+    end
+  end
+end

ee/lib/api/dependency_proxy/packages/maven.rb

@@ -5,34 +5,18 @@ class DependencyProxy
     module Packages
       class Maven < ::API::Base
         include ::API::Helpers::Authentication
-        helpers ::API::Helpers::PackagesHelpers
         helpers ::API::Helpers::Packages::Maven
         helpers ::API::Helpers::Packages::Maven::BasicAuthHelpers
         helpers ::API::Helpers::RelatedResourcesHelpers
-
-        feature_category :package_registry
-        urgency :low
+        include ::API::Concerns::DependencyProxy::PackagesHelpers
 
         content_type :md5, 'text/plain'
         content_type :sha1, 'text/plain'
         content_type :binary, 'application/octet-stream'
 
-        TIMEOUTS = {
-          open: 10,
-          read: 10
-        }.freeze
-
-        RESPONSE_STATUSES = {
-          error: :bad_gateway,
-          timeout: :gateway_timeout
-        }.freeze
-
-        TRACKING_EVENT_NAME_FROM_EXTERNAL = 'dependency_proxy_packages_maven_file_pulled_from_external'
-        TRACKING_EVENT_NAME_FROM_CACHE = 'dependency_proxy_packages_maven_file_pulled_from_cache'
+        DIGESTS_FORMATS = %w[sha1 md5].freeze
 
         helpers do
-          include ::Gitlab::Utils::StrongMemoize
-
           delegate :maven_external_registry_username, :maven_external_registry_password, :maven_external_registry_url,
             to: :dependency_proxy_setting
 
@@ -40,24 +24,6 @@ def project
             authorized_user_project(action: :read_package)
           end
 
-          def dependency_proxy_setting
-            setting = project.dependency_proxy_packages_setting
-            return unless setting&.enabled && setting&.maven_external_registry_url
-
-            return setting if can?(current_user, :read_package, setting)
-            # guest users can have :read_project but not :read_package
-            return forbidden! if can?(current_user, :read_project, project)
-          end
-          strong_memoize_attr :dependency_proxy_setting
-
-          def destroy_package_file(package_file)
-            return unless package_file
-
-            ::Packages::MarkPackageFilesForDestructionService.new(
-              ::Packages::PackageFile.id_in(package_file.id)
-            ).execute
-          end
-
           def remote_package_file_url
             full_url = [maven_external_registry_url, declared_params[:path], declared_params[:file_name]].join('/')
             uri = Addressable::URI.parse(full_url)
@@ -69,82 +35,12 @@ def remote_package_file_url
 
             uri.to_s
           end
-          strong_memoize_attr :remote_package_file_url
-
-          def respond_with(package_file:, format:)
-            return respond_digest(package_file.file_md5) if format == 'md5'
-            return respond_digest(package_file.file_sha1) if format == 'sha1'
-
-            result = ::DependencyProxy::Packages::Maven::VerifyPackageFileEtagService.new(
-              remote_url: remote_package_file_url,
-              package_file: package_file
-            ).execute
-
-            if result.success? || (result.error? && result.reason != :wrong_etag)
-              track_file_pulled_event(from_cache: true)
-              present_carrierwave_file_with_head_support!(package_file)
-            elsif can?(current_user, :destroy_package, dependency_proxy_setting) &&
-                can?(current_user, :create_package, dependency_proxy_setting)
-              destroy_package_file(package_file) if package_file
-
-              send_and_upload_remote_url(format: format)
-            else
-              send_remote_url(remote_package_file_url)
-            end
-          end
 
           def respond_digest(digest)
             track_file_pulled_event(from_cache: true)
             digest
           end
 
-          def send_remote_url(url)
-            track_file_pulled_event(from_cache: false)
-            header(
-              *Gitlab::Workhorse.send_url(
-                url,
-                allow_redirects: true,
-                timeouts: TIMEOUTS,
-                response_statuses: RESPONSE_STATUSES
-              )
-            )
-            env['api.format'] = :binary
-            status :ok
-            body ''
-          end
-
-          def send_dependency(headers, url, upload_config: {})
-            track_file_pulled_event(from_cache: false)
-            header(*Gitlab::Workhorse.send_dependency(headers, url, upload_config: upload_config))
-            env['api.format'] = :binary
-            status :ok
-            body ''
-          end
-
-          def track_file_pulled_event(from_cache: false)
-            event_name = from_cache ? TRACKING_EVENT_NAME_FROM_CACHE : TRACKING_EVENT_NAME_FROM_EXTERNAL
-
-            # we can't send deploy tokens to #track_event
-            user = current_user if current_user.is_a?(User)
-
-            ::Gitlab::InternalEvents.track_event(event_name, user: user, project: project)
-          end
-
-          def send_and_upload_remote_url(format:)
-            if format == 'md5' || format == 'sha1'
-              # We don't store those formats. Fall back to sending the file from the remote registry.
-              return send_remote_url(remote_package_file_url)
-            end
-
-            upload_config = {
-              method: 'PUT',
-              url: upload_url,
-              headers: upload_headers
-            }
-
-            send_dependency({}, remote_package_file_url, upload_config: upload_config)
-          end
-
           def upload_url
             url = api_v4_projects_packages_maven_path_path(
               {
@@ -175,11 +71,10 @@ def upload_headers
             _, token = user_name_and_password(current_request)
             { header_name => token }
           end
-        end
 
-        after_validation do
-          require_packages_enabled!
-          require_dependency_proxy_enabled!
+          def wrap_error_response
+            unauthorized_or! { yield }
+          end
         end
 
         authenticate_with do |accept|
@@ -215,19 +110,15 @@ def upload_headers
           end
           get ':id/dependency_proxy/packages/maven/*path/:file_name',
             requirements: ::API::MavenPackages::MAVEN_ENDPOINT_REQUIREMENTS do
-            unauthorized_or! { not_found! } unless dependency_proxy_setting
-            unauthorized_or! { forbidden! } unless project.licensed_feature_available?(:dependency_proxy_for_packages)
-
             file_name, format = extract_format(params[:file_name])
             package = fetch_package(project: project, file_name: file_name)
             package_file = ::Packages::PackageFileFinder.new(package, file_name).execute if package
 
-            if package && package_file
-              respond_with(package_file: package_file, format: format)
-            elsif can?(current_user, :create_package, dependency_proxy_setting)
-              send_and_upload_remote_url(format: format)
-            else
-              send_remote_url(remote_package_file_url)
+            handle(package_file) do |callbacks|
+              callbacks.skip_upload = -> { format.in?(DIGESTS_FORMATS) }
+              callbacks.before_respond_with = -> do
+                respond_digest(package_file["file_#{format}"]) if format.in?(DIGESTS_FORMATS)
+              end
             end
           end
         end

ee/spec/features/dependency_proxy/packages/maven_spec.rb

@@ -61,7 +61,7 @@
 
   shared_examples 'returning the cached file' do
     it 'returns the cached file' do
-      expect_next_instance_of(::DependencyProxy::Packages::Maven::VerifyPackageFileEtagService) do |service|
+      expect_next_instance_of(::DependencyProxy::Packages::VerifyPackageFileEtagService) do |service|
         expect(service).to receive(:execute).and_return(ServiceResponse.success)
       end
       expect(Gitlab::Workhorse).not_to receive(:send_url)
@@ -105,7 +105,7 @@
 
   shared_context 'with a wrong etag returned' do
     before do
-      allow_next_instance_of(::DependencyProxy::Packages::Maven::VerifyPackageFileEtagService) do |service|
+      allow_next_instance_of(::DependencyProxy::Packages::VerifyPackageFileEtagService) do |service|
         allow(service).to receive(:execute).and_return(ServiceResponse.error(message: '', reason: :wrong_etag))
       end
     end
@@ -113,7 +113,7 @@
 
   shared_context 'with no etag returned' do
     before do
-      allow_next_instance_of(::DependencyProxy::Packages::Maven::VerifyPackageFileEtagService) do |service|
+      allow_next_instance_of(::DependencyProxy::Packages::VerifyPackageFileEtagService) do |service|
         allow(service).to receive(:execute).and_return(ServiceResponse.error(message: '', reason: :no_etag))
       end
     end

ee/spec/requests/api/dependency_proxy/packages/maven_spec.rb

@@ -39,7 +39,7 @@
         let(:file_name) { package_file.file_name }
 
         before do
-          allow_next_instance_of(::DependencyProxy::Packages::Maven::VerifyPackageFileEtagService) do |service|
+          allow_next_instance_of(::DependencyProxy::Packages::VerifyPackageFileEtagService) do |service|
             allow(service).to receive(:execute).and_return(ServiceResponse.success)
           end
         end
@@ -131,9 +131,9 @@
         shared_examples 'tracking an internal event' do |from_cache: false|
           it 'tracks an internal event' do
             event_name = if from_cache
-                           described_class::TRACKING_EVENT_NAME_FROM_CACHE
+                           'dependency_proxy_packages_maven_file_pulled_from_cache'
                          else
-                           described_class::TRACKING_EVENT_NAME_FROM_EXTERNAL
+                           'dependency_proxy_packages_maven_file_pulled_from_external'
                          end
 
             u = user unless using_a_deploy_token
@@ -216,7 +216,7 @@
 
             with_them do
               before do
-                allow_next_instance_of(::DependencyProxy::Packages::Maven::VerifyPackageFileEtagService) do |service|
+                allow_next_instance_of(::DependencyProxy::Packages::VerifyPackageFileEtagService) do |service|
                   allow(service).to receive(:execute).and_return(etag_service_response)
                 end
               end

ee/spec/services/dependency_proxy/packages/maven/verify_package_file_etag_service_spec.rb → ee/spec/services/dependency_proxy/packages/verify_package_file_etag_service_spec.rb

@@ -2,11 +2,11 @@
 
 require 'spec_helper'
 
-RSpec.describe DependencyProxy::Packages::Maven::VerifyPackageFileEtagService, :aggregate_failures, feature_category: :package_registry do
+RSpec.describe DependencyProxy::Packages::VerifyPackageFileEtagService, :aggregate_failures, feature_category: :package_registry do
   let_it_be(:setting) { create(:dependency_proxy_packages_setting, :maven) }
   let_it_be(:package_file) { create(:package_file, :jar) }
 
-  let(:remote_url) { "http://#{setting.maven_external_registry_username}:#{setting.maven_external_registry_password}@test/package.file" }
+  let(:remote_url) { 'http://test/package.file' }
 
   let(:authorization_header) do
     ActionController::HttpAuthentication::Basic.encode_credentials(
@@ -15,8 +15,10 @@
     )
   end
 
+  let(:request_headers) { { 'Authorization' => authorization_header } }
+
   let(:service) do
-    described_class.new(remote_url: remote_url, package_file: package_file)
+    described_class.new(remote_url: remote_url, package_file: package_file, headers: request_headers)
   end
 
   describe '#execute' do
@@ -41,27 +43,22 @@
 
     context 'with valid arguments' do
       context 'with a successful head request' do
-        it 'returns a successful service response' do
-          stub_external_registry_request(status: 200, etag: "\"#{package_file.file_md5}\"")
+        let(:etag) { package_file.file_md5 }
 
-          expect(result).to be_a(ServiceResponse)
-          expect(result).to be_success
+        before do
+          stub_external_registry_request(status: 200, etag: etag)
         end
 
+        it_behaves_like 'returning a success service response'
+
         context 'with an etag that contains a digest' do
-          it 'returns a successful service response' do
-            etag = "\"{SHA1{#{package_file.file_sha1}\"}"
-            stub_external_registry_request(status: 200, etag: etag)
+          let(:etag) { "\"{SHA1{#{package_file.file_sha1}\"}" }
 
-            expect(result).to be_a(ServiceResponse)
-            expect(result).to be_success
-          end
+          it_behaves_like 'returning a success service response'
         end
 
         context 'with an unmatched etag' do
-          before do
-            stub_external_registry_request(status: 200, etag: 'wrong_etag')
-          end
+          let(:etag) { 'wrong_etag' }
 
           it_behaves_like 'expecting a service response error with',
             message: "etag from external registry doesn't match any known digests",
@@ -69,9 +66,7 @@
         end
 
         context 'with an absent etag' do
-          before do
-            stub_external_registry_request(status: 200, etag: nil)
-          end
+          let(:etag) { nil }
 
           it_behaves_like 'expecting a service response error with',
             message: 'no etag from external registry',
@@ -90,6 +85,22 @@
             expect(result).to be_success
           end
         end
+
+        context 'with an inline basic auth' do
+          let(:remote_url) { "http://#{setting.maven_external_registry_username}:#{setting.maven_external_registry_password}@test/package.file" }
+
+          let(:service) do
+            described_class.new(remote_url: remote_url, package_file: package_file)
+          end
+
+          it_behaves_like 'returning a success service response'
+        end
+
+        context 'with custom headers' do
+          let(:request_headers) { { 'Authorization' => 'Bearer test' } }
+
+          it_behaves_like 'returning a success service response'
+        end
       end
 
       context 'with a unsuccessful head request' do
@@ -126,11 +137,11 @@
     end
 
     def stub_external_registry_request(status: 200, etag: 'etag', response_headers: {})
-      headers = response_headers
-      headers[:etag] = "\"#{etag}\"" if etag
+      response_headers[:etag] = "\"#{etag}\"" if etag
+
       stub_request(:head, 'http://test/package.file')
-        .with(headers: { 'Authorization' => authorization_header })
-        .to_return(status: status, body: '', headers: headers)
+        .with(headers: request_headers)
+        .to_return(status: status, body: '', headers: response_headers)
     end
   end
 end