Merge pull request #87 from square/federman/migration

[2.2.2] Migration should not fail because `canAccessKeychain` has been called on both Valets

Gemfile

@@ -1,3 +1,4 @@
-source 'https://rubygems.org'
-
-gem 'cocoapods', '0.39.0'
+source 'https://rubygems.org' do
+  gem 'cocoapods', '1.0.0'
+  gem 'activesupport', '~> 4.0'
+end

Valet.podspec

@@ -1,6 +1,6 @@
 Pod::Spec.new do |s|
   s.name     = 'Valet'
-  s.version  = '2.2.1'
+  s.version  = '2.2.2'
   s.license  = 'Apache License, Version 2.0'
   s.summary  = 'Valet lets you securely store data in the iOS or OS X Keychain without knowing a thing about how the Keychain works. It\'s easy. We promise.'
   s.homepage = 'https://github.com/square/Valet'

Valet/VALValet.m

@@ -24,6 +24,7 @@
 
 
 NSString * const VALMigrationErrorDomain = @"VALMigrationErrorDomain";
+NSString * const VALCanAccessKeychainCanaryKey = @"VAL_KeychainCanaryUsername";
 
 
 NSString *VALStringForAccessibility(VALAccessibility accessibility)
@@ -308,21 +309,20 @@ - (BOOL)canAccessKeychain;
 {
     __block BOOL canAccessKeychain = NO;
     VALExecuteBlockInLock(^{
-        NSString *const canaryKey = @"VAL_KeychainCanaryUsername";
         NSString *const canaryValue = @"VAL_KeychainCanaryPassword";
 
-        NSString *const retrievedCanaryValue = [self stringForKey:canaryKey];
+        NSString *const retrievedCanaryValue = [self stringForKey:VALCanAccessKeychainCanaryKey];
         if ([canaryValue isEqualToString:retrievedCanaryValue]) {
             canAccessKeychain = YES;
 
         } else {
             // Canary value can't be found. Manually add the value to see if we can pull it back out.
             NSMutableDictionary *query = [self.baseQuery mutableCopy];
-            [query addEntriesFromDictionary:[self _secItemFormatDictionaryWithKey:canaryKey]];
+            [query addEntriesFromDictionary:[self _secItemFormatDictionaryWithKey:VALCanAccessKeychainCanaryKey]];
             query[(__bridge id)kSecValueData] = [canaryValue dataUsingEncoding:NSUTF8StringEncoding];
             (void)VALAtomicSecItemAdd((__bridge CFDictionaryRef)query, NULL);
 
-            NSString *const retrievedCanaryValueAfterAdding = [self stringForKey:canaryKey];
+            NSString *const retrievedCanaryValueAfterAdding = [self stringForKey:VALCanAccessKeychainCanaryKey];
             canAccessKeychain = [canaryValue isEqualToString:retrievedCanaryValueAfterAdding];
         }
 
@@ -425,6 +425,11 @@ - (nullable NSError *)migrateObjectsMatchingQuery:(nonnull NSDictionary *)secIte
     NSMutableSet *const keysToMigrate = [NSMutableSet new];
     for (NSDictionary *const keychainEntry in queryResultWithData) {
         NSString *const key = keychainEntry[(__bridge id)kSecAttrAccount];
+        if ([key isEqualToString:VALCanAccessKeychainCanaryKey]) {
+            // We don't care about this key. Move along.
+            continue;
+        }
+
         NSData *const data = keychainEntry[(__bridge id)kSecValueData];
 
         VALCheckCondition(key.length > 0, [NSError errorWithDomain:VALMigrationErrorDomain code:VALMigrationErrorKeyInQueryResultInvalid userInfo:nil], @"Can not migrate keychain entry with no key");

ValetTests/ValetTests.m

@@ -747,6 +747,27 @@ - (void)test_migrateObjectsFromValetRemoveOnCompletion_bailsOutAndLeavesKeychain
     }
 }
 
+- (void)test_migrateObjectsFromValetRemoveOnCompletion_migratesDataSuccessfullyWhenBothValetsHavePreviouslyCalled_canAccessKeychain;
+{
+    VALValet *otherValet = [[VALValet alloc] initWithIdentifier:@"Migrate_Me_To_Valet" accessibility:VALAccessibilityAfterFirstUnlock];
+    [self.additionalValets addObject:otherValet];
+
+    NSDictionary *keyStringPairToMigrateMap = @{ @"foo" : @"bar", @"testing" : @"migration", @"is" : @"quite", @"entertaining" : @"if", @"you" : @"don't", @"screw" : @"up" };
+
+    for (NSString *const key in keyStringPairToMigrateMap) {
+        XCTAssertTrue([otherValet setString:keyStringPairToMigrateMap[key] forKey:key]);
+    }
+
+    XCTAssertTrue([self.valet canAccessKeychain]);
+    XCTAssertTrue([otherValet canAccessKeychain]);
+    XCTAssertNil([self.valet migrateObjectsFromValet:otherValet removeOnCompletion:NO]);
+
+    for (NSString *const key in keyStringPairToMigrateMap) {
+        XCTAssertEqualObjects([self.valet stringForKey:key], keyStringPairToMigrateMap[key]);
+        XCTAssertEqualObjects([otherValet stringForKey:key], keyStringPairToMigrateMap[key]);
+    }
+}
+
 #if VAL_SECURE_ENCLAVE_SDK_AVAILABLE
 - (void)test_migrateObjectsFromValetRemoveOnCompletion_migratesDataSuccessfullyWhenMigratingToSecureEnclave;
 {