Merge branch 'pl-rubocop-put-under-scope' into 'master'

Check HTTP methods in Cop/Put{Project,Group}RoutesUnderScope

See merge request gitlab-org/gitlab!37345

.rubocop.yml

@@ -481,3 +481,13 @@ Rails/SaveBang:
     - 'ee/spec/**/*.rb'
     - 'qa/spec/**/*.rb'
     - 'qa/qa/specs/**/*.rb'
+
+Cop/PutProjectRoutesUnderScope:
+  Include:
+    - 'config/routes/project.rb'
+    - 'ee/config/routes/project.rb'
+
+Cop/PutGroupRoutesUnderScope:
+  Include:
+    - 'config/routes/group.rb'
+    - 'ee/config/routes/group.rb'

config/routes/group.rb

@@ -5,23 +5,27 @@ constraints(::Constraints::GroupUrlConstrainer.new) do
         controller: :groups,
         constraints: { id: Gitlab::PathRegex.full_namespace_route_regex, format: /(html|json|atom|ics)/ }) do
     scope(path: '-') do
-      get :edit, as: :edit_group
-      get :issues, as: :issues_group_calendar, action: :issues_calendar, constraints: lambda { |req| req.format == :ics }
-      get :issues, as: :issues_group
-      get :merge_requests, as: :merge_requests_group
-      get :projects, as: :projects_group
-      get :details, as: :details_group
-      get :activity, as: :activity_group
-      put :transfer, as: :transfer_group
-      post :export, as: :export_group
-      get :download_export, as: :download_export_group
+      # These routes are legit and the cop rule will be improved in
+      # https://gitlab.com/gitlab-org/gitlab/-/issues/230703
+      get :edit, as: :edit_group # rubocop:disable Cop/PutGroupRoutesUnderScope
+      get :issues, as: :issues_group_calendar, action: :issues_calendar, constraints: lambda { |req| req.format == :ics } # rubocop:disable Cop/PutGroupRoutesUnderScope
+      get :issues, as: :issues_group # rubocop:disable Cop/PutGroupRoutesUnderScope
+      get :merge_requests, as: :merge_requests_group # rubocop:disable Cop/PutGroupRoutesUnderScope
+      get :projects, as: :projects_group # rubocop:disable Cop/PutGroupRoutesUnderScope
+      get :details, as: :details_group # rubocop:disable Cop/PutGroupRoutesUnderScope
+      get :activity, as: :activity_group # rubocop:disable Cop/PutGroupRoutesUnderScope
+      put :transfer, as: :transfer_group # rubocop:disable Cop/PutGroupRoutesUnderScope
+      post :export, as: :export_group # rubocop:disable Cop/PutGroupRoutesUnderScope
+      get :download_export, as: :download_export_group # rubocop:disable Cop/PutGroupRoutesUnderScope
 
       # TODO: Remove as part of refactor in https://gitlab.com/gitlab-org/gitlab-foss/issues/49693
-      get 'shared', action: :show, as: :group_shared
-      get 'archived', action: :show, as: :group_archived
+      get 'shared', action: :show, as: :group_shared # rubocop:disable Cop/PutGroupRoutesUnderScope
+      get 'archived', action: :show, as: :group_archived # rubocop:disable Cop/PutGroupRoutesUnderScope
     end
 
-    get '/', action: :show, as: :group_canonical
+    # These routes are legit and the cop rule will be improved in
+    # https://gitlab.com/gitlab-org/gitlab/-/issues/230703
+    get '/', action: :show, as: :group_canonical # rubocop:disable Cop/PutGroupRoutesUnderScope
   end
 
   scope(path: 'groups/*group_id/-',
@@ -106,9 +110,11 @@ constraints(::Constraints::GroupUrlConstrainer.new) do
         as: :group,
         constraints: { id: Gitlab::PathRegex.full_namespace_route_regex, format: /(html|json|atom)/ },
         controller: :groups) do
-    get '/', action: :show
-    patch '/', action: :update
-    put '/', action: :update
-    delete '/', action: :destroy
+    # These routes are legit and the cop rule will be improved in
+    # https://gitlab.com/gitlab-org/gitlab/-/issues/230703
+    get '/', action: :show # rubocop:disable Cop/PutGroupRoutesUnderScope
+    patch '/', action: :update # rubocop:disable Cop/PutGroupRoutesUnderScope
+    put '/', action: :update # rubocop:disable Cop/PutGroupRoutesUnderScope
+    delete '/', action: :destroy # rubocop:disable Cop/PutGroupRoutesUnderScope
   end
 end

config/routes/project.rb

@@ -362,18 +362,18 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
       #
       # Service Desk
       #
-      get '/service_desk' => 'service_desk#show', as: :service_desk
-      put '/service_desk' => 'service_desk#update', as: :service_desk_refresh
+      get '/service_desk' => 'service_desk#show', as: :service_desk # rubocop:todo Cop/PutProjectRoutesUnderScope
+      put '/service_desk' => 'service_desk#update', as: :service_desk_refresh # rubocop:todo Cop/PutProjectRoutesUnderScope
 
       #
       # Templates
       #
-      get '/templates/:template_type/:key' => 'templates#show',
+      get '/templates/:template_type/:key' => 'templates#show', # rubocop:todo Cop/PutProjectRoutesUnderScope
           as: :template,
           defaults: { format: 'json' },
           constraints: { key: %r{[^/]+}, template_type: %r{issue|merge_request}, format: 'json' }
 
-      get '/description_templates/names/:template_type',
+      get '/description_templates/names/:template_type', # rubocop:todo Cop/PutProjectRoutesUnderScope
           to: 'templates#names',
           as: :template_names,
           defaults: { format: 'json' },
@@ -382,39 +382,39 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
       resource :pages, only: [:show, :update, :destroy] do # rubocop: disable Cop/PutProjectRoutesUnderScope
         resources :domains, except: :index, controller: 'pages_domains', constraints: { id: %r{[^/]+} } do # rubocop: disable Cop/PutProjectRoutesUnderScope
           member do
-            post :verify
-            post :retry_auto_ssl
-            delete :clean_certificate
+            post :verify # rubocop:todo Cop/PutProjectRoutesUnderScope
+            post :retry_auto_ssl # rubocop:todo Cop/PutProjectRoutesUnderScope
+            delete :clean_certificate # rubocop:todo Cop/PutProjectRoutesUnderScope
           end
         end
       end
 
       namespace :prometheus do
         resources :alerts, constraints: { id: /\d+/ }, only: [:index, :create, :show, :update, :destroy] do # rubocop: disable Cop/PutProjectRoutesUnderScope
-          post :notify, on: :collection
+          post :notify, on: :collection # rubocop:todo Cop/PutProjectRoutesUnderScope
           member do
-            get :metrics_dashboard
+            get :metrics_dashboard # rubocop:todo Cop/PutProjectRoutesUnderScope
           end
         end
 
         resources :metrics, constraints: { id: %r{[^\/]+} }, only: [:index, :new, :create, :edit, :update, :destroy] do # rubocop: disable Cop/PutProjectRoutesUnderScope
-          get :active_common, on: :collection
-          post :validate_query, on: :collection
+          get :active_common, on: :collection # rubocop:todo Cop/PutProjectRoutesUnderScope
+          post :validate_query, on: :collection # rubocop:todo Cop/PutProjectRoutesUnderScope
         end
       end
 
-      post 'alerts/notify', to: 'alerting/notifications#create'
+      post 'alerts/notify', to: 'alerting/notifications#create' # rubocop:todo Cop/PutProjectRoutesUnderScope
 
       draw :legacy_builds
 
       resources :hooks, only: [:index, :create, :edit, :update, :destroy], constraints: { id: /\d+/ } do # rubocop: disable Cop/PutProjectRoutesUnderScope
         member do
-          post :test
+          post :test # rubocop:todo Cop/PutProjectRoutesUnderScope
         end
 
         resources :hook_logs, only: [:show] do # rubocop: disable Cop/PutProjectRoutesUnderScope
           member do
-            post :retry
+            post :retry # rubocop:todo Cop/PutProjectRoutesUnderScope
           end
         end
       end
@@ -431,7 +431,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
             resources :tags, only: [:index, :destroy], # rubocop: disable Cop/PutProjectRoutesUnderScope
                              constraints: { id: Gitlab::Regex.container_registry_tag_regex } do
               collection do
