Camino only - Bug 372153: Don't try to access keychain data until necessary, and don't keep trying after being denied. r=josh sr=pink

git-svn-id: svn://10.0.0.236/trunk@223986 18797224-902f-48f8-a5cc-f745e15eee43

mozilla/camino/src/formfill/KeychainItem.h

@@ -44,6 +44,7 @@
  @private
   SecKeychainItemRef mKeychainItemRef; // strong
   BOOL mDataLoaded;
+  BOOL mPasswordLoaded;
   NSString* mUsername;                 // strong
   NSString* mPassword;                 // strong
   NSString* mHost;                     // strong

mozilla/camino/src/formfill/KeychainItem.m

@@ -41,6 +41,7 @@
 @interface KeychainItem(Private)
 - (KeychainItem*)initWithRef:(SecKeychainItemRef)ref;
 - (void)loadKeychainData;
+- (void)loadKeychainPassword;
 - (BOOL)setAttributeType:(SecKeychainAttrType)type toString:(NSString*)value;
 - (BOOL)setAttributeType:(SecKeychainAttrType)type toValue:(void*)valuePtr withLength:(UInt32)length;
 @end
@@ -204,15 +205,11 @@
   attrInfo.format = NULL;
 
   SecKeychainAttributeList *attrList;
-  UInt32 passwordLength;
-  char* passwordData;
-  OSStatus result = SecKeychainItemCopyAttributesAndData(mKeychainItemRef, &attrInfo, NULL, &attrList,
-                                                         &passwordLength, (void**)(&passwordData));
+  OSStatus result = SecKeychainItemCopyAttributesAndData(mKeychainItemRef, &attrInfo, NULL,
+                                                         &attrList, NULL, NULL);
 
   [mUsername autorelease];
   mUsername = nil;
-  [mPassword autorelease];
-  mPassword = nil;
   [mHost autorelease];
   mHost = nil;
   [mComment autorelease];
@@ -221,9 +218,8 @@
   mSecurityDomain = nil;
 
   if (result != noErr) {
-    NSLog(@"Couldn't load keychain data");
+    NSLog(@"Couldn't load keychain data (error %d)", result);
     mUsername = [[NSString alloc] init];
-    mPassword = [[NSString alloc] init];
     mHost = [[NSString alloc] init];
     mComment = [[NSString alloc] init];
     mSecurityDomain = [[NSString alloc] init];
@@ -249,11 +245,39 @@
     else if (attr.tag == kSecCreatorItemAttr)
       mCreator = attr.data ? *((OSType*)(attr.data)) : 0;
   }
-  mPassword = [[NSString alloc] initWithBytes:passwordData length:passwordLength encoding:NSUTF8StringEncoding];
-  SecKeychainItemFreeAttributesAndData(attrList, (void*)passwordData);
   mDataLoaded = YES;
 }
 
+// Password is fetched separately, since trying to read the password will
+// trigger an auth request if we aren't on the trust list.
+- (void)loadKeychainPassword
+{
+  if (!mKeychainItemRef)
+    return;
+  UInt32 passwordLength;
+  char* passwordData;
+  OSStatus result = SecKeychainItemCopyAttributesAndData(mKeychainItemRef, NULL, NULL, NULL,
+                                                         &passwordLength, (void**)(&passwordData));
+  
+  [mPassword autorelease];
+  mPassword = nil;
+  
+  if (result == noErr) {
+    mPassword = [[NSString alloc] initWithBytes:passwordData
+                                         length:passwordLength
+                                       encoding:NSUTF8StringEncoding];
+    SecKeychainItemFreeAttributesAndData(NULL, (void*)passwordData);
+  }
+  else {
+    // Being denied access isn't a failure case, so don't log it.
+    if (result != errSecAuthFailed)
+      NSLog(@"Couldn't load keychain data (%d)", result);
+  }
+  // Mark it as loaded either way, so that we can return nil password as an
+  // indicator that the item is inaccessible.
+  mPasswordLoaded = YES;
+}
+
 - (NSString*)username
 {
   if (!mDataLoaded)
@@ -263,8 +287,8 @@
 
 - (NSString*)password
 {
-  if (!mDataLoaded)
-    [self loadKeychainData];
+  if (!mPasswordLoaded)
+    [self loadKeychainPassword];
   return mPassword;
 }
 

mozilla/camino/src/formfill/KeychainService.mm

@@ -241,18 +241,23 @@ int KeychainPrefChangedCallback(const char* inPref, void* unused)
     }
     newKeychainItems = [matchingItems arrayByAddingObjectsFromArray:genericItems];
   }
+  
+  // Find the best matching keychain entry. The password is always checked to
+  // ensure that it's not nil before returning that item (which triggers an auth
+  // request if necsessary) so that we know we are always returning something
+  // that is actually useful.
 
   // First, check for a new-style Camino keychain entry
   KeychainItem* item;
   NSEnumerator* keychainEnumerator = [newKeychainItems objectEnumerator];
   while ((item = [keychainEnumerator nextObject])) {
-    if ([item creator] == kCaminoKeychainCreatorCode)
+    if (([item creator] == kCaminoKeychainCreatorCode) && [item password])
       return item;
   }
 
   // If there isn't one, check for an old-style Camino entry
   item = [self findLegacyKeychainEntryForHost:host port:port];
-  if (item)
+  if (item && [item password])
     return item;
 
   // Finally, check for a new style entry created by something other than Camino.
@@ -267,14 +272,16 @@ int KeychainPrefChangedCallback(const char* inPref, void* unused)
       // items; that's just the way they roll. This fragile method is the best we can do.
       if ([[item password] isEqualToString:@" "])
         continue;
-      return item;
+      if ([item password])
+        return item;
     }
   }
   keychainEnumerator = [newKeychainItems objectEnumerator];
   while ((item = [keychainEnumerator nextObject])) {
     if ([[item password] isEqualToString:@" "])
       continue;
-    return item;
+    if ([item password])
+      return item;
   }
 
   return nil;