Repository navigation
Add restricted controller messenger - #378
Merged
Merged
Conversation
Member
Author
|
|
Gudahtt
force-pushed
the
controller-messaging
branch
from
March 8, 2021 14:38
159d49e to
756e986
Compare
Gudahtt
force-pushed
the
restricted-controller-messenger
branch
from
March 8, 2021 14:38
30751bd to
d5cb725
Compare
Member
Author
|
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.). |
Gudahtt
force-pushed
the
restricted-controller-messenger
branch
from
March 8, 2021 16:12
d5cb725 to
4433aa7
Compare
Gudahtt
force-pushed
the
controller-messaging
branch
from
March 8, 2021 18:20
756e986 to
5202621
Compare
Gudahtt
force-pushed
the
restricted-controller-messenger
branch
3 times, most recently
from
March 8, 2021 21:26
993d382 to
53cc024
Compare
Gudahtt
commented
Mar 8, 2021
Gudahtt
force-pushed
the
restricted-controller-messenger
branch
3 times, most recently
from
March 9, 2021 05:31
4a885cc to
626fd07
Compare
Gudahtt
commented
Mar 9, 2021
Member
Author
There was a problem hiding this comment.
This is the most complete example so far. It demonstrates calling an action and subscribing to an event in another controller.
Gudahtt
force-pushed
the
controller-messaging
branch
from
March 11, 2021 04:14
c4fb8f4 to
c267a31
Compare
Gudahtt
force-pushed
the
restricted-controller-messenger
branch
from
March 11, 2021 04:14
e8cb578 to
e7bb905
Compare
Gudahtt
force-pushed
the
controller-messaging
branch
from
March 11, 2021 16:52
c267a31 to
1671284
Compare
Gudahtt
force-pushed
the
restricted-controller-messenger
branch
from
March 11, 2021 17:32
e7bb905 to
a97dfe4
Compare
Gudahtt
marked this pull request as ready for review
March 11, 2021 17:40
Gudahtt
force-pushed
the
restricted-controller-messenger
branch
from
March 12, 2021 17:31
a97dfe4 to
be56e2b
Compare
Gudahtt
force-pushed
the
restricted-controller-messenger
branch
5 times, most recently
from
March 19, 2021 20:11
99cd49d to
2e7b0b6
Compare
Gudahtt
force-pushed
the
restricted-controller-messenger
branch
from
March 23, 2021 18:11
94d73b9 to
e660361
Compare
Gudahtt
force-pushed
the
restricted-controller-messenger
branch
from
April 7, 2021 16:20
d387679 to
47e0cab
Compare
rekmarks
suggested changes
Apr 13, 2021
rekmarks
left a comment
rekmarks
left a comment
Contributor
There was a problem hiding this comment.
A bunch of nits, but overall I think this rules.
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.
Gudahtt
force-pushed
the
restricted-controller-messenger
branch
from
April 13, 2021 23:09
47e0cab to
d25bb8e
Compare
The new tests cover restricted messenger instances with access to both internal and external events and actions.
Gudahtt
force-pushed
the
restricted-controller-messenger
branch
from
April 13, 2021 23:13
d25bb8e to
d028d1b
Compare
Member
Author
|
This has been rebased, and all comments have been addressed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.