Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add global data collection switch. #1219

Merged
merged 4 commits into from May 14, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
130 changes: 130 additions & 0 deletions Example/Core/Tests/FIRAppTest.m
Expand Up @@ -42,6 +42,9 @@ + (BOOL)validateAppID:(NSString *)appID;
+ (BOOL)validateAppIDFormat:(NSString *)appID withVersion:(NSString *)version;
+ (BOOL)validateAppIDFingerprint:(NSString *)appID withVersion:(NSString *)version;

+ (nullable NSNumber *)readDataCollectionSwitchFromPlist;
+ (nullable NSNumber *)readDataCollectionSwitchFromUserDefaultsForApp:(FIRApp *)app;

@end

@interface FIRAppTest : FIRTestCase
Expand Down Expand Up @@ -552,6 +555,133 @@ - (void)testAppIDFingerprintInvalid {
[FIRApp validateAppIDFingerprint:@"1:1337:ios:deadbeef:ab" withVersion:kGoodVersionV1]);
}

#pragma mark - Automatic Data Collection Tests

- (void)testGlobalDataCollectionNoFlags {
// Test: No flags set.
[FIRApp configure];
OCMStub([self.appClassMock readDataCollectionSwitchFromPlist]).andReturn(nil);
OCMStub([self.appClassMock readDataCollectionSwitchFromUserDefaultsForApp:OCMOCK_ANY])
.andReturn(nil);

XCTAssertTrue([FIRApp defaultApp].isAutomaticDataCollectionEnabled);
}

- (void)testGlobalDataCollectionPlistSetEnabled {
// Test: Plist set to enabled, no override.
[FIRApp configure];
OCMStub([self.appClassMock readDataCollectionSwitchFromPlist]).andReturn(@YES);
OCMStub([self.appClassMock readDataCollectionSwitchFromUserDefaultsForApp:OCMOCK_ANY])
.andReturn(nil);

XCTAssertTrue([FIRApp defaultApp].isAutomaticDataCollectionEnabled);
}

- (void)testGlobalDataCollectionPlistSetDisabled {
// Test: Plist set to disabled, no override.
[FIRApp configure];
OCMStub([self.appClassMock readDataCollectionSwitchFromPlist]).andReturn(@NO);
OCMStub([self.appClassMock readDataCollectionSwitchFromUserDefaultsForApp:OCMOCK_ANY])
.andReturn(nil);

XCTAssertFalse([FIRApp defaultApp].isAutomaticDataCollectionEnabled);
}

- (void)testGlobalDataCollectionUserSpecifiedEnabled {
// Test: User specified as enabled, no plist value.
[FIRApp configure];
OCMStub([self.appClassMock readDataCollectionSwitchFromPlist]).andReturn(nil);
OCMStub([self.appClassMock readDataCollectionSwitchFromUserDefaultsForApp:OCMOCK_ANY])
.andReturn(@YES);

XCTAssertTrue([FIRApp defaultApp].isAutomaticDataCollectionEnabled);
}

- (void)testGlobalDataCollectionUserSpecifiedDisabled {
// Test: User specified as disabled, no plist value.
[FIRApp configure];
OCMStub([self.appClassMock readDataCollectionSwitchFromPlist]).andReturn(nil);
OCMStub([self.appClassMock readDataCollectionSwitchFromUserDefaultsForApp:OCMOCK_ANY])
.andReturn(@NO);

XCTAssertFalse([FIRApp defaultApp].isAutomaticDataCollectionEnabled);
}

- (void)testGlobalDataCollectionUserOverriddenEnabled {
// Test: User specified as enabled, with plist set as disabled.
[FIRApp configure];
OCMStub([self.appClassMock readDataCollectionSwitchFromPlist]).andReturn(@NO);
OCMStub([self.appClassMock readDataCollectionSwitchFromUserDefaultsForApp:OCMOCK_ANY])
.andReturn(@YES);

XCTAssertTrue([FIRApp defaultApp].isAutomaticDataCollectionEnabled);
}

- (void)testGlobalDataCollectionUserOverriddenDisabled {
// Test: User specified as disabled, with plist set as enabled.
[FIRApp configure];
OCMStub([self.appClassMock readDataCollectionSwitchFromPlist]).andReturn(@YES);
OCMStub([self.appClassMock readDataCollectionSwitchFromUserDefaultsForApp:OCMOCK_ANY])
.andReturn(@NO);

XCTAssertFalse([FIRApp defaultApp].isAutomaticDataCollectionEnabled);
}

- (void)testGlobalDataCollectionWriteToDefaults {
id defaultsMock = OCMPartialMock([NSUserDefaults standardUserDefaults]);
[FIRApp configure];

FIRApp *app = [FIRApp defaultApp];
app.automaticDataCollectionEnabled = YES;
NSString *key =
[NSString stringWithFormat:kFIRGlobalAppDataCollectionEnabledDefaultsKeyFormat, app.name];
OCMVerify([defaultsMock setObject:@YES forKey:key]);

[FIRApp defaultApp].automaticDataCollectionEnabled = NO;
OCMVerify([defaultsMock setObject:@NO forKey:key]);

[defaultsMock stopMocking];
}

- (void)testGlobalDataCollectionClearedAfterDelete {
// Configure and disable data collection for the default FIRApp.
[FIRApp configure];
FIRApp *app = [FIRApp defaultApp];
app.automaticDataCollectionEnabled = NO;
XCTAssertFalse(app.isAutomaticDataCollectionEnabled);

// Delete the app, and verify that the switch was reset.
XCTestExpectation *deleteFinished =
[self expectationWithDescription:@"The app should successfully delete."];
[app deleteApp:^(BOOL success) {
if (success) {
[deleteFinished fulfill];
}
}];

// Wait for the delete to complete.
[self waitForExpectations:@[ deleteFinished ] timeout:1];

// Set up the default app again, and check the data collection flag.
[FIRApp configure];
XCTAssertTrue([FIRApp defaultApp].isAutomaticDataCollectionEnabled);
}

- (void)testGlobalDataCollectionNoDiagnosticsSent {
[FIRApp configure];

// Stub out reading from user defaults since stubbing out the BOOL has issues. If the data
// collection switch is disabled, the `sendLogs` call should return immediately and not fire a
// notification.
OCMStub([self.appClassMock readDataCollectionSwitchFromUserDefaultsForApp:OCMOCK_ANY])
.andReturn(@NO);
OCMReject([self.notificationCenterMock postNotificationName:kFIRAppDiagnosticsNotification
object:OCMOCK_ANY
userInfo:OCMOCK_ANY]);
NSError *error = [NSError errorWithDomain:@"com.firebase" code:42 userInfo:nil];
[[FIRApp defaultApp] sendLogsWithServiceName:@"Service" version:@"Version" error:error];
}

#pragma mark - Internal Methods

- (void)testAuthGetUID {
Expand Down
40 changes: 40 additions & 0 deletions Example/Messaging/Tests/FIRMessagingTest.m
Expand Up @@ -75,6 +75,46 @@ - (void)testAutoInitEnableFlag {
XCTAssertTrue(_messaging.isAutoInitEnabled);
}

- (void)testAutoInitEnableFlagOverrideGlobalTrue {
OCMStub([self.mockMessaging isGlobalAutomaticDataCollectionEnabled]).andReturn(YES);
id bundleMock = OCMPartialMock([NSBundle mainBundle]);
OCMStub([bundleMock objectForInfoDictionaryKey:kFIRMessagingPlistAutoInitEnabled]).andReturn(nil);
XCTAssertTrue(self.messaging.isAutoInitEnabled);

self.messaging.autoInitEnabled = NO;
XCTAssertFalse(self.messaging.isAutoInitEnabled);
[bundleMock stopMocking];
}

- (void)testAutoInitEnableFlagOverrideGlobalFalse {
OCMStub([self.mockMessaging isGlobalAutomaticDataCollectionEnabled]).andReturn(YES);
id bundleMock = OCMPartialMock([NSBundle mainBundle]);
OCMStub([bundleMock objectForInfoDictionaryKey:kFIRMessagingPlistAutoInitEnabled]).andReturn(nil);
XCTAssertTrue(self.messaging.isAutoInitEnabled);

self.messaging.autoInitEnabled = NO;
XCTAssertFalse(self.messaging.isAutoInitEnabled);
[bundleMock stopMocking];
}

- (void)testAutoInitEnableGlobalDefaultTrue {
OCMStub([self.mockMessaging isGlobalAutomaticDataCollectionEnabled]).andReturn(YES);
id bundleMock = OCMPartialMock([NSBundle mainBundle]);
OCMStub([bundleMock objectForInfoDictionaryKey:kFIRMessagingPlistAutoInitEnabled]).andReturn(nil);

XCTAssertTrue(self.messaging.isAutoInitEnabled);
[bundleMock stopMocking];
}

- (void)testAutoInitEnableGlobalDefaultFalse {
OCMStub([self.mockMessaging isGlobalAutomaticDataCollectionEnabled]).andReturn(NO);
id bundleMock = OCMPartialMock([NSBundle mainBundle]);
OCMStub([bundleMock objectForInfoDictionaryKey:kFIRMessagingPlistAutoInitEnabled]).andReturn(nil);

XCTAssertFalse(self.messaging.isAutoInitEnabled);
[bundleMock stopMocking];
}

#pragma mark - Direct Channel Establishment Testing

// Should connect with valid token and application in foreground
Expand Down
85 changes: 84 additions & 1 deletion Firebase/Core/FIRApp.m
Expand Up @@ -46,6 +46,11 @@
NSString *const kFIRAppNameKey = @"FIRAppNameKey";
NSString *const kFIRGoogleAppIDKey = @"FIRGoogleAppIDKey";

NSString *const kFIRGlobalAppDataCollectionEnabledDefaultsKeyFormat =
@"/google/firebase/global_data_collection_enabled:%@";
NSString *const kFIRGlobalAppDataCollectionEnabledPlistKey =
@"FirebaseAutomaticDataCollectionEnabled";

NSString *const kFIRAppDiagnosticsNotification = @"FIRAppDiagnosticsNotification";

NSString *const kFIRAppDiagnosticsConfigurationTypeKey = @"ConfigType";
Expand Down Expand Up @@ -227,6 +232,7 @@ - (void)deleteApp:(FIRAppVoidBoolCallback)completion {
if (sAllApps && sAllApps[self.name]) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.name can be nil. We should check it before accessing the dictionary.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We throw an exception if a user tries to configure the app with a nil or empty name, which should catch this. If you're still concerned, we can add it in a separate PR since it's unrelated to this change.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. I think we should be careful of accessing dictionary when the provided key is nil. I'm concerned that self.name might be nil at some point during the entire app life cycle.

FIRLogDebug(kFIRLoggerCore, @"I-COR000006", @"Deleting app named %@", self.name);
[sAllApps removeObjectForKey:self.name];
[self clearDataCollectionSwitchFromUserDefaults];
if ([self.name isEqualToString:kFIRDefaultAppName]) {
sDefaultApp = nil;
}
Expand Down Expand Up @@ -332,6 +338,30 @@ - (FIROptions *)options {
return [_options copy];
}

- (void)setAutomaticDataCollectionEnabled:(BOOL)automaticDataCollectionEnabled {
NSString *key =
[NSString stringWithFormat:kFIRGlobalAppDataCollectionEnabledDefaultsKeyFormat, self.name];
[[NSUserDefaults standardUserDefaults] setBool:automaticDataCollectionEnabled forKey:key];
}

- (BOOL)isAutomaticDataCollectionEnabled {
// Check if it's been manually set before in code, and use that as the higher priority value.
NSNumber *defaultsObject = [[self class] readDataCollectionSwitchFromUserDefaultsForApp:self];
if (defaultsObject) {
return [defaultsObject boolValue];
}

// Read the Info.plist to see if the flag is set. If it's not set, it should default to `YES`.
// As per the implementation of `readDataCollectionSwitchFromPlist`, it's a cached value and has
// no performance impact calling multiple times.
NSNumber *collectionEnabledPlistValue = [[self class] readDataCollectionSwitchFromPlist];
if (collectionEnabledPlistValue) {
return [collectionEnabledPlistValue boolValue];
} else {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: no need for else

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

return YES;
}
}

#pragma mark - private

+ (void)sendNotificationsToSDKs:(FIRApp *)app {
Expand Down Expand Up @@ -613,11 +643,64 @@ - (NSString *)expectedBundleID {
}

// end App ID validation
#pragma mark

#pragma mark - Reading From Plist & User Defaults

/**
* A wrapper to clear the data collection switch from the standard NSUserDefaults for easier testing
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Clears the data collection ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

* and readability.
*/
- (void)clearDataCollectionSwitchFromUserDefaults {
NSString *key =
[NSString stringWithFormat:kFIRGlobalAppDataCollectionEnabledDefaultsKeyFormat, self.name];
[[NSUserDefaults standardUserDefaults] removeObjectForKey:key];
}

/**
* A wrapper to read the data collection switch from the standard NSUserDefaults for easier testing
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Reads the data collection...
Please remove "A wrapper to" here and below

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

* and readability.
*/
+ (nullable NSNumber *)readDataCollectionSwitchFromUserDefaultsForApp:(FIRApp *)app {
// Read the object in user defaults, and only return if it's an NSNumber.
NSString *key =
[NSString stringWithFormat:kFIRGlobalAppDataCollectionEnabledDefaultsKeyFormat, app.name];
id collectionEnabledDefaultsObject = [[NSUserDefaults standardUserDefaults] objectForKey:key];
if ([collectionEnabledDefaultsObject isKindOfClass:[NSNumber class]]) {
return collectionEnabledDefaultsObject;
} else {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: no need for else

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

return nil;
}
}

/**
* A wrapper to read the data collection switch from the Info.plist for easier testing and
* readability. Will only read once from the plist and return the cached value.
*/
+ (nullable NSNumber *)readDataCollectionSwitchFromPlist {
static NSNumber *collectionEnabledPlistObject;
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
// Read the data from the `Info.plist`, only assign it if it's there and an NSNumber.
id plistValue = [[NSBundle mainBundle]
objectForInfoDictionaryKey:kFIRGlobalAppDataCollectionEnabledPlistKey];
if (plistValue && [plistValue isKindOfClass:[NSNumber class]]) {
collectionEnabledPlistObject = (NSNumber *)plistValue;
}
});

return collectionEnabledPlistObject;
}

#pragma mark - Sending Logs

- (void)sendLogsWithServiceName:(NSString *)serviceName
version:(NSString *)version
error:(NSError *)error {
// If the user has manually turned off data collection, return and don't send logs.
if (![self isAutomaticDataCollectionEnabled]) {
return;
}

NSMutableDictionary *userInfo = [[NSMutableDictionary alloc] initWithDictionary:@{
kFIRAppDiagnosticsConfigurationTypeKey : @(FIRConfigTypeSDK),
kFIRAppDiagnosticsSDKNameKey : serviceName,
Expand Down
28 changes: 28 additions & 0 deletions Firebase/Core/Private/FIRAppInternal.h
Expand Up @@ -60,6 +60,22 @@ extern NSString *const kFIRAppIsDefaultAppKey;
extern NSString *const kFIRAppNameKey;
extern NSString *const kFIRGoogleAppIDKey;

/**
* The format string for the User Defaults key used for storing the data collection enabled flag.
* This includes formatting to append the Firebase App's name.
*/
extern NSString *const kFIRGlobalAppDataCollectionEnabledDefaultsKeyFormat;

/**
* The plist key used for storing the data collection enabled flag.
*/
extern NSString *const kFIRGlobalAppDataCollectionEnabledPlistKey;

/**
* A notification fired containing diagnostic information when SDK errors occur.
*/
extern NSString *const kFIRAppDiagnosticsNotification;

/** @var FIRAuthStateDidChangeInternalNotification
@brief The name of the @c NSNotificationCenter notification which is posted when the auth state
changes (e.g. a new token has been produced, a user logs in or out). The object parameter of
Expand Down Expand Up @@ -181,6 +197,18 @@ typedef NSString *_Nullable (^FIRAppGetUIDImplementation)(void);
*/
- (nullable NSString *)getUID;

/**
* WARNING: THIS SETTING DOES NOT WORK YET. IT WILL BE MOVED TO THE PUBLIC HEADER ONCE ALL SDKS
* CONFORM TO THIS PREFERENCE. DO NOT RELY ON IT.
*
* Gets or sets whether automatic data collection is enabled for all products.
* Defaults to `YES` unless `FirebaseAutomaticDataCollectionEnabled` is set to `NO` in
* your app's Info.plist. This value is persisted across runs of the app so that it
* can be set once when users have consented to collection.
*/
@property(nonatomic, readwrite, getter=isAutomaticDataCollectionEnabled)
BOOL automaticDataCollectionEnabled;

@end

NS_ASSUME_NONNULL_END
1 change: 1 addition & 0 deletions Firebase/Messaging/FIRMessaging+FIRApp.m
Expand Up @@ -72,6 +72,7 @@ - (void)configureMessaging:(FIRApp *)app {
}

self.fcmSenderID = [options.GCMSenderID copy];
self.globalAutomaticDataCollectionEnabled = [app isAutomaticDataCollectionEnabled];

// Swizzle remote-notification-related methods (app delegate and UNUserNotificationCenter)
if ([FIRMessagingRemoteNotificationsProxy canSwizzleMethods]) {
Expand Down
7 changes: 4 additions & 3 deletions Firebase/Messaging/FIRMessaging.m
Expand Up @@ -75,7 +75,7 @@

NSString *const kFIRMessagingAPNSTokenType = @"APNSTokenType"; // APNS Token type key stored in user info.

static NSString *const kFIRMessagingPlistAutoInitEnabled =
NSString *const kFIRMessagingPlistAutoInitEnabled =
@"FirebaseMessagingAutoInitEnabled"; // Auto Init Enabled key stored in Info.plist

@interface FIRMessagingMessageInfo ()
Expand Down Expand Up @@ -471,8 +471,9 @@ - (BOOL)isAutoInitEnabled {
if (isAutoInitEnabledObject) {
return [isAutoInitEnabledObject boolValue];
}
// If none of above exists, we default assume FCM auto init is enabled.
return YES;

// If none of above exists, we default to the global switch that comes from FIRApp.
return self.isGlobalAutomaticDataCollectionEnabled;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before we make sure it's default true if no special action happened on the user side, so we make sure we are not breaking app's existing behavior on fetching instanceID, and other activities.

Is this global boolean value also serves the same purpose, that if developer doesn't do anything, it will be default ture?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, if nothing is set in the plist or at runtime, the default is true and we have unit tests to confirm that.

}

- (void)setAutoInitEnabled:(BOOL)autoInitEnabled {
Expand Down
4 changes: 4 additions & 0 deletions Firebase/Messaging/FIRMessaging_Private.h
Expand Up @@ -25,6 +25,7 @@ typedef NS_ENUM(int8_t, FIRMessagingNetworkStatus) {
kFIRMessagingReachabilityReachableViaWWAN,
};

FOUNDATION_EXPORT NSString *const kFIRMessagingPlistAutoInitEnabled;
FOUNDATION_EXPORT NSString *const kFIRMessagingUserDefaultsKeyAutoInitEnabled;

@interface FIRMessagingRemoteMessage ()
Expand All @@ -37,6 +38,9 @@ FOUNDATION_EXPORT NSString *const kFIRMessagingUserDefaultsKeyAutoInitEnabled;

#pragma mark - Private API

// The data collection flag from Core.
@property(nonatomic, readwrite, getter=isGlobalAutomaticDataCollectionEnabled) BOOL globalAutomaticDataCollectionEnabled;

- (NSString *)defaultFcmToken;
- (FIRMessagingClient *)client;
- (FIRMessagingPubSub *)pubsub;
Expand Down