Update plugin permissions flow

[rekmarks]

Summary

Fixes #27, Fixes #94

Tightens up plugin permissions request flow, and makes working with plugins and permissions more ergonomic for dapp/plugin developers. Includes a much more recent rebase on develop and LoginPerSite, without UI changes. (I may break that off into its own PR for review purposes.)

Note that this PR includes a merge of LoginPerSite updates before they were merged into metamask-extension#develop, excluding LoginPerSite-ui. I apologize for this.

Important Files to Review

• Permissions controller method middleware: https://github.com/MetaMask/metamask-snaps-beta/pull/87/files#diff-285c7704cf5cc39a8aab9b3858d1fe56
• Plugins controller: https://github.com/MetaMask/metamask-snaps-beta/pull/87/files#diff-b2d32cddbb4d0093eeace770848515ac

Review Notes

• There are a number of renamed files; no major plugin- or permission-related files have been deleted
  • The permissions controller itself was renamed from permissions/permissions.js to permissions/index.js, per LoginPerSite. Some of its functionality was moved to the plugin controller, otherwise behavioral changes are minimal in the controller file itself.
• Informed by (does not fully fi#119 5, Add plugin update mechanism #45, Add API for notifying plugin when connection to page is closed #89, Refine notion of plugin state per account #97, #119

New Plugin Installation Flow

1. The external domain requests permissions using wallet_enable

ethereum.send('wallet_enable', [IRequestedPermissions]) => {
  accounts: Array<string>,
  plugins: IResolvedPlugins,
  permissions: Array<IOcapLdCapability>
}

We've added syntactic sugar to IRequestedPermissions in the form of the wallet_plugin permission (just that string, not a prefix). Whereas the value of a normal permission is currently {}, the value of wallet_plugin is IRequestedPlugins, which closely resembles IRequestedPermissions:

wallet_plugin: {
  [pluginName1]: {},
  [pluginName2]: {},
}

Here's an example call:

await ethereum.send('wallet_enable', [{
  'wallet_plugin': { 'http://localhost:8084/package.json': {} },
  'eth_accounts': {}
}])

Note that plugins can also be installed using the wallet_installPlugins method, if the dapp has permissions to interact with the plugin:

ethereum.send('wallet_installPlugins', [IRequestedPlugins]) => {
  accounts: Array<string>,
  plugins: IResolvedPlugins,
  permissions: Array<IOcapLdCapability>
}

IResolvedPlugins is an object of plugin name keys to objects with plugin metadata, currently only the name of its corresponding permission, under the key permission.

2. The extension processes the wallet_enable request

This is implemented in the permission controller's methodMiddleware. This method:

1. Unwraps the wallet_plugin sugar into actual permissions
2. Makes the permissions request
3. Attempts to install any requested plugins
4. Returns the wallet_enable return object, with information about permissions, plugins, and accounts

• If errors were encountered installing a specific plugin, its object (keyed by its name) will include an error property
• If the permission request fails completely, the entire request fails

Notes

• Try it out with any example in snaps-cli, they should all work with a build of this branch
• See Revisit plugin installation API surface #94 for more details

[danfinlay]

The approve() method should be an RPC method, I'd love if we kept it within requestPermissions, potentially extending that method.

I'm a little on the fence about leaving installation in the hands of the app, but it also does give very concrete lifecycle hooks to the application, where it can react to approval while it waits for dependencies to resolve, so that's actually kinda nice, I think I could grow into that.

I included a comment that has some example code (how I think this works, and some other ways it could work). I'm not completely clear what this API surface looks like, I'd like us to get some very concrete examples documented before agreeing this is the API we're going forward with.

[danfinlay]

I appreciate the organization of this PR! Highlighting important files is very nice.

This is a large enough (breaking) change that we should couple it with a migration guide, a full migration of the examples, and then send that guide out to our devs (keybase, telegram, plugin beta email, maybe even twitter).

> Try it out with any example in snaps-cli, they should all work with a build of this branch

Wait, I thought this was a very breaking change. How do the examples still work?

[danfinlay]

This looks good. Left some notes, one in particular could really improve performance, but nothing that needs to block, although I would advise against merging until we have a migration guide & sample snaps & external comms lined up for this.

[danfinlay]

Installing several plugins at once, especially over what could be a very high-latency protocol like IPFS, is a great time to use Promise.all() over a for of loop with await calls. Allows parallelization.

[rekmarks]

Hm, we can't make any guarantees about installation order if we do that, although I don't think we need to.

The reason I did it that way was to construct the result object, although looking at it with fresh eyes, I'm not sure there's any reason I couldn't accomplish the same with with await Promise.all(...)...

I'll give it a shot and see.

[rekmarks]

I think I got a functional implementation! await Promise.all(...) worked, I just needed to fix some UX hiccups that resulted from it.

[danfinlay]

Sure feels like we should have an easier convenience method for triggering a middleware function, eh? We did export an engine related to the origin on the initial request, I partly think we should be storing a weak reference to it so we could just call engine.send(req, cb).

Not a blocker, just pointing out where we're bending over backwards to do something a little strange.

[rekmarks]

Ah, you mean storing the PermissionsController middleware engines weakly by origin? I'm definitely not against it; that's what we do in LoginPerSite in metamask-controller in order to send notifications to external domains.

However, I think what we're currently doing there is more performant, because it bypasses the rest of the middleware stack to just hit the CapabilitiesController's middleware function. Or am I missing something?

Edit: Although to your point, this definitely looks and feels hacky, and we should probably fix ourselves a better way of doing it.

[danfinlay]

I would hope this would hit sentry. I assume this is LoginPerSite, not new stuff?

[rekmarks]

Yeah, that's right. It's purely a defensive measure. It will hit sentry if RPC errors hit sentry, which I assume they do?

[danfinlay]

Naturally this is also another place where batching would be very valuable, as the popup closing/opening is painfully slow. Not a blocker, just a note (in case it works cheaply).

[rekmarks]

Hmm, I see the point, but in this particular case the permissions request must be fully resolved before we can continue, unless we're saying that a dapp can (1) request a plugin, (2) be denied access to the plugin, and (3) still cause the plugin to be installed.