fix: Move permission middleware to be later than rpc method middleware

[Cal-L]

Description

~~This PR adds eth_accounts to the unrestricted methods list that is used by the permission system as a temporary workaround to fix the broken deeplink behavior when connecting to OpenSea. The breaking change happened when we introduced the permission middleware - https://github.com/MetaMask/metamask-mobile/pull/9521, which resulted in eth_accounts being handled by the permission middleware instead of RPCMethodMiddleware. Before the PS middleware was introduced, an unauthorized RPC error was caught and MetaMask would return an empty array. After the PS middleware was introduced, MetaMask would throw the error instead. Our suspicion is that OpenSea changes the connect button behavior to deeplink whenever eth_accounts encounters an error. By adding eth_accounts method to the unrestricted list, the method skip the PS middleware and be handled by RPCMethodMiddleware, which preserves the old behavior.~~

Update: The general sentiment is to follow extension's implementation for now to preserve the correct behavior and to remove the need to manage eth_accounts in the unrestricted list. The change moves permission middleware to be later than rpc method middleware

Related issues

fixes #9443 fixes MetaMask/mobile-planning#1796

Manual testing steps

1. Open OpenSea.io in the MetaMask in-app browser
2. Tap connect
3. Notice that the permissions flow will appear instead of being redirected to the app store

Screenshots/Recordings

Before

https://github.com/MetaMask/metamask-mobile/assets/10508597/56bd2b4d-d6a7-4fca-a9b8-be1fee27da18

After

https://github.com/MetaMask/metamask-mobile/assets/10508597/631b2f78-6b8a-45c9-a744-609b4ad51733

Pre-merge author checklist

• [X] I’ve followed MetaMask Coding Standards.
• [X] I've completed the PR template to the best of my ability
• [X] I’ve included tests if applicable
• [X] I’ve documented my code using JSDoc format if applicable
• [X] I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

• [X] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
• [X] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

[github-actions[bot]]

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

[github-actions[bot]]

Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: b6c83b9dbd3bdb0ee7804ce2bf78e3fdcd03609c Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/a3701d19-b821-46f6-b99c-a5ae57737299

[!NOTE]

• You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

[github-actions[bot]]

Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: b662384b1e1c56ea4ab6381c7d8f67030fa344bd Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/f43d777f-0f50-4d45-9973-a0bd05ebdfec

[!NOTE]

• You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

[naugtur]

Silent failure or empty result are acceptable. Returning the correct value without permission is not.

@Cal-L It's not clear to me from the testcase if the result is undefined because there's nothing to return or it's a silent failure. Please expand the tests to show actual addresses being returned for an authorized request and an empty result for an unauthorized one.

cc @leotm

[tommasini]

I don't think this is causing a privacy issue because at the RPCMethodMiddleware file, where we handle the eth_accounts rpc method, we call getPermittedAccounts , which awaits the permission controller executeRestrictedMethod that returns an error if the user is not connected to the dapp

https://github.com/MetaMask/metamask-mobile/assets/46944231/d6545aa8-077b-461f-80cc-df756bf04d81

[tommasini]

It's not clear to me from the testcase if the result is undefined because there's nothing to return or it's a silent failure

We (with @leotm) checked the test and it seems the mock of getPermittedAccounts took us to a wrong behaviour (a response with a result undefined instead of an permission error), because getPermittedAccounts mock doesn't have an error as a response.

Next step looks like getting the unit tests to mock the getPermittedAccounts behaviour as it is, since it's just mocking the possible accounts by hostname here

[Cal-L]

Please expand the tests to show actual addresses being returned for an authorized request and an empty result for an unauthorized one.

@naugtur I've reverted the test changes in this PR since the tests prior to the change shows how the permission middleware handles this if placed before rpc method middleware - https://github.com/MetaMask/metamask-mobile/blob/6c5f55c3407b26f42d270c26cff21883ee0a2e31/app/core/RPCMethods/RPCMethodMiddleware.test.ts#L321. Here's a reference to unit tests that show how the RPCMethodMiddleware handles the same cases - https://github.com/MetaMask/metamask-mobile/blob/6c5f55c3407b26f42d270c26cff21883ee0a2e31/app/core/RPCMethods/RPCMethodMiddleware.test.ts#L444

[Cal-L]

as I understand it, this would expose addresses to all websites without approval of the user. if this is correct, I find it an unacceptable privacy issue even if temporary

@kumavis This wouldn't be exposing addresses without user approval since eth_accounts on the rpc method middleware level uses permission controller under the hood. To preserve existing API behavior, an authorized error from the permission controller will result in the dapp receiving an empty array.

[github-actions[bot]]

Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 6035bb18e83a04aa0b03f8c5742582b2fe90294d Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/9dfdcd1f-adc5-42f3-8fca-487557c029ca

[!NOTE]

• You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

[github-actions[bot]]

Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 7772dc4004ab76e4f179e24d39acf1f7c401d437 Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/4495153f-bd50-4a58-a37f-fd975ef058e7

[!NOTE]

• You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

[sonarqubecloud[bot]]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud