keepassxreboot
Add 1Password 1PUX and Bitwarden JSON Importers
Add 1Password 1PUX and Bitwarden JSON Importers:
- 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
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)
- 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...andDatabase > 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.
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.
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.
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.
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.
@michaelk83 do you like this better?
Also the currently visible database (if unlocked) is default selected for the existing database.
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).
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
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
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.txtwith 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?
