cli/login: skip login when API key is available #6472
Conversation
| 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
| @@ -55,8 +55,8 @@ type newlineStdoutCloser struct { | |||
| } | |||
|
|
|||
| func (n *newlineStdoutCloser) Close() error { | |||
| if isatty.IsTerminal(n.File.Fd()) { | |||
| n.File.Write([]byte{'\n'}) | |||
| if isatty.IsTerminal(n.Fd()) { | |||
| @@ -51,7 +51,7 @@ func CacheDir() (string, error) { | |||
| return cacheDir, nil | |||
| } | |||
|
|
|||
| var repoRootPath = sync.OnceValues(func() (string, error) { | |||
| var RepoRootPath = sync.OnceValues(func() (string, error) { | |||
| @@ -37,20 +102,22 @@ func HandleLogin(args []string) (exitCode int, err error) { | |||
| if apiKey == "" { | |||
| return -1, fmt.Errorf("invalid input: API key is empty") | |||
| } | |||
| if !isValidApiKey(apiKey) { | |||
There was a problem hiding this comment.
isValidAPIKey should probably return (bool, error) so that we can distinguish between requests that succeed but return an auth error, and requests that fail due to network conditions
| ) | ||
|
|
||
| const ( | ||
| apiKeyRepoSetting = "api-key" | ||
| ApiKeyRepoSetting = "api-key" |
| loginURL = loginFlagSet.String("login_url", "https://app.buildbuddy.io/settings/cli-login", "URL for user to login") | ||
| ) | ||
|
|
||
| var getApiClient = sync.OnceValues(func() (bbspb.BuildBuddyServiceClient, error) { |
There was a problem hiding this comment.
this is only called once, so you could have isValidApiKey create it internally and make sure to defer Close() the connection
| 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
| log.Debugf("unable to read git repo config: %v", err) | ||
| return false | ||
| } | ||
| log.Printf("Found an API key in your .git/config. Verifying it's validity.") |
There was a problem hiding this comment.
should we remove this? IIUC this will be called on every bazel invocation (?) so I could see this getting noisy.
| func HandleLogin(args []string) (exitCode int, err error) { | ||
| if err := openInBrowser(loginURL); err != nil { | ||
| if hasValidApiKey() { | ||
| log.Printf("Current API key is valid. Skipping login.") |
There was a problem hiding this comment.
Should this be removed too? (To reduce noise, if the plan is to call this on every invocation)
Also call GetUser when API key is found so that we could check if the
key is still valid.
Close #6468