From 84c811efe711632032280c0cccc521c34fa169b2 Mon Sep 17 00:00:00 2001 From: Igor Drozdov Date: Mon, 7 Aug 2023 12:40:44 +0200 Subject: [PATCH 1/2] Unify EE/FOSS versions of API find_project! method --- ee/lib/ee/api/helpers.rb | 15 +++------------ lib/api/helpers.rb | 6 +++++- 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/ee/lib/ee/api/helpers.rb b/ee/lib/ee/api/helpers.rb index c347818f8a68eb..a94838ad1d2b02 100644 --- a/ee/lib/ee/api/helpers.rb +++ b/ee/lib/ee/api/helpers.rb @@ -69,21 +69,12 @@ def authenticated_with_ldap_admin_access! .allow_group_owners_to_manage_ldap end - override :find_project! - def find_project!(id) - project = find_project(id) - - return forbidden! unless authorized_project_scope?(project) - + override :read_project_ability + def read_project_ability # CI job token authentication: # this method grants limited privileged for admin users # admin users can only access project if they are direct member - ability = job_token_authentication? ? :build_read_project : :read_project - - return project if can?(current_user, ability, project) - return unauthorized! if authenticate_non_public? - - not_found!('Project') + job_token_authentication? ? :build_read_project : :read_project end override :find_group! diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index b616f1b35b3aa4..c8449819da9747 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -157,12 +157,16 @@ def find_project!(id) return forbidden! unless authorized_project_scope?(project) - return project if can?(current_user, :read_project, project) + return project if can?(current_user, read_project_ability, project) return unauthorized! if authenticate_non_public? not_found!('Project') end + def read_project_ability + :read_project + end + def authorized_project_scope?(project) return true unless job_token_authentication? return true unless route_authentication_setting[:job_token_scope] == :project -- GitLab From fa19b60b7f337ec7ce8290c3cd842fe975a8498e Mon Sep 17 00:00:00 2001 From: Igor Drozdov Date: Mon, 7 Aug 2023 17:51:51 +0200 Subject: [PATCH 2/2] Redirect API requests to moved projects When a project is moved and being accessed by the old full path our API returns 404. However, we should redirect user to the existing link. This commit: - When request is GET, finds a project and takes redirect routes into account - If the specified full path is not equal to the current full path, 301 (Moved permanently) is returned with Location set as a header - The Location header contains the new url that specifies id Changelog: fixed --- .../api_redirect_moved_projects.yml | 8 +++ doc/api/rest/index.md | 28 +++++++++ ee/lib/ee/api/helpers.rb | 2 +- .../api/analytics/product_analytics_spec.rb | 12 ++++ lib/api/api.rb | 16 +++++ lib/api/helpers.rb | 42 +++++++++++-- spec/requests/api/files_spec.rb | 17 +++++ spec/requests/api/projects_spec.rb | 62 +++++++++++++++++++ 8 files changed, 182 insertions(+), 5 deletions(-) create mode 100644 config/feature_flags/development/api_redirect_moved_projects.yml diff --git a/config/feature_flags/development/api_redirect_moved_projects.yml b/config/feature_flags/development/api_redirect_moved_projects.yml new file mode 100644 index 00000000000000..56c01d3862b0d3 --- /dev/null +++ b/config/feature_flags/development/api_redirect_moved_projects.yml @@ -0,0 +1,8 @@ +--- +name: api_redirect_moved_projects +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/128642 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/421992 +milestone: '16.3' +type: development +group: group::source code +default_enabled: false diff --git a/doc/api/rest/index.md b/doc/api/rest/index.md index 20e260cfbb46b3..f24b3225f69084 100644 --- a/doc/api/rest/index.md +++ b/doc/api/rest/index.md @@ -314,6 +314,7 @@ The following table shows the possible return codes for API requests. | `201 Created` | The `POST` request was successful, and the resource is returned as JSON. | | `202 Accepted` | The `GET`, `PUT` or `DELETE` request was successful, and the resource is scheduled for processing. | | `204 No Content` | The server has successfully fulfilled the request, and there is no additional content to send in the response payload body. | +| `301 Moved Permanently` | The resource has been definitively moved to the URL given by the `Location` headers. | | `304 Not Modified` | The resource hasn't been modified since the last request. | | `400 Bad Request` | A required attribute of the API request is missing. For example, the title of an issue is not given. | | `401 Unauthorized` | The user isn't authenticated. A valid [user token](#authentication) is necessary. | @@ -327,6 +328,33 @@ The following table shows the possible return codes for API requests. | `500 Server Error` | While handling the request, something went wrong on the server. | | `503 Service Unavailable` | The server cannot handle the request because the server is temporarily overloaded. | +## Redirects + +> Introduced in GitLab 16.4 [with a flag](../../user/feature_flags.md) named `api_redirect_moved_projects`. Disabled by default. + +FLAG: +On GitLab.com, this feature is not available. +On self-managed GitLab, by default this feature is not available. To make it available, +an administrator can [enable the feature flag](../../user/feature_flags.md) named `api_redirect_moved_projects`. + +REST API can respond with a redirect and users should be able to handle such responses. +The users should follow the redirect and repeat the request to the URI specified in the `Location` header. + +Example of a project moved to a different path: + +```shell +curl --verbose "https://gitlab.example.com/api/v4/projects/gitlab-org%2Fold-path-project" +``` + +The response is: + +```plaintext +... +< Location: http://gitlab.example.com/api/v4/projects/81 +... +This resource has been moved permanently to https://gitlab.example.com/api/v4/projects/81 +``` + ## Pagination GitLab supports the following pagination methods: diff --git a/ee/lib/ee/api/helpers.rb b/ee/lib/ee/api/helpers.rb index a94838ad1d2b02..32c10105ef150e 100644 --- a/ee/lib/ee/api/helpers.rb +++ b/ee/lib/ee/api/helpers.rb @@ -74,7 +74,7 @@ def read_project_ability # CI job token authentication: # this method grants limited privileged for admin users # admin users can only access project if they are direct member - job_token_authentication? ? :build_read_project : :read_project + job_token_authentication? ? :build_read_project : super end override :find_group! diff --git a/ee/spec/requests/api/analytics/product_analytics_spec.rb b/ee/spec/requests/api/analytics/product_analytics_spec.rb index ba04a02919c78f..78a462bd5fea52 100644 --- a/ee/spec/requests/api/analytics/product_analytics_spec.rb +++ b/ee/spec/requests/api/analytics/product_analytics_spec.rb @@ -71,6 +71,18 @@ let(:request) { get api("/projects/#{project.id}/product_analytics/funnels", current_user) } it_behaves_like 'well behaved cube query', { stub_service: false } + + context 'when a project is moved' do + let_it_be(:redirect_route) { 'new/project/location' } + + it 'returns 404 error' do + project.route.create_redirect(redirect_route) + + get api("/projects/#{CGI.escape(redirect_route)}/product_analytics/funnels", current_user) + + expect(response).to have_gitlab_http_status(:not_found) + end + end end private diff --git a/lib/api/api.rb b/lib/api/api.rb index 7a384d9cf59fd7..8ebd7f83acbb0e 100644 --- a/lib/api/api.rb +++ b/lib/api/api.rb @@ -15,6 +15,18 @@ class API < ::API::Base LOG_FORMATTER = Gitlab::GrapeLogging::Formatters::LogrageWithTimestamp.new LOGGER = Logger.new(LOG_FILENAME) + class MovedPermanentlyError < StandardError + MSG_PREFIX = 'This resource has been moved permanently to' + + attr_reader :location_url + + def initialize(location_url) + @location_url = location_url + + super("#{MSG_PREFIX} #{location_url}") + end + end + insert_before Grape::Middleware::Error, GrapeLogging::Middleware::RequestLogger, logger: LOGGER, @@ -142,6 +154,10 @@ class API < ::API::Base error! e.message, e.status, e.headers end + rescue_from MovedPermanentlyError do |e| + rack_response(e.message, 301, { 'Location' => e.location_url }) + end + rescue_from Gitlab::Auth::TooManyIps do |e| rack_response({ 'message' => '403 Forbidden' }.to_json, 403) end diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index c8449819da9747..e1207e7e222f72 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -147,7 +147,7 @@ def find_project(id) if id.is_a?(Integer) || id =~ INTEGER_ID_REGEX projects.find_by(id: id) elsif id.include?("/") - projects.find_by_full_path(id) + projects.find_by_full_path(id, follow_redirects: Feature.enabled?(:api_redirect_moved_projects)) end end # rubocop: enable CodeReuse/ActiveRecord @@ -157,10 +157,19 @@ def find_project!(id) return forbidden! unless authorized_project_scope?(project) - return project if can?(current_user, read_project_ability, project) - return unauthorized! if authenticate_non_public? + unless can?(current_user, read_project_ability, project) + return unauthorized! if authenticate_non_public? + + return not_found!('Project') + end + + if project_moved?(id, project) + return not_allowed!('Non GET methods are not allowed for moved projects') unless request.get? - not_found!('Project') + return redirect!(url_with_project_id(project)) + end + + project end def read_project_ability @@ -442,6 +451,13 @@ def order_options_with_tie_breaker order_options end + # An error is raised to interrupt user's request and redirect them to the right route. + # The error! helper behaves similarly, but it cannot be used because it formats the + # response message. + def redirect!(location_url) + raise ::API::API::MovedPermanentlyError, location_url + end + # error helpers def forbidden!(reason = nil) @@ -841,6 +857,24 @@ def ip_address def sanitize_id_param(id) id.present? ? id.to_i : nil end + + def project_moved?(id, project) + return false unless Feature.enabled?(:api_redirect_moved_projects) + return false unless id.is_a?(String) && id.include?('/') + return false if project.blank? || id == project.full_path + return false unless params[:id] == id + + true + end + + def url_with_project_id(project) + new_params = params.merge(id: project.id.to_s).transform_values { |v| v.is_a?(String) ? CGI.escape(v) : v } + new_path = GrapePathHelpers::DecoratedRoute.new(route).path_segments_with_values(new_params).join('/') + + Rack::Request.new(env).tap do |r| + r.path_info = "/#{new_path}" + end.url + end end end diff --git a/spec/requests/api/files_spec.rb b/spec/requests/api/files_spec.rb index ea341703301e02..01acb83360c2d5 100644 --- a/spec/requests/api/files_spec.rb +++ b/spec/requests/api/files_spec.rb @@ -342,6 +342,23 @@ def expect_to_send_git_blob(url, params) expect(response).to have_gitlab_http_status(:ok) end + context 'when a project is moved' do + let_it_be(:redirect_route) { 'new/project/location' } + let_it_be(:file_path) { 'files%2Fruby%2Fpopen.rb' } + + it 'redirects to the new project location' do + project.route.create_redirect(redirect_route) + + url = "/projects/#{CGI.escape(redirect_route)}/repository/files/#{file_path}" + get api(url, api_user, **options), params: params + + expect(response).to have_gitlab_http_status(:moved_permanently) + expect(response.headers['Location']).to start_with( + "#{request.base_url}/api/v4/projects/#{project.id}/repository/files/#{file_path}" + ) + end + end + it 'sets inline content disposition by default' do url = route(file_path) + '/raw' diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 34447e12d11534..26132215404f7e 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -2770,6 +2770,45 @@ def failure_message(diff) expect(json_response['name']).to eq(project.name) end + context 'when a project is moved' do + let(:redirect_route) { 'new/project/location' } + let(:perform_request) { get api("/projects/#{CGI.escape(redirect_route)}", user), params: { license: true } } + + before do + project.route.create_redirect(redirect_route) + end + + it 'redirects to the new project location' do + perform_request + + expect(response).to have_gitlab_http_status(:moved_permanently) + + url = response.headers['Location'] + expect(url).to start_with("#{request.base_url}/api/v4/projects/#{project.id}") + expect(CGI.parse(URI(url).query)).to include({ 'license' => ['true'] }) + end + + context 'when a user do not have access' do + let(:user) { create(:user) } + + it 'returns a 404 error' do + perform_request + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + context 'when api_redirect_moved_projects is disabled' do + it 'returns a 404 error' do + stub_feature_flags(api_redirect_moved_projects: false) + + perform_request + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end + it 'returns a 404 error if not found' do get api("/projects/#{non_existing_record_id}", user) expect(response).to have_gitlab_http_status(:not_found) @@ -4454,6 +4493,29 @@ def failure_message(diff) expect(response).to have_gitlab_http_status(:forbidden) end end + + context 'when a project is moved' do + let_it_be(:redirect_route) { 'new/project/location' } + let_it_be(:path) { "/projects/#{CGI.escape(redirect_route)}/archive" } + + before do + project.route.create_redirect(redirect_route) + end + + it 'returns 405 error' do + post api(path, user) + + expect(response).to have_gitlab_http_status(:method_not_allowed) + end + + context 'when user do not have access to the project' do + it 'returns 404 error' do + post api(path, create(:user)) + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end end describe 'POST /projects/:id/unarchive' do -- GitLab