feature(API): Adds input validation in all API methods and validates empty user Id. (#341)

yasirfolio3 · thomaszurkan-optimizely · commit 4e1f9bdca25b · 2018-12-20T16:13:36.000-05:00
* feature(API): Adds input validation in all API methods and validates empty user Id.

* Addressed code review comments.

* Addressed code review comments.
diff --git a/OptimizelySDKCore/OptimizelySDKCore/OPTLYLoggerMessages.h b/OptimizelySDKCore/OptimizelySDKCore/OPTLYLoggerMessages.h
@@ -204,6 +204,8 @@ extern NSString *const OPTLYLoggerMessagesGroupUnknownForGroupId;
 extern NSString *const OPTLYLoggerMessagesGetVariationNilVariation;
 extern NSString *const OPTLYLoggerMessagesVariationKeyUnknownForExperimentKey;
 extern NSString *const OPTLYLoggerMessagesProjectConfigUserIdInvalid;
+extern NSString *const OPTLYLoggerMessagesProjectConfigExperimentKeyInvalid;
+extern NSString *const OPTLYLoggerMessagesProjectConfigVariationKeyInvalid;
 extern NSString *const OPTLYLoggerMessagesAttributeIsReserved;
 extern NSString *const OPTLYLoggerMessagesAttributeNotFound;
 
diff --git a/OptimizelySDKCore/OptimizelySDKCore/OPTLYLoggerMessages.m b/OptimizelySDKCore/OptimizelySDKCore/OPTLYLoggerMessages.m
@@ -197,7 +197,9 @@
 NSString *const OPTLYLoggerMessagesGroupUnknownForGroupId = @"[PROJECT CONFIG] Group not found for group ID: %@. Group ID is not in the datafile."; // group id
 NSString *const OPTLYLoggerMessagesGetVariationNilVariation = @"[PROJECT CONFIG] Get variation returned a nil variation for user %@, experiment %@";
 NSString *const OPTLYLoggerMessagesVariationKeyUnknownForExperimentKey = @"[PROJECT CONFIG] Variation key %@ not found for experiment key %@.";
-NSString *const OPTLYLoggerMessagesProjectConfigUserIdInvalid = @"[PROJECT CONFIG] User ID cannot be an empty string.";
+NSString *const OPTLYLoggerMessagesProjectConfigUserIdInvalid = @"[PROJECT CONFIG] User ID cannot be nil.";
+NSString *const OPTLYLoggerMessagesProjectConfigExperimentKeyInvalid = @"[PROJECT CONFIG] Experiment Key cannot be nil or an empty string.";
+NSString *const OPTLYLoggerMessagesProjectConfigVariationKeyInvalid = @"[PROJECT CONFIG] Variation key cannot be an empty string.";
 NSString *const OPTLYLoggerMessagesAttributeIsReserved = @"[PROJECT CONFIG] Attribute %@ unexpectedly has reserved prefix %@; using attribute ID instead of reserved attribute name.";
 NSString *const OPTLYLoggerMessagesAttributeNotFound = @"[PROJECT CONFIG] Attribute key %@ is not in datafile.";
 
diff --git a/OptimizelySDKCore/OptimizelySDKCore/OPTLYProjectConfig.h b/OptimizelySDKCore/OptimizelySDKCore/OPTLYProjectConfig.h
@@ -165,7 +165,7 @@ __attribute((deprecated("Use OPTLYProjectConfig initWithBuilder method instead."
  */
 - (BOOL)setForcedVariation:(nonnull NSString *)experimentKey
                     userId:(nonnull NSString *)userId
-              variationKey:(nonnull NSString *)variationKey;
+              variationKey:(nullable NSString *)variationKey;
 
 /**
  * Get variation for experiment and user ID with user attributes.
diff --git a/OptimizelySDKCore/OptimizelySDKCore/OPTLYProjectConfig.m b/OptimizelySDKCore/OptimizelySDKCore/OPTLYProjectConfig.m
@@ -308,74 +308,89 @@ - (OPTLYVariable *)getVariableForVariableKey:(NSString *)variableKey {
 
 - (OPTLYVariation *)getForcedVariation:(nonnull NSString *)experimentKey
                                 userId:(nonnull NSString *)userId {
+
+    NSMutableDictionary<NSString *, NSString *> *dictionary = self.forcedVariationMap[userId];
+    if (dictionary == nil) {
+        return nil;
+    }
     // Get experiment from experimentKey .
     OPTLYExperiment *experiment = [self getExperimentForKey:experimentKey];
-    if (!experiment) {
-        // NOTE: getExperimentForKey: will log any non-existent experimentKey and return experiment == nil for us.
+    // this case is logged in getExperimentFromKey
+    if (!experiment || [self isNullOrEmpty:experiment.experimentId]) {
         return nil;
     }
+    
     OPTLYVariation *variation = nil;
     @synchronized (self.forcedVariationMap) {
-        NSMutableDictionary<NSString *, NSString *> *dictionary = self.forcedVariationMap[userId];
-        if (dictionary != nil) {
-            // Get variation from experimentId and variationId .
-            NSString *experimentId = experiment.experimentId;
-            NSString *variationId = dictionary[experimentId];
-            variation = [experiment getVariationForVariationId:variationId];
+        // Get variation from experimentId and variationId .
+        NSString *variationId = dictionary[experiment.experimentId];
+        if ([self isNullOrEmpty:variationId]) {
+            return nil;
+        }
+        variation = [experiment getVariationForVariationId:variationId];
+        
+        if (!variation || [self isNullOrEmpty:variation.variationKey]) {
+            return nil;
         }
     }
+    
     return variation;
 }
 
 - (BOOL)setForcedVariation:(nonnull NSString *)experimentKey
                     userId:(nonnull NSString *)userId
-              variationKey:(nonnull NSString *)variationKey {
+              variationKey:(nullable NSString *)variationKey {
     // Return YES if there were no errors, OW return NO .
+    //Check if variationKey is empty string
+    if (variationKey != nil && [variationKey isEqualToString:@""]) {
+        [self.logger logMessage:OPTLYLoggerMessagesProjectConfigVariationKeyInvalid withLevel:OptimizelyLogLevelDebug];
+        return NO;
+    }
+    
     // Get experiment from experimentKey .
     OPTLYExperiment *experiment = [self getExperimentForKey:experimentKey];
     if (!experiment) {
         // NOTE: getExperimentForKey: will log any non-existent experimentKey and return experiment == nil for us.
         return NO;
     }
-    NSString *experimentId=experiment.experimentId;
-    // Check for valid userId
-    if ([userId length]==0) {
-        [self.logger logMessage:OPTLYLoggerMessagesProjectConfigUserIdInvalid withLevel:OptimizelyLogLevelDebug];
+    NSString *experimentId = experiment.experimentId;
+    //Check if experimentId is valid
+    if ([self isNullOrEmpty:experimentId]) {
         return NO;
     }
-    // Get variation from experiment and non-nil variationKey, if applicable.
-    OPTLYVariation *variation = nil;
-    if (variationKey != nil) {
-        variation = [experiment getVariationForVariationKey:variationKey];
-        if (!variation) {
-            NSString *logMessage = [NSString stringWithFormat:OPTLYLoggerMessagesVariationKeyUnknownForExperimentKey, variationKey, experimentKey];
-            [self.logger logMessage:logMessage withLevel:OptimizelyLogLevelDebug];
-            // Leave in current state, and report NO meaning there was an error.
-            return NO;
+    
+    @synchronized (self.forcedVariationMap) {
+        // clear the forced variation if the variation key is null
+        if (variationKey == nil) {
+            // Locate relevant dictionary inside forcedVariationMap
+            NSMutableDictionary<NSString *, NSString *> *dictionary = self.forcedVariationMap[userId];
+            if (dictionary != nil) {
+                [dictionary removeObjectForKey:experimentId];
+                self.forcedVariationMap[userId] = dictionary;
+            }
+            return YES;
         }
     }
+    
+    // Get variation from experiment and non-nil variationKey, if applicable.
+    OPTLYVariation *variation = [experiment getVariationForVariationKey:variationKey];
+    if (!variation || [self isNullOrEmpty:variation.variationId]) {
+        NSString *logMessage = [NSString stringWithFormat:OPTLYLoggerMessagesVariationKeyUnknownForExperimentKey, variationKey, experimentKey];
+        [self.logger logMessage:logMessage withLevel:OptimizelyLogLevelDebug];
+        // Leave in current state, and report NO meaning there was an error.
+        return NO;
+    }
+    
     @synchronized (self.forcedVariationMap) {
-        // Locate relevant dictionary inside forcedVariationMap
+        // Add User if not exist.
         NSMutableDictionary<NSString *, NSString *> *dictionary = self.forcedVariationMap[userId];
-        if ((dictionary == nil) && (variationKey != nil)) {
+        if (dictionary == nil) {
             // We need a non-nil dictionary to store an OPTLYVariation .
             dictionary = [[NSMutableDictionary alloc] init];
             self.forcedVariationMap[userId] = dictionary;
         }
-        // Apply change to dictionary
-        if (variation == nil) {
-            // NOTE: removeObjectForKey: "Does nothing if [experimentKey] does not exist."
-            // https://developer.apple.com/documentation/foundation/nsmutabledictionary/1416518-removeobjectforkey?language=objc
-            [dictionary removeObjectForKey:experimentId];
-            if ([dictionary count] == 0) {
-                // For elegance, we can remove empty dictionary for userId from self.forcedVariationMap
-                [self.forcedVariationMap removeObjectForKey:userId];
-            }
-        } else {
-            // If there is no OPTLYExperiment *experiment corresponding to experimentKey ,
-            // then we will land in the "(variation == nil)" case above due to code above.
-            dictionary[experimentId] = variation.variationId;
-        };
+        // Add/Replace Experiment to Variation ID map.
+        self.forcedVariationMap[userId][experimentId] = variation.variationId;
     };
     return YES;
 }
@@ -623,5 +638,7 @@ - (OPTLYVariation *)getVariationForExperiment:(NSString *)experimentKey
     return bucketedVariation;
 }
 
-
+- (BOOL)isNullOrEmpty:(nullable NSString *)value {
+    return ((value == nil) || [value isKindOfClass: [NSNull class]] || [value isEqualToString:@""]);
+}
 @end
diff --git a/OptimizelySDKCore/OptimizelySDKCore/Optimizely.m b/OptimizelySDKCore/OptimizelySDKCore/Optimizely.m
@@ -43,6 +43,15 @@
 #import "OPTLYVariable.h"
 #import "OPTLYVariationVariable.h"
 
+NSString *const OptimizelyNotificationsUserDictionaryExperimentKey = @"experiment";
+NSString *const OptimizelyNotificationsUserDictionaryVariationKey = @"variation";
+NSString *const OptimizelyNotificationsUserDictionaryUserIdKey = @"userId";
+NSString *const OptimizelyNotificationsUserDictionaryAttributesKey = @"attributes";
+NSString *const OptimizelyNotificationsUserDictionaryEventNameKey = @"eventKey";
+NSString *const OptimizelyNotificationsUserDictionaryFeatureKey = @"feature";
+NSString *const OptimizelyNotificationsUserDictionaryVariableKey = @"variable";
+NSString *const OptimizelyNotificationsUserDictionaryExperimentVariationMappingKey = @"ExperimentVariationMapping";
+
 @implementation Optimizely
 
 + (instancetype)init:(OPTLYBuilderBlock)builderBlock {
@@ -186,27 +195,39 @@ - (OPTLYVariation *)variation:(NSString *)experimentKey
 #pragma mark Forced variation methods
 - (OPTLYVariation *)getForcedVariation:(nonnull NSString *)experimentKey
                                 userId:(nonnull NSString *)userId {
+    NSMutableDictionary<NSString *, NSString *> *inputValues = [[NSMutableDictionary alloc] initWithDictionary:@{
+                                                                                                                    OptimizelyNotificationsUserDictionaryUserIdKey:[self ObjectOrNull:userId],
+                                                                                                                    OptimizelyNotificationsUserDictionaryExperimentKey:[self ObjectOrNull:experimentKey]}];
+    if (![self validateStringInputs:inputValues logs:@{}]) {
+        return nil;
+    }
     return [self.config getForcedVariation:experimentKey
                                     userId:userId];
 }
 
 - (BOOL)setForcedVariation:(nonnull NSString *)experimentKey
                     userId:(nonnull NSString *)userId
               variationKey:(nullable NSString *)variationKey {
-    return [self.config setForcedVariation:experimentKey
+    NSMutableDictionary<NSString *, NSString *> *inputValues = [[NSMutableDictionary alloc] initWithDictionary:@{
+                                                                                                                    OptimizelyNotificationsUserDictionaryUserIdKey:[self ObjectOrNull:userId],
+                                                                                                                    OptimizelyNotificationsUserDictionaryExperimentKey:[self ObjectOrNull:experimentKey]}];
+    return [self validateStringInputs:inputValues logs:@{}] && [self.config setForcedVariation:experimentKey
                                     userId:userId
                               variationKey:variationKey];
 }
 
 #pragma mark - Feature Flag Methods
 
 - (BOOL)isFeatureEnabled:(NSString *)featureKey userId:(NSString *)userId attributes:(nullable NSDictionary<NSString *, NSObject *> *)attributes {
-    if ([userId getValidString] == nil) {
-        [self.logger logMessage:OPTLYLoggerMessagesFeatureDisabledUserIdInvalid withLevel:OptimizelyLogLevelError];
-        return false;
-    }
-    if ([featureKey getValidString] == nil) {
-        [self.logger logMessage:OPTLYLoggerMessagesFeatureDisabledFlagKeyInvalid withLevel:OptimizelyLogLevelError];
+    
+    NSMutableDictionary<NSString *, NSString *> *inputValues = [[NSMutableDictionary alloc] initWithDictionary:@{
+                                                                                                                    OptimizelyNotificationsUserDictionaryUserIdKey:[self ObjectOrNull:userId],
+                                                                                                                    OptimizelyNotificationsUserDictionaryExperimentKey:[self ObjectOrNull:featureKey]}];
+    NSDictionary <NSString *, NSString *> *logs = @{
+                                                    OptimizelyNotificationsUserDictionaryUserIdKey:OPTLYLoggerMessagesFeatureDisabledUserIdInvalid,
+                                                    OptimizelyNotificationsUserDictionaryExperimentKey:OPTLYLoggerMessagesFeatureDisabledFlagKeyInvalid};
+    
+    if (![self validateStringInputs:inputValues logs:logs]) {
         return false;
     }
     
@@ -246,16 +267,17 @@ - (NSString *)getFeatureVariableValueForType:(NSString *)variableType
                                  variableKey:(nullable NSString *)variableKey
                                       userId:(nullable NSString *)userId
                                   attributes:(nullable NSDictionary<NSString *, NSObject *> *)attributes {
-    if ([featureKey getValidString] == nil) {
-        [self.logger logMessage:OPTLYLoggerMessagesFeatureVariableValueFlagKeyInvalid withLevel:OptimizelyLogLevelError];
-        return nil;
-    }
-    if ([variableKey getValidString] == nil) {
-        [self.logger logMessage:OPTLYLoggerMessagesFeatureVariableValueVariableKeyInvalid withLevel:OptimizelyLogLevelError];
-        return nil;
-    }
-    if ([userId getValidString] == nil) {
-        [self.logger logMessage:OPTLYLoggerMessagesFeatureVariableValueUserIdInvalid withLevel:OptimizelyLogLevelError];
+    
+    NSMutableDictionary<NSString *, NSString *> *inputValues = [[NSMutableDictionary alloc] initWithDictionary:@{
+                                                                                                                    OptimizelyNotificationsUserDictionaryUserIdKey:[self ObjectOrNull:userId],
+                                                                                                                    OptimizelyNotificationsUserDictionaryFeatureKey:[self ObjectOrNull:featureKey],
+                                                                                                                    OptimizelyNotificationsUserDictionaryVariableKey:[self ObjectOrNull:variableKey]}];
+    NSDictionary <NSString *, NSString *> *logs = @{
+                                                    OptimizelyNotificationsUserDictionaryUserIdKey:OPTLYLoggerMessagesFeatureVariableValueUserIdInvalid,
+                                                    OptimizelyNotificationsUserDictionaryVariableKey:OPTLYLoggerMessagesFeatureVariableValueVariableKeyInvalid,
+                                                    OptimizelyNotificationsUserDictionaryFeatureKey:OPTLYLoggerMessagesFeatureVariableValueFlagKeyInvalid};
+    
+    if (![self validateStringInputs:inputValues logs:logs]) {
         return nil;
     }
     
@@ -364,7 +386,17 @@ - (NSString *)getFeatureVariableString:(nullable NSString *)featureKey
 -(NSArray<NSString *> *)getEnabledFeatures:(NSString *)userId
                                 attributes:(NSDictionary<NSString *, NSObject *> *)attributes {
     
+    
     NSMutableArray<NSString *> *enabledFeatures = [NSMutableArray new];
+    
+    NSMutableDictionary<NSString *, NSString *> *inputValues = [[NSMutableDictionary alloc] initWithDictionary:@{
+                                                                                                                    OptimizelyNotificationsUserDictionaryUserIdKey:[self ObjectOrNull:userId]}];
+    NSDictionary <NSString *, NSString *> *logs = @{};
+    
+    if (![self validateStringInputs:inputValues logs:logs]) {
+        return enabledFeatures;
+    }
+    
     for (OPTLYFeatureFlag *feature in self.config.featureFlags) {
         NSString *featureKey = feature.key;
         if ([self isFeatureEnabled:featureKey userId:userId attributes:attributes]) {
@@ -927,4 +959,57 @@ - (OPTLYVariation *)sendImpressionEventFor:(OPTLYExperiment *)experiment
     }
     return decisions;
 }
+                                      
++ (BOOL)isEmptyArray:(NSObject*)array {
+    return (!array
+            || ![array isKindOfClass:[NSArray class]]
+            || (((NSArray *)array).count == 0));
+}
+
++ (BOOL)isEmptyString:(NSObject*)string {
+    return (!string
+            || ![string isKindOfClass:[NSString class]]
+            || [(NSString *)string isEqualToString:@""]);
+}
+
++ (BOOL)isEmptyDictionary:(NSObject*)dict {
+    return (!dict
+            || ![dict isKindOfClass:[NSDictionary class]]
+            || (((NSDictionary *)dict).count == 0));
+}
+
++ (NSString *)stringOrEmpty:(NSString *)str {
+    NSString *string = str != nil ? str : @"";
+    return string;
+}
+
+- (BOOL)validateStringInputs:(NSMutableDictionary<NSString *, NSString *> *)inputs logs:(NSDictionary<NSString *, NSString *> *)logs {
+    NSMutableDictionary *_inputs = [inputs mutableCopy];
+    BOOL __block isValid = true;
+    // Empty user Id is valid value.
+    if (_inputs.allKeys.count > 0) {
+        if ([_inputs.allKeys containsObject:OptimizelyNotificationsUserDictionaryUserIdKey]) {
+            if ([[_inputs objectForKey:OptimizelyNotificationsUserDictionaryUserIdKey] isKindOfClass:[NSNull class]]) {
+                isValid = false;
+                if ([logs objectForKey:OptimizelyNotificationsUserDictionaryUserIdKey]) {
+                    [self.logger logMessage:[logs objectForKey:OptimizelyNotificationsUserDictionaryUserIdKey] withLevel:OptimizelyLogLevelError];
+                }
+            }
+            [_inputs removeObjectForKey:OptimizelyNotificationsUserDictionaryUserIdKey];
+        }
+    }
+    [_inputs enumerateKeysAndObjectsUsingBlock:^(id key, id value, BOOL* stop) {
+        if ([value isKindOfClass:[NSNull class]] || [value isEqualToString:@""]) {
+            if ([logs objectForKey:key]) {
+                [self.logger logMessage:[logs objectForKey:key] withLevel:OptimizelyLogLevelError];
+            }
+            isValid = false;
+        }
+    }];
+    return isValid;
+}
+
+- (id)ObjectOrNull:(id)object {
+    return object ?: [NSNull null];
+}
 @end
diff --git a/OptimizelySDKCore/OptimizelySDKCoreTests/OPTLYDecisionServiceTest.m b/OptimizelySDKCore/OptimizelySDKCoreTests/OPTLYDecisionServiceTest.m
@@ -471,7 +471,7 @@ - (void)testSetForcedVariationCalledOnInvalidVariationKey2
 - (void)testSetForcedVariationCalledOnInvalidUserId
 {
     NSString *invalidUserId = @"";
-    XCTAssertFalse([self.optimizely setForcedVariation:kExperimentNotRunningKey
+    XCTAssertTrue([self.optimizely setForcedVariation:kExperimentNotRunningKey
                                                 userId:invalidUserId
                                           variationKey:kExperimentNoAudienceVariationKey]);
 }
diff --git a/OptimizelySDKCore/OptimizelySDKCoreTests/OptimizelyTest.m b/OptimizelySDKCore/OptimizelySDKCoreTests/OptimizelyTest.m

Original file line number	Diff line number	Diff line change
`@@ -471,7 +471,7 @@ - (void)testSetForcedVariationCalledOnInvalidVariationKey2`
`471`	`471`	`- (void)testSetForcedVariationCalledOnInvalidUserId`
`472`	`472`	`{`
`473`	`473`	`NSString *invalidUserId = @"";`
`474`		`- XCTAssertFalse([self.optimizely setForcedVariation:kExperimentNotRunningKey`
	`474`	`+ XCTAssertTrue([self.optimizely setForcedVariation:kExperimentNotRunningKey`
`475`	`475`	`userId:invalidUserId`
`476`	`476`	`variationKey:kExperimentNoAudienceVariationKey]);`
`477`	`477`	`}`