1520960 - Allow saving passwords in private windows with a dismissed-by-default doorhanger

Status: RESOLVED FIXED

(Toolkit :: Password Manager, enhancement, P2)

Description

[Matthew N. [:MattN]]

We currently don't allow saving/updating passwords in private browsing which makes sense if PB is only used to avoid leaving any local traces of your visit but doesn't support other use cases for PB like not persisting financial information on your hard drive or segregating login cookies for multi-account uses.

Since we require user action to save passwords, we should give the user the option to save them, like we do with bookmarks.

As a first step, partly to mitigate click-jacking, and to ease into this change, I suggest that the save/remember doorhanger be dismissed by default in PB windows. This should be as easy as setting dismissed:true[1] on the doorhanger's .show call.

For the update doorhanger, it probably doesn't make sense for it to be dismissed by default since not updating means the user will have stale data.

If it's straightforward, this new logic should be implemented behind a preference.

[1] https://dxr.mozilla.org/mozilla-central/rev/c2593a3058afdfeaac5c990e18794ee8257afe99/toolkit/modules/PopupNotifications.jsm#379-381

Comment 1

[Sam Foster [:sfoster] (he/him)]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=932ba535a509e4ea3757885cb4458cd4d0af4cf2

This is my first pass at a patch for this. There are other guards/early-returns for private windows in nsLoginManagerPrompter.js, but I've tried to limit to the patch to just the code paths concerning the capture doorhanger. I'm not sure if there are other ramifications to the change in LoginManagerContent.jsm?

Comment 2

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1520960 - Allow login capture from form submissions in private browsing when pref'd on. r?MattN

Comment 3

[Matthew N. [:MattN]]

(In reply to Sam Foster [:sfoster] from comment #1)

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=932ba535a509e4ea3757885cb4458cd4d0af4cf2

This is my first pass at a patch for this. There are other guards/early-returns for private windows in nsLoginManagerPrompter.js, but I've tried to limit to the patch to just the code paths concerning the capture doorhanger. I'm not sure if there are other ramifications to the change in LoginManagerContent.jsm?

One thing I wondered is if we should avoid updating the timeLastUsed metadata on the login in PB since that doesn't require a user interaction.

Comment 4

[Sam Foster [:sfoster] (he/him)]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #3)

One thing I wondered is if we should avoid updating the timeLastUsed metadata on the login in PB since that doesn't require a user interaction.

Do you mean we should essentially treat the login timeLastUsed metadata as read-only in PB - regardless of any user interaction or lack of interaction? Or only avoid updating this property in this specific case where we show the doorhanger as dismissed.

I'm looking at onFormSubmit in LoginManagerParent.jsm: https://searchfox.org/mozilla-central/source/toolkit/components/passwordmgr/LoginManagerParent.jsm#317 and one solution would be a check in recordLoginUse to see if PrivateBrowsingUtils.isBrowserPrivate(target) ?

Comment 5

[Matthew N. [:MattN]]

(In reply to Sam Foster [:sfoster] from comment #4)

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #3)

One thing I wondered is if we should avoid updating the timeLastUsed metadata on the login in PB since that doesn't require a user interaction.

Do you mean we should essentially treat the login timeLastUsed metadata as read-only in PB - regardless of any user interaction or lack of interaction? Or only avoid updating this property in this specific case where we show the doorhanger as dismissed.

I was thinking of treating it as read-only when there is no user interaction. A user interaction would normally involve a change to timeCreated and/or timePasswordChanged so not updating timeLastUsed doesn't really hide usage of a site. Of course we could avoid writing all timestamps in private windows but I think UI and potentially other codes assumed timeCreated is a real timestamp.

I'm looking at onFormSubmit in LoginManagerParent.jsm: https://searchfox.org/mozilla-central/source/toolkit/components/passwordmgr/LoginManagerParent.jsm#317 and one solution would be a check in recordLoginUse to see if PrivateBrowsingUtils.isBrowserPrivate(target) ?

I think that would align with my answer above, right?

Comment 6

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1520960 - Allow login capture from HTTP auth prompts in private browsing when pref'd on. r?MattN

Depends on D18409

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b4645dc802f9
Allow login capture from form submissions in private browsing when pref'd on. r=MattN

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5656f8b5c547
Allow login capture from HTTP auth prompts in private browsing when pref'd on. r=MattN

Comment 9

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/b4645dc802f9
https://hg.mozilla.org/mozilla-central/rev/5656f8b5c547