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

Disconnect Solana dapps when its permission is revoked #19123

Merged
merged 1 commit into from
Jul 6, 2023
Merged

Disconnect Solana dapps when its permission is revoked #19123

merged 1 commit into from
Jul 6, 2023

Conversation

darkdh
Copy link
Member

@darkdh darkdh commented Jun 30, 2023 •
edited
Loading

disconnect event now will be initiated from browser and it won't be emitted when there is no accounts connected

Resolves brave/brave-browser#24974

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • 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 wiki
    • npm run lint, npm run presubmit wiki, 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:

  1. Setup wallet with fresh profile.
  2. Open two tabs and each navigates to different pages and set network to Solana.
  3. Open console on both tabs and type await window.braveSolana.connect()
  4. Give permissions to both tabs, check both panels showing "Connected".
  5. Open brave://settings/content/solana, there should be two entries.
  6. Revoke the entry associated with tab A.
  7. Switch to tab A and open panel, there is no "Connected" status shown.
  8. Switch to tab B and open panel, it should still show "Connected".

@darkdh darkdh requested a review from supermassive June 30, 2023 19:50
@darkdh darkdh requested a review from a team as a code owner June 30, 2023 19:50
@darkdh darkdh self-assigned this Jun 30, 2023
darkdh
darkdh commented Jun 30, 2023
View reviewed changes
browser/brave_wallet/solana_provider_renderer_browsertest.cc
@@ -817,22 +817,6 @@ IN_PROC_BROWSER_TEST_F(SolanaProviderRendererTest, Disconnect) {
}
disconnect();)");
EXPECT_EQ(base::Value(true), result.value);

// OnDisconnect
Copy link
Member Author

Choose a reason for hiding this comment

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

test moved to SolanaProviderTest because it is not renderer initiated anymore

@darkdh darkdh force-pushed the solana-permission-revoked branch 2 times, most recently from dca4066 to 0e9fbc7 Compare June 30, 2023 22:01
@darkdh darkdh requested a review from a team as a code owner June 30, 2023 22:01
supermassive
supermassive reviewed Jul 3, 2023
View reviewed changes
components/brave_wallet/browser/solana_provider_impl.h
base::flat_map<std::string, SignAndSendTransactionCallback>
sign_and_send_tx_callbacks_;
// Pending callback and arg are for waiting user unlock before connect
ConnectCallback pending_connect_callback_;
absl::optional<base::Value::Dict> pending_connect_arg_;

const raw_ref<HostContentSettingsMap> host_content_settings_map_;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: through chromium's code this is typically captured by raw_ptr

Copy link
Member Author

Choose a reason for hiding this comment

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

use raw_ref here is intentional to enforce that host_content_settings_map_ would never be null.
Same usage is in brave/components/permissions/permission_lifetime_manager.h written by @goodov

supermassive
supermassive reviewed Jul 3, 2023
View reviewed changes
browser/brave_wallet/solana_provider_impl_unittest.cc Outdated
// connect the account
Navigate(GURL("https://brave.com"));
AddSolanaPermission(GetOrigin(), added_account->account_id);
std::string account = Connect(absl::nullopt, nullptr, nullptr);
ASSERT_TRUE(!account.empty());
ASSERT_TRUE(IsConnected());

EXPECT_CALL(*observer_, AccountChangedEvent(testing::_)).Times(0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: using testing::_;

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed and squashed

supermassive
supermassive reviewed Jul 3, 2023
View reviewed changes
test/data/brave-wallet/solana_provider.html
function registerDisconnect() {
window.braveSolana.on('disconnect', result => {
disconnectEmitted = true
window.domAutomationController.send('result ready')
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: at some point in future waiting for 'result ready' message should be refactored to waiting for disconnectEmitted promise

Copy link
Member Author

Choose a reason for hiding this comment

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

That is useful when we only want to test the event is emitted but when it is not being emitted there is no obvious way to resolve a different value. If we set a timer to resolve the un-emitted value, the test will be unstable.

supermassive
supermassive reviewed Jul 3, 2023
View reviewed changes
browser/brave_wallet/solana_provider_impl_unittest.cc Outdated
@@ -688,21 +677,23 @@ TEST_F(SolanaProviderImplUnitTest, Disconnect) {
ASSERT_TRUE(!account.empty());
ASSERT_TRUE(IsConnected());

EXPECT_CALL(*observer_, DisconnectEvent()).Times(1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

afaik Times(1) is a noop
or is it just to be explicit and match vs Times(0) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was just to explicitly specify one time but I think I can remove it because without WillOnce() nor WillRepeatedly() it would infer just 1

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed and squashed

supermassive
supermassive approved these changes Jul 3, 2023
View reviewed changes
Copy link
Collaborator

@supermassive supermassive left a comment

Choose a reason for hiding this comment

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

LGTM

@thypon thypon unassigned fmarier and thypon Jul 4, 2023
@thypon
Copy link
Member

thypon commented Jul 4, 2023

Sec gtg

@darkdh
Disconnect Solana dapps when its permission is revoked
cdfc282 
`disconnect` event now will be initiated from browser and it won't be
emitted when there is no accounts connected
@darkdh darkdh force-pushed the solana-permission-revoked branch from 0e9fbc7 to cdfc282 Compare July 5, 2023 21:06
@darkdh darkdh merged commit c4834b2 into master Jul 6, 2023
6 checks passed
@darkdh darkdh deleted the solana-permission-revoked branch July 6, 2023 16:07
@github-actions github-actions bot added this to the 1.55.x - Nightly milestone Jul 6, 2023
@srirambv srirambv mentioned this pull request Aug 31, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions GitHub Actions github-actions left review comments

@supermassive supermassive supermassive approved these changes

@StephenHeaps StephenHeaps StephenHeaps approved these changes

Assignees

@darkdh darkdh

Labels
feature/web3/wallet/core feature/web3/wallet needs-security-review
Projects
None yet
Milestone
1.58.x - Release
Development

Successfully merging this pull request may close these issues.

Solana connected status should be changed when permission is revoked
5 participants
@darkdh @thypon @supermassive @StephenHeaps @fmarier