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

RESOLVED FIXED in Firefox 67

Status

()

P2
enhancement
RESOLVED FIXED
2 months ago
a month ago

People

(Reporter: MattN, Assigned: sfoster)

Tracking

Trunk
mozilla67
Points:
---

Firefox Tracking Flags

(firefox67 fixed)

Details

(Whiteboard: [passwords:capture-UI])

Attachments

(2 attachments)

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

(Assignee)

Updated

2 months ago
Assignee: nobody → sfoster
Status: NEW → ASSIGNED
Whiteboard: [passwords:capture-UI]
(Assignee)

Comment 1

2 months ago

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?

(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.

(Assignee)

Comment 4

2 months ago

(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) ?

Flags: needinfo?(MattN+bmo)

(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?

Flags: needinfo?(MattN+bmo)
Attachment #9040859 - Attachment description: Bug 1520960 - Allow login capture in private browsing when pref'd on. r?MattN → Bug 1520960 - Allow login capture from form submissions in private browsing when pref'd on. r?MattN

Comment 7

a month ago
Pushed by sfoster@mozilla.com:
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

a month ago
Pushed by sfoster@mozilla.com:
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

a month ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.