keepassxc-cli: Implement attachment handling

[andreblanke]

This pull request implements three new commands and strives to address #4462:

• attachment-export database entry name which exports the contents of an attachment inside a database entry to the command line
• attachment-import [-f] database entry name attachment which imports a file attachment into a database entry under a given name. An existing attachment with the same name may be overridden if the -f option is specified
• attachment-rm database entry name which removes the attachment with the provided name from the entry inside the database

Design decisions

The linked issue suggests modifications to the add and edit commands instead of introducing new commands like I have done but I felt having dedicated commands for it was a cleaner approach then adding additional functionality to add and edit, even though I am a bit unsure about it.
A getattachment or attachment-export command would be needed anyway and this way there exists symmetry between attachment-export and attachment-import.

For consistency with export, attachment-export outputs the contents of an attachment to the command line instead of taking an additional file argument.

I am unsure whether a command like attachment-mv to rename an attachment or to move it from one entry to another would be a worthwhile addition, as that use case can be covered using a combination of the commands implemented in this pull request.

Additionally, the name argument of attachment-import could be made optional, in which case it would default to the name of the attachment file. In that case switching the order of arguments to attachment-import [-f] database entry attachment [name] might also make sense.

Testing strategy

A unit test for each of the new commands has been added in TestCli.cpp.
Along with that I introduced tests/data/Attachment.txt used as a test attachment and added the Sample attachment.txt (with content "Sample content\n") to the /Sample Entry in NewDatabase.kdbx.

Type of change

• ✅ New feature (change that adds functionality)

[zarelit]

This PR is almost ready and it brings something towards the CLI as a full alternative to the GUI. I use attachments extensively and in the CLI it appears like they don't even exist.

Now the PR is stuck on something that can be debated and should be debated but it definitely looks broader and deeper than the scope of the PR itself.

By reading the comments it looks like in this particular instance both the solutions might be safe in the present and only become problematic as the development goes on and the caller changes or new code does not respect the same style.

Is there someone willing to unstuck this? Maybe @louib?

[louib]

@andreblanke Added a few comments, but overall I think this PR is really close to be ready to merge. The tests are looking good. Thx for the PR! 🙏

[droidmonkey]

I personally don't like that we are introducing new named commands. I would much rather handle this through entry editing with switches for attachment handling.

keepassxc-cli edit --attachment-add _attachmentfile_ _database_ _entry_

keepassxc-cli show --attachment _attachmentname_ _database_ _entry_

keepassxc-cli edit --attachment-rm _attachmentname_ _database_ _entry_

This is the defined workflow with our cli, by adding new commands you create new workflows and that is generally frowned upon.

[louib]

@droidmonkey the main problem that I see with jamming all the attachment commands in add and edit is that, when you add an attachment, you require 2 params: the name and the path. Would you use 2 new options just for that? Or encode both in the --attachment-add param? I feel like the attachments are specific enough to justify a new set of commands, and I'm not a fan of hiding sub-commands into other commands using parameters. I'd rather have a lot of small, focused commands than a few overloaded commands. Also, if I remember clearly, the add and edit commands are still missing some other fields, so they would become even more bloated.

Whichever decision we make, I think we should also update the Show command in this PR to display the attachment names of the entry, otherwise I believe there's no way to see the attachments of an entry from the CLI.

[droidmonkey]

Can we just add an "attachment" command and use add/edit/get subcommands?

[louib]

@droidmonkey that could be an option, although it would be inconsistent with what we have for the entry commands, the group commands and the db commands, which have dedicated commands for every action.

[droidmonkey]

Yah I agree, blah... this just sets up bad precedent moving forward for other features. We need to be careful of command creep... already have that problem!

[andreblanke]

I'm sorry for this pull request becoming stale, partially due to the discussion. In hindsight I should have just added the additional null safety. However, I mainly remember not being able to resolve the build failures and pausing development due to that.

@louib Thanks for the feedback! I will try to implement the changes you suggested in the next few days. I unfortunately don't have too much time on my hands at the moment but will try to get to it soon.

Concerning the command design: I tried to stay consistent but agree that an attachment command with the various subcommands would definitely be the better choice for the future, though inconsistent.

[louib]

@andreblanke let me know when you need another review!

[louib]

Almost there!

A few things:

• You can format the codebase using make format. This should fix the formatting CI job.
• We should add the attachments in the output of the show command, otherwise there's no way to see the attachments of an entry from the CLI.
• There's a conflict in the NewDatabase file, you'll probably need to rebase your branch against develop and fix the conflict. It'll probably be easier to start from the version on develop and re-apply your changes on it.

