Add restricted controller messenger #378

Gudahtt · 2021-03-08T14:36:14Z

A method has been added to the controller messenger that returns a restricted instance of the controller messenger. This instance is restricted to registering actions and events within a particular namespace, typically being the name of the controller. Access to event subscriptions and calling actions is also restricted based on the given allowlist for each.

This is meant to address feedback left on the Controller Messaging System proposal.

Gudahtt · 2021-03-08T14:36:34Z

~~This depends upon #377~~

Gudahtt · 2021-03-08T15:35:06Z

My objective with this restricted messaging system was to prevent accidental use of methods beyond the intended access. I'm not protecting against access to private methods/properties, or anything sneaky like that. We have lint rules and types to protect against us doing that. This approach isn't adequate for dealing with untrusted code. I don't think we'll be using the controller messaging system for untrusted code though - at least not directly.

The snaps API will likely use the messaging system, but it will use other means to prevent unintended access (e.g. an RPC layer, SES, the permission system, etc.).

src/ControllerMessenger.ts

Gudahtt · 2021-03-09T18:02:28Z

src/BaseControllerV2.test.ts

+      }
+    }
+
+    it('should allow messaging between controllers', () => {


This is the most complete example so far. It demonstrates calling an action and subscribing to an event in another controller.

I think I love it. 😍

rekmarks

A bunch of nits, but overall I think this rules.

src/ControllerMessenger.ts

src/ControllerMessenger.test.ts

src/BaseControllerV2.test.ts

rekmarks · 2021-04-13T19:40:02Z

src/BaseControllerV2.test.ts

+      }
+    }
+
+    it('should allow messaging between controllers', () => {


I think I love it. 😍

A method has been added to the controller messenger that returns a _restricted_ instance of the controller messenger. This instance is restricted to registering actions and events within a particular namespace, typically being the name of the controller. Access to event subscriptions and calling actions is also restricted based on the given allowlist for each. This is meant to address feedback left on the Controller Messaging System proposal[1]. [1]: https://www.notion.so/Controller-Messaging-System-617efb02b9e54bd0a0b0e44c6f776d85

The parameters for `getRestricted` were moved into an object, to improve readability.

The "controllerName" option has been renamed to "name", so as to not assume that the name is of a controller.

All methods now have corresponding JSDoc comments.

Previously the Base Controller would not accept any action types, and it would only accept the single action it used. Now it accepts any types, so long as the required type is present.

There is now a unit test that demonstrates inter-controller communication using the controller messaging system. This is a Base Controller test because it's also testing the Base Controller's messenger integration at the same time.

This requirement was added recently in #422

Comments have been added alongside each `istanbul ignore if` statement explaining why it was required.

The new tests cover restricted messenger instances with access to both internal and external events and actions.

Gudahtt · 2021-04-13T23:24:34Z

This has been rebased, and all comments have been addressed.

rekmarks

Let's gooo!

- Add restricted controller messenger ([#378](#378)) - **BREAKING:** Update minimum Node.js version to v12 ([#441](#441)) - **BREAKING:** Replace controller context ([#387](#387)) - Bump @metamask/contract-metadata from 1.23.0 to 1.24.0 ([#440](#440)) - Update lint rules ([#442](#442), [#426](#426)) - Don't remove collectibles during auto detection ([#439](#439))

A method has been added to the controller messenger that returns a _restricted_ instance of the controller messenger. This instance is restricted to registering actions and events within a particular namespace, typically being the name of the controller. Access to event subscriptions and calling actions is also restricted based on the given allowlist for each. This is meant to address feedback left on the Controller Messaging System proposal[1]. [1]: https://www.notion.so/Controller-Messaging-System-617efb02b9e54bd0a0b0e44c6f776d85

Gudahtt force-pushed the controller-messaging branch from 159d49e to 756e986 Compare March 8, 2021 14:38

Gudahtt force-pushed the restricted-controller-messenger branch from 30751bd to d5cb725 Compare March 8, 2021 14:38

Gudahtt mentioned this pull request Mar 8, 2021

Add controller messaging system #377

Merged

Gudahtt changed the title ~~Add restricted controller~~ Add restricted controller messenger Mar 8, 2021

Gudahtt force-pushed the restricted-controller-messenger branch from d5cb725 to 4433aa7 Compare March 8, 2021 16:12

Gudahtt force-pushed the controller-messaging branch from 756e986 to 5202621 Compare March 8, 2021 18:20

Gudahtt force-pushed the restricted-controller-messenger branch 3 times, most recently from 993d382 to 53cc024 Compare March 8, 2021 21:26

Gudahtt commented Mar 8, 2021

View reviewed changes

src/ControllerMessenger.ts Outdated Show resolved Hide resolved

Gudahtt force-pushed the restricted-controller-messenger branch 3 times, most recently from 4a885cc to 626fd07 Compare March 9, 2021 05:31

Gudahtt commented Mar 9, 2021

View reviewed changes

Gudahtt force-pushed the controller-messaging branch from c4fb8f4 to c267a31 Compare March 11, 2021 04:14

Gudahtt force-pushed the restricted-controller-messenger branch from e8cb578 to e7bb905 Compare March 11, 2021 04:14

Gudahtt force-pushed the controller-messaging branch from c267a31 to 1671284 Compare March 11, 2021 16:52

Base automatically changed from controller-messaging to develop March 11, 2021 17:32

Gudahtt force-pushed the restricted-controller-messenger branch from e7bb905 to a97dfe4 Compare March 11, 2021 17:32

Gudahtt marked this pull request as ready for review March 11, 2021 17:40

Gudahtt requested a review from a team as a code owner March 11, 2021 17:40

Gudahtt force-pushed the restricted-controller-messenger branch from a97dfe4 to be56e2b Compare March 12, 2021 17:31

Gudahtt mentioned this pull request Mar 12, 2021

Prototype/propsoal: Implement controller messaging proposal #320

Closed

Gudahtt force-pushed the restricted-controller-messenger branch 5 times, most recently from 99cd49d to 2e7b0b6 Compare March 19, 2021 20:11

Gudahtt dismissed danfinlay’s stale review via e660361 March 23, 2021 18:11

Gudahtt force-pushed the restricted-controller-messenger branch from 94d73b9 to e660361 Compare March 23, 2021 18:11

Gudahtt force-pushed the restricted-controller-messenger branch from d387679 to 47e0cab Compare April 7, 2021 16:20

rekmarks self-assigned this Apr 12, 2021

rekmarks requested changes Apr 13, 2021

View reviewed changes

Gudahtt added 11 commits April 13, 2021 19:36

Pass parameters to getRestricted as object

d6e2559

The parameters for `getRestricted` were moved into an object, to improve readability.

Rename "controllerName" option to "name"

5e16bff

The "controllerName" option has been renamed to "name", so as to not assume that the name is of a controller.

Add documentation for the restricted controller messenger.

9538e92

All methods now have corresponding JSDoc comments.

Add doc comment for exported 'Namespaced' type

6745b4b

Improve mock variable names

002cdc6

Make BaseController messenger types less restrictive

7787fde

Previously the Base Controller would not accept any action types, and it would only accept the single action it used. Now it accepts any types, so long as the required type is present.

Add inter-controller communication test

9cd745f

There is now a unit test that demonstrates inter-controller communication using the controller messaging system. This is a Base Controller test because it's also testing the Base Controller's messenger integration at the same time.

Improve types of mock controller

41c54a7

Add expected error messages to controller messenger tests

05cfe70

This requirement was added recently in #422

Add comments explaining coverage gaps

65c6721

Comments have been added alongside each `istanbul ignore if` statement explaining why it was required.

Gudahtt force-pushed the restricted-controller-messenger branch from 47e0cab to d25bb8e Compare April 13, 2021 23:09

Gudahtt added 2 commits April 13, 2021 20:43

Add additional controller messenger tests

653e1be

The new tests cover restricted messenger instances with access to both internal and external events and actions.

Add newlines around type declarations

d028d1b

Gudahtt force-pushed the restricted-controller-messenger branch from d25bb8e to d028d1b Compare April 13, 2021 23:13

Gudahtt mentioned this pull request Apr 13, 2021

@metamask/eslint [email protected] #442

Merged

rekmarks approved these changes Apr 13, 2021

View reviewed changes

Gudahtt merged commit 942f6b2 into develop Apr 14, 2021

Gudahtt deleted the restricted-controller-messenger branch April 14, 2021 13:24

This was referenced Jul 24, 2024

[DNM] Preview branch of complete ComposableController fix with no type error suppression MetaMask/metamask-mobile#10374

Closed

chore(deps): Bump @metamask/composable-controller from ^3.0.0 to ^10.0.0 MetaMask/metamask-mobile#10441

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add restricted controller messenger #378

Add restricted controller messenger #378

Gudahtt commented Mar 8, 2021

Gudahtt commented Mar 8, 2021 •

edited

Loading

Gudahtt commented Mar 8, 2021

Gudahtt Mar 9, 2021

rekmarks Apr 13, 2021

rekmarks left a comment

rekmarks Apr 13, 2021

Gudahtt commented Apr 13, 2021

rekmarks left a comment

Add restricted controller messenger #378

Add restricted controller messenger #378

Conversation

Gudahtt commented Mar 8, 2021

Gudahtt commented Mar 8, 2021 • edited Loading

Gudahtt commented Mar 8, 2021

Gudahtt Mar 9, 2021

Choose a reason for hiding this comment

rekmarks Apr 13, 2021

Choose a reason for hiding this comment

rekmarks left a comment

Choose a reason for hiding this comment

rekmarks Apr 13, 2021

Choose a reason for hiding this comment

Gudahtt commented Apr 13, 2021

rekmarks left a comment

Choose a reason for hiding this comment

Gudahtt commented Mar 8, 2021 •

edited

Loading