[meta] Implement Password Manager for fxos

ASSIGNED
Assigned to
(NeedInfo from)

Status

Firefox OS
Gaia::Browser
P3
normal
ASSIGNED
4 years ago
a year ago

People

(Reporter: Matěj Cepl, Assigned: arcturus, NeedInfo)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, 4 keywords)

unspecified
2.6 S6 - 1/29
ARM
Gonk (Firefox OS)
feature, foxfood, productwanted, uiwanted
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: c=browser u=user [UCID: Browser10, FT:Browser, KOI:P2])

(Reporter)

Description

4 years ago
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.
This will require new platform support but adding to backlog for prioritisation.
Whiteboard: c=browser u=user
Keywords: feature
Priority: -- → P1

Comment 2

4 years ago
Is this the same as bug 879250?
Duplicate of this bug: 879250
Flags: needinfo?(ibarlow)
Keywords: productwanted, uiwanted

Comment 4

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

Updated

4 years ago
Duplicate of this bug: 889603
Whiteboard: c=browser u=user → c=browser u=user [UCID: Browser10, FT:Browser, KOI:P2]
Not just store, but also to delete the password if they decide that they no longer want it stored.

Comment 7

4 years ago
Making it easier to find this in Bugzilla ;-)
Summary: storing credentials from the visited web pages → storing credentials from the visited web pages (Password Manager)
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.
A related feature is syncing passwords between devices: bug 824026.

Updated

3 years ago
Flags: needinfo?(ibarlow)
Duplicate of this bug: 963909
(Reporter)

Comment 11

3 years ago
(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.
Blocks: 1050574
No longer blocks: 824026
Component: Gaia::Browser → Gaia::Keyboard
Depends on: 701634
No longer depends on: 877535
Keywords: feature, productwanted, uiwanted
OS: Linux → Mac OS X
Priority: P1 → --
Hardware: x86_64 → x86
Summary: storing credentials from the visited web pages (Password Manager) → Allow built-in keyboard app download dictionary dynamically
Whiteboard: c=browser u=user [UCID: Browser10, FT:Browser, KOI:P2]

Comment 12

3 years ago
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.
Blocks: 824026
No longer blocks: 1050574
Component: Gaia::Keyboard → Gaia::Browser
Depends on: 877535
No longer depends on: 701634
Keywords: feature, productwanted, uiwanted
OS: Mac OS X → Gonk (Firefox OS)
Priority: -- → P1
Hardware: x86 → ARM
Summary: Allow built-in keyboard app download dictionary dynamically → storing credentials from the visited web pages (Password Manager) → Allow built-in keyboard app download dictionary dynamically
Whiteboard: c=browser u=user [UCID: Browser10, FT:Browser, KOI:P2]
Summary: storing credentials from the visited web pages (Password Manager) → Allow built-in keyboard app download dictionary dynamically → storing credentials from the visited web pages (Password Manager)
Duplicate of this bug: 1033355
[Blocking Requested - why for this release]:
blocking-b2g: --- → 3.0?
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
Assignee: nobody → dale
Flags: needinfo?(ttaubert)
Flags: needinfo?(margaret.leibovic)
fwiw, I'd like us to try to implement that as an add-on.

Comment 17

2 years ago
(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
Flags: needinfo?(ttaubert)
Flags: needinfo?(margaret.leibovic)
Flags: needinfo?(MattN+bmo)
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.
Flags: needinfo?(MattN+bmo)
blocking-b2g: 3.0? → ---
feature-b2g: --- → 3.0+
Whiteboard: c=browser u=user [UCID: Browser10, FT:Browser, KOI:P2] → c=browser u=user [UCID: Browser10, FT:Browser, KOI:P2][systemsfe]
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?
Flags: needinfo?(fabrice)
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.
Flags: needinfo?(fabrice)
Target Milestone: --- → FxOS-S4 (07Aug)
Target Milestone: FxOS-S4 (07Aug) → FxOS-S5 (21Aug)
feature-b2g: 3.0+ → 2.5+
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
Flags: needinfo?(fabrice)
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.
Flags: needinfo?(fabrice)

Updated

2 years ago
Target Milestone: FxOS-S5 (21Aug) → FxOS-S6 (04Sep)
Happy to provide security input throughout the design and implementation phase.
Duplicate of this bug: 1178057
Keywords: foxfood
Flags: needinfo?(tshakespeare)
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.
Flags: needinfo?(fabrice)
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?
Target Milestone: FxOS-S6 (04Sep) → ---
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

2 years ago
You removed the target milestone without explaining why, so I guess that was unintended and I'm putting it back. :)
Target Milestone: --- → FxOS-S6 (04Sep)
(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.
Flags: needinfo?(fabrice)
(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).
Adding needinfo, Fabrice is sendMessage something scheduled for 2.5?
Flags: needinfo?(fabrice)

Comment 32

2 years ago
+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.
> 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.
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?
Flags: needinfo?(fabrice)
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
This feature was always a nice to have for 2.5 and it's very unlikely to be ready for the 11/2 deadline.
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
Flags: needinfo?(fabrice)
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.
Flags: needinfo?(fabrice)
Depends on: 1211401
Blocks: 1210928
Summary: storing credentials from the visited web pages (Password Manager) → [meta] Implement Password Manager for fxos
Depends on: 1212743
feature-b2g: 2.5+ → ---
Target Milestone: FxOS-S6 (04Sep) → FxOS-S11 (13Nov)
Priority: P1 → P3
Depends on: 1225463
No longer depends on: 1225463
Target Milestone: FxOS-S11 (13Nov) → 2.6 S2 - 12/4
Target Milestone: 2.6 S2 - 12/4 → 2.6 S4 - 1/1
Status: NEW → ASSIGNED
Target Milestone: 2.6 S4 - 1/1 → 2.6 S5 - 1/15
Talked to Fernando and Michael about this but deassigning myself for now
Assignee: dale → nobody
Status: ASSIGNED → NEW
Depends on: 1237294
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
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
Flags: needinfo?(ptheriault)
Flags: needinfo?(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.
Flags: needinfo?(francisco)
(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...
(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.
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.
Depends on: 1239701
(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.
Flags: needinfo?(ptheriault)
Target Milestone: 2.6 S5 - 1/15 → 2.6 S6 - 1/29
Assignee: nobody → francisco
Status: NEW → ASSIGNED
No longer depends on: 1239701
Whiteboard: c=browser u=user [UCID: Browser10, FT:Browser, KOI:P2][systemsfe] → c=browser u=user [UCID: Browser10, FT:Browser, KOI:P2]
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
This sounds like a good approach!
Let us know when you need someone from security to look over your implementation.
You need to log in before you can comment on or make changes to this bug.