User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.7) Gecko/20060909 Firefox/1.5.0.7 Build Identifier: Camino 1.0.3 (Sorry I can't get the full exact UA string, because I overwrite it to Firefox in the hidden prefs) When I enter a username/password pair, and choose "Never save password" after submitting the form, Camino should store this in the keychain, like Safari does. So that I can easily view and edit the list of sites where I do want passwords saved. I couldn't find where Camino does store those info. Reproducible: Always

Camino stores the info in a filed called "Keychain Deny List" in the profile folder. Saving these passwords seems as though it would go against the whole point of "Never save", so perhaps I'm not understanding what you're asking here. cl

Safari actually keeps keychain entries (without passwords) as markers for sites that have been put on the deny list. This makes it possible to remove a site if you make a mistake or change you mind, whereas with Camino a) you have to know where the list is stored, and b) your only option is to nuke the entire list. Confirming, but changing title and re-purposing slightly. I don't think we necessarily need to store non-information in the keychain, but it definitely needs to be somewhere more accessible than an obscure binary-format file.

(In reply to comment #1) > Camino stores the info in a filed called "Keychain Deny List" in the profile > folder. Oh yes, correct. This file is written only once you quit Camino, that's why I didn't find it at first. > Saving these passwords seems as though it would go against the whole point of > "Never save", so perhaps I'm not understanding what you're asking here. Let me rephrase. First, here's how Safari works. When I choose "Never for this Website" after filling a form with username/password, an entry is added to the keychain, that reads, e.g.: Name: www.paypal.com (Passwords not saved) Kind: Web form passsword This does not store any password, but the fact that I don't want to be asked again to save any. If I accidentally clicked "Never for this Website", or change my mind later and now want to save passwords at www.paypal.com, I can simply remove this entry from the keychain. Camino should follow the same idea, because editing "Keychain Deny List" is rather odd, and its format not designed for human editing. In a perfect world, Camino would use the same format than Safari, and share those "Never save passwords" entries in the keychain. But reading the other bugs on the keychain subject, I see that Camino and Safari are already not using the same format for other kind of entries...

(In reply to comment #3) > In a perfect world, Camino would use the same format than Safari, and share > those "Never save passwords" entries in the keychain. But reading the other > bugs on the keychain subject, I see that Camino and Safari are already not > using the same format for other kind of entries... That will be much less true in the near future though, which is certainly an argument in favor of mimicing Safari's implementation.

Fixing summary.

See also bug 331407 comment 1, but if we store a "not saved" entry that we can also delete along with "real" entries when resetting Camino, that should work, too. (Though if a keychain is locked and we have to prompt to unlock the keychain when the user has just clicked "never save", that might be odd....)

It will make me sad to ship another release where even when people specifically ask how to remove a site the best answer we can give is "remove the entire list". Since we aren't doing any UI around this or putting it in the keychain for 1.1, this switches the list to a plist, so that it's easy to remove a specific entry.

Comment on attachment 257105 [details] [diff] [review] interim solution [checked in] per comment 7, I'd like to slip this into 1.1.

Comment on attachment 257105 [details] [diff] [review] interim solution [checked in] -// -// pathToDenyListFile -// -// returns a path ('/' delimited) that cocoa can use to point to the -// deny list save file in the current user's profile -// why remove the comment? *shrug* sr=pink

I just thought that "pathToDenyListFile" was fairly self-explanatory, and the fact that it's /-delimited and in the profile folder of the user using Camino seemed not to be terribly interesting.

Comment on attachment 257105 [details] [diff] [review] interim solution [checked in] Checked in on trunk and MOZILLA_1_8_BRANCH. Leaving the bug open for a real fix later on.

This, along with the accompanying nib update, adds an "Edit Keychain Exclusions…" button to the Privacy preference pane. The button is the gateway to a panel that lets you remove things on the KeychainDenyList, à la the cookie and cookie permission panels. The weird twist here is that the KeychainDenyList is owned by KeychainService.mm in the application, where it's not accessible by the Privacy preference pane, because we don't link preference panes against the application. At the same time, the application doesn't link against the preference panes, so there isn't a good way to move the list's ownership to the pane and have the application still access it. The correct solution is probably to link preference panes that need it against the application. I'm not in the mood for project file antics right now, though, so I've taken the weird and cheesey approach: id mKeychainDenyList = [[[NSBundle mainBundle] classNamed:@"KeychainDenyList"] instance]; Ha. No, I won't check it in like this. But I found it kind of interesting, and it's the reason I started looking more closely at this bug after discovering it earlier today. I also found that when I edited Privacy.nib (possibly significant: I'm on 10.5), I couldn't open the cookie panel any more, and I'd get messages like this: 2007-12-09 00:52:16.352 Camino[65671:10b] *** -[NSCFString timeIntervalSince1970]: unrecognized selector sent to instance 0x5f3e70 I don't have time to care about that right now, so I added a check for NSDate-ness to the code to get the cookie panel working.

Comment on attachment 292285 [details] nib On Leopard, the action buttons in the two existing sheets in the Privacy preference pane were missing the bottom pixel. I didn't check Tiger or earlier, but I assume they're OK, because it looked pretty obviously like ass. This nib fixes those buttons.

Yeah, froodian very carefully edited the size/position of those before 1.5, but I discovered over the weekend poking your nib that IB 3 (and presumably 10.5) measures things differently from 10.4/IB 2.x (which in turn does so differently from 10.3/IB 1.x). It's fun, let me tell you :p

I was eager to take -bundle_loader for a spin now that bug 383934 is fixed, so I adapted the "weird twist" patch. The project file changes add KeychainDenyList.h,mm to CaminoApp and CaminoStaticApp, and KeychainDenyList.h to PrivacyPrefPane. KeychainDenyList.h,mm were split off from KeychainService.h,mm to avoid having to carry the Gecko gunk in the latter over into the preference pane.

Comment on attachment 292285 [details] nib This looks fine on 10.5.1. I can't test 10.3.9 until the prefs stuff lands on branch (that's a little too much porting for me right now), but the one sheet I could open in an existing build seems to show the wrong-sized action button just fine. (Someone probably should check on 10.4.x to be sure the action buttons look OK there.)

