Merge branch 'disallow-usage-of-licensed-feature-flags' into 'master'

Disallow feature flags to use licensed names

See merge request gitlab-org/gitlab!45904

bin/feature-flag

@@ -126,6 +126,8 @@ class FeatureFlagOptionParser
       $stdout.puts ">> Specify the feature flag type:"
       $stdout.puts
       TYPES.each do |type, data|
+        next if data[:deprecated]
+
         $stdout.puts "#{type.to_s.rjust(15)}#{' '*6}#{data[:description]}"
       end
 
@@ -133,7 +135,7 @@ class FeatureFlagOptionParser
         $stdout.print "?> "
 
         type = $stdin.gets.strip.to_sym
-        return type if TYPES[type]
+        return type if TYPES[type] && !TYPES[type][:deprecated]
 
         $stderr.puts "Invalid type specified '#{type}'"
       end

config/initializers/0_inject_feature_flags.rb

@@ -4,3 +4,49 @@
 Feature.register_feature_groups
 Feature.register_definitions
 Feature.register_hot_reloader unless Rails.configuration.cache_classes
+
+# This disallows usage of licensed feature names with the same name
+# as feature flags. This naming collision creates confusion and it was
+# decided to be removed in favor of explicit check.
+# https://gitlab.com/gitlab-org/gitlab/-/issues/259611
+if Gitlab.ee? && Gitlab.dev_or_test_env?
+  # These are the names of feature flags that do violate the constraint of
+  # being unique to licensed names. These feature flags should be reworked to
+  # be "development" with explicit check
+  IGNORED_FEATURE_FLAGS = %i[
+    resource_access_token
+    ci_secrets_management
+    feature_flags_related_issues
+    group_coverage_reports
+    group_wikis
+    incident_sla
+    swimlanes
+    minimal_access_role
+  ].to_set
+
+  # First, we validate a list of overrides to ensure that these overrides
+  # are removed if feature flag is gone
+  missing_feature_flags = IGNORED_FEATURE_FLAGS.reject do |feature_flag|
+    Feature::Definition.definitions[feature_flag]
+  end
+
+  if missing_feature_flags.any?
+    raise "The following feature flags were added as an override for discovering licensed features. " \
+          "Since these feature flags seems to be gone, ensure to remove them from \`IGNORED_FEATURE_FLAGS\` " \
+          "in \`#{__FILE__}'`: #{missing_feature_flags.join(", ")}"
+  end
+
+  # Second, we validate that there's no feature flag under the name as licensed feature
+  # flag, to ensure that the name used, is unique
+  licensed_features = License::PLANS_BY_FEATURE.keys.select do |licensed_feature_name|
+    IGNORED_FEATURE_FLAGS.exclude?(licensed_feature_name) &&
+      Feature::Definition.definitions[licensed_feature_name]
+  end
+
+  if licensed_features.any?
+    raise "The following feature flags do use a licensed feature. " \
+          "To avoid the confusion between their usage it is disallowed to use feature flag " \
+          "with exact the same name as licensed feature name. Use a different name to create " \
+          "a distinction: #{licensed_features.join(", ")}"
+  end
+end

doc/development/feature_flags/development.md

@@ -61,30 +61,6 @@ Feature.disabled?(:my_ops_flag, project, type: :ops)
 push_frontend_feature_flag(:my_ops_flag, project, type: :ops)
 ```
 
-### `licensed` type
-
-`licensed` feature flags are used to temporarily disable licensed features. There
-should be a one-to-one mapping of `licensed` feature flags to licensed features.
-
-`licensed` feature flags must be `default_enabled: true`, because that's the only
-supported option in the current implementation. This is under development as per
-the [related issue](https://gitlab.com/gitlab-org/gitlab/-/issues/218667).
-
-The `licensed` type has a dedicated set of functions to check if a licensed
-feature is available for a project or namespace. This check validates
-if the license is assigned to the namespace and feature flag itself.
-The `licensed` feature flag has the same name as a licensed feature name:
-
-```ruby
-# Good: checks if feature flag is enabled
-project.feature_available?(:my_licensed_feature)
-namespace.feature_available?(:my_licensed_feature)
-
-# Bad: licensed flag must be accessed via `feature_available?`
-Feature.enabled?(:my_licensed_feature, type: :licensed)
-push_frontend_feature_flag(:my_licensed_feature, type: :licensed)
-```
-
 ## Feature flag definition and validation
 
 > [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/229161) in GitLab 13.3.
@@ -309,25 +285,16 @@ used as an actor for `Feature.enabled?`.
 
 ### Feature flags for licensed features
 
-The [`Project#feature_available?`](https://gitlab.com/gitlab-org/gitlab/blob/4cc1c62918aa4c31750cb21dfb1a6c3492d71080/app/models/project_feature.rb#L63-68),
-[`Namespace#feature_available?`](https://gitlab.com/gitlab-org/gitlab/blob/4cc1c62918aa4c31750cb21dfb1a6c3492d71080/ee/app/models/ee/namespace.rb#L71-85) (EE), and
-[`License.feature_available?`](https://gitlab.com/gitlab-org/gitlab/blob/4cc1c62918aa4c31750cb21dfb1a6c3492d71080/ee/app/models/license.rb#L293-300) (EE) methods all implicitly check for
-a by default enabled feature flag with the same name as the provided argument.
-
-**An important side-effect of the implicit feature flags mentioned above is that
-unless the feature is explicitly disabled or limited to a percentage of users,
-the feature flag check defaults to `true`.**
-
-NOTE: **Note:**
-Due to limitations with `feature_available?`, the YAML definition for `licensed` feature
-flags accepts only `default_enabled: true`. This is under development as per the
-[related issue](https://gitlab.com/gitlab-org/gitlab/-/issues/218667).
+You can't use a feature flag with the same name as a licensed feature name, because
+it would cause a naming collision. This was [widely discussed and removed](https://gitlab.com/gitlab-org/gitlab/-/issues/259611)
+because it is confusing.
 
-If you want a licensed feature to be disabled by default or enabled only for a given gate, you can use a feature flag with a different name. The feature checks would then
-look like:
+To check for licensed features, add a dedicated feature flag under a different name
+and check it explicitly, for example:
 
 ```ruby
-Feature.enabled?(:licensed_feature_feature_flag, project) && project.feature_available?(:licensed_feature)
+Feature.enabled?(:licensed_feature_feature_flag, project) &&
+  project.feature_available?(:licensed_feature)
 ```
 
 ### Feature groups

ee/lib/ee/api/features.rb

+# frozen_string_literal: true
+
+module EE
+  module API
+    module Features
+      extend ActiveSupport::Concern
+
+      prepended do
+        helpers do
+          extend ::Gitlab::Utils::Override
+
+          override :validate_licensed_name!
+          def validate_feature_flag_name!(name)
+            super
+
+            if License::PLANS_BY_FEATURE[name.to_sym]
+              bad_request!(
+                "The '#{name}' is a licensed feature name, " \
+                "and thus it cannot be used as a feature flag name. " \
+                "Use `rails console` to set this feature flag state."
+              )
+            end
+          end
+        end
+      end
+    end
+  end
+end

ee/spec/requests/api/features_spec.rb

@@ -32,6 +32,18 @@ RSpec.describe API::Features, stub_feature_flags: false do
         end.to change(Geo::CacheInvalidationEvent, :count).by(1)
       end
     end
+
+    context 'when licensed feature name is given' do
+      let(:feature_name) do
+        License::PLANS_BY_FEATURE.each_key.first
+      end
+
+      it 'returns bad request' do
+        post api("/features/#{feature_name}", admin), params: { value: 'true' }
+
+        expect(response).to have_gitlab_http_status(:bad_request)
+      end
+    end
   end
 
   describe 'DELETE /feature/:name' do

lib/api/features.rb

@@ -61,6 +61,8 @@ module API
         mutually_exclusive :key, :project
       end
       post ':name' do
+        validate_feature_flag_name!(params[:name])
+
         feature = Feature.get(params[:name]) # rubocop:disable Gitlab/AvoidFeatureGet
         targets = gate_targets(params)
         value = gate_value(params)
@@ -97,5 +99,13 @@ module API
         no_content!
       end
     end
+
+    helpers do
+      def validate_feature_flag_name!(name)
+        # no-op
+      end
+    end
   end
 end
+
+API::Features.prepend_if_ee('EE::API::Features')

lib/feature/shared.rb

@@ -10,6 +10,8 @@ class Feature
     # rollout_issue: defines if `bin/feature-flag` asks for rollout issue
     # default_enabled: defines a default state of a feature flag when created by `bin/feature-flag`
     # ee_only: defines that a feature flag can only be created in a context of EE
+    # deprecated: defines if a feature flag type that is deprecated and to be removed,
+    #             the deprecated types are hidden from all interfaces
     # example: usage being shown when exception is raised
     TYPES = {
       development: {
@@ -37,6 +39,7 @@ class Feature
       },
       licensed: {
         description: 'Permanent feature flags used to temporarily disable licensed features.',
+        deprecated: true,
         optional: true,
         rollout_issue: false,
         ee_only: true,

spec/bin/feature_flag_spec.rb

@@ -123,6 +123,29 @@ RSpec.describe 'bin/feature-flag' do
         end
       end
 
+      context 'when there is deprecated feature flag type' do
+        before do
+          stub_const('FeatureFlagOptionParser::TYPES',
+            development: { description: 'short' },
+            deprecated: { description: 'deprecated', deprecated: true }
+          )
+        end
+
+        context 'and deprecated type is given' do
+          let(:type) { 'deprecated' }
+
+          it 'shows error message and retries' do
+            expect($stdin).to receive(:gets).and_return(type)
+            expect($stdin).to receive(:gets).and_raise('EOF')
+
+            expect do
+              expect { described_class.read_type }.to raise_error(/EOF/)
+            end.to output(/Specify the feature flag type/).to_stdout
+              .and output(/Invalid type specified/).to_stderr
+          end
+        end
+      end
+
       context 'when there are many types defined' do
         before do
           stub_const('FeatureFlagOptionParser::TYPES',