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 open command, interactive CLI session support #3426

Merged
merged 1 commit into from
Sep 28, 2019

Conversation

sjamesr
Copy link
Contributor

@sjamesr sjamesr commented Aug 5, 2019

Type of change

  • ✅ New feature (non-breaking change which adds functionality)

Description and Context

This change adds an interactive mode to keepassxc-cli. For example:

$ ./src/cli/keepassxc-cli open ~/Downloads/passwords.kdbx
Insert password to unlock /home/whoever/Downloads/passwords.kdbx: <password>
Downloads/passwords.kdbx> ls /
General/
Windows/
Network/
Internet/
eMail/
Homebanking/
Recycle Bin/
Banking/
Downloads/passwords.kdbx> ls /Internet
Facebook
Twitter
Downloads/passwords.kdbx> show /Internet/Facebook
Title: Facebook
...

This change adds the following command: open. If open is invoked, the keepassxc-cli program attempts to open the database, then drops into interactive mode.

Interactive mode uses readline (falls back to just reading stdin if readline is not available). Each user command causes processing much like keepassxc-cli does today, except the command being invoked already has a database present, meaning the command should not attempt to open any database. I've changed the List and Show commands as examples.

Fixes #3224.

Screenshots

Testing strategy

Checklist:

  • ✅ I have read the CONTRIBUTING document. [REQUIRED]
  • ✅ All new and existing tests passed. [REQUIRED]

@sjamesr sjamesr force-pushed the add_open_command branch 11 times, most recently from dd55a71 to f1bf156 Compare August 6, 2019 19:52
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.

Thanks for the PR @sjamesr , I'm glad to see such a major contribution to the CLI!

Out of curiosity, have you thought about a session based approach, were the process with the unlocked (or partially unlocked) database would be spawned first, and then the next commands would be piped/sent to that running process?

I think we would need tests specifically for the 2 new commands, including tests to commands with a current database.

Also, don't forget to update the man page cli/keepassxc-cli.1 and the CHANGELOG!

src/cli/CommandParser.cpp Outdated Show resolved Hide resolved
src/cli/keepassxc-cli.cpp Outdated Show resolved Hide resolved
@sjamesr sjamesr force-pushed the add_open_command branch 7 times, most recently from 8df4f2b to 92e67f2 Compare August 12, 2019 02:07
CHANGELOG Outdated Show resolved Hide resolved
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.

Thank you, this looks like a great new feature. Here's my review.

cmake/FindReadline.cmake Outdated Show resolved Hide resolved
src/cli/Command.cpp Outdated Show resolved Hide resolved
src/cli/Command.cpp Outdated Show resolved Hide resolved
src/cli/Command.cpp Outdated Show resolved Hide resolved
src/cli/Command.cpp Outdated Show resolved Hide resolved
src/cli/keepassxc-cli.cpp Outdated Show resolved Hide resolved
src/cli/keepassxc-cli.cpp Outdated Show resolved Hide resolved
src/cli/keepassxc-cli.cpp Outdated Show resolved Hide resolved
tests/TestCli.cpp Outdated Show resolved Hide resolved
tests/TestCli.cpp Outdated Show resolved Hide resolved
@sjamesr sjamesr force-pushed the add_open_command branch 4 times, most recently from ec27e48 to b74926c Compare August 20, 2019 14:46
Copy link
Member

@droidmonkey droidmonkey left a comment

Choose a reason for hiding this comment

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

This is great, but a couple of minor changes (while in the "open" terminal prompt only) will make it even better:

  • Disable the extract command, it just dumps XML to the view and doesn't let you write to a file
  • Add exit and quit keywords that closes the session and returns to bash
  • Add close keyword that closes the current database and returns to the interactive prompt (currently bareword open does this as well)
  • Instead of list the relative path to the database file, just show the database file name (if present) or filename if not.
  • Check if the database file exists (using Canonical File Path) prior to requesting unlock password

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

droidmonkey commented Sep 1, 2019

NOTE: we will have to add libreadline.dylib to the MacOS App Bundle fixups. I don't have the full pathname handy right now.

Copy link
Contributor Author

@sjamesr sjamesr left a comment

Choose a reason for hiding this comment

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

I added the improvements you requested, with the exception of disabling the extract command. Currently there's no way to have a command enabled only in non-interactive mode. Can we add this in a later change?

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

droidmonkey commented Sep 5, 2019

I wasn't intending for you to make "close", "quit" and "exit" actual commands. Just keywords that would be intercepted in the while loop before the Command::getCommand() call. You can do the same thing with "extract" by intercepting that and telling the user that it is disabled in interactive mode.

@sjamesr
Copy link
Contributor Author

sjamesr commented Sep 5, 2019 via email

@droidmonkey
Copy link
Member

droidmonkey commented Sep 5, 2019

Oh good point, let me think about it a little

@sjamesr sjamesr force-pushed the add_open_command branch 2 times, most recently from 4bd6336 to 8e43f58 Compare September 9, 2019 22:10
@droidmonkey
Copy link
Member

I don't like that we populate the command list with interactive mode only commands. In the populateCommands() function, can we only add close, quit, and exit if we are in interactive mode? This would also let you remove extract while in interactive mode.

@sjamesr
Copy link
Contributor Author

sjamesr commented Sep 10, 2019

@droidmonkey I added a separate "Commands" class that has the set of commands that we're using, please take a look. help now only displays commands relevant to the current mode (batch or interactive). Using a command from the other mode will result in an "unknown command" error.

@droidmonkey
Copy link
Member

Very nice fix, this is now ready for merge

@droidmonkey
Copy link
Member

I updated how we handle commands, batch vs interactive, to comply with our coding standards. Need to add the readline dylib path conversion and this will be ready.

This change adds a GNU Readline-based interactive mode to keepassxc-cli. If GNU Readline is not available, commands are just read from stdin with no editing or auto-complete support.

DatabaseCommand is modified to add the path to the current database to the arguments passed to executeWithDatabase. In this way, instances of DatabaseCommand do not have to prompt to re-open the database after each invocation, and existing command implementations do not have to be changed to support interactive mode.

This change also introduces a new way of handling commands between interactive and batch modes.

* Fixes keepassxreboot#3224.
* Ran make format
@droidmonkey
Copy link
Member

I've confirmed that the macOS build uses built-in libraries for the readline so no need to worry about the name tool fixes.

@droidmonkey droidmonkey merged commit b1eda37 into keepassxreboot:develop Sep 28, 2019
@droidmonkey droidmonkey added this to the v2.5.0 milestone Oct 16, 2019
yan12125 pushed a commit to macports/macports-ports that referenced this pull request Oct 20, 2019
Other changes:

* Use cmake 1.1 port group
* Use new cxx_standard
* A new feature [1] requires readline

[1] keepassxreboot/keepassxc#3426
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the keepassxc-cli session based
4 participants