Closed Bug 1386427 Opened 2 years ago Closed 2 years ago

Basic implementation of storage.managed

Categories

(WebExtensions :: Untriaged, enhancement, P2)

enhancement

Tracking

(firefox57 verified, firefox58 verified, firefox59 verified)

VERIFIED FIXED
mozilla57
Tracking Status
firefox57 --- verified
firefox58 --- verified
firefox59 --- verified

People

(Reporter: mkaply, Assigned: Tomislav)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [design-decision-needed])

Attachments

(5 files)

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.
Whiteboard: [design-decision-needed]
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.
App manifest could go something like:

  {
    "name": "PartnerAddon",
    "description": "Example host for native messaging",
    "type": "data",
    "allowed_extensions": [ "PartnerAddon@mozilla.org" ],
    "data": {
      "campaignId": "1234567890-abcdef"
    }
  }

adapted from https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Native_messaging#App_manifest
I like this idea a lot.

How does the extension read the manifest without having a native app to do the messaging?
Just cc'ing security so they know about the ideas here.
(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).
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.
Flags: needinfo?(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...
Flags: needinfo?(aswan)
Summary: Provide a way for WebExtensions to read registry entries → Provide a way for installers to provide static data to a WebExtension
Is this something that has the remotest potential to make 57? This is important for partners (including MozillaOnline)
Per discussion over irc.
Assignee: nobody → tomica
Status: NEW → ASSIGNED
Priority: -- → P2
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?
See Also: → 1230802
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?
Attachment #8909059 - Flags: review?(kmaglione+bmo)
Attachment #8909060 - Flags: review?(kmaglione+bmo)
Attachment #8909061 - Flags: review?(kmaglione+bmo)
Attachment #8909062 - Flags: review?(kmaglione+bmo)
Attachment #8909059 - Flags: review?(kmaglione+bmo)
(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.
Blocks: 1230802
Summary: Provide a way for installers to provide static data to a WebExtension → Basic implementation of storage.managed
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.
Attachment #8909060 - Flags: review?(kmaglione+bmo) → review+
(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.
(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.
(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.
Attachment #8909059 - Flags: review?(kmaglione+bmo)
Comment on attachment 8909059 [details]
Bug 1386427 - Part 1: Add XRE*NativeManifests locations to dirsvc

https://reviewboard.mozilla.org/r/180656/#review186884
Attachment #8909059 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8909061 [details]
Bug 1386427 - Part 3: Add `storage` and `pkcs11` NativeManifest types

https://reviewboard.mozilla.org/r/180660/#review186886
Attachment #8909061 - Flags: review?(kmaglione+bmo) → review+
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.
Attachment #8909062 - Flags: review?(kmaglione+bmo) → review+
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.
Pushed by tomica@gmail.com:
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
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.
Keywords: dev-doc-needed
>       "name": "$",

sorry, that should be

    "name": "${extensionId}",
Blocks: 1333828
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 :).
Flags: needinfo?(tomica)
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 ;).
Flags: needinfo?(tomica)
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.
Flags: needinfo?(mozilla)
Flags: needinfo?(mozilla) → needinfo?(tomica)
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
Flags: needinfo?(tomica)
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
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
Hi Marius, just pinging here to make sure this gets verified across platforms soon.
Flags: needinfo?(marius.santa)
Attached image 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
Flags: needinfo?(marius.santa)
Status: RESOLVED → VERIFIED
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
Flags: needinfo?(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)
Flags: needinfo?(marius.santa)
Duplicate of this bug: 1442668
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.