Merge branch '326819-check-maven-path' into 'master'

Check maven path for maven file API endpoints [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!59241

app/models/packages/maven/metadatum.rb

@@ -19,6 +19,7 @@ class Packages::Maven::Metadatum < ApplicationRecord
   validate :maven_package_type
 
   scope :for_package_ids, -> (package_ids) { where(package_id: package_ids) }
+  scope :with_path, ->(path) { where(path: path) }
   scope :order_created, -> { reorder('created_at ASC') }
 
   def self.pluck_app_name

changelogs/unreleased/326819-check-maven-path.yml

+---
+title: Add index for the path column on the packages_maven_metadata table
+merge_request: 59241
+author:
+type: performance

config/feature_flags/development/check_maven_path_first.yml

+---
+name: check_maven_path_first
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/59241
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/327487
+milestone: '13.11'
+type: development
+group: group::package
+default_enabled: false

db/post_migrate/20210413092922_add_index_to_packages_maven_metadata_path.rb

+# frozen_string_literal: true
+
+class AddIndexToPackagesMavenMetadataPath < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  INDEX_NAME = 'index_packages_maven_metadata_on_path'
+
+  def up
+    add_concurrent_index :packages_maven_metadata, :path, name: INDEX_NAME
+  end
+
+  def down
+    remove_concurrent_index :packages_maven_metadata, :path, name: INDEX_NAME
+  end
+end

db/schema_migrations/20210413092922

+2da634fa920e3989d9b8e53ddc1ba005e5bc0f4701426e3841d90a42bd2e908f
\ No newline at end of file

db/structure.sql

@@ -23360,6 +23360,8 @@ CREATE INDEX index_packages_events_on_package_id ON packages_events USING btree
 
 CREATE INDEX index_packages_maven_metadata_on_package_id_and_path ON packages_maven_metadata USING btree (package_id, path);
 
+CREATE INDEX index_packages_maven_metadata_on_path ON packages_maven_metadata USING btree (path);
+
 CREATE INDEX index_packages_nuget_dl_metadata_on_dependency_link_id ON packages_nuget_dependency_link_metadata USING btree (dependency_link_id);
 
 CREATE UNIQUE INDEX index_packages_on_project_id_name_version_unique_when_generic ON packages_packages USING btree (project_id, name, version) WHERE (package_type = 7);

lib/api/maven_packages.rb

@@ -23,6 +23,15 @@ module API
     helpers ::API::Helpers::PackagesHelpers
 
     helpers do
+      def path_exists?(path)
+        # return true when FF disabled so that processing the request is not stopped
+        return true unless Feature.enabled?(:check_maven_path_first)
+        return false if path.blank?
+
+        Packages::Maven::Metadatum.with_path(path)
+                                  .exists?
+      end
+
       def extract_format(file_name)
         name, _, format = file_name.rpartition('.')
 
@@ -104,6 +113,9 @@ module API
     end
     route_setting :authentication, job_token_allowed: true, deploy_token_allowed: true
     get 'packages/maven/*path/:file_name', requirements: MAVEN_ENDPOINT_REQUIREMENTS do
+      # return a similar failure to authorize_read_package!(project)
+      forbidden! unless path_exists?(params[:path])
+
       file_name, format = extract_format(params[:file_name])
 
       # To avoid name collision we require project path and project package be the same.
@@ -142,6 +154,9 @@ module API
       end
       route_setting :authentication, job_token_allowed: true, deploy_token_allowed: true
       get ':id/-/packages/maven/*path/:file_name', requirements: MAVEN_ENDPOINT_REQUIREMENTS do
+        # return a similar failure to group = find_group(params[:id])
+        not_found!('Group') unless path_exists?(params[:path])
+
         file_name, format = extract_format(params[:file_name])
 
         group = find_group(params[:id])
@@ -181,6 +196,9 @@ module API
       end
       route_setting :authentication, job_token_allowed: true, deploy_token_allowed: true
       get ':id/packages/maven/*path/:file_name', requirements: MAVEN_ENDPOINT_REQUIREMENTS do
+        # return a similar failure to user_project
+        not_found!('Project') unless path_exists?(params[:path])
+
         authorize_read_package!(user_project)
 
         file_name, format = extract_format(params[:file_name])

spec/models/packages/maven/metadatum_spec.rb

@@ -54,7 +54,7 @@ RSpec.describe Packages::Maven::Metadatum, type: :model do
         let_it_be(:metadatum3) { create(:maven_metadatum, package: package) }
         let_it_be(:metadatum4) { create(:maven_metadatum, package: package) }
 
-        subject { Packages::Maven::Metadatum.for_package_ids(package.id).order_created }
+        subject { described_class.for_package_ids(package.id).order_created }
 
         it { is_expected.to eq([metadatum1, metadatum2, metadatum3, metadatum4]) }
       end
@@ -64,10 +64,20 @@ RSpec.describe Packages::Maven::Metadatum, type: :model do
         let_it_be(:metadatum2) { create(:maven_metadatum, package: package, app_name: 'two') }
         let_it_be(:metadatum3) { create(:maven_metadatum, package: package, app_name: 'three') }
 
-        subject { Packages::Maven::Metadatum.for_package_ids(package.id).pluck_app_name }
+        subject { described_class.for_package_ids(package.id).pluck_app_name }
 
         it { is_expected.to match_array([metadatum1, metadatum2, metadatum3].map(&:app_name)) }
       end
+
+      describe '.with_path' do
+        let_it_be(:metadatum1) { create(:maven_metadatum, package: package, path: 'one') }
+        let_it_be(:metadatum2) { create(:maven_metadatum, package: package, path: 'two') }
+        let_it_be(:metadatum3) { create(:maven_metadatum, package: package, path: 'three') }
+
+        subject { described_class.with_path('two') }
+
+        it { is_expected.to match_array([metadatum2]) }
+      end
     end
   end
 end