1641777 - Import logins from Firefox login export CSV files

Status: VERIFIED FIXED

(Toolkit :: Password Manager, enhancement, P2)

Description

[Matthew N. [:MattN]]

Before supporting import from other password managers, let's get the basic of importing our own export files working.

Comment 1

[Andrei Petcu]

Attachment: Bug 1641777 - Import logins from Firefox, Bitwarden, & Chrome CSV files

Depends on D77593

Comment 2

[Andrei Petcu]

What icon should I use for the import? Same as the other import or the other import icon?
https://design.firefox.com/icons/viewer/#import

Comment 3

[Matthew N. [:MattN]]

shorlander suggested https://design.firefox.com/icons/viewer/#open

Comment 4

[Andrei Petcu]

How should the UI look? Should we ask for confirmation? master password? success message?

Comment 5

[Matthew N. [:MattN]]

(In reply to petcuandrei from comment #4)

How should the UI look?

I think the minimum would be the new menu item opens a Open File picker which immediately imports using LoginHelper.maybeImport… and then tells you how many logins were import and how many were updated.

Alternatively we could re-add a browse button to the Import from Another Browser wizard (like we used to have for bookmarks.html). It already has a confirmation screen (that currently lies to you but I would love to fix that for all imports).

Should we ask for confirmation?

IMO this would only be useful if we gave the the user enough context to make an informed decision… that might even require show the exact logins that will be changed and how they will be changed but that sounds like too much…

master password?

This part I can answer: We don't need this since we only use MP to protect reading the plaintext password (not deleting/overwriting) and import isn't reading. The user will automatically get a MP prompt upon import if the MP happens to be locked.

success message?

See above. Ideally we would indicate how many were updated and how many were added but idk what UI we would use for that if we don't do it in the wizard dialog. I don't recall having any kind of "toast" UI in about:logins atm. I'm not sure when Katie will be able to help with this. For now I think do my minimal suggestion without the confirmation and we can add that in a separate bug if Katie doesn't have time to design it yet.

Comment 6

[Andrei Petcu]

I'm getting mixed messages here.

1. use the success UI from the wizard (which for some reason is disabled on my machine, maybe due to the OS)
2. not add a success message since there is no toast and Katie does not have time for this

What am I missing? :)
Also, if we do show some success message, the skipped rows would also be useful since there might be issues with the CSV or with the import system.

Comment 7

[katieC]

Hi Andrei - thanks for all your time and thought on this. I really appreciate your patience as my ux time is limited (at the moment!) for logins and passwords. The ideal situation is UX creates a user flow diagram with the team, then creates wireframes for all surfaces the user will interact with - then you'd be able to reference those docs and have answers and content for these questions... but here we are.

Small update so you're in loop, Matt and I will chat this week to discuss timeline and ux issues.
Again, thanks for raising some of those ux issues here and for your patience!

Comment 8

[Andrei Petcu]

We can export items without a httpRealm and a formActionOrigin but we get an error when creating a new item. "Can't add a login without a httpRealm or formActionOrigin.". What should I do?

Comment 9

[Tom S]

I suspect you can just use null. That is what the ChromeProfileMigrator does: https://searchfox.org/mozilla-central/source/browser/components/migration/ChromeProfileMigrator.jsm#544-545

Comment 10

[Andrei Petcu]

My CSV parser was way off :) The JSON password messed up the parser and ended up setting both httpRealm and formActionOrigin.
I fixed it now and it has a simpler implementation, easier to read.

I also noticed a problem in the export: I always quote all the values. That's not a big deal, it's still valid CSV. The problem is that if I edit the CSV with LibreOffice, it will change ALL THE LINES and remove the quotes where they are not needed. Now I changed the export to only add quotes when needed.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/10c9e57fb4f8
Import logins from Firefox, Bitwarden, & Chrome CSV files r=MattN,flod

Comment 12

[Razvan Maries]

Backed out for bc failures.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&searchStr=linux%2C18.04%2Cx64%2Casan%2Copt%2Cmochitests%2Ctest-linux1804-64-asan%2Fopt-mochitest-browser-chrome-e10s-9%2Cm%28bc9%29&revision=10c9e57fb4f8c70d4772a7809ba3d652536b984d&selectedTaskRun=NpZyTYl_Ti-OAoF9lNKTaQ.0

Log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=308556466&repo=autoland&lineNumber=3070

Backout: https://hg.mozilla.org/integration/autoland/rev/46c1c161cbe189a59d8274e011085d76163db7f4

Comment 13

[Andrei Petcu]

d3 path changed. will update soon.

Comment 14

[Andrei Petcu]

$hg update central --clean
15858 files updated, 0 files merged, 12238 files removed, 0 files unresolved

$moz-phab patch D77603 --no-bookmark
Patching revisions: D75716 D77324 D77593 D77603
Checked out 9dae371f2b33
D75716 applied
patching file browser/components/aboutlogins/tests/browser/browser_openExport.js
Hunk #3 FAILED at 69
1 out of 4 hunks FAILED -- saving rejects to file browser/components/aboutlogins/tests/browser/browser_openExport.js.rej
patching file browser/components/aboutlogins/content/aboutLogins.js
Hunk #1 FAILED at 123
1 out of 1 hunks FAILED -- saving rejects to file browser/components/aboutlogins/content/aboutLogins.js.rej
abort: patch failed to apply
CommandError: command 'hg' failed to complete successfully
Run moz-phab again with '--trace' to show debugging output
Sentry is attempting to send 0 pending error messages
Waiting up to 2 seconds
Press Ctrl-C to quit

Comment 15

[Andrei Petcu]

I ran this command and the tests pass locally
moz-phab patch D77603 --apply-to . --skip-dependencies

I'm on Fedora.

Comment 16

[Matthew N. [:MattN]]

Try push in progress with the failing linux tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c39857ee4dff999cc6d6785cf2fef1f0c863bb72

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/80886c7f83dd
Import logins from Firefox, Bitwarden, & Chrome CSV files r=MattN,flod

Comment 18

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/80886c7f83dd

Comment 19

[Simona Rosu [:srosu], Ecosystem QA]

I have verified this issue using the latest Firefox Nightly 80.0a1 (Build ID: 20200707213111) on Windows 10 x64, Ubuntu 18.04, Mac 10.15.

• The “Import from a file…” option is displayed in the Ellipsis menu and has the correct icon.
• After clicking the “Import from a file…” option the “Import Logins File” picker is shown.
• The logins are correctly imported from the CSV file after clicking on the “Open” button from the “Import Logins File” picker.

Comment 20

[Matthew N. [:MattN]]

The feature is now disabled in Fx80 outside Nightly via signon.management.page.fileImport.enabled

Comment 21

[Dev Singh]

The import from CSV doesn't seem to discard/override duplicates between the CSV import and the passwords already stored in lockwise. Is this intended behavior and/or something that the community would be interested in having? If so, if someone could point me towards which file the actual importing takes place in I could work on that.

Comment 22

[Andrei Petcu]

what are the steps to reproduce?

Comment 23

[Dev Singh]

On windows 10:

Install firefox nightly, log into my firefox account (which already has some passwords for accounts.google.com)

Import CSV from bitwarden

After the importing has completed, I see two entries for accounts.google.com for the same email and same password.

Comment 24

[Andrei Petcu]

Dev Singh can you please re-test with the current implementation in Nightly? If you still have issues please provide a CSV file with data similar to your use case.

Comment 25

[Dev Singh]

(In reply to petcuandrei from comment #24)

Dev Singh can you please re-test with the current implementation in Nightly? If you still have issues please provide a CSV file with data similar to your use case.

Hi,
I am no longer having any issues.

Comment 26

[Mathew Hodson]

This is enabled in all channels now.