Merge branch 'chore/rubocop-user-admin' into 'master'

Create Cop to enforce using policies framework for administrators

See merge request gitlab-org/gitlab!55693

.haml-lint.yml

@@ -150,6 +150,9 @@ linters:
       - Style/WordArray
       - Style/ZeroLengthPredicate
 
+      # WIP See https://gitlab.com/gitlab-org/gitlab/-/issues/207950
+      - Cop/UserAdmin
+
   RubyComments:
     enabled: true
 

.rubocop.yml

@@ -629,3 +629,14 @@ Lint/RedundantSafeNavigation:
 
 Style/ClassEqualityComparison:
   Enabled: true
+
+# WIP See https://gitlab.com/gitlab-org/gitlab/-/issues/207950
+Cop/UserAdmin:
+  Enabled: true
+  Exclude:
+    - 'app/controllers/admin/sessions_controller.rb'
+    - 'app/controllers/concerns/enforces_admin_authentication.rb'
+    - 'app/policies/base_policy.rb'
+    - 'lib/gitlab/auth/current_user_mode.rb'
+    - 'spec/**/*.rb'
+    - 'ee/spec/**/*.rb'

.rubocop_manual_todo.yml

@@ -2497,3 +2497,79 @@ Style/HashTransformation:
     - 'spec/support/helpers/graphql_helpers.rb'
     - 'spec/support/import_export/project_tree_expectations.rb'
     - 'spec/support/shared_contexts/services/projects/container_repository/delete_tags_service_shared_context.rb'
+
+Style/ClassEqualityComparison:
+  Exclude:
+    - spec/lib/peek/views/active_record_spec.rb
+    - ee/spec/lib/peek/views/active_record_spec.rb
+
+# WIP See https://gitlab.com/gitlab-org/gitlab/-/issues/207950
+Cop/UserAdmin:
+  Exclude:
+    - 'app/controllers/admin/impersonations_controller.rb'
+    - 'app/controllers/concerns/spammable_actions.rb'
+    - 'app/controllers/sessions_controller.rb'
+    - 'app/finders/autocomplete/routes_finder.rb'
+    - 'app/finders/ci/jobs_finder.rb'
+    - 'app/finders/ci/runners_finder.rb'
+    - 'app/finders/personal_access_tokens_finder.rb'
+    - 'app/finders/users_finder.rb'
+    - 'app/graphql/mutations/admin/sidekiq_queues/delete_jobs.rb'
+    - 'app/graphql/resolvers/admin/analytics/usage_trends/measurements_resolver.rb'
+    - 'app/helpers/application_helper.rb'
+    - 'app/helpers/import_helper.rb'
+    - 'app/helpers/nav_helper.rb'
+    - 'app/helpers/projects_helper.rb'
+    - 'app/helpers/search_helper.rb'
+    - 'app/helpers/user_callouts_helper.rb'
+    - 'app/helpers/users_helper.rb'
+    - 'app/helpers/visibility_level_helper.rb'
+    - 'app/models/concerns/protected_ref_access.rb'
+    - 'app/models/concerns/spammable.rb'
+    - 'app/models/issue_collection.rb'
+    - 'app/models/merge_requests_closing_issues.rb'
+    - 'app/models/protected_branch.rb'
+    - 'app/models/user.rb'
+    - 'app/policies/note_policy.rb'
+    - 'app/serializers/deploy_key_entity.rb'
+    - 'app/services/auth/container_registry_authentication_service.rb'
+    - 'app/services/emails/create_service.rb'
+    - 'app/services/projects/enable_deploy_key_service.rb'
+    - 'app/services/projects/fork_service.rb'
+    - 'app/services/users/build_service.rb'
+    - 'ee/app/controllers/ee/projects_controller.rb'
+    - 'ee/app/graphql/mutations/admin/analytics/devops_adoption/segments/mixins.rb'
+    - 'ee/app/graphql/resolvers/admin/analytics/devops_adoption/segments_resolver.rb'
+    - 'ee/app/helpers/ee/dashboard_helper.rb'
+    - 'ee/app/helpers/ee/import_helper.rb'
+    - 'ee/app/helpers/ee/subscribable_banner_helper.rb'
+    - 'ee/app/helpers/ee/user_callouts_helper.rb'
+    - 'ee/app/helpers/license_monitoring_helper.rb'
+    - 'ee/app/helpers/push_rules_helper.rb'
+    - 'ee/app/models/concerns/ee/protected_ref_access.rb'
+    - 'ee/app/models/ee/user.rb'
+    - 'ee/app/models/protected_environment/deploy_access_level.rb'
+    - 'ee/app/policies/ee/group_policy.rb'
+    - 'ee/app/policies/ee/project_policy.rb'
+    - 'ee/app/services/ee/groups/create_service.rb'
+    - 'ee/app/services/ee/groups/update_service.rb'
+    - 'ee/app/services/ee/projects/update_service.rb'
+    - 'ee/lib/ee/api/helpers.rb'
+    - 'ee/lib/ee/gitlab/git_access.rb'
+    - 'lib/api/award_emoji.rb'
+    - 'lib/api/ci/runners.rb'
+    - 'lib/api/entities/runner_details.rb'
+    - 'lib/api/entities/user_safe.rb'
+    - 'lib/api/groups.rb'
+    - 'lib/api/helpers.rb'
+    - 'lib/api/personal_access_tokens.rb'
+    - 'lib/api/users.rb'
+    - 'lib/api/v3/github.rb'
+    - 'lib/constraints/admin_constrainer.rb'
+    - 'lib/gitlab/auth.rb'
+    - 'lib/gitlab/background_migration/user_mentions/models/group.rb'
+    - 'lib/gitlab/ci/runner_instructions.rb'
+    - 'lib/gitlab/import_export/members_mapper.rb'
+    - 'lib/gitlab/performance_bar.rb'
+    - 'lib/gitlab/visibility_level.rb'
+    - 'qa/qa/runtime/api/client.rb'

changelogs/unreleased/chore-rubocop-user-admin.yml

+---
+title: Create Cop to enforce using policies framework for administrators
+merge_request: 55693
+author: Diego Louzán
+type: other

rubocop/cop/user_admin.rb

+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    # Cop that rejects the usage of `User#admin?`
+    class UserAdmin < RuboCop::Cop::Cop
+      MSG = 'Direct calls to `User#admin?` to determine admin status should be ' \
+        'avoided as they will not take into account the policies framework ' \
+        'and will ignore Admin Mode if enabled. Please use a policy check ' \
+        'with `User#can_admin_all_resources?` or `User#can_read_all_resources?`.'
+
+      def_node_matcher :admin_call?, <<~PATTERN
+        ({send | csend} _ :admin? ...)
+      PATTERN
+
+      def on_send(node)
+        on_handler(node)
+      end
+
+      def on_csend(node)
+        on_handler(node)
+      end
+
+      private
+
+      def on_handler(node)
+        return unless admin_call?(node)
+
+        add_offense(node, location: :selector)
+      end
+    end
+  end
+end

spec/rubocop/cop/user_admin_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+
+require 'rubocop'
+require_relative '../../../rubocop/cop/user_admin'
+
+RSpec.describe RuboCop::Cop::UserAdmin do
+  subject(:cop) { described_class.new }
+
+  it 'flags a method call' do
+    expect_offense(<<~SOURCE)
+      user.admin?
+           ^^^^^^ #{described_class::MSG}
+    SOURCE
+  end
+
+  it 'flags a method call with safe operator' do
+    expect_offense(<<~SOURCE)
+      user&.admin?
+            ^^^^^^ #{described_class::MSG}
+    SOURCE
+  end
+end