Limit Dapp permissions to primary account #8653
Merged
Conversation
This file contains 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
|
@rekmarks I tested locally with a locally running test-dapp with some console.log statements to confirm I was getting the expected length of response. ➕ It looks like its working but I'm sure this missing some finesse on how we can test it etc. |
rekmarks
reviewed
May 26, 2020
brad-decker
force-pushed
the
permissions
branch
3 times, most recently
from
9f2b709 to
c035cdc
Compare
rekmarks
reviewed
May 27, 2020
rekmarks
reviewed
May 27, 2020
rekmarks
reviewed
May 27, 2020
test/unit/app/controllers/permissions/permissions-controller-test.js
Outdated
Show resolved
Hide resolved
rekmarks
reviewed
May 27, 2020
test/unit/app/controllers/permissions/permissions-controller-test.js
Outdated
Show resolved
Hide resolved
Builds ready [0752434]
Page Load Metrics (689 ± 79 ms)
|
rekmarks
reviewed
May 27, 2020
Comment on lines
+433
to
+443
| ethAccounts.caveats.push({ | ||
| type: CAVEAT_TYPES.limitResponseLength, | ||
| value: 1, | ||
| name: CAVEAT_NAMES.primaryAccountOnly, | ||
| }) | ||
|
|
||
| ethAccounts.caveats.push({ | ||
| type: CAVEAT_TYPES.filterResponse, | ||
| value: finalizedAccounts, | ||
| name: CAVEAT_NAMES.exposedAccounts, | ||
| }) |
There was a problem hiding this comment.
I want to note for posterity that this order is unfortunately important. Fortunately, it is preserved when we call addPermittedAccount, which calls CapabilitiesController.updateCaveatFor on the exposedAccounts caveat, which adds the updated caveat to the end of the caveat array.
There's currently no way to ensure that rpc-cap enforces a particular order of caveat application, except to add/update them such that they're in the order you desire.
So, happily not a problem here, but I'll create an issue on rpc-cap for a feature request.
Builds ready [d112e95]
Page Load Metrics (627 ± 61 ms)
|
Builds ready [3e868be]
Page Load Metrics (585 ± 43 ms)
|
Gudahtt
added a commit
that referenced
this pull request
Jun 5, 2020
The "Connected accounts" modal was throwing an exception when attempting to render an account that has no `lastActive` time. The component and related selector has been updated to no longer expect the `lastActive` time to be set. Prior to #8653 there was a guarantee that all connected accounts had a `lastActive` time set, as the time would be set on any account returned from the `eth_requestAccounts` call. But after #8653 only the primary account was returned, so only the primary account had a `lastActive` time set. Fixes #8733
Gudahtt
added a commit
that referenced
this pull request
Jun 5, 2020
The "Connected accounts" modal was throwing an exception when attempting to render an account that has no `lastActive` time. The component and related selector has been updated to no longer expect the `lastActive` time to be set. Prior to #8653 there was a guarantee that all connected accounts had a `lastActive` time set, as the time would be set on any account returned from the `eth_requestAccounts` call. But after #8653 only the primary account was returned, so only the primary account had a `lastActive` time set. Fixes #8733
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #8651
Uses the new limitResponseLength caveat exposed in rpc-cap to limit the Dapp's permission to only be able to access the primary account. This is step 1 of a two PR(+) contribution to add the ability for Dapp authors to request permission to interact with all connected accounts. This PR is a requirement for v8 RC