Skip to content

Firebase Installations: validation of FIROptions parameters #4683

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

Merged
merged 25 commits into from
Jan 16, 2020
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
d2ea3bc
FIS: Throw exception on incomplete Firebase config.
maksymmalyhin Jan 15, 2020
2f6898b
API docs updated.
maksymmalyhin Jan 15, 2020
501af60
FirebaseInstallations bump minor version: 1.1.0
maksymmalyhin Jan 15, 2020
381f9a9
Fix tests
maksymmalyhin Jan 15, 2020
1254d1a
Text fixed
maksymmalyhin Jan 15, 2020
45208a6
Make exception message more informative
maksymmalyhin Jan 15, 2020
060740c
Merge remote-tracking branch 'origin/master' into mm/fis-config
maksymmalyhin Jan 15, 2020
832adfa
Fix Remote Config tests.
maksymmalyhin Jan 15, 2020
dac08c6
Merge branch 'master' into mm/fis-config
maksymmalyhin Jan 15, 2020
9af869a
Tests updated to use GCMSenderID if projectID is not available.
maksymmalyhin Jan 15, 2020
83afada
Use `GCMSenderID` when `projectID` is not available.
maksymmalyhin Jan 15, 2020
5a74f2e
./scripts/style.sh
maksymmalyhin Jan 15, 2020
724ce8f
Comments
maksymmalyhin Jan 15, 2020
bff6efd
GCMSenderID and projectID validation updated.
maksymmalyhin Jan 16, 2020
45d1a6a
Comment
maksymmalyhin Jan 16, 2020
43934b5
FirebaseInstallations: version bump to 1.0.1.
maksymmalyhin Jan 16, 2020
3f58525
FIS changelog
maksymmalyhin Jan 16, 2020
6fbb14d
FirebaseInstallations: version bump to 1.1.0
maksymmalyhin Jan 16, 2020
1761ed6
FIS and Core changelogs.
maksymmalyhin Jan 16, 2020
22c527d
Changelogs remove GCMSenderID
maksymmalyhin Jan 16, 2020
0fd43bf
Typo
maksymmalyhin Jan 16, 2020
e1d0cf2
Typo
maksymmalyhin Jan 16, 2020
87922d5
fix link
maksymmalyhin Jan 16, 2020
d7ef952
Revert Core changelog
maksymmalyhin Jan 16, 2020
c68b86c
FIS changelog fix.
maksymmalyhin Jan 16, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Example/InstanceID/Tests/FIRInstanceIDTest.m
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,8 @@ - (void)testSharedInstance {
// The shared instance relies on the default app being configured. Configure it.
FIROptions *options = [[FIROptions alloc] initWithGoogleAppID:kGoogleAppID
GCMSenderID:kGCMSenderID];
options.APIKey = @"api-key";
options.projectID = @"project-id";
[FIRApp configureWithName:kFIRDefaultAppName options:options];
FIRInstanceID *instanceID = [FIRInstanceID instanceID];
XCTAssertNotNil(instanceID);
Expand Down
2 changes: 2 additions & 0 deletions Example/Messaging/Tests/FIRMessagingInstanceTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ class FIRMessagingInstanceTest: XCTestCase {
// This is an example of a functional test case.
// Use XCTAssert and related functions to verify your tests produce the correct results.
let options = FirebaseOptions(googleAppID: "1:123:ios:123abc", gcmSenderID: "valid-sender-id")
options.apiKey = "api-key"
options.projectID = "project-id"
FirebaseApp.configure(options: options)
let original = Messaging.messaging()

Expand Down
2 changes: 1 addition & 1 deletion FirebaseInstallations.podspec
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
Pod::Spec.new do |s|
s.name = 'FirebaseInstallations'
s.version = '1.0.0'
s.version = '1.1.0'
s.summary = 'Firebase Installations for iOS'

s.description = <<-DESC
Expand Down
34 changes: 32 additions & 2 deletions FirebaseInstallations/Source/Library/FIRInstallations.m
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
#import "FIRInstallationsErrorUtil.h"
#import "FIRInstallationsIDController.h"
#import "FIRInstallationsItem.h"
#import "FIRInstallationsLogger.h"
#import "FIRInstallationsStoredAuthToken.h"
#import "FIRInstallationsVersion.h"

Expand Down Expand Up @@ -101,6 +102,7 @@ - (instancetype)initWithAppOptions:(FIROptions *)appOptions
prefetchAuthToken:(BOOL)prefetchAuthToken {
self = [super init];
if (self) {
[[self class] validateAppOptions:appOptions appName:appName];
[[self class] assertCompatibleIIDVersion];

_appOptions = [appOptions copy];
Expand All @@ -117,12 +119,40 @@ - (instancetype)initWithAppOptions:(FIROptions *)appOptions
return self;
}

+ (void)validateAppOptions:(FIROptions *)appOptions appName:(NSString *)appName {
NSMutableArray *missingFields = [NSMutableArray array];
if (appName.length < 1) {
[missingFields addObject:@"`FirebaseApp.name`"];
}
if (appOptions.APIKey.length < 1) {
[missingFields addObject:@"`FirebaseOptions.APIKey`"];
}
if (appOptions.googleAppID.length < 1) {
[missingFields addObject:@"`FirebaseOptions.googleAppID`"];
}
if (appOptions.GCMSenderID.length < 1) {
[missingFields addObject:@"`FirebaseOptions.GCMSenderID`"];
}

if (missingFields.count > 0) {
[NSException
raise:kFirebaseInstallationsErrorDomain
format:@"%@[%@] Could not configure Firebase Installations due to invalid FirebaseApp "
@"options. The following parameters are nil or empty: %@. If you use "
@"GoogleServices-Info.plist please download the most recent version from Firebase "
@"Console. If you configure Firebase in code, please make sure you specify all "
@"required parameters.",
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend that we list the required parameters:
"APIKey and projectID"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The list of the missing parameters will be added instead of %@ at the string "...The following parameters are nil or empty: %@..." from missingFields

kFIRLoggerInstallations, kFIRInstallationsMessageCodeInvalidFirebaseAppOptions,
[missingFields componentsJoinedByString:@", "]];
}
}

#pragma mark - Public

+ (FIRInstallations *)installations {
FIRApp *defaultApp = [FIRApp defaultApp];
if (!defaultApp) {
[NSException raise:NSInternalInconsistencyException
[NSException raise:kFirebaseInstallationsErrorDomain
format:@"The default FirebaseApp instance must be configured before the default"
@"FirebaseApp instance can be initialized. One way to ensure that is to "
@"call `[FIRApp configure];` (`FirebaseApp.configure()` in Swift) in the App"
Expand Down Expand Up @@ -191,7 +221,7 @@ + (void)assertCompatibleIIDVersion {
return;
#else
if (![self isIIDVersionCompatible]) {
[NSException raise:NSInternalInconsistencyException
[NSException raise:kFirebaseInstallationsErrorDomain

Choose a reason for hiding this comment

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

I am kind of lost with this message and I have no idea what to do here. I have downloaded the info.plist again, I have checked your other PR, I have checked the release notes, I have also look into the code and still no idea, so this is kind of my last resort 😅.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tiagoalmeida sounds like you are having issue with the Firebase update. I am sorry to hear if it is so. Would you like to create a ticket with a description of your issue so we can track all related details there and potentially help other people having similar problems?

format:@"FirebaseInstallations will not work correctly with current version of "
@"Firebase Instance ID. Please update your Firebase Instance ID version."];
}
Expand Down
3 changes: 3 additions & 0 deletions FirebaseInstallations/Source/Library/FIRInstallationsLogger.h
Original file line number Diff line number Diff line change
Expand Up @@ -46,3 +46,6 @@ extern NSString *const kFIRInstallationsMessageCodeAuthTokenCoderVersionMismatch
// FIRInstallationsStoredIIDCheckin.m
extern NSString *const kFIRInstallationsMessageCodeIIDCheckinCoderVersionMismatch;
extern NSString *const kFIRInstallationsMessageCodeIIDCheckinFailedToDecode;

// FIRInstallations.m
extern NSString *const kFIRInstallationsMessageCodeInvalidFirebaseAppOptions;
3 changes: 3 additions & 0 deletions FirebaseInstallations/Source/Library/FIRInstallationsLogger.m
Original file line number Diff line number Diff line change
Expand Up @@ -44,3 +44,6 @@
// FIRInstallationsStoredIIDCheckin.m
NSString *const kFIRInstallationsMessageCodeIIDCheckinCoderVersionMismatch = @"I-FIS007000";
NSString *const kFIRInstallationsMessageCodeIIDCheckinFailedToDecode = @"I-FIS007001";

// FIRInstallations.m
NSString *const kFIRInstallationsMessageCodeInvalidFirebaseAppOptions = @"I-FIS008000";
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ NS_ASSUME_NONNULL_BEGIN
APIKey:(NSString *)APIKey
projectID:(NSString *)projectID
GCMSenderID:(NSString *)GCMSenderID
accessGroup:(NSString *)accessGroup;
accessGroup:(nullable NSString *)accessGroup;

- (FBLPromise<FIRInstallationsItem *> *)getInstallationItem;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,12 @@ - (instancetype)initWithGoogleAppID:(NSString *)appID
FIRSecureStorage *secureStorage = [[FIRSecureStorage alloc] init];
FIRInstallationsStore *installationsStore =
[[FIRInstallationsStore alloc] initWithSecureStorage:secureStorage accessGroup:accessGroup];

// Use `GCMSenderID` as `projectID` is not available.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change to:
"User GCMSenderID as project identifier, if projectID is not set"
(or "is not available")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

NSString *APIServiceProjectID = (projectID.length > 0) ? projectID : GCMSenderID;
FIRInstallationsAPIService *apiService =
[[FIRInstallationsAPIService alloc] initWithAPIKey:APIKey projectID:projectID];
[[FIRInstallationsAPIService alloc] initWithAPIKey:APIKey projectID:APIServiceProjectID];

FIRInstallationsIIDStore *IIDStore = [[FIRInstallationsIIDStore alloc] init];
FIRInstallationsIIDTokenStore *IIDCheckingStore =
[[FIRInstallationsIIDTokenStore alloc] initWithGCMSenderID:GCMSenderID];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,15 +59,17 @@ NS_SWIFT_NAME(Installations)

/**
* Returns a default instance of `Installations`.
* @return Returns an instance of `Installations` for `FirebaseApp.defaultApp(). Throws an exception
* if the default app is not configured yet.
* @return Returns an instance of `Installations` for `FirebaseApp.defaultApp().
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend removing the word "Returns"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Usage of @return is a bit inconsistent in Firebase iOS SDK, but I can find more without first Returns. I'll update it.

cc @ryanwilson

* @throw Throws an exception if the default app is not configured yet or required `FirebaseApp`
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend removing the word "Throws".
Also I recommend replacing the words "an exception" by the Exception type.
In JavaDoc the first word after @throw is expected to be a fully qualified class name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good recommendation, but I'll probably keep it as is to be consistent with the rest of the AppleDocs in the repo. Objective C exceptions are not handled usually, so the exception type is not so important and is mostly informative for a crash report reader. Because of that we use just an SDK domain for most of exceptions.

@ryanwilson do you have an opinion on it?

Copy link
Member

Choose a reason for hiding this comment

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

Keeping it like this is fine I think - when rendered it's just a text field so having it start with "Throws" is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, thanks for the explanation.
I quickly googles Objective C documentation and apparently there is no @throw keyword, but rather a @warning keyword. How about using that instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@throw renders well for me:

Screen Shot 2020-01-16 at 10 36 33 AM

* options are missing.
*/
+ (FIRInstallations *)installations NS_SWIFT_NAME(installations());

/**
* Returns an instance of `Installations` for an application.
* @param application A configured `FirebaseApp` instance.
* @return Returns an instance of `Installations` corresponding to the passed application.
* @throw Throws an exception if required `FirebaseApp` options are missing.
Copy link
Contributor

Choose a reason for hiding this comment

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

as above

*/
+ (FIRInstallations *)installationsWithApp:(FIRApp *)application NS_SWIFT_NAME(installations(app:));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,8 @@ - (FIRApp *)createAndConfigureAppWithName:(NSString *)name {
FIROptions *options =
[[FIROptions alloc] initWithGoogleAppID:@"1:100000000000:ios:aaaaaaaaaaaaaaaaaaaaaaaa"
GCMSenderID:@"valid_sender_id"];
options.APIKey = @"some_api_key";
options.projectID = @"project_id";
[FIRApp configureWithName:name options:options];

return [FIRApp appNamed:name];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,46 @@ - (void)tearDown {
self.appName = nil;
}

#pragma mark - Initialization

- (void)testInitWhenProjectIDSetThenItIsPassedToAPIService {
NSString *APIKey = @"api-key";
NSString *projectID = @"project-id";
OCMExpect([self.mockAPIService alloc]).andReturn(self.mockAPIService);
OCMExpect([self.mockAPIService initWithAPIKey:APIKey projectID:projectID])
.andReturn(self.mockAPIService);

FIRInstallationsIDController *controller =
[[FIRInstallationsIDController alloc] initWithGoogleAppID:@"app-id"
appName:@"app-name"
APIKey:APIKey
projectID:projectID
GCMSenderID:@"sender-id"
accessGroup:nil];
XCTAssertNotNil(controller);

OCMVerifyAll(self.mockAPIService);
}

- (void)testInitWhenProjectIDIsNilThenGCMSenderIDIsPassedToAPIServiceAsProjectID {
NSString *APIKey = @"api-key";
NSString *GCMSenderID = @"sender-id";
OCMExpect([self.mockAPIService alloc]).andReturn(self.mockAPIService);
OCMExpect([self.mockAPIService initWithAPIKey:APIKey projectID:GCMSenderID])
.andReturn(self.mockAPIService);

FIRInstallationsIDController *controller =
[[FIRInstallationsIDController alloc] initWithGoogleAppID:@"app-id"
appName:@"app-name"
APIKey:APIKey
projectID:@""
GCMSenderID:GCMSenderID
accessGroup:nil];
XCTAssertNotNil(controller);

OCMVerifyAll(self.mockAPIService);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't find a test that fails when no GCMSenderID and no projectID are set...
And a test that fails when API-Key is missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are here


#pragma mark - Get Installation

- (void)testGetInstallationItem_WhenFIDExists_ThenItIsReturned {
Expand Down Expand Up @@ -1134,4 +1174,8 @@ - (FIRInstallationsItem *)expectAuthTokenRefreshForInstallation:
return responseInstallation;
}

- (void)expectAPIServiceInitWithAPIKey:(NSString *)APIKey projectID:(NSString *)projectID {
OCMExpect([self.mockAPIService initWithAPIKey:APIKey projectID:projectID]);
}

@end
56 changes: 56 additions & 0 deletions FirebaseInstallations/Source/Tests/Unit/FIRInstallationsTests.m
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ - (void)setUp {

self.appOptions = [[FIROptions alloc] initWithGoogleAppID:@"GoogleAppID"
GCMSenderID:@"GCMSenderID"];
self.appOptions.APIKey = @"APIKey";
self.appOptions.projectID = @"ProjectID";

self.mockIDController = OCMClassMock([FIRInstallationsIDController class]);
self.installations = [[FIRInstallations alloc] initWithAppOptions:self.appOptions
appName:@"appName"
Expand Down Expand Up @@ -234,4 +237,57 @@ - (void)testDeleteError {
[self waitForExpectations:@[ deleteExpectation ] timeout:0.5];
}

#pragma mark - Invalid Firebase configuration

- (void)testInitWhenProjectIDMissingThenNoThrow {
FIROptions *options = [self.appOptions copy];
options.projectID = nil;
XCTAssertNoThrow([self createInstallationsWithAppOptions:options appName:@"missingProjectID"]);

options.projectID = @"";
XCTAssertNoThrow([self createInstallationsWithAppOptions:options appName:@"emptyProjectID"]);
}

- (void)testInitWhenAPIKeyMissingThenThrows {
FIROptions *options = [self.appOptions copy];
options.APIKey = nil;
XCTAssertThrows([self createInstallationsWithAppOptions:options appName:@"missingAPIKey"]);

options.APIKey = @"";
XCTAssertThrows([self createInstallationsWithAppOptions:options appName:@"emptyAPIKey"]);
}

- (void)testInitWhenGoogleAppIDMissingThenThrows {
FIROptions *options = [self.appOptions copy];
options.googleAppID = @"";
XCTAssertThrows([self createInstallationsWithAppOptions:options appName:@"emptyGoogleAppID"]);
}

- (void)testInitWhenGCMSenderIDMissingThenThrows {
FIROptions *options = [self.appOptions copy];
options.GCMSenderID = @"";
XCTAssertThrows([self createInstallationsWithAppOptions:options appName:@"emptyGCMSenderID"]);
}

- (void)testInitWhenAppNameMissingThenThrows {
FIROptions *options = [self.appOptions copy];
XCTAssertThrows([self createInstallationsWithAppOptions:options appName:@""]);
XCTAssertThrows([self createInstallationsWithAppOptions:options appName:nil]);
}

- (void)testInitWhenAppOptionsMissingThenThrows {
XCTAssertThrows([self createInstallationsWithAppOptions:nil appName:@"missingOptions"]);
}

#pragma mark - Helpers

- (FIRInstallations *)createInstallationsWithAppOptions:(FIROptions *)options
appName:(NSString *)appName {
id mockIDController = OCMClassMock([FIRInstallationsIDController class]);
return [[FIRInstallations alloc] initWithAppOptions:options
appName:appName
installationsIDController:mockIDController
prefetchAuthToken:NO];
}

@end
Original file line number Diff line number Diff line change
Expand Up @@ -173,8 +173,11 @@ - (void)testThrowsWithNilGCMSenderID {
#pragma mark - Helpers

- (FIROptions *)fakeOptions {
return [[FIROptions alloc] initWithGoogleAppID:@"1:123:ios:123abc"
GCMSenderID:@"correct_gcm_sender_id"];
FIROptions *options = [[FIROptions alloc] initWithGoogleAppID:@"1:123:ios:123abc"
GCMSenderID:@"correct_gcm_sender_id"];
options.APIKey = @"api-key";
options.projectID = @"project-id";
return options;
}

- (NSString *)generatedTestAppName {
Expand Down