[CLI] Add a db-edit command

[louib]

This PR adds the db-edit command.

Changes in this PR include:

• Renamed the Create command to DatabaseCreate
• Renamed the Info command to DatabaseInfo
• Added a DatabaseEdit (db-edit) command with the following options:
  • Changing the password with -p
  • Removing the password with --unset-password
  • Changing the key file with --set-key-file
  • Removing the key file with --unset-key-file
• Deprecated the -k option for setting the key file. Otherwise it would clash with the -k option to specify the current key file when calling db-edit

The challenge response key options will be addressed in another PR. Those include:

• Changing the yubikey with --set-yubikey.
• Removing the yubikey with --unset-yubikey.

Other editing options will also be covered in another PR. For example, we could set the name and description of the db, but we don't even do that in db-create, so it could be added to both at the same time.

I also did not add the --decryption-time option that is available to db-create. It would take a bit of refactoring to be able to reuse the code, and I feel like this PR is already big enough, so I'll defer that to another PR.

Testing strategy

So far, unit tests and manual testing.

Type of change

• ✅ New feature (change that adds functionality)

[codecov-commenter]

Codecov Report

Base: 64.39% // Head: 64.43% // Increases project coverage by +0.05% 🎉

Coverage data is based on head (6cf9c7c) compared to base (b1e7c34).
Patch coverage: 86.90% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #8400      +/-   ##
===========================================
+ Coverage    64.39%   64.43%   +0.05%     
===========================================
  Files          339      340       +1     
  Lines        44007    44138     +131     
===========================================
+ Hits         28334    28439     +105     
- Misses       15673    15699      +26     

Impacted Files	Coverage Δ
src/cli/DatabaseInfo.cpp	97.37% <ø> (ø)
src/keys/CompositeKey.cpp	82.93% <80.00%> (-0.26%)	⬇️
src/cli/DatabaseEdit.cpp	85.59% <85.59%> (ø)
src/cli/DatabaseCreate.cpp	91.35% <93.75%> (ø)
src/cli/Command.cpp	87.91% <100.00%> (+0.13%)	⬆️
src/cli/Import.cpp	87.50% <100.00%> (+0.32%)	⬆️
src/gui/Clipboard.cpp	78.95% <0.00%> (-7.02%)	⬇️
src/gui/osutils/nixutils/NixUtils.cpp	41.44% <0.00%> (-2.70%)	⬇️
src/core/FileWatcher.cpp	86.75% <0.00%> (-1.20%)	⬇️
src/gui/DatabaseWidget.cpp	61.23% <0.00%> (+0.07%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[phoerious]

Should we add failsafes to prevent databases without any key?

Is it really possible to have multiple password keys or multiple file keys?

For our use cases, no, but in theory, yes. It's also important to keep the correct order of different key types, since they are hashed sequentially. I would prefer this to remain a list.

The second blocker is smaller: The -k option name is overloaded. Normally it is used to specify the key file when opening a database. But in the db-create command, it is used to set the keyfile for the new database. I think it would be better to use only the longer form (--set-key-file) for the db-create command, so that it can be reused by the db-edit command. That is a breaking change though, so we will have to document that in the release notes.

Emit a deprecation warning, then we can break things in 2.9.

[louib]

Should we add failsafes to prevent databases without any key?

Good call.

For our use cases, no, but in theory, yes.

Ouch. How would you change the password if there might be many? 🤔

It's also important to keep the correct order of different key types, since they are hashed sequentially. I would prefer this to remain a list.

This can be enforced from within the keys() function, which would still return a list. That would also make the order explicit, and encapsulate that logic in the CompositeKey class.

Emit a deprecation warning, then we can break things in 2.9.

👍

[phoerious]

I still don't think the overhead of a map compared to a small list is justified, especially not when you would have to return a list from keys() anyway. I think it makes things more complicated, not less. I'd rather implement a removeByType() convenience function.

[louib]

Should we add failsafes to prevent databases without any key?

This is done.

Emit a deprecation warning, then we can break things in 2.9.

This is done as well.

It's also important to keep the correct order of different key types, since they are hashed sequentially.

I'm still not sure I understand how this is currently implemented. Right now I'm pretty sure the getNewDatabaseKey function can change the order of the different key types. The tests are passing though, so I'm not sure this is a concern in this case.

This is still a work in progress, but feedback would be much appreciated.

[louib]

I think this PR is close to being mergeable, so I'm taking it out of the draft state. I only added an hasKey function to CompositeKey, which makes the code a bit cleaner in the new command. I will try to think about other unit tests that could be added.

There's still a chance that the order of the different key types changes during an update. For example, if there is only a key file and a password is added, the password will be added after the key file. Not sure if that's a problem though.

I'm thinking about renaming the Database related commands to DatabaseCreate and DatabaseInfo, for consistency with the new DatabaseEdit command. Let me know what you guys think of that.

[louib]

There's still a chance that the order of the different key types changes during an update. For example, if there is only a key file and a password is added, the password will be added after the key file. Not sure if that's a problem though.

Turns out that this was indeed a problem! Adding more tests confirmed that the key order is important, as @phoerious mentioned in a previous comment. The getNewDatabaseKey function now handles the key types in the expected order: password, then key file, then challenge response. I also added a sanity check to make sure that those are the only key types in the CompositeKey, in case we add more types in the future. I think some of that code should be encapsulated in CompositeKey at some point, but that could be deferred to another PR.

The last thing I would test before merging is that the challenge-response key will be preserved when editing the database with db-edit, although the code is pretty straightforward so I don't expect any surprises.

This PR is ready for review!

[louib]

@droidmonkey I found a bug with the handling of challenge-response keys, which I fixed in the last commit. I tested locally and everything is working, the only concern I have left is that I get prompted 3 times to touch my yubikey. Once for unlocking, and twice for saving. Not sure why 2 times is necessary when editing the database 🤔

[louib]

@droidmonkey please have a look at the last commit. It fixes the double yubikey interaction when saving, although I'm not 100% that it's ok to disable the key transformation in that case. I see that we call setKey when writing, with the following parameters:

db->setKey(db->key(), false, true)

So I guess the key transformation is going to happen anyway.

[droidmonkey]

Did my own yubikey test and found that you could not --unset-password and have just a Yubikey on the database. Fixed that problem, good to go now!

[louib]

@droidmonkey good catch!