1386427 - Basic implementation of storage.managed

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, enhancement, P2)

Description

[Mike Kaply [:mkaply]]

One of the things that is pretty common with partner add-ons in the past was providing ways for third party installers to provide campaign IDs and things like that.

They traditionally did it by writing a file to the profile directory or something like so the add-on could read that value.

In investigating ways to do this within WebExtensions, the best way I could come up with was to allow WebExtensions to read registry entries.

Perhaps this could be sandboxed somehow so that a WebExtension could only read entries within a particular location?

This is something that has come up with multiple partners.

Comment 1

[Tomislav Jovanovic :zombie]

I think we could use something similar to Native Messaging manifests for this use case, like what I'm proposing in bug 1357391 for PKCS#11.  

A manifest file installed by the native application, with JSON data needed by the extension and with permissions listing what extension can access it.

Comment 2

[Tomislav Jovanovic :zombie]

App manifest could go something like:

  {
    "name": "PartnerAddon",
    "description": "Example host for native messaging",
    "type": "data",
    "allowed_extensions": [ "[e-mail]" ],
    "data": {
      "campaignId": "1234567890-abcdef"
    }
  }

adapted from https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Native_messaging#App_manifest

Comment 3

[Mike Kaply [:mkaply]]

I like this idea a lot.

How does the extension read the manifest without having a native app to do the messaging?

Comment 4

[Andy McKay]

Just cc'ing security so they know about the ideas here.

Comment 5

[Tomislav Jovanovic :zombie]

