- Improve the favourites editing process to edit keychain items, rather than deleting and recreating them. This has two advantages: firstly, it matches the Apple recommendation, as it preserves keychain item access lists and comments for keychain items when they are edited; secondly, it works around a bug in 10.7 which appears to be a Keychain cacheing issue, causing password retrievals to return the original keychain item on launch - which is no longer valid after deletion/recreation. This addresses Issue #1197.

3 files changed, 84 insertions, 22 deletions

diff --git a/Source/SPFavoritesPreferencePane.m b/Source/SPFavoritesPreferencePane.m
index 6a5270c0..c009f8e3 100644
--- a/Source/SPFavoritesPreferencePane.m
+++ b/Source/SPFavoritesPreferencePane.m
@@ -703,7 +703,7 @@
 		[[sheet window] orderOut:nil];
 	}
 	
-	// Remove the current database
+	// Remove the current favorite
 	if ([contextInfo isEqualToString:@"removeFavorite"]) {
 		if (returnCode == NSAlertDefaultReturn) {
 			
@@ -855,17 +855,17 @@
 		// Get the old keychain name and account strings
 		oldKeychainName = [keychain nameForFavoriteName:[currentFavorite objectForKey:@"name"] id:[favoritesController valueForKeyPath:@"selection.id"]];
 		oldKeychainAccount = [keychain accountForUser:[currentFavorite objectForKey:@"user"] host:oldHostnameForPassword database:[currentFavorite objectForKey:@"database"]];
-		
-		// Delete the old keychain item
-		[keychain deletePasswordForName:oldKeychainName account:oldKeychainAccount];
-		
-		// Set up the new keychain name and account strings
-		newKeychainName = [keychain nameForFavoriteName:[favoritesController valueForKeyPath:@"selection.name"] id:[favoritesController valueForKeyPath:@"selection.id"]];
-		newKeychainAccount = [keychain accountForUser:[favoritesController valueForKeyPath:@"selection.user"] host:newHostnameForPassword database:[favoritesController valueForKeyPath:@"selection.database"]];
-		
-		// Add the new keychain item if the password field has a value
-		if ([passwordValue length])
-			[keychain addPassword:passwordValue forName:newKeychainName account:newKeychainAccount];
+
+		// If there's no new password, remove the old item from the keychain
+		if (![passwordValue length]) {
+			[keychain deletePasswordForName:oldKeychainName account:oldKeychainAccount];
+
+		// Otherwise, set up the new keychain name and account strings and edit the item
+		} else {
+			newKeychainName = [keychain nameForFavoriteName:[favoritesController valueForKeyPath:@"selection.name"] id:[favoritesController valueForKeyPath:@"selection.id"]];
+			newKeychainAccount = [keychain accountForUser:[favoritesController valueForKeyPath:@"selection.user"] host:newHostnameForPassword database:[favoritesController valueForKeyPath:@"selection.database"]];
+			[keychain updateItemWithName:oldKeychainName account:oldKeychainAccount toName:newKeychainName account:newKeychainAccount password:passwordValue];
+		}
 		
 		// Synch password changes
 		[standardPasswordField setStringValue:passwordValue];
@@ -885,16 +885,16 @@
 		oldKeychainName = [keychain nameForSSHForFavoriteName:[currentFavorite objectForKey:@"name"] id:[favoritesController valueForKeyPath:@"selection.id"]];
 		oldKeychainAccount = [keychain accountForSSHUser:[currentFavorite objectForKey:@"sshUser"] sshHost:[currentFavorite objectForKey:@"sshHost"]];
 		
-		// Delete the old keychain item
-		[keychain deletePasswordForName:oldKeychainName account:oldKeychainAccount];
-		
-		// Set up the new keychain name and account strings
-		newKeychainName = [keychain nameForSSHForFavoriteName:[favoritesController valueForKeyPath:@"selection.name"] id:[favoritesController valueForKeyPath:@"selection.id"]];
-		newKeychainAccount = [keychain accountForSSHUser:[favoritesController valueForKeyPath:@"selection.sshUser"] sshHost:[favoritesController valueForKeyPath:@"selection.sshHost"]];
-		
-		// Add the new keychain item if the password field has a value
-		if ([[sshPasswordField stringValue] length])
-			[keychain addPassword:[sshPasswordField stringValue] forName:newKeychainName account:newKeychainAccount];
+		// If there's no new password, delete the keychain item
+		if (![[sshPasswordField stringValue] length]) {
+			[keychain deletePasswordForName:oldKeychainName account:oldKeychainAccount];
+
+		// Otherwise, set up the new keychain name and account strings and update the keychain item
+		} else {
+			newKeychainName = [keychain nameForSSHForFavoriteName:[favoritesController valueForKeyPath:@"selection.name"] id:[favoritesController valueForKeyPath:@"selection.id"]];
+			newKeychainAccount = [keychain accountForSSHUser:[favoritesController valueForKeyPath:@"selection.sshUser"] sshHost:[favoritesController valueForKeyPath:@"selection.sshHost"]];
+			[keychain updateItemWithName:oldKeychainName account:oldKeychainAccount toName:newKeychainName account:newKeychainAccount password:[sshPasswordField stringValue]];
+		}
 	}
 	
 	// Update the current favorite
diff --git a/Source/SPKeychain.h b/Source/SPKeychain.h
index c1984be1..31182fe1 100644
--- a/Source/SPKeychain.h
+++ b/Source/SPKeychain.h
@@ -30,6 +30,8 @@
 - (NSString *)getPasswordForName:(NSString *)name account:(NSString *)account;
 - (void)deletePasswordForName:(NSString *)name account:(NSString *)account;
 - (BOOL)passwordExistsForName:(NSString *)name account:(NSString *)account;
+- (void)updateItemWithName:(NSString *)name account:(NSString *)account toPassword:(NSString *)password;
+- (void)updateItemWithName:(NSString *)name account:(NSString *)account toName:(NSString *)newName account:(NSString *)newAccount password:(NSString *)password;
 - (NSString *)nameForFavoriteName:(NSString *)theName id:(NSString *)theID;
 - (NSString *)accountForUser:(NSString *)theUser host:(NSString *)theHost database:(NSString *)theDatabase;
 - (NSString *)nameForSSHForFavoriteName:(NSString *)theName id:(NSString *)theID;
diff --git a/Source/SPKeychain.m b/Source/SPKeychain.m
index 931f14d7..aa38f7b5 100644
--- a/Source/SPKeychain.m
+++ b/Source/SPKeychain.m
@@ -232,6 +232,66 @@
 }
 
 /**
+ * Change the password for a keychain item.  This should be used instead of
+ * deleting and recreating the keychain item, as it allows preservation of
+ * access lists and works around Lion cacheing issues.
+ */
+- (void)updateItemWithName:(NSString *)name account:(NSString *)account toPassword:(NSString *)password
+{
+	[self updateItemWithName:name account:account toName:password account:name password:account];
+}
+
+/**
+ * Change the details for a keychain item.  This should be used instead of
+ * deleting and recreating the keychain item, as it allows preservation of
+ * access lists and works around Lion cacheing issues.
+ */
+- (void)updateItemWithName:(NSString *)name account:(NSString *)account toName:(NSString *)newName account:(NSString *)newAccount password:(NSString *)password
+{
+	OSStatus status;
+	SecKeychainItemRef itemRef;
+	SecKeychainAttribute attributes[2];
+	SecKeychainAttributeList attList;
+
+	// Retrieve a reference to the keychain item
+	status = SecKeychainFindGenericPassword(NULL,														// Default keychain
+											(UInt32)strlen([name UTF8String]), [name UTF8String],		// Service name and length
+											(UInt32)strlen([account UTF8String]), [account UTF8String],	// Account name and length
+											NULL, NULL,													// No password retrieval required
+											&itemRef);													// The item reference
+
+	if (status != noErr) {
+		NSLog(@"Error (%i) while trying to find keychain item to edit for name: %@ account: %@", (int)status, name, account);
+		SPBeginAlertSheet(NSLocalizedString(@"Error retrieving Keychain item to edit", @"error finding keychain item to edit message"), 
+						  NSLocalizedString(@"OK", @"OK button"), 
+						  nil, nil, [NSApp mainWindow], self, nil, nil,
+						  [NSString stringWithFormat:NSLocalizedString(@"An error occured while trying to retrieve the Keychain item you're trying to edit. Repairing your Keychain might resolve this, but if it doesn't please report it to the Sequel Pro team, supplying the error code %i.", @"error finding keychain item to edit informative message"), status]);
+		return;
+	}
+
+	// Set up the attributes to modify
+	attributes[0].tag = kSecAccountItemAttr;
+	attributes[0].data = (unichar *)[newAccount UTF8String];
+	attributes[0].length = (UInt32)strlen([newAccount UTF8String]);
+	attributes[1].tag = kSecServiceItemAttr;
+	attributes[1].data = (unichar *)[newName UTF8String];
+	attributes[1].length = (UInt32)strlen([newName UTF8String]);
+	attList.count = 2;
+	attList.attr = attributes;
+
+	// Amend the keychain item
+	status = SecKeychainItemModifyAttributesAndData(itemRef, &attList, (UInt32)strlen([password UTF8String]), [password UTF8String]);
+
+	if (status != noErr) {
+		NSLog(@"Error (%i) while updating keychain item for name: %@ account: %@", (int)status, name, account);
+		SPBeginAlertSheet(NSLocalizedString(@"Error updating Keychain item", @"error updating keychain item message"), 
+						  NSLocalizedString(@"OK", @"OK button"), 
+						  nil, nil, [NSApp mainWindow], self, nil, nil,
+						  [NSString stringWithFormat:NSLocalizedString(@"An error occured while trying to update the Keychain item. Repairing your Keychain might resolve this, but if it doesn't please report it to the Sequel Pro team, supplying the error code %i.", @"error updating keychain item informative message"), status]);
+	}
+}
+
+/**
  * Retrieve the keychain item name for a supplied name and id.
  */
 - (NSString *)nameForFavoriteName:(NSString *)theName id:(NSString *)theID