1361364 - Implement chrome.privacy.services.passwordSavingEnabled

Status: RESOLVED FIXED

(WebExtensions :: Compatibility, enhancement, P3)

Description

[Roman Taraban]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.81 Safari/537.36

Comment 1

[Roman Taraban]

Implement chrome.privacy.services as described here https://developer.chrome.com/extensions/privacy#property-services

chrome.privacy.services.passwordSavingEnabled is used to disable/enable build-in password saving functionality, which is quite useful for password manager extensions. This helps to provide a better UX, by not storing password in multiple places and not show password saving UI when the saving action is handled by an extension.

Comment 2

[:shell escalante]

jsut NI bob to check if there isn't already a bug for this... since youve looked at a lot of this code

Comment 3

[Bob Silverberg [:bsilverberg]]

No, there is no existing bug for this. It falls into the bucket of all of the parts of the privacy API which we didn't implement and were waiting to see if anyone requested them.

As this is just asking for privacy.services.passwordSavingEnabled, I have updated the title.

Comment 4

[Bob Silverberg [:bsilverberg]]

It looks like we can control this by flipping the pref `signon.rememberSignons`. Does that sound accurate, Matt? Is there anything else we should consider around creating an API that disables password saving?

Comment 5

[Matthew N. [:MattN]]

(In reply to Bob Silverberg [:bsilverberg] from comment #4)
> It looks like we can control this by flipping the pref
> `signon.rememberSignons`. Does that sound accurate, Matt?

Yes, right now that controls both prompting to save and filling saved logins (though I swear it used to only affect saving). Bug 1344657 is wanting to change to only affecting saving but I don't think that will change given new plans/direction for password manager that are coming.

> Is there anything else we should consider around creating an API that disables password saving?

Keep in mind that it doesn't clear the saved logins (which avoids data-loss for users).

There are new password manager plans in progress but I suspect there would still be a way to opt-out of saving.

Comment 6

[Bob Silverberg [:bsilverberg]]

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

Thanks for your quick response, Matt.
> 
> Yes, right now that controls both prompting to save and filling saved logins
> (though I swear it used to only affect saving). Bug 1344657 is wanting to
> change to only affecting saving but I don't think that will change given new
> plans/direction for password manager that are coming.
> 
> > Is there anything else we should consider around creating an API that disables password saving?
> 
> Keep in mind that it doesn't clear the saved logins (which avoids data-loss
> for users).
>

That is what we would expect, so no problem there.

> There are new password manager plans in progress but I suspect there would
> still be a way to opt-out of saving.

Great, it sounds like we are okay to implement this as described above for now.

Comment 7

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1361364 - Implement chrome.privacy.services.passwordSavingEnabled,

Review commit: https://reviewboard.mozilla.org/r/142862/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142862/

Comment 8

[Andrew Swan [:aswan]]

signon.rememberSignons is directly linked to a checkbox in about:preferences and is therefore easily changeable by users.  ExtensionsPreferecesManager.jsm is not designed to work with preferences that might be changed at any time by something other than an extension...

I'm not sure how we should handle this, perhaps a good starting point would be a proposal for how we should handle all the relevant sequences of events.  For example:
- what happens if the user has manually disabled saved passwords and an extension attempts to enable them?
- what happens if the user installs an extension that disables them then the user attmpts to manually enable them?
- in the previous case, what happens if the extension is subsequently disabled?

Or, to approach this from a different direction, how do we expect extensions to use this?  Is it just used by password managers so that their password saving capability and the builtin one aren't active simultaneously?  The ideal long-term solution there would be bug 1362981 but that's not going to be ready for some time.

Comment 9

[Andrew Swan [:aswan]]

Comment on attachment 8871385 [details]
Bug 1361364 - Implement chrome.privacy.services.passwordSavingEnabled,

https://reviewboard.mozilla.org/r/142862/#review147202

Clearing the review flag until the issue raised directly in bugzilla is sorted out.

Comment 10

[Bob Silverberg [:bsilverberg]]

