tfoxy / chrome-promise

Promises for chrome JavaScript APIs used in extensions and apps.
MIT License
150 stars 14 forks source link

ChromePromise instance doesn't update when optional permissions are requested #21

Closed fwextensions closed 6 years ago

fwextensions commented 6 years ago

The ChromePromise constructor wraps whatever Chrome extension APIs are available when the library is first loaded. But it's also possible to request optional permissions after the extension first loads. If the user gives that permission, though, any existing ChromePromise instances aren't updated to provide the newly available API.

One way to solve this might be wrap the chrome.permissions.request() method and intercept the request and the response. If the response is true, then the instance could look at the requested permissions and update itself with those APIs.

tfoxy commented 6 years ago

I think it would be better to listen to the onAdded and onRemoved events.

https://developer.chrome.com/apps/permissions#events

Are you willing to make a PR?

fwextensions commented 6 years ago

I thought about the onAdded event, but my assumption was that it wouldn't fire until after the request() callback is executed (haven't tested it, though). So you wouldn't be able to do something like, which seemed like a useful approach:

const cp = new ChromePromise();
cp.permissions.request({ permissions: ["history"] })
    .then(approved => if (approved) cp.history.search(...))

This is my monkeypatch of the constructor that seems to be working so far:

function ChromePromise(options) {
  options = options || {};
  var chrome = options.chrome || root.chrome;
  var Promise = options.Promise || root.Promise;
  var runtime = chrome.runtime;
  var self = this;

  fillProperties(chrome, this);

  var permissionsRequest = this.permissions.request;

  this.permissions.request = function(perms) {
    return permissionsRequest(perms)
      .then(function(result) {
        var approvedPerms = {};

        if (result && perms.permissions && perms.permissions.length) {
          perms.permissions.forEach(function(permission) {
            approvedPerms[permission] = chrome[permission];
          });
          fillProperties(approvedPerms, self);
        }

        return result;
      });
  };
  ...
tfoxy commented 6 years ago

I did a little test, and onAdded listeners are called before the request callback. So adding the listener would be better than monkeypatching. Can you check if your code works using:

function ChromePromise(options) {
  options = options || {};
  var chrome = options.chrome || root.chrome;
  var Promise = options.Promise || root.Promise;
  var runtime = chrome.runtime;
  var self = this;

  fillProperties(chrome, this);

  chrome.permissions.onAdded.addListener(function(perms) {
    if (perms.permissions && perms.permissions.length) {
      var approvedPerms = {};
      perms.permissions.forEach(function(permission) {
        approvedPerms[permission] = chrome[permission];
      });
      fillProperties(approvedPerms, self);
    }
  });
  ...
fwextensions commented 6 years ago

Cool, I should've tested the events first. At least in my quick test, the requested API is indeed available in the cp.permissions.request().then() handler, so it's working well.

Not every optional permission adds an object of the same name, so there are a few special cases to handle, but this solution is most of the way there.

tfoxy commented 6 years ago

I released a new version (2.2.0) with this issue fixed! Thanks for reporting it!

fwextensions commented 6 years ago

Nice!

One thing to note is that there's at least one optional permission where this won't work correctly. The webRequestBlocking permission gives access to the webRequest API, not a webRequestBlocking API. So requesting just the blocking permission would fail.

Also, the clipboardRead and clipboardWrite permissions don't seem to map to an API (it's not clear they're even necessary, given the description).

I haven't done a thorough comparison of the list of optional permissions and the objects they give access to on these two pages, but did note the above discrepancies:

https://developer.chrome.com/extensions/permissions https://developer.chrome.com/extensions/declare_permissions

This wouldn't work for the clipboard permissions or for ones like enterprise.deviceAttributes (not sure if those can be optional), but one quick way of handling permissions like webRequestBlocking would be using an object to map the permission string to a different object name:

const permAliases = { webRequestBlocking: "webRequest" };
...
  perms.permissions.forEach(function(permission) {
    permission = permAliases[permission] || permission;
    approvedPerms[permission] = chrome[permission];
  });

I decided not to use optional permissions in the extension I'm working on, so I haven't gotten back to testing this out, but just wanted to call out these edge cases.

Also, it might be nice to support removing permissions as well.

tfoxy commented 6 years ago

Thanks for all the research. I didn't add the alias for webRequestBlocking because the webRequest permission is also needed. So I added a condition to check if the api exists or not. You are right about clipboardRead and clipboardWrite, so I didn't do anything with those. Finally, for enterprise.deviceAttributes, it will add the enterprise api.

When permissions are removed, the api still exists in chrome, but calling a method throws an error. When using chromep, the promise is rejected. So I think the current behaviour is correct.