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

[CLI] create does not honor --no-password #4307

Closed
Crote opened this issue Feb 10, 2020 · 8 comments · Fixed by #4313
Closed

[CLI] create does not honor --no-password #4307

Crote opened this issue Feb 10, 2020 · 8 comments · Fixed by #4313
Assignees

Comments

@Crote
Copy link

Crote commented Feb 10, 2020

Expected Behavior

When I run keepassxc-cli create --no-password --key-file somekey newdb.kdbx, the CLI should create a database without a password. This behaviour would be similar to that of all other cli commands and is heavily implied by the manpage

Current Behavior

The command returns Unknown option 'no-password'

Possible Solution

A current workaround is to use echo '' | keepassxc-cli create --key-file somekey newdb.kdbx instead, but this is very cumbersome.

It seems to be fixable by wrapping the https://github.com/keepassxreboot/keepassxc/blob/develop/src/cli/Create.cpp#L100 part in a if (!parser->isSet(Command::NoPasswordOption)) block and adding that flag to the options parser.

Context

Current behaviour is inconsistent with other commands and contradicts the manpage.

@louib
Copy link
Member

louib commented Feb 10, 2020

I think the no-password option is used when unlocking the database, whereas here it would be used to set (or not) the password. What about an option --no-set-password? When we'll have a db-edit CLI command, we'll then be able to add a --set-password option to change the password, while still using the --no-password to unlock the database to edit.

@Crote
Copy link
Author

Crote commented Feb 10, 2020

The --key-file option is also used both for setting and unlocking, so there is already a precedence.

The phrasing --no-set-password is a bit weird for create: what else would you do with a password besides setting it?

I don't really have any opinion regarding a future db-edit command, as long as the flags are somewhat consistent and well-documented.

@JulianVolodia
Copy link
Contributor

--no-password is put in General options in the man pages, so should work imho.

@JulianVolodia
Copy link
Contributor

JulianVolodia commented Feb 10, 2020

Adding Command::NoPasswordOption in src/cli/Create.cpp to cover execution of:

100     auto password = Utils::getPasswordFromStdin();

in the if clause?

Edit: If commit 22aa304 does what you want refactor commitmsg and apply. Need tests. Cheers!

JulianVolodia added a commit to JulianVolodia/keepassxc that referenced this issue Feb 10, 2020
@louib
Copy link
Member

louib commented Feb 11, 2020

@Crote thanks for pointing out the --key-file, I also think it's a mistake. I think we should be using different option names for the creation options and the opening options.

@JulianVolodia
Copy link
Contributor

JulianVolodia commented Feb 11, 2020

@louib why not to used meta options like -k -no-password found in general options, instead of having flustrating differences and long manpage?

@louib
Copy link
Member

louib commented Feb 11, 2020

@JulianVolodia we would still have to document in the manpage what the options are used for in different contexts (creation and opening), so we'll have to add stuff to the manpage anyway. I'd rather have argument with names that are explicit about what they do, than re-use arguments from other commands to implement a completely different functionality.

@JulianVolodia
Copy link
Contributor

Documented manpage is excuse. But, you are in Team - you decide.

In free time check out some cli tools out there and how you like using one which have parameters context dependant. As we all type keepass-cli create and then --no-password that is self-explaining that we want to create with setting no password (and begging keepass-cli to not ask for that to avoid the hacks like piping "\n" by echo '' | prefix.

I accept your decision - it is your issue, but I want that you see other point and still say that yours is better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants