From f7a062c417efe26bb118b3ce08aa1ed5744d3847 Mon Sep 17 00:00:00 2001 From: Eduardo Bonet Date: Fri, 19 May 2023 17:35:46 +0200 Subject: [PATCH 01/10] Adds authorize endpoint for pacakges/ml_models Adds first endpoint for model registry. Authorization follows the exact same rules as generic packages, and the tests are the same. Changelog: added --- config/open_api.yml | 2 + lib/api/api.rb | 1 + lib/api/ml_model_packages.rb | 82 ++++++++ lib/gitlab/regex.rb | 12 ++ spec/requests/api/ml_model_packages_spec.rb | 209 ++++++++++++++++++++ workhorse/internal/upstream/routes.go | 3 + workhorse/upload_test.go | 1 + 7 files changed, 310 insertions(+) create mode 100644 lib/api/ml_model_packages.rb create mode 100644 spec/requests/api/ml_model_packages_spec.rb diff --git a/config/open_api.yml b/config/open_api.yml index cbf70c24ce1ce2..257db9dd692582 100644 --- a/config/open_api.yml +++ b/config/open_api.yml @@ -95,6 +95,8 @@ metadata: description: Operations related to metadata of the GitLab instance - name: metrics_user_starred_dashboards description: Operations related to User-starred metrics dashboards + - name: ml_model_registry + description: Operations related to Model registry - name: npm_packages description: Operations related to NPM packages - name: nuget_packages diff --git a/lib/api/api.rb b/lib/api/api.rb index a7acd44e72a2bb..5f7faa7eb7d37f 100644 --- a/lib/api/api.rb +++ b/lib/api/api.rb @@ -255,6 +255,7 @@ class API < ::API::Base mount ::API::Metadata mount ::API::Metrics::Dashboard::Annotations mount ::API::Metrics::UserStarredDashboards + mount ::API::MlModelPackages mount ::API::Namespaces mount ::API::NpmGroupPackages mount ::API::NpmInstancePackages diff --git a/lib/api/ml_model_packages.rb b/lib/api/ml_model_packages.rb new file mode 100644 index 00000000000000..baed1103d373ed --- /dev/null +++ b/lib/api/ml_model_packages.rb @@ -0,0 +1,82 @@ +# frozen_string_literal: true + +module API + class MlModelPackages < ::API::Base + include APIGuard + + ML_MODEL_PACKAGES_REQUIREMENTS = { + package_name: API::NO_SLASH_URL_PART_REGEX, + file_name: API::NO_SLASH_URL_PART_REGEX + }.freeze + + ALLOWED_STATUSES = %w[default hidden].freeze + + feature_category :mlops + urgency :low + + rescue_from ArgumentError do |e| + render_api_error!(e.message, 400) + end + + rescue_from ActiveRecord::RecordInvalid do |e| + render_api_error!(e.message, 400) + end + + before do + require_packages_enabled! + authenticate_non_get! + + not_found! unless can?(current_user, :read_model_registry, user_project) + end + + params do + requires :id, types: [String, Integer], desc: 'The ID or URL-encoded path of the project' + end + + resource :projects, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do + route_setting :authentication, job_token_allowed: true, basic_auth_personal_access_token: true, + deploy_token_allowed: true + + namespace ':id/packages/ml_models' do + namespace ':package_name/*package_version/:file_name', requirements: ML_MODEL_PACKAGES_REQUIREMENTS do + desc 'Workhorse authorize model package file' do + detail 'Introduced in GitLab 16.1' + success code: 200 + failure [ + { code: 401, message: 'Unauthorized' }, + { code: 403, message: 'Forbidden' }, + { code: 404, message: 'Not Found' } + ] + tags %w[ml_model_registry] + end + + route_setting :authentication, job_token_allowed: true, basic_auth_personal_access_token: true, + deploy_token_allowed: true + + params do + requires :package_name, type: String, desc: 'Package name', regexp: Gitlab::Regex.ml_model_name_regex, + file_path: true + requires :package_version, type: String, desc: 'Package version', + regexp: Gitlab::Regex.ml_model_version_regex + requires :file_name, type: String, desc: 'Package file name', + regexp: Gitlab::Regex.ml_model_file_name_regex, file_path: true + optional :status, type: String, values: ALLOWED_STATUSES, desc: 'Package status' + end + + put 'authorize' do + authorize_workhorse!(subject: user_project, maximum_size: user_project.actual_limits.ml_model_max_file_size) + end + end + end + end + + helpers do + include ::API::Helpers::PackagesHelpers + include ::API::Helpers::Packages::BasicAuthHelpers + + def max_file_size_exceeded? + authorized_user_project.actual_limits.exceeded?(:ml_model_max_file_size, params[:file].size) + end + end + end +end diff --git a/lib/gitlab/regex.rb b/lib/gitlab/regex.rb index 79f12ee13f78d8..559b577abe031f 100644 --- a/lib/gitlab/regex.rb +++ b/lib/gitlab/regex.rb @@ -623,6 +623,18 @@ def sep_by_1(separator, part) def x509_subject_key_identifier_regex @x509_subject_key_identifier_regex ||= /\A(?:\h{2}:)*\h{2}\z/.freeze end + + def ml_model_version_regex + maven_version_regex + end + + def ml_model_name_regex + maven_file_name_regex + end + + def ml_model_file_name_regex + generic_package_name_regex + end end end diff --git a/spec/requests/api/ml_model_packages_spec.rb b/spec/requests/api/ml_model_packages_spec.rb new file mode 100644 index 00000000000000..e6c5fac38aba70 --- /dev/null +++ b/spec/requests/api/ml_model_packages_spec.rb @@ -0,0 +1,209 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::API::MlModelPackages, feature_category: :mlops do + include HttpBasicAuthHelpers + using RSpec::Parameterized::TableSyntax + + include_context 'workhorse headers' + + let_it_be(:personal_access_token) { create(:personal_access_token) } + let_it_be(:project, reload: true) { create(:project) } + let_it_be(:another_project, reload: true) { create(:project) } + let_it_be(:deploy_token_rw) { create(:deploy_token, read_package_registry: true, write_package_registry: true) } + let_it_be(:project_deploy_token_rw) { create(:project_deploy_token, deploy_token: deploy_token_rw, project: project) } + let_it_be(:deploy_token_ro) { create(:deploy_token, read_package_registry: true, write_package_registry: false) } + let_it_be(:project_deploy_token_ro) { create(:project_deploy_token, deploy_token: deploy_token_ro, project: project) } + let_it_be(:deploy_token_wo) { create(:deploy_token, read_package_registry: false, write_package_registry: true) } + let_it_be(:project_deploy_token_wo) { create(:project_deploy_token, deploy_token: deploy_token_wo, project: project) } + + let(:user) { personal_access_token.user } + let(:ci_build) { create(:ci_build, :running, user: user, project: project) } + let(:project_to_enable_ff) { project } + + def auth_header + return {} if user_role == :anonymous + + case authenticate_with + when :personal_access_token + personal_access_token_header + when :job_token + job_token_header + when :invalid_personal_access_token + personal_access_token_header('wrong token') + when :invalid_job_token + job_token_header('wrong token') + when :user_basic_auth + user_basic_auth_header(user) + when :invalid_user_basic_auth + basic_auth_header('invalid user', 'invalid password') + end + end + + def deploy_token_auth_header + case authenticate_with + when :deploy_token_rw + deploy_token_header(deploy_token_rw.token) + when :deploy_token_ro + deploy_token_header(deploy_token_ro.token) + when :deploy_token_wo + deploy_token_header(deploy_token_wo.token) + when :invalid_deploy_token + deploy_token_header('wrong token') + end + end + + def personal_access_token_header(value = nil) + { Gitlab::Auth::AuthFinders::PRIVATE_TOKEN_HEADER => value || personal_access_token.token } + end + + def job_token_header(value = nil) + { Gitlab::Auth::AuthFinders::JOB_TOKEN_HEADER => value || ci_build.token } + end + + def deploy_token_header(value) + { Gitlab::Auth::AuthFinders::DEPLOY_TOKEN_HEADER => value } + end + + shared_examples 'secure endpoint' do + before do + project.add_developer(user) + end + + it 'rejects malicious request' do + subject + + expect(response).to have_gitlab_http_status(:bad_request) + end + end + + before do + stub_feature_flags(model_registry: false) + stub_feature_flags(model_registry: project_to_enable_ff) if project_to_enable_ff.present? + end + + describe 'PUT /api/v4/projects/:id/packages/ml_models/:package_name/:package_version/:file_name/authorize' do + context 'with valid project' do + where(:project_visibility, :user_role, :member?, :authenticate_with, :expected_status) do + 'PUBLIC' | :developer | true | :personal_access_token | :success + 'PUBLIC' | :guest | true | :personal_access_token | :forbidden + 'PUBLIC' | :developer | true | :invalid_personal_access_token | :unauthorized + 'PUBLIC' | :guest | true | :invalid_personal_access_token | :unauthorized + 'PUBLIC' | :developer | true | :user_basic_auth | :success + 'PUBLIC' | :guest | true | :user_basic_auth | :forbidden + 'PUBLIC' | :developer | true | :invalid_user_basic_auth | :unauthorized + 'PUBLIC' | :guest | true | :invalid_user_basic_auth | :unauthorized + 'PUBLIC' | :developer | false | :personal_access_token | :forbidden + 'PUBLIC' | :guest | false | :personal_access_token | :forbidden + 'PUBLIC' | :developer | false | :invalid_personal_access_token | :unauthorized + 'PUBLIC' | :guest | false | :invalid_personal_access_token | :unauthorized + 'PUBLIC' | :developer | false | :user_basic_auth | :forbidden + 'PUBLIC' | :guest | false | :user_basic_auth | :forbidden + 'PUBLIC' | :developer | false | :invalid_user_basic_auth | :unauthorized + 'PUBLIC' | :guest | false | :invalid_user_basic_auth | :unauthorized + 'PUBLIC' | :anonymous | false | :none | :unauthorized + 'PRIVATE' | :developer | true | :personal_access_token | :success + 'PRIVATE' | :guest | true | :personal_access_token | :forbidden + 'PRIVATE' | :developer | true | :invalid_personal_access_token | :unauthorized + 'PRIVATE' | :guest | true | :invalid_personal_access_token | :unauthorized + 'PRIVATE' | :developer | true | :user_basic_auth | :success + 'PRIVATE' | :guest | true | :user_basic_auth | :forbidden + 'PRIVATE' | :developer | true | :invalid_user_basic_auth | :unauthorized + 'PRIVATE' | :guest | true | :invalid_user_basic_auth | :unauthorized + 'PRIVATE' | :developer | false | :personal_access_token | :not_found + 'PRIVATE' | :guest | false | :personal_access_token | :not_found + 'PRIVATE' | :developer | false | :invalid_personal_access_token | :unauthorized + 'PRIVATE' | :guest | false | :invalid_personal_access_token | :unauthorized + 'PRIVATE' | :developer | false | :user_basic_auth | :not_found + 'PRIVATE' | :guest | false | :user_basic_auth | :not_found + 'PRIVATE' | :developer | false | :invalid_user_basic_auth | :unauthorized + 'PRIVATE' | :guest | false | :invalid_user_basic_auth | :unauthorized + 'PRIVATE' | :anonymous | false | :none | :unauthorized + 'PUBLIC' | :developer | true | :job_token | :success + 'PUBLIC' | :developer | true | :invalid_job_token | :unauthorized + 'PUBLIC' | :developer | false | :job_token | :forbidden + 'PUBLIC' | :developer | false | :invalid_job_token | :unauthorized + 'PRIVATE' | :developer | true | :job_token | :success + 'PRIVATE' | :developer | true | :invalid_job_token | :unauthorized + 'PRIVATE' | :developer | false | :job_token | :not_found + 'PRIVATE' | :developer | false | :invalid_job_token | :unauthorized + end + + with_them do + before do + project.update!(visibility_level: Gitlab::VisibilityLevel.const_get(project_visibility, false)) + project.send("add_#{user_role}", user) if member? && user_role != :anonymous + end + + it "responds with #{params[:expected_status]}" do + authorize_upload_file(workhorse_headers.merge(auth_header)) + + expect(response).to have_gitlab_http_status(expected_status) + end + end + + where(:authenticate_with, :expected_status) do + :deploy_token_rw | :success + :deploy_token_wo | :success + :deploy_token_ro | :forbidden + :invalid_deploy_token | :unauthorized + end + + with_them do + it "responds with #{params[:expected_status]}" do + authorize_upload_file(workhorse_headers.merge(deploy_token_auth_header)) + + expect(response).to have_gitlab_http_status(expected_status) + end + end + + context 'when feature flag is disabled' do + let(:project_to_enable_ff) { nil } + let(:authenticate_with) { :deploy_token_rw } + + it "is not found" do + authorize_upload_file(workhorse_headers.merge(deploy_token_auth_header)) + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + context 'when feature flag is disabled for current group' do + let(:project_to_enable_ff) { another_project } + let(:authenticate_with) { :deploy_token_rw } + + it "is not found" do + authorize_upload_file(workhorse_headers.merge(deploy_token_auth_header)) + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end + + describe 'application security' do + using RSpec::Parameterized::TableSyntax + + where(:param_name, :param_value) do + :package_name | 'my-package/../' + :package_name | 'my-package%2f%2e%2e%2f' + :file_name | '../.ssh%2fauthorized_keys' + :file_name | '%2e%2e%2f.ssh%2fauthorized_keys' + end + + with_them do + subject do + authorize_upload_file(workhorse_headers.merge(personal_access_token_header), param_name => param_value) + end + + it_behaves_like 'secure endpoint' + end + end + + def authorize_upload_file(request_headers, package_name: 'mypackage', file_name: 'myfile.tar.gz') + url = "/projects/#{project.id}/packages/ml_models/#{package_name}/0.0.1/#{file_name}/authorize" + + put api(url), headers: request_headers + end + end +end diff --git a/workhorse/internal/upstream/routes.go b/workhorse/internal/upstream/routes.go index c5af9dc3f006fa..dd1725a723e515 100644 --- a/workhorse/internal/upstream/routes.go +++ b/workhorse/internal/upstream/routes.go @@ -279,6 +279,9 @@ func configureRoutes(u *upstream) { // Generic Packages Repository u.route("PUT", apiProjectPattern+`/packages/generic/`, requestBodyUploader), + // Ml Model Packages Repository + u.route("PUT", apiProjectPattern+`/packages/ml_models/`, requestBodyUploader), + // NuGet Artifact Repository u.route("PUT", apiProjectPattern+`/packages/nuget/`, mimeMultipartUploader), diff --git a/workhorse/upload_test.go b/workhorse/upload_test.go index c05af7317973b4..39aa2782873921 100644 --- a/workhorse/upload_test.go +++ b/workhorse/upload_test.go @@ -580,6 +580,7 @@ func TestPackageFilesUpload(t *testing.T) { {"PUT", "/api/v4/projects/group%2Fproject/packages/conan/v1/files"}, {"PUT", "/api/v4/projects/group%2Fproject/packages/maven/v1/files"}, {"PUT", "/api/v4/projects/group%2Fproject/packages/generic/mypackage/0.0.1/myfile.tar.gz"}, + {"PUT", "/api/v4/projects/group%2Fproject/packages/ml_models/0.0.1/myfile.tar.gz"}, {"PUT", "/api/v4/projects/group%2Fproject/packages/debian/libsample0_1.2.3~alpha2-1_amd64.deb"}, {"POST", "/api/v4/projects/group%2Fproject/packages/rubygems/api/v1/gems/sample.gem"}, {"POST", "/api/v4/projects/group%2Fproject/packages/rpm/sample-4.23.fc21.x86_64.rpm"}, -- GitLab From ab36f67dcf6742dd393ae82f0d48a31ef00bd7ab Mon Sep 17 00:00:00 2001 From: Eduardo Bonet Date: Tue, 30 May 2023 16:33:28 +0200 Subject: [PATCH 02/10] Refactors test tables --- spec/requests/api/generic_packages_spec.rb | 55 ++------------- spec/requests/api/ml_model_packages_spec.rb | 55 ++------------- .../api/generic_packages_shared_context.rb | 70 +++++++++++++++++++ 3 files changed, 80 insertions(+), 100 deletions(-) create mode 100644 spec/support/shared_contexts/requests/api/generic_packages_shared_context.rb diff --git a/spec/requests/api/generic_packages_spec.rb b/spec/requests/api/generic_packages_spec.rb index 9e8bfab646817d..1aaac42495f37e 100644 --- a/spec/requests/api/generic_packages_spec.rb +++ b/spec/requests/api/generic_packages_spec.rb @@ -78,50 +78,11 @@ def deploy_token_header(value) end describe 'PUT /api/v4/projects/:id/packages/generic/:package_name/:package_version/:file_name/authorize' do + include_context 'generic packages permission table' + context 'with valid project' do where(:project_visibility, :user_role, :member?, :authenticate_with, :expected_status) do - 'PUBLIC' | :developer | true | :personal_access_token | :success - 'PUBLIC' | :guest | true | :personal_access_token | :forbidden - 'PUBLIC' | :developer | true | :invalid_personal_access_token | :unauthorized - 'PUBLIC' | :guest | true | :invalid_personal_access_token | :unauthorized - 'PUBLIC' | :developer | true | :user_basic_auth | :success - 'PUBLIC' | :guest | true | :user_basic_auth | :forbidden - 'PUBLIC' | :developer | true | :invalid_user_basic_auth | :unauthorized - 'PUBLIC' | :guest | true | :invalid_user_basic_auth | :unauthorized - 'PUBLIC' | :developer | false | :personal_access_token | :forbidden - 'PUBLIC' | :guest | false | :personal_access_token | :forbidden - 'PUBLIC' | :developer | false | :invalid_personal_access_token | :unauthorized - 'PUBLIC' | :guest | false | :invalid_personal_access_token | :unauthorized - 'PUBLIC' | :developer | false | :user_basic_auth | :forbidden - 'PUBLIC' | :guest | false | :user_basic_auth | :forbidden - 'PUBLIC' | :developer | false | :invalid_user_basic_auth | :unauthorized - 'PUBLIC' | :guest | false | :invalid_user_basic_auth | :unauthorized - 'PUBLIC' | :anonymous | false | :none | :unauthorized - 'PRIVATE' | :developer | true | :personal_access_token | :success - 'PRIVATE' | :guest | true | :personal_access_token | :forbidden - 'PRIVATE' | :developer | true | :invalid_personal_access_token | :unauthorized - 'PRIVATE' | :guest | true | :invalid_personal_access_token | :unauthorized - 'PRIVATE' | :developer | true | :user_basic_auth | :success - 'PRIVATE' | :guest | true | :user_basic_auth | :forbidden - 'PRIVATE' | :developer | true | :invalid_user_basic_auth | :unauthorized - 'PRIVATE' | :guest | true | :invalid_user_basic_auth | :unauthorized - 'PRIVATE' | :developer | false | :personal_access_token | :not_found - 'PRIVATE' | :guest | false | :personal_access_token | :not_found - 'PRIVATE' | :developer | false | :invalid_personal_access_token | :unauthorized - 'PRIVATE' | :guest | false | :invalid_personal_access_token | :unauthorized - 'PRIVATE' | :developer | false | :user_basic_auth | :not_found - 'PRIVATE' | :guest | false | :user_basic_auth | :not_found - 'PRIVATE' | :developer | false | :invalid_user_basic_auth | :unauthorized - 'PRIVATE' | :guest | false | :invalid_user_basic_auth | :unauthorized - 'PRIVATE' | :anonymous | false | :none | :unauthorized - 'PUBLIC' | :developer | true | :job_token | :success - 'PUBLIC' | :developer | true | :invalid_job_token | :unauthorized - 'PUBLIC' | :developer | false | :job_token | :forbidden - 'PUBLIC' | :developer | false | :invalid_job_token | :unauthorized - 'PRIVATE' | :developer | true | :job_token | :success - 'PRIVATE' | :developer | true | :invalid_job_token | :unauthorized - 'PRIVATE' | :developer | false | :job_token | :not_found - 'PRIVATE' | :developer | false | :invalid_job_token | :unauthorized + generic_packages_permission_table end with_them do @@ -138,10 +99,7 @@ def deploy_token_header(value) end where(:authenticate_with, :expected_status) do - :deploy_token_rw | :success - :deploy_token_wo | :success - :deploy_token_ro | :forbidden - :invalid_deploy_token | :unauthorized + generic_packages_token_table end with_them do @@ -157,10 +115,7 @@ def deploy_token_header(value) using RSpec::Parameterized::TableSyntax where(:param_name, :param_value) do - :package_name | 'my-package/../' - :package_name | 'my-package%2f%2e%2e%2f' - :file_name | '../.ssh%2fauthorized_keys' - :file_name | '%2e%2e%2f.ssh%2fauthorized_keys' + generic_packages_naming_table end with_them do diff --git a/spec/requests/api/ml_model_packages_spec.rb b/spec/requests/api/ml_model_packages_spec.rb index e6c5fac38aba70..1e3167e19851f3 100644 --- a/spec/requests/api/ml_model_packages_spec.rb +++ b/spec/requests/api/ml_model_packages_spec.rb @@ -84,50 +84,11 @@ def deploy_token_header(value) end describe 'PUT /api/v4/projects/:id/packages/ml_models/:package_name/:package_version/:file_name/authorize' do + include_context 'generic packages permission table' + context 'with valid project' do where(:project_visibility, :user_role, :member?, :authenticate_with, :expected_status) do - 'PUBLIC' | :developer | true | :personal_access_token | :success - 'PUBLIC' | :guest | true | :personal_access_token | :forbidden - 'PUBLIC' | :developer | true | :invalid_personal_access_token | :unauthorized - 'PUBLIC' | :guest | true | :invalid_personal_access_token | :unauthorized - 'PUBLIC' | :developer | true | :user_basic_auth | :success - 'PUBLIC' | :guest | true | :user_basic_auth | :forbidden - 'PUBLIC' | :developer | true | :invalid_user_basic_auth | :unauthorized - 'PUBLIC' | :guest | true | :invalid_user_basic_auth | :unauthorized - 'PUBLIC' | :developer | false | :personal_access_token | :forbidden - 'PUBLIC' | :guest | false | :personal_access_token | :forbidden - 'PUBLIC' | :developer | false | :invalid_personal_access_token | :unauthorized - 'PUBLIC' | :guest | false | :invalid_personal_access_token | :unauthorized - 'PUBLIC' | :developer | false | :user_basic_auth | :forbidden - 'PUBLIC' | :guest | false | :user_basic_auth | :forbidden - 'PUBLIC' | :developer | false | :invalid_user_basic_auth | :unauthorized - 'PUBLIC' | :guest | false | :invalid_user_basic_auth | :unauthorized - 'PUBLIC' | :anonymous | false | :none | :unauthorized - 'PRIVATE' | :developer | true | :personal_access_token | :success - 'PRIVATE' | :guest | true | :personal_access_token | :forbidden - 'PRIVATE' | :developer | true | :invalid_personal_access_token | :unauthorized - 'PRIVATE' | :guest | true | :invalid_personal_access_token | :unauthorized - 'PRIVATE' | :developer | true | :user_basic_auth | :success - 'PRIVATE' | :guest | true | :user_basic_auth | :forbidden - 'PRIVATE' | :developer | true | :invalid_user_basic_auth | :unauthorized - 'PRIVATE' | :guest | true | :invalid_user_basic_auth | :unauthorized - 'PRIVATE' | :developer | false | :personal_access_token | :not_found - 'PRIVATE' | :guest | false | :personal_access_token | :not_found - 'PRIVATE' | :developer | false | :invalid_personal_access_token | :unauthorized - 'PRIVATE' | :guest | false | :invalid_personal_access_token | :unauthorized - 'PRIVATE' | :developer | false | :user_basic_auth | :not_found - 'PRIVATE' | :guest | false | :user_basic_auth | :not_found - 'PRIVATE' | :developer | false | :invalid_user_basic_auth | :unauthorized - 'PRIVATE' | :guest | false | :invalid_user_basic_auth | :unauthorized - 'PRIVATE' | :anonymous | false | :none | :unauthorized - 'PUBLIC' | :developer | true | :job_token | :success - 'PUBLIC' | :developer | true | :invalid_job_token | :unauthorized - 'PUBLIC' | :developer | false | :job_token | :forbidden - 'PUBLIC' | :developer | false | :invalid_job_token | :unauthorized - 'PRIVATE' | :developer | true | :job_token | :success - 'PRIVATE' | :developer | true | :invalid_job_token | :unauthorized - 'PRIVATE' | :developer | false | :job_token | :not_found - 'PRIVATE' | :developer | false | :invalid_job_token | :unauthorized + generic_packages_permission_table end with_them do @@ -144,10 +105,7 @@ def deploy_token_header(value) end where(:authenticate_with, :expected_status) do - :deploy_token_rw | :success - :deploy_token_wo | :success - :deploy_token_ro | :forbidden - :invalid_deploy_token | :unauthorized + generic_packages_token_table end with_them do @@ -185,10 +143,7 @@ def deploy_token_header(value) using RSpec::Parameterized::TableSyntax where(:param_name, :param_value) do - :package_name | 'my-package/../' - :package_name | 'my-package%2f%2e%2e%2f' - :file_name | '../.ssh%2fauthorized_keys' - :file_name | '%2e%2e%2f.ssh%2fauthorized_keys' + generic_packages_naming_table end with_them do diff --git a/spec/support/shared_contexts/requests/api/generic_packages_shared_context.rb b/spec/support/shared_contexts/requests/api/generic_packages_shared_context.rb new file mode 100644 index 00000000000000..2587f99a14d85f --- /dev/null +++ b/spec/support/shared_contexts/requests/api/generic_packages_shared_context.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +RSpec.shared_context 'generic packages permission table' do # rubocop:disable RSpec/ContextWording + using RSpec::Parameterized::TableSyntax + + # rubocop: disable Metrics/AbcSize + # :project_visibility, :user_role, :member?, :authenticate_with, :expected_status + def generic_packages_permission_table + 'PUBLIC' | :developer | true | :personal_access_token | :success + 'PUBLIC' | :guest | true | :personal_access_token | :forbidden + 'PUBLIC' | :developer | true | :invalid_personal_access_token | :unauthorized + 'PUBLIC' | :guest | true | :invalid_personal_access_token | :unauthorized + 'PUBLIC' | :developer | true | :user_basic_auth | :success + 'PUBLIC' | :guest | true | :user_basic_auth | :forbidden + 'PUBLIC' | :developer | true | :invalid_user_basic_auth | :unauthorized + 'PUBLIC' | :guest | true | :invalid_user_basic_auth | :unauthorized + 'PUBLIC' | :developer | false | :personal_access_token | :forbidden + 'PUBLIC' | :guest | false | :personal_access_token | :forbidden + 'PUBLIC' | :developer | false | :invalid_personal_access_token | :unauthorized + 'PUBLIC' | :guest | false | :invalid_personal_access_token | :unauthorized + 'PUBLIC' | :developer | false | :user_basic_auth | :forbidden + 'PUBLIC' | :guest | false | :user_basic_auth | :forbidden + 'PUBLIC' | :developer | false | :invalid_user_basic_auth | :unauthorized + 'PUBLIC' | :guest | false | :invalid_user_basic_auth | :unauthorized + 'PUBLIC' | :anonymous | false | :none | :unauthorized + 'PRIVATE' | :developer | true | :personal_access_token | :success + 'PRIVATE' | :guest | true | :personal_access_token | :forbidden + 'PRIVATE' | :developer | true | :invalid_personal_access_token | :unauthorized + 'PRIVATE' | :guest | true | :invalid_personal_access_token | :unauthorized + 'PRIVATE' | :developer | true | :user_basic_auth | :success + 'PRIVATE' | :guest | true | :user_basic_auth | :forbidden + 'PRIVATE' | :developer | true | :invalid_user_basic_auth | :unauthorized + 'PRIVATE' | :guest | true | :invalid_user_basic_auth | :unauthorized + 'PRIVATE' | :developer | false | :personal_access_token | :not_found + 'PRIVATE' | :guest | false | :personal_access_token | :not_found + 'PRIVATE' | :developer | false | :invalid_personal_access_token | :unauthorized + 'PRIVATE' | :guest | false | :invalid_personal_access_token | :unauthorized + 'PRIVATE' | :developer | false | :user_basic_auth | :not_found + 'PRIVATE' | :guest | false | :user_basic_auth | :not_found + 'PRIVATE' | :developer | false | :invalid_user_basic_auth | :unauthorized + 'PRIVATE' | :guest | false | :invalid_user_basic_auth | :unauthorized + 'PRIVATE' | :anonymous | false | :none | :unauthorized + 'PUBLIC' | :developer | true | :job_token | :success + 'PUBLIC' | :developer | true | :invalid_job_token | :unauthorized + 'PUBLIC' | :developer | false | :job_token | :forbidden + 'PUBLIC' | :developer | false | :invalid_job_token | :unauthorized + 'PRIVATE' | :developer | true | :job_token | :success + 'PRIVATE' | :developer | true | :invalid_job_token | :unauthorized + 'PRIVATE' | :developer | false | :job_token | :not_found + 'PRIVATE' | :developer | false | :invalid_job_token | :unauthorized + end + + # :authenticate_with, :expected_status + def generic_packages_token_table + :deploy_token_rw | :success + :deploy_token_wo | :success + :deploy_token_ro | :forbidden + :invalid_deploy_token | :unauthorized + end + + # :param_name, :param_value + def generic_packages_naming_table + :package_name | 'my-package/../' + :package_name | 'my-package%2f%2e%2e%2f' + :file_name | '../.ssh%2fauthorized_keys' + :file_name | '%2e%2e%2f.ssh%2fauthorized_keys' + end + + # rubocop: enable Metrics/AbcSize +end -- GitLab From ed3126a44a09a16a6c83444e5f4dc0d26683e2fc Mon Sep 17 00:00:00 2001 From: Eduardo Bonet Date: Tue, 30 May 2023 16:58:11 +0200 Subject: [PATCH 03/10] Uses better regexes --- lib/gitlab/regex.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/regex.rb b/lib/gitlab/regex.rb index 559b577abe031f..26ca9d2547c862 100644 --- a/lib/gitlab/regex.rb +++ b/lib/gitlab/regex.rb @@ -629,11 +629,11 @@ def ml_model_version_regex end def ml_model_name_regex - maven_file_name_regex + package_name_regex end def ml_model_file_name_regex - generic_package_name_regex + maven_file_name_regex end end end -- GitLab From 8ca4cc7402f9896ac6b417fb4d0a082f7d2748b3 Mon Sep 17 00:00:00 2001 From: Eduardo Bonet Date: Wed, 31 May 2023 10:56:11 +0200 Subject: [PATCH 04/10] Removes unused code --- lib/api/ml_model_packages.rb | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/lib/api/ml_model_packages.rb b/lib/api/ml_model_packages.rb index baed1103d373ed..b95854fa207b33 100644 --- a/lib/api/ml_model_packages.rb +++ b/lib/api/ml_model_packages.rb @@ -14,14 +14,6 @@ class MlModelPackages < ::API::Base feature_category :mlops urgency :low - rescue_from ArgumentError do |e| - render_api_error!(e.message, 400) - end - - rescue_from ActiveRecord::RecordInvalid do |e| - render_api_error!(e.message, 400) - end - before do require_packages_enabled! authenticate_non_get! @@ -73,10 +65,6 @@ class MlModelPackages < ::API::Base helpers do include ::API::Helpers::PackagesHelpers include ::API::Helpers::Packages::BasicAuthHelpers - - def max_file_size_exceeded? - authorized_user_project.actual_limits.exceeded?(:ml_model_max_file_size, params[:file].size) - end end end end -- GitLab From f4f3e12a6ebf8c023d109e370e9d2c7d89860dd6 Mon Sep 17 00:00:00 2001 From: Eduardo Bonet Date: Thu, 1 Jun 2023 11:18:40 +0200 Subject: [PATCH 05/10] Uses autenticate_with for ml_model_packages api --- lib/api/ml_model_packages.rb | 24 +-- spec/requests/api/generic_packages_spec.rb | 55 +++++- spec/requests/api/ml_model_packages_spec.rb | 165 ++++++++---------- .../api/generic_packages_shared_context.rb | 70 -------- 4 files changed, 138 insertions(+), 176 deletions(-) delete mode 100644 spec/support/shared_contexts/requests/api/generic_packages_shared_context.rb diff --git a/lib/api/ml_model_packages.rb b/lib/api/ml_model_packages.rb index b95854fa207b33..d944e6fbed53be 100644 --- a/lib/api/ml_model_packages.rb +++ b/lib/api/ml_model_packages.rb @@ -3,6 +3,7 @@ module API class MlModelPackages < ::API::Base include APIGuard + include ::API::Helpers::Authentication ML_MODEL_PACKAGES_REQUIREMENTS = { package_name: API::NO_SLASH_URL_PART_REGEX, @@ -14,21 +15,28 @@ class MlModelPackages < ::API::Base feature_category :mlops urgency :low - before do + after_validation do require_packages_enabled! authenticate_non_get! not_found! unless can?(current_user, :read_model_registry, user_project) end + authenticate_with do |accept| + accept.token_types(:personal_access_token, :deploy_token, :job_token) + .sent_through(:http_token) + end + + helpers do + include ::API::Helpers::PackagesHelpers + include ::API::Helpers::Packages::BasicAuthHelpers + end + params do requires :id, types: [String, Integer], desc: 'The ID or URL-encoded path of the project' end resource :projects, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do - route_setting :authentication, job_token_allowed: true, basic_auth_personal_access_token: true, - deploy_token_allowed: true - namespace ':id/packages/ml_models' do namespace ':package_name/*package_version/:file_name', requirements: ML_MODEL_PACKAGES_REQUIREMENTS do desc 'Workhorse authorize model package file' do @@ -42,9 +50,6 @@ class MlModelPackages < ::API::Base tags %w[ml_model_registry] end - route_setting :authentication, job_token_allowed: true, basic_auth_personal_access_token: true, - deploy_token_allowed: true - params do requires :package_name, type: String, desc: 'Package name', regexp: Gitlab::Regex.ml_model_name_regex, file_path: true @@ -61,10 +66,5 @@ class MlModelPackages < ::API::Base end end end - - helpers do - include ::API::Helpers::PackagesHelpers - include ::API::Helpers::Packages::BasicAuthHelpers - end end end diff --git a/spec/requests/api/generic_packages_spec.rb b/spec/requests/api/generic_packages_spec.rb index 1aaac42495f37e..9e8bfab646817d 100644 --- a/spec/requests/api/generic_packages_spec.rb +++ b/spec/requests/api/generic_packages_spec.rb @@ -78,11 +78,50 @@ def deploy_token_header(value) end describe 'PUT /api/v4/projects/:id/packages/generic/:package_name/:package_version/:file_name/authorize' do - include_context 'generic packages permission table' - context 'with valid project' do where(:project_visibility, :user_role, :member?, :authenticate_with, :expected_status) do - generic_packages_permission_table + 'PUBLIC' | :developer | true | :personal_access_token | :success + 'PUBLIC' | :guest | true | :personal_access_token | :forbidden + 'PUBLIC' | :developer | true | :invalid_personal_access_token | :unauthorized + 'PUBLIC' | :guest | true | :invalid_personal_access_token | :unauthorized + 'PUBLIC' | :developer | true | :user_basic_auth | :success + 'PUBLIC' | :guest | true | :user_basic_auth | :forbidden + 'PUBLIC' | :developer | true | :invalid_user_basic_auth | :unauthorized + 'PUBLIC' | :guest | true | :invalid_user_basic_auth | :unauthorized + 'PUBLIC' | :developer | false | :personal_access_token | :forbidden + 'PUBLIC' | :guest | false | :personal_access_token | :forbidden + 'PUBLIC' | :developer | false | :invalid_personal_access_token | :unauthorized + 'PUBLIC' | :guest | false | :invalid_personal_access_token | :unauthorized + 'PUBLIC' | :developer | false | :user_basic_auth | :forbidden + 'PUBLIC' | :guest | false | :user_basic_auth | :forbidden + 'PUBLIC' | :developer | false | :invalid_user_basic_auth | :unauthorized + 'PUBLIC' | :guest | false | :invalid_user_basic_auth | :unauthorized + 'PUBLIC' | :anonymous | false | :none | :unauthorized + 'PRIVATE' | :developer | true | :personal_access_token | :success + 'PRIVATE' | :guest | true | :personal_access_token | :forbidden + 'PRIVATE' | :developer | true | :invalid_personal_access_token | :unauthorized + 'PRIVATE' | :guest | true | :invalid_personal_access_token | :unauthorized + 'PRIVATE' | :developer | true | :user_basic_auth | :success + 'PRIVATE' | :guest | true | :user_basic_auth | :forbidden + 'PRIVATE' | :developer | true | :invalid_user_basic_auth | :unauthorized + 'PRIVATE' | :guest | true | :invalid_user_basic_auth | :unauthorized + 'PRIVATE' | :developer | false | :personal_access_token | :not_found + 'PRIVATE' | :guest | false | :personal_access_token | :not_found + 'PRIVATE' | :developer | false | :invalid_personal_access_token | :unauthorized + 'PRIVATE' | :guest | false | :invalid_personal_access_token | :unauthorized + 'PRIVATE' | :developer | false | :user_basic_auth | :not_found + 'PRIVATE' | :guest | false | :user_basic_auth | :not_found + 'PRIVATE' | :developer | false | :invalid_user_basic_auth | :unauthorized + 'PRIVATE' | :guest | false | :invalid_user_basic_auth | :unauthorized + 'PRIVATE' | :anonymous | false | :none | :unauthorized + 'PUBLIC' | :developer | true | :job_token | :success + 'PUBLIC' | :developer | true | :invalid_job_token | :unauthorized + 'PUBLIC' | :developer | false | :job_token | :forbidden + 'PUBLIC' | :developer | false | :invalid_job_token | :unauthorized + 'PRIVATE' | :developer | true | :job_token | :success + 'PRIVATE' | :developer | true | :invalid_job_token | :unauthorized + 'PRIVATE' | :developer | false | :job_token | :not_found + 'PRIVATE' | :developer | false | :invalid_job_token | :unauthorized end with_them do @@ -99,7 +138,10 @@ def deploy_token_header(value) end where(:authenticate_with, :expected_status) do - generic_packages_token_table + :deploy_token_rw | :success + :deploy_token_wo | :success + :deploy_token_ro | :forbidden + :invalid_deploy_token | :unauthorized end with_them do @@ -115,7 +157,10 @@ def deploy_token_header(value) using RSpec::Parameterized::TableSyntax where(:param_name, :param_value) do - generic_packages_naming_table + :package_name | 'my-package/../' + :package_name | 'my-package%2f%2e%2e%2f' + :file_name | '../.ssh%2fauthorized_keys' + :file_name | '%2e%2e%2f.ssh%2fauthorized_keys' end with_them do diff --git a/spec/requests/api/ml_model_packages_spec.rb b/spec/requests/api/ml_model_packages_spec.rb index 1e3167e19851f3..bf76d3d7f4c22e 100644 --- a/spec/requests/api/ml_model_packages_spec.rb +++ b/spec/requests/api/ml_model_packages_spec.rb @@ -8,63 +8,25 @@ include_context 'workhorse headers' - let_it_be(:personal_access_token) { create(:personal_access_token) } let_it_be(:project, reload: true) { create(:project) } + let_it_be(:personal_access_token) { create(:personal_access_token) } + let_it_be(:job) { create(:ci_build, :running, user: personal_access_token.user, project: project) } + let_it_be(:deploy_token) { create(:deploy_token, read_package_registry: true, write_package_registry: true) } + let_it_be(:project_deploy_token) { create(:project_deploy_token, deploy_token: deploy_token, project: project) } let_it_be(:another_project, reload: true) { create(:project) } - let_it_be(:deploy_token_rw) { create(:deploy_token, read_package_registry: true, write_package_registry: true) } - let_it_be(:project_deploy_token_rw) { create(:project_deploy_token, deploy_token: deploy_token_rw, project: project) } - let_it_be(:deploy_token_ro) { create(:deploy_token, read_package_registry: true, write_package_registry: false) } - let_it_be(:project_deploy_token_ro) { create(:project_deploy_token, deploy_token: deploy_token_ro, project: project) } - let_it_be(:deploy_token_wo) { create(:deploy_token, read_package_registry: false, write_package_registry: true) } - let_it_be(:project_deploy_token_wo) { create(:project_deploy_token, deploy_token: deploy_token_wo, project: project) } + + let_it_be(:tokens) do + { + personal_access_token: personal_access_token.token, + deploy_token: deploy_token.token, + job_token: job.token + } + end let(:user) { personal_access_token.user } let(:ci_build) { create(:ci_build, :running, user: user, project: project) } let(:project_to_enable_ff) { project } - - def auth_header - return {} if user_role == :anonymous - - case authenticate_with - when :personal_access_token - personal_access_token_header - when :job_token - job_token_header - when :invalid_personal_access_token - personal_access_token_header('wrong token') - when :invalid_job_token - job_token_header('wrong token') - when :user_basic_auth - user_basic_auth_header(user) - when :invalid_user_basic_auth - basic_auth_header('invalid user', 'invalid password') - end - end - - def deploy_token_auth_header - case authenticate_with - when :deploy_token_rw - deploy_token_header(deploy_token_rw.token) - when :deploy_token_ro - deploy_token_header(deploy_token_ro.token) - when :deploy_token_wo - deploy_token_header(deploy_token_wo.token) - when :invalid_deploy_token - deploy_token_header('wrong token') - end - end - - def personal_access_token_header(value = nil) - { Gitlab::Auth::AuthFinders::PRIVATE_TOKEN_HEADER => value || personal_access_token.token } - end - - def job_token_header(value = nil) - { Gitlab::Auth::AuthFinders::JOB_TOKEN_HEADER => value || ci_build.token } - end - - def deploy_token_header(value) - { Gitlab::Auth::AuthFinders::DEPLOY_TOKEN_HEADER => value } - end + let(:headers) { {} } shared_examples 'secure endpoint' do before do @@ -72,9 +34,7 @@ def deploy_token_header(value) end it 'rejects malicious request' do - subject - - expect(response).to have_gitlab_http_status(:bad_request) + is_expected.to have_gitlab_http_status(:bad_request) end end @@ -84,57 +44,83 @@ def deploy_token_header(value) end describe 'PUT /api/v4/projects/:id/packages/ml_models/:package_name/:package_version/:file_name/authorize' do - include_context 'generic packages permission table' + let(:token) { tokens[:personal_access_token] } + let(:user_headers) { { 'HTTP_AUTHORIZATION' => token } } + let(:headers) { user_headers.merge(workhorse_headers) } + let(:request) { authorize_upload_file(headers) } + + subject(:api_response) do + request + response + end context 'with valid project' do - where(:project_visibility, :user_role, :member?, :authenticate_with, :expected_status) do - generic_packages_permission_table + where(:visibility, :user_role, :member, :token_type, :valid_token, :expected_status) do + :public | :developer | true | :personal_access_token | true | :success + :public | :guest | true | :personal_access_token | true | :forbidden + :public | :developer | true | :personal_access_token | false | :unauthorized + :public | :guest | true | :personal_access_token | false | :unauthorized + :public | :developer | false | :personal_access_token | true | :forbidden + :public | :guest | false | :personal_access_token | true | :forbidden + :public | :developer | false | :personal_access_token | false | :unauthorized + :public | :guest | false | :personal_access_token | false | :unauthorized + :public | :anonymous | false | :personal_access_token | true | :unauthorized + :private | :developer | true | :personal_access_token | true | :success + :private | :guest | true | :personal_access_token | true | :forbidden + :private | :developer | true | :personal_access_token | false | :unauthorized + :private | :guest | true | :personal_access_token | false | :unauthorized + :private | :developer | false | :personal_access_token | true | :not_found + :private | :guest | false | :personal_access_token | true | :not_found + :private | :developer | false | :personal_access_token | false | :unauthorized + :private | :guest | false | :personal_access_token | false | :unauthorized + :private | :anonymous | false | :personal_access_token | true | :unauthorized + :public | :developer | true | :job_token | true | :success + :public | :guest | true | :job_token | true | :forbidden + :public | :developer | true | :job_token | false | :unauthorized + :public | :guest | true | :job_token | false | :unauthorized + :public | :developer | false | :job_token | true | :forbidden + :public | :guest | false | :job_token | true | :forbidden + :public | :developer | false | :job_token | false | :unauthorized + :public | :guest | false | :job_token | false | :unauthorized + :private | :developer | true | :job_token | true | :success + :private | :guest | true | :job_token | true | :forbidden + :private | :developer | true | :job_token | false | :unauthorized + :private | :guest | true | :job_token | false | :unauthorized + :private | :developer | false | :job_token | true | :not_found + :private | :guest | false | :job_token | true | :not_found + :private | :developer | false | :job_token | false | :unauthorized + :private | :guest | false | :job_token | false | :unauthorized + :public | :developer | true | :deploy_token | true | :success + :public | :developer | true | :deploy_token | false | :unauthorized + :private | :developer | true | :deploy_token | true | :success + :private | :developer | true | :deploy_token | false | :unauthorized end with_them do - before do - project.update!(visibility_level: Gitlab::VisibilityLevel.const_get(project_visibility, false)) - project.send("add_#{user_role}", user) if member? && user_role != :anonymous - end + let(:token) { valid_token ? tokens[token_type] : 'invalid-token123' } + let(:user_headers) { user_role == :anonymous ? {} : { 'HTTP_AUTHORIZATION' => token } } - it "responds with #{params[:expected_status]}" do - authorize_upload_file(workhorse_headers.merge(auth_header)) - - expect(response).to have_gitlab_http_status(expected_status) + before do + project.update_column(:visibility_level, Gitlab::VisibilityLevel.level_value(visibility.to_s)) + project.send("add_#{user_role}", user) if member && user_role != :anonymous end - end - where(:authenticate_with, :expected_status) do - generic_packages_token_table - end - - with_them do - it "responds with #{params[:expected_status]}" do - authorize_upload_file(workhorse_headers.merge(deploy_token_auth_header)) - - expect(response).to have_gitlab_http_status(expected_status) - end + it { is_expected.to have_gitlab_http_status(expected_status) } end context 'when feature flag is disabled' do let(:project_to_enable_ff) { nil } - let(:authenticate_with) { :deploy_token_rw } it "is not found" do - authorize_upload_file(workhorse_headers.merge(deploy_token_auth_header)) - - expect(response).to have_gitlab_http_status(:not_found) + is_expected.to have_gitlab_http_status(:not_found) end end - context 'when feature flag is disabled for current group' do + context 'when feature flag is disabled for current project' do let(:project_to_enable_ff) { another_project } - let(:authenticate_with) { :deploy_token_rw } it "is not found" do - authorize_upload_file(workhorse_headers.merge(deploy_token_auth_header)) - - expect(response).to have_gitlab_http_status(:not_found) + is_expected.to have_gitlab_http_status(:not_found) end end end @@ -143,13 +129,14 @@ def deploy_token_header(value) using RSpec::Parameterized::TableSyntax where(:param_name, :param_value) do - generic_packages_naming_table + :package_name | 'my-package/../' + :package_name | 'my-package%2f%2e%2e%2f' + :file_name | '../.ssh%2fauthorized_keys' + :file_name | '%2e%2e%2f.ssh%2fauthorized_keys' end with_them do - subject do - authorize_upload_file(workhorse_headers.merge(personal_access_token_header), param_name => param_value) - end + let(:request) { authorize_upload_file(headers, param_name => param_value) } it_behaves_like 'secure endpoint' end diff --git a/spec/support/shared_contexts/requests/api/generic_packages_shared_context.rb b/spec/support/shared_contexts/requests/api/generic_packages_shared_context.rb deleted file mode 100644 index 2587f99a14d85f..00000000000000 --- a/spec/support/shared_contexts/requests/api/generic_packages_shared_context.rb +++ /dev/null @@ -1,70 +0,0 @@ -# frozen_string_literal: true - -RSpec.shared_context 'generic packages permission table' do # rubocop:disable RSpec/ContextWording - using RSpec::Parameterized::TableSyntax - - # rubocop: disable Metrics/AbcSize - # :project_visibility, :user_role, :member?, :authenticate_with, :expected_status - def generic_packages_permission_table - 'PUBLIC' | :developer | true | :personal_access_token | :success - 'PUBLIC' | :guest | true | :personal_access_token | :forbidden - 'PUBLIC' | :developer | true | :invalid_personal_access_token | :unauthorized - 'PUBLIC' | :guest | true | :invalid_personal_access_token | :unauthorized - 'PUBLIC' | :developer | true | :user_basic_auth | :success - 'PUBLIC' | :guest | true | :user_basic_auth | :forbidden - 'PUBLIC' | :developer | true | :invalid_user_basic_auth | :unauthorized - 'PUBLIC' | :guest | true | :invalid_user_basic_auth | :unauthorized - 'PUBLIC' | :developer | false | :personal_access_token | :forbidden - 'PUBLIC' | :guest | false | :personal_access_token | :forbidden - 'PUBLIC' | :developer | false | :invalid_personal_access_token | :unauthorized - 'PUBLIC' | :guest | false | :invalid_personal_access_token | :unauthorized - 'PUBLIC' | :developer | false | :user_basic_auth | :forbidden - 'PUBLIC' | :guest | false | :user_basic_auth | :forbidden - 'PUBLIC' | :developer | false | :invalid_user_basic_auth | :unauthorized - 'PUBLIC' | :guest | false | :invalid_user_basic_auth | :unauthorized - 'PUBLIC' | :anonymous | false | :none | :unauthorized - 'PRIVATE' | :developer | true | :personal_access_token | :success - 'PRIVATE' | :guest | true | :personal_access_token | :forbidden - 'PRIVATE' | :developer | true | :invalid_personal_access_token | :unauthorized - 'PRIVATE' | :guest | true | :invalid_personal_access_token | :unauthorized - 'PRIVATE' | :developer | true | :user_basic_auth | :success - 'PRIVATE' | :guest | true | :user_basic_auth | :forbidden - 'PRIVATE' | :developer | true | :invalid_user_basic_auth | :unauthorized - 'PRIVATE' | :guest | true | :invalid_user_basic_auth | :unauthorized - 'PRIVATE' | :developer | false | :personal_access_token | :not_found - 'PRIVATE' | :guest | false | :personal_access_token | :not_found - 'PRIVATE' | :developer | false | :invalid_personal_access_token | :unauthorized - 'PRIVATE' | :guest | false | :invalid_personal_access_token | :unauthorized - 'PRIVATE' | :developer | false | :user_basic_auth | :not_found - 'PRIVATE' | :guest | false | :user_basic_auth | :not_found - 'PRIVATE' | :developer | false | :invalid_user_basic_auth | :unauthorized - 'PRIVATE' | :guest | false | :invalid_user_basic_auth | :unauthorized - 'PRIVATE' | :anonymous | false | :none | :unauthorized - 'PUBLIC' | :developer | true | :job_token | :success - 'PUBLIC' | :developer | true | :invalid_job_token | :unauthorized - 'PUBLIC' | :developer | false | :job_token | :forbidden - 'PUBLIC' | :developer | false | :invalid_job_token | :unauthorized - 'PRIVATE' | :developer | true | :job_token | :success - 'PRIVATE' | :developer | true | :invalid_job_token | :unauthorized - 'PRIVATE' | :developer | false | :job_token | :not_found - 'PRIVATE' | :developer | false | :invalid_job_token | :unauthorized - end - - # :authenticate_with, :expected_status - def generic_packages_token_table - :deploy_token_rw | :success - :deploy_token_wo | :success - :deploy_token_ro | :forbidden - :invalid_deploy_token | :unauthorized - end - - # :param_name, :param_value - def generic_packages_naming_table - :package_name | 'my-package/../' - :package_name | 'my-package%2f%2e%2e%2f' - :file_name | '../.ssh%2fauthorized_keys' - :file_name | '%2e%2e%2f.ssh%2fauthorized_keys' - end - - # rubocop: enable Metrics/AbcSize -end -- GitLab From 4d0e44588e17dc389f56d93456d3f706e94c3a30 Mon Sep 17 00:00:00 2001 From: Eduardo Bonet Date: Fri, 2 Jun 2023 14:28:55 +0200 Subject: [PATCH 06/10] Adds Upload endpoint --- lib/api/ml_model_packages.rb | 61 ++++- spec/requests/api/ml_model_packages_spec.rb | 234 ++++++++++++++------ 2 files changed, 215 insertions(+), 80 deletions(-) diff --git a/lib/api/ml_model_packages.rb b/lib/api/ml_model_packages.rb index d944e6fbed53be..0df5104394f405 100644 --- a/lib/api/ml_model_packages.rb +++ b/lib/api/ml_model_packages.rb @@ -30,6 +30,14 @@ class MlModelPackages < ::API::Base helpers do include ::API::Helpers::PackagesHelpers include ::API::Helpers::Packages::BasicAuthHelpers + + def project + authorized_user_project + end + + def max_file_size_exceeded? + project.actual_limits.exceeded?(:ml_model_max_file_size, params[:file].size) + end end params do @@ -38,6 +46,15 @@ class MlModelPackages < ::API::Base resource :projects, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do namespace ':id/packages/ml_models' do + params do + requires :package_name, type: String, desc: 'Package name', regexp: Gitlab::Regex.ml_model_name_regex, + file_path: true + requires :package_version, type: String, desc: 'Package version', + regexp: Gitlab::Regex.ml_model_version_regex + requires :file_name, type: String, desc: 'Package file name', + regexp: Gitlab::Regex.ml_model_file_name_regex, file_path: true + optional :status, type: String, values: ALLOWED_STATUSES, desc: 'Package status' + end namespace ':package_name/*package_version/:file_name', requirements: ML_MODEL_PACKAGES_REQUIREMENTS do desc 'Workhorse authorize model package file' do detail 'Introduced in GitLab 16.1' @@ -49,19 +66,45 @@ class MlModelPackages < ::API::Base ] tags %w[ml_model_registry] end + put 'authorize' do + authorize_workhorse!(subject: project, maximum_size: project.actual_limits.ml_model_max_file_size) + end + desc 'Workhorse upload model package file' do + detail 'Introduced in GitLab 16.1' + success code: 201 + failure [ + { code: 401, message: 'Unauthorized' }, + { code: 403, message: 'Forbidden' }, + { code: 404, message: 'Not Found' } + ] + tags %w[ml_model_registry] + end params do - requires :package_name, type: String, desc: 'Package name', regexp: Gitlab::Regex.ml_model_name_regex, - file_path: true - requires :package_version, type: String, desc: 'Package version', - regexp: Gitlab::Regex.ml_model_version_regex - requires :file_name, type: String, desc: 'Package file name', - regexp: Gitlab::Regex.ml_model_file_name_regex, file_path: true - optional :status, type: String, values: ALLOWED_STATUSES, desc: 'Package status' + requires :file, + type: ::API::Validations::Types::WorkhorseFile, + desc: 'The package file to be published (generated by Multipart middleware)', + documentation: { type: 'file' } end + put do + authorize_upload!(project) - put 'authorize' do - authorize_workhorse!(subject: user_project, maximum_size: user_project.actual_limits.ml_model_max_file_size) + bad_request!('File is too large') if max_file_size_exceeded? + + create_package_file_params = declared(params).merge(build: current_authenticated_job) + package_file = ::Packages::MlModel::CreatePackageFileService + .new(project, current_user, create_package_file_params) + .execute + + bad_request!('Package creation failed') unless package_file + + created! + rescue ObjectStorage::RemoteStoreError => e + Gitlab::ErrorTracking.track_exception(e, extra: { file_name: params[:file_name], project_id: project.id }) + + forbidden! + rescue ::Packages::DuplicatePackageError + bad_request!('Duplicate package is not allowed') end end end diff --git a/spec/requests/api/ml_model_packages_spec.rb b/spec/requests/api/ml_model_packages_spec.rb index bf76d3d7f4c22e..016dbed98964e7 100644 --- a/spec/requests/api/ml_model_packages_spec.rb +++ b/spec/requests/api/ml_model_packages_spec.rb @@ -4,6 +4,8 @@ RSpec.describe ::API::MlModelPackages, feature_category: :mlops do include HttpBasicAuthHelpers + include PackagesManagerApiSpecHelpers + include WorkhorseHelpers using RSpec::Parameterized::TableSyntax include_context 'workhorse headers' @@ -24,76 +26,99 @@ end let(:user) { personal_access_token.user } + let(:user_role) { :developer } + let(:member) { true } let(:ci_build) { create(:ci_build, :running, user: user, project: project) } let(:project_to_enable_ff) { project } let(:headers) { {} } - shared_examples 'secure endpoint' do - before do - project.add_developer(user) + shared_examples 'not found if ml_model_registry is disabled' do + context 'when feature flag is disabled' do + let(:project_to_enable_ff) { nil } + + it "is not found" do + is_expected.to have_gitlab_http_status(:not_found) + end + end + + context 'when feature flag is disabled for current project' do + let(:project_to_enable_ff) { another_project } + + it "is not found" do + is_expected.to have_gitlab_http_status(:not_found) + end end + end - it 'rejects malicious request' do - is_expected.to have_gitlab_http_status(:bad_request) + shared_context 'ml model authorize permissions table' do # rubocop:disable RSpec/ContextWording + # rubocop:disable Metrics/AbcSize + # :visibility, :user_role, :member, :token_type, :valid_token, :expected_status + def authorize_permissions_table + :public | :developer | true | :personal_access_token | true | :success + :public | :guest | true | :personal_access_token | true | :forbidden + :public | :developer | true | :personal_access_token | false | :unauthorized + :public | :guest | true | :personal_access_token | false | :unauthorized + :public | :developer | false | :personal_access_token | true | :forbidden + :public | :guest | false | :personal_access_token | true | :forbidden + :public | :developer | false | :personal_access_token | false | :unauthorized + :public | :guest | false | :personal_access_token | false | :unauthorized + :public | :anonymous | false | :personal_access_token | true | :unauthorized + :private | :developer | true | :personal_access_token | true | :success + :private | :guest | true | :personal_access_token | true | :forbidden + :private | :developer | true | :personal_access_token | false | :unauthorized + :private | :guest | true | :personal_access_token | false | :unauthorized + :private | :developer | false | :personal_access_token | true | :not_found + :private | :guest | false | :personal_access_token | true | :not_found + :private | :developer | false | :personal_access_token | false | :unauthorized + :private | :guest | false | :personal_access_token | false | :unauthorized + :private | :anonymous | false | :personal_access_token | true | :unauthorized + :public | :developer | true | :job_token | true | :success + :public | :guest | true | :job_token | true | :forbidden + :public | :developer | true | :job_token | false | :unauthorized + :public | :guest | true | :job_token | false | :unauthorized + :public | :developer | false | :job_token | true | :forbidden + :public | :guest | false | :job_token | true | :forbidden + :public | :developer | false | :job_token | false | :unauthorized + :public | :guest | false | :job_token | false | :unauthorized + :private | :developer | true | :job_token | true | :success + :private | :guest | true | :job_token | true | :forbidden + :private | :developer | true | :job_token | false | :unauthorized + :private | :guest | true | :job_token | false | :unauthorized + :private | :developer | false | :job_token | true | :not_found + :private | :guest | false | :job_token | true | :not_found + :private | :developer | false | :job_token | false | :unauthorized + :private | :guest | false | :job_token | false | :unauthorized + :public | :developer | true | :deploy_token | true | :success + :public | :developer | true | :deploy_token | false | :unauthorized + :private | :developer | true | :deploy_token | true | :success + :private | :developer | true | :deploy_token | false | :unauthorized end + # rubocop:enable Metrics/AbcSize end before do stub_feature_flags(model_registry: false) stub_feature_flags(model_registry: project_to_enable_ff) if project_to_enable_ff.present? + + project.send("add_#{user_role}", user) if member && user_role != :anonymous + end + + subject(:api_response) do + request + response end describe 'PUT /api/v4/projects/:id/packages/ml_models/:package_name/:package_version/:file_name/authorize' do + include_context 'ml model authorize permissions table' + let(:token) { tokens[:personal_access_token] } let(:user_headers) { { 'HTTP_AUTHORIZATION' => token } } let(:headers) { user_headers.merge(workhorse_headers) } let(:request) { authorize_upload_file(headers) } - subject(:api_response) do - request - response - end - - context 'with valid project' do + describe 'user access' do where(:visibility, :user_role, :member, :token_type, :valid_token, :expected_status) do - :public | :developer | true | :personal_access_token | true | :success - :public | :guest | true | :personal_access_token | true | :forbidden - :public | :developer | true | :personal_access_token | false | :unauthorized - :public | :guest | true | :personal_access_token | false | :unauthorized - :public | :developer | false | :personal_access_token | true | :forbidden - :public | :guest | false | :personal_access_token | true | :forbidden - :public | :developer | false | :personal_access_token | false | :unauthorized - :public | :guest | false | :personal_access_token | false | :unauthorized - :public | :anonymous | false | :personal_access_token | true | :unauthorized - :private | :developer | true | :personal_access_token | true | :success - :private | :guest | true | :personal_access_token | true | :forbidden - :private | :developer | true | :personal_access_token | false | :unauthorized - :private | :guest | true | :personal_access_token | false | :unauthorized - :private | :developer | false | :personal_access_token | true | :not_found - :private | :guest | false | :personal_access_token | true | :not_found - :private | :developer | false | :personal_access_token | false | :unauthorized - :private | :guest | false | :personal_access_token | false | :unauthorized - :private | :anonymous | false | :personal_access_token | true | :unauthorized - :public | :developer | true | :job_token | true | :success - :public | :guest | true | :job_token | true | :forbidden - :public | :developer | true | :job_token | false | :unauthorized - :public | :guest | true | :job_token | false | :unauthorized - :public | :developer | false | :job_token | true | :forbidden - :public | :guest | false | :job_token | true | :forbidden - :public | :developer | false | :job_token | false | :unauthorized - :public | :guest | false | :job_token | false | :unauthorized - :private | :developer | true | :job_token | true | :success - :private | :guest | true | :job_token | true | :forbidden - :private | :developer | true | :job_token | false | :unauthorized - :private | :guest | true | :job_token | false | :unauthorized - :private | :developer | false | :job_token | true | :not_found - :private | :guest | false | :job_token | true | :not_found - :private | :developer | false | :job_token | false | :unauthorized - :private | :guest | false | :job_token | false | :unauthorized - :public | :developer | true | :deploy_token | true | :success - :public | :developer | true | :deploy_token | false | :unauthorized - :private | :developer | true | :deploy_token | true | :success - :private | :developer | true | :deploy_token | false | :unauthorized + authorize_permissions_table end with_them do @@ -102,32 +127,15 @@ before do project.update_column(:visibility_level, Gitlab::VisibilityLevel.level_value(visibility.to_s)) - project.send("add_#{user_role}", user) if member && user_role != :anonymous end it { is_expected.to have_gitlab_http_status(expected_status) } end - context 'when feature flag is disabled' do - let(:project_to_enable_ff) { nil } - - it "is not found" do - is_expected.to have_gitlab_http_status(:not_found) - end - end - - context 'when feature flag is disabled for current project' do - let(:project_to_enable_ff) { another_project } - - it "is not found" do - is_expected.to have_gitlab_http_status(:not_found) - end - end + it_behaves_like 'not found if ml_model_registry is disabled' end describe 'application security' do - using RSpec::Parameterized::TableSyntax - where(:param_name, :param_value) do :package_name | 'my-package/../' :package_name | 'my-package%2f%2e%2e%2f' @@ -138,14 +146,98 @@ with_them do let(:request) { authorize_upload_file(headers, param_name => param_value) } - it_behaves_like 'secure endpoint' + it 'rejects malicious request' do + is_expected.to have_gitlab_http_status(:bad_request) + end end end + end + + describe 'PUT /api/v4/projects/:id/packages/ml_models/:package_name/:package_version/:file_name' do + include_context 'ml model authorize permissions table' - def authorize_upload_file(request_headers, package_name: 'mypackage', file_name: 'myfile.tar.gz') - url = "/projects/#{project.id}/packages/ml_models/#{package_name}/0.0.1/#{file_name}/authorize" + let_it_be(:file_name) { 'model.md5' } - put api(url), headers: request_headers + let(:token) { tokens[:personal_access_token] } + let(:user_headers) { { 'HTTP_AUTHORIZATION' => token } } + let(:headers) { user_headers.merge(workhorse_headers) } + let(:params) { { file: temp_file(file_name) } } + let(:file_key) { :file } + let(:send_rewritten_field) { true } + + let(:request) do + upload_file(headers) + end + + describe 'success' do + it 'creates a new package' do + expect { api_response }.to change { Packages::PackageFile.count }.by(1) + expect(api_response).to have_gitlab_http_status(:created) + end end + + describe 'user access' do + where(:visibility, :user_role, :member, :token_type, :valid_token, :expected_status) do + authorize_permissions_table + end + + with_them do + let(:token) { valid_token ? tokens[token_type] : 'invalid-token123' } + let(:user_headers) { user_role == :anonymous ? {} : { 'HTTP_AUTHORIZATION' => token } } + + before do + project.update_column(:visibility_level, Gitlab::VisibilityLevel.level_value(visibility.to_s)) + end + + if params[:expected_status] == :success + it { is_expected.to have_gitlab_http_status(:created) } + else + it { is_expected.to have_gitlab_http_status(expected_status) } + end + end + + it_behaves_like 'not found if ml_model_registry is disabled' + end + + describe 'bad request scenarios' do + context 'when file is too large' do + it 'is bad request', :aggregate_failures do + allow_next_instance_of(UploadedFile) do |uploaded_file| + allow(uploaded_file).to receive(:size).and_return(project.actual_limits.ml_model_max_file_size + 1) + end + + expect(api_response).to have_gitlab_http_status(:bad_request) + end + end + + context 'when package is not created' do + it 'is bad request', :aggregate_failures do + allow_next_instance_of(::Packages::MlModel::CreatePackageFileService) do |instance| + expect(instance).to receive(:execute).and_return(nil) + end + + expect(api_response).to have_gitlab_http_status(:bad_request) + end + end + end + end + + def authorize_upload_file(request_headers, package_name: 'mypackage', file_name: 'myfile.tar.gz') + url = "/projects/#{project.id}/packages/ml_models/#{package_name}/0.0.1/#{file_name}/authorize" + + put api(url), headers: request_headers + end + + def upload_file(request_headers, package_name: 'mypackage', file_name: 'myfile.tar.gz') + url = "/projects/#{project.id}/packages/ml_models/#{package_name}/0.0.1/#{file_name}" + + workhorse_finalize( + api(url), + method: :put, + file_key: file_key, + params: params, + headers: request_headers, + send_rewritten_field: send_rewritten_field + ) end end -- GitLab From 17b18e71caec66cb76c00b9cbab3696a7eeec826 Mon Sep 17 00:00:00 2001 From: Eduardo Bonet Date: Fri, 2 Jun 2023 14:34:08 +0200 Subject: [PATCH 07/10] Fix route on workhorse test --- workhorse/upload_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/workhorse/upload_test.go b/workhorse/upload_test.go index 39aa2782873921..8effe29197930a 100644 --- a/workhorse/upload_test.go +++ b/workhorse/upload_test.go @@ -580,7 +580,7 @@ func TestPackageFilesUpload(t *testing.T) { {"PUT", "/api/v4/projects/group%2Fproject/packages/conan/v1/files"}, {"PUT", "/api/v4/projects/group%2Fproject/packages/maven/v1/files"}, {"PUT", "/api/v4/projects/group%2Fproject/packages/generic/mypackage/0.0.1/myfile.tar.gz"}, - {"PUT", "/api/v4/projects/group%2Fproject/packages/ml_models/0.0.1/myfile.tar.gz"}, + {"PUT", "/api/v4/projects/group%2Fproject/packages/ml_models/mymodel/0.0.1/myfile.tar.gz"}, {"PUT", "/api/v4/projects/group%2Fproject/packages/debian/libsample0_1.2.3~alpha2-1_amd64.deb"}, {"POST", "/api/v4/projects/group%2Fproject/packages/rubygems/api/v1/gems/sample.gem"}, {"POST", "/api/v4/projects/group%2Fproject/packages/rpm/sample-4.23.fc21.x86_64.rpm"}, -- GitLab From 1be31decf59a8dd49f81a666b7ee2d931e9012ca Mon Sep 17 00:00:00 2001 From: Eduardo Bonet Date: Mon, 5 Jun 2023 13:55:03 +0200 Subject: [PATCH 08/10] Adds more tests --- lib/api/ml_model_packages.rb | 2 - spec/requests/api/ml_model_packages_spec.rb | 48 +------- .../api/ml_model_packages_shared_examples.rb | 111 ++++++++++++++++++ 3 files changed, 115 insertions(+), 46 deletions(-) create mode 100644 spec/support/shared_examples/requests/api/ml_model_packages_shared_examples.rb diff --git a/lib/api/ml_model_packages.rb b/lib/api/ml_model_packages.rb index 0df5104394f405..fec72b03ffd419 100644 --- a/lib/api/ml_model_packages.rb +++ b/lib/api/ml_model_packages.rb @@ -103,8 +103,6 @@ def max_file_size_exceeded? Gitlab::ErrorTracking.track_exception(e, extra: { file_name: params[:file_name], project_id: project.id }) forbidden! - rescue ::Packages::DuplicatePackageError - bad_request!('Duplicate package is not allowed') end end end diff --git a/spec/requests/api/ml_model_packages_spec.rb b/spec/requests/api/ml_model_packages_spec.rb index 016dbed98964e7..9d38f16df17084 100644 --- a/spec/requests/api/ml_model_packages_spec.rb +++ b/spec/requests/api/ml_model_packages_spec.rb @@ -32,24 +32,6 @@ let(:project_to_enable_ff) { project } let(:headers) { {} } - shared_examples 'not found if ml_model_registry is disabled' do - context 'when feature flag is disabled' do - let(:project_to_enable_ff) { nil } - - it "is not found" do - is_expected.to have_gitlab_http_status(:not_found) - end - end - - context 'when feature flag is disabled for current project' do - let(:project_to_enable_ff) { another_project } - - it "is not found" do - is_expected.to have_gitlab_http_status(:not_found) - end - end - end - shared_context 'ml model authorize permissions table' do # rubocop:disable RSpec/ContextWording # rubocop:disable Metrics/AbcSize # :visibility, :user_role, :member, :token_type, :valid_token, :expected_status @@ -132,7 +114,7 @@ def authorize_permissions_table it { is_expected.to have_gitlab_http_status(expected_status) } end - it_behaves_like 'not found if ml_model_registry is disabled' + it_behaves_like 'Endpoint not found if ml_model_registry is disabled' end describe 'application security' do @@ -190,35 +172,13 @@ def authorize_permissions_table end if params[:expected_status] == :success - it { is_expected.to have_gitlab_http_status(:created) } + it_behaves_like 'process ml model package upload' else it { is_expected.to have_gitlab_http_status(expected_status) } end end - it_behaves_like 'not found if ml_model_registry is disabled' - end - - describe 'bad request scenarios' do - context 'when file is too large' do - it 'is bad request', :aggregate_failures do - allow_next_instance_of(UploadedFile) do |uploaded_file| - allow(uploaded_file).to receive(:size).and_return(project.actual_limits.ml_model_max_file_size + 1) - end - - expect(api_response).to have_gitlab_http_status(:bad_request) - end - end - - context 'when package is not created' do - it 'is bad request', :aggregate_failures do - allow_next_instance_of(::Packages::MlModel::CreatePackageFileService) do |instance| - expect(instance).to receive(:execute).and_return(nil) - end - - expect(api_response).to have_gitlab_http_status(:bad_request) - end - end + it_behaves_like 'Endpoint not found if ml_model_registry is disabled' end end @@ -228,7 +188,7 @@ def authorize_upload_file(request_headers, package_name: 'mypackage', file_name: put api(url), headers: request_headers end - def upload_file(request_headers, package_name: 'mypackage', file_name: 'myfile.tar.gz') + def upload_file(request_headers, package_name: 'mypackage') url = "/projects/#{project.id}/packages/ml_models/#{package_name}/0.0.1/#{file_name}" workhorse_finalize( diff --git a/spec/support/shared_examples/requests/api/ml_model_packages_shared_examples.rb b/spec/support/shared_examples/requests/api/ml_model_packages_shared_examples.rb new file mode 100644 index 00000000000000..6ab288ddd55c29 --- /dev/null +++ b/spec/support/shared_examples/requests/api/ml_model_packages_shared_examples.rb @@ -0,0 +1,111 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'Endpoint not found if ml_model_registry is disabled' do + context 'when feature flag is disabled' do + let(:project_to_enable_ff) { nil } + + it "is not found" do + is_expected.to have_gitlab_http_status(:not_found) + end + end + + context 'when feature flag is disabled for current project' do + let(:project_to_enable_ff) { another_project } + + it "is not found" do + is_expected.to have_gitlab_http_status(:not_found) + end + end +end + +RSpec.shared_examples 'creates model experiments package files' do + it 'creates package files', :aggregate_failures do + expect { api_response } + .to change { project.packages.count }.by(1) + .and change { Packages::PackageFile.count }.by(1) + expect(api_response).to have_gitlab_http_status(:created) + + package_file = project.packages.last.package_files.reload.last + expect(package_file.file_name).to eq(file_name) + end + + it 'returns bad request if package creation fails' do + allow_next_instance_of(::Packages::MlModel::CreatePackageFileService) do |instance| + expect(instance).to receive(:execute).and_return(nil) + end + + expect(api_response).to have_gitlab_http_status(:bad_request) + end + + context 'when file is too large' do + it 'is bad request', :aggregate_failures do + allow_next_instance_of(UploadedFile) do |uploaded_file| + allow(uploaded_file).to receive(:size).and_return(project.actual_limits.ml_model_max_file_size + 1) + end + + expect(api_response).to have_gitlab_http_status(:bad_request) + end + end +end + +RSpec.shared_examples 'process ml model package upload' do + context 'with object storage disabled' do + before do + stub_package_file_object_storage(enabled: false) + end + + context 'without a file from workhorse' do + let(:send_rewritten_field) { false } + + it_behaves_like 'returning response status', :bad_request + end + + context 'with correct params' do + it_behaves_like 'package workhorse uploads' + it_behaves_like 'creates model experiments package files' + # To be reactivated with https://gitlab.com/gitlab-org/gitlab/-/issues/414270 + # it_behaves_like 'a package tracking event', '::API::MlModelPackages', 'push_package' + end + end + + context 'with object storage enabled' do + let(:tmp_object) do + fog_connection.directories.new(key: 'packages').files.create( # rubocop:disable Rails/SaveBang + key: "tmp/uploads/#{file_name}", + body: 'content' + ) + end + + let(:fog_file) { fog_to_uploaded_file(tmp_object) } + let(:params) { { file: fog_file, 'file.remote_id' => file_name } } + + context 'and direct upload enabled' do + let(:fog_connection) do + stub_package_file_object_storage(direct_upload: true) + end + + it_behaves_like 'creates model experiments package files' + + ['123123', '../../123123'].each do |remote_id| + context "with invalid remote_id: #{remote_id}" do + let(:params) do + { + file: fog_file, + 'file.remote_id' => remote_id + } + end + + it { is_expected.to have_gitlab_http_status(:forbidden) } + end + end + end + + context 'and direct upload disabled' do + let(:fog_connection) do + stub_package_file_object_storage(direct_upload: false) + end + + it_behaves_like 'creates model experiments package files' + end + end +end -- GitLab From c802ec394adbc06d486536ffb34d5d4a639aa2ed Mon Sep 17 00:00:00 2001 From: Eduardo Bonet Date: Tue, 6 Jun 2023 10:02:03 +0200 Subject: [PATCH 09/10] Improves tests for model_registry feature flag --- spec/requests/api/ml_model_packages_spec.rb | 3 --- .../api/ml_model_packages_shared_examples.rb | 13 ++++--------- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/spec/requests/api/ml_model_packages_spec.rb b/spec/requests/api/ml_model_packages_spec.rb index 9d38f16df17084..be14fd0f47258c 100644 --- a/spec/requests/api/ml_model_packages_spec.rb +++ b/spec/requests/api/ml_model_packages_spec.rb @@ -79,9 +79,6 @@ def authorize_permissions_table end before do - stub_feature_flags(model_registry: false) - stub_feature_flags(model_registry: project_to_enable_ff) if project_to_enable_ff.present? - project.send("add_#{user_role}", user) if member && user_role != :anonymous end diff --git a/spec/support/shared_examples/requests/api/ml_model_packages_shared_examples.rb b/spec/support/shared_examples/requests/api/ml_model_packages_shared_examples.rb index 6ab288ddd55c29..a3ca8cdb6feea1 100644 --- a/spec/support/shared_examples/requests/api/ml_model_packages_shared_examples.rb +++ b/spec/support/shared_examples/requests/api/ml_model_packages_shared_examples.rb @@ -1,16 +1,11 @@ # frozen_string_literal: true RSpec.shared_examples 'Endpoint not found if ml_model_registry is disabled' do - context 'when feature flag is disabled' do - let(:project_to_enable_ff) { nil } - - it "is not found" do - is_expected.to have_gitlab_http_status(:not_found) - end - end - context 'when feature flag is disabled for current project' do - let(:project_to_enable_ff) { another_project } + before do + stub_feature_flags(model_registry: false) + stub_feature_flags(model_registry: another_project) + end it "is not found" do is_expected.to have_gitlab_http_status(:not_found) -- GitLab From 1f4961c3ca1ed06165e67d39ae2c56d6d3bf200b Mon Sep 17 00:00:00 2001 From: Eduardo Bonet Date: Wed, 7 Jun 2023 10:39:38 +0200 Subject: [PATCH 10/10] Tests can? instead of feature flag --- spec/requests/api/ml_model_packages_spec.rb | 4 ++-- .../requests/api/ml_model_packages_shared_examples.rb | 10 ++++++---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/spec/requests/api/ml_model_packages_spec.rb b/spec/requests/api/ml_model_packages_spec.rb index be14fd0f47258c..9c19f522e46cc2 100644 --- a/spec/requests/api/ml_model_packages_spec.rb +++ b/spec/requests/api/ml_model_packages_spec.rb @@ -111,7 +111,7 @@ def authorize_permissions_table it { is_expected.to have_gitlab_http_status(expected_status) } end - it_behaves_like 'Endpoint not found if ml_model_registry is disabled' + it_behaves_like 'Endpoint not found if read_model_registry not available' end describe 'application security' do @@ -175,7 +175,7 @@ def authorize_permissions_table end end - it_behaves_like 'Endpoint not found if ml_model_registry is disabled' + it_behaves_like 'Endpoint not found if read_model_registry not available' end end diff --git a/spec/support/shared_examples/requests/api/ml_model_packages_shared_examples.rb b/spec/support/shared_examples/requests/api/ml_model_packages_shared_examples.rb index a3ca8cdb6feea1..81ff004779af24 100644 --- a/spec/support/shared_examples/requests/api/ml_model_packages_shared_examples.rb +++ b/spec/support/shared_examples/requests/api/ml_model_packages_shared_examples.rb @@ -1,10 +1,12 @@ # frozen_string_literal: true -RSpec.shared_examples 'Endpoint not found if ml_model_registry is disabled' do - context 'when feature flag is disabled for current project' do +RSpec.shared_examples 'Endpoint not found if read_model_registry not available' do + context 'when read_model_registry disabled for current project' do before do - stub_feature_flags(model_registry: false) - stub_feature_flags(model_registry: another_project) + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?) + .with(user, :read_model_registry, project) + .and_return(false) end it "is not found" do -- GitLab