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

feature: Add get options to cli for interface with get_credential and support JSON output #678

Merged
merged 15 commits into from Apr 26, 2024

Conversation

BakerNet
Copy link
Contributor

@BakerNet BakerNet commented Apr 16, 2024

Internally, keyring supports get_credential, which is useful for backends which can supply their own username. This adds an exposed get_credential in the cli - keyring --mode creds get [service] [OPTIONAL - username]

This also adds support for JSON output type via --output json option

Help text for reference:

  --output {plain,json}
                        Output format for 'get' operation. Default is 'plain'
  --mode {password,creds}
                        Mode for 'get' operation. 'password' requires a username and will return only the password.
                        'creds' does not require a username and will return both the username and password separated by
                        a newline. Default is 'password'

Example output:

 ➜  keyring --mode creds get https://example.com/
hello
world
 ➜  keyring --mode creds --output json get https://example.com/
{"username": "hello", "password": "world"}
 ➜  keyring --output json get https://example.com/ hello
{"password": "world"}

@BakerNet BakerNet changed the title add getcreds to cli for interface to get_credentials feature: Add getcreds to cli for interface with get_credential Apr 16, 2024
@BakerNet BakerNet marked this pull request as ready for review April 16, 2024 17:41
keyring/cli.py Outdated
Comment on lines 101 to 102
print(f"username: {creds.username}")
print(f"password: {creds.password}")
Copy link

Choose a reason for hiding this comment

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

Should we just print them newline separated without the prefix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am in favor of without prefix - I only included prefix because the only other command which outputs more than one item (diagnose) uses prefixes.

Copy link
Owner

Choose a reason for hiding this comment

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

What if the username or password contains a newline? Probably unlikely for both, but it's also maybe allowed?

FWIW, the diagnose interface is meant for human consumption so can be sloppy about syntax. I'm kinda leaning the same way for the rest of the keyring CLI API - meant primarily for human consumption. Still, no reason it can't provide a reasonably stable interface that a wrapper could rely upon.

I'm okay without the prefix as long as the help guides the user as to what they're getting from the output.

Copy link

@zanieb zanieb Apr 16, 2024

Choose a reason for hiding this comment

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

Is it allowed for the username or password to include a newline? I guess the keyring is arbitrary enough that it certainly could, but then it could also include username: or password: .

The more complex option, I guess, is to output them base64 encoded.

Copy link
Owner

Choose a reason for hiding this comment

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

I looked at the tests and they do test for difficult characters on every keyring, so in theory they are supported. Still, I just wanted to flag it as a possibility.

The more complex option, I guess, is to output them base64 encoded.

Oof. That wouldn't be user friendly. I'm thinking JSON would be better for safety, but that's not very friendly either. Maybe there should be an option to emit in an unambiguous form like JSON. (--output json (default 'plain'))?

Copy link

Choose a reason for hiding this comment

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

An output format enum seems reasonable.

Are the main users of the CLI humans? I presumed this was mostly used for programmatic interaction.

Copy link
Contributor Author

@BakerNet BakerNet Apr 16, 2024

Choose a reason for hiding this comment

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

Added --output argument

Help text for reference:

  --output {plain,json}
                        Output format for 'get' operation. Default is 'plain'

Example output:

 ➜  keyring --get-mode creds --output json get https://example.com
{"username": "hello", "password": "world"}
 ➜  keyring --get-mode creds get https://example.com
hello
world

Copy link
Owner

Choose a reason for hiding this comment

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

Are the main users of the CLI humans? I presumed this was mostly used for programmatic interaction.

I'm unsure. In my personal usage, I always use the CLI as a human and always use the library for programming, but that's because I'm always coding in Python. It's my impression that my experience has been the most common experience for a long time but that may be changing, especially with the introduction of keyring to homebrew and the use of it by tools like uv.

Copy link
Owner

Choose a reason for hiding this comment

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

Added --output argument

Help text for reference:

  --output {plain,json}
                        Output format for 'get' operation. Default is 'plain'

Example output:

 ➜  keyring --get-mode creds --output json get https://example.com
{"username": "hello", "password": "world"}
 ➜  keyring --get-mode creds get https://example.com
hello
world

I'm liking this a lot! Thanks. Is it possible to use --mode (since get is implied by the command)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

keyring/cli.py Outdated
@@ -41,7 +42,7 @@ def __init__(self):
self.parser.add_argument(
"--disable", action="store_true", help="Disable keyring and exit"
)
self.parser._operations = ["get", "set", "del", "diagnose"]
self.parser._operations = ["get", "set", "del", "getcreds", "diagnose"]
Copy link
Owner

