From 66ac821cfc0ed1f81ffe074dab3d8380faa9971e Mon Sep 17 00:00:00 2001
From: Halil Coban <hcoban@gitlab.com>
Date: Mon, 9 Dec 2024 04:44:10 +0000
Subject: [PATCH] Add RuboCop rule to enforce class-level
allow_access_with_scope
Implement RuboCop rule to prevent misuse of allow_access_with_scope
method. This ensures the method is only used at the class level, not
nested in namespaces. Because, if this method is called multiple times
on the same class, the scopes are all aggregated
Changelog: other
---
.rubocop.yml | 6 +
.../class_level_allow_access_with_scope.yml | 3 +
.../class_level_allow_access_with_scope.rb | 54 ++++++++
...lass_level_allow_access_with_scope_spec.rb | 120 ++++++++++++++++++
4 files changed, 183 insertions(+)
create mode 100644 .rubocop_todo/api/class_level_allow_access_with_scope.yml
create mode 100644 rubocop/cop/api/class_level_allow_access_with_scope.rb
create mode 100644 spec/rubocop/cop/api/class_level_allow_access_with_scope_spec.rb
diff --git a/.rubocop.yml b/.rubocop.yml
index 9171c586b9c62..66db1f36d72f3 100644
--- a/.rubocop.yml
+++ b/.rubocop.yml
@@ -723,6 +723,12 @@ API/GrapeArrayMissingCoerce:
- 'lib/**/api/**/*.rb'
- 'ee/**/api/**/*.rb'
+API/ClassLevelAllowAccessWithScope:
+ Enabled: true
+ Include:
+ - 'lib/**/api/**/*.rb'
+ - 'ee/lib/**/api/**/*.rb'
+
Cop/SidekiqOptionsQueue:
Enabled: true
Exclude:
diff --git a/.rubocop_todo/api/class_level_allow_access_with_scope.yml b/.rubocop_todo/api/class_level_allow_access_with_scope.yml
new file mode 100644
index 0000000000000..9ebd0fb8c0cd4
--- /dev/null
+++ b/.rubocop_todo/api/class_level_allow_access_with_scope.yml
@@ -0,0 +1,3 @@
+---
+API/ClassLevelAllowAccessWithScope:
+ Details: grace period
diff --git a/rubocop/cop/api/class_level_allow_access_with_scope.rb b/rubocop/cop/api/class_level_allow_access_with_scope.rb
new file mode 100644
index 0000000000000..ad045584e47f7
--- /dev/null
+++ b/rubocop/cop/api/class_level_allow_access_with_scope.rb
@@ -0,0 +1,54 @@
+# frozen_string_literal: true
+
+require_relative '../../code_reuse_helpers'
+
+module RuboCop
+ module Cop
+ module API
+ class ClassLevelAllowAccessWithScope < RuboCop::Cop::Base
+ include CodeReuseHelpers
+
+ # This cop checks that `allow_access_with_scope` is called only at the class level.
+ # This is because `allow_access_with_scope` aggregates scopes for each call in a class.
+ # Calling `allow_access_with_scope` within a `namespace` or an alias method such as
+ # `resource`, `resources`, `segment` or `group` may mislead developers to think the scope
+ # would be only allowed within given namespace which is not the case.
+ #
+ # @example
+ #
+ # # bad
+ # class MyClass < ::API::Base
+ # include APIGuard
+ # namespace 'my_namespace' do
+ # resource :my_resource do
+ # allow_access_with_scope :ai_workflows
+ #
+ # # good
+ # class MyClass < ::API::Base
+ # include APIGuard
+ # allow_access_with_scope :ai_workflows
+ #
+ MSG = '`allow_access_with_scope` should only be called on class-level and not within a namespace.'
+
+ # In Grape::DSL::Routing::ClassMethods
+ # group, segment, resource, and resources are all aliased to namespace
+ BANNED_BLOCKS = %i[group namespace resource resources segment].freeze
+
+ RESTRICT_ON_SEND = %i[allow_access_with_scope].freeze
+ def on_send(node)
+ return unless namespace?(node)
+
+ add_offense(node)
+ end
+
+ private
+
+ def namespace?(node)
+ node.each_ancestor(:block).any? do |block_node|
+ BANNED_BLOCKS.include?(block_node.method_name)
+ end
+ end
+ end
+ end
+ end
+end
diff --git a/spec/rubocop/cop/api/class_level_allow_access_with_scope_spec.rb b/spec/rubocop/cop/api/class_level_allow_access_with_scope_spec.rb
new file mode 100644
index 0000000000000..30b124959c65c
--- /dev/null
+++ b/spec/rubocop/cop/api/class_level_allow_access_with_scope_spec.rb
@@ -0,0 +1,120 @@
+# frozen_string_literal: true
+
+require 'rubocop_spec_helper'
+require_relative '../../../../rubocop/cop/api/class_level_allow_access_with_scope'
+
+RSpec.describe RuboCop::Cop::API::ClassLevelAllowAccessWithScope, feature_category: :shared do
+ let(:msg) { described_class::MSG }
+
+ context "when there is no `allow_access_with_scope`" do
+ it "does not add an offense" do
+ expect_no_offenses(<<~CODE)
+ class MyClass < ::API::Base
+ include APIGuard
+ namespace 'my_namespace' do
+
+ end
+ end
+ CODE
+ end
+ end
+
+ context "when there is class level `allow_access_with_scope`" do
+ it "does not add an offense" do
+ expect_no_offenses(<<~CODE)
+ class MyClass < ::API::Base
+ include APIGuard
+ allow_access_with_scope :my_scope
+ namespace 'my_namespace' do
+
+ end
+ end
+ CODE
+ end
+ end
+
+ context "when there is `allow_access_with_scope` under namespace" do
+ it "adds an offense" do
+ expect_offense(<<~CODE)
+ class MyClass < ::API::Base
+ include APIGuard
+ namespace 'my_namespace' do
+ allow_access_with_scope :my_scope
+ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{msg}
+ end
+ end
+ CODE
+ end
+ end
+
+ context "when there is `allow_access_with_scope` under group" do
+ it "adds an offense" do
+ expect_offense(<<~CODE)
+ class MyClass < ::API::Base
+ include APIGuard
+ group 'my_namespace' do
+ allow_access_with_scope :my_scope
+ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{msg}
+ end
+ end
+ CODE
+ end
+ end
+
+ context "when there is `allow_access_with_scope` under resource" do
+ it "adds an offense" do
+ expect_offense(<<~CODE)
+ class MyClass < ::API::Base
+ include APIGuard
+ resource 'my_namespace' do
+ allow_access_with_scope :my_scope
+ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{msg}
+ end
+ end
+ CODE
+ end
+ end
+
+ context "when there is `allow_access_with_scope` under resources" do
+ it "adds an offense" do
+ expect_offense(<<~CODE)
+ class MyClass < ::API::Base
+ include APIGuard
+ resources 'my_namespace' do
+ allow_access_with_scope :my_scope
+ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{msg}
+ end
+ end
+ CODE
+ end
+ end
+
+ context "when there is `allow_access_with_scope` under segment" do
+ it "adds an offense" do
+ expect_offense(<<~CODE)
+ class MyClass < ::API::Base
+ include APIGuard
+ segment 'my_namespace' do
+ allow_access_with_scope :my_scope
+ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{msg}
+ end
+ end
+ CODE
+ end
+ end
+
+ context "when there are `allow_access_with_scope`s both class level and under namespace" do
+ it "adds an offense" do
+ expect_offense(<<~CODE)
+ class MyClass < ::API::Base
+ include APIGuard
+ allow_access_with_scope :my_scope
+ namespace 'my_namespace' do
+ allow_access_with_scope :my_scope
+ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{msg}
+ end
+ end
+ CODE
+ end
+ end
+end
--
GitLab