877648 - [meta] Implement Password Manager for fxos

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::Browser, defect, P3)

Description

[Matěj Cepl]

Before doing anything fancy (like syncing credentials via Firefox Sync, which is bug 824026), could we use API designed in bug 877535 and just store credentials from logged-in webpages? Not having this in Browser at all in this time and age feels crazy.

Comment 1

[Ben Francis [:benfrancis]]

This will require new platform support but adding to backlog for prioritisation.

Comment 2

[Robert Kaiser]

Is this the same as bug 879250?

Comment 4

[ffos-browser]

A Pivotal Tracker story has been created for this Bug: http://www.pivotaltracker.com/story/show/53548691

Comment 6

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Not just store, but also to delete the password if they decide that they no longer want it stored.

Comment 7

[Robert Kaiser]

Making it easier to find this in Bugzilla ;-)

Comment 8

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Adding links to recent discussions on this feature on Gaia and B2G lists (please avoid cross-posting in the future people!)

https://groups.google.com/d/msg/mozilla.dev.gaia/rd-TDukXiKA/BtlhZH3y3NQJ
https://groups.google.com/d/msg/mozilla.dev.b2g/gblgbPwD0b0/tbwmz2vhRQIJ

NB: This feature has been added to the backlog of Firefox Accounts. Just fyi in case any one was thinking of working on this.

Comment 9

[Paul Theriault [:pauljt] (no longer reading bugmail)]

A related feature is syncing passwords between devices: bug 824026.

Comment 11

[Matěj Cepl]

