Refactoring database extraction#2698
Conversation
Previously, extracting the xml from a database was done with the `saveXml` attribute in the `KeePass2Reader` class. This had several unfortunate consequences: * The `KdbxReader` class had to import the `KdbxXmlWriter` class in order to perform the export (bad separation of concerns); * The CLI database unlocking logic had to be duplicated only for the `Extract` command; * The `xmlData` had to be stored in the `KeePass2Reader` as a temporary result. * Lots of `setSaveXml` functions were implemented only to trickle down this functionality. Also, The naming of the `saveXml` variable was not really helpful to understand it's role. Overall, this change will make it easier to maintain and expand the CLI database unlocking logic (for example, adding a `--no-password` option as requested in keepassxreboot#1873) It also opens to door to other types of extraction/exporting (for example exporting to CSV, as requested in keepassxreboot#2572) Note that I had to add a `m_kdbxVersion` attribute in the `KdbxWriter`, the same way there's one in the `KdbxReader` class.
phoerious
left a comment
There was a problem hiding this comment.
Overall, I like the changes. Moving XML extraction into the Database object seem sensible and makes my workaround for decrypting inner-stream secrets obsolete. I need you to change a few implementation details, though.
src/format/KdbxWriter.h
Outdated
| */ | ||
| virtual bool writeDatabase(QIODevice* device, Database* db) = 0; | ||
|
|
||
| virtual quint32 getFormatVersion() = 0; |
There was a problem hiding this comment.
Remove the get prefix and add a short descriptive doc string (pure-virtual methods should never be undocumented). For parity, you may want to add the same method to the KdbxReader class.
There was a problem hiding this comment.
@phoerious I thought about adding the same method to KdbxReader, but didn't do it since it doesn't have a constant version (dynamically set when reading the database). I could add it, I don't have a strong opinion for or against it.
There was a problem hiding this comment.
True. The Reader class determines the version from whatever it gets as input. It might make sense to add such a method anyway, but then we'd also need some sort of default value that's returned if nothing has been read so for. I guess we can leave it as is then.
|
@phoerious thanks for the review! |
- Fix database deletion when using unsafe saves to a different file system [#2889] - Fix opening databases with legacy key files that contain '/' [#2872] - Fix opening database files from the command line [#2919] - Fix crash when editing master key [#2836] - Fix multiple issues with apply button behavior [#2947] - Fix issues on application startup (tab order, --pw-stdin, etc.) [#2830] - Fix building without WITH_XC_KEESHARE - Fix reference entry coloring on macOS dark mode [#2984] - Hide window when performing entry auto-type on macOS [#2969] - Improve UX of update checker; reduce checks to every 7 days [#2968] - KeeShare improvements [#2946, #2978, #2824] - Re-enable Ctrl+C to copy password from search box [#2947] - Add KeePassXC-Browser integration for Brave browser [#2933] - SSH Agent: Re-Add keys on database unlock [#2982] - SSH Agent: Only remove keys on app exit if they are removed on lock [#2985] - CLI: Add --no-password option [#2708] - CLI: Improve database extraction to XML [#2698] - CLI: Don't call mandb on build [#2774] - CLI: Add debug info [#2714] - Improve support for Snap theming [#2832] - Add support for building on Haiku OS [#2859] - Ctrl+PgDn now goes to the next tab and Ctrl+PgUp to the previous - Fix compiling on GCC 5 / Xenial [#2990] - Add .gitrev output to tarball for third-party builds [#2970] - Add WITH_XC_UPDATECHECK compile flag to toggle the update checker [#2968]
Previously, extracting the xml from a database was done with the
saveXmlattribute in theKeePass2Readerclass.This had several unfortunate consequences:
KdbxReaderclass had to import theKdbxXmlWriterclassin order to perform the export (bad separation of concerns);
for the
Extractcommand;xmlDatahad to be stored in theKeePass2Readerasa temporary result.
setSaveXmlfunctions were implemented onlyto trickle down this functionality.
Also, The naming of the
saveXmlvariable was not reallyhelpful to understand it's role.
Overall, this change will make it easier to maintain and expand
the CLI database unlocking logic (for example, adding a
--no-passwordoption as requested in #1873)
It also opens to door to other types of extraction/exporting (for
example exporting to CSV, as requested in
#2572)
Note that I had to add a
m_kdbxVersionattribute in theKdbxWriter,the same way there's one in the
KdbxReaderclass.Type of change
Description and Context
I was working on adding yubikey support for the CLI, and saw the duplicated database unlocking
in
Extractcommand.Testing strategy
unit tests + locally running the
extractcommand.Checklist:
-DWITH_ASAN=ON. [REQUIRED]