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.

Sign up for GitHub

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

Jump to bottom

Refactoring database extraction #2698

Merged
merged 3 commits into from
Feb 13, 2019
Merged

Refactoring database extraction #2698

merged 3 commits into from
Feb 13, 2019

Conversation

louib
Copy link
Member

@louib louib commented Feb 12, 2019

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 #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_kdbxVersion attribute in the KdbxWriter,
the same way there's one in the KdbxReader class.

Type of change

  • ✅ Refactor

Description and Context

I was working on adding yubikey support for the CLI, and saw the duplicated database unlocking
in Extract command.

Testing strategy

unit tests + locally running the extract command.

Checklist:

  • ✅ I have read the CONTRIBUTING document. [REQUIRED]
  • ✅ My code follows the code style of this project. [REQUIRED]
  • ✅ All new and existing tests passed. [REQUIRED]
  • ✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

Refactoring database extraction
8ca771a 
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.
@louib louib added feature: CLI pr: refactoring Pull request that refactors code labels Feb 12, 2019
@louib louib requested a review from a team February 12, 2019 22:22
@louib louib added this to the v2.4.1 milestone Feb 12, 2019
phoerious
phoerious requested changes Feb 12, 2019
View reviewed changes
Copy link
Member

@phoerious phoerious left a comment

Choose a reason for hiding this comment

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

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/cli/Extract.cpp Outdated Show resolved Hide resolved
src/core/Database.cpp Outdated Show resolved Hide resolved
src/format/Kdbx3Writer.h Outdated Show resolved Hide resolved
src/format/Kdbx4Writer.h Outdated Show resolved Hide resolved
src/format/KdbxWriter.cpp Outdated Show resolved Hide resolved
src/format/KdbxWriter.h Outdated Show resolved Hide resolved
phoerious
phoerious requested changes Feb 13, 2019
View reviewed changes
Copy link
Member

@phoerious phoerious left a comment

Choose a reason for hiding this comment

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

One more thing...

src/format/KdbxWriter.h Outdated
@@ -52,6 +52,10 @@ class KdbxWriter
*/
virtual bool writeDatabase(QIODevice* device, Database* db) = 0;

virtual quint32 getFormatVersion() = 0;
Copy link
Member

@phoerious phoerious Feb 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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 phoerious merged commit 504904a into keepassxreboot:develop Feb 13, 2019
@louib louib deleted the cli_extract_refactoring branch February 13, 2019 18:39
@louib
Copy link
Member Author

louib commented Feb 13, 2019

@phoerious thanks for the review!

droidmonkey added a commit that referenced this pull request Apr 12, 2019
@droidmonkey
Release 2.4.1
7bafe65 
- 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]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phoerious phoerious phoerious approved these changes

Assignees
No one assigned
Labels
feature: CLI pr: refactoring Pull request that refactors code
Projects
None yet
Milestone
v2.4.1
Development

Successfully merging this pull request may close these issues.
2 participants
@louib @phoerious