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

Handle "Allow in private" for Crypto Wallet and Greaselion #7579

Merged
merged 2 commits into from
Jan 13, 2021

Conversation

darkdh
Copy link
Member

@darkdh darkdh commented Jan 13, 2021

Resolves brave/brave-browser#13279
Resolves brave/brave-browser#13506

This PR

  1. Reinstall crypto wallet as non component extension while still leverage component updater's update mechanism
  2. Add "not_allowed" to manifest of Greaselion
    (https://developer.chrome.com/docs/extensions/mv2/manifest/incognito/#not_allowed)

Submitter Checklist:

  • There is a ticket for my issue.
  • Used Github auto-closing keywords in the commit message.
  • Wrote a good PR/commit description
  • 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).
  • Requested a security/privacy review as needed.

Reviewer Checklist:

  • 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:

Crypto Wallet

  1. Launch Brave without --show-component-extenstion-options
  2. Open brave://wallet and install wallet
  3. Go to brave://extensions and go to details for Crypto Wallet extension
  4. Allow in private option is available and disabled by default

Crypto Wallet: dApp

  1. Launch Brave without --show-component-extenstion-options
  2. Open brave://wallet and install wallet
  3. Open https://www.cryptokitties.co/ in regular window and setup
  4. The page should displayScreen Shot 2021-01-12 at 17 30 59
  5. Open https://www.cryptokitties.co/ in private window
  6. The page should showScreen Shot 2021-01-12 at 17 31 51
  7. Open brave://extensions/?id=odbfpeeihdkbihmopkbjmoonfanlbfcl and toggle on "Allow in private"
  8. Open https://www.cryptokitties.co/ in private window again
  9. We should be able to see locked wallet waiting to be unlocked

Crypto Wallet: Migration

  1. Launch with older Brave
  2. Open brave://wallet and install wallet
  3. Go to https://goerli-faucet.slock.it/ and request 0.05 ETH
  4. Make sure the balance shows 0.05ETH on goerli test network
  5. Exit and Launch newer Brave with the profile
  6. Open brave://wallet
  7. It should still show 0.05 ETH

Greaselion

  1. Launch Brave with --show-component-extenstion-options
  2. Open brave://extensions and go to details for any greaselion
  3. There is no Allow in private option because it is off by default and can not be toggled on

@darkdh darkdh self-assigned this Jan 13, 2021
Copy link
Member

@bbondy bbondy left a comment

Choose a reason for hiding this comment

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

Looks great, thanks! Please just make sure that existing data is preserved before merging.

@bbondy
Copy link
Member

bbondy commented Jan 13, 2021

@srirambv @ryanml can you both test this to ensure no data loss for an existing profile before it is merged?

@darkdh
Copy link
Member Author

darkdh commented Jan 13, 2021

verified migrate from component extension to normal extension, the ETH from faucet on testnet is still there

@ryanml ryanml self-requested a review January 13, 2021 04:27
Copy link
Contributor

@ryanml ryanml left a comment

Choose a reason for hiding this comment

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

++ looks good to me, confirmed no data loss

@darkdh darkdh force-pushed the component-allow-in-private branch from 8ff9e2c to 6bbdfb1 Compare January 13, 2021 04:54
@srirambv
Copy link
Contributor

srirambv commented Jan 13, 2021

Verification passed on

Brave 1.20.76 Chromium: 88.0.4324.79 (Official Build) nightly (64-bit)
Revision bd1e9353659b2491dac971226a973ca3b5684a14-refs/branch-heads/4324@{#1520}
OS Linux
  • Verified no data loss on wallet after upgrade from 1.20.74
  • Verified Allow in private is disabled by default
  • Verified enabling it allows the extension to run in Private/Tor windows
Private Tor
image image

@srirambv
Copy link
Contributor

This PR should also try to fix brave/brave-browser#13535

@darkdh darkdh merged commit 4d3f236 into master Jan 13, 2021
@darkdh darkdh deleted the component-allow-in-private branch January 13, 2021 20:14
@darkdh darkdh added this to the 1.20.x - Nightly milestone Jan 13, 2021
darkdh added a commit that referenced this pull request Jan 14, 2021
Handle "Allow in private" for Crypto Wallet and Greaselion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants