Merge branch 'userid-ff' into 'master'

Support Feature Flag userWithId Strategy on Backend

See merge request gitlab-org/gitlab-ee!14752

ee/app/controllers/projects/feature_flags_controller.rb

@@ -101,14 +101,14 @@ class Projects::FeatureFlagsController < Projects::ApplicationController
     params.require(:operations_feature_flag)
       .permit(:name, :description, :active,
               scopes_attributes: [:environment_scope, :active,
-                                  strategies: [:name, parameters: [:groupId, :percentage]]])
+                                  strategies: [:name, parameters: [:groupId, :percentage, :userIds]]])
   end
 
   def update_params
     params.require(:operations_feature_flag)
           .permit(:name, :description, :active,
                   scopes_attributes: [:id, :environment_scope, :active, :_destroy,
-                                      strategies: [:name, parameters: [:groupId, :percentage]]])
+                                      strategies: [:name, parameters: [:groupId, :percentage, :userIds]]])
   end
 
   def feature_flag_json(feature_flag)

ee/app/validators/feature_flag_strategies_validator.rb

@@ -3,6 +3,14 @@
 class FeatureFlagStrategiesValidator < ActiveModel::EachValidator
   STRATEGY_DEFAULT = 'default'.freeze
   STRATEGY_GRADUALROLLOUTUSERID = 'gradualRolloutUserId'.freeze
+  STRATEGY_USERWITHID = 'userWithId'.freeze
+  # Order key names alphabetically
+  STRATEGIES = {
+    STRATEGY_DEFAULT => [].freeze,
+    STRATEGY_GRADUALROLLOUTUSERID => %w[groupId percentage].freeze,
+    STRATEGY_USERWITHID => ['userIds'].freeze
+  }.freeze
+  USERID_MAX_LENGTH = 256
 
   def validate_each(record, attribute, value)
     return unless value
@@ -12,20 +20,40 @@ class FeatureFlagStrategiesValidator < ActiveModel::EachValidator
         strategy_validations(record, attribute, strategy)
       end
     else
-      record.errors.add(attribute, 'must be an array of strategy hashes')
+      error(record, attribute, 'must be an array of strategy hashes')
     end
   end
 
   private
 
   def strategy_validations(record, attribute, strategy)
+    validate_name(record, attribute, strategy) &&
+    validate_parameters_type(record, attribute, strategy) &&
+    validate_parameters_keys(record, attribute, strategy) &&
+    validate_parameters_values(record, attribute, strategy)
+  end
+
+  def validate_name(record, attribute, strategy)
+    STRATEGIES.key?(strategy['name']) || error(record, attribute, 'strategy name is invalid')
+  end
+
+  def validate_parameters_type(record, attribute, strategy)
+    strategy['parameters'].is_a?(Hash) || error(record, attribute, 'parameters are invalid')
+  end
+
+  def validate_parameters_keys(record, attribute, strategy)
+    name, parameters = strategy.values_at('name', 'parameters')
+    actual_keys = parameters.keys.sort
+    expected_keys = STRATEGIES[name]
+    expected_keys == actual_keys || error(record, attribute, 'parameters are invalid')
+  end
+
+  def validate_parameters_values(record, attribute, strategy)
     case strategy['name']
-    when STRATEGY_DEFAULT
-      default_parameters_validation(record, attribute, strategy)
     when STRATEGY_GRADUALROLLOUTUSERID
       gradual_rollout_user_id_parameters_validation(record, attribute, strategy)
-    else
-      record.errors.add(attribute, 'strategy name is invalid')
+    when STRATEGY_USERWITHID
+      user_with_id_parameters_validation(record, attribute, strategy)
     end
   end
 
@@ -34,17 +62,34 @@ class FeatureFlagStrategiesValidator < ActiveModel::EachValidator
     group_id = strategy.dig('parameters', 'groupId')
 
     unless percentage.is_a?(String) && percentage.match(/\A[1-9]?[0-9]\z|\A100\z/)
-      record.errors.add(attribute, 'percentage must be a string between 0 and 100 inclusive')
+      error(record, attribute, 'percentage must be a string between 0 and 100 inclusive')
     end
 
     unless group_id.is_a?(String) && group_id.match(/\A[a-z]{1,32}\z/)
-      record.errors.add(attribute, 'groupId parameter is invalid')
+      error(record, attribute, 'groupId parameter is invalid')
     end
   end
 
-  def default_parameters_validation(record, attribute, strategy)
-    unless strategy['parameters'] == {}
-      record.errors.add(attribute, 'parameters must be empty for default strategy')
+  def user_with_id_parameters_validation(record, attribute, strategy)
+    user_ids = strategy.dig('parameters', 'userIds')
+    unless user_ids.is_a?(String) && !user_ids.match(/[\n\r\t]|,,/) && valid_ids?(user_ids.split(","))
+      error(record, attribute, "userIds must be a string of unique comma separated values each #{USERID_MAX_LENGTH} characters or less")
     end
   end
+
+  def valid_ids?(user_ids)
+    user_ids.uniq.length == user_ids.length &&
+    user_ids.all? { |id| valid_id?(id) }
+  end
+
+  def valid_id?(user_id)
+    user_id.present? &&
+    user_id.strip == user_id &&
+    user_id.length <= USERID_MAX_LENGTH
+  end
+
+  def error(record, attribute, msg)
+    record.errors.add(attribute, msg)
+    false
+  end
 end

ee/changelogs/unreleased/userid-ff.yml

+---
+title: Support feature flag userWithId strategy on backend
+merge_request: 14752
+author:
+type: added

ee/spec/controllers/projects/feature_flags_controller_spec.rb

@@ -440,6 +440,30 @@ describe Projects::FeatureFlagsController do
       end
     end
 
+    context 'when creates additional scope with a userWithId strategy' do
+      it 'creates a strategy for the scope' do
+        params = view_params.merge({
+          operations_feature_flag: {
+            name: 'my_feature_flag',
+            active: true,
+            scopes_attributes: [{ environment_scope: '*', active: true },
+                                { environment_scope: 'production', active: false,
+                                  strategies: [{ name: 'userWithId',
+                                                 parameters: { userIds: '123,4,6722' } }] }]
+          }
+        })
+
+        post(:create, params: params, format: :json)
+
+        expect(response).to have_gitlab_http_status(:ok)
+        production_strategies_json = json_response['scopes'].second['strategies']
+        expect(production_strategies_json).to eq([{
+          'name' => 'userWithId',
+          'parameters' => { "userIds" => "123,4,6722" }
+        }])
+      end
+    end
+
     context 'when creates an additional scope without a strategy' do
       it 'creates a default strategy' do
         params = view_params.merge({
@@ -774,6 +798,22 @@ describe Projects::FeatureFlagsController do
         }])
       end
 
