Merge branch 'revert-65d99621' into 'master'

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

See merge request gitlab-org/gitlab!19803

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!
+  before_action :authenticate_user!, except: [:route_not_found]
   before_action :enforce_terms!, if: :should_enforce_terms?
   before_action :validate_user_service_ticket!
   before_action :check_password_expiration
@@ -95,11 +95,13 @@ class ApplicationController < ActionController::Base
   end
 
   def route_not_found
-    # We need to call #authenticate_user! here because sometimes this is called from another action
-    # and not from our wildcard fallback route
-    authenticate_user!
+    if current_user
+      not_found
+    else
+      store_location_for(:user, request.fullpath) unless request.xhr?
 
-    not_found
+      redirect_to new_user_session_path, alert: I18n.t('devise.failure.unauthenticated')
+    end
   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) &&
+      ::Constraints::ProjectUrlConstrainer.new.matches?(request, existence_check: false) &&
         (request.query_string.blank? ||
          request.query_string.match(/\Aservice=git-(upload|receive)-pack\z/))
     end

config/routes/project.rb

@@ -245,6 +245,12 @@ 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
@@ -347,6 +353,17 @@ 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]
@@ -380,6 +397,11 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
           get :failures
           get :status
           get :test_report
+
+          Gitlab.ee do
+            get :security
+            get :licenses
+          end
         end
 
         member do
@@ -514,11 +536,24 @@ 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
 
@@ -594,15 +629,6 @@ 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,
@@ -627,4 +653,22 @@ 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, only: [], constraints: { id: %r{[a-zA-Z./0-9_\-]+} } do
+  resources :users, constraints: { id: %r{[a-zA-Z./0-9_\-]+} } do
     member do
       post :reset_runners_minutes
     end

ee/config/routes/group.rb

@@ -144,6 +144,14 @@ 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,18 +52,6 @@ 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]
@@ -79,22 +67,6 @@ 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
@@ -106,13 +78,6 @@ 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,6 +56,16 @@ 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,6 +31,16 @@ 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
-      sign_in(user) if user
+      allow(controller).to receive(:current_user).and_return(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 an unauthorized status' do
+      it 'returns a redirect' do
         subject
 
-        expect(response).to have_gitlab_http_status(:unauthorized)
+        expect(response).to have_gitlab_http_status(:redirect)
       end
     end
 
@@ -98,7 +98,7 @@ describe Projects::ManagedLicensesController do
 
   describe 'GET #show' do
     subject do
-      sign_in(user) if user
+      allow(controller).to receive(:current_user).and_return(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 an unauthorized status' do
+      it 'returns a redirect' do
         subject
 
-        expect(response).to have_gitlab_http_status(:unauthorized)
+        expect(response).to have_gitlab_http_status(:redirect)
       end
     end
 
@@ -151,7 +151,7 @@ describe Projects::ManagedLicensesController do
     let(:user) { dev_user }
 
     subject do
-      sign_in(user) if user
+      allow(controller).to receive(:current_user).and_return(user)
 
       get :show,
           params: {
@@ -189,7 +189,7 @@ describe Projects::ManagedLicensesController do
     end
 
     subject do
-      sign_in(user) if user
+      allow(controller).to receive(:current_user).and_return(user)
 
       post :create,
         params: {
@@ -235,10 +235,10 @@ describe Projects::ManagedLicensesController do
         new_software_license_policy_attributes
       end
 
-      it 'returns an unauthorized status' do
+      it 'returns a redirect' do
         expect { subject }.not_to change { project.software_license_policies.count }
 
-        expect(response).to have_gitlab_http_status(:unauthorized)
+        expect(response).to have_gitlab_http_status(:redirect)
       end
     end
 
@@ -300,7 +300,7 @@ describe Projects::ManagedLicensesController do
     end
 
     subject do
-      sign_in(user) if user
+      allow(controller).to receive(:current_user).and_return(user)
 
       patch :update,
         params: {
@@ -347,10 +347,10 @@ describe Projects::ManagedLicensesController do
         new_software_license_policy_attributes
       end
 
-      it 'returns an unauthorized status' do
+      it 'returns a redirect' do
         expect { subject }.not_to change { project.software_license_policies.count }
 
-        expect(response).to have_gitlab_http_status(:unauthorized)
+        expect(response).to have_gitlab_http_status(:redirect)
       end
     end
 
@@ -406,7 +406,7 @@ describe Projects::ManagedLicensesController do
     let(:id_to_destroy) { software_license_policy.id }
 
     subject do
-      sign_in(user) if user
+      allow(controller).to receive(:current_user).and_return(user)
 
       delete :destroy,
         params: {
@@ -452,10 +452,10 @@ describe Projects::ManagedLicensesController do
         new_software_license_policy_attributes
       end
 
-      it 'returns an unauthorized status' do
+      it 'returns a redirect' do
         expect { subject }.not_to change { project.software_license_policies.count }
 
-        expect(response).to have_gitlab_http_status(:unauthorized)
+        expect(response).to have_gitlab_http_status(:redirect)
       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 unauthorized status' do
+      it 'returns a redirect' do
         reset_alerting_token
 
-        expect(response).to have_gitlab_http_status(:unauthorized)
+        expect(response).to have_gitlab_http_status(:redirect)
       end
     end
 

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

@@ -5,16 +5,6 @@ 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,12 +2,17 @@
 
 module Constraints
   class ProjectUrlConstrainer
-    def matches?(request)
+    def matches?(request, existence_check: true)
       namespace_path = request.params[:namespace_id]
       project_path = request.params[:project_id] || request.params[:id]
       full_path = [namespace_path, project_path].join('/')
 
-      ProjectPathValidator.valid_path?(full_path)
+      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?
     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_ee(routes_name) | draw_ce(routes_name)
+        drawn_any = draw_ce(routes_name) | draw_ee(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 via authenticate_user! if not authenticated' do
+    it 'redirects to login page 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 do
+      it_behaves_like 'authenticates sessionless user', :show, :atom, { public: true, ignore_incrementing: 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 do
+      it_behaves_like 'authenticates sessionless user', :show, :atom, { public: false, ignore_incrementing: true } 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(:unauthorized)
+        expect(response).to have_gitlab_http_status(:redirect)
       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 do
+    it_behaves_like 'authenticates sessionless user', :index, :atom, ignore_incrementing: true 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 do
+    it_behaves_like 'authenticates sessionless user', :calendar, :ics, ignore_incrementing: true 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 401' do
-          expect(response).to have_gitlab_http_status(:unauthorized)
+        it 'returns a redirect' do
+          expect(response).to have_gitlab_http_status(:redirect)
         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 do
+    it_behaves_like 'authenticates sessionless user', :index, :atom, ignore_incrementing: true 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 do
+    it_behaves_like 'authenticates sessionless user', :show, :atom, ignore_incrementing: true do
       before do
         default_params.merge!(id: private_project, namespace_id: private_project.namespace)
 

spec/features/projects/pipelines/pipelines_spec.rb

@@ -819,7 +819,10 @@ describe 'Pipelines', :js do
     context 'when project is private' do
       let(:project) { create(:project, :private, :repository) }
 
-      it { expect(page).to have_content 'You need to sign in' }
+      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
     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).to have_gitlab_http_status(401)
+        expect(page.current_path).to eq("/users/sign_in")
       end
     end
 

spec/lib/constraints/project_url_constrainer_spec.rb

@@ -14,15 +14,42 @@ 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)
-    double(:request, params: { namespace_id: namespace, id: project })
+  def build_request(namespace, project, method = 'GET')
+    double(:request,
+      'get?': (method == 'GET'),
+      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,6 +776,10 @@ 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,8 +34,15 @@ 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
-      expect(authentication_metrics)
-        .to increment(:user_unauthenticated_counter)
+      # 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
 
       personal_access_token.update(scopes: [:read_user])
 
@@ -84,8 +91,15 @@ shared_examples 'authenticates sessionless user' do |path, format, params|
   end
 
   it "doesn't log the user in otherwise", unless: params[:public] do
-    expect(authentication_metrics)
-      .to increment(:user_unauthenticated_counter)
+    # 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
 
     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(parent.is_a?(Group) ? 401 : 302)
+      expect(response).to have_gitlab_http_status(302)
     end
   end
 end