(In reply to Mike Kaply [:mkaply] from comment #3)
> How does the extension read the manifest without having a native app to do
> the messaging?

For native messaging, the extensions calls runtime.connectNative("myAppName"), so by analogy we could implement runtime.readNativeAppData("myAppName"), which would just return the content of the manifest's data key.


(In reply to Andy McKay [:andym] from comment #4)
> Just cc'ing security so they know about the ideas here.

I don't think there are any security implications here, as the functionality proposed would be a strict subset of native messaging (in fact, it's just a convenience for the simplest form of a native messaging executable that only spews back static data).

Comment 6

[Tomislav Jovanovic :zombie]

Hey Andrew, can you please sanity-check my proposals from comment 1 above and bug 1357391 comment 39 (and 48), where I suggest using the same logic from NativeMessaging App Manifests for finding/reading stuff that native apps may install on the user's system.

Comment 7

[Andrew Swan [:aswan]]

This seems reasonable to me.  My only concrete suggestion is that naming needs some thought here.  There isn't (necessarily) a native application involved here, and the name "data" is way too vague, but I don't have a great idea for something better than isn't awkwardly long.
Also, the bug title should be updated, I was scratching my head trying to figure out what this had to do with the registry for a minute...

Comment 8

[Mike Kaply [:mkaply]]

Is this something that has the remotest potential to make 57? This is important for partners (including MozillaOnline)

Comment 9

[Tomislav Jovanovic :zombie]

Per discussion over irc.

Comment 10

[Andy McKay]

Although its not quite the same this feels similar to chrome.storage.managed which is bug 1230802. I'm reluctant to focus time on building an API to fit one particular use case only to then implement the other later on.

If we did go this route, could we mark it and document it as temporary pending chrome.storage.managed? Or can we start down the .managed route and keep it as simple and bare bones as possible for now?

Comment 11

[Mike Kaply [:mkaply]]

I'm just not clear on how chrome.storage.managed is available to everyone. The chrome docs say:

Items in the managed storage area are set by the domain administrator, and are read-only for the extension; trying to modify this namespace results in an error. 

And really give no indication of how these values are set on the machine.

Is there some documentation on this that I'm missing?

Comment 12

[YUKI "Piro" Hiroshi]

http://www.chromium.org/administrators/configuring-policy-for-extensions ?

Comment 13

[Tomislav Jovanovic :zombie]

Attachment: Bug 1386427 - Part 1: Add XRE*NativeManifests locations to dirsvc

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

Comment 14

[Tomislav Jovanovic :zombie]

Attachment: Bug 1386427 - Part 2: Extract HostManifestManager to NativeManifests.jsm

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

Comment 15

[Tomislav Jovanovic :zombie]

Attachment: Bug 1386427 - Part 3: Add `storage` and `pkcs11` NativeManifest types

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

Comment 16

[Tomislav Jovanovic :zombie]

Attachment: Bug 1386427 - Part 4: Implement basic storage.managed functionality

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

Comment 17

[Tomislav Jovanovic :zombie]

Comment on attachment 8909061 [details]
Bug 1386427 - Part 3: Add `storage` and `pkcs11` NativeManifest types

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

Comment 18

[Tomislav Jovanovic :zombie]

Comment on attachment 8909062 [details]
Bug 1386427 - Part 4: Implement basic storage.managed functionality

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

Comment 19

[Tomislav Jovanovic :zombie]

(In reply to Andy McKay [:andym] from comment #10)
> Or can we start down the .managed route and
> keep it as simple and bare bones as possible for now?

After a few discussions over irc, this was deemed as the best solution.

The implementation is still fairly similar to what I proposed in comment 1, though exposed via the storage.managed API.

This first iteration (scoped for 57) is different/missing a few parts from Chrome:
 - no onChanged events, managed storage is read only once on startup (or actually when first requested).
 - no platform-specific storage backends (like registry on Windows or MCX preferences bundles on OSX).
 - no enforcing of extension-provided managed storage JSON schema.

All of these could be revisited and implemented later.

Comment 20

[Kris Maglione [:kmag]]

Comment on attachment 8909060 [details]
Bug 1386427 - Part 2: Extract HostManifestManager to NativeManifests.jsm

https://reviewboard.mozilla.org/r/180658/#review185840

::: toolkit/components/extensions/NativeManifests.jsm:9
(Diff revision 1)
>  "use strict";
>  
> -this.EXPORTED_SYMBOLS = ["HostManifestManager", "NativeApp"];
> +this.EXPORTED_SYMBOLS = ["NativeManifests"];
> -/* globals NativeApp */
>  
> -const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> +const {interfaces: Ci, utils: Cu} = Components;

Files that declare any C? shortcuts should always declare all of them.

::: toolkit/components/extensions/NativeMessaging.jsm:9
(Diff revision 1)
>  "use strict";
>  
> -this.EXPORTED_SYMBOLS = ["HostManifestManager", "NativeApp"];
> +this.EXPORTED_SYMBOLS = ["NativeApp"];
> -/* globals NativeApp */
>  
> -const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> +const {utils: Cu} = Components;

Same here.

::: toolkit/components/extensions/test/xpcshell/test_native_manifests.js:3
(Diff revision 1)
>  "use strict";
>  
> -/* global OS, HostManifestManager, NativeApp */
> +/* global OS, NativeManifests, NativeApp */

No need for `NatifeManifests` here.

Comment 21

[Tomislav Jovanovic :zombie]

Comment on attachment 8909059 [details]
Bug 1386427 - Part 1: Add XRE*NativeManifests locations to dirsvc

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

Comment 22

[Tomislav Jovanovic :zombie]

Comment on attachment 8909060 [details]
Bug 1386427 - Part 2: Extract HostManifestManager to NativeManifests.jsm

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

Comment 23

[Tomislav Jovanovic :zombie]

Comment on attachment 8909061 [details]
Bug 1386427 - Part 3: Add `storage` and `pkcs11` NativeManifest types

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

Comment 24

[Tomislav Jovanovic :zombie]

Comment on attachment 8909062 [details]
Bug 1386427 - Part 4: Implement basic storage.managed functionality

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

Comment 25

[Andy McKay]

(In reply to Tomislav Jovanovic :zombie from comment #19)
> The implementation is still fairly similar to what I proposed in comment 1,
> though exposed via the storage.managed API.

Awesome, thanks for doing this zombie.

>  - no enforcing of extension-provided managed storage JSON schema.

Could you expand that a little, enforcing... means? I'm curious if this would allow someone to configure an extension that uses chrome.storage.managed, eg: uBlock.

Comment 26

[Tomislav Jovanovic :zombie]

(In reply to Mike Kaply [:mkaply] from comment #11)
> I'm just not clear on how chrome.storage.managed is available to everyone.
> The chrome docs say:

We are only following the same (read-only) API exposed to the extension as Chrome, but not exactly copying the implementation, since that's tied to Chrome's Enterprise/Cloud stuff, and wouldn't make sense for us.

The details of how to setup a JSON file to be readable from an extension are still similar to what I described in comment 1.


(In reply to Andy McKay [:andym] from comment #25)
> Could you expand that a little, enforcing... means? 

See https://developer.chrome.com/apps/manifest/storage

TLDR: In Chrome, extension needs to include a JSON schema that describes names and types of all the fields in managed storage.

That probably makes some sense in their setup, but doesn't do much for our implementation, and I didn't want to spend time thinking about implications of running our Schema.jsm on extension-provided inputs.


> I'm curious if this would allow someone to configure an extension
> that uses chrome.storage.managed, eg: uBlock.

I don't understand this.  But to try to answer, if you place a manifest file of the right format in the right dir, and the extension tries to read via `chrome.storage.managed.get()`, it will get the config values you set.

Comment 27

[Andy McKay]

(In reply to Tomislav Jovanovic :zombie from comment #26)
> I don't understand this.  But to try to answer, if you place a manifest file
> of the right format in the right dir, and the extension tries to read via
> `chrome.storage.managed.get()`, it will get the config values you set.

Thanks for the clarification. And yes that sounds like it might work for the use case where administrators want to configure add-ons.

Comment 28

[Kris Maglione [:kmag]]

Comment on attachment 8909059 [details]
Bug 1386427 - Part 1: Add XRE*NativeManifests locations to dirsvc

https://reviewboard.mozilla.org/r/180656/#review186884

Comment 29

[Kris Maglione [:kmag]]

Comment on attachment 8909061 [details]
Bug 1386427 - Part 3: Add `storage` and `pkcs11` NativeManifest types

https://reviewboard.mozilla.org/r/180660/#review186886

Comment 30

[Kris Maglione [:kmag]]

Comment on attachment 8909062 [details]
Bug 1386427 - Part 4: Implement basic storage.managed functionality

https://reviewboard.mozilla.org/r/180662/#review187284

::: toolkit/components/extensions/ext-c-storage.js:143
(Diff revision 3)
> +            return Promise.reject({message: "storage.managed is read only"});
> +          },
> +          remove(keys) {
> +            return Promise.reject({message: "storage.managed is read only"});
> +          },
> +          clear() {
> +            return Promise.reject({message: "storage.managed is read only"});

Nit: read-only

::: toolkit/components/extensions/ext-storage.js:75
(Diff revision 3)
> +          async get(keys) {
> +            enforceNoTemporaryAddon(extension.id);
> +            let data = managedStorage.get(extension);
> +
> +            if (data === undefined) {
> +              let info = await NativeManifests.lookupManifest("storage", extension.id, context);

This is going to wind up doing the same work multiple times with multiple .get() calls. We should probably just store the promise in the `managedStorage` map rather than awaiting on it.

Comment 31

[Tomislav Jovanovic :zombie]

Comment on attachment 8909059 [details]
Bug 1386427 - Part 1: Add XRE*NativeManifests locations to dirsvc

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

Comment 32

[Tomislav Jovanovic :zombie]

Comment on attachment 8909060 [details]
Bug 1386427 - Part 2: Extract HostManifestManager to NativeManifests.jsm

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

Comment 33

[Tomislav Jovanovic :zombie]

Comment on attachment 8909061 [details]
Bug 1386427 - Part 3: Add `storage` and `pkcs11` NativeManifest types

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180660/diff/3-4/

Comment 34

[Tomislav Jovanovic :zombie]

Comment on attachment 8909062 [details]
Bug 1386427 - Part 4: Implement basic storage.managed functionality

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180662/diff/3-4/

Comment 35

[Tomislav Jovanovic :zombie]

Comment on attachment 8909062 [details]
Bug 1386427 - Part 4: Implement basic storage.managed functionality

https://reviewboard.mozilla.org/r/180662/#review187284

> This is going to wind up doing the same work multiple times with multiple .get() calls. We should probably just store the promise in the `managedStorage` map rather than awaiting on it.

I did store `data || null`, but yeah, there could be multiple calls before the lookup resolves.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/05ff50abca05
Part 1: Add XRE*NativeManifests locations to dirsvc r=kmag
https://hg.mozilla.org/integration/autoland/rev/660f2242c95f
Part 2: Extract HostManifestManager to NativeManifests.jsm r=kmag
https://hg.mozilla.org/integration/autoland/rev/603d09a85dd6
Part 3: Add `storage` and `pkcs11` NativeManifest types r=kmag
https://hg.mozilla.org/integration/autoland/rev/a439e2ac4305
Part 4: Implement basic storage.managed functionality r=kmag

Comment 37

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/05ff50abca05
https://hg.mozilla.org/mozilla-central/rev/660f2242c95f
https://hg.mozilla.org/mozilla-central/rev/603d09a85dd6
https://hg.mozilla.org/mozilla-central/rev/a439e2ac4305

Comment 38

[Tomislav Jovanovic :zombie]

For documentation:

 * This bug implements the browser.storage.managed.* api (with Chrome differences listed in comment 19).

 * Storage backends are the same on all platforms, a json manifest file of the following format:
    {
      "name": "$",
      "description": "ignored",
      "type": "storage",
      "data": {
        "key": "value",
        "another": [2, 3],
        // any valid JSON values
      }
    }

 * On *nix, the file must be named `${extensionId}.json` 
   - and placed in similar directories as native messaging manifests [1], 
   - except the subdir is named `managed-storage` on linux and `ManagedStorage` on OSX.

   1) https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Native_messaging#App_manifest_location

 * On Windows, the registry key at `Software\Mozilla\ManagedStorage\${extensionId}` must point to the file.


 * This bug also adds support for "pkcs11" manifest type (for bug 1357391), of the same format as native messaging one.
   - We are calling these NativeManifests internally, and probably makes sense to document them with the same name
   - I suggest a new "Native Manifests" page with info common to all manifest types.

Comment 39

[Tomislav Jovanovic :zombie]

>       "name": "$",

sorry, that should be

    "name": "${extensionId}",

Comment 40

[Intermittent Failures Robot]

1 failures in 943 pushes (0.001 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* mozilla-central: 1

Platform breakdown:
* linux64-jsdcov: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1386427&startday=2017-09-18&endday=2017-09-24&tree=all

Comment 41

[Will Bamberg [:wbamberg]]

Tomislav, I've updated this page: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/storage/managed

> I suggest a new "Native Manifests" page with info common to all manifest types.

I thought about this, but decided that while this makes most sense from the implementation point of view, it doesn't seem best from the user point of view. I'd expect someone to some to these docs thinking "how can I have managed storage" not "how can I learn about native manifests", so organizing it by application seemed more intuitive.

Also, there are lots of differences between the different manifests, some of which are a bit subtle (so for instance, this and the native messaging one both have "name", but it has a different meaning).

So I've just documented it all at storage.managed. Let me know what you think, anyway :).

Comment 42

[Tomislav Jovanovic :zombie]

Fair enough, I was just hoping to avoid repeating the same info across multiple pages, but instead linking it from wherever manifests are used.

The rest looks good, thanks.  I added the detail that this is also meant for native apps to configure their extensions (which this bug was originally about ;).

Comment 43

[marius.santa]

Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.

Comment 44

[Tomislav Jovanovic :zombie]

The actual storage.managed api added in this bug is new, and there are probably no extensions using it yet, but I did refactor the code for native manifests used by Native Messaging, so that could be useful to verify everything still works as expected.

Not sure if you have test extensions that use Native Messaging, or if you can find some from AMO that do.  I believe 1Password extension does, so please test on Windows, Linux and OSX, since the code is platform specific.  

https://agilebits.com/onepassword/extensions

Comment 45

[Andy McKay]

uBlock origin does, but it can be a little hard to test.

I added it to favourite-colour along with some instructions:

https://github.com/mdn/webextensions-examples/tree/master/favourite-colour

Comment 46

[YUKI "Piro" Hiroshi]

FYI: There is a library for defining preferences-like configurations, and it helps you to use storage.managed like as "lockPref()" in old preferences system (MCD aka AutoConfig). It is actually available on IEView WE, Tree Style Tabs, and some other my addons.
https://github.com/piroor/webextensions-lib-configs

Comment 47

[Tomislav Jovanovic :zombie]

Hi Marius, just pinging here to make sure this gets verified across platforms soon.

Comment 48

[marius.santa]

Attachment: storage managed.PNG

I can reproduce this issue on Firefox 57.0a1 (20170901220209) under Wind 10 64-bit. 

This issue is verified as fixed on Firefox 57.0 (20171112125346), Firefox  	58.0b6 (20171123161455) and Firefox 59.0a1 (20171127100433) under Wind 10 64-bit, Mac OS X 10.13 and ubuntu 14.04 LTS

Comment 49

[Tomislav Jovanovic :zombie]

Hi Marius, can you please also verify with an existing native messaging extension across platforms?

> but I did refactor the code for native manifests used by Native Messaging, 
> so that could be useful to verify everything still works as expected.
> 
> Not sure if you have test extensions that use Native Messaging, or if you
> can find some from AMO that do.  I believe 1Password extension does, so
> please test on Windows, Linux and OSX, since the code is platform specific.  
> 
> https://agilebits.com/onepassword/extensions

Comment 50

[marius.santa]

We tested on multiple devices OS-es and browsers but the 1Password extension does not seem to be working, this issue seems to affect more people out there https://discussions.agilebits.com/discussion/80897/1password-button-in-firefox-greyed-out-not-working. We can't get the extension to work.

We tested on Firefox 57.0 (20171128222554), Firefoxn58.0b8 (20171130160223) and Firefox 59.0a1 (20171204100103) under Wind 10 64-bit, Mac OS X 10.13 and ubuntu 14.04 LTS(also on Opera, Chrome, Safari)