diff --git a/src/Optimizely/Optimizely.php b/src/Optimizely/Optimizely.php index 4a0946ab..1e9d4886 100644 --- a/src/Optimizely/Optimizely.php +++ b/src/Optimizely/Optimizely.php @@ -36,6 +36,7 @@ use Optimizely\Notification\NotificationCenter; use Optimizely\Notification\NotificationType; use Optimizely\UserProfile\UserProfileServiceInterface; +use Optimizely\Utils\Errors; use Optimizely\Utils\Validator; use Optimizely\Utils\VariableTypeUtils; @@ -46,6 +47,10 @@ */ class Optimizely { + const USER_ID = 'User ID'; + const EVENT_KEY = 'Event Key'; + const EXPERIMENT_KEY ='Experiment Key'; + /** * @var ProjectConfig */ @@ -288,6 +293,16 @@ public function activate($experimentKey, $userId, $attributes = null) return null; } + if (!$this->validateInputs( + [ + self::EXPERIMENT_KEY =>$experimentKey, + self::USER_ID => $userId + ] + ) + ) { + return null; + } + $variationKey = $this->getVariation($experimentKey, $userId, $attributes); if (is_null($variationKey)) { $this->_logger->log(Logger::INFO, sprintf('Not activating user "%s".', $userId)); @@ -314,6 +329,16 @@ public function track($eventKey, $userId, $attributes = null, $eventTags = null) return; } + if (!$this->validateInputs( + [ + self::EVENT_KEY =>$eventKey, + self::USER_ID => $userId + ] + ) + ) { + return null; + } + if (!$this->validateUserInputs($attributes, $eventTags)) { return; } @@ -401,6 +426,16 @@ public function getVariation($experimentKey, $userId, $attributes = null) return null; } + if (!$this->validateInputs( + [ + self::EXPERIMENT_KEY =>$experimentKey, + self::USER_ID => $userId + ] + ) + ) { + return null; + } + $experiment = $this->_config->getExperimentFromKey($experimentKey); if (is_null($experiment->getKey())) { @@ -728,4 +763,27 @@ public function getFeatureVariableString($featureFlagKey, $variableKey, $userId, return $variableValue; } + + /** + * Calls Validator::validateNonEmptyString for each value in array + * Logs for each invalid value + * + * @param array values to validate + * @param logger + * + * @return bool True if all of the values are valid, False otherwise + */ + protected function validateInputs(array $values, $logLevel = Logger::ERROR) + { + $isValid = true; + foreach ($values as $key => $value) { + if (!Validator::validateNonEmptyString($value)) { + $isValid = false; + $message = sprintf(Errors::INVALID_FORMAT, $key); + $this->_logger->log($logLevel, $message); + } + } + + return $isValid; + } } diff --git a/src/Optimizely/Utils/Errors.php b/src/Optimizely/Utils/Errors.php new file mode 100644 index 00000000..326c64d7 --- /dev/null +++ b/src/Optimizely/Utils/Errors.php @@ -0,0 +1,22 @@ +expectOutputRegex('/Datafile has invalid format. Failing "activate"./'); } + public function testActivateCallsValidateInputs() + { + $optimizelyMock = $this->getMockBuilder(Optimizely::class) + ->setConstructorArgs(array($this->datafile)) + ->setMethods(array('validateInputs')) + ->getMock(); + + $experimentKey = 'test_experiment'; + $userId = 'test_user'; + $inputArray = [ + Optimizely::EXPERIMENT_KEY => $experimentKey, + Optimizely::USER_ID => $userId + ]; + + // assert that validateInputs gets called with exactly same keys + $optimizelyMock->expects($this->once()) + ->method('validateInputs') + ->with($inputArray) + ->willReturn(false); + + $this->assertNull($optimizelyMock->activate($experimentKey, $userId)); + } + public function testActivateInvalidAttributes() { $this->loggerMock->expects($this->exactly(2)) @@ -460,6 +483,29 @@ public function testGetVariationInvalidOptimizelyObject() $this->expectOutputRegex('/Datafile has invalid format. Failing "getVariation"./'); } + public function testGetVariationCallsValidateInputs() + { + $optimizelyMock = $this->getMockBuilder(Optimizely::class) + ->setConstructorArgs(array($this->datafile)) + ->setMethods(array('validateInputs')) + ->getMock(); + + $experimentKey = 'test_experiment'; + $userId = 'test_user'; + $inputArray = [ + Optimizely::EXPERIMENT_KEY => $experimentKey, + Optimizely::USER_ID => $userId + ]; + + // assert that validateInputs gets called with exactly same keys + $optimizelyMock->expects($this->once()) + ->method('validateInputs') + ->with($inputArray) + ->willReturn(false); + + $this->assertNull($optimizelyMock->getVariation($experimentKey, $userId)); + } + public function testGetVariationInvalidAttributes() { $this->loggerMock->expects($this->once()) @@ -679,6 +725,29 @@ public function testTrackInvalidOptimizelyObject() $this->expectOutputRegex('/Datafile has invalid format. Failing "track"./'); } + public function testTrackCallsValidateInputs() + { + $optimizelyMock = $this->getMockBuilder(Optimizely::class) + ->setConstructorArgs(array($this->datafile)) + ->setMethods(array('validateInputs')) + ->getMock(); + + $eventKey = 'test_event'; + $userId = 'test_user'; + $inputArray = [ + Optimizely::EVENT_KEY => $eventKey, + Optimizely::USER_ID => $userId + ]; + + // assert that validateInputs gets called with exactly same keys + $optimizelyMock->expects($this->once()) + ->method('validateInputs') + ->with($inputArray) + ->willReturn(false); + + $this->assertNull($optimizelyMock->track($eventKey, $userId)); + } + public function testTrackInvalidAttributes() { $this->loggerMock->expects($this->once()) @@ -1632,14 +1701,17 @@ public function testSetForcedVariationWithInvalidUserIDs() 'location' => 'San Francisco' ]; - // null user ID --> set should fail, normal bucketing [TODO (Alda): getVariation on a null userID should return null] + // null user ID should fail setForcedVariation. getVariation on a null userID should return null $this->assertFalse($optlyObject->setForcedVariation('test_experiment', null, 'bad_variation'), 'Set variation for null user ID should have failed.'); + $variationKey = $optlyObject->getVariation('test_experiment', null, $userAttributes); - $this->assertEquals('variation', $variationKey, sprintf('Invalid variation "%s" for null user ID.', $variationKey)); - // empty string user ID --> set should fail, normal bucketing [TODO (Alda): getVariation on an empty userID should return null] + $this->assertNull($variationKey); + + // empty string user ID should fail setForcedVariation. getVariation on an empty userID should return null $this->assertFalse($optlyObject->setForcedVariation('test_experiment', '', 'bad_variation'), 'Set variation for empty string user ID should have failed.'); + $variationKey = $optlyObject->getVariation('test_experiment', '', $userAttributes); - $this->assertEquals('variation', $variationKey, sprintf('Invalid variation "%s" for empty string user ID.', $variationKey)); + $this->assertNull($variationKey); } public function testSetForcedVariationWithInvalidExperimentKeys() @@ -2850,4 +2922,66 @@ public function testSendImpressionEventWithAttributes() $optlyObject->sendImpressionEvent('test_experiment', 'control', 'test_user', $userAttributes); } + + /* + * test ValidateInputs method validates and logs for different and multiple keys + */ + public function testValidateInputs() + { + $optlyObject = new OptimizelyTester($this->datafile, null, $this->loggerMock); + + $INVALID_USER_ID_LOG = 'Provided User ID is in an invalid format.'; + $INVALID_EVENT_KEY_LOG = 'Provided Event Key is in an invalid format.'; + $INVALID_RANDOM_KEY_LOG = 'Provided ABCD is in an invalid format.'; + + $this->loggerMock->expects($this->at(0)) + ->method('log') + ->with(Logger::ERROR, $INVALID_USER_ID_LOG); + $this->assertFalse($optlyObject->validateInputs([Optimizely::USER_ID => null])); + + $this->loggerMock->expects($this->at(0)) + ->method('log') + ->with(Logger::ERROR, $INVALID_RANDOM_KEY_LOG); + $this->assertFalse($optlyObject->validateInputs(['ABCD' => null])); + + // Verify that multiple messages are logged. + $this->loggerMock->expects($this->at(0)) + ->method('log') + ->with(Logger::ERROR, $INVALID_EVENT_KEY_LOG); + + $this->loggerMock->expects($this->at(1)) + ->method('log') + ->with(Logger::ERROR, $INVALID_USER_ID_LOG); + $this->assertFalse($optlyObject->validateInputs([Optimizely::EVENT_KEY => null, Optimizely::USER_ID => null])); + + // Verify that logger level is taken into account + $this->loggerMock->expects($this->at(0)) + ->method('log') + ->with(Logger::INFO, $INVALID_RANDOM_KEY_LOG); + $this->assertFalse($optlyObject->validateInputs(['ABCD' => null], Logger::INFO)); + + // Verify that it returns true and nothing is logged if valid values given + $this->loggerMock->expects($this->never()) + ->method('log'); + $this->assertTrue($optlyObject->validateInputs([Optimizely::EVENT_KEY => 'test_event', Optimizely::USER_ID => 'test_user'])); + } + + /* + * test ValidateInputs method only returns true for non-empty string + */ + public function testValidateInputsWithDifferentValues() + { + $optlyObject = new OptimizelyTester($this->datafile); + + $this->assertTrue($optlyObject->validateInputs(['key' => '0'])); + $this->assertTrue($optlyObject->validateInputs(['key' => 'test_user'])); + + $this->assertFalse($optlyObject->validateInputs(['key' => ''])); + $this->assertFalse($optlyObject->validateInputs(['key' => null])); + $this->assertFalse($optlyObject->validateInputs(['key' => false])); + $this->assertFalse($optlyObject->validateInputs(['key' => true])); + $this->assertFalse($optlyObject->validateInputs(['key' => 2])); + $this->assertFalse($optlyObject->validateInputs(['key' => 2.0])); + $this->assertFalse($optlyObject->validateInputs(['key' => array()])); + } } diff --git a/tests/TestData.php b/tests/TestData.php index 8ebabe60..e25fe695 100644 --- a/tests/TestData.php +++ b/tests/TestData.php @@ -17,6 +17,9 @@ namespace Optimizely\Tests; use Exception; + +use Monolog\Logger; + use Optimizely\Bucketer; use Optimizely\Event\Dispatcher\EventDispatcherInterface; use Optimizely\Event\LogEvent; @@ -797,6 +800,11 @@ public function sendImpressionEvent($experimentKey, $variationKey, $userId, $att { parent::sendImpressionEvent($experimentKey, $variationKey, $userId, $attributes); } + + public function validateInputs(array $values, $logLevel = Logger::ERROR) + { + return parent::validateInputs($values, $logLevel); + } } class FireNotificationTester