Closed Bug 646649 Opened 9 years ago Closed 9 years ago

Passwords API should segregate add-ons' credentials

Categories

(Add-on SDK Graveyard :: General, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: wbamberg, Assigned: irakli)

Details

One of the uses of the Passwords API is for add-ons to store credentials which are associated with the add-on, rather than with a particular web site, so the add-on can access them in subsequent sessions.

Credentials such as these should not be visible to other add-ons, but they currently are. To reproduce, see: https://bugzilla.mozilla.org/show_bug.cgi?id=642335#c7 (comment 7, bug 642335).

The idea is that the add-on ID should be used to generate a URL for the credential which is unique to the add-on.
This is going to be a breaking change (our second in the 1.0 beta phase), but it's important to do.
Priority: -- → P1
Hardware: x86 → All
Target Milestone: --- → 1.0b5
Assignee: nobody → rFobic
Actually credentials are already stored with url's associated with the given addon, which has form of "about:{{addon-id}}"

Jetpack repl
js> require("passwords").store({ realm: 'yarr!!!', username: 'pirate', password: "SeCrEt123", onComplete: console.log.bind(null, 'stored') })
info: stored undefined
js> require("passwords").search({ url: require('self').uri, realm: 'yarr!!!', onComplete: function($) console.log(JSON.stringify($)) })
info: [{"url":"addon:jep4repl","realm":"yarr!!!","formSubmitURL":null,"username":"pirate","password":"SeCrEt123","usernameField":"","passwordField":""}]
js> 

That being said other add-ons still can discover each others credentials, but I don't think it's a bug.

I guess it will be useful to either show example of search where url is passed as require('self').uri
(In reply to comment #2)
> Actually credentials are already stored with url's associated with the given
> addon, which has form of "about:{{addon-id}}"

Thanks Irakli. I had figured out addon:{{jid}} but didn't know about self.uri. I suppose that should be documented too. Yes, the passwords doc should have an example of its use, I'll include it.
(In reply to comment #2)

> Actually credentials are already stored with url's associated with the
> given addon, which has form of "about:{{addon-id}}"

Huh, that's kinda weird.. it looks like any use of the require("passwords")
API that doesn't explicitly provide a ".url" property will use that
about:$JID URL as a default, which could be surprising.. maybe we should make
that more explicit?

> That being said other add-ons still can discover each others credentials,
> but I don't think it's a bug.

Huh? If the goal is to let add-ons keep secrets from each other, that does
sound like a bug. JIDs are public knowledge, so these URLs are too.

What if we say that the full-powered "passwords" API lets an add-on get to
everything in the password manager (including data from other addons), but
there's a less-powerful API (implemented as a wrapper on top of "passwords")
that doesn't. There could be a module that provides just addon-local secrets.
Maybe a different sort of module that gives access just to passwords for a
particular URL prefix/site. (this sounds related to the "module limiter"
stuff we've talked about in the context of restricting the "request" module
to a single site)

> I guess it will be useful to either show example of search where url is
> passed as require('self').uri

True. Does anything useful happen when you type that "addon:$JID" URL into
the address bar? Should it?
(In reply to comment #4)
> (In reply to comment #2)
> 
> > Actually credentials are already stored with url's associated with the
> > given addon, which has form of "about:{{addon-id}}"
> 
> Huh, that's kinda weird.. it looks like any use of the require("passwords")
> API that doesn't explicitly provide a ".url" property will use that
> about:$JID URL as a default, which could be surprising.. maybe we should make
> that more explicit?
> 
> > That being said other add-ons still can discover each others credentials,
> > but I don't think it's a bug.
> 
> Huh? If the goal is to let add-ons keep secrets from each other, that does
> sound like a bug. JIDs are public knowledge, so these URLs are too.
> 

I don't think that was a goal, to my understanding goal was exposing native login manager in a high level API, which gives access to all the passwords, including ones saved by other addons.

> What if we say that the full-powered "passwords" API lets an add-on get to
> everything in the password manager (including data from other addons), but
> there's a less-powerful API (implemented as a wrapper on top of "passwords")
> that doesn't. There could be a module that provides just addon-local secrets.
> Maybe a different sort of module that gives access just to passwords for a
> particular URL prefix/site. (this sounds related to the "module limiter"
> stuff we've talked about in the context of restricting the "request" module
> to a single site)
> 

Current "passwords" API is similar to "request" in that sense, so once "module limiter" will be on the table we can definitely work on this as well. 

> > I guess it will be useful to either show example of search where url is
> > passed as require('self').uri
> 
> True. Does anything useful happen when you type that "addon:$JID" URL into
> the address bar? Should it?

Not unless add-on's register "addon:$JID" protocol. I was hoping to add high level API in a future for that, which can be used by addons to create preference pages etc for their addons.
(In reply to comment #3)
> (In reply to comment #2)
> > Actually credentials are already stored with url's associated with the given
> > addon, which has form of "about:{{addon-id}}"
> 
> Thanks Irakli. I had figured out addon:{{jid}} but didn't know about self.uri.
> I suppose that should be documented too. Yes, the passwords doc should have an
> example of its use, I'll include it.

Will can we close this bug then, or you want to add examples to docs under this bug ? If so maybe you can reassign it to yourself ?
(In reply to comment #6)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > Actually credentials are already stored with url's associated with the given
> > > addon, which has form of "about:{{addon-id}}"
> > 
> > Thanks Irakli. I had figured out addon:{{jid}} but didn't know about self.uri.
> > I suppose that should be documented too. Yes, the passwords doc should have an
> > example of its use, I'll include it.
> 
> Will can we close this bug then, or you want to add examples to docs under this
> bug ? If so maybe you can reassign it to yourself ?

Yes, if this kind of segregation isn't a requirement we can close it, and I'll document the use of "addon:jid" in bug 646865.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → INVALID
(In reply to comment #5)
> I don't think that was a goal, to my understanding goal was exposing native
> login manager in a high level API, which gives access to all the passwords,
> including ones saved by other addons.

This is incorrect.  The Passwords API was intended to satisfy two use cases:

1. Retrieval of stored credentials for web sites.
2. Storage and retrieval of an addon's own credentials.

It was not intended to provide retrieval of other addons' stored credentials, and it should not do so.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
(In reply to comment #8)
> (In reply to comment #5)
> > I don't think that was a goal, to my understanding goal was exposing native
> > login manager in a high level API, which gives access to all the passwords,
> > including ones saved by other addons.
> 
> This is incorrect.  The Passwords API was intended to satisfy two use cases:
> 
> 1. Retrieval of stored credentials for web sites.
> 2. Storage and retrieval of an addon's own credentials.
> 
> It was not intended to provide retrieval of other addons' stored credentials,
> and it should not do so.

Then there are open questions. From the platform perspective any credential is associated to a URI which is either web site URL or something add-on used to store it's credentials. Below are my questions:

1. Should we forbid storing passwords only to http / https / ftp URLs or without url ? Cause if we allow storing passwords to the custom URIs one would expect to have a way to access them later (see 2. for details).
2. Should we limit search results to the http / https / ftp and blanck urls only ? This will make jetpackification of the existing addons harder as they no longer will be able to access their passwords.
Target Milestone: 1.0b5 → 1.0
(In reply to comment #9)
> 1. Should we forbid storing passwords only to http / https / ftp URLs or
> without url ? Cause if we allow storing passwords to the custom URIs one would
> expect to have a way to access them later (see 2. for details).

Yes, I think we should.  However, see below.


> 2. Should we limit search results to the http / https / ftp and blanck urls
> only ? This will make jetpackification of the existing addons harder as they no
> longer will be able to access their passwords.

That's a good point.  And we're well into the release cycle for the version, to that point where I'm reluctant to make significant breaking changes that aren't absolutely necessary.  *sigh*  Ok, let's leave this API as it is.

This makes it more of a low- than a high- level API, since it mostly mirrors the low-level functionality of the underlying Firefox service instead of providing the simplest possible high-level interface that satisfies the common use cases.

But we do need to make it easy for traditional extensions to migrate to the SDK, and they do need to access their saved credentials.

Perhaps this will turn out to be a good test case for the kind of module layering Brian describes in comment 4 and/or for static analysis tools that distinguish addons that use the full power of this API from those that only access their own credentials.
Resolving wontfix per comment 10.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.