fix: pw protect seed during backup after on-board #14541

josheleonard · 2022-08-09T00:39:29Z

Resolves brave/brave-browser#24534

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:

Screen.Recording.2022-08-09.at.11.08.10.AM.mov

brave-builds · 2022-08-09T01:41:41Z

A Storybook has been deployed to preview UI for the latest push

Douglashdaniel

LGTM!

onyb · 2022-08-09T10:19:47Z

@josheleonard Can you please upload a screen recording?

components/brave_wallet_ui/page/screens/backup-wallet/intro/backup-wallet-intro.tsx

kdenhartog

Just did a quick static analysis and it looks good. I'll grab the build from CI and make sure it's working as expected and can give a recording for @onyb as well while I'm at it

kdenhartog · 2022-08-09T11:19:23Z

Hmm not sure if this is doing something odd with this specifically, but I just encountered a delay in opening the wallet panel button now and then after testing once, resetting wallet and trying again while recording I got a crash. Would this be specific to something we're introducing here by chance?

kdenhartog · 2022-08-09T11:27:18Z

@onyb this is a recording of how it should be working, but I seem to get quite a bit of crashing on this build now. Can anyone else reproduce these crashes?

kdenhartog

From a security perspective, could be good to refactor that maxAttempts number to be set once by the function rather than by the caller, but that's non blocking and this looks good to me.

Let's double check that the crashing isn't related to this change and then I think it's good to go

josheleonard · 2022-08-09T16:43:21Z

Taking a look at the crashing issue now.

josheleonard · 2022-08-09T17:08:59Z

I tried recreating this by resetting the wallet and onboarding a second time while recording. No crash here on M1 mac.

Screen.Recording.2022-08-09.at.11.04.03.AM.mov

josheleonard · 2022-08-09T17:18:20Z

From a security perspective, could be good to refactor that maxAttempts number to be set once by the function rather than by the caller, but that's non blocking and this looks good to me.

Let's double check that the crashing isn't related to this change and then I think it's good to go

Refactored here: 68792720acfaf1091b23a0687b4afc5095e18ce7

brave-builds · 2022-08-09T18:27:30Z

A Storybook has been deployed to preview UI for the latest push

kdenhartog · 2022-08-09T21:17:08Z

I tried recreating this by resetting the wallet and onboarding a second time while recording. No crash here on M1 mac.

Screen.Recording.2022-08-09.at.11.04.03.AM.mov

👍🏻 I'll double check this either this afternoon or tomorrow to try and build locally to see if it was just a problem with how I was using it.

After doublechecking this it seems like it's still failing for me even with a fresh install.

From a security perspective, could be good to refactor that maxAttempts number to be set once by the function rather than by the caller, but that's non blocking and this looks good to me.
Let's double check that the crashing isn't related to this change and then I think it's good to go

Refactored here: 6879272

Sounds good thanks for updating that

josheleonard · 2022-08-10T21:13:11Z

I tried recreating this by resetting the wallet and onboarding a second time while recording. No crash here on M1 mac.
Screen.Recording.2022-08-09.at.11.04.03.AM.mov

👍🏻 I'll double check this either this afternoon or tomorrow to try and build locally to see if it was just a problem with how I was using it.

After doublechecking this it seems like it's still failing for me even with a fresh install.

From a security perspective, could be good to refactor that maxAttempts number to be set once by the function rather than by the caller, but that's non blocking and this looks good to me.
Let's double check that the crashing isn't related to this change and then I think it's good to go

Refactored here: 6879272

Sounds good thanks for updating that

@kdenhartog, were you able to confirm that this branch is causing crashes, or is it related to something on master?

brave-builds · 2022-08-16T20:39:44Z

A Storybook has been deployed to preview UI for the latest push

muliswilliam · 2022-08-17T12:18:49Z

components/brave_wallet_ui/page/screens/backup-wallet/intro/backup-wallet-intro.tsx

+    setPassword(value)
+  }
+
+  const onContinue = async () => {


Can you add useCallback?

muliswilliam

Minor change

kdenhartog · 2022-08-18T20:58:48Z

Hey it seems that we're going to move away from a UI based approach for this based on this discussion: https://bravesoftware.slack.com/archives/C023VS4HJ6Q/p1660751349341859

@yrliou are you the one who's going to help coordinate this?

yrliou · 2022-08-18T21:57:06Z

@kdenhartog I can help prepare a backend commit that makes GetMnemonicForDefaultKeyring API taking a password argument and validate it in the backend. @josheleonard and someone from the Android team would need to push the UI change to this PR.

yrliou · 2022-08-18T23:55:14Z

Created b2f2893 for backend API protection that's ready to be cherry-picked once this PR branch is rebased on master.

yrliou · 2022-08-19T00:16:10Z

@darkdh Please help review the backend commit at 5f53a5c, thanks.

brave-builds · 2022-08-19T01:50:12Z

A Storybook has been deployed to preview UI for the latest push

kdenhartog · 2022-08-19T06:56:32Z

browser/brave_wallet/keyring_service_unittest.cc


  // unlock with wrong password
-  ASSERT_FALSE(Unlock(&service, "brave123"));
+  ASSERT_FALSE(Unlock(&service, kPasswordBrave123));


Nice to see it locks after 1 failed attempt here since that's simpler for state management at this layer.

It's not because of 1 failed attempt, it's just still locked because this line try to unlock with wrong password.

kdenhartog

Reviewed the API layer changes made by @yrliou

Looks like we'd just need to do the refactor to use this and then this would be good to go

kdenhartog · 2022-08-19T06:58:06Z

components/brave_wallet/browser/keyring_service.cc

@@ -2137,11 +2143,9 @@ void KeyringService::IsStrongPassword(const std::string& password,
  std::move(callback).Run(true);
 }

-void KeyringService::ValidatePassword(const std::string& password,
-                                      ValidatePasswordCallback callback) {
+bool KeyringService::ValidatePasswordInternal(const std::string& password) {


looks good to me, thanks for the fix!

darkdh

backend++

brave-builds · 2022-08-23T00:10:53Z

A Storybook has been deployed to preview UI for the latest push

muliswilliam

LGTM

SergeyZhukovsky · 2022-08-23T18:54:53Z

@Pavneet-Sing would you mind to review Android fixes? I didn't post a video as we hide onboarding with password prompts for a sec reasons, but it does similar as desktop plus uses biometric if it's set.

brave-builds · 2022-08-23T20:01:37Z

A Storybook has been deployed to preview UI for the latest push

...romium/chrome/browser/crypto_wallet/fragments/onboarding_fragments/BackupWalletFragment.java

android/java/res/layout/fragment_backup_wallet.xml

browser/ui/android/strings/android_brave_strings.grd

…al on Android

Pavneet-Sing

LGTM

SergeyZhukovsky

++

brave-builds · 2022-08-24T14:49:50Z

A Storybook has been deployed to preview UI for the latest push

yrliou · 2022-08-24T17:33:53Z

[2022-08-24T13:41:12.432Z] FAIL components/test/brave_new_tab_ui/components/settings_test.tsx

[2022-08-24T13:41:12.432Z]   ● Test suite failed to run

[2022-08-24T13:41:12.432Z] 

[2022-08-24T13:41:12.432Z]     Cannot find module '$web-common/locale' from 'components/brave_new_tab_ui/components/default/searchPromotion/searchBox.tsx'

[2022-08-24T13:41:12.432Z] 

[2022-08-24T13:41:12.432Z]     Require stack:

[2022-08-24T13:41:12.432Z]       components/brave_new_tab_ui/components/default/searchPromotion/searchBox.tsx

[2022-08-24T13:41:12.432Z]       components/brave_new_tab_ui/components/default/searchPromotion/index.tsx

[2022-08-24T13:41:12.432Z]       components/brave_new_tab_ui/components/default/index.ts

[2022-08-24T13:41:12.432Z]       components/test/brave_new_tab_ui/components/settings_test.tsx

[2022-08-24T13:41:12.432Z] 

[2022-08-24T13:41:12.432Z]       1 | import * as React from 'react'

[2022-08-24T13:41:12.432Z]       2 | import styled from 'styled-components'

[2022-08-24T13:41:12.432Z]     > 3 | import { getLocale } from '$web-common/locale'

[2022-08-24T13:41:12.432Z]         | ^

[2022-08-24T13:41:12.432Z]       4 |

[2022-08-24T13:41:12.432Z]       5 | const Box = styled.div`

[2022-08-24T13:41:12.432Z]       6 |   display: flex;

[2022-08-24T13:41:12.432Z] 

[2022-08-24T13:41:12.432Z]       at Resolver.resolveModule (node_modules/jest-resolve/build/index.js:306:11)

[2022-08-24T13:41:12.432Z]       at Object.<anonymous> (components/brave_new_tab_ui/components/default/searchPromotion/searchBox.tsx:3:1)

test-javascript failure on linux, windows-x86, windows, it's known brave/brave-browser#24787 and irrelevant to this PR.

yrliou · 2022-08-24T22:46:46Z

For macOS https://ci.brave.com/job/pr-brave-browser-fix-pw-protect-seed-during-backup-macos/8/ was successful with only the same javascript test error above, after that it's only android changes due to review which won't affect macOS build at all.

yrliou · 2022-08-24T22:55:05Z

Merging, see above comments for CI notes.

josheleonard requested review from onyb, Douglashdaniel, muliswilliam and a team August 9, 2022 00:39

josheleonard self-assigned this Aug 9, 2022

github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Aug 9, 2022

Douglashdaniel approved these changes Aug 9, 2022

View reviewed changes

kdenhartog reviewed Aug 9, 2022

View reviewed changes

components/brave_wallet_ui/page/screens/backup-wallet/intro/backup-wallet-intro.tsx Outdated Show resolved Hide resolved

kdenhartog reviewed Aug 9, 2022

View reviewed changes

kdenhartog approved these changes Aug 9, 2022

View reviewed changes

josheleonard force-pushed the fix-pw-protect-seed-during-backup branch from 6879272 to fa88d2a Compare August 16, 2022 18:40

muliswilliam reviewed Aug 17, 2022

View reviewed changes

muliswilliam suggested changes Aug 17, 2022

View reviewed changes

josheleonard added 2 commits August 18, 2022 18:13

fix: pw protect seed during backup after on-board

792c97f

fix: remove option to set number of pw attempts

eb99d97

josheleonard force-pushed the fix-pw-protect-seed-during-backup branch from fa88d2a to eb99d97 Compare August 19, 2022 00:13

fix: update mnemonic reveal calls to use password

9a3e8de

kdenhartog reviewed Aug 19, 2022

View reviewed changes

yrliou mentioned this pull request Aug 19, 2022

feat: password-protect wallet account removal #14669

Merged

25 tasks

darkdh approved these changes Aug 19, 2022

View reviewed changes

fix: memoize backup wallet intro step methods

adb81ac

muliswilliam self-requested a review August 23, 2022 05:09

muliswilliam approved these changes Aug 23, 2022

View reviewed changes

SergeyZhukovsky requested a review from Pavneet-Sing August 23, 2022 18:53

SergeyZhukovsky self-assigned this Aug 23, 2022

Pavneet-Sing reviewed Aug 24, 2022

View reviewed changes

SergeyZhukovsky force-pushed the fix-pw-protect-seed-during-backup branch from 717f920 to 0e0283e Compare August 24, 2022 13:27

Adds a password prompt or biometric input before mnemonic phrase reve…

3a3c3fa

…al on Android

SergeyZhukovsky force-pushed the fix-pw-protect-seed-during-backup branch from 0e0283e to 3a3c3fa Compare August 24, 2022 13:32

Pavneet-Sing approved these changes Aug 24, 2022

View reviewed changes

SergeyZhukovsky approved these changes Aug 24, 2022

View reviewed changes

yrliou self-assigned this Aug 24, 2022

yrliou merged commit af36931 into master Aug 24, 2022

yrliou deleted the fix-pw-protect-seed-during-backup branch August 24, 2022 22:55

github-actions bot added this to the 1.45.x - Nightly milestone Aug 24, 2022

srirambv mentioned this pull request Oct 7, 2022

Add additional Password-protection for Wallet Backup Seed reveal brave/brave-browser#24534

Closed

fix: pw protect seed during backup after on-board #14541

fix: pw protect seed during backup after on-board #14541

Conversation

josheleonard commented Aug 9, 2022 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

brave-builds commented Aug 9, 2022

Douglashdaniel left a comment

Choose a reason for hiding this comment

onyb commented Aug 9, 2022

kdenhartog left a comment

Choose a reason for hiding this comment

kdenhartog commented Aug 9, 2022

kdenhartog commented Aug 9, 2022

kdenhartog left a comment

Choose a reason for hiding this comment

josheleonard commented Aug 9, 2022

josheleonard commented Aug 9, 2022

josheleonard commented Aug 9, 2022

brave-builds commented Aug 9, 2022

kdenhartog commented Aug 9, 2022 • edited Loading

josheleonard commented Aug 10, 2022

brave-builds commented Aug 16, 2022

muliswilliam Aug 17, 2022

Choose a reason for hiding this comment

muliswilliam left a comment

Choose a reason for hiding this comment

kdenhartog commented Aug 18, 2022

yrliou commented Aug 18, 2022

yrliou commented Aug 18, 2022

yrliou commented Aug 19, 2022

brave-builds commented Aug 19, 2022

kdenhartog Aug 19, 2022

Choose a reason for hiding this comment

yrliou Aug 19, 2022

Choose a reason for hiding this comment

kdenhartog left a comment

Choose a reason for hiding this comment

kdenhartog Aug 19, 2022

Choose a reason for hiding this comment

darkdh left a comment

Choose a reason for hiding this comment

brave-builds commented Aug 23, 2022

muliswilliam left a comment

Choose a reason for hiding this comment

SergeyZhukovsky commented Aug 23, 2022

brave-builds commented Aug 23, 2022

Pavneet-Sing left a comment

Choose a reason for hiding this comment

SergeyZhukovsky left a comment

Choose a reason for hiding this comment

brave-builds commented Aug 24, 2022

yrliou commented Aug 24, 2022

yrliou commented Aug 24, 2022

yrliou commented Aug 24, 2022

josheleonard commented Aug 9, 2022 •

edited

Loading

kdenhartog commented Aug 9, 2022 •

edited

Loading