cli/login: skip login when API key is available #6472
Conversation
cli/login/login.go
Outdated
| ctx := context.Background() | ||
| ctx = metadata.AppendToOutgoingContext(ctx, "x-buildbuddy-api-key", apiKey) | ||
|
|
||
| _, err = bbsClient.GetUser(ctx, &uspb.GetUserRequest{}) |
There was a problem hiding this comment.
What does GetUser return if using an org-level api key?
There was a problem hiding this comment.
I pushed a change a while ago.
GetUser RPC will return "user not found" error when the org API key is used, so I am catching that exact error here. WDYT?
There was a problem hiding this comment.
What does it return if you use an invalid API key?
Feels weird to be string matching an error message for a "success" case.
There was a problem hiding this comment.
Invalid API key will give rpc error: code = Unauthenticated desc = Invalid API key "***"
> BB_VERBOSE=1 bb login
[bb-debug] CLI started at 2024-05-13 18:11:08.734295 +0200 CEST m=+0.017330709
[bb-debug] args[0]: bb
[bb-debug] env: BAZEL_REAL=
[bb-debug] env: BAZELISK_SKIP_WRAPPER=
[bb-debug] env: USE_BAZEL_VERSION=
[bb-debug] env: BB_USE_BAZEL_VERSION=
[bb-debug] .bazelversion file contents: [7.1.1]
[bb-debug] setBazelVersion: Set env version to 7.1.1
Found an API key in your .git/config. Verifying it's validity.
[bb-debug] fail to get user: rpc error: code = Unauthenticated desc = Invalid API key "***"
Press Enter to open https://app.buildbuddy.io/settings/cli-login in your browser...
Also call GetUser when API key is found so that we could check if the key is still valid. Close #6468
It is case sensitive
sluongng
force-pushed
the
sluongng/cli-conditionally-login
branch
from
d50640f
to
7eb5af1
Compare
cli/login/login.go
Outdated
| if hasValidApiKey() { | ||
| log.Printf("Current API key is valid. Skipping login.") | ||
| return 0, nil | ||
| } |
There was a problem hiding this comment.
nit: should we hide this code block behind a flag like --check? (i.e. check credentials and exit if OK, otherwise prompt for login). otherwise, if the user wants to purposely run bb login to log in with a new API key, it would require manually deleting the existing API key in .git/config, currently. IIUC, Brentley wanted to call this with a pre-bazel script, so I assume he could pass the flag in there (?) CC @brentleyjones
There was a problem hiding this comment.
Yeah, we could use a flag. And we could also use a flag to have it check but not prompt, and if invalid it returns an error code instead. That way people can:
- Unconditionally set a new API (always prompt)
- Only prompt if invalid
- Just check it invalid, never prompt
There was a problem hiding this comment.
@brentleyjones I added a --check flag. Basic usage would look like this:
bb login --check || bb login
Slightly more advanced usage if you want control over the specific failure conditions:
if ! bb login --check ; then
exit_code=$?
if [[ "$exit_code" == 1 ]]; then
bb login # unauthenticated - login
else
exit "$exit_code" # other error - don't bother running `bb login`
fi
fi
There was a problem hiding this comment.
Actually, to make life easy, I added an --allow_existing option which basically does the same logic as that bash script above. So you can just call bb login --allow_existing in your scripts, and it will silently succeed if the user is logged in, otherwise if there was a network error it will just print the error, otherwise if the credentials are just invalid then it will prompt for login.
bduffany
force-pushed
the
sluongng/cli-conditionally-login
branch
2 times, most recently
from
517069b
to
27f74af
Compare
bduffany
force-pushed
the
sluongng/cli-conditionally-login
branch
from
27f74af
to
a69bbb9
Compare
Also call GetUser when API key is found so that we could check if the
key is still valid.
Close #6468