+      it 'creates a userWithId strategy' do
+        scope = create_scope(feature_flag, 'production', true, [{ name: 'default', parameters: {} }])
+        params = request_params(scope, [{ name: 'userWithId', parameters: { userIds: 'sam,fred' } }])
+
+        put(:update, params: params, format: :json, as: :json)
+
+        expect(response).to have_gitlab_http_status(:ok)
+        scope_json = json_response['scopes'].select do |s|
+          s['environment_scope'] == 'production'
+        end.first
+        expect(scope_json['strategies']).to eq([{
+          "name" => "userWithId",
+          "parameters" => { "userIds" => "sam,fred" }
+        }])
+      end
+
       it 'updates an existing strategy' do
         scope = create_scope(feature_flag, 'production', true, [{ name: 'default', parameters: {} }])
         params = request_params(scope, [{ name: 'gradualRolloutUserId',
@@ -807,6 +847,30 @@ describe Projects::FeatureFlagsController do
         expect(scope_json['strategies']).to eq([])
       end
 
+      it 'accepts multiple strategies' do
+        scope = create_scope(feature_flag, 'production', true, [{ name: 'default', parameters: {} }])
+        params = request_params(scope, [
+          { name: 'gradualRolloutUserId', parameters: { groupId: 'mygroup', percentage: '55' } },
+          { name: 'userWithId', parameters: { userIds: 'joe' } }
+        ])
+
+        put(:update, params: params, format: :json, as: :json)
+
+        expect(response).to have_gitlab_http_status(:ok)
+        scope_json = json_response['scopes'].select do |s|
+          s['environment_scope'] == 'production'
+        end.first
+        expect(scope_json['strategies'].length).to eq(2)
+        expect(scope_json['strategies']).to include({
+          "name" => "gradualRolloutUserId",
+          "parameters" => { "groupId" => "mygroup", "percentage" => "55" }
+        })
+        expect(scope_json['strategies']).to include({
+          "name" => "userWithId",
+          "parameters" => { "userIds" => "joe" }
+        })
+      end
+
       it 'does not modify strategies when there is no strategies key in the params' do
         scope = create_scope(feature_flag, 'production', true, [{ name: 'default', parameters: {} }])
         params = {
@@ -853,6 +917,16 @@ describe Projects::FeatureFlagsController do
           "parameters" => { "groupId" => "default", "percentage" => "10" }
         }])
       end
+
+      it 'does not accept extra parameters in the strategy params' do
+        scope = create_scope(feature_flag, 'production', true, [{ name: 'default', parameters: {} }])
+        params = request_params(scope, [{ name: 'userWithId', parameters: { userIds: 'joe', groupId: 'default' } }])
+
+        put(:update, params: params, format: :json, as: :json)
+
+        expect(response).to have_gitlab_http_status(:bad_request)
+        expect(json_response['message']).to eq(["Scopes strategies parameters are invalid"])
+      end
     end
   end
 

ee/spec/models/operations/feature_flag_scope_spec.rb

@@ -87,8 +87,9 @@ describe Operations::FeatureFlagScope do
         using RSpec::Parameterized::TableSyntax
 
         where(:name, :params, :expected) do
-          'default' | {} | []
+          'default'              | {}                                       | []
           'gradualRolloutUserId' | { groupId: 'mygroup', percentage: '50' } | []
+          'userWithId'           | { userIds: 'sam' }                       | []
           5                      | nil                                      | ['strategy name is invalid']
           nil                    | nil                                      | ['strategy name is invalid']
           "nothing"              | nil                                      | ['strategy name is invalid']
@@ -98,7 +99,7 @@ describe Operations::FeatureFlagScope do
           []                     | nil                                      | ['strategy name is invalid']
         end
         with_them do
-          it 'must be one of "default" or "gradualRolloutUserId"' do
+          it 'must be one of "default", "gradualRolloutUserId", or "userWithId"' do
             feature_flag = create(:operations_feature_flag)
             scope = described_class.create(feature_flag: feature_flag, strategies: [{ name: name, parameters: params }])
 
@@ -109,6 +110,38 @@ describe Operations::FeatureFlagScope do
 
       describe 'parameters' do
         context 'when the strategy name is gradualRolloutUserId' do
+          it 'must have parameters' do
+            feature_flag = create(:operations_feature_flag)
+            scope = described_class.create(feature_flag: feature_flag,
+                                           strategies: [{ name: 'gradualRolloutUserId' }])
+
+            expect(scope.errors[:strategies]).to eq(['parameters are invalid'])
+          end
+
+          where(:invalid_parameters) do
+            [nil, {}, { percentage: '40', groupId: 'mygroup', userIds: '4' }, { percentage: '40' },
+             { percentage: '40', groupId: 'mygroup', extra: nil }, { groupId: 'mygroup' }]
+          end
+          with_them do
+            it 'must have valid parameters for the strategy' do
+              feature_flag = create(:operations_feature_flag)
+              scope = described_class.create(feature_flag: feature_flag,
+                                             strategies: [{ name: 'gradualRolloutUserId',
+                                                            parameters: invalid_parameters }])
+
+              expect(scope.errors[:strategies]).to eq(['parameters are invalid'])
+            end
+          end
+
+          it 'allows the parameters in any order' do
+            feature_flag = create(:operations_feature_flag)
+            scope = described_class.create(feature_flag: feature_flag,
+                                           strategies: [{ name: 'gradualRolloutUserId',
+                                                          parameters: { percentage: '10', groupId: 'mygroup' } }])
+
+            expect(scope.errors[:strategies]).to be_empty
+          end
+
           describe 'percentage' do
             where(:invalid_value) do
               [50, 40.0, { key: "value" }, "garbage", "00", "01", "101", "-1", "-10", "0100",
@@ -163,25 +196,83 @@ describe Operations::FeatureFlagScope do
             with_them do
               it 'must be a string value of up to 32 lowercase characters' do
                 feature_flag = create(:operations_feature_flag)
-                scope = described_class.create(feature_flag: feature_flag, strategies: [{ name: 'gradualRolloutUserId',
-                                                                                          parameters: { groupId: valid_value, percentage: '40' } }])
+                scope = described_class.create(feature_flag: feature_flag,
+                                               strategies: [{ name: 'gradualRolloutUserId',
+                                                              parameters: { groupId: valid_value, percentage: '40' } }])
 
                 expect(scope.errors[:strategies]).to eq([])
               end
             end
           end
+        end
 
+        context 'when the strategy name is userWithId' do
           it 'must have parameters' do
             feature_flag = create(:operations_feature_flag)
-            scope = described_class.create(feature_flag: feature_flag, strategies: [{ name: 'gradualRolloutUserId' }])
-            expect(scope.errors[:strategies]).to include('groupId parameter is invalid')
-            expect(scope.errors[:strategies]).to include('percentage must be a string between 0 and 100 inclusive')
+            scope = described_class.create(feature_flag: feature_flag, strategies: [{ name: 'userWithId' }])
+
+            expect(scope.errors[:strategies]).to eq(['parameters are invalid'])
+          end
+
+          where(:invalid_parameters) do
+            [nil, { userIds: 'sam', percentage: '40' }, { userIds: 'sam', some: 'param' }, { percentage: '40' }, {}]
+          end
+          with_them do
+            it 'must have valid parameters for the strategy' do
+              feature_flag = create(:operations_feature_flag)
+              scope = described_class.create(feature_flag: feature_flag, strategies: [{ name: 'userWithId',
+                                                                                        parameters: invalid_parameters }])
+
+              expect(scope.errors[:strategies]).to eq(['parameters are invalid'])
+            end
+          end
+
+          describe 'userIds' do
+            where(:valid_value) do
+              ["", "sam", "1", "a", "uuid-of-some-kind", "sam,fred,tom,jane,joe,mike",
+               "gitlab@example.com", "123,4", "UPPER,Case,charActeRS", "0",
+               "$valid$email#2345#$%..{}+=-)?\\/@example.com", "spaces allowed",
+               "a" * 256, "a,#{'b' * 256},ccc", "many    spaces"]
+            end
+            with_them do
+              it 'is valid with a string of comma separated values' do
+                feature_flag = create(:operations_feature_flag)
+                scope = described_class.create(feature_flag: feature_flag,
+                                               strategies: [{ name: 'userWithId', parameters: { userIds: valid_value } }])
+
+                expect(scope.errors[:strategies]).to be_empty
+              end
+            end
+
+            where(:invalid_value) do
+              [1, 2.5, {}, [], nil, "123\n456", "1,2,3,12\t3", "\n", "\n\r",
+               "joe\r,sam", "1,2,2", "1,,2", "1,2,,,,", "b" * 257, "1, ,2", "tim,    ,7", " ",
+               "    ", " ,1", "1,  ", " leading,1", "1,trailing  ", "1, both ,2"]
+            end
+            with_them do
+              it 'is invalid' do
+                feature_flag = create(:operations_feature_flag)
+                scope = described_class.create(feature_flag: feature_flag,
+                                               strategies: [{ name: 'userWithId', parameters: { userIds: invalid_value } }])
+
+                expect(scope.errors[:strategies]).to include(
+                  'userIds must be a string of unique comma separated values each 256 characters or less'
+                )
+              end
+            end
           end
         end
 
         context 'when the strategy name is default' do
+          it 'must have parameters' do
+            feature_flag = create(:operations_feature_flag)
+            scope = described_class.create(feature_flag: feature_flag, strategies: [{ name: 'default' }])
+
+            expect(scope.errors[:strategies]).to eq(['parameters are invalid'])
+          end
+
           where(:invalid_value) do
-            [{ groupId: "hi", percentage: "7" }, "", "nothing", 7, nil]
+            [{ groupId: "hi", percentage: "7" }, "", "nothing", 7, nil, [], 2.5]
           end
           with_them do
             it 'must be empty' do
@@ -190,7 +281,7 @@ describe Operations::FeatureFlagScope do
                                              strategies: [{ name: 'default',
                                                             parameters: invalid_value }])
 
-              expect(scope.errors[:strategies]).to eq(['parameters must be empty for default strategy'])
+              expect(scope.errors[:strategies]).to eq(['parameters are invalid'])
             end
           end
 
@@ -200,7 +291,7 @@ describe Operations::FeatureFlagScope do
                                            strategies: [{ name: 'default',
                                                           parameters: {} }])
 
-            expect(scope.errors[:strategies]).to eq([])
+            expect(scope.errors[:strategies]).to be_empty
           end
         end
       end

ee/spec/requests/api/unleash_spec.rb

@@ -213,6 +213,36 @@ describe API::Unleash do
         strategies = json_response['features'].first['strategies']
         expect(strategies).to eq([{ "name" => "default", "parameters" => {} }])
       end
+
+      it 'returns multiple strategies for a feature flag' do
+        client = create(:operations_feature_flags_client, project: project)
+        feature_flag = create(:operations_feature_flag, project: project, name: 'feature1', active: true)
+        create(:operations_feature_flag_scope,
+               feature_flag: feature_flag,
+               environment_scope: 'staging',
+               active: true,
+               strategies: [{ name: "userWithId", parameters: { userIds: "max,fred" } },
+                            { name: "gradualRolloutUserId",
+                              parameters: { groupId: "default", percentage: "50" } }])
+        headers = { "UNLEASH-INSTANCEID" => client.token, "UNLEASH-APPNAME" => "staging" }
+
+        get api(features_url), headers: headers
+
+        expect(response).to have_gitlab_http_status(:ok)
+        strategies = json_response['features'].first['strategies'].sort_by { |s| s['name'] }
+        expect(strategies).to eq([{
+          "name" => "gradualRolloutUserId",
+          "parameters" => {
+            "percentage" => "50",
+            "groupId" => "default"
+          }
+        }, {
+          "name" => "userWithId",
+          "parameters" => {
+            "userIds" => "max,fred"
+          }
+        }])
+      end
     end
   end