331407 - "Reset Camino" does not remove Camino-inserted password entries from Keychain

Status: RESOLVED FIXED

(Camino Graveyard :: OS Integration, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

We removed that feature because it ate the entire Keychain; this is the placeholder for restoring it when it is able to delete only Camino-added entries that no other app is also using.  (This depends on the Keychain Services API bug, right?)

In light of the "Firefox broke up my engagement" issue, we should also delete the "Keychain Deny List" file from the profile when doing this (that file is not human-editable, so there's no way to change your mind about one site without trashing the entire list; should there be a way?)

Comment 1

[Simon Fraser [no longer active]]

(In reply to comment #0)

> (that file is
> not human-editable, so there's no way to change your mind about one site
> without trashing the entire list; should there be a way?)

It should juset be saved in XML format, not binary.

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

WRT the second pgh of comment 0, see also bug 331652.  However, I think the solution of making the Keychain Deny List be cleared by "Reset Camino" (and not "stored permanently" in the future Private Browsing mode) while making it XML is better.  The file *does* need to be reasonably-easily editable (we fielded a query this week on fixing an accidentally-pressed deny) without requiring us to implement any UI to do so.

Also targeting this for 1.1 atm, since in theory big chunks of both comment 0 pgh 1 and pgh 2 are a Keychain-related bug, and in theory the Keychain-related bugs are getting fixed for 1.1 ;)

Comment 3

[Samuel Sidler (old account; do not CC)]

Keychain bugs -> 1.2.

Comment 4

[Stuart Morgan]

Taking. We should start setting the kSecCreatorItemAttr so that we can definitively identify passwords we create.  We'll either have to fix this in two stages--one where we start tagging items as we find them, and then another where we re-enable deletion--or else we'll need to do a sweep of all keychain items. Not doing a sweep has the disadvantage that any password you haven't used since stage one lands wouldn't be deleted.

In theory, neither of those things is safe, but in practice it seems unlikely that anyone else uses our somewhat archaic auth/protocol pair. (It would be good to know if bug 320208 ever actually happened to anyone.)

Comment 5

[Stuart Morgan]

Apparently bug 320208 was seen in testing, but given that the deletion code was finding items to delete essentially the same way it finds items to fill with, and the old keys we use mean we don't find Safari's keychain items, I don't see how that could have happened. We'll need to understand that before we can do any kind of upgrading of old items, since we don't want to mark existing items as Camino-owned if they aren't.

Comment 6

[Stuart Morgan]

(In reply to comment #4)
> In theory, neither of those things is safe, but in practice it seems unlikely
> that anyone else uses our somewhat archaic auth/protocol pair.

At least iDisk uses this pair, which is what prompted bug 320208. They have both a path and crtr though, so that's filterable.

Still, probably best to do incremental upgrades and live with a partial delete, rather than risk catching things we don't own in a big one-pass sweep.

Comment 7

[Stuart Morgan]

*** Bug 362690 has been marked as a duplicate of this bug. ***

Comment 8

[Stuart Morgan]

Bug 202337 added creator markers for all new entries we create, so this will be easy to fix. We're not going to be able to clean up any entries made before that change went in though, so we'll have to relnote that or something.

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Plus it will be a bit wonky for people moving from Safari permanently, but I don't see any way to be safe for other Safari-sharing cases and please these people, who are probably a very small edge case ;)

Comment 10

[Stuart Morgan]

Attachment: fix

I just discovered that I had this in my tree and never posted it. Causes 'Reset Camino' to delete Camnio-stored entries, as well as clearing the deny list. (It also removes some needless complexity from the deny list storage object.)

There's probably no reason we couldn't take this for 1.1

Comment 11

[Stuart Morgan]

Comment on attachment 254767 [details] [diff] [review]
fix

Smokey, could you put the reset part of this through its paces in your test account?

The only other thing this will need is a strings change like:
"Reset Warning Message" = "Resetting Camino will erase your browsing history, empty the cache, clear downloads, clear all cookies and site cookie permissions, and clear any passwords stored in the keychain by Camino.\n\nYou cannot undo this action.\n";

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

This'll need a string change along these lines:

- "Reset Warning Message" = "Resetting Camino will erase your browsing history, empty the cache, clear downloads, clear all cookies, and clear all site cookie permissions.\n\nPasswords saved in your Keychain are not removed.\n\nYou cannot undo this action.\n";

+ "Reset Warning Message" = "Resetting Camino will erase your browsing history, empty the cache, clear downloads, clear all cookies, clear all site cookie permissions, and remove any passwords Camino saved in your Keychain.\n\nYou cannot undo this action.\n";

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Er, ignore that last comment; I hit the wrong button on the conflict page :(

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 254767 [details] [diff] [review]
fix

r=ardissone; this only deleted the 7(!) entries I added or changed since the rewrite.

We'll need to relnote this well, like the rewrite.

Comment 15

[Stuart Morgan]

(In reply to comment #13)
> Er, ignore that last comment; I hit the wrong button on the conflict page :(

I'm glad you did, since your text is better :)

Comment 16

[Stuart Morgan]

Checked in on trunk and MOZILLA_1_8_BRANCH with text slightly between mine and Smokey's

Comment 17

[Samuel Sidler (old account; do not CC)]

Moving fixed "1.2" bugs to 1.1 where they were really fixed. Filter on CaminoFixed1.1 for bugmail purposes.