Merge branch '35289-remove-existence-check-in-url-constrainer' into 'master'

Remove existence check from ProjectUrlConstrainer

See merge request gitlab-org/gitlab!19412

app/controllers/application_controller.rb

@@ -17,7 +17,7 @@ class ApplicationController < ActionController::Base
   include Gitlab::Tracking::ControllerConcern
   include Gitlab::Experimentation::ControllerConcern
 
-  before_action :authenticate_user!, except: [:route_not_found]
+  before_action :authenticate_user!
   before_action :enforce_terms!, if: :should_enforce_terms?
   before_action :validate_user_service_ticket!
   before_action :check_password_expiration
@@ -95,13 +95,11 @@ class ApplicationController < ActionController::Base
   end
 
   def route_not_found
-    if current_user
-      not_found
-    else
-      store_location_for(:user, request.fullpath) unless request.xhr?
+    # We need to call #authenticate_user! here because sometimes this is called from another action
+    # and not from our wildcard fallback route
+    authenticate_user!
 
-      redirect_to new_user_session_path, alert: I18n.t('devise.failure.unauthenticated')
-    end
+    not_found
   end
 
   def render(*args)

changelogs/unreleased/35289-remove-existence-check-in-url-constrainer.yml

+---
+title: Fix JSON responses returning 302 instead of 401
+merge_request: 19412
+author:
+type: fixed

config/routes/git_http.rb

