Fix bug where multiple keychain entries would result in user persistence failure (#5930)

* Fix bug where multiple keychain entries would result in user persistence failure

* Fix missing header

* Fix tests

* fix tests on macOS

* Always return non-legacy keychain value

* Fix thing

* Fix missing type

* fix missing bridge call

Author: morganchen12

FirebaseAuth/Sources/Storage/FIRAuthKeychainServices.m

@@ -18,6 +18,8 @@
 
 #import <Security/Security.h>
 
+#import "FirebaseCore/Sources/Private/FirebaseCoreInternal.h"
+
 #import "FirebaseAuth/Sources/Storage/FIRAuthUserDefaults.h"
 #import "FirebaseAuth/Sources/Utilities/FIRAuthErrorUtils.h"
 
@@ -129,18 +131,34 @@ - (nullable NSData *)itemWithQuery:(NSDictionary *)query error:(NSError **_Nulla
 
   if (status == noErr && result != NULL) {
     NSArray *items = (__bridge_transfer NSArray *)result;
-    if (items.count != 1) {
+    if (items.count == 0) {
       if (error) {
+        // The keychain query returned no error, but there were no items found.
         *error = [FIRAuthErrorUtils keychainErrorWithFunction:@"SecItemCopyMatching" status:status];
       }
       return nil;
+    } else if (items.count > 1) {
+      // More than one keychain item was found, all but the first will be ignored.
+      FIRLogWarning(
+          kFIRLoggerAuth, @"I-AUT000005",
+          @"Keychain query returned multiple results, all but the first will be ignored: %@",
+          items);
     }
 
     if (error) {
       *error = nil;
     }
-    NSDictionary *item = items[0];
-    return item[(__bridge id)kSecValueData];
+    // Return the non-legacy item.
+    for (NSDictionary *item in items) {
+      if (item[(__bridge NSString *)kSecAttrService] != nil) {
+        return item[(__bridge id)kSecValueData];
+      }
+    }
+
+    // If they were all legacy items, just return the first one.
+    // This should not happen, since only one account should be
+    // stored.
+    return items[0][(__bridge id)kSecValueData];
   }
 
   if (status == errSecItemNotFound) {

FirebaseAuth/Tests/Unit/FIRAuthKeychainServicesTests.m

@@ -84,6 +84,13 @@
   return [NSError errorWithDomain:@"ERROR" code:-1 userInfo:nil];
 }
 
+@interface FIRAuthKeychainServices ()
+
+// Exposed for testing.
+- (nullable NSData *)itemWithQuery:(NSDictionary *)query error:(NSError **_Nullable)error;
+
+@end
+
 /** @class FIRAuthKeychainTests
     @brief Tests for @c FIRAuthKeychainTests .
  */
@@ -116,6 +123,30 @@ - (void)testReadExisting {
   [self deletePasswordWithAccount:accountFromKey(kKey) service:kService];
 }
 
+/** @fn testReadMultiple
+    @brief Tests reading multiple items from keychain returns only the first item.
+ */
+- (void)testReadMultiple {
+  [self addPassword:kData account:accountFromKey(kKey) service:kService];
+  [self addPassword:kOtherData account:accountFromKey(kKey) service:kOtherService];
+  FIRAuthKeychainServices *keychain = [[FIRAuthKeychainServices alloc] initWithService:kService];
+  NSString *queriedAccount = accountFromKey(kKey);
+  NSDictionary *query = @{
+    (__bridge id)kSecClass : (__bridge id)kSecClassGenericPassword,
+    (__bridge id)kSecAttrAccount : queriedAccount,
+  };
+  NSError *error = fakeError();
+  // Keychain on macOS returns items in a different order than keychain on iOS,
+  // so test that the returned object is one of any of the added objects.
+  NSData *queriedData = [keychain itemWithQuery:query error:&error];
+  BOOL isValidKeychainItem =
+      [@[ dataFromString(kData), dataFromString(kOtherData) ] containsObject:queriedData];
+  XCTAssertTrue(isValidKeychainItem);
+  XCTAssertNil(error);
+  [self deletePasswordWithAccount:accountFromKey(kKey) service:kService];
+  [self deletePasswordWithAccount:accountFromKey(kKey) service:kOtherService];
+}
+
 /** @fn testNotReadOtherService
     @brief Tests not reading keychain item belonging to other service.
  */