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.

Sign up for GitHub

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

Jump to bottom

Solana permission context #12945

Merged
merged 17 commits into from
Apr 21, 2022
Merged

Solana permission context #12945

merged 17 commits into from
Apr 21, 2022

Conversation

darkdh
Copy link
Member

@darkdh darkdh commented Apr 8, 2022 •
edited
Loading

Resolves brave/brave-browser#21765
security review: https://github.com/brave/security/issues/824

  1. Add new Solana content setting/permission request type, UI setting page included.
  2. Update BraveWalletPermissionContext for multi coin support
  3. Use BraveWalletProdiverImpl to handle permission requests from both Ethereum and Solana
  4. Generalize BraveWalletService::Add/Has/Reset Permission for multi coin support
  5. Finish up provider API (https://docs.phantom.app/integrating/extension-and-mobile-browser/establishing-a-connection)
    • solana.connect
      • eagerly connect (onlyIfTrusted), which means it can only connect when site has permission, otherwise it will automatically reject the request.
    • solana.disconnect
      • Note this will not remove site permission. The only way to remove site permission is through content setting (Trusted app on Phantom)
    • solana.isConnected
    • solana.onAccountChanged
      • This will be fired whenever Solana selected account is changed, it will be null value when the account is not in connected state
  6. Phantom maintains a non-persistent state of connected in addition to site permission (Trusted app in Phantom) which would be addressed in Solana connected status brave-browser#22184
  7. Misc
    • Prevent BraveWalletProviderDelegateImpl from using raw pointer of KeyringService, only in component code can have access to it, otherwise use mojo interface
    • GetActiveWebContents in WalletPanelUI instead of passing callback function since the CreatePanelHandler is async active WebContents might change
    • Intermittent BraveWalletSignMessageBrowserTest failure fix
    • Rename original SolanaProviderTest to SolanaProviderRendererTest

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

@github-actions github-actions bot added the potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false label Apr 8, 2022
@darkdh darkdh force-pushed the solana-permission-context branch 4 times, most recently from 30d2c58 to 56dea41 Compare April 13, 2022 22:23
@darkdh darkdh marked this pull request as ready for review April 13, 2022 22:55
@darkdh darkdh requested review from a team as code owners April 13, 2022 22:55
@darkdh darkdh requested review from bbondy and yrliou April 13, 2022 22:55
@github-actions github-actions bot added rebase and removed rebase labels Apr 13, 2022
@darkdh darkdh added this to the 1.39.x - Nightly milestone Apr 13, 2022
@github-actions github-actions bot added rebase and removed rebase labels Apr 13, 2022
@darkdh darkdh force-pushed the solana-permission-context branch 2 times, most recently from ad87e4a to d841e72 Compare April 14, 2022 00:29
darkdh
darkdh commented Apr 14, 2022
View reviewed changes
android/java/org/chromium/chrome/browser/crypto_wallet/util/AccountsPermissionsHelper.java
@@ -45,8 +46,8 @@ public void checkAccounts(Runnable runWhenDone) {

accountsPermissionsContexts.add(accountPermissionContext);

mBraveWalletService.hasEthereumPermission(
mOrigin, account.address, accountPermissionContext);
mBraveWalletService.hasPermission(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @SergeyZhukovsky for related android change

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

java changes looks ok

@darkdh darkdh force-pushed the solana-permission-context branch 3 times, most recently from b4cea76 to 4286ac7 Compare April 14, 2022 02:09
SergeyZhukovsky
SergeyZhukovsky approved these changes Apr 14, 2022
View reviewed changes
Copy link
Member

@SergeyZhukovsky SergeyZhukovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Android side ++

@darkdh darkdh force-pushed the solana-permission-context branch 3 times, most recently from d48701e to 7aac572 Compare April 14, 2022 18:37
@darkdh darkdh force-pushed the solana-permission-context branch 7 times, most recently from 597c58d to e300b10 Compare April 21, 2022 18:55
kylehickinson
kylehickinson approved these changes Apr 21, 2022
View reviewed changes
Copy link
Collaborator

@kylehickinson kylehickinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iOS++ after new changes

@darkdh
Add Solana type to content setting and permission along with setting UI
385e5c3
@darkdh
Use BraveWalletPermissionContext for both Ethereum and Solana
ceb1a38 
Many interfaces now require content setting type or permission request type
@darkdh
Add SolanaProviderDelegate and tie it with SolanaProvider::Connect
a88b0a9 
Also prevent BraveWalletProviderDelegateImpl from using
raw pointer of KeyringService
@darkdh
Add Solana to BraveWalletPermissionPromptImpl
58d1422
@darkdh
Merge SolanaProviderDelegateImpl into BraveWalletProdiverImpl
dd3951b 
and fix showing correct user reject request error instead of internal error
@darkdh
Rename BraveWalletProviderImpl to EthereumProviderImpl
fc7fce1
@darkdh
Implement Disconnect, IsConnected, OnAccountChanged and onlyIfTrusted…
04861fd 
… option

for Connect
@darkdh
Generalize Add/Has/Reset Permission for both Ethereum and Solana
7022558
@darkdh
Add SolanaProviderImplUnitTest
c355cc5
@darkdh
GetActiveWebContents in WalletPanelUI instead of passing callback
4107a64 
function since the `CreatePanelHandler` is async active WebContents
might change. And the panel is not shared accross tabs so it is safe to
bind webcontent when panel is created
@darkdh
Fix intermittent BraveWalletSignMessageBrowserTest by calling
3365013 
ethereum.enable() manually and wait
@darkdh
Rename brave_ethereum_permission_prompt_android.* to /brave_wallet_pe…
561bb52 
…rmission_prompt_android.*
@darkdh
Finish BraveWalletProviderDelegateBridge implementation on iOS
bd0dbe3
@darkdh
Rename SolanaProviderTest to SolanaProviderRendererTest
ba45ac8
@darkdh
Add SolanaProviderTest.* which are ConnectedStatusAndPermission and
da6de58 
ConnectedStatusInMultiFrames
@darkdh
Generalize BraveWalletProviderDelegate interfaces and move provider
1380fa7 
specific logic back to EthereumProvider and SolanaProvider
@darkdh
Adapt BraveWalletProviderBridge to new interfaces on iOS
dcb1d88
@darkdh darkdh merged commit 575cd04 into master Apr 21, 2022
@darkdh darkdh deleted the solana-permission-context branch April 21, 2022 22:19
@kdenhartog kdenhartog mentioned this pull request Sep 17, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kdenhartog kdenhartog kdenhartog approved these changes

@Douglashdaniel Douglashdaniel Douglashdaniel approved these changes

@SergeyZhukovsky SergeyZhukovsky SergeyZhukovsky approved these changes

@yrliou yrliou yrliou approved these changes

@kylehickinson kylehickinson kylehickinson approved these changes

@bridiver bridiver bridiver approved these changes

@bbondy bbondy Awaiting requested review from bbondy

Assignees

@darkdh darkdh

Labels
potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false
Projects
None yet
Milestone
1.40.x - Release
Development

Successfully merging this pull request may close these issues.

Solana provider browser process integration: PermissionContext
7 participants
@darkdh @bridiver @kylehickinson @yrliou @SergeyZhukovsky @kdenhartog @Douglashdaniel