Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Connect distinct accounts per site #7004

Merged
merged 21 commits into from
Dec 3, 2019
Merged

Connect distinct accounts per site #7004

merged 21 commits into from
Dec 3, 2019

Conversation

danfinlay
Copy link
Contributor

@danfinlay danfinlay commented Aug 13, 2019

This absolutely tremendous PR is finally ready for review.

For the complete picture, be sure to refer to the inpage provider.

Thanks to @Gudahtt for his extensive review work so far!

Code Owners

Changes (Non-Exhaustive)

  • Accounts are now connected individually to different sites, regardless of the globally selected account in the UI
    • Only one account can be connected to any one site at a time
  • inpage.js has been simplified, and much of its functionality has moved to the inpage provider module
  • The PermissionsController has replaced the ProviderApprovalController
    • PermissionsController wraps rpc-cap, and should be the single source of truth for the permissions of all external domains (be they dapps, other extensions, or in the future, plugins)

@danfinlay danfinlay added the DO-NOT-MERGE Pull requests that should not be merged label Aug 13, 2019
@metamaskbot
Copy link
Collaborator

Builds ready [774d931]: chrome, firefox, edge, opera

@metamaskbot
Copy link
Collaborator

Builds ready [66b1789]: chrome, firefox, edge, opera

@metamaskbot
Copy link
Collaborator

Builds ready [f56a011]: chrome, firefox, edge, opera

@metamaskbot
Copy link
Collaborator

Builds ready [eac7e8e]: chrome, firefox, edge, opera

@rekmarks rekmarks force-pushed the LoginPerSite branch 2 times, most recently from 7d0c81c to 15b78d3 Compare August 26, 2019 23:51
@metamaskbot
Copy link
Collaborator

Builds ready [15b78d3]: chrome, firefox, edge, opera

@metamaskbot
Copy link
Collaborator

Builds ready [0eaf89c]: chrome, firefox, edge, opera

@metamaskbot
Copy link
Collaborator

Builds ready [879ee72]: chrome, firefox, edge, opera

@metamaskbot
Copy link
Collaborator

Builds ready [0237333]: chrome, firefox, edge, opera

@metamaskbot
Copy link
Collaborator

Builds ready [186cf12]

@metamaskbot
Copy link
Collaborator

Builds ready [82b3b02]

@metamaskbot
Copy link
Collaborator

Builds ready [b5b6bc8]

@metamaskbot
Copy link
Collaborator

Builds ready [058e89d]

@metamaskbot
Copy link
Collaborator

Builds ready [be7285d]

@metamaskbot
Copy link
Collaborator

Builds ready [6097392]

@metamaskbot
Copy link
Collaborator

Builds ready [ea3d6be]

@metamaskbot
Copy link
Collaborator

Builds ready [d4c490c]

@metamaskbot
Copy link
Collaborator

Builds ready [4852e3e]

@metamaskbot
Copy link
Collaborator

Builds ready [79f1073]

@metamaskbot
Copy link
Collaborator

Builds ready [24d94b4]

@metamaskbot
Copy link
Collaborator

Builds ready [f571822]

Gudahtt and others added 9 commits December 3, 2019 12:39
The 'Connected Sites' button in the accounts details now closes the
sidebar, if it is open. This was accomplished by pulling the click
handler for that button up to the wallet view component, where another
button already followed a similar pattern of closing the sidebar.

It seemed confusing to me that one handler was in the `AccountsDetails`
container component, and one was handed down from above, so I added
PropTypes to the container component.

I'm not sure that the WalletView component is the best place for this
logic, but I've put it there for now to be consistent with the add
token button.
Permissions requests are now rejected when the page is closed. This
only applies to the full-screen view, as that is the view permission
requests should be handled in. The case where the user deals with the
request through a different view is handled in #7617
`extension.tabs.update` can sometimes fail if the user interacts with
the tabs directly around the same time. The redirect flow has been
updated to ensure that the permissions tab is still closed in that
case. The user is on their own to find the dapp tab again in that case.
* Handle redirect in response to state update in permissions-connect

* Ensure origin is available to permissions-connect subcomponents during redirect

* Hide app bar whenever on redirect route

* Improvements to handling of redirects in permissions-connect

* Ensure permission request id change handling only happens when page is not null

* Lint fix
…7623)

There was a race condition in the logic responsible for switching the
selected account based upon the active tab. It was asynchronously
querying the active tab, then assuming it had been retrieved later.

The active tab info itself was already in the redux store in another
spot, one that is guaranteed to be set before the UI renders. The
race condition was avoided by deleting the duplicate state, and using
the other active tab state.
The "redirect back to dapp" behaviour can be disruptive when the
permissions connect tab is not active. The purpose of the redirect was
to maintain context between the dapp and the permissions request, but
if the user has already moved to another tab, that no longer applies.
@Gudahtt
Copy link
Member

Gudahtt commented Dec 3, 2019

@danjm FYI I just removed some unused state, as required by a new lint rule. I guess we missed that when reviewing the UI changes? Let me know if it seems odd to you: 706dd35

@danjm
Copy link
Contributor

danjm commented Dec 3, 2019

@danjm FYI I just removed some unused state, as required by a new lint rule. I guess we missed that when reviewing the UI changes? Let me know if it seems odd to you: 706dd35

Thanks for the heads up. This removal looks correct.

@metamaskbot
Copy link
Collaborator

Builds ready [706dd35]

@danfinlay danfinlay merged commit f519fa1 into develop Dec 3, 2019
Gudahtt pushed a commit that referenced this pull request Dec 3, 2019
* add PermissionsController

remove provider approval controller
integrate rpc-cap
create PermissionsController
move provider approval functionality to permissions controller
add permissions approval ui, settings page
add permissions activity and history
move some functionality to metamask-inpage-provider
rename siteMetadata -> domainMetadata

add accountsChange notification to inpage provider
move functionality to inpage provider
update inpage provider
Remove 'Connections' settings page (#7369)
add hooks for exposing accounts in settings
rename unused messages in non-English locales

Add external extension id to metadata (#7396)

update inpage provider, rpc-cap
add eth_requestAccounts handling to background
prevent notifying connections if extension is locked
update inpage provider
Fix lint errors
add migration
review fixes
transaction controller review updates
removed unused messages

* Login Per Site UI (#7368)

* LoginPerSite original UI changes to keep

* First commit

* Get necessary connected tab info for redirect and icon display for permissioned sites

* Fix up designs and add missing features

* Some lint fixes

* More lint fixes

* Ensures the tx controller + tx-state-manager orders transactions in the order they are received

* Code cleanup for LoginPerSite-ui

* Update e2e tests to use new connection flow

* Fix display of connect screen and app header after login when connect request present

* Update metamask-responsive-ui.spec for new item in accounts dropdown

* Fix approve container by replacing approvedOrigins with domainMetaData

* Adds test/e2e/permissions.spec.js

* Correctly handle cancellation of a permissions request

* Redirect to home after disconnecting all sites / cancelling all permissions

* Fix display of site icons in menu

* Fix height of permissions page container

* Remove unused locale messages

* Set default values for openExternalTabs and tabIdOrigins in account-menu.container

* More code cleanup for LoginPerSite-ui

* Use extensions api to close tab in permissions-connect

* Remove unnecessary change in domIsReady() in contentscript

* Remove unnecessary private function markers and class methods (for background tab info) in metamask-controller.

* Adds getOriginOfCurrentTab selector

* Adds IconWithFallback component and substitutes for appropriate cases

* Add and utilize font mixins

* Remove unused  method in disconnect-all.container.js

* Simplify buttonSizeLarge code in page-container-footer.component.js

* Add and utilize getAccountsWithLabels selector

* Remove console.log in ui/app/store/actions.js

* Change last connected time format to yyyy-M-d

* Fix css associated with IconWithFallback change

* Ensure tracked openNonMetamaskTabsIDs are correctly set to inactive on tab changes

* Code cleanup for LoginPerSite-ui

* Use reusable function for modifying openNonMetamaskTabsIDs in background.js

* Enables automatic switching to connected account when connected domain is open

* Prevent exploit of tabIdOriginMap in background.js

* Remove unneeded code from contentscript.js

* Simplify current tab origin and window opener logic using remotePort listener tabs.queryTabs

* Design and styling fixes for LoginPerSite-ui

* Fix permissionHistory and permission logging for eth_requestAccounts and eth_accounts

* Front end changes to support display of lastConnected time in connected and permissions screens

* Fix lint errors

* Refactor structure of permissionsHistory

* Fix default values and object modifications for domain and permissionsHistory related data

* Fix connecting to new accounts from modal

* Replace retweet.svg with connect-white.svg

* Fix signature-request.spec

* Update metamask-inpage-provider version

* Fix permissions e2e tests

* Remove unneeded delay from test/e2e/signature-request.spec.js

* Add delay before attempting to retrieve network id in dapp in ethereum-on=.spec

* Use requestAccountTabIds strategy for determining tab id that opened a given window

* Improve default values for permissions requests

* Add some message descriptions to app/_locales/en/messages.json

* Code clean up in permission controller

* Stopped deep cloning object in mapObjectValues

* Bump metamask-inpage-provider version

* Add missing description in app/_locales/en/messages.json

* Return promises from queryTabs and switchToTab of extension.js

* Remove unused getAllPermissions function

* Use default props in icon-with-fallback.component.js

* Stop passing  to permissions controller

* Delete no longer used clear-approved-origins modal code

* Remove duplicate imports in ui/app/components/app/index.scss

* Use URL instead of regex in getOriginFromUrl()

* Add runtime error checking to platform, promise based extension.tab methods

* Support permission requests from external extensions

* Improve font size and colour of the domain origin on the permission confirmation screen

* Add support for toggling permissions

* Ensure getRenderablePermissionsDomains only returns domains with exposedAccount caveat permissions

* Remove unused code from LoginPerSite-ui branch

* Ensure modal closes on Enter press for new-account-modal.component.js

* Lint fix

* fixup! Login Per Site UI (#7368)

* Some code cleanup for LoginPerSite

* Adds UX for connecting to dapps via the connected sites screen (#7593)

* Adds UX for connecting to dapps via the connected sites screen

* Use openMetaMaskTabIds from background.js to determine if current active tab is MetaMask

* Delete unused permissions controller methods

* Fixes two small bugs in the LoginPerSite ui (#7595)

* Restore `providerRequest` message translations (#7600)

This message was removed, but it was replaced with a very similar
message called `likeToConnect`. The only difference is that the new
message has "MetaMask" in it. Preserving these messages without
"MetaMask" is probably better than deleting them, so these messages
have all been restored and renamed to `likeToConnect`.

* Login per site no sitemetadata fix (#7610)

* Support connected sites for which we have no site metadata.

* Change property containing subtitle info often populated by origin to a more accurate of purpose name

* Lint fix

* Improve disconnection modal messages (#7612)

* Improve disconnectAccountModalDescription and disconnectAllModalDescription messages

* Update disconnectAccountModalDescription app/_locales/en/messages.json

Co-Authored-By: Mark Stacey <markjstacey@gmail.com>

* Improve disconnectAccount modal message clarity

* Adds cancel button to the account selection screen of the permissions request flow (#7613)

* Fix eth_accounts permission language & selectability (#7614)

* fix eth_accounts language & selectability

* fix MetaMask capitalization in all messages

* Close sidebar when opening connected sites (#7611)

The 'Connected Sites' button in the accounts details now closes the
sidebar, if it is open. This was accomplished by pulling the click
handler for that button up to the wallet view component, where another
button already followed a similar pattern of closing the sidebar.

It seemed confusing to me that one handler was in the `AccountsDetails`
container component, and one was handed down from above, so I added
PropTypes to the container component.

I'm not sure that the WalletView component is the best place for this
logic, but I've put it there for now to be consistent with the add
token button.

* Reject permissions request upon tab close (#7618)

Permissions requests are now rejected when the page is closed. This
only applies to the full-screen view, as that is the view permission
requests should be handled in. The case where the user deals with the
request through a different view is handled in #7617

* Handle tab update failure (#7619)

`extension.tabs.update` can sometimes fail if the user interacts with
the tabs directly around the same time. The redirect flow has been
updated to ensure that the permissions tab is still closed in that
case. The user is on their own to find the dapp tab again in that case.

* Login per site tab popup fixes (#7617)

* Handle redirect in response to state update in permissions-connect

* Ensure origin is available to permissions-connect subcomponents during redirect

* Hide app bar whenever on redirect route

* Improvements to handling of redirects in permissions-connect

* Ensure permission request id change handling only happens when page is not null

* Lint fix

* Decouple confirm transaction screen from the selected address (#7622)

* Avoid race condtion that could prevent contextual account switching (#7623)

There was a race condition in the logic responsible for switching the
selected account based upon the active tab. It was asynchronously
querying the active tab, then assuming it had been retrieved later.

The active tab info itself was already in the redux store in another
spot, one that is guaranteed to be set before the UI renders. The
race condition was avoided by deleting the duplicate state, and using
the other active tab state.

* Only redirect back to dapp if current tab is active (#7621)

The "redirect back to dapp" behaviour can be disruptive when the
permissions connect tab is not active. The purpose of the redirect was
to maintain context between the dapp and the permissions request, but
if the user has already moved to another tab, that no longer applies.

* Fix JSX style lint errors

* Remove unused state
Gudahtt added a commit that referenced this pull request Dec 3, 2019
* origin/develop:
  Fix SimpleDropdown event bubbling (#7627)
  Remove unneeded store connections (#7625)
  Replace wild random number key with index (#7629)
  Use ES6 exports for selectors (#7626)
  Fix a typo made comparing previous prop (#7628)
  Connect distinct accounts per site (#7004)
  eslint: Enable more react/jsx-* rules (#7592)
  Enable react/no-unused-state rule for ESLint (#7609)
  Enable no-var rule for ESLint (#7590)
  Process URL fragment for ens-ipfs redirects (#7604)
  add locale fix script (#7580)
  Prevent redux state mutation (#7598)
@Gudahtt Gudahtt deleted the LoginPerSite branch December 4, 2019 01:33
Gudahtt added a commit that referenced this pull request Apr 9, 2020
This selector was added as part of #7004, but wasn't used by the time
it was merged.
Gudahtt added a commit that referenced this pull request Apr 10, 2020
This selector was added as part of #7004, but wasn't used by the time
it was merged.
Gudahtt added a commit that referenced this pull request Sep 10, 2020
This route was used for the "Connections" page in Settings, which was
removed in #7004
Gudahtt added a commit that referenced this pull request Sep 11, 2020
This route was used for the "Connections" page in Settings, which was
removed in #7004
@rekmarks rekmarks mentioned this pull request Nov 27, 2021
rekmarks added a commit that referenced this pull request Dec 7, 2021
# Permission System 2.0

## Background

This PR migrates the extension permission system to [the new `PermissionController`](https://github.com/MetaMask/snaps-skunkworks/tree/main/packages/controllers/src/permissions).
The original permission system, based on [`rpc-cap`](https://github.com/MetaMask/rpc-cap), introduced [`ZCAP-LD`](https://w3c-ccg.github.io/zcap-ld/)-like permissions to our JSON-RPC stack.
We used it to [implement](#7004) what we called "LoginPerSite" in [version 7.7.0](https://github.com/MetaMask/metamask-extension/releases/tag/v7.7.0) of the extension, which enabled the user to choose which accounts, if any, should be exposed to each dapp.
While that was a worthwhile feature in and of itself, we wanted a permission _system_ in order to enable everything we are going to with Snaps.
Unfortunately, the original permission system was difficult to use, and necessitated the creation of the original `PermissionsController` (note the "s"), which was more or less a wrapper for `rpc-cap`.

With this PR, we shake off the yoke of the original permission system, in favor of the modular, self-contained, ergonomic, and more mature permission system 2.0.

Note that [the `PermissionController` readme](https://github.com/MetaMask/snaps-skunkworks/tree/main/packages/controllers/src/permissions/README.md) explains how the new permission system works.

The `PermissionController` and `SubjectMetadataController` are currently shipped via `@metamask/snap-controllers`. This is a temporary state of affairs, and we'll move them to `@metamask/controllers` once they've landed in prod.

## Changes in Detail

First, the changes in this PR are not as big as they seem. Roughly half of the additions in this PR are fixtures in the test for the new migration (number 68), and a significant portion of the remaining ~2500 lines are due to find-and-replace changes in other test fixtures and UI files.

- The extension `PermissionsController` has been deleted, and completely replaced with the new `PermissionController` from [`@metamask/snap-controllers`](https://www.npmjs.com/package/@metamask/snap-controllers).
- The original `PermissionsController` "domain metadata" functionality is now managed by the new `SubjectMetadataController`, also from [`@metamask/snap-controllers`](https://www.npmjs.com/package/@metamask/snap-controllers).
- The permission activity and history log controller has been renamed `PermissionLogController` and has its own top-level state key, but is otherwise functionally equivalent to the existing implementation.
- Migration number 68 has been added to account for the new state changes.
- The tests in `app/scripts/controllers/permissions` have been migrated from `mocha` to `jest`.

Reviewers should focus their attention on the following files:

- `app/scripts/`
  - `metamask-controller.js`
    - This is where most of the integration work for the new `PermissionController` occurs.
      Some functions that were internal to the original controller were moved here.
  - `controllers/permissions/`
    - `selectors.js`
      - These selectors are for `ControllerMessenger` selector subscriptions. The actual subscriptions occur in `metamask-controller.js`. See the `ControllerMessenger` implementation for details.
    - `specifications.js`
      - The caveat and permission specifications are required by the new `PermissionController`, and are used to specify the `eth_accounts` permission and its JSON-RPC method implementation.
        See the `PermissionController` readme for details.
  - `migrations/068.js`
    - The new state should be cross-referenced with the controllers that manage it.
      The accompanying tests should also be thoroughly reviewed.

Some files may appear new but have just moved and/or been renamed:

- `app/scripts/lib/rpc-method-middleware/handlers/request-accounts.js`
  - This was previously implemented in `controllers/permissions/permissionsMethodMiddleware.js`.
- `test/mocks/permissions.js`
  - A truncated version of `test/mocks/permission-controller.js`.

Co-authored-by: Mark Stacey <markjstacey@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-background Issues relating to the extension background process. area-injection Relating to how the JS interface is injected into a website. area-permissions Issues relating to exposing permissions from the trusted MetaMask context to less-trusted contexts. area-provider Relating to the provider module. area-UI Relating to the user interface.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants