diff --git a/ee/app/validators/feature_flag_strategies_validator.rb b/ee/app/validators/feature_flag_strategies_validator.rb index e542d52c50a8054e1cc4d6740c41c7e3d99ecd00..527e494282c31dc92415c99820289c0bba4c3bea 100644 --- a/ee/app/validators/feature_flag_strategies_validator.rb +++ b/ee/app/validators/feature_flag_strategies_validator.rb @@ -11,26 +11,40 @@ class FeatureFlagStrategiesValidator < ActiveModel::EachValidator STRATEGY_USERWITHID => ['userIds'].freeze }.freeze USERID_MAX_LENGTH = 256 + STRATEGY_NAME_MAX_LENGTH = 100 + MAX_STRATEGY_COUNT = 50 + MAX_PARAMETER_KEYS_COUNT = 30 + MAX_PARAMETER_KEY_LENGTH = 100 + MAX_PARAMETER_VALUE_LENGTH = 1000 def validate_each(record, attribute, value) return unless value - if value.is_a?(Array) && value.all? { |s| s.is_a?(Hash) } - value.each do |strategy| - strategy_validations(record, attribute, strategy) - end - else - error(record, attribute, 'must be an array of strategy hashes') + unless value.is_a?(Array) && value.all? { |s| s.is_a?(Hash) } + return error(record, attribute, 'must be an array of strategy hashes') + end + + unless value.length <= MAX_STRATEGY_COUNT + return error(record, attribute, "cannot contain more than #{MAX_STRATEGY_COUNT} strategies") + end + + value.each do |strategy| + strategy_validations(record, attribute, strategy) 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) + if Feature.enabled?(:feature_flag_api, record.feature_flag&.project) + validate_name_length(record, attribute, strategy) && + validate_parameters_format(record, attribute, strategy) + else + validate_name(record, attribute, strategy) && + validate_parameters_type(record, attribute, strategy) && + validate_parameters_keys(record, attribute, strategy) && + validate_parameters_values(record, attribute, strategy) + end end def validate_name(record, attribute, strategy) @@ -41,6 +55,26 @@ def validate_parameters_type(record, attribute, strategy) strategy['parameters'].is_a?(Hash) || error(record, attribute, 'parameters are invalid') end + def validate_name_length(record, attribute, strategy) + strategy['name'].length < STRATEGY_NAME_MAX_LENGTH || error(record, attribute, 'strategy name is too long') + end + + def validate_parameters_format(record, attribute, strategy) + unless strategy['parameters'].is_a?(Hash) + return error(record, attribute, 'parameters is not hash') + end + + unless strategy['parameters'].keys.length < MAX_PARAMETER_KEYS_COUNT + return error(record, attribute, 'too many keys in a parameter') + end + + unless strategy['parameters'].all? { |key, value| key.length < MAX_PARAMETER_KEY_LENGTH && value.length < MAX_PARAMETER_VALUE_LENGTH } + return error(record, attribute, 'too long key/value in a parameter') + end + + true + end + def validate_parameters_keys(record, attribute, strategy) name, parameters = strategy.values_at('name', 'parameters') actual_keys = parameters.keys.sort diff --git a/ee/spec/controllers/projects/feature_flags_controller_spec.rb b/ee/spec/controllers/projects/feature_flags_controller_spec.rb index cdc3b06b5650e73290e0436e98e997cdb19c4a9f..636f13326bb64d23e52da14bc7ad929d5c24371a 100644 --- a/ee/spec/controllers/projects/feature_flags_controller_spec.rb +++ b/ee/spec/controllers/projects/feature_flags_controller_spec.rb @@ -920,14 +920,35 @@ def request_params(scope, strategies) }]) 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' } }]) + context 'when feature_flag_api feature flag is enabled' do + before do + stub_feature_flags(feature_flag_api: true) + end - put(:update, params: params, format: :json, as: :json) + it 'accepts 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(:ok) + end + end + + context 'when feature_flag_api feature flag is disabled' do + before do + stub_feature_flags(feature_flag_api: false) + end - expect(response).to have_gitlab_http_status(:bad_request) - expect(json_response['message']).to eq(["Scopes strategies parameters are invalid"]) + 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 end diff --git a/ee/spec/models/operations/feature_flag_scope_spec.rb b/ee/spec/models/operations/feature_flag_scope_spec.rb index ce81ffb3209c6541ee80f2bc804b7cbe34ad0d26..e5824cbe5927825ac111b1d6ded87cfe9611d626 100644 --- a/ee/spec/models/operations/feature_flag_scope_spec.rb +++ b/ee/spec/models/operations/feature_flag_scope_spec.rb @@ -8,6 +8,10 @@ end describe 'validations' do + before do + stub_feature_flags(feature_flag_api: false) + end + context 'when duplicate environment scope is going to be created' do let!(:existing_feature_flag_scope) do create(:operations_feature_flag_scope) @@ -60,15 +64,6 @@ expect(scope.errors[:strategies]).to be_empty end - it 'validates multiple strategies' do - feature_flag = create(:operations_feature_flag) - scope = described_class.create(feature_flag: feature_flag, - strategies: [{ name: "default", parameters: {} }, - { name: "invalid", parameters: {} }]) - - expect(scope.errors[:strategies]).not_to be_empty - end - where(:invalid_value) do [{}, 600, "bad", [{ name: 'default', parameters: {} }, 300]] end @@ -296,6 +291,53 @@ end end end + + context 'when feature_flag_api feature flag is enabled' do + before do + stub_feature_flags(feature_flag_api: true) + end + + it 'disallows too many strategies' do + strategies = Array.new(51) { { name: 'default', parameters: {} } } + scope = build(:operations_feature_flag_scope, strategies: strategies ) + + expect(scope).not_to be_valid + expect(scope.errors[:strategies]).to include('cannot contain more than 50 strategies') + end + + it 'disallows too long strategy name' do + strategies = [{ name: 'a' * 101, parameters: {} }] + scope = build(:operations_feature_flag_scope, strategies: strategies ) + + expect(scope).not_to be_valid + expect(scope.errors[:strategies]).to include('strategy name is too long') + end + + it 'disallows too many keys in parameters' do + parameters = 31.times.with_object({}) { |i, h| h[i.to_s] = i.to_s } + strategies = [{ name: 'userWithId', parameters: parameters }] + scope = build(:operations_feature_flag_scope, strategies: strategies ) + + expect(scope).not_to be_valid + expect(scope.errors[:strategies]).to include('too many keys in a parameter') + end + + it 'disallows too long key in parameters' do + strategies = [{ name: 'userWithId', parameters: { 'a' * 101 => 'ok' } }] + scope = build(:operations_feature_flag_scope, strategies: strategies ) + + expect(scope).not_to be_valid + expect(scope.errors[:strategies]).to include('too long key/value in a parameter') + end + + it 'disallows too long value in parameters' do + strategies = [{ name: 'userWithId', parameters: { 'ok' => 'a' * 1001 } }] + scope = build(:operations_feature_flag_scope, strategies: strategies ) + + expect(scope).not_to be_valid + expect(scope.errors[:strategies]).to include('too long key/value in a parameter') + end + end end describe '.enabled' do