From e9258560fde096376ec4340825cb897951395c20 Mon Sep 17 00:00:00 2001 From: Rashid Siddique Date: Fri, 28 Sep 2018 21:00:47 +0500 Subject: [PATCH 1/3] refact(API): Adds missing input validations in all API methods and validates empty user Id. --- lib/optimizely.rb | 51 +++++++--- lib/optimizely/project_config.rb | 14 ++- spec/project_config_spec.rb | 36 ++++--- spec/project_spec.rb | 161 +++++++++++++++++++++---------- 4 files changed, 181 insertions(+), 81 deletions(-) diff --git a/lib/optimizely.rb b/lib/optimizely.rb index 0b095fcf..fe8997a9 100644 --- a/lib/optimizely.rb +++ b/lib/optimizely.rb @@ -96,11 +96,15 @@ def activate(experiment_key, user_id, attributes = nil) return nil unless Optimizely::Helpers::Validator.inputs_valid?( { - experiment_key: experiment_key, - user_id: user_id + experiment_key: experiment_key }, @logger, Logger::ERROR ) + unless user_id.is_a?(String) + @logger.log(Logger::ERROR, "#{Optimizely::Helpers::Constants::INPUT_VARIABLES['USER_ID']} is invalid") + return nil + end + variation_key = get_variation(experiment_key, user_id, attributes) if variation_key.nil? @@ -132,11 +136,15 @@ def get_variation(experiment_key, user_id, attributes = nil) return nil unless Optimizely::Helpers::Validator.inputs_valid?( { - experiment_key: experiment_key, - user_id: user_id + experiment_key: experiment_key }, @logger, Logger::ERROR ) + unless user_id.is_a?(String) + @logger.log(Logger::ERROR, "#{Optimizely::Helpers::Constants::INPUT_VARIABLES['USER_ID']} is invalid") + return nil + end + unless user_inputs_valid?(attributes) @logger.log(Logger::INFO, "Not activating user '#{user_id}.") return nil @@ -194,11 +202,15 @@ def track(event_key, user_id, attributes = nil, event_tags = nil) return nil unless Optimizely::Helpers::Validator.inputs_valid?( { - event_key: event_key, - user_id: user_id + event_key: event_key }, @logger, Logger::ERROR ) + unless user_id.is_a?(String) + @logger.log(Logger::ERROR, "#{Optimizely::Helpers::Constants::INPUT_VARIABLES['USER_ID']} is invalid") + return nil + end + return nil unless user_inputs_valid?(attributes, event_tags) experiment_ids = @config.get_experiment_ids_for_event(event_key) @@ -253,11 +265,17 @@ def is_feature_enabled(feature_flag_key, user_id, attributes = nil) return false unless Optimizely::Helpers::Validator.inputs_valid?( { - feature_flag_key: feature_flag_key, - user_id: user_id + feature_flag_key: feature_flag_key }, @logger, Logger::ERROR ) + unless user_id.is_a?(String) + @logger.log(Logger::ERROR, "#{Optimizely::Helpers::Constants::INPUT_VARIABLES['USER_ID']} is invalid") + return false + end + + return false unless user_inputs_valid?(attributes) + feature_flag = @config.get_feature_flag_from_key(feature_flag_key) unless feature_flag @logger.log(Logger::ERROR, "No feature flag was found for key '#{feature_flag_key}'.") @@ -305,7 +323,12 @@ def get_enabled_features(user_id, attributes = nil) return enabled_features end - return enabled_features unless Optimizely::Helpers::Validator.inputs_valid?({user_id: user_id}, @logger, Logger::ERROR) + unless user_id.is_a?(String) + @logger.log(Logger::ERROR, "#{Optimizely::Helpers::Constants::INPUT_VARIABLES['USER_ID']} is invalid") + return enabled_features + end + + return enabled_features unless user_inputs_valid?(attributes) @config.feature_flags.each do |feature| enabled_features.push(feature['key']) if is_feature_enabled( @@ -444,12 +467,18 @@ def get_feature_variable_for_type(feature_flag_key, variable_key, variable_type, { feature_flag_key: feature_flag_key, variable_key: variable_key, - variable_type: variable_type, - user_id: user_id + variable_type: variable_type }, @logger, Logger::ERROR ) + unless user_id.is_a?(String) + @logger.log(Logger::ERROR, "#{Optimizely::Helpers::Constants::INPUT_VARIABLES['USER_ID']} is invalid") + return nil + end + + return nil unless user_inputs_valid?(attributes) + feature_flag = @config.get_feature_flag_from_key(feature_flag_key) unless feature_flag @logger.log(Logger::INFO, "No feature flag was found for key '#{feature_flag_key}'.") diff --git a/lib/optimizely/project_config.rb b/lib/optimizely/project_config.rb index f452c4e4..49547046 100644 --- a/lib/optimizely/project_config.rb +++ b/lib/optimizely/project_config.rb @@ -275,11 +275,15 @@ def get_forced_variation(experiment_key, user_id) return nil unless Optimizely::Helpers::Validator.inputs_valid?( { - experiment_key: experiment_key, - user_id: user_id + experiment_key: experiment_key }, @logger, Logger::DEBUG ) + unless user_id.is_a?(String) + @logger.log(Logger::DEBUG, "#{Optimizely::Helpers::Constants::INPUT_VARIABLES['USER_ID']} is invalid") + return nil + end + unless @forced_variation_map.key? user_id @logger.log(Logger::DEBUG, "User '#{user_id}' is not in the forced variation map.") return nil @@ -324,11 +328,15 @@ def set_forced_variation(experiment_key, user_id, variation_key) return false unless Optimizely::Helpers::Validator.inputs_valid?( { - user_id: user_id, experiment_key: experiment_key }, @logger, Logger::DEBUG ) + unless user_id.is_a?(String) + @logger.log(Logger::DEBUG, "#{Optimizely::Helpers::Constants::INPUT_VARIABLES['USER_ID']} is invalid") + return false + end + experiment = get_experiment_from_key(experiment_key) experiment_id = experiment['id'] if experiment # check if the experiment exists in the datafile diff --git a/spec/project_config_spec.rb b/spec/project_config_spec.rb index 03aa894e..1eb974ea 100644 --- a/spec/project_config_spec.rb +++ b/spec/project_config_spec.rb @@ -856,12 +856,6 @@ expect(spy_logger).to have_received(:log).with(Logger::DEBUG, 'User ID is invalid') end - # User ID is an empty string - it 'should log a message and return nil when user_id is passed as empty string for get_forced_variation' do - expect(config.get_forced_variation(@valid_experiment[:key], '')).to eq(nil) - expect(spy_logger).to have_received(:log).with(Logger::DEBUG, - 'User ID is invalid') - end # User ID is not defined in the forced variation map it 'should log a message and return nil when user is not in forced variation map' do expect(config.get_forced_variation(@valid_experiment[:key], @user_id)).to eq(nil) @@ -902,12 +896,6 @@ expect(spy_logger).to have_received(:log).with(Logger::DEBUG, 'User ID is invalid') end - # User ID is an empty string - it 'should log a message and return false when user_id is passed as empty string' do - expect(config.set_forced_variation(@valid_experiment[:key], '', @valid_variation[:key])).to eq(false) - expect(spy_logger).to have_received(:log).with(Logger::DEBUG, - 'User ID is invalid') - end # Experiment key is nil it 'should return false when experiment_key is passed as nil' do expect(config.set_forced_variation(nil, @user_id, @valid_variation[:key])).to eq(false) @@ -957,23 +945,41 @@ it 'should call inputs_valid? with the proper arguments in set_forced_variation' do expect(Optimizely::Helpers::Validator).to receive(:inputs_valid?).with( { - user_id: @user_id, experiment_key: @valid_experiment[:key] }, spy_logger, Logger::DEBUG ) config.set_forced_variation(@valid_experiment[:key], @user_id, @valid_variation[:key]) end + it 'should log and return false when user ID is non string in set_forced_variation' do + expect(config.set_forced_variation(@valid_experiment[:key], nil, @valid_variation[:key])).to be false + expect(config.set_forced_variation(@valid_experiment[:key], 5, @valid_variation[:key])).to be false + expect(config.set_forced_variation(@valid_experiment[:key], 5.5, @valid_variation[:key])).to be false + expect(config.set_forced_variation(@valid_experiment[:key], true, @valid_variation[:key])).to be false + expect(config.set_forced_variation(@valid_experiment[:key], {}, @valid_variation[:key])).to be false + expect(config.set_forced_variation(@valid_experiment[:key], [], @valid_variation[:key])).to be false + expect(spy_logger).to have_received(:log).with(Logger::DEBUG, 'User ID is invalid').exactly(6).times + end + it 'should call inputs_valid? with the proper arguments in get_forced_variation' do expect(Optimizely::Helpers::Validator).to receive(:inputs_valid?).with( { - experiment_key: @valid_experiment[:key], - user_id: @user_id + experiment_key: @valid_experiment[:key] }, spy_logger, Logger::DEBUG ) config.get_forced_variation(@valid_experiment[:key], @user_id) end + it 'should log and return nil when user ID is non string in get_forced_variation' do + expect(config.get_forced_variation(@valid_experiment[:key], nil)).to eq(nil) + expect(config.get_forced_variation(@valid_experiment[:key], 5)).to eq(nil) + expect(config.get_forced_variation(@valid_experiment[:key], 5.5)).to eq(nil) + expect(config.get_forced_variation(@valid_experiment[:key], true)).to eq(nil) + expect(config.get_forced_variation(@valid_experiment[:key], {})).to eq(nil) + expect(config.get_forced_variation(@valid_experiment[:key], [])).to eq(nil) + expect(spy_logger).to have_received(:log).with(Logger::DEBUG, 'User ID is invalid').exactly(6).times + end + # Call set variation with different variations on one user/experiment to confirm that each set is expected. it 'should set and return expected variations when different variations are set and removed for one user/experiment' do expect(config.set_forced_variation(@valid_experiment[:key], @user_id, @valid_variation[:key])).to eq(true) diff --git a/spec/project_spec.rb b/spec/project_spec.rb index aa206510..f6300088 100644 --- a/spec/project_spec.rb +++ b/spec/project_spec.rb @@ -271,13 +271,22 @@ class InvalidErrorHandler; end it 'should call inputs_valid? with the proper arguments in activate' do expect(Optimizely::Helpers::Validator).to receive(:inputs_valid?).with( { - experiment_key: 'test_experiment_with_audience', - user_id: 'test_user' + experiment_key: 'test_experiment_with_audience' }, spy_logger, Logger::ERROR ) project_instance.activate('test_experiment_with_audience', 'test_user') end + it 'should log return nil when user ID is non string' do + expect(project_instance.activate('test_experiment_with_audience', nil)).to eq(nil) + expect(project_instance.activate('test_experiment_with_audience', 5)).to eq(nil) + expect(project_instance.activate('test_experiment_with_audience', 5.5)).to eq(nil) + expect(project_instance.activate('test_experiment_with_audience', true)).to eq(nil) + expect(project_instance.activate('test_experiment_with_audience', {})).to eq(nil) + expect(project_instance.activate('test_experiment_with_audience', [])).to eq(nil) + expect(spy_logger).to have_received(:log).with(Logger::ERROR, 'User ID is invalid').exactly(6).times + end + it 'should return false when invalid inputs are passed' do expect(Optimizely::Helpers::Validator.inputs_valid?({})).to eq(false) expect(Optimizely::Helpers::Validator.inputs_valid?([])).to eq(false) @@ -290,38 +299,37 @@ class InvalidErrorHandler; end end it 'should log and return false when non string value inputs are passed' do - expect(Optimizely::Helpers::Validator.inputs_valid?({user_id: nil}, spy_logger, Logger::ERROR)).to eq(false) - expect(Optimizely::Helpers::Validator.inputs_valid?({user_id: ''}, spy_logger, Logger::ERROR)).to eq(false) - expect(Optimizely::Helpers::Validator.inputs_valid?({user_id: []}, spy_logger, Logger::ERROR)).to eq(false) - expect(Optimizely::Helpers::Validator.inputs_valid?({user_id: {}}, spy_logger, Logger::ERROR)).to eq(false) - expect(Optimizely::Helpers::Validator.inputs_valid?({user_id: 2}, spy_logger, Logger::ERROR)).to eq(false) - expect(Optimizely::Helpers::Validator.inputs_valid?({user_id: 2.0}, spy_logger, Logger::ERROR)).to eq(false) - expect(Optimizely::Helpers::Validator.inputs_valid?({user_id: true}, spy_logger, Logger::ERROR)).to eq(false) - expect(Optimizely::Helpers::Validator.inputs_valid?({user_id: false}, spy_logger, Logger::ERROR)).to eq(false) - expect(spy_logger).to have_received(:log).with(Logger::ERROR, 'User ID is invalid').exactly(8).times + expect(Optimizely::Helpers::Validator.inputs_valid?({experiment_key: nil}, spy_logger, Logger::ERROR)).to eq(false) + expect(Optimizely::Helpers::Validator.inputs_valid?({experiment_key: []}, spy_logger, Logger::ERROR)).to eq(false) + expect(Optimizely::Helpers::Validator.inputs_valid?({experiment_key: {}}, spy_logger, Logger::ERROR)).to eq(false) + expect(Optimizely::Helpers::Validator.inputs_valid?({experiment_key: 2}, spy_logger, Logger::ERROR)).to eq(false) + expect(Optimizely::Helpers::Validator.inputs_valid?({experiment_key: 2.0}, spy_logger, Logger::ERROR)).to eq(false) + expect(Optimizely::Helpers::Validator.inputs_valid?({experiment_key: true}, spy_logger, Logger::ERROR)).to eq(false) + expect(Optimizely::Helpers::Validator.inputs_valid?({experiment_key: false}, spy_logger, Logger::ERROR)).to eq(false) + expect(spy_logger).to have_received(:log).with(Logger::ERROR, 'Experiment key is invalid').exactly(7).times end it 'should log and return false when multiple non string value inputs are passed' do - expect(Optimizely::Helpers::Validator.inputs_valid?({user_id: '', experiment_key: true}, spy_logger, Logger::ERROR)).to eq(false) - expect(Optimizely::Helpers::Validator.inputs_valid?({user_id: [], variation_key: 2.0}, spy_logger, Logger::ERROR)).to eq(false) - expect(spy_logger).to have_received(:log).twice.with(Logger::ERROR, 'User ID is invalid') + expect(Optimizely::Helpers::Validator.inputs_valid?({variable_key: nil, experiment_key: true}, spy_logger, Logger::ERROR)).to eq(false) + expect(Optimizely::Helpers::Validator.inputs_valid?({variable_key: [], variation_key: 2.0}, spy_logger, Logger::ERROR)).to eq(false) + expect(spy_logger).to have_received(:log).twice.with(Logger::ERROR, 'Variable key is invalid') expect(spy_logger).to have_received(:log).once.with(Logger::ERROR, 'Experiment key is invalid') expect(spy_logger).to have_received(:log).once.with(Logger::ERROR, 'Variation key is invalid') end it 'should return true when valid input values are passed' do - expect(Optimizely::Helpers::Validator.inputs_valid?({user_id: '2'}, spy_logger, Logger::ERROR)).to eq(true) + expect(Optimizely::Helpers::Validator.inputs_valid?({experiment_key: '2'}, spy_logger, Logger::ERROR)).to eq(true) expect(Optimizely::Helpers::Validator.inputs_valid?({ - user_id: 'test_user', + variable_key: 'test_variable', experiment_key: 'test_experiment', variation_key: 'test_variation' }, spy_logger, Logger::ERROR)).to eq(true) end it 'should not log when logger or level are nil' do - expect(Optimizely::Helpers::Validator.inputs_valid?({user_id: nil}, nil, Logger::ERROR)).to eq(false) - expect(Optimizely::Helpers::Validator.inputs_valid?({user_id: nil}, spy_logger, nil)).to eq(false) - expect(spy_logger).not_to have_received(:log).with(Logger::ERROR, 'User ID is invalid') + expect(Optimizely::Helpers::Validator.inputs_valid?({variable_key: nil}, nil, Logger::ERROR)).to eq(false) + expect(Optimizely::Helpers::Validator.inputs_valid?({variable_key: nil}, spy_logger, nil)).to eq(false) + expect(spy_logger).not_to have_received(:log).with(Logger::ERROR, 'Variable key is invalid') end it 'should return nil when user is in no variation' do @@ -438,22 +446,30 @@ class InvalidErrorHandler; end } end - it 'should return nil when user_id is empty or nil' do - expect(project_instance.track('test_event', '', nil, 'revenue' => 42)).to eq(nil) + it 'should return nil when user_id is nil' do expect(project_instance.track('test_event', nil, nil, 'revenue' => 42)).to eq(nil) - expect(spy_logger).to have_received(:log).twice.with(Logger::ERROR, 'User ID is invalid') + expect(spy_logger).to have_received(:log).once.with(Logger::ERROR, 'User ID is invalid') end it 'should call inputs_valid? with the proper arguments in track' do expect(Optimizely::Helpers::Validator).to receive(:inputs_valid?).with( { - user_id: 'test_user', event_key: 'test_event' }, spy_logger, Logger::ERROR ) project_instance.track('test_event', 'test_user') end + it 'should log and return nil when user ID is non string' do + expect(project_instance.track('test_event', nil)).to eq(nil) + expect(project_instance.track('test_event', 5)).to eq(nil) + expect(project_instance.track('test_event', 5.5)).to eq(nil) + expect(project_instance.track('test_event', true)).to eq(nil) + expect(project_instance.track('test_event', {})).to eq(nil) + expect(project_instance.track('test_event', [])).to eq(nil) + expect(spy_logger).to have_received(:log).with(Logger::ERROR, 'User ID is invalid').exactly(6).times + end + it 'should properly track an event by calling dispatch_event with right params' do params = @expected_track_event_params @@ -627,22 +643,25 @@ class InvalidErrorHandler; end end describe '#get_variation' do - it 'should return nil when user_id is empty or nil' do - expect(project_instance.get_variation('test_experiment_with_audience', '', nil)).to eq(nil) - expect(project_instance.get_variation('test_experiment_with_audience', nil, nil)).to eq(nil) - expect(spy_logger).to have_received(:log).twice.with(Logger::ERROR, 'User ID is invalid') - end - it 'should call inputs_valid? with the proper arguments in get_variation' do expect(Optimizely::Helpers::Validator).to receive(:inputs_valid?).with( { - user_id: 'test_user', experiment_key: 'test_experiment_with_audience' }, spy_logger, Logger::ERROR ) project_instance.get_variation('test_experiment_with_audience', 'test_user', nil) end + it 'should log and return nil when user ID is non string' do + expect(project_instance.get_variation('test_experiment_with_audience', nil)).to eq(nil) + expect(project_instance.get_variation('test_experiment_with_audience', 5)).to eq(nil) + expect(project_instance.get_variation('test_experiment_with_audience', 5.5)).to eq(nil) + expect(project_instance.get_variation('test_experiment_with_audience', true)).to eq(nil) + expect(project_instance.get_variation('test_experiment_with_audience', {})).to eq(nil) + expect(project_instance.get_variation('test_experiment_with_audience', [])).to eq(nil) + expect(spy_logger).to have_received(:log).with(Logger::ERROR, 'User ID is invalid').exactly(6).times + end + it 'should have get_variation return expected variation when there are no audiences' do expect(project_instance.get_variation('test_experiment', 'test_user')) .to eq(config_body['experiments'][0]['variations'][0]['key']) @@ -771,12 +790,6 @@ class InvalidErrorHandler; end expect(spy_logger).to have_received(:log).once.with(Logger::ERROR, 'Feature flag key is invalid') end - it 'should return false when user_id is empty or nil' do - expect(project_instance.is_feature_enabled('boolean_single_variable_feature', '')).to be false - expect(project_instance.is_feature_enabled('boolean_single_variable_feature', nil)).to be false - expect(spy_logger).to have_received(:log).twice.with(Logger::ERROR, 'User ID is invalid') - end - it 'should return false when the feature flag key is invalid' do expect(project_instance.is_feature_enabled('totally_invalid_feature_key', 'test_user')).to be false expect(spy_logger).to have_received(:log).once.with(Logger::ERROR, "Feature flag key 'totally_invalid_feature_key' is not in datafile.") @@ -786,13 +799,29 @@ class InvalidErrorHandler; end it 'should call inputs_valid? with the proper arguments in is_feature_enabled' do expect(Optimizely::Helpers::Validator).to receive(:inputs_valid?).with( { - feature_flag_key: 'multi_variate_feature', - user_id: 'test_user' + feature_flag_key: 'multi_variate_feature' }, spy_logger, Logger::ERROR ) project_instance.is_feature_enabled('multi_variate_feature', 'test_user') end + it 'should log and return false when user ID is non string' do + expect(project_instance.is_feature_enabled('multi_variate_feature', nil)).to be(false) + expect(project_instance.is_feature_enabled('multi_variate_feature', 5)).to be(false) + expect(project_instance.is_feature_enabled('multi_variate_feature', 5.5)).to be(false) + expect(project_instance.is_feature_enabled('multi_variate_feature', true)).to be(false) + expect(project_instance.is_feature_enabled('multi_variate_feature', {})).to be(false) + expect(project_instance.is_feature_enabled('multi_variate_feature', [])).to be(false) + expect(spy_logger).to have_received(:log).with(Logger::ERROR, 'User ID is invalid').exactly(6).times + end + + it 'should return false when attributes are invalid' do + expect(Optimizely::Helpers::Validator).to receive(:attributes_valid?).once.with('invalid') + expect(error_handler).to receive(:handle_error).once.with(Optimizely::InvalidAttributeFormatError) + expect(project_instance.is_feature_enabled('multi_variate_feature', 'test_user', 'invalid')).to be false + expect(spy_logger).to have_received(:log).once.with(Logger::ERROR, 'Provided attributes are in an invalid format.') + end + it 'should return false when the user is not bucketed into any variation' do allow(project_instance.decision_service).to receive(:get_variation_for_feature).and_return(nil) @@ -902,10 +931,21 @@ class InvalidErrorHandler; end expect(project_instance.get_enabled_features('test_user')).to be_empty end - it 'should call inputs_valid? with the proper arguments in get_enabled_features' do - expect(Optimizely::Helpers::Validator.inputs_valid?({user_id: 'test_user'}, spy_logger, Logger::ERROR)).to eq(true) - expect(Optimizely::Helpers::Validator).to receive(:inputs_valid?).with({user_id: 'test_user'}, spy_logger, Logger::ERROR) - project_instance.get_enabled_features('test_user', 'browser_type' => 'chrome') + it 'should log and return empty when user ID is non string' do + expect(project_instance.get_enabled_features(nil)).to be_empty + expect(project_instance.get_enabled_features(5)).to be_empty + expect(project_instance.get_enabled_features(5.5)).to be_empty + expect(project_instance.get_enabled_features(true)).to be_empty + expect(project_instance.get_enabled_features({})).to be_empty + expect(project_instance.get_enabled_features([])).to be_empty + expect(spy_logger).to have_received(:log).with(Logger::ERROR, 'User ID is invalid').exactly(6).times + end + + it 'should return empty when attributes are invalid' do + expect(Optimizely::Helpers::Validator).to receive(:attributes_valid?).once.with('invalid') + expect(error_handler).to receive(:handle_error).once.with(Optimizely::InvalidAttributeFormatError) + expect(project_instance.get_enabled_features('test_user', 'invalid')).to be_empty + expect(spy_logger).to have_received(:log).once.with(Logger::ERROR, 'Provided attributes are in an invalid format.') end it 'should return only enabled feature flags keys' do @@ -1204,26 +1244,43 @@ class InvalidErrorHandler; end expect(spy_logger).to have_received(:log).once.with(Logger::ERROR, 'Variable key is invalid') end - it 'should return nil if user_id is empty or nil' do - expect(project_instance.get_feature_variable_integer('integer_single_variable_feature', 'integer_variable', nil, user_attributes)) - .to eq(nil) - expect(project_instance.get_feature_variable_integer('integer_single_variable_feature', 'integer_variable', '', user_attributes)) - .to eq(nil) - expect(spy_logger).to have_received(:log).twice.with(Logger::ERROR, 'User ID is invalid') - end - it 'should call inputs_valid? with the proper arguments in get_feature_variable_for_type' do expect(Optimizely::Helpers::Validator).to receive(:inputs_valid?).with( { feature_flag_key: 'integer_single_variable_feature', variable_key: 'integer_variable', - variable_type: 'integer', - user_id: nil + variable_type: 'integer' }, spy_logger, Logger::ERROR ) expect(project_instance.get_feature_variable_integer('integer_single_variable_feature', 'integer_variable', nil, user_attributes)) .to eq(nil) end + + it 'should log and return nil when user ID is non string' do + expect(project_instance.get_feature_variable_integer('integer_single_variable_feature', 'integer_variable', nil)).to eq(nil) + expect(project_instance.get_feature_variable_integer('integer_single_variable_feature', 'integer_variable', 5)).to eq(nil) + expect(project_instance.get_feature_variable_integer('integer_single_variable_feature', 'integer_variable', 5.5)).to eq(nil) + expect(project_instance.get_feature_variable_integer('integer_single_variable_feature', 'integer_variable', true)).to eq(nil) + expect(project_instance.get_feature_variable_integer('integer_single_variable_feature', 'integer_variable', {})).to eq(nil) + expect(project_instance.get_feature_variable_integer('integer_single_variable_feature', 'integer_variable', [])).to eq(nil) + expect(spy_logger).to have_received(:log).with(Logger::ERROR, 'User ID is invalid').exactly(6).times + end + end + + describe '#get_feature_variable_for_type with invalid attributes' do + it 'should return nil when attributes are invalid' do + expect(Optimizely::Helpers::Validator).to receive(:attributes_valid?).once.with('invalid') + expect(error_handler).to receive(:handle_error).once.with(Optimizely::InvalidAttributeFormatError) + expect(project_instance.send( + :get_feature_variable_for_type, + 'integer_single_variable_feature', + 'integer_variable', + 'integer', + 'test_user', + 'invalid' + )).to eq(nil) + expect(spy_logger).to have_received(:log).once.with(Logger::ERROR, 'Provided attributes are in an invalid format.') + end end describe 'when forced variation is used' do From 1bd41121f695a6acd4a52c1f86adb0379acfdf37 Mon Sep 17 00:00:00 2001 From: Rashid Siddique Date: Thu, 15 Nov 2018 18:21:18 +0500 Subject: [PATCH 2/3] refact(API): validates user_id inside validator. --- lib/optimizely.rb | 47 +++++++++-------------------- lib/optimizely/helpers/validator.rb | 11 ++++++- lib/optimizely/project_config.rb | 15 ++------- spec/project_config_spec.rb | 4 ++- spec/project_spec.rb | 25 +++++++++++---- 5 files changed, 49 insertions(+), 53 deletions(-) diff --git a/lib/optimizely.rb b/lib/optimizely.rb index fe8997a9..8750fbac 100644 --- a/lib/optimizely.rb +++ b/lib/optimizely.rb @@ -96,15 +96,11 @@ def activate(experiment_key, user_id, attributes = nil) return nil unless Optimizely::Helpers::Validator.inputs_valid?( { - experiment_key: experiment_key + experiment_key: experiment_key, + user_id: user_id }, @logger, Logger::ERROR ) - unless user_id.is_a?(String) - @logger.log(Logger::ERROR, "#{Optimizely::Helpers::Constants::INPUT_VARIABLES['USER_ID']} is invalid") - return nil - end - variation_key = get_variation(experiment_key, user_id, attributes) if variation_key.nil? @@ -136,15 +132,11 @@ def get_variation(experiment_key, user_id, attributes = nil) return nil unless Optimizely::Helpers::Validator.inputs_valid?( { - experiment_key: experiment_key + experiment_key: experiment_key, + user_id: user_id }, @logger, Logger::ERROR ) - unless user_id.is_a?(String) - @logger.log(Logger::ERROR, "#{Optimizely::Helpers::Constants::INPUT_VARIABLES['USER_ID']} is invalid") - return nil - end - unless user_inputs_valid?(attributes) @logger.log(Logger::INFO, "Not activating user '#{user_id}.") return nil @@ -202,15 +194,11 @@ def track(event_key, user_id, attributes = nil, event_tags = nil) return nil unless Optimizely::Helpers::Validator.inputs_valid?( { - event_key: event_key + event_key: event_key, + user_id: user_id }, @logger, Logger::ERROR ) - unless user_id.is_a?(String) - @logger.log(Logger::ERROR, "#{Optimizely::Helpers::Constants::INPUT_VARIABLES['USER_ID']} is invalid") - return nil - end - return nil unless user_inputs_valid?(attributes, event_tags) experiment_ids = @config.get_experiment_ids_for_event(event_key) @@ -265,15 +253,11 @@ def is_feature_enabled(feature_flag_key, user_id, attributes = nil) return false unless Optimizely::Helpers::Validator.inputs_valid?( { - feature_flag_key: feature_flag_key + feature_flag_key: feature_flag_key, + user_id: user_id }, @logger, Logger::ERROR ) - unless user_id.is_a?(String) - @logger.log(Logger::ERROR, "#{Optimizely::Helpers::Constants::INPUT_VARIABLES['USER_ID']} is invalid") - return false - end - return false unless user_inputs_valid?(attributes) feature_flag = @config.get_feature_flag_from_key(feature_flag_key) @@ -323,10 +307,11 @@ def get_enabled_features(user_id, attributes = nil) return enabled_features end - unless user_id.is_a?(String) - @logger.log(Logger::ERROR, "#{Optimizely::Helpers::Constants::INPUT_VARIABLES['USER_ID']} is invalid") - return enabled_features - end + return enabled_features unless Optimizely::Helpers::Validator.inputs_valid?( + { + user_id: user_id + }, @logger, Logger::ERROR + ) return enabled_features unless user_inputs_valid?(attributes) @@ -467,16 +452,12 @@ def get_feature_variable_for_type(feature_flag_key, variable_key, variable_type, { feature_flag_key: feature_flag_key, variable_key: variable_key, + user_id: user_id, variable_type: variable_type }, @logger, Logger::ERROR ) - unless user_id.is_a?(String) - @logger.log(Logger::ERROR, "#{Optimizely::Helpers::Constants::INPUT_VARIABLES['USER_ID']} is invalid") - return nil - end - return nil unless user_inputs_valid?(attributes) feature_flag = @config.get_feature_flag_from_key(feature_flag_key) diff --git a/lib/optimizely/helpers/validator.rb b/lib/optimizely/helpers/validator.rb index 041e31aa..6a2466c7 100644 --- a/lib/optimizely/helpers/validator.rb +++ b/lib/optimizely/helpers/validator.rb @@ -108,13 +108,22 @@ def inputs_valid?(variables, logger = NoOpLogger.new, level = Logger::ERROR) return false unless variables.respond_to?(:each) && !variables.empty? is_valid = true + if variables.include? :user_id + # Empty str is a valid user ID. + unless variables[:user_id].is_a?(String) + is_valid = false + logger.log(level, "#{Constants::INPUT_VARIABLES['USER_ID']} is invalid") + end + variables.delete :user_id + end + variables.each do |key, value| next if value.is_a?(String) && !value.empty? is_valid = false next unless logger_valid?(logger) && level - logger.log(level, "#{Optimizely::Helpers::Constants::INPUT_VARIABLES[key.to_s.upcase]} is invalid") + logger.log(level, "#{Constants::INPUT_VARIABLES[key.to_s.upcase]} is invalid") end is_valid end diff --git a/lib/optimizely/project_config.rb b/lib/optimizely/project_config.rb index fb178800..3a90c6be 100644 --- a/lib/optimizely/project_config.rb +++ b/lib/optimizely/project_config.rb @@ -282,15 +282,11 @@ def get_forced_variation(experiment_key, user_id) return nil unless Optimizely::Helpers::Validator.inputs_valid?( { - experiment_key: experiment_key + experiment_key: experiment_key, + user_id: user_id }, @logger, Logger::DEBUG ) - unless user_id.is_a?(String) - @logger.log(Logger::DEBUG, "#{Optimizely::Helpers::Constants::INPUT_VARIABLES['USER_ID']} is invalid") - return nil - end - unless @forced_variation_map.key? user_id @logger.log(Logger::DEBUG, "User '#{user_id}' is not in the forced variation map.") return nil @@ -333,15 +329,10 @@ def set_forced_variation(experiment_key, user_id, variation_key) # # Returns a boolean value that indicates if the set completed successfully. - input_values = {experiment_key: experiment_key} + input_values = {experiment_key: experiment_key, user_id: user_id} input_values[:variation_key] = variation_key unless variation_key.nil? return false unless Optimizely::Helpers::Validator.inputs_valid?(input_values, @logger, Logger::DEBUG) - unless user_id.is_a?(String) - @logger.log(Logger::DEBUG, "#{Optimizely::Helpers::Constants::INPUT_VARIABLES['USER_ID']} is invalid") - return false - end - experiment = get_experiment_from_key(experiment_key) experiment_id = experiment['id'] if experiment # check if the experiment exists in the datafile diff --git a/spec/project_config_spec.rb b/spec/project_config_spec.rb index 7116de54..50b341d0 100644 --- a/spec/project_config_spec.rb +++ b/spec/project_config_spec.rb @@ -947,6 +947,7 @@ expect(Optimizely::Helpers::Validator).to receive(:inputs_valid?).with( { experiment_key: @valid_experiment[:key], + user_id: @user_id, variation_key: @valid_variation[:key] }, spy_logger, Logger::DEBUG ) @@ -966,7 +967,8 @@ it 'should call inputs_valid? with the proper arguments in get_forced_variation' do expect(Optimizely::Helpers::Validator).to receive(:inputs_valid?).with( { - experiment_key: @valid_experiment[:key] + experiment_key: @valid_experiment[:key], + user_id: @user_id }, spy_logger, Logger::DEBUG ) config.get_forced_variation(@valid_experiment[:key], @user_id) diff --git a/spec/project_spec.rb b/spec/project_spec.rb index f6300088..b47a08d5 100644 --- a/spec/project_spec.rb +++ b/spec/project_spec.rb @@ -271,7 +271,8 @@ class InvalidErrorHandler; end it 'should call inputs_valid? with the proper arguments in activate' do expect(Optimizely::Helpers::Validator).to receive(:inputs_valid?).with( { - experiment_key: 'test_experiment_with_audience' + experiment_key: 'test_experiment_with_audience', + user_id: 'test_user' }, spy_logger, Logger::ERROR ) project_instance.activate('test_experiment_with_audience', 'test_user') @@ -454,7 +455,8 @@ class InvalidErrorHandler; end it 'should call inputs_valid? with the proper arguments in track' do expect(Optimizely::Helpers::Validator).to receive(:inputs_valid?).with( { - event_key: 'test_event' + event_key: 'test_event', + user_id: 'test_user' }, spy_logger, Logger::ERROR ) project_instance.track('test_event', 'test_user') @@ -646,7 +648,8 @@ class InvalidErrorHandler; end it 'should call inputs_valid? with the proper arguments in get_variation' do expect(Optimizely::Helpers::Validator).to receive(:inputs_valid?).with( { - experiment_key: 'test_experiment_with_audience' + experiment_key: 'test_experiment_with_audience', + user_id: 'test_user' }, spy_logger, Logger::ERROR ) project_instance.get_variation('test_experiment_with_audience', 'test_user', nil) @@ -799,7 +802,8 @@ class InvalidErrorHandler; end it 'should call inputs_valid? with the proper arguments in is_feature_enabled' do expect(Optimizely::Helpers::Validator).to receive(:inputs_valid?).with( { - feature_flag_key: 'multi_variate_feature' + feature_flag_key: 'multi_variate_feature', + user_id: 'test_user' }, spy_logger, Logger::ERROR ) project_instance.is_feature_enabled('multi_variate_feature', 'test_user') @@ -926,6 +930,15 @@ class InvalidErrorHandler; end expect(invalid_project.get_enabled_features('test_user')).to be_empty end + it 'should call inputs_valid? with the proper arguments in get_enabled_features' do + expect(Optimizely::Helpers::Validator).to receive(:inputs_valid?).with( + { + user_id: 'test_user' + }, spy_logger, Logger::ERROR + ) + project_instance.get_enabled_features('test_user') + end + it 'should return empty when no feature flag is enabled' do allow(project_instance).to receive(:is_feature_enabled).and_return(false) expect(project_instance.get_enabled_features('test_user')).to be_empty @@ -1249,11 +1262,11 @@ class InvalidErrorHandler; end { feature_flag_key: 'integer_single_variable_feature', variable_key: 'integer_variable', + user_id: 'test_user', variable_type: 'integer' }, spy_logger, Logger::ERROR ) - expect(project_instance.get_feature_variable_integer('integer_single_variable_feature', 'integer_variable', nil, user_attributes)) - .to eq(nil) + project_instance.get_feature_variable_integer('integer_single_variable_feature', 'integer_variable', 'test_user', user_attributes) end it 'should log and return nil when user ID is non string' do From 9a053beb6d2b87d76db659f331908032f2459e45 Mon Sep 17 00:00:00 2001 From: Rashid Siddique Date: Tue, 27 Nov 2018 19:48:07 +0500 Subject: [PATCH 3/3] fix: master branch update issues --- lib/optimizely.rb | 2 -- spec/project_spec.rb | 4 ++-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/optimizely.rb b/lib/optimizely.rb index 83291dc1..8750fbac 100644 --- a/lib/optimizely.rb +++ b/lib/optimizely.rb @@ -315,8 +315,6 @@ def get_enabled_features(user_id, attributes = nil) return enabled_features unless user_inputs_valid?(attributes) - return enabled_features unless user_inputs_valid?(attributes) - @config.feature_flags.each do |feature| enabled_features.push(feature['key']) if is_feature_enabled( feature['key'], diff --git a/spec/project_spec.rb b/spec/project_spec.rb index d99afccd..cbc63a7f 100644 --- a/spec/project_spec.rb +++ b/spec/project_spec.rb @@ -910,7 +910,7 @@ class InvalidErrorHandler; end expect(error_handler).to receive(:handle_error).once.with(Optimizely::InvalidAttributeFormatError) expect(project_instance.is_feature_enabled('multi_variate_feature', 'test_user', 'invalid')).to be false end - + it 'should log and raise an exception when called with attributes in an invalid format' do expect_any_instance_of(Optimizely::RaiseErrorHandler).to receive(:handle_error).once.with(Optimizely::InvalidAttributeFormatError) expect(project_instance.is_feature_enabled('multi_variate_feature', 'test_user', 'invalid_attributes')).to be false @@ -1394,7 +1394,7 @@ class InvalidErrorHandler; end it 'should log and raise an exception when called with attributes in an invalid format' do expect_any_instance_of(Optimizely::RaiseErrorHandler).to receive(:handle_error).once.with(Optimizely::InvalidAttributeFormatError) - expect(project_instance.get_feature_variable_integer('integer_single_variable_feature', 'integer_variable', user_id, 'invalid_attributes')).to eq(nil) + expect(project_instance.get_feature_variable_integer('integer_single_variable_feature', 'integer_variable', 'test_user', 'invalid_attributes')).to eq(nil) expect(spy_logger).to have_received(:log).once.with(Logger::ERROR, 'Provided attributes are in an invalid format.') end end