@@ -52,7 +52,7 @@ scope(path: '*namespace_id/:project_id',
     # /info/refs?service=git-receive-pack, but nothing else.
     #
     git_http_handshake = lambda do |request|
-      ::Constraints::ProjectUrlConstrainer.new.matches?(request, existence_check: false) &&
+      ::Constraints::ProjectUrlConstrainer.new.matches?(request) &&
         (request.query_string.blank? ||
          request.query_string.match(/\Aservice=git-(upload|receive)-pack\z/))
     end

config/routes/project.rb

@@ -245,12 +245,6 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
             post :validate_query, on: :collection
           end
         end
-
-        Gitlab.ee do
-          resources :alerts, constraints: { id: /\d+/ }, only: [:index, :create, :show, :update, :destroy] do
-            post :notify, on: :collection
-          end
-        end
       end
 
       resources :merge_requests, concerns: :awardable, except: [:new, :create, :show], constraints: { id: /\d+/ } do
@@ -353,17 +347,6 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
         end
       end
 
-      Gitlab.ee do
-        resources :path_locks, only: [:index, :destroy] do
-          collection do
-            post :toggle
-          end
-        end
-
-        get '/service_desk' => 'service_desk#show', as: :service_desk
-        put '/service_desk' => 'service_desk#update', as: :service_desk_refresh
-      end
-
       resource :variables, only: [:show, :update]
 
       resources :triggers, only: [:index, :create, :edit, :update, :destroy]
@@ -397,11 +380,6 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
           get :failures
           get :status
           get :test_report
-
-          Gitlab.ee do
-            get :security
-            get :licenses
-          end
         end
 
         member do
@@ -536,24 +514,11 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
           get :realtime_changes
           post :create_merge_request
           get :discussions, format: :json
-
-          Gitlab.ee do
-            get 'designs(/*vueroute)', to: 'issues#designs', as: :designs, format: false
-          end
         end
 
         collection do
           post :bulk_update
           post :import_csv
-
-          Gitlab.ee do
-            post :export_csv
-            get :service_desk
-          end
-        end
-
-        Gitlab.ee do
-          resources :issue_links, only: [:index, :create, :destroy], as: 'links', path: 'links'
         end
       end
 
@@ -629,6 +594,15 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
       Gitlab.ee do
         resources :managed_licenses, only: [:index, :show, :new, :create, :edit, :update, :destroy]
       end
+
+      # Legacy routes.
+      # Introduced in 12.0.
+      # Should be removed after 12.1
+      Gitlab::Routing.redirect_legacy_paths(self, :settings, :branches, :tags,
+                                            :network, :graphs, :autocomplete_sources,
+                                            :project_members, :deploy_keys, :deploy_tokens,
+                                            :labels, :milestones, :services, :boards, :releases,
+                                            :forks, :group_links, :import, :avatar)
     end
 
     resources(:projects,
@@ -653,22 +627,4 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
       end
     end
   end
-
-  # Legacy routes.
-  # Introduced in 12.0.
-  # Should be removed after 12.1
-  scope(path: '*namespace_id',
-        as: :namespace,
-        namespace_id: Gitlab::PathRegex.full_namespace_route_regex) do
-    scope(path: ':project_id',
-          constraints: { project_id: Gitlab::PathRegex.project_route_regex },
-          module: :projects,
-          as: :project) do
-      Gitlab::Routing.redirect_legacy_paths(self, :settings, :branches, :tags,
-                                            :network, :graphs, :autocomplete_sources,
-                                            :project_members, :deploy_keys, :deploy_tokens,
-                                            :labels, :milestones, :services, :boards, :releases,
-                                            :forks, :group_links, :import, :avatar)
-    end
-  end
 end

ee/config/routes/admin.rb

 # frozen_string_literal: true
 
 namespace :admin do
-  resources :users, constraints: { id: %r{[a-zA-Z./0-9_\-]+} } do
+  resources :users, only: [], constraints: { id: %r{[a-zA-Z./0-9_\-]+} } do
     member do
       post :reset_runners_minutes
     end

ee/config/routes/group.rb

@@ -144,14 +144,6 @@ constraints(::Constraints::GroupUrlConstrainer.new) do
 
     resource :roadmap, only: [:show], controller: 'roadmap'
 
-    legacy_ee_group_boards_redirect = redirect do |params, request|
-      path = "/groups/#{params[:group_id]}/-/boards"
-      path << "/#{params[:extra_params]}" if params[:extra_params].present?
-      path << "?#{request.query_string}" if request.query_string.present?
-      path
-    end
-    get 'boards(/*extra_params)', as: :legacy_ee_group_boards_redirect, to: legacy_ee_group_boards_redirect
-
     resource :dependency_proxy, only: [:show, :update]
     resources :packages, only: [:index]
   end

ee/config/routes/project.rb

@@ -52,6 +52,18 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
       end
       # End of the /-/ scope.
 
+      resources :path_locks, only: [:index, :destroy] do
+        collection do
+          post :toggle
+        end
+      end
+
+      namespace :prometheus do
+        resources :alerts, constraints: { id: /\d+/ }, only: [:index, :create, :show, :update, :destroy] do
+          post :notify, on: :collection
+        end
+      end
+
       post 'alerts/notify', to: 'alerting/notifications#create'
 
       resource :tracing, only: [:show]
@@ -67,6 +79,22 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
         end
       end
 
+      resources :issues, only: [], constraints: { id: /\d+/ } do
+        member do
+          get 'designs(/*vueroute)', to: 'issues#designs', as: :designs, format: false
+        end
+
+        collection do
+          post :export_csv
+          get :service_desk
+        end
+
+        resources :issue_links, only: [:index, :create, :destroy], as: 'links', path: 'links'
+      end
+
+      get '/service_desk' => 'service_desk#show', as: :service_desk
+      put '/service_desk' => 'service_desk#update', as: :service_desk_refresh
+
       resources :merge_requests, only: [], constraints: { id: /\d+/ } do
         member do
           get :metrics_reports
@@ -78,6 +106,13 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
         end
       end
 
+      resources :pipelines, only: [] do
+        member do
+          get :security
+          get :licenses
+        end
+      end
+
       resource :insights, only: [:show], trailing_slash: true do
         collection do
           post :query

ee/spec/controllers/groups/boards_controller_spec.rb

@@ -56,16 +56,6 @@ describe Groups::BoardsController do
     let(:parent) { group }
 
     it_behaves_like 'returns recently visited boards'
-
-    context 'unauthenticated' do
-      it 'returns a 401' do
-        sign_out(user)
-
-        list_boards(recent: true)
-
-        expect(response).to have_gitlab_http_status(401)
-      end
-    end
   end
 
   describe 'GET show' do

ee/spec/controllers/projects/boards_controller_spec.rb

@@ -31,16 +31,6 @@ describe Projects::BoardsController do
     let(:parent) { project }
 
     it_behaves_like 'returns recently visited boards'
-
-    context 'unauthenticated' do
-      it 'returns a 302' do
-        sign_out(user)
-
-        list_boards(recent: true)
-
-        expect(response).to have_gitlab_http_status(302)
-      end
-    end
   end
 
   describe 'GET show' do

ee/spec/controllers/projects/managed_licenses_controller_spec.rb

@@ -41,7 +41,7 @@ describe Projects::ManagedLicensesController do
 
   describe 'GET #index' do
     subject do
-      allow(controller).to receive(:current_user).and_return(user)
+      sign_in(user) if user
 
       get :index, params: { namespace_id: project.namespace.to_param, project_id: project }, format: :json
     end
@@ -72,10 +72,10 @@ describe Projects::ManagedLicensesController do
     context 'with no logged in user' do
       let(:user) { unlogged_user }
 
-      it 'returns a redirect' do
+      it 'returns an unauthorized status' do
         subject
 
-        expect(response).to have_gitlab_http_status(:redirect)
+        expect(response).to have_gitlab_http_status(:unauthorized)
       end
     end
 
@@ -98,7 +98,7 @@ describe Projects::ManagedLicensesController do
 
   describe 'GET #show' do
     subject do
-      allow(controller).to receive(:current_user).and_return(user)
+      sign_in(user) if user
 
       get :show,
         params: {
@@ -122,10 +122,10 @@ describe Projects::ManagedLicensesController do
     context 'with no logged in user' do
       let(:user) { unlogged_user }
 
-      it 'returns a redirect' do
+      it 'returns an unauthorized status' do
         subject
 
-        expect(response).to have_gitlab_http_status(:redirect)
+        expect(response).to have_gitlab_http_status(:unauthorized)
       end
     end
 
@@ -151,7 +151,7 @@ describe Projects::ManagedLicensesController do
     let(:user) { dev_user }
 
     subject do
-      allow(controller).to receive(:current_user).and_return(user)
+      sign_in(user) if user
 
       get :show,
           params: {
@@ -189,7 +189,7 @@ describe Projects::ManagedLicensesController do
     end
 
     subject do
-      allow(controller).to receive(:current_user).and_return(user)
+      sign_in(user) if user
 
       post :create,
         params: {
@@ -235,10 +235,10 @@ describe Projects::ManagedLicensesController do
         new_software_license_policy_attributes
       end
 
-      it 'returns a redirect' do
+      it 'returns an unauthorized status' do
         expect { subject }.not_to change { project.software_license_policies.count }
 
-        expect(response).to have_gitlab_http_status(:redirect)
+        expect(response).to have_gitlab_http_status(:unauthorized)
       end
     end
 
@@ -300,7 +300,7 @@ describe Projects::ManagedLicensesController do
     end
 
     subject do
-      allow(controller).to receive(:current_user).and_return(user)
+      sign_in(user) if user
 
       patch :update,
         params: {
@@ -347,10 +347,10 @@ describe Projects::ManagedLicensesController do
         new_software_license_policy_attributes
       end
 
-      it 'returns a redirect' do
+      it 'returns an unauthorized status' do
         expect { subject }.not_to change { project.software_license_policies.count }
 
-        expect(response).to have_gitlab_http_status(:redirect)
+        expect(response).to have_gitlab_http_status(:unauthorized)
       end
     end
 
@@ -406,7 +406,7 @@ describe Projects::ManagedLicensesController do
     let(:id_to_destroy) { software_license_policy.id }
 
     subject do
-      allow(controller).to receive(:current_user).and_return(user)
+      sign_in(user) if user
 
       delete :destroy,
         params: {
@@ -452,10 +452,10 @@ describe Projects::ManagedLicensesController do
         new_software_license_policy_attributes
       end
 
-      it 'returns a redirect' do
+      it 'returns an unauthorized status' do
         expect { subject }.not_to change { project.software_license_policies.count }
 
-        expect(response).to have_gitlab_http_status(:redirect)
+        expect(response).to have_gitlab_http_status(:unauthorized)
       end
     end
 

ee/spec/controllers/projects/settings/operations_controller_spec.rb

@@ -506,10 +506,10 @@ describe Projects::Settings::OperationsController do
         sign_out(user)
       end
 
-      it 'returns a redirect' do
+      it 'returns unauthorized status' do
         reset_alerting_token
 
-        expect(response).to have_gitlab_http_status(:redirect)
+        expect(response).to have_gitlab_http_status(:unauthorized)
       end
     end
 

ee/spec/support/shared_examples/controllers/recent_boards.rb

@@ -5,6 +5,16 @@ require 'spec_helper'
 shared_examples 'returns recently visited boards' do
   let(:boards) { create_list(:board, 8, resource_parent: parent) }
 
+  context 'unauthenticated' do
+    it 'returns a 401' do
+      sign_out(user)
+
+      list_boards(recent: true)
+
+      expect(response).to have_gitlab_http_status(401)
+    end
+  end
+
   it 'returns last 4 visited boards' do
     [0, 2, 5, 3, 7, 1].each_with_index do |board_index, i|
       visit_board(boards[board_index], Time.now + i.minutes)

lib/constraints/project_url_constrainer.rb

@@ -2,17 +2,12 @@
 
 module Constraints
   class ProjectUrlConstrainer
-    def matches?(request, existence_check: true)
+    def matches?(request)
       namespace_path = request.params[:namespace_id]
       project_path = request.params[:project_id] || request.params[:id]
       full_path = [namespace_path, project_path].join('/')
 
-      return false unless ProjectPathValidator.valid_path?(full_path)
-      return true unless existence_check
-
-      # We intentionally allow SELECT(*) here so result of this query can be used
-      # as cache for further Project.find_by_full_path calls within request
-      Project.find_by_full_path(full_path, follow_redirects: request.get?).present?
+      ProjectPathValidator.valid_path?(full_path)
     end
   end
 end

lib/gitlab/patch/draw_route.rb

@@ -10,7 +10,7 @@ module Gitlab
       RoutesNotFound = Class.new(StandardError)
 
       def draw(routes_name)
-        drawn_any = draw_ce(routes_name) | draw_ee(routes_name)
+        drawn_any = draw_ee(routes_name) | draw_ce(routes_name)
 
         drawn_any || raise(RoutesNotFound.new("Cannot find #{routes_name}"))
       end

spec/controllers/application_controller_spec.rb

@@ -186,7 +186,7 @@ describe ApplicationController do
       expect(response).to have_gitlab_http_status(404)
     end
 
-    it 'redirects to login page if not authenticated' do
+    it 'redirects to login page via authenticate_user! if not authenticated' do
       get :index
 
       expect(response).to redirect_to new_user_session_path

spec/controllers/projects/commits_controller_spec.rb

@@ -142,7 +142,7 @@ describe Projects::CommitsController do
 
   context 'token authentication' do
     context 'public project' do
-      it_behaves_like 'authenticates sessionless user', :show, :atom, { public: true, ignore_incrementing: true } do
+      it_behaves_like 'authenticates sessionless user', :show, :atom, public: true do
         before do
           public_project = create(:project, :repository, :public)
 
@@ -152,7 +152,7 @@ describe Projects::CommitsController do
     end
 
     context 'private project' do
-      it_behaves_like 'authenticates sessionless user', :show, :atom, { public: false, ignore_incrementing: true } do
+      it_behaves_like 'authenticates sessionless user', :show, :atom, public: false do
         before do
           private_project = create(:project, :repository, :private)
           private_project.add_maintainer(user)

spec/controllers/projects/error_tracking_controller_spec.rb

@@ -146,7 +146,7 @@ describe Projects::ErrorTrackingController do
       it 'redirects to sign-in page' do
         post :list_projects, params: list_projects_params
 
-        expect(response).to have_gitlab_http_status(:redirect)
+        expect(response).to have_gitlab_http_status(:unauthorized)
       end
     end
 

spec/controllers/projects/issues_controller_spec.rb

@@ -1441,7 +1441,7 @@ describe Projects::IssuesController do
   context 'private project with token authentication' do
     let(:private_project) { create(:project, :private) }
 
-    it_behaves_like 'authenticates sessionless user', :index, :atom, ignore_incrementing: true do
+    it_behaves_like 'authenticates sessionless user', :index, :atom do
       before do
         default_params.merge!(project_id: private_project, namespace_id: private_project.namespace)
 
@@ -1449,7 +1449,7 @@ describe Projects::IssuesController do
       end
     end
 
-    it_behaves_like 'authenticates sessionless user', :calendar, :ics, ignore_incrementing: true do
+    it_behaves_like 'authenticates sessionless user', :calendar, :ics do
       before do
         default_params.merge!(project_id: private_project, namespace_id: private_project.namespace)
 

spec/controllers/projects/releases_controller_spec.rb

@@ -111,8 +111,8 @@ describe Projects::ReleasesController do
       context 'when the project is private and the user is not logged in' do
         let(:project) { private_project }
 
-        it 'returns a redirect' do
-          expect(response).to have_gitlab_http_status(:redirect)
+        it 'returns a 401' do
+          expect(response).to have_gitlab_http_status(:unauthorized)
         end
       end
     end

spec/controllers/projects/tags_controller_spec.rb

@@ -41,7 +41,7 @@ describe Projects::TagsController do
   context 'private project with token authentication' do
     let(:private_project) { create(:project, :repository, :private) }
 
-    it_behaves_like 'authenticates sessionless user', :index, :atom, ignore_incrementing: true do
+    it_behaves_like 'authenticates sessionless user', :index, :atom do
       before do
         default_params.merge!(project_id: private_project, namespace_id: private_project.namespace)
 

spec/controllers/projects_controller_spec.rb

@@ -1149,7 +1149,7 @@ describe ProjectsController do
   context 'private project with token authentication' do
     let(:private_project) { create(:project, :private) }
 
-    it_behaves_like 'authenticates sessionless user', :show, :atom, ignore_incrementing: true do
+    it_behaves_like 'authenticates sessionless user', :show, :atom do
       before do
         default_params.merge!(id: private_project, namespace_id: private_project.namespace)
 

spec/features/projects/pipelines/pipelines_spec.rb

@@ -819,10 +819,7 @@ describe 'Pipelines', :js do
     context 'when project is private' do
       let(:project) { create(:project, :private, :repository) }
 
-      it 'redirects the user to sign_in and displays the flash alert' do
-        expect(page).to have_content 'You need to sign in'
-        expect(page.current_path).to eq("/users/sign_in")
-      end
+      it { expect(page).to have_content 'You need to sign in' }
     end
   end
 

spec/features/projects/tags/user_views_tags_spec.rb

@@ -15,7 +15,7 @@ describe 'User views tags', :feature do
       it do
         visit project_tags_path(project, format: :atom)
 
-        expect(page.current_path).to eq("/users/sign_in")
+        expect(page).to have_gitlab_http_status(401)
       end
     end
 

spec/lib/constraints/project_url_constrainer_spec.rb

@@ -14,42 +14,15 @@ describe Constraints::ProjectUrlConstrainer do
     end
 
     context 'invalid request' do
-      context "non-existing project" do
-        let(:request) { build_request('foo', 'bar') }
-
-        it { expect(subject.matches?(request)).to be_falsey }
-
-        context 'existence_check is false' do
-          it { expect(subject.matches?(request, existence_check: false)).to be_truthy }
-        end
-      end
-
       context "project id ending with .git" do
         let(:request) { build_request(namespace.full_path, project.path + '.git') }
 
         it { expect(subject.matches?(request)).to be_falsey }
       end
     end
-
-    context 'when the request matches a redirect route' do
-      let(:old_project_path) { 'old_project_path' }
-      let!(:redirect_route) { project.redirect_routes.create!(path: "#{namespace.full_path}/#{old_project_path}") }
-
-      context 'and is a GET request' do
-        let(:request) { build_request(namespace.full_path, old_project_path) }
-        it { expect(subject.matches?(request)).to be_truthy }
-      end
-
-      context 'and is NOT a GET request' do
-        let(:request) { build_request(namespace.full_path, old_project_path, 'POST') }
-        it { expect(subject.matches?(request)).to be_falsey }
-      end
-    end
   end
 
-  def build_request(namespace, project, method = 'GET')
-    double(:request,
-      'get?': (method == 'GET'),
-      params: { namespace_id: namespace, id: project })
+  def build_request(namespace, project)
+    double(:request, params: { namespace_id: namespace, id: project })
   end
 end

spec/requests/projects/blob_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::BlobController do
+  let(:project) { create(:project, :private, :repository) }
+  let(:namespace) { project.namespace }
+
+  context 'anonymous user views blob in inaccessible project' do
+    context 'with default HTML format' do
+      before do
+        get namespace_project_blob_path(namespace_id: namespace, project_id: project, id: 'master/README.md')
+      end
+
+      context 'when project is private' do
+        it { expect(response).to have_gitlab_http_status(:redirect) }
+      end
+
+      context 'when project does not exist' do
+        let(:namespace) { 'non_existent_namespace' }
+        let(:project)   { 'non_existent_project' }
+
+        it { expect(response).to have_gitlab_http_status(:redirect) }
+      end
+    end
+
+    context 'with JSON format' do
+      before do
+        get namespace_project_blob_path(namespace_id: namespace, project_id: project, id: 'master/README.md', format: :json)
+      end
+
+      context 'when project is private' do
+        it { expect(response).to have_gitlab_http_status(:unauthorized) }
+      end
+
+      context 'when project does not exist' do
+        let(:namespace) { 'non_existent_namespace' }
+        let(:project)   { 'non_existent_project' }
+
+        it { expect(response).to have_gitlab_http_status(:unauthorized) }
+      end
+    end
+  end
+end

spec/routing/project_routing_spec.rb

@@ -776,10 +776,6 @@ describe 'project routing' do
       it 'routes when :template_type is `issue`' do
         expect(get(show_with_template_type('issue'))).to route_to('projects/templates#show', namespace_id: 'gitlab', project_id: 'gitlabhq', template_type: 'issue', key: 'template_name', format: 'json')
       end
-
-      it 'routes to application#route_not_found when :template_type is unknown' do
-        expect(get(show_with_template_type('invalid'))).to route_to('application#route_not_found', unmatched_route: 'gitlab/gitlabhq/templates/invalid/template_name')
-      end
     end
   end
 

spec/support/controllers/sessionless_auth_controller_shared_examples.rb

@@ -34,15 +34,8 @@ shared_examples 'authenticates sessionless user' do |path, format, params|
 
   context 'when the personal access token has no api scope', unless: params[:public] do
     it 'does not log the user in' do
-      # Several instances of where these specs are shared route the request
-      #   through ApplicationController#route_not_found which does not involve
-      #   the usual auth code from Devise, so does not increment the
-      #   :user_unauthenticated_counter
-      #
-      unless params[:ignore_incrementing]
-        expect(authentication_metrics)
-          .to increment(:user_unauthenticated_counter)
-      end
+      expect(authentication_metrics)
+        .to increment(:user_unauthenticated_counter)
 
       personal_access_token.update(scopes: [:read_user])
 
@@ -91,15 +84,8 @@ shared_examples 'authenticates sessionless user' do |path, format, params|
   end
 
   it "doesn't log the user in otherwise", unless: params[:public] do
-    # Several instances of where these specs are shared route the request
-    #   through ApplicationController#route_not_found which does not involve
-    #   the usual auth code from Devise, so does not increment the
-    #   :user_unauthenticated_counter
-    #
-    unless params[:ignore_incrementing]
-      expect(authentication_metrics)
-        .to increment(:user_unauthenticated_counter)
-    end
+    expect(authentication_metrics)
+      .to increment(:user_unauthenticated_counter)
 
     get path, params: default_params.merge(private_token: 'token')
 

spec/support/shared_examples/controllers/todos_shared_examples.rb

@@ -39,7 +39,7 @@ shared_examples 'todos actions' do
         post_create
       end.to change { user.todos.count }.by(0)
 
-      expect(response).to have_gitlab_http_status(302)
+      expect(response).to have_gitlab_http_status(parent.is_a?(Group) ? 401 : 302)
     end
   end
 end