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

Fix wrong default service name and account name are used when getting #10108

Merged
merged 1 commit into from
Sep 16, 2021

Conversation

darkdh
Copy link
Member

@darkdh darkdh commented Sep 15, 2021

keychain password. Also fix getting correct keychain password when
importing from chrome, brave and chromium

In C94 GetServiceName and GetAccountName were calling upstream one from upstream GetPassword
Import didn't work correctly because BRAVE_KEYCHAIN_PASSWORD_GET_PASSWORD had no effect

Resolves brave/brave-browser#18138

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

Check keychain access

  1. Open Keychain Access.app
  2. Under Local Items->All Items and find Chromium Safe Storage and Brave Safe Storage
  3. Open them separately and delete all items in access control list
  4. Launch Brave with this fix
  5. Brave should prompts asking the access to Brave Safe Storage instead of Chromium Safe Storage

Import

  1. Make sure chrome have stored passwords
  2. Open Brave and import passwords from chrome
  3. Allow access to chrome safe storage
  4. passwords should be successfully imported
  5. change chrome to chromium and repeat step 1-4

Migration

  1. Store passwords, credit cards and setup sync in pre C94 Brave
  2. Upgrade to C94 Brave
  3. check passwords, credit cards are gone and sync is not working
  4. Upgrade to Brave with this fix
  5. Passwords, credit cards in step 1 should be back and sync is working

@darkdh
Copy link
Member Author

darkdh commented Sep 15, 2021

test will be addressed in follow up brave/brave-browser#18145

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.

Can we check this in a test to make sure the Brave service and account name is returned?

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.

test will be addressed in follow up brave/brave-browser#18145
Sorry just saw that now.

keychain password. Also fix getting correct keychain password when
importing from chrome, brave and chromium
Copy link
Collaborator

@mkarolin mkarolin left a comment

Choose a reason for hiding this comment

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

++

@darkdh
Copy link
Member Author

darkdh commented Sep 16, 2021

https://ci.brave.com/job/pr-brave-browser-keychain_mac_fix-macos/3/execution/node/218/log/
unrelated failure due to using release node to run macos CI

@darkdh darkdh merged commit 623c3e2 into master Sep 16, 2021
@darkdh darkdh deleted the keychain_mac_fix branch September 16, 2021 04:35
@darkdh darkdh added this to the 1.31.x - Nightly milestone Sep 16, 2021
mkarolin pushed a commit that referenced this pull request Sep 16, 2021
Fix wrong default service name and account name are used when getting
@rebron
Copy link
Collaborator

rebron commented Sep 16, 2021

Updated to 1.31.46 from 1.31.41.

Brave 1.31.46 Chromium: 94.0.4606.41 (Official Build) nightly (arm64)
Revision 333e85df3c9b656b518b5f1add5ff246365b6c24-refs/branch-heads/4606@{#845}
OS macOS Version 12.0 (Build 21A5506j)
JavaScript V8 9.4.146.13
User Agent Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/94.0.4606.41 Safari/537.36

Verified that all of my passwords (164) are restored and sync settings brave://settings/braveSync/setup are back to normal.


Verification PASSED on macOS 11.5.2 x64 using the following build:

Brave | 1.31.37 Chromium: 93.0.4577.63 (Official Build) nightly (x86_64)
-- | --
Revision | ff5c0da2ec0adeaed5550e6c7e98417dac77d98a-refs/branch-heads/4577@{#1135}
OS | macOS Version 11.5.2 (Build 20G95)

Upgrade Cases

Upgraded from 1.31.37Chromium: 93.0.4577.82 to 1.31.46 Chromium: 94.0.4606.41 and ensured that the following data has been preserved after upgrading:

  • Cookies & Passwords
  • Bookmarks/History
  • Autofill (Address & CC Payment)

Check keychain access

Screen Shot 2021-09-16 at 11 36 46 AM

Importing Passwords

  • ensured that the passwords from Chrome imported into Brave after the appropriate Safe Storage permissions were given

Screen Shot 2021-09-16 at 11 41 24 AM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sync stops working and password data loss, cr94 related
4 participants