fix race condition that checkin can be deleted right after fetched (#2438) (#2631)

charlotteliang · web-flow · commit bddf94e48bee · 2019-03-26T10:18:02.000-07:00
diff --git a/Example/InstanceID/Tests/FIRInstanceIDCheckinStoreTest.m b/Example/InstanceID/Tests/FIRInstanceIDCheckinStoreTest.m
@@ -45,6 +45,8 @@ - (NSString *)bundleIdentifierForKeychainAccount;
 static NSString *const kScope = @"test-scope";
 static NSString *const kSecret = @"test-secret";
 static NSString *const kToken = @"test-token";
+static NSString *const kFakeErrorDomain = @"fakeDomain";
+static const NSUInteger kFakeErrorCode = -1;
 
 static int64_t const kLastCheckinTimestamp = 123456;
 
@@ -111,7 +113,6 @@ - (void)testCheckinSaveFailsOnKeychainWriteFailure {
   FIRInstanceIDBackupExcludedPlist *checkinPlist =
       [[FIRInstanceIDBackupExcludedPlist alloc] initWithFileName:kFakeCheckinPlistName
                                                     subDirectory:kSubDirectoryName];
-
   FIRInstanceIDFakeKeychain *fakeKeychain = [[FIRInstanceIDFakeKeychain alloc] init];
   fakeKeychain.cannotWriteToKeychain = YES;
 
@@ -134,6 +135,65 @@ - (void)testCheckinSaveFailsOnKeychainWriteFailure {
   [self waitForExpectationsWithTimeout:kExpectationTimeout handler:nil];
 }
 
+- (void)testCheckinSaveFailsOnPlistWriteFailure {
+  XCTestExpectation *checkinSaveFailsExpectation =
+      [self expectationWithDescription:@"Checkin save should fail after plist write failure"];
+  FIRInstanceIDBackupExcludedPlist *checkinPlist =
+      [[FIRInstanceIDBackupExcludedPlist alloc] initWithFileName:kFakeCheckinPlistName
+                                                    subDirectory:kSubDirectoryName];
+  id plistMock = OCMPartialMock(checkinPlist);
+  NSError *error = [NSError errorWithDomain:kFakeErrorDomain code:kFakeErrorCode userInfo:nil];
+  OCMStub([plistMock writeDictionary:[OCMArg any] error:[OCMArg setTo:error]]).andReturn(NO);
+
+  FIRInstanceIDFakeKeychain *fakeKeychain = [[FIRInstanceIDFakeKeychain alloc] init];
+
+  FIRInstanceIDCheckinStore *checkinStore =
+      [[FIRInstanceIDCheckinStore alloc] initWithCheckinPlist:plistMock keychain:fakeKeychain];
+
+  __block FIRInstanceIDCheckinPreferences *preferences =
+      [[FIRInstanceIDCheckinPreferences alloc] initWithDeviceID:kAuthID secretToken:kSecret];
+  [preferences updateWithCheckinPlistContents:[[self class] newCheckinPlistPreferences]];
+  [checkinStore saveCheckinPreferences:preferences
+                               handler:^(NSError *error) {
+                                 XCTAssertNotNil(error);
+                                 XCTAssertEqual(error.code, kFakeErrorCode);
+
+                                 preferences = [checkinStore cachedCheckinPreferences];
+                                 XCTAssertNil(preferences.deviceID);
+                                 XCTAssertNil(preferences.secretToken);
+                                 XCTAssertFalse([preferences hasValidCheckinInfo]);
+                                 [checkinSaveFailsExpectation fulfill];
+                               }];
+  [self waitForExpectationsWithTimeout:kExpectationTimeout handler:nil];
+}
+
+- (void)testCheckinSaveSuccess {
+  XCTestExpectation *checkinSaveSuccessExpectation =
+      [self expectationWithDescription:@"Checkin save should succeed"];
+  FIRInstanceIDBackupExcludedPlist *checkinPlist =
+      [[FIRInstanceIDBackupExcludedPlist alloc] initWithFileName:kFakeCheckinPlistName
+                                                    subDirectory:kSubDirectoryName];
+  id plistMock = OCMPartialMock(checkinPlist);
+
+  FIRInstanceIDFakeKeychain *fakeKeychain = [[FIRInstanceIDFakeKeychain alloc] init];
+  FIRInstanceIDCheckinStore *checkinStore =
+      [[FIRInstanceIDCheckinStore alloc] initWithCheckinPlist:plistMock keychain:fakeKeychain];
+
+  __block FIRInstanceIDCheckinPreferences *preferences =
+      [[FIRInstanceIDCheckinPreferences alloc] initWithDeviceID:kAuthID secretToken:kSecret];
+  [preferences updateWithCheckinPlistContents:[[self class] newCheckinPlistPreferences]];
+  [checkinStore saveCheckinPreferences:preferences
+                               handler:^(NSError *error) {
+                                 XCTAssertNil(error);
+
+                                 preferences = [checkinStore cachedCheckinPreferences];
+                                 XCTAssertEqualObjects(preferences.deviceID, kAuthID);
+                                 XCTAssertEqualObjects(preferences.secretToken, kSecret);
+                                 [checkinSaveSuccessExpectation fulfill];
+                               }];
+  [self waitForExpectationsWithTimeout:kExpectationTimeout handler:nil];
+}
+
 // Write fake checkin data to legacy location, then test if migration worked.
 - (void)testCheckinMigrationMovesToNewLocationInKeychain {
   XCTestExpectation *checkinMigrationExpectation =
diff --git a/Firebase/InstanceID/FIRIMessageCode.h b/Firebase/InstanceID/FIRIMessageCode.h
@@ -77,6 +77,8 @@ typedef NS_ENUM(NSInteger, FIRInstanceIDMessageCode) {
   kFIRInstanceIDMessageCodeKeyPair002 = 9002,
   kFIRInstanceIDMessageCodeKeyPairMigrationError = 9004,
   kFIRInstanceIDMessageCodeKeyPairMigrationSuccess = 9005,
+  kFIRInstanceIDMessageCodeKeyPairNoLegacyKeyPair = 9006,
+
   // FIRInstanceIDKeyPairStore.m
   kFIRInstanceIDMessageCodeKeyPairStore000 = 10000,
   kFIRInstanceIDMessageCodeKeyPairStore001 = 10001,
diff --git a/Firebase/InstanceID/FIRInstanceIDCheckinService.m b/Firebase/InstanceID/FIRInstanceIDCheckinService.m
@@ -133,8 +133,9 @@ - (void)checkinWithExistingCheckin:(FIRInstanceIDCheckinPreferences *)existingCh
         // Somehow the server clock gets out of sync with the device clock.
         // Reset the last checkin timestamp in case this happens.
         if (lastCheckinTimestampMillis > currentTimestampMillis) {
-          FIRInstanceIDLoggerDebug(kFIRInstanceIDMessageCodeService002,
-                                   @"Invalid last checkin timestamp in future.");
+          FIRInstanceIDLoggerDebug(
+              kFIRInstanceIDMessageCodeService002, @"Invalid last checkin timestamp %@ in future.",
+              [NSDate dateWithTimeIntervalSince1970:lastCheckinTimestampMillis / 1000.0]);
           lastCheckinTimestampMillis = currentTimestampMillis;
         }
 
diff --git a/Firebase/InstanceID/FIRInstanceIDCheckinStore.m b/Firebase/InstanceID/FIRInstanceIDCheckinStore.m
@@ -108,8 +108,21 @@ - (void)saveCheckinPreferences:(FIRInstanceIDCheckinPreferences *)preferences
     return;
   }
 
+  // Save all other checkin preferences in a plist
+  NSError *error;
+  if (![self.plist writeDictionary:checkinPlistContents error:&error]) {
+    FIRInstanceIDLoggerDebug(kFIRInstanceIDMessageCodeCheckinStore003,
+                             @"Failed to save checkin plist contents."
+                             @"Will delete auth credentials");
+    [self.keychain removeItemsMatchingService:kFIRInstanceIDCheckinKeychainService
+                                      account:self.bundleIdentifierForKeychainAccount
+                                      handler:nil];
+    if (handler) {
+      handler(error);
+    }
+    return;
+  }
   // Save the deviceID and secret in the Keychain
-  __block BOOL shouldContinue = YES;
   if (!preferences.hasPreCachedAuthCredentials) {
     NSData *data = [checkinKeychainContent dataUsingEncoding:NSUTF8StringEncoding];
     [self.keychain setData:data
@@ -121,30 +134,15 @@ - (void)saveCheckinPreferences:(FIRInstanceIDCheckinPreferences *)preferences
                        if (handler) {
                          handler(error);
                        }
-                       shouldContinue = NO;
                        return;
                      }
+                     if (handler) {
+                       handler(nil);
+                     }
                    }];
+  } else {
+    handler(nil);
   }
-  if (!shouldContinue) {
-    return;
-  }
-
-  // Save all other checkin preferences in a plist
-  NSError *error;
-  if (![self.plist writeDictionary:checkinPlistContents error:&error]) {
-    FIRInstanceIDLoggerDebug(kFIRInstanceIDMessageCodeCheckinStore003,
-                             @"Failed to save checkin plist contents."
-                             @"Will delete auth credentials");
-    [self.keychain removeItemsMatchingService:kFIRInstanceIDCheckinKeychainService
-                                      account:self.bundleIdentifierForKeychainAccount
-                                      handler:nil];
-    if (handler) {
-      handler(error);
-    }
-    return;
-  }
-  handler(nil);
 }
 
 - (void)removeCheckinPreferencesWithHandler:(void (^)(NSError *error))handler {
diff --git a/Firebase/InstanceID/FIRInstanceIDKeyPairStore.m b/Firebase/InstanceID/FIRInstanceIDKeyPairStore.m
@@ -159,7 +159,7 @@ - (BOOL)hasCachedKeyPairs {
   if ([self cachedKeyPairWithSubtype:kFIRInstanceIDKeyPairSubType error:&error] == nil) {
     if (error) {
       FIRInstanceIDLoggerDebug(kFIRInstanceIDMessageCodeKeyPairStore000,
-                               @"Failed to get cached keyPair %@", error);
+                               @"Failed to get the cached keyPair %@", error);
     }
     error = nil;
     [self removeKeyPairCreationTimePlistWithError:&error];
@@ -316,7 +316,7 @@ + (FIRInstanceIDKeyPair *)keyPairForPrivateKeyTag:(NSString *)privateKeyTag
       *error = [NSError errorWithFIRInstanceIDErrorCode:kFIRInstanceIDErrorCodeMissingKeyPair];
     }
     FIRInstanceIDLoggerDebug(kFIRInstanceIDMessageCodeKeyPair000,
-                             @"No keypair info is retrieved with tag %@", privateKeyTag);
+                             @"No keypair info is found with tag %@", privateKeyTag);
     return nil;
   }
 
@@ -344,6 +344,8 @@ - (void)migrateKeyPairCacheIfNeededWithHandler:(void (^)(NSError *error))handler
                                             publicKeyTag:legacyPublicKeyTag
                                                    error:&error];
   if (![keyPair isValid]) {
+    FIRInstanceIDLoggerDebug(kFIRInstanceIDMessageCodeKeyPairNoLegacyKeyPair,
+                             @"There's no legacy keypair so no need to do migration.");
     if (handler) {
       handler(nil);
     }
diff --git a/Firebase/InstanceID/FIRInstanceIDKeychain.m b/Firebase/InstanceID/FIRInstanceIDKeychain.m
@@ -60,10 +60,9 @@ - (CFTypeRef)itemWithQuery:(NSDictionary *)keychainQuery {
       if (keyRef) {
         CFRelease(keyRef);
       }
-      FIRInstanceIDLoggerDebug(
-          kFIRInstanceIDKeychainReadItemError,
-          @"No info is retrieved from Keychain OSStatus: %d with the keychain query %@",
-          (int)status, keychainQuery);
+      FIRInstanceIDLoggerDebug(kFIRInstanceIDKeychainReadItemError,
+                               @"Info is not found in Keychain. OSStatus: %d. Keychain query: %@",
+                               (int)status, keychainQuery);
     }
   });
   return keyRef;
diff --git a/Firebase/InstanceID/FIRInstanceIDStore.m b/Firebase/InstanceID/FIRInstanceIDStore.m
@@ -153,11 +153,13 @@ - (void)resetCredentialsIfNeeded {
   if (oldCheckinPreferences) {
     [self.checkinStore removeCheckinPreferencesWithHandler:^(NSError *error) {
       if (!error) {
-        FIRInstanceIDLoggerDebug(kFIRInstanceIDMessageCodeStore002,
-                                 @"Removed cached checkin preferences from Keychain.");
+        FIRInstanceIDLoggerDebug(
+            kFIRInstanceIDMessageCodeStore002,
+            @"Removed cached checkin preferences from Keychain because this is a fresh install.");
       } else {
-        FIRInstanceIDLoggerError(kFIRInstanceIDMessageCodeStore003,
-                                 @"Couldn't remove cached checkin preferences. Error: %@", error);
+        FIRInstanceIDLoggerError(
+            kFIRInstanceIDMessageCodeStore003,
+            @"Couldn't remove cached checkin preferences for a fresh install. Error: %@", error);
       }
       if (oldCheckinPreferences.deviceID.length && oldCheckinPreferences.secretToken.length) {
         FIRInstanceIDLoggerDebug(kFIRInstanceIDMessageCodeStore006,
diff --git a/ZipBuilder/Sources/ZipBuilder/FrameworkBuilder.swift b/ZipBuilder/Sources/ZipBuilder/FrameworkBuilder.swift
@@ -101,7 +101,7 @@ struct FrameworkBuilder {
 //    }
 //
 //    // TODO: Figure out if we need the MD5 at all.
-      let md5 = podName
+    let md5 = podName
 //    let md5 = Shell.calculateMD5(for: podInfo.installedLocation)
 
     // Get (or create) the cache directory for storing built frameworks.

Original file line number	Diff line number	Diff line change
`@@ -159,7 +159,7 @@ - (BOOL)hasCachedKeyPairs {`
`159`	`159`	`if ([self cachedKeyPairWithSubtype:kFIRInstanceIDKeyPairSubType error:&error] == nil) {`
`160`	`160`	`if (error) {`
`161`	`161`	`FIRInstanceIDLoggerDebug(kFIRInstanceIDMessageCodeKeyPairStore000,`
`162`		`- @"Failed to get cached keyPair %@", error);`
	`162`	`+ @"Failed to get the cached keyPair %@", error);`
`163`	`163`	`}`
`164`	`164`	`error = nil;`
`165`	`165`	`[self removeKeyPairCreationTimePlistWithError:&error];`
`@@ -316,7 +316,7 @@ + (FIRInstanceIDKeyPair )keyPairForPrivateKeyTag:(NSString )privateKeyTag`
`316`	`316`	`*error = [NSError errorWithFIRInstanceIDErrorCode:kFIRInstanceIDErrorCodeMissingKeyPair];`
`317`	`317`	`}`
`318`	`318`	`FIRInstanceIDLoggerDebug(kFIRInstanceIDMessageCodeKeyPair000,`
`319`		`- @"No keypair info is retrieved with tag %@", privateKeyTag);`
	`319`	`+ @"No keypair info is found with tag %@", privateKeyTag);`
`320`	`320`	`return nil;`
`321`	`321`	`}`
`322`	`322`
`@@ -344,6 +344,8 @@ - (void)migrateKeyPairCacheIfNeededWithHandler:(void (^)(NSError *error))handler`
`344`	`344`	`publicKeyTag:legacyPublicKeyTag`
`345`	`345`	`error:&error];`
`346`	`346`	`if (![keyPair isValid]) {`
	`347`	`+ FIRInstanceIDLoggerDebug(kFIRInstanceIDMessageCodeKeyPairNoLegacyKeyPair,`
	`348`	`+ @"There's no legacy keypair so no need to do migration.");`
`347`	`349`	`if (handler) {`
`348`	`350`	`handler(nil);`
`349`	`351`	`}`
Original file line number	Diff line number	Diff line change
`@@ -60,10 +60,9 @@ - (CFTypeRef)itemWithQuery:(NSDictionary *)keychainQuery {`
`60`	`60`	`if (keyRef) {`
`61`	`61`	`CFRelease(keyRef);`
`62`	`62`	`}`
`63`		`- FIRInstanceIDLoggerDebug(`
`64`		`- kFIRInstanceIDKeychainReadItemError,`
`65`		`- @"No info is retrieved from Keychain OSStatus: %d with the keychain query %@",`
`66`		`- (int)status, keychainQuery);`
	`63`	`+ FIRInstanceIDLoggerDebug(kFIRInstanceIDKeychainReadItemError,`
	`64`	`+ @"Info is not found in Keychain. OSStatus: %d. Keychain query: %@",`
	`65`	`+ (int)status, keychainQuery);`
`67`	`66`	`}`
`68`	`67`	`});`
`69`	`68`	`return keyRef;`