474792 - Need an API to clear all "login saving enabled" entries (for Clear Recent History)

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P2)

Description

[Johnathan Nightingale [:johnath]]

Bug 380852 added support for clearing site-specific settings from the Clear Recent History dialog. Implementation-wise, this includes ContentPrefs and PermissionManager entries.  But Marcia reports that one thing that isn't cleared is "never remember passwords for this site", which is unsurprising since it doesn't use the permission manager (cf: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/src/storage-mozStorage.js#460 )

If there's already a bug to move pwmgr to permission manager, this is probably dupable, but if that's not desirable, then it would at least be nice to have an API for dropping these "don't save passwords for host X" entries so that people could Clear Recent History without clearing their entire password database.

Comment 1

[Johnathan Nightingale [:johnath]]

Nominating this to block final release (P3).  If we ship with this, people risk having embarassing information preserved that they expected to have deleted.  On the other hand, it is a limited case, most site-specific data is correctly cleared by the code in bug 380852, so to the extent that we are saying some things (P1, P2) "block more" than others, I'd argue this one blocks less.  Since it changes an API, we might be reluctant to take it post-release (though it's a purely additive change, so there's less risk, but risk still exists).

Comment 2

[Justin Dolske [:Dolske]]

A new API isn't necessarily needed here. You can clear the list with:

var hosts = pwmgr.getAllDisabledHosts()
for each (var host in hosts)
    pwmgr.setLoginSavingEnabled(host, true);

But I wouldn't be opposed to having a clearAllDisabledHosts() that does the same thing.

I'm a *little* wary of having CRH clear this; it feels different than the normal site-specific settings because entries were set as a result of an explicit user choice to prevent us from nagging to remember a login (or a deliberate decision not to not save a sensitive bank login). It's less ephemeral than, say, a saved page-zoom setting. [Or maybe not, I could see arguments either way.]

OTOH, having separate UI option just for this list would be overkill, and it's probably not easy to explain as a CRH item in 3-4 words. Good is better than Perfect here, I think.

Comment 3

[Johnathan Nightingale [:johnath]]

(In reply to comment #2)
> I'm a *little* wary of having CRH clear this; it feels different than the
> normal site-specific settings because entries were set as a result of an
> explicit user choice to prevent us from nagging to remember a login (or a
> deliberate decision not to not save a sensitive bank login). It's less
> ephemeral than, say, a saved page-zoom setting. [Or maybe not, I could see
> arguments either way.]

Yeah, I hear that, but we are already clearing nsIPermissionManager entries like "allow popups" which are similarly explicit choices. I feel silly for not seeing getAllDisabledHosts(), that would certainly be the least invasive way to do this.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Doesn't block, but I'd likely approve a patch and we should get it as cleanup.

Comment 5

[Johnathan Nightingale [:johnath]]

Attachment: Clear using the existing API, rather than creating a new one

If I'm the only consumer for such an API, why not just use the thing that already exists?  What do you think, Dolske?

Comment 6

[Justin Dolske [:Dolske]]

Comment on attachment 359351 [details] [diff] [review]
Clear using the existing API, rather than creating a new one

Yeah, this is fine. No need to block on API work... I'll spin off a clone of this as a pwmgr bug to add such as API, as I still think it would be a good idea.

Comment 7

[Justin Dolske [:Dolske]]

Filed bug 476015 for adding this as an explicit API.

Comment 8

[Johnathan Nightingale [:johnath]]

Landed on trunk for baking.

http://hg.mozilla.org/mozilla-central/rev/ce260f307e23