(In reply to Andrew Swan [:aswan] from comment #8)
> signon.rememberSignons is directly linked to a checkbox in about:preferences
> and is therefore easily changeable by users. 
> ExtensionsPreferecesManager.jsm is not designed to work with preferences
> that might be changed at any time by something other than an extension...
> 

Yes, this seems to be coming up again and again so I think we need to decide whether:

a) We simply will not allow WebExtensions to change any settings that could be changed easily (i.e., via the browser UI), or
b) We come up with a scheme to handle users changing settings directly which can also be changed by WebExtensions.

While a) is certainly simpler, it also seems like it may be too limiting. I do have an idea of how we address b) using the existing PreferencesManager code, with some minor changes, but I'll start by discussing the cases you listed below.

> I'm not sure how we should handle this, perhaps a good starting point would
> be a proposal for how we should handle all the relevant sequences of events.
> For example:
> - what happens if the user has manually disabled saved passwords and an
> extension attempts to enable them?

In this case I think the extension should be able to enable them. If that is not the case then the extension is virtually useless. One would hope that the extension would make it clear to the user in some way that it is going to do this, and by installing the extension the user is giving permission for it to happen. Maybe we need even finer grained permissions for some of these things (e.g., rather than a permission for "privacy" it would be for "privacy - passwordSaving").

I can also see the argument for preventing this. If a user has specifically set password saving to disabled maybe an extension should not be allowed to change that. It would be up to the user to change it back. But that seems a lot less useful from an extension's perspective.

> - what happens if the user installs an extension that disables them then the
> user attmpts to manually enable them?

In this case they would become enabled. I think that's a given. Any changes made directly by a user should take effect immediately. We would want to be able to record this in a way that is understandable to the extension ecosystem so that things don't get out of sync though, and that's not something that is currently happening.

> - in the previous case, what happens if the extension is subsequently
> disabled?

In this case nothing should happen, as the user-set preference should stick. If we record the user-set preference in a way that the ExtensionPreferencesManager can recognize then this should work.

My off-the-top-of-my-head idea for doing this is to do the following:
- For specific settings that can be changed by users, we find a way of watching for those changes. Ideally this would be linked to the functions that are called behind the UI, so we only watch for changes made directly by users, and are not catching changes made by other extensions.
- When we catch a change to a setting by a user, we clear out the precedence queue for that setting, effectively removing all settings previously set by extensions. I think this makes sense because there is no way for a user-set setting to be automatically backed out (as can happen to extension-set settings when extensions are uninstalled or disabled). So we're basically starting from scratch again. This will also mean that the next time an extension tries to set a setting the initial value will be recorded based on the user-set value, which should be correct.

> 
> Or, to approach this from a different direction, how do we expect extensions
> to use this?  Is it just used by password managers so that their password
> saving capability and the builtin one aren't active simultaneously?  The
> ideal long-term solution there would be bug 1362981 but that's not going to
> be ready for some time.

That is a worthwhile question to ask, although I'm not sure it's possible to answer. While we may assume that extensions will be using this for that purpose, once we've made it available an extension could use it for whatever purpose they choose, so we still need to make sure it will work in any conceivable case.

Maybe this bug isn't the best place to discuss this, but we've already started so we might as well keep doing it for now. Andrew, what do you think of my answers to your cases and my suggested approach?

Comment 11

[Bob Silverberg [:bsilverberg]]

We discussed the general issue some more in today's meeting, so I am going to move the conversation over to a new bug and will redirect my needinfo there as well.

Comment 12

[Bob Silverberg [:bsilverberg]]

Now that bug 1368545 has landed, I think it is safe to proceed with this. We will want to address the UI issue, which will likely be done via changes to about:preferences, and is being discussed as part of Project Jazz, but that shouldn't block landing this.

Comment 13

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8871385 [details]
Bug 1361364 - Implement chrome.privacy.services.passwordSavingEnabled,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142862/diff/1-2/

Comment 14

[Andrew Swan [:aswan]]

Comment on attachment 8871385 [details]
Bug 1361364 - Implement chrome.privacy.services.passwordSavingEnabled,

https://reviewboard.mozilla.org/r/142862/#review154668

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ff101886def
Implement chrome.privacy.services.passwordSavingEnabled, r=aswan

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5ff101886def

Comment 17

[Will Bamberg [:wbamberg]]

Docs: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/privacy/services

Please let me know if we need anything else here.

Comment 18

[Bob Silverberg [:bsilverberg]]

Looks good, thanks Will.