Choose a reason for hiding this comment

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

When naming things, I tend to separate words with something (space, dash, underscore). Here, I think dash may be appropriate.

Suggested change
self.parser._operations = ["get", "set", "del", "getcreds", "diagnose"]
self.parser._operations = ["get", "set", "del", "get-creds", "diagnose"]

But then I notice we have get and get-creds and the asymetry is bothering me, and it makes me wonder if it should be keyring get password + keyring get credential (or get pass and get creds) or maybe get with --creds or --password (default). All of those options have more onerous transitional concerns, so I'm not loving any option.

I don't feel strongly about it, so happy to move forward with something pragmatic.

Copy link

Choose a reason for hiding this comment

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

get-creds seems like simplest transition for the API, I feel your pain though. Perhaps it'd be worth opening a tracking issue to consider a different API in some future major version. I think get with some sort of --mode might make the most sense. You could also just assume a single value is the service name and that the username is null and return the full credentials in that case? eek.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think get with some sort of --mode might make the most sense.

This would be my personal vote over separate command. Updated PR to add --get-mode with default password

Help text for reference:

  --get-mode {password,creds}
                        Mode for 'get' operation. 'password' requires a username and will return only the password.
                        'creds' does not require a username and will return both the username and password separated by
                        a newline. Default is 'password'

Copy link

Choose a reason for hiding this comment

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

I don't really understand how we're going to use this downstream though. We'll need to try try --get-mode creds first and if it fails fall back to excluding the flag? Presumably there will be a different exit code? I guess we'll cache this so we don't need to do it on every request? but we have a lot of concurrent requests that make this difficult to do efficiently.

Copy link
Contributor Author

@BakerNet BakerNet Apr 16, 2024

Choose a reason for hiding this comment

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

I don't really understand how we're going to use this downstream though.

If no username on the index url, use --get-mode creds else use default. After that, the cacheing by netloc should just work

Copy link
Contributor Author

@BakerNet BakerNet Apr 16, 2024

Choose a reason for hiding this comment

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

Yeah but we won't know if the user's keyring version supports this without trial and error.

You can tell this is the problem on the first call if the status code is 2 instead of 1 and don't try again after first 2 status.

➜ keyring get --get-mode creds https://example.com/ oauth2accesstoken
usage: keyring [-h] [-p KEYRING_PATH] [-b KEYRING_BACKEND] [--list-backends]
               [--disable] [--print-completion {bash,zsh,tcsh}]
               [{get,set,del,diagnose}] [service] [username]
keyring: error: unrecognized arguments: --get-mode creds https://example.com/
X➜   echo $?                       +
2
➜  keyring get https://example.com/ oauth2accesstoken
X➜  echo $?                       +
1

The user manually requests keyring use, so uv would already be failing for them if no username provided. This is just a different failure mode.

if status code 2, warn user to either use username in index-url or update to keyring version X

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Err... I think I see an issue - for index-urls which don't need keyring (PyPi) - you'd hit the status code 2 when keyring requested but don't know whether you need to warn the user about it.

Copy link
Contributor Author

@BakerNet BakerNet Apr 17, 2024

Choose a reason for hiding this comment

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

I guess we'll cache this so we don't need to do it on every request? but we have a lot of concurrent requests that make this difficult to do efficiently.

On this point, you're using a mutex on the keyring cache, so the first time a host fails the check, you're returning None for all other checks on that host (keyring only running once per host). The only way you'll run into new efficiency issues is if there are a large number of unique index-url hosts involved.

This seems unlikely - but if you want to avoid it, you could check the exit code of the command and skip subsequent keyring calls if status code was 2.

Copy link

Choose a reason for hiding this comment

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

👍 Sorry I didn't mean to distract, unless usage in uv helps inform the design here I don't think we need to discuss it too much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zanieb We're kind of off-topic to discussion of this PR - can move to draft example PR on uv here:
astral-sh/uv#3081

@BakerNet BakerNet changed the title feature: Add getcreds to cli for interface with get_credential feature: Add get options to cli for interface with get_credential and support JSON output Apr 18, 2024
@BakerNet BakerNet requested a review from jaraco April 18, 2024 17:04
keyring/cli.py Outdated Show resolved Hide resolved
@jaraco jaraco merged commit 63dfe11 into jaraco:main Apr 26, 2024
11 of 12 checks passed
@jaraco
Copy link
Owner

jaraco commented Apr 26, 2024

Releasing as v25.2.0.

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

Successfully merging this pull request may close these issues.

None yet

3 participants