-                delete :bulk_destroy
+                delete :bulk_destroy # rubocop:todo Cop/PutProjectRoutesUnderScope
               end
             end
           end
@@ -440,32 +440,32 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
 
       resources :notes, only: [:create, :destroy, :update], concerns: :awardable, constraints: { id: /\d+/ } do # rubocop: disable Cop/PutProjectRoutesUnderScope
         member do
-          delete :delete_attachment
-          post :resolve
-          delete :resolve, action: :unresolve
+          delete :delete_attachment # rubocop:todo Cop/PutProjectRoutesUnderScope
+          post :resolve # rubocop:todo Cop/PutProjectRoutesUnderScope
+          delete :resolve, action: :unresolve # rubocop:todo Cop/PutProjectRoutesUnderScope
         end
       end
 
-      get 'noteable/:target_type/:target_id/notes' => 'notes#index', as: 'noteable_notes'
+      get 'noteable/:target_type/:target_id/notes' => 'notes#index', as: 'noteable_notes' # rubocop:todo Cop/PutProjectRoutesUnderScope
 
       resources :todos, only: [:create] # rubocop: disable Cop/PutProjectRoutesUnderScope
 
       resources :uploads, only: [:create] do # rubocop: disable Cop/PutProjectRoutesUnderScope
         collection do
-          get ":secret/:filename", action: :show, as: :show, constraints: { filename: %r{[^/]+} }, format: false, defaults: { format: nil }
-          post :authorize
+          get ":secret/:filename", action: :show, as: :show, constraints: { filename: %r{[^/]+} }, format: false, defaults: { format: nil } # rubocop:todo Cop/PutProjectRoutesUnderScope
+          post :authorize # rubocop:todo Cop/PutProjectRoutesUnderScope
         end
       end
 
       resources :runners, only: [:index, :edit, :update, :destroy, :show] do # rubocop: disable Cop/PutProjectRoutesUnderScope
         member do
-          post :resume
-          post :pause
+          post :resume # rubocop:todo Cop/PutProjectRoutesUnderScope
+          post :pause # rubocop:todo Cop/PutProjectRoutesUnderScope
         end
 
         collection do
-          post :toggle_shared_runners
-          post :toggle_group_runners
+          post :toggle_shared_runners # rubocop:todo Cop/PutProjectRoutesUnderScope
+          post :toggle_group_runners # rubocop:todo Cop/PutProjectRoutesUnderScope
         end
       end
 
@@ -474,26 +474,26 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
         collection do
           scope '*ref', constraints: { ref: Gitlab::PathRegex.git_reference_regex } do
             constraints format: /svg/ do
-              get :pipeline
-              get :coverage
+              get :pipeline # rubocop:todo Cop/PutProjectRoutesUnderScope
+              get :coverage # rubocop:todo Cop/PutProjectRoutesUnderScope
             end
           end
         end
       end
 
       scope :usage_ping, controller: :usage_ping do
-        post :web_ide_clientside_preview
-        post :web_ide_pipelines_count
+        post :web_ide_clientside_preview # rubocop:todo Cop/PutProjectRoutesUnderScope
+        post :web_ide_pipelines_count # rubocop:todo Cop/PutProjectRoutesUnderScope
       end
 
       resources :web_ide_terminals, path: :ide_terminals, only: [:create, :show], constraints: { id: /\d+/, format: :json } do # rubocop: disable Cop/PutProjectRoutesUnderScope
         member do
-          post :cancel
-          post :retry
+          post :cancel # rubocop:todo Cop/PutProjectRoutesUnderScope
+          post :retry # rubocop:todo Cop/PutProjectRoutesUnderScope
         end
 
         collection do
-          post :check_config
+          post :check_config # rubocop:todo Cop/PutProjectRoutesUnderScope
         end
       end
 
@@ -506,8 +506,8 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
         # Issue https://gitlab.com/gitlab-org/gitlab/-/issues/29572
         resources :snippets, concerns: :awardable, constraints: { id: /\d+/ } do # rubocop: disable Cop/PutProjectRoutesUnderScope
           member do
-            get :raw
-            post :mark_as_spam
+            get :raw # rubocop:todo Cop/PutProjectRoutesUnderScope
+            post :mark_as_spam # rubocop:todo Cop/PutProjectRoutesUnderScope
           end
         end
       end

ee/config/routes/group.rb

@@ -5,7 +5,7 @@ constraints(::Constraints::GroupUrlConstrainer.new) do
         controller: :groups,
         constraints: { id: Gitlab::PathRegex.full_namespace_route_regex, format: /(html|json|atom|ics)/ }) do
     scope(path: '-') do
-      get :subgroups, as: :subgroups_group
+      get :subgroups, as: :subgroups_group # rubocop:todo Cop/PutGroupRoutesUnderScope
     end
   end
 
@@ -184,10 +184,10 @@ end
 # Dependency proxy for containers
 # Because docker adds v2 prefix to URI this need to be outside of usual group routes
 scope format: false do
-  get 'v2', to: proc { [200, {}, ['']] }
+  get 'v2', to: proc { [200, {}, ['']] } # rubocop:disable Cop/PutGroupRoutesUnderScope
 
   constraints image: Gitlab::PathRegex.container_image_regex, sha: Gitlab::PathRegex.container_image_blob_sha_regex do
-    get 'v2/*group_id/dependency_proxy/containers/*image/manifests/*tag' => 'groups/dependency_proxy_for_containers#manifest'
-    get 'v2/*group_id/dependency_proxy/containers/*image/blobs/:sha' => 'groups/dependency_proxy_for_containers#blob'
+    get 'v2/*group_id/dependency_proxy/containers/*image/manifests/*tag' => 'groups/dependency_proxy_for_containers#manifest' # rubocop:todo Cop/PutGroupRoutesUnderScope
+    get 'v2/*group_id/dependency_proxy/containers/*image/blobs/:sha' => 'groups/dependency_proxy_for_containers#blob' # rubocop:todo Cop/PutGroupRoutesUnderScope
   end
 end

ee/config/routes/project.rb

@@ -141,6 +141,8 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
   end
 end
 
+# It's under /-/jira scope but cop is only checking /-/
+# rubocop: disable Cop/PutProjectRoutesUnderScope
 scope path: '(/-/jira)', constraints: ::Constraints::JiraEncodedUrlConstrainer.new, as: :jira do
   scope path: '*namespace_id/:project_id',
         namespace_id: Gitlab::Jira::Dvcs::ENCODED_ROUTE_REGEX,
@@ -171,3 +173,4 @@ scope path: '(/-/jira)', constraints: ::Constraints::JiraEncodedUrlConstrainer.n
     }
   end
 end
+# rubocop: enable Cop/PutProjectRoutesUnderScope

rubocop/cop/put_group_routes_under_scope.rb

 # frozen_string_literal: true
 
+require_relative '../routes_under_scope'
+
 module RuboCop
   module Cop
     # Checks for a group routes outside '/-/' scope.
     # For more information see: https://gitlab.com/gitlab-org/gitlab/issues/29572
     class PutGroupRoutesUnderScope < RuboCop::Cop::Cop
+      include RoutesUnderScope
+
       MSG = 'Put new group routes under /-/ scope'
 
       def_node_matcher :dash_scope?, <<~PATTERN
         (:send nil? :scope (hash <(pair (sym :path)(str "groups/*group_id/-")) ...>))
       PATTERN
-
-      def on_send(node)
-        return unless in_group_routes?(node)
-        return unless resource?(node)
-        return unless outside_scope?(node)
-
-        add_offense(node)
-      end
-
-      def outside_scope?(node)
-        node.each_ancestor(:block).none? do |parent|
-          dash_scope?(parent.to_a.first)
-        end
-      end
-
-      def in_group_routes?(node)
-        path = node.location.expression.source_buffer.name
-        dirname = File.dirname(path)
-        filename = File.basename(path)
-
-        dirname.end_with?('config/routes') &&
-          filename.end_with?('group.rb')
-      end
-
-      def resource?(node)
-        node.method_name == :resource ||
-          node.method_name == :resources
-      end
     end
   end
 end

rubocop/cop/put_project_routes_under_scope.rb

 # frozen_string_literal: true
 
+require_relative '../routes_under_scope'
+
 module RuboCop
   module Cop
     # Checks for a project routes outside '/-/' scope.
     # For more information see: https://gitlab.com/gitlab-org/gitlab/issues/29572
     class PutProjectRoutesUnderScope < RuboCop::Cop::Cop
+      include RoutesUnderScope
+
       MSG = 'Put new project routes under /-/ scope'
 
       def_node_matcher :dash_scope?, <<~PATTERN
         (:send nil? :scope (:str "-"))
       PATTERN
-
-      def on_send(node)
-        return unless in_project_routes?(node)
-        return unless resource?(node)
-        return unless outside_scope?(node)
-
-        add_offense(node)
-      end
-
-      def outside_scope?(node)
-        node.each_ancestor(:block).none? do |parent|
-          dash_scope?(parent.to_a.first)
-        end
-      end
-
-      def in_project_routes?(node)
-        path = node.location.expression.source_buffer.name
-        dirname = File.dirname(path)
-        filename = File.basename(path)
-
-        dirname.end_with?('config/routes') &&
-          filename.end_with?('project.rb')
-      end
-
-      def resource?(node)
-        node.method_name == :resource ||
-          node.method_name == :resources
-      end
     end
   end
 end

rubocop/routes_under_scope.rb

+# frozen_string_literal: true
+
+module RuboCop
+  # Common code used to implement cops checking routes outside of /-/ scope.
+  #
+  # Examples:
+  # * RuboCop::Cop::PutProjectRoutesUnderScope
+  # * RuboCop::Cop::PutGroupRoutesUnderScope
+  module RoutesUnderScope
+    ROUTE_METHODS = Set.new(%i[resource resources get post put patch delete]).freeze
+
+    def on_send(node)
+      return unless route_method?(node)
+      return unless outside_scope?(node)
+
+      add_offense(node)
+    end
+
+    def outside_scope?(node)
+      node.each_ancestor(:block).none? do |parent|
+        dash_scope?(parent.to_a.first)
+      end
+    end
+
+    def route_method?(node)
+      ROUTE_METHODS.include?(node.method_name)
+    end
+  end
+end

spec/rubocop/cop/put_group_routes_under_scope_spec.rb

@@ -9,20 +9,21 @@ RSpec.describe RuboCop::Cop::PutGroupRoutesUnderScope, type: :rubocop do
 
   subject(:cop) { described_class.new }
 
-  before do
-    allow(cop).to receive(:in_group_routes?).and_return(true)
-  end
+  %w[resource resources get post put patch delete].each do |route_method|
+    it "registers an offense when route is outside scope for `#{route_method}`" do
+      offense = "#{route_method} :notes"
+      marker = '^' * offense.size
 
-  it 'registers an offense when route is outside scope' do
       expect_offense(<<~PATTERN)
       scope(path: 'groups/*group_id/-', module: :groups) do
         resource :issues
       end
 
-      resource :notes
-      ^^^^^^^^^^^^^^^ Put new group routes under /-/ scope
+      #{offense}
+      #{marker} Put new group routes under /-/ scope
       PATTERN
     end
+  end
 
   it 'does not register an offense when resource inside the scope' do
     expect_no_offenses(<<~PATTERN)

spec/rubocop/cop/put_project_routes_under_scope_spec.rb

@@ -9,20 +9,21 @@ RSpec.describe RuboCop::Cop::PutProjectRoutesUnderScope, type: :rubocop do
 
   subject(:cop) { described_class.new }
 
-  before do
-    allow(cop).to receive(:in_project_routes?).and_return(true)
-  end
+  %w[resource resources get post put patch delete].each do |route_method|
+    it "registers an offense when route is outside scope for `#{route_method}`" do
+      offense = "#{route_method} :notes"
+      marker = '^' * offense.size
 
-  it 'registers an offense when route is outside scope' do
       expect_offense(<<~PATTERN)
       scope '-' do
         resource :issues
       end
 
-      resource :notes
-      ^^^^^^^^^^^^^^^ Put new project routes under /-/ scope
+      #{offense}
+      #{marker} Put new project routes under /-/ scope
       PATTERN
     end
+  end
 
   it 'does not register an offense when resource inside the scope' do
     expect_no_offenses(<<~PATTERN)