[droidmonkey]

Could also merge your version of new database into the develop version using keepassxc merge feature 😃

[louib]

@andreblanke the only thing remaining would be to add some tests to the show command test case for entries with and without attachments. You'll need to configure the project with WITH_GUI_TESTS to run the CLI unit tests locally. It's a bit counterintuitive, I will be fixing that soon.

[andreblanke]

I introduced the --show-attachments option to the show to avoid printing attachment information when only specific attributes are requested via the --attributes flag. If you'd like me to handle it any differently please let me know.

Other than that the tests should be good to go with the exception of TestGui::testAutoreloadDatabase and TestGui::testEditEntry which are most likely failing due to the changes done to NewDatabase.kdbx, though I'm not to sure how to fix those at the moment but I haven't looked into it too far either.

[droidmonkey]

This import should be in the cpp only

[droidmonkey]

These imports are not necessary, already in the header

[droidmonkey]

I would prefer some way to also optionally specify the output file name instead of always dumping to stdout.

[andreblanke]

I decided to entirely remove dumping to stdout when using attachment-export for the following reasons:

1. Symmetry with attachment-import which is only able to import from a file, not from stdin.
2. Binary output might mess up the terminal.
3. I found no good way to output the attachment which could both be checked by the tests using m_stdout->readAll() but also actually displayed the content to the user. All solutions I tried so far only allowed one of those things (at least without including some kind of workaround like m_fout that was used previously in AttachmentExport, but is now removed).

[droidmonkey]

I concur with this and it's consistent with how the gui handles attachments... as files.

[droidmonkey]

Thinking this through... there might be value in a --stdout flag on attachment-export for people who want to pipe the contents of an attachment into another command. When that flag is issued then the export_file positional parameter is ignored/optional. This is coming from #6947

[louib]

@andreblanke everything is looking good, except for the tests failing. Might need a rebase on develop as well.

[andreblanke]

Alright. I'm a bit busy at the moment but I'll try to get around to it on the weekend. While I'm at it I'll look into adding --stdout as well.

[droidmonkey]

I squashed all the commits, fixed the tests, and found a typo too! This is ready for merge after CI completes.

[louib]

@droidmonkey some tests are failing, and apparently it's related to entry history?

FAIL!  : TestGui::testAutoreloadDatabase() Compared values are not the same
   Actual   (entry->historyItems().size()): 10
   Expected (++editCount)                 : 11
   Loc: [/opt/buildagent/work/c401303cba1b4098/tests/gui/TestGui.cpp(490)]
QDEBUG : TestGui::testAutoreloadDatabase() Merge Sample Entry/Sample Entry_test with alien on top under NewDatabase
QWARN  : TestGui::testAutoreloadDatabase() Inconsistent history entry of Sample Entry[9f4544c2ab00c74a8a1a6eaf26cf57e9] at 2021-11-04 11-08-32-000 contains conflicting changes - conflict resolution may lose data!
PASS   : TestGui::testTabs()
FAIL!  : TestGui::testEditEntry() Compared values are not the same
   Actual   (entry->historyItems().size()): 10
   Expected (++editCount)                 : 11
   Loc: [/opt/buildagent/work/c401303cba1b4098/tests/gui/TestGui.cpp(490)]

[droidmonkey]

Yah I have no idea what is going on, could be because the entry was edited in NewDatabase.kdbx

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 94.83871% with 8 lines in your changes missing coverage. Please review.

Project coverage is 64.11%. Comparing base (7811f10) to head (e83efe9).
Report is 577 commits behind head on develop.

Files with missing lines	Patch %	Lines
src/cli/AttachmentExport.cpp	92.00%	4 Missing ⚠️
src/cli/AttachmentImport.cpp	95.83%	2 Missing ⚠️
src/cli/AttachmentRemove.cpp	94.44%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #5538      +/-   ##
===========================================
+ Coverage    63.87%   64.11%   +0.25%     
===========================================
  Files          330      333       +3     
  Lines        41838    41992     +154     
===========================================
+ Hits         26720    26923     +203     
+ Misses       15118    15069      -49     

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

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests
• JS Bundle Analysis - Avoid shipping oversized bundles

[andreblanke]

The GUI tests were exactly the ones I didn't know how to fix. Glad I wasn't the only one confused by them though. Anyway, thanks @droidmonkey for finishing off the PR. It was a pleasure working with you two and I appreciate the feedback that you gave me regarding my commits