Add 1Password 1PUX and Bitwarden JSON Importers #9815

droidmonkey · 2023-09-01T02:44:55Z

Add 1Password 1PUX and Bitwarden JSON Importers:

Closes Support for importing 1Password 1pux file format #7545 - Support 1Password 1PUX import format based on https://support.1password.com/1pux-format/
Closes Import bitwarden json format #8367 - Support Bitwarden JSON import format (both unencrypted and encrypted) based on https://bitwarden.com/help/encrypted-export/
Fixes 1Password import changes username field, data loss (value dropped when username field is not username) #9577 - OPVault import when fields have the same name or type
Introduce the import wizard to handle all import tasks (CSV, KDBX1, OPVault, 1PUX, JSON)
Clean up CSV parser code to make it much more efficient and easier to read
Combine all importer tests (except CSV) into one test file

Also included some tangentially related changes while doing the Import Wizard work...

Minor changes to Group API to make it more explicit:

Include check for group as recycle bin directly into the Group::isRecycled() function
Return the original root group from Database::setRootGroup(...) to force memory management transfer

Fix Spacing of QGroupBox:

Previously our base style sheet added roughly 20px of margin to the top and bottom of all QGroupBox. This caused visual errors where that margin was not needed/desired.
Transferred padding to the specific layouts instead where it belongs.

Screenshots

Testing strategy

Tested on Windows and wrote unit tests

Type of change

✅ New feature (change that adds functionality)
✅ Refactor (significant modification to existing code)

src/gui/csvImport/CsvImportWidget.cpp

michaelk83 · 2023-09-01T10:02:56Z

If all the import options are files, then you can use a the same file selector for all of them, and detect the source type from the extension (+ header if available). Then the type selection from the 1st screenshot isn't needed.
The credentials are only needed after the database selection (and only if the db isn't already open), so can be on a separate step. It should reuse the existing new/open database widgets.
I would suggest, therefore, to leave only the file chooser and database selection on the 1st wizard page. You may indicate the import type after a file has been selected, but this only needs a simple label. On the next step, reuse the existing new/open database widgets if the database is not already open.
The existing database dropdown should default to the current database. Or you may add an explicit "Current Database (name)" radio button.
An alternative route might be to drop the 1st page entirely, and have two menu actions: Database > Import to New... and Database > Import to Current.... But this would lose the "Go Back" feature of the wizard:
- Import to Current would immediately open the file chooser, and proceed with the import from there.
- Import to New would open the usual New Database window. Once the empty database is created, it would automatically call Import to Current.

Since this is a large PR, I'll take this opportunity to remind you of #9701, #8480, #8591 (and #8983), just in case. Though I'm sure you already remember.

droidmonkey · 2023-09-01T10:06:53Z

Thanks but I'm not going to make any of those changes. I'm done working on this and it works really well.

The credential fields are for the import file not the database. Some file types are encrypted.

michaelk83 · 2023-09-01T10:35:24Z

It's your choice, of course, but IMO the 1st page is much more complicated than it needs to be. This is my preferred route:

I would suggest, therefore, to leave only the file chooser and database selection on the 1st wizard page. You may indicate the import type after a file has been selected, but this only needs a simple label.

In particular, the credentials seem completely out of place.

The credential fields are for the import file not the database. Some file types are encrypted.

Ah, I see. That is not clear at all from this UI. And in any case, I would still leave them to a separate wizard page, and only if they're needed.

The other thing is a just minor convenience, since importing to the current database is likely to be the 2nd most common selection:

The existing database dropdown should default to the current database. Or you may add an explicit "Current Database (name)" radio button.

This would reduce the number of clicks to select the current database from 3 to 1.

michaelk83 · 2023-09-01T10:57:59Z

If you insist on leaving the credentials on the 1st page, to make it clearer you could move them right under the file selector, and group them together.

droidmonkey · 2023-09-01T11:07:33Z

I might try the visibility of the fields again, last time I tested that the wizard dialog got all hokey. I am also playing in the constraints of a qt horror show.

droidmonkey · 2023-09-27T20:35:36Z

@michaelk83 do you like this better?

Also the currently visible database (if unlocked) is default selected for the existing database.

michaelk83 · 2023-09-28T13:05:53Z

Yeah, this is much better.

codecov · 2023-10-01T11:39:38Z

Codecov Report

Attention: Patch coverage is 47.63285% with 542 lines in your changes are missing coverage. Please review.

Project coverage is 63.64%. Comparing base (0acb15d) to head (e96d34d).

Files	Patch %	Lines
src/gui/wizard/ImportWizardPageSelect.cpp	0.00%	160 Missing ⚠️
src/gui/wizard/ImportWizardPageReview.cpp	0.00%	123 Missing ⚠️
src/gui/csvImport/CsvImportWidget.cpp	0.00%	92 Missing ⚠️
src/gui/wizard/ImportWizard.cpp	0.00%	38 Missing ⚠️
src/gui/DatabaseTabWidget.cpp	15.00%	34 Missing ⚠️
src/format/BitwardenReader.cpp	86.73%	30 Missing ⚠️
src/gui/csvImport/CsvParserModel.cpp	0.00%	22 Missing ⚠️
src/format/OPUXReader.cpp	90.64%	19 Missing ⚠️
src/format/OpVaultReader.cpp	37.04%	17 Missing ⚠️
src/gui/DatabaseWidget.cpp	88.00%	3 Missing ⚠️
... and 3 more

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #9815      +/-   ##
===========================================
- Coverage    63.95%   63.64%   -0.31%     
===========================================
  Files          358      362       +4     
  Lines        43375    43954     +579     
===========================================
+ Hits         27737    27973     +236     
- Misses       15638    15981     +343

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

danperezasensio · 2023-12-27T20:40:55Z

Any update on this?

droidmonkey · 2023-12-27T20:47:21Z

@phoerious I need a review

uKL · 2024-01-28T15:46:48Z

Any updates on a review for this PR?

Kriechi · 2024-02-03T11:57:11Z

The fix for #9577 works great fields with the same name overwriting them on multiple occurrences, but I think it's incomplete, as the same problem also occurs for attached files:

in 1Password, have an entry with 3 attachements who's file names are identical: foo.txt with three different file sizes
import the OPVault into KeePass
observe that the newly imported entry only has one attachment foo.txt

Does this flow through the same code path or is this an independent bug?

droidmonkey · 2024-02-03T12:24:47Z

Different bug, easy fix, will get that in -- and all done.

src/format/BitwardenReader.cpp

src/format/OPUXReader.cpp

phoerious

Some minor things. Rest looks good.

src/format/BitwardenReader.cpp

src/format/OPUXReader.cpp

src/gui/csvImport/CsvImportWidget.cpp

src/format/BitwardenReader.cpp

* Previously our base style sheet added roughly 20px of margin to the top and bottom of all QGroupBox. This caused visual errors where that margin was not needed/desired. * Transferred padding to the specific layouts instead where it belongs.

* Include check for group as recycle bin directly into the Group::isRecycled() function * Return the original root group from Database::setRootGroup(...) to force memory management transfer

* Closes #7545 - Support 1Password 1PUX import format based on https://support.1password.com/1pux-format/ * Closes #8367 - Support Bitwarden JSON import format (both unencrypted and encrypted) based on https://bitwarden.com/help/encrypted-export/ * Fixes #9577 - OPVault import when fields have the same name or type * Introduce the import wizard to handle all import tasks (CSV, KDBX1, OPVault, 1PUX, JSON) * Clean up CSV parser code to make it much more efficient and easier to read * Combine all importer tests (except CSV) into one test file

Release 2.7.7 - Support USB Hotplug for Hardware Key interface [keepassxreboot#10092] - Support 1PUX and Bitwarden import [keepassxreboot#9815] - Browser: Add support for PassKeys [keepassxreboot#8825, keepassxreboot#9987, keepassxreboot#10318] - Build System: Move to vcpkg manifest mode [keepassxreboot#10088] - Fix multiple TOTP issues [keepassxreboot#9874] - Fix focus loss on save when the editor is not visible anymore [keepassxreboot#10075] - Fix visual when removing entry from history [keepassxreboot#9947] - Fix first entry is not selected when a search is performed [keepassxreboot#9868] - Prevent scrollbars on entry drag/drop [keepassxreboot#9747] - Prevent duplicate characters in "Also choose from" field of password generator [keepassxreboot#9803] - Security: Prevent byte-by-byte and attachment inference side channel attacks [keepassxreboot#10266] - Browser: Fix raising Update Entry messagebox [keepassxreboot#9853] - Browser: Fix bugs when returning credentials [keepassxreboot#9136] - Browser: Fix crash on database open from browser [keepassxreboot#9939] - Browser: Fix support for referenced URL fields [keepassxreboot#8788] - MacOS: Fix crash when changing highlight/accent color [keepassxreboot#10348] - MacOS: Fix TouchID appearing even though lid is closed [keepassxreboot#10092] - Windows: Fix terminating KeePassXC processes with MSI installer [keepassxreboot#9822] - FdoSecrets: Fix database merge crash when enabled [keepassxreboot#10136] # -----BEGIN PGP SIGNATURE----- # # iQEzBAABCAAdFiEENIkEDB8MPuq41ValRA/GXy4MbgEFAmXs7VsACgkQRA/GXy4M # bgHLpwf/brnyPPs3gJxZmD2pn8542D4CCsDh0fTceurOtqCe3J4Y+Fftc5euuoQu # 6rP4vJdd586l7JX5FnYIPXvGiU9op3MudJh+y+RN/PWwKcXNIXfUItMhpZEka49n # xnw+Wvbilg1QIHSSmZdIjBpohnEkA67qhWauc3bCacrRyEvIOzVMTxnqDTe4GUDy # CyauaRMMKezRTpLxSsk63TDAZZgDwK4ci5lC6ysHekc1Za6IbI3fMFjz1BGj+kPU # tMHMfDCWqK/5JZ27ZWcxy7m8tJY9m3rb+MoCyFRQz9ixaEe29yf5NqYdm9sn1Dlh # O7aFi7/EJtsBlXdguw5BcTPbsL7XEQ== # =Cots # -----END PGP SIGNATURE----- # gpg: directory '/home/runner/.gnupg' created # gpg: keybox '/home/runner/.gnupg/pubring.kbx' created # gpg: Signature made Sat Mar 9 23:14:35 2024 UTC # gpg: using RSA key 3489040C1F0C3EEAB8D556A5440FC65F2E0C6E01 # gpg: Can't check signature: No public key

dtgay · 2024-03-16T05:33:18Z

@droidmonkey Thanks for your work on this!

droidmonkey added user interface new feature refactor ux labels Sep 1, 2023

droidmonkey added this to the v2.8.0 milestone Sep 1, 2023

droidmonkey requested a review from phoerious September 1, 2023 02:44

droidmonkey changed the title ~~Fix spacing of QGroupBox's~~ Add 1Password 1PUX and Bitwarden JSON Importers Sep 1, 2023

github-advanced-security bot found potential problems Sep 1, 2023

View reviewed changes

src/gui/csvImport/CsvImportWidget.cpp Fixed Show fixed Hide fixed

droidmonkey force-pushed the feature/1pux-import branch from 60a7168 to d3d4ff5 Compare September 1, 2023 19:27

droidmonkey force-pushed the feature/1pux-import branch from d3d4ff5 to 4c2ddff Compare September 27, 2023 20:42

droidmonkey force-pushed the feature/1pux-import branch 4 times, most recently from 530cd56 to 268fd85 Compare October 1, 2023 11:28

droidmonkey force-pushed the feature/1pux-import branch 2 times, most recently from f112889 to 6e1432f Compare January 16, 2024 04:45

droidmonkey mentioned this pull request Jan 20, 2024

Import .1pif-Datei (1Password-Datenbank-Austauschdatei) #2513

Closed

droidmonkey force-pushed the feature/1pux-import branch from 6e1432f to f761d31 Compare February 3, 2024 12:44

droidmonkey force-pushed the feature/1pux-import branch 4 times, most recently from 22c8ea8 to bbdb7b2 Compare March 8, 2024 13:44

varjolintu reviewed Mar 8, 2024

View reviewed changes

src/format/BitwardenReader.cpp Outdated Show resolved Hide resolved

varjolintu reviewed Mar 8, 2024

View reviewed changes

src/format/OPUXReader.cpp Outdated Show resolved Hide resolved

phoerious approved these changes Mar 9, 2024

View reviewed changes

droidmonkey added 2 commits March 9, 2024 08:55

Fix spacing of QGroupBox's

9c17091

* Previously our base style sheet added roughly 20px of margin to the top and bottom of all QGroupBox. This caused visual errors where that margin was not needed/desired. * Transferred padding to the specific layouts instead where it belongs.

Minor changes to Group API to make it more explicit

95755ea

* Include check for group as recycle bin directly into the Group::isRecycled() function * Return the original root group from Database::setRootGroup(...) to force memory management transfer

droidmonkey force-pushed the feature/1pux-import branch from bbdb7b2 to f1d8f16 Compare March 9, 2024 14:51

droidmonkey force-pushed the feature/1pux-import branch from f1d8f16 to e96d34d Compare March 9, 2024 15:03

droidmonkey merged commit e700195 into develop Mar 9, 2024
10 of 11 checks passed

droidmonkey deleted the feature/1pux-import branch March 9, 2024 15:44

droidmonkey added the backported label Mar 9, 2024

droidmonkey mentioned this pull request Mar 9, 2024

Final backport PR for 2.7.7 #10364

Merged

scollovati mentioned this pull request Mar 18, 2024

Add Proton Pass importer #10465

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 1Password 1PUX and Bitwarden JSON Importers #9815

Add 1Password 1PUX and Bitwarden JSON Importers #9815

droidmonkey commented Sep 1, 2023 •

edited

Loading

michaelk83 commented Sep 1, 2023 •

edited

Loading

droidmonkey commented Sep 1, 2023 •

edited

Loading

michaelk83 commented Sep 1, 2023

michaelk83 commented Sep 1, 2023

droidmonkey commented Sep 1, 2023

droidmonkey commented Sep 27, 2023 •

edited

Loading

michaelk83 commented Sep 28, 2023 •

edited

Loading

codecov bot commented Oct 1, 2023 •

edited

Loading

danperezasensio commented Dec 27, 2023

droidmonkey commented Dec 27, 2023

uKL commented Jan 28, 2024

Kriechi commented Feb 3, 2024

droidmonkey commented Feb 3, 2024 •

edited

Loading

phoerious left a comment

dtgay commented Mar 16, 2024

Add 1Password 1PUX and Bitwarden JSON Importers #9815

Add 1Password 1PUX and Bitwarden JSON Importers #9815

Conversation

droidmonkey commented Sep 1, 2023 • edited Loading

Screenshots

Testing strategy

Type of change

michaelk83 commented Sep 1, 2023 • edited Loading

droidmonkey commented Sep 1, 2023 • edited Loading

michaelk83 commented Sep 1, 2023

michaelk83 commented Sep 1, 2023

droidmonkey commented Sep 1, 2023

droidmonkey commented Sep 27, 2023 • edited Loading

michaelk83 commented Sep 28, 2023 • edited Loading

codecov bot commented Oct 1, 2023 • edited Loading

Codecov Report

danperezasensio commented Dec 27, 2023

droidmonkey commented Dec 27, 2023

uKL commented Jan 28, 2024

Kriechi commented Feb 3, 2024

droidmonkey commented Feb 3, 2024 • edited Loading

phoerious left a comment

Choose a reason for hiding this comment

dtgay commented Mar 16, 2024

droidmonkey commented Sep 1, 2023 •

edited

Loading

michaelk83 commented Sep 1, 2023 •

edited

Loading

droidmonkey commented Sep 1, 2023 •

edited

Loading

droidmonkey commented Sep 27, 2023 •

edited

Loading

michaelk83 commented Sep 28, 2023 •

edited

Loading

codecov bot commented Oct 1, 2023 •

edited

Loading

droidmonkey commented Feb 3, 2024 •

edited

Loading