(In reply to Paul Theriault [:pauljt] from comment #9)
> A related feature is syncing passwords between devices: bug 824026.

This is very unfortunate to bundle this into a long time project like Firefox Accounts. Note in my initial description (notice also the age of the bug):

> Before doing anything fancy (like syncing credentials via Firefox Sync,

I hoped that Browser could not feel like a crippleware even before something large like Firefox Accounts will be done. Now it just dropped to the bottom of the Firefox Account bucket, and so it is mostly meaningless, because when FA come, eventually, it will be fixed by itself.

Comment 12

[Robert Kaiser]

I take it those bug field changes were unintentional, so restoring the old fields. And even if they were intentional, for changes this huge, a new bug should be filed.

Comment 14

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

[Blocking Requested - why for this release]:

Comment 15

[Dale Harvey (:daleharvey)]

Hey Tim / Margaret, is there any chance you could point this to someone who could give me a quick overview of how this works in desktop / android? Hoping there is some infrastructure to reuse here and have it implemented without blocking on fxa

Cheers

Comment 16

[[:fabrice] Fabrice Desré]

fwiw, I'd like us to try to implement that as an add-on.

Comment 17

[:Margaret Leibovic]

(In reply to Dale Harvey (:daleharvey) from comment #15)
> Hey Tim / Margaret, is there any chance you could point this to someone who
> could give me a quick overview of how this works in desktop / android?
> Hoping there is some infrastructure to reuse here and have it implemented
> without blocking on fxa

The logic to store passwords is shared between desktop/Android. Most of the code for that is in here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/

MattN knows this well, so he could probably give a good overview.

A version of this was also recently ported to Firefox for iOS, so maybe that would be easier to use as inspiration:
https://github.com/mozilla/firefox-ios/blob/acaa352f14b63f33f7b01ca495a445d1e4153d9f/Client/Assets/PasswordHelper.js

Comment 18

[Matthew N. [:MattN]]

Hey Dale,

You should be able to re-use everything except the UI for FxOS. I made a (WIP) diagram of the architecture at https://wiki.mozilla.org/Toolkit:Password_Manager#Architecture. The integration starts at the bottom-right of the diagram via event listeners that should work in FxOS too. I'll gladly discuss how you can integrate with the Toolkit password manager. You can find me in #passwords or via PM or you can schedule a meeting to discuss more details.

I wouldn't recommend using the fork for iOS as we're probably going to unfork that later this year and it would just make that harder and you wouldn't get the improvements that are currently being worked on.

Comment 19

[Dale Harvey (:daleharvey)]

Thats awesome, thanks Matthew. 

Fabrice, When you say implemented as an add on, do you mean one that hooks into the existing infrastructure or starting it from scratch? would the addon be listening to 'DomFormHasPassword' and communicating with LoginParentManager etc or how do you see it working?

Comment 20

[[:fabrice] Fabrice Desré]

I mean a b2g add-on. Right now they are just content scripts, which means that we can reuse a lot of the ios content script from https://github.com/mozilla/firefox-ios/blob/acaa352f14b63f33f7b01ca495a445d1e4153d9f/Client/Assets/PasswordHelper.js but we need a way for the add-on to at least send / receive messages from other places, eg. if we want to the UI in the system app.

For that we need our add-ons to have some superpowers to do more than the web page they are injected into. That should be possible by carefully adding features on the sandbox that we use, but I haven't tried yet.

Comment 21

[Dale Harvey (:daleharvey)]

Ok so playing around with this, there are 3 main distinct parts

 1. There is the content script that deals with the in page dom, it fills the inputs values when there is match and catches form submissions and finds details.

 2. There is the Storage manager that stores the logins, this has to be outside the content script because its lifecycle is longer than a single page load and it needs to be a singleton

 3. There is the UX inside the system app.

I think Fabrice your idea was that 2/3 live in the an addon, and 3 just has to be in the system app, right now as we dont have a way to do messaging in addons nor storage I believe? I am thinking we can do 1 as an addon and 2/3 in the system app, that way I can do the messaging with a dirty metachanges / mozbrowser hack and then when the addon messaging api gets implement we can move 2 to the addon. If we could figure out a way to have the addon show UX in the system app that would be super sweet.

How does that sounds Fabrice?

One issue I do seem to be having is when I update an addon, I do seem to have ghost instances of the old addon code lying around, but not a big deal right now

Comment 22

[[:fabrice] Fabrice Desré]

Works for me. Note that you can just use the add-on to inject whatever you want in the system app, including your ui.

By "updating" do you mean pushing a new version from webIDE? I guess that does the same thing as disabling/enabling, but we have no good way to rollback script injection in these cases.

Comment 23

[Frederik Braun [:freddy]]

Happy to provide security input throughout the design and implementation phase.

Comment 25

[Dale Harvey (:daleharvey)]

So I have a very hacky prototype of up @ https://github.com/daleharvey/fxospwdmgr, its currently an addon that runs 2 content scripts, one injected into the page and another into the system app.

The primary problem right now is the communication between the content scripts, the current implementation is an extremely hacky in that it sets the password back to the system app via a meta tag, and the system app send data back via |mozbrowser.executeScript|, we either need to not do it in an addon and create new mozbrowser apis for the interactions the password manager needs, or we need to support https://developer.chrome.com/extensions/runtime#method-connectNative

Fabrice do you know of any plans for |sendMessage| support in addons, or at least who to ask?

In parallel I will take a look at integrating Firefox Sync into that as it will have a big effect on how we store the data.

Comment 26

[Fernando Jiménez Moreno [:ferjm]]

Sorry to jump this late into the discussion, but what are the benefits of implementing all of this as an add-on vs implementing the missing UI pieces for FxOS to use the Gecko PasswordManager as Matthew suggests on comment 18? In other words, why not reusing the existing code?

Comment 27

[Dale Harvey (:daleharvey)]

Will also leave that as an open question for Fabrice, looking at the architecture it seems like pretty much all of the child process stuff would need reimplemented inside BrowserElementChild.js in gecko, I am not sure we use a whole lot of the same parent process infrastructure either.

Comment 28

[Robert Kaiser]

You removed the target milestone without explaining why, so I guess that was unintended and I'm putting it back. :)

Comment 29

[[:fabrice] Fabrice Desré]

(In reply to Dale Harvey (:daleharvey) from comment #25)
> 
> Fabrice do you know of any plans for |sendMessage| support in addons, or at
> least who to ask?

Yes, we definitely plan to support sendMessage(). We're currently blocked on some refactoring of the WebExtensions test infrastructure to share it between desktop and b2g.

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #26)
> Sorry to jump this late into the discussion, but what are the benefits of
> implementing all of this as an add-on vs implementing the missing UI pieces
> for FxOS to use the Gecko PasswordManager as Matthew suggests on comment 18?
> In other words, why not reusing the existing code?

I'd like this kind of feature to be more plugable than something baked into the platform. Users should have the choice to enable/disable our password manager or to use an alternative one. There are a few available as WebExtensions for instance that are viable alternatives.

Comment 30

[Fernando Jiménez Moreno [:ferjm]]

(In reply to [:fabrice] Fabrice Desré from comment #29)
> (In reply to Dale Harvey (:daleharvey) from comment #25)
> > 
> > Fabrice do you know of any plans for |sendMessage| support in addons, or at
> > least who to ask?
> 
> Yes, we definitely plan to support sendMessage(). We're currently blocked on
> some refactoring of the WebExtensions test infrastructure to share it
> between desktop and b2g.
> 

Is this planned for 2.5?

We are looking forward to have passwords synchronization on 2.5 and we depend on the passwords manager for that.

> (In reply to Fernando Jiménez Moreno [:ferjm] from comment #26)
> > Sorry to jump this late into the discussion, but what are the benefits of
> > implementing all of this as an add-on vs implementing the missing UI pieces
> > for FxOS to use the Gecko PasswordManager as Matthew suggests on comment 18?
> > In other words, why not reusing the existing code?
> 
> I'd like this kind of feature to be more plugable than something baked into
> the platform. Users should have the choice to enable/disable our password
> manager or to use an alternative one. There are a few available as
> WebExtensions for instance that are viable alternatives.

I think users can already do that on Desktop and Android. We just need a setting to disable the built in password manager (probably easier for the user to understand than disabling an add-on).

Comment 31

[Dale Harvey (:daleharvey)]

Adding needinfo, Fabrice is sendMessage something scheduled for 2.5?

Comment 32

[Aaron Wu]

+Joe Chang, TV PM

Echo Fernando, we did plan and commit to have passwords synchronization on 2.5. Especially as we mentioned that TV Team required sync features for their upcoming commercial launch, including Bookmark, History and Password. We better to have code complete by 11/2, please raise up any risk or help on password sync on 2.5, since TV team also has committed this to Panasonic partner.

Comment 33

[Dale Harvey (:daleharvey)]

> We just need a setting to disable the built in password
> manager (probably easier for the user to understand than disabling an add-on).

It being an addon makes no assumption on what UI is presented, similiar to say "Pocket" integration in desktop, we can instrument UI in the addon or we can put small hooks of UI into core with the functionality implemented in the addon.

> Echo Fernando, we did plan and commit to have passwords synchronization on 2.5. 
> Especially as we mentioned that TV Team required sync features for their upcoming commercial launch

Sorry but this is not 'committed' We aimed to have this for 2.5 but said it would ride the train, I can / will not promise it will be done and making architectural decisions that effect the quality of the feature based on giving arbritrary commitments to partners is exactly the opposite of what we said we would be doing in Whistler.

I will take more of a look at hooking into the current gecko implementation of the password manager but based on my limited understanding I mostly agree with fabrice at this moment that it is far cleaner to have this implemented as an addon than to go via chrome events and adding a bunch of adhoc api's baked into the platform.

Comment 34

[[:fabrice] Fabrice Desré]

I can't commit for add-on support for sendMessage() because I haven't tested that at all yet.

However I'm quite sure that the level of effort needed to implement an add-on for this feature is not more than doing it "from scratch" using the gecko backend. And we can always ship the add-on later, unless the integration with sync is not doable from an add-on?

Comment 35

[Dale Harvey (:daleharvey)]

So I figured I could use chrome to build out the prototype, however chrome doesnt let content_scripts use sendMessage from arbitrary web pages so without our own implementation that has that ability then hard blocked on the pure addon side.

Going to work on hooking into the existing gecko side implementation, I think it will make syncing harder, but there isnt much else to try that I can think of

Comment 36

[Gregor Wagner [:gwagner]]

This feature was always a nice to have for 2.5 and it's very unlikely to be ready for the 11/2 deadline.

Comment 37

[Dale Harvey (:daleharvey)]

Yo Fabrice, so talking in here is probably easier than trying to catch each other on IRC

So sendMessage is a blocker for the addon route as it currently is, and even with sendMessage implemented as in Chrome we would need extra permission to be able to sendMessage from arbitrary content_scripts. It would be used to communicate between the content_script and the password manager add on and I dont know of any other way to do that. So either there is nothing to do until sendMessage is done or there is another way to do that communication.

Alternatively, there is reusing the existing gecko implementation, triggering the UX from gecko as well as talking to the sync engine is not likely be be the easiest or cleanest thing in the world, to keep adding this type of functionality via tightly coupled gecko components is clearly less than ideal, but it is already there and not blocked. 

Right now I am working on the gecko implementation since thats about the only thing I can do at the moment but if there is anything else you think I should be doing then a good time to say, I am likely not gonna be experiences enough with gecko internals to implement sendMessage but I might be able to get enough to prototype working with some pointers.

Cheers

Comment 38

[[:fabrice] Fabrice Desré]

Which extra permission would you need once we have tabs.sendMessage() ?
If you have time to spend doing gecko hacking, help us implement add-ons apis instead of gluing the gecko password manager to gaia. We need to implement the tabs api, adapting the desktop version from https://mxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-tabs.js to b2g. You can look at bug 1199504 for the overall setup to ship b2g specific api implementations.

Comment 39

[Dale Harvey (:daleharvey)]

Talked to Fernando and Michael about this but deassigning myself for now

Comment 40

[Dale Harvey (:daleharvey)]

Francisco mentioning picking this up, leaving details of whats been done so far

https://github.com/daleharvey/fxospwdmgr/ is an initial prototype developed as an addon using webExtensions apis (Fabrice strongly suggested building it that way), it should work fine in firefox desktop and in theory should work fine on fxos however when I tested it I ran into the above bug, once that is fixed it should hopefully work. There may be some additional UX wanted, ie to clear currently stored passwords.

With it working we need to hook it into the sync adapter so people can sync their passwords, currently we have a sync application that shares data via datastores, however that does seem a little risky for passwords so its likely we will want to embed the sync code directly into the addon so the addon is the only one that needs to touch the passwords

Comment 41

[Dale Harvey (:daleharvey)]

Pinging Francisco and Paul mostly about ^ but also Paul mentioned that security may have resources to also work on this, and will definitely need to be included at the least to review so figured would ping you both to coordinate. Cheers

Comment 42

[Francisco Jordano [:arcturus] [:francisco]]

Talking with Fernando, and the experienced we had with the folks implementing the DataStore api, seems this api wont be privileged, ever.

So to me, unless Paul says the opposite, we could have a first implementation using DS.

Comment 43

[Chris Lord [:cwiiis]]

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #42)
> Talking with Fernando, and the experienced we had with the folks
> implementing the DataStore api, seems this api wont be privileged, ever.
> 
> So to me, unless Paul says the opposite, we could have a first
> implementation using DS.

The datastore is open to privileged homescreens - I'd really recommend against using it for this unless you can somehow add an exception there...

Comment 44

[Francisco Jordano [:arcturus] [:francisco]]

(In reply to Chris Lord [:cwiiis] from comment #43)
> (In reply to Francisco Jordano [:arcturus] [:francisco] from comment #42)
> > Talking with Fernando, and the experienced we had with the folks
> > implementing the DataStore api, seems this api wont be privileged, ever.
> > 
> > So to me, unless Paul says the opposite, we could have a first
> > implementation using DS.
> 
> The datastore is open to privileged homescreens - I'd really recommend
> against using it for this unless you can somehow add an exception there...

Ugh! Good to know, but definitely a homescreen could potentially change or know what are your in case of emergency numbers (or even delete them :P)

We will need to look for a solution then.

Comment 45

[Francisco Jordano [:arcturus] [:francisco]]

Just created bug 1239701, to allow a way of defining datastores just accessible by certified apps. So we can prevent any data loss by homescreen apps.

Comment 46

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Dale Harvey (:daleharvey) from comment #40)
> Francisco mentioning picking this up, leaving details of whats been done so
> far
> 
> https://github.com/daleharvey/fxospwdmgr/ is an initial prototype developed
> as an addon using webExtensions apis (Fabrice strongly suggested building it
> that way), it should work fine in firefox desktop and in theory should work
> fine on fxos however when I tested it I ran into the above bug, once that is
> fixed it should hopefully work. There may be some additional UX wanted, ie
> to clear currently stored passwords.
> 
> With it working we need to hook it into the sync adapter so people can sync
> their passwords, currently we have a sync application that shares data via
> datastores, however that does seem a little risky for passwords so its
> likely we will want to embed the sync code directly into the addon so the
> addon is the only one that needs to touch the passwords

Thanks for the heads up Dale. Definitely need to discuss data at rest protection (i.e. encrypting the passwords with a key derived from the lockcode) further as that's the obvious missing piece.

Comment 47

[Fernando Jiménez Moreno [:ferjm]]

We decided to take a different approach for the PM data adapter. Basically we are not going to use DataStore for it, so the PM can store passwords in a local IDB. We want to make the PM addon to inject a content script in the Sync app exposing a chrome.runtime.sendMessage based API to access and modify the content stored by the PM background script. We can limit the access to this API by injecting this script only in the Sync app and by checking the sender [1] when handling messages from this API.

[1] https://developer.chrome.com/extensions/runtime#type-MessageSender

Comment 48

[Frederik Braun [:freddy]]

This sounds like a good approach!
Let us know when you need someone from security to look over your implementation.

Comment 49

[BMO Automation]

Firefox OS is not being worked on