Skip keyring for auth check #7169
Conversation
cliAutomation
added
the
external
pkg/cmdutil/auth_check.go
Outdated
| @@ -18,7 +18,7 @@ func CheckAuth(cfg config.Config) bool { | |||
| // authentication tokens set for enterprise hosts. | |||
| // Any non-github.com hostname is fine here | |||
| dummyHostname := "example.com" | |||
| token, _ := cfg.Authentication().Token(dummyHostname) | |||
| token, _ := cfg.Authentication().TokenFromEnvOrConfig(dummyHostname) | |||
There was a problem hiding this comment.
Since we don't need an actual token, could we come up with an approach to just test whether an environment variable was set? We used to go the token route in the past because it was cheap, but now with keyring it's not so cheap anymore, so instead of coming up with workarounds I would prefer to just see an approach that specifically asks for the information we're interested in (and nothing more).
I'm imagining a method called something like IsEnterpriseTokenEnv() bool
There was a problem hiding this comment.
Both call points are only interested in whether GITHUB_ENTERPRISE_TOKEN or GH_ENTERPRISE_TOKEN was set, but not interested in the value itself. I'd vote for a new method that is precisely tuned to what is being asked for.
There was a problem hiding this comment.
maybe something to export from https://github.com/cli/go-gh?
There was a problem hiding this comment.
I am okay with a method HasEnterpriseEnvToken() bool. Sorry for the back and forth on this. I don't think we need to expose any additional functionality in go-gh to achieve this. The ghAuth.TokenFromEnvOrConfig function returns the source of the token as its second return value. We just need to verify that the source is either GITHUB_ENTERPRISE_TOKEN or GH_ENTERPRISE_TOKEN.
There was a problem hiding this comment.
@mislav I didn't notice at first, but this is not actually specific to Enterprise. If there is an Enterprise token, it will be returned, true. But if there is no enterprise token, it will fallback to GH_TOKEN, GITHUB_TOKEN.
My proposal is to have the original behaviour (TokenFromEnvOrConfig() != ""), wrapped in a function HasEnvOrConfigToken to encapsulate this logic
internal/config/config.go
Outdated
| return ghAuth.TokenFromEnvOrConfig(hostname) | ||
| // HasEnterpriseEnvToken checks whether the current env or config contains an enterprise token | ||
| func (c *AuthConfig) HasEnterpriseEnvToken() bool { | ||
| token, source := ghAuth.TokenFromEnvOrConfig("") |
There was a problem hiding this comment.
This works right now, but only because of an accident. We just so happen to interpret an empty string passed to TokenFromEnvOrConfig as an "enterprise" hostname because it technically doesn't match github.com. We should add a test to go-gh to make note that we shouldn't accidentally break this behavior. But this shouldn't prevent this PR from shipping 👍
There was a problem hiding this comment.
would you prefer I use example.com as before?
Co-authored-by: Sam Coe <samcoe@users.noreply.github.com>
benjlevesque
force-pushed
the
fix/skip-keyring-auth-check
branch
from
bea6e78
to
f91f1db
Compare
benjlevesque
changed the title
Fixes #7145