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.

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

Add keyfile option to keepassxc cli import cmd #5402

Conversation

Colfenor
Copy link
Contributor

@Colfenor Colfenor commented Sep 7, 2020

Fixes #5311

Basically added the add keyFile logic from the create
command to the import command and moved the loadFileKey() function
to the Utils class since it is now used in both create & import classes.

Testing strategy

export an existing Database with a keyfile to .xml

import it again using the keyfile option in the import command: (e.g.)
./keepassxc-cli import -k ~/test.key ~/test.xml ~/test.kdbx

at the current implementation state the filePath parameters have to be in that exact order.

Type of change

  • ✅ New feature (change that adds functionality)

@louib
Copy link
Member

louib commented Sep 12, 2020

Hi @Colfenor ! Thanks for the PR 🙏

There is a manpage for the CLI located in docs/man/ which would need to be updated with the new option.

Tests for the CLI are located in tests/TestCli.cpp and I think it's definitely worth it to add tests for the new option. For the CLI tests to run, you need to configure the project with -DWITH_GUI_TESTS=ON. You probably can use the test for the create command as a template.

Also, there seems to be some formatting issues with the PR. Formatting can be performed automatically by calling make format from the build/ directory. See https://github.com/keepassxreboot/keepassxc/blob/develop/.github/CONTRIBUTING.md#coding-styleguide for details.

@Colfenor
Copy link
Contributor Author

Hi @louib ! Thanks a lot for your info on the make format option and on the gui test option.
I added the man-page documentation, fixed format issues and added import cmd tests.

Copy link
Member

@louib louib left a comment

Choose a reason for hiding this comment

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

The password setting behavior is not the same for the db-create and import commands. By default, we don't ask for a password when creating the db, but we do when importing. I feel like the behavior should be the same, whichever we choose. @droidmonkey what do you think?

src/cli/Import.cpp Outdated Show resolved Hide resolved
@droidmonkey
Copy link
Member

The behavior should be the same because its the same action "create a db" just one is blank the other has data in it.

@Colfenor
Copy link
Contributor Author

Colfenor commented Sep 20, 2020

The behavior should be the same because its the same action "create a db" just one is blank the other has data in it.

okidok, I added the SetPasswordOption to the import command and updated man-page + tests.
Moreover the db-create and import cmd only accept -p -k in this exact order. I feel like this should be further clarified in the man pages, that exactly this order is accepted.

@louib
Copy link
Member

louib commented Sep 20, 2020

Moreover the db-create and import cmd only accept -p -k in this exact order.

Is it not because the -k option takes an argument and not the -p option? I feel like -k key_file_path.txt -p should work, no?

@Colfenor
Copy link
Contributor Author

Moreover the db-create and import cmd only accept -p -k in this exact order.

Is it not because the -k option takes an argument and not the -p option? I feel like -k key_file_path.txt -p should work, no?

Ah yes, I missed the argument. I just tested your statement, and can confirm that -k key_file_path.txt -p works :)

@droidmonkey droidmonkey requested a review from louib September 22, 2020 01:33
src/cli/Create.cpp Outdated Show resolved Hide resolved
@louib
Copy link
Member

louib commented Oct 5, 2020

@Colfenor looking good! Can you do one last rebase?

@Colfenor
Copy link
Contributor Author

Colfenor commented Oct 6, 2020

@Colfenor looking good! Can you do one last rebase?

yay ! I'm still a bit new to git rebasing, do you want me to rebase this branch on top of develop ?
like:
git checkout myFeatureBranch
git rebase develop

@droidmonkey
Copy link
Member

That'll do it, but make sure everything looks good prior to force pushing it back up

@Colfenor Colfenor force-pushed the feature/Add-keyfile-option-to-keepassxc-cli-import branch from 3cb935d to 407b97a Compare October 6, 2020 11:43
@Colfenor
Copy link
Contributor Author

Colfenor commented Oct 6, 2020

That'll do it, but make sure everything looks good prior to force pushing it back up

Ok thanks for clarification ! I hope it worked out ?

Copy link
Member

@louib louib left a comment

Choose a reason for hiding this comment

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

Awesome work @Colfenor , thx a lot for your contribution 🎉 🍾

src/cli/Utils.cpp Outdated Show resolved Hide resolved
@louib
Copy link
Member

louib commented Oct 7, 2020

@Colfenor Ubuntu tests are failing though. Are they passing locally?

@Colfenor
Copy link
Contributor Author

Colfenor commented Oct 7, 2020

@Colfenor Ubuntu tests are failing though. Are they passing locally?

@louib I checked the TeamCity build log (via guest login) and apparently the TestPassphraseGenerator.cpp -Test failed,
I saw this randomly fail on other commits I've made and assume this generates in rare cases combinations which are not matching QRegularExpression regex("^([A-Z][a-z]* ?)+$"); and therefore it fails ?

I haven't touched this test in this PR.

@droidmonkey droidmonkey merged commit fd3cc7e into keepassxreboot:develop Oct 10, 2020
@droidmonkey
Copy link
Member

droidmonkey commented Oct 10, 2020

DONE! Great work 🎉

@Colfenor
Copy link
Contributor Author

Thank you :) and thanks a lot for reviewing & support !

@phoerious phoerious added pr: new feature Pull request that adds a new feature and removed new feature labels Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: CLI pr: new feature Pull request that adds a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

keepassxc-cli import does not have keyfile option
4 participants