Merge branch 'introduce-a-rubocop-check-for-license-available-usage' into 'master'

Introduce a rubocop check for license available usage

See merge request gitlab-org/gitlab!53278

.haml-lint.yml

@@ -106,6 +106,7 @@ linters:
       - Cop/LineBreakAfterGuardClauses
       - Cop/LineBreakAroundConditionalBlock
       - Cop/ProjectPathHelper
+      - Gitlab/FeatureAvailableUsage
       - GitlabSecurity/PublicSend
       - Layout/EmptyLineAfterGuardClause
       - Layout/LeadingCommentSpace

app/models/namespace.rb

@@ -287,8 +287,13 @@ class Namespace < ApplicationRecord
     false
   end
 
+  # Deprecated, use #licensed_feature_available? instead. Remove once Namespace#feature_available? isn't used anymore.
+  def feature_available?(feature)
+    licensed_feature_available?(feature)
+  end
+
   # Overridden in EE::Namespace
-  def feature_available?(_feature)
+  def licensed_feature_available?(_feature)
     false
   end
 

app/models/project.rb

@@ -2324,6 +2324,11 @@ class Project < ApplicationRecord
                .external_authorization_service_default_label
   end
 
+  # Overridden in EE::Project
+  def licensed_feature_available?(_feature)
+    false
+  end
+
   def licensed_features
     []
   end

app/models/project_feature.rb

@@ -3,6 +3,7 @@
 class ProjectFeature < ApplicationRecord
   include Featurable
 
+  # When updating this array, make sure to update rubocop/cop/gitlab/feature_available_usage.rb as well.
   FEATURES = %i[
     issues
     forking

ee/app/models/ee/namespace.rb

@@ -146,8 +146,8 @@ module EE
     # Checks features (i.e. https://about.gitlab.com/pricing/) availabily
     # for a given Namespace plan. This method should consider ancestor groups
     # being licensed.
-    override :feature_available?
-    def feature_available?(feature)
+    override :licensed_feature_available?
+    def licensed_feature_available?(feature)
       available_features = strong_memoize(:feature_available) do
         Hash.new do |h, f|
           h[f] = load_feature_available(f)

ee/app/models/ee/project.rb

@@ -804,6 +804,16 @@ module EE
       jira_issue_association_required_to_merge_enabled? && prevent_merge_without_jira_issue
     end
 
+    def licensed_feature_available?(feature, user = nil)
+      available_features = strong_memoize(:licensed_feature_available) do
+        Hash.new do |h, f|
+          h[f] = load_licensed_feature_available(f)
+        end
+      end
+
+      available_features[feature]
+    end
+
     private
 
     def group_hooks
@@ -819,16 +829,6 @@ module EE
       import_state.set_next_execution_to_now
     end
 
-    def licensed_feature_available?(feature, user = nil)
-      available_features = strong_memoize(:licensed_feature_available) do
-        Hash.new do |h, f|
-          h[f] = load_licensed_feature_available(f)
-        end
-      end
-
-      available_features[feature]
-    end
-
     def load_licensed_feature_available(feature)
       globally_available = License.feature_available?(feature)
 

ee/app/models/ee/project_feature.rb

@@ -4,6 +4,7 @@ module EE
   module ProjectFeature
     extend ActiveSupport::Concern
 
+    # When updating this array, make sure to update rubocop/cop/gitlab/feature_available_usage.rb as well.
     EE_FEATURES = %i(requirements).freeze
     NOTES_PERMISSION_TRACKED_FIELDS = %w(
       issues_access_level

lib/gitlab/background_migration/user_mentions/models/namespace.rb

@@ -23,8 +23,13 @@ module Gitlab
             parent_id.present? || parent.present?
           end
 
+          # Deprecated, use #licensed_feature_available? instead. Remove once Namespace#feature_available? isn't used anymore.
+          def feature_available?(feature)
+            licensed_feature_available?(feature)
+          end
+
           # Overridden in EE::Namespace
-          def feature_available?(_feature)
+          def licensed_feature_available?(_feature)
             false
           end
         end

rubocop/cop/gitlab/feature_available_usage.rb

+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Gitlab
+      # Cop that checks for correct calling of #feature_available?
+      class FeatureAvailableUsage < RuboCop::Cop::Cop
+        OBSERVED_METHOD = :feature_available?
+        LICENSED_FEATURE_LITERAL_ARG_MSG = '`feature_available?` should not be called for features that can be licensed (`%s` given), use `licensed_feature_available?(feature)` instead.'
+        LICENSED_FEATURE_DYNAMIC_ARG_MSG = "`feature_available?` should not be called for features that can be licensed (`%s` isn't a literal so we cannot say if it's legit or not), using `licensed_feature_available?(feature)` may be more appropriate."
+        NOT_ENOUGH_ARGS_MSG = '`feature_available?` should be called with two arguments: `feature` and `user`.'
+        FEATURES = %i[
+          issues
+          forking
+          merge_requests
+          wiki
+          snippets
+          builds
+          repository
+          pages
+          metrics_dashboard
+          analytics
+          operations
+          security_and_compliance
+          container_registry
+        ].freeze
+        EE_FEATURES = %i[requirements].freeze
+        ALL_FEATURES = (FEATURES + EE_FEATURES).freeze
+        SPECIAL_CLASS = %w[License].freeze
+
+        def on_send(node)
+          return unless method_name(node) == OBSERVED_METHOD
+          return if caller_is_special_case?(node)
+          return if feature_name(node).nil?
+          return if ALL_FEATURES.include?(feature_name(node)) && args_count(node) == 2
+
+          if !ALL_FEATURES.include?(feature_name(node))
+            add_offense(node, location: :expression, message: licensed_feature_message(node))
+          elsif args_count(node) < 2
+            add_offense(node, location: :expression, message: NOT_ENOUGH_ARGS_MSG)
+          end
+        end
+
+        def licensed_feature_message(node)
+          message_template = dynamic_feature?(node) ? LICENSED_FEATURE_DYNAMIC_ARG_MSG : LICENSED_FEATURE_LITERAL_ARG_MSG
+
+          message_template % feature_arg_name(node)
+        end
+
+        private
+
+        def method_name(node)
+          node.children[1]
+        end
+
+        def class_caller(node)
+          node.children[0]&.const_name.to_s
+        end
+
+        def caller_is_special_case?(node)
+          SPECIAL_CLASS.include?(class_caller(node))
+        end
+
+        def feature_arg(node)
+          node.children[2]
+        end
+
+        def dynamic_feature?(node)
+          feature = feature_arg(node)
+          return false unless feature
+
+          !feature.literal?
+        end
+
+        def feature_name(node)
+          feature = feature_arg(node)
+          return unless feature
+          return feature.children.compact.join('.') if dynamic_feature?(node)
+          return feature.value if feature.respond_to?(:value)
+
+          feature.type
+        end
+
+        def feature_arg_name(node)
+          feature = feature_arg(node)
+          return unless feature
+          return feature.children.compact.join('.') if dynamic_feature?(node)
+          return feature.children[0].inspect if feature.literal?
+
+          feature.type
+        end
+
+        def args_count(node)
+          node.children[2..].size
+        end
+      end
+    end
+  end
+end

spec/rubocop/cop/gitlab/feature_available_usage_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require 'rubocop'
+require 'rubocop/rspec/support'
+require_relative '../../../../rubocop/cop/gitlab/feature_available_usage'
+
+RSpec.describe RuboCop::Cop::Gitlab::FeatureAvailableUsage do
+  subject(:cop) { described_class.new }
+
+  context 'no arguments given' do
+    it 'does not flag the use of Gitlab::Sourcegraph.feature_available? with no arguments' do
+      expect_no_offenses('Gitlab::Sourcegraph.feature_available?')
+      expect_no_offenses('subject { described_class.feature_available? }')
+    end
+  end
+
+  context 'one argument given' do
+    it 'does not flag the use of License.feature_available?' do
+      expect_no_offenses('License.feature_available?(:push_rules)')
+    end
+
+    it 'flags the use with a dynamic feature as nil' do
+      expect_offense(<<~SOURCE)
+        feature_available?(nil)
+        ^^^^^^^^^^^^^^^^^^^^^^^ `feature_available?` should not be called for features that can be licensed (`nil` given), use `licensed_feature_available?(feature)` instead.
+      SOURCE
+      expect_offense(<<~SOURCE)
+        project.feature_available?(nil)
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `feature_available?` should not be called for features that can be licensed (`nil` given), use `licensed_feature_available?(feature)` instead.
+      SOURCE
+    end
+
+    it 'flags the use with an OSS project feature' do
+      expect_offense(<<~SOURCE)
+        project.feature_available?(:issues)
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `feature_available?` should be called with two arguments: `feature` and `user`.
+      SOURCE
+      expect_offense(<<~SOURCE)
+        feature_available?(:issues)
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^ `feature_available?` should be called with two arguments: `feature` and `user`.
+      SOURCE
+    end
+
+    it 'flags the use with a feature that is not a project feature' do
+      expect_offense(<<~SOURCE)
+        feature_available?(:foo)
+        ^^^^^^^^^^^^^^^^^^^^^^^^ `feature_available?` should not be called for features that can be licensed (`:foo` given), use `licensed_feature_available?(feature)` instead.
+      SOURCE
+      expect_offense(<<~SOURCE)
+        project.feature_available?(:foo)
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `feature_available?` should not be called for features that can be licensed (`:foo` given), use `licensed_feature_available?(feature)` instead.
+      SOURCE
+      expect_offense(<<~SOURCE)
+        feature_available?(foo)
+        ^^^^^^^^^^^^^^^^^^^^^^^ `feature_available?` should not be called for features that can be licensed (`foo` isn't a literal so we cannot say if it's legit or not), using `licensed_feature_available?(feature)` may be more appropriate.
+      SOURCE
+      expect_offense(<<~SOURCE)
+        foo = :feature
+        feature_available?(foo)
+        ^^^^^^^^^^^^^^^^^^^^^^^ `feature_available?` should not be called for features that can be licensed (`foo` isn't a literal so we cannot say if it's legit or not), using `licensed_feature_available?(feature)` may be more appropriate.
+      SOURCE
+    end
+  end
+
+  context 'two arguments given' do
+    it 'does not flag the use with an OSS project feature' do
+      expect_no_offenses('feature_available?(:issues, user)')
+      expect_no_offenses('project.feature_available?(:issues, user)')
+    end
+
+    it 'does not flag the use with an EE project feature' do
+      expect_no_offenses('feature_available?(:requirements, user)')
+      expect_no_offenses('project.feature_available?(:requirements, user)')
+    end
+
+    it 'flags the use with a dynamic feature as a method call with two args' do
+      expect_offense(<<~SOURCE)
+        feature_available?(:foo, current_user)
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `feature_available?` should not be called for features that can be licensed (`:foo` given), use `licensed_feature_available?(feature)` instead.
+      SOURCE
+      expect_offense(<<~SOURCE)
+        project.feature_available?(:foo, current_user)
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `feature_available?` should not be called for features that can be licensed (`:foo` given), use `licensed_feature_available?(feature)` instead.
+      SOURCE
+      expect_offense(<<~SOURCE)
+        feature_available?(foo, current_user)
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `feature_available?` should not be called for features that can be licensed (`foo` isn't a literal so we cannot say if it's legit or not), using `licensed_feature_available?(feature)` may be more appropriate.
+      SOURCE
+      expect_offense(<<~SOURCE)
+        project.feature_available?(foo, current_user)
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `feature_available?` should not be called for features that can be licensed (`foo` isn't a literal so we cannot say if it's legit or not), using `licensed_feature_available?(feature)` may be more appropriate.
+      SOURCE
+    end
+  end
+end