Comment on attachment 292547 [details] [diff] [review] Take advantage of -bundle_loader Patch is missing changes to the prefPane's Localizable.strings: RemoveAllKeychainExclusionsWarningTitle RemoveAllKeychainExclusionsWarning RemoveAllKeychainExclusionsButton

Oops, sorry, I meant to include this in the patch.

Since 404250 has landed, the column id's in the permissions sheet of the new nib need to change to "host" and "policy"

rot

And be sure to re-apply your fix for the action menu buttons, too :(

Un-bit-rotted following Stuart's bug 404250. This patch contains the strings diff, too. The project diff only applies on the branch. It adds the new files to the right phases of the right targets (as in comment 16). This, with the accompanying nib, also fixes bug 356204 for the Privacy preference pane.

This was totally redone in Interface Builder 2.5.6 (Xcode 2.5). Should be more compatible than the last version: I didn't have to add the NSDate-kindness checks to stringForObjectValue:, the cookie sheet worked on its own as-is. This version also contains the action button size fix, and has the renamed class. I'm requesting nib review because I redid this, and it's possible that I missed something.

In addition to fixing the size of the action menu button, this puts them back in the right position. Doh!

Comment on attachment 293954 [details] Privacy.nib r=ardissone Some of the sides of the action buttons seem perhaps off a single pixel, but it's not really noticeable (and isn't displayed consistently across OSes), so this is fine as-is

Comment on attachment 293941 [details] [diff] [review] Allow in-app removal from KeychainDenyList.plist. Take advantage of -bundle_loader. Un-bit-rotted. r=smorgan >+@implementation NSString (ReverseCompare) >+ >+- (NSComparisonResult)reverseCompare:(NSString*)other { >+ NSComparisonResult result = [self compare:other]; >+ if (result == NSOrderedAscending) { >+ return NSOrderedDescending; >+ } >+ else if (result == NSOrderedDescending) { >+ return NSOrderedAscending; >+ } >+ return result; >+} Why not just use sort descriptors, where you can use the standard comparison method and tell it to do a reverse sort? >+ [mKeychainExclusionsTable setIndicatorImage:[NSImage imageNamed:@"NSAscendingSortIndicator"] inTableColumn:sortedColumn]; Line break before inTableColumn:? >Index: mozilla/camino/src/formfill/KeychainDenyList.h >=================================================================== ... >+ * Portions created by the Initial Developer are Copyright (C) 2002 ... >+ * Contributor(s): >+ * >Index: mozilla/camino/src/formfill/KeychainDenyList.mm >=================================================================== >+ * Portions created by the Initial Developer are Copyright (C) 1998 ... >+ * Contributor(s): >+ * Benoit Foucher <bfoucher@mac.com> >+ * Stuart Morgan <stuart.morgan@gmail.com> >+ * Shouldn't these match?

Checked in on the trunk and MOZILLA_1_8_BRANCH for 1.6b1 with Stuart's suggestions. >Why not just use sort descriptors, where you can use the standard comparison >method and tell it to do a reverse sort? OK, but this required mapping the "Website" column identifier to the "self" key. You can switch this to use reverseHostnameCompare: too when you check in bug 377359. >Line break before inTableColumn:? Actually, we don't even need this line and a couple others in the immediate vicinity. From right here (editKeychainExclusions:), we already call sortKeychainExclusionsByColumn:inAscendingOrder:, which calls updateSortIndicatorWithColumn:, which in turn highlights the proper column and sets the indicator image. >Shouldn't these match? Yeah, I just took the headers straight out of the KeychainService.{h,mm} files from which I was forking. I looked through Bonsai and made them better reflect the deal.

A subsequent bustage fix was checked in on the trunk and MOZILLA_1_8_BRANCH because Xcode 2.x's ld dead-strips some .objc_class_name_* symbols: http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/camino/src/formfill&command=DIFF_FRAMESET&file=KeychainDenyList.mm&rev1=1.1&rev2=1.2&root=/cvsroot