STACKITGit/cli
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

More descriptive error when aborting auth due to environment variables

Browse source 
Old message:

    read-only token in GH_TOKEN cannot be modified

This message was vague and some users did not understand that this
refers to the value that is read from environment variables.

New message:

    $ GH_TOKEN=123 ghd auth login -h github.com
    The value of the GH_TOKEN environment variable is being used for authentication.
    To have GitHub CLI store credentials instead, first clear the value from the environment.
This commit is contained in:
Mislav Marohnić 2021-01-08 18:13:19 +01:00
parent b5366c6ebf
commit c9407b2629
6 changed files with 152 additions and 84 deletions
Download patch file Download diff file Expand all files Collapse all files

10
internal/config/from_env.go
View file

@ -14,6 +14,14 @@ const (
GITHUB_ENTERPRISE_TOKEN = "GITHUB_ENTERPRISE_TOKEN"
)
type ReadOnlyEnvError struct {
Variable string
}
func (e *ReadOnlyEnvError) Error() string {
return fmt.Sprintf("read-only value in %s", e.Variable)
}
func InheritEnv(c Config) Config {
return &envConfig{Config: c}
}
@ -56,7 +64,7 @@ func (c *envConfig) GetWithSource(hostname, key string) (string, string, error)
func (c *envConfig) CheckWriteable(hostname, key string) error {
if hostname != "" && key == "oauth_token" {
if token, env := AuthTokenFromEnv(hostname); token != "" {
return fmt.Errorf("read-only token in %s cannot be modified", env)
return &ReadOnlyEnvError{Variable: env}
}
}

125
pkg/cmd/auth/login/login.go
View file

@ -120,70 +120,56 @@ func loginRun(opts *LoginOptions) error {
return err
}
hostname := opts.Hostname
if hostname == "" {
if opts.Interactive {
var err error
hostname, err = promptForHostname()
if err != nil {
return err
}
} else {
return errors.New("must specify --hostname")
}
}
if err := cfg.CheckWriteable(hostname, "oauth_token"); err != nil {
var roErr *config.ReadOnlyEnvError
if errors.As(err, &roErr) {
fmt.Fprintf(opts.IO.ErrOut, "The value of the %s environment variable is being used for authentication.\n", roErr.Variable)
fmt.Fprint(opts.IO.ErrOut, "To have GitHub CLI store credentials instead, first clear the value from the environment.\n")
return cmdutil.SilentError
}
return err
}
if opts.Token != "" {
// I chose to not error on existing host here; my thinking is that for --with-token the user
// probably doesn't care if a token is overwritten since they have a token in hand they
// explicitly want to use.
if opts.Hostname == "" {
return errors.New("empty hostname would leak oauth_token")
}
err := cfg.Set(opts.Hostname, "oauth_token", opts.Token)
err := cfg.Set(hostname, "oauth_token", opts.Token)
if err != nil {
return err
}
err = shared.ValidateHostCfg(opts.Hostname, cfg)
apiClient, err := shared.ClientFromCfg(hostname, cfg)
if err != nil {
return err
}
if err := apiClient.HasMinimumScopes(hostname); err != nil {
return fmt.Errorf("error validating token: %w", err)
}
return cfg.Write()
}
// TODO consider explicitly telling survey what io to use since it's implicit right now
hostname := opts.Hostname
if hostname == "" {
var hostType int
err := prompt.SurveyAskOne(&survey.Select{
Message: "What account do you want to log into?",
Options: []string{
"GitHub.com",
"GitHub Enterprise Server",
},
}, &hostType)
if err != nil {
return fmt.Errorf("could not prompt: %w", err)
}
isEnterprise := hostType == 1
hostname = ghinstance.Default()
if isEnterprise {
err := prompt.SurveyAskOne(&survey.Input{
Message: "GHE hostname:",
}, &hostname, survey.WithValidator(ghinstance.HostnameValidator))
if err != nil {
return fmt.Errorf("could not prompt: %w", err)
}
}
}
fmt.Fprintf(opts.IO.ErrOut, "- Logging into %s\n", hostname)
existingToken, _ := cfg.Get(hostname, "oauth_token")
if existingToken != "" && opts.Interactive {
err := shared.ValidateHostCfg(hostname, cfg)
if err == nil {
apiClient, err := shared.ClientFromCfg(hostname, cfg)
if err != nil {
return err
}
apiClient, err := shared.ClientFromCfg(hostname, cfg)
if err != nil {
return err
}
if err := apiClient.HasMinimumScopes(hostname); err == nil {
username, err := api.CurrentLoginName(apiClient, hostname)
if err != nil {
return fmt.Errorf("error using api: %w", err)
@ -206,10 +192,6 @@ func loginRun(opts *LoginOptions) error {
}
}
if err := cfg.CheckWriteable(hostname, "oauth_token"); err != nil {
return err
}
var authMode int
if opts.Web {
authMode = 0
@ -244,19 +226,19 @@ func loginRun(opts *LoginOptions) error {
return fmt.Errorf("could not prompt: %w", err)
}
if hostname == "" {
return errors.New("empty hostname would leak oauth_token")
}
err = cfg.Set(hostname, "oauth_token", token)
if err != nil {
return err
}
err = shared.ValidateHostCfg(hostname, cfg)
apiClient, err := shared.ClientFromCfg(hostname, cfg)
if err != nil {
return err
}
if err := apiClient.HasMinimumScopes(hostname); err != nil {
return fmt.Errorf("error validating token: %w", err)
}
}
cs := opts.IO.ColorScheme()
@ -322,6 +304,35 @@ func loginRun(opts *LoginOptions) error {
return nil
}
func promptForHostname() (string, error) {
var hostType int
err := prompt.SurveyAskOne(&survey.Select{
Message: "What account do you want to log into?",
Options: []string{
"GitHub.com",
"GitHub Enterprise Server",
},
}, &hostType)
if err != nil {
return "", fmt.Errorf("could not prompt: %w", err)
}
isEnterprise := hostType == 1
hostname := ghinstance.Default()
if isEnterprise {
err := prompt.SurveyAskOne(&survey.Input{
Message: "GHE hostname:",
}, &hostname, survey.WithValidator(ghinstance.HostnameValidator))
if err != nil {
return "", fmt.Errorf("could not prompt: %w", err)
}
}
return hostname, nil
}
func getAccessTokenTip(hostname string) string {
ghHostname := hostname
if ghHostname == "" {

75
pkg/cmd/auth/login/login_test.go
View file

@ -3,9 +3,11 @@ package login
import (
"bytes"
"net/http"
"os"
"regexp"
"testing"
"github.com/MakeNowJust/heredoc"
"github.com/cli/cli/api"
"github.com/cli/cli/internal/config"
"github.com/cli/cli/pkg/cmd/auth/shared"
@ -189,11 +191,13 @@ func Test_NewCmdLogin(t *testing.T) {
func Test_loginRun_nontty(t *testing.T) {
tests := []struct {
name string
opts *LoginOptions
httpStubs func(*httpmock.Registry)
wantHosts string
wantErr string
name string
opts *LoginOptions
httpStubs func(*httpmock.Registry)
env map[string]string
wantHosts string
wantErr string
wantStderr string
}{
{
name: "with token",
@ -201,6 +205,9 @@ func Test_loginRun_nontty(t *testing.T) {
Hostname: "github.com",
Token: "abc123",
},
httpStubs: func(reg *httpmock.Registry) {
reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("repo,read:org"))
},
wantHosts: "github.com:\n oauth_token: abc123\n",
},
{
@ -223,7 +230,7 @@ func Test_loginRun_nontty(t *testing.T) {
httpStubs: func(reg *httpmock.Registry) {
reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("read:org"))
},
wantErr: `could not validate token: missing required scope 'repo'`,
wantErr: `error validating token: missing required scope 'repo'`,
},
{
name: "missing read scope",
@ -234,7 +241,7 @@ func Test_loginRun_nontty(t *testing.T) {
httpStubs: func(reg *httpmock.Registry) {
reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("repo"))
},
wantErr: `could not validate token: missing required scope 'read:org'`,
wantErr: `error validating token: missing required scope 'read:org'`,
},
{
name: "has admin scope",
@ -247,6 +254,36 @@ func Test_loginRun_nontty(t *testing.T) {
},
wantHosts: "github.com:\n oauth_token: abc456\n",
},
{
name: "github.com token from environment",
opts: &LoginOptions{
Hostname: "github.com",
Token: "abc456",
},
env: map[string]string{
"GH_TOKEN": "value_from_env",
},
wantErr: "SilentError",
wantStderr: heredoc.Doc(`
The value of the GH_TOKEN environment variable is being used for authentication.
To have GitHub CLI store credentials instead, first clear the value from the environment.
`),
},
{
name: "GHE token from environment",
opts: &LoginOptions{
Hostname: "ghe.io",
Token: "abc456",
},
env: map[string]string{
"GH_ENTERPRISE_TOKEN": "value_from_env",
},
wantErr: "SilentError",
wantStderr: heredoc.Doc(`
The value of the GH_ENTERPRISE_TOKEN environment variable is being used for authentication.
To have GitHub CLI store credentials instead, first clear the value from the environment.
`),
},
}
for _, tt := range tests {
@ -256,7 +293,8 @@ func Test_loginRun_nontty(t *testing.T) {
io.SetStdoutTTY(false)
tt.opts.Config = func() (config.Config, error) {
return config.NewBlankConfig(), nil
cfg := config.NewBlankConfig()
return config.InheritEnv(cfg), nil
}
tt.opts.IO = io
@ -271,10 +309,23 @@ func Test_loginRun_nontty(t *testing.T) {
return api.NewClientFromHTTP(httpClient), nil
}
old_GH_TOKEN := os.Getenv("GH_TOKEN")
os.Setenv("GH_TOKEN", tt.env["GH_TOKEN"])
old_GITHUB_TOKEN := os.Getenv("GITHUB_TOKEN")
os.Setenv("GITHUB_TOKEN", tt.env["GITHUB_TOKEN"])
old_GH_ENTERPRISE_TOKEN := os.Getenv("GH_ENTERPRISE_TOKEN")
os.Setenv("GH_ENTERPRISE_TOKEN", tt.env["GH_ENTERPRISE_TOKEN"])
old_GITHUB_ENTERPRISE_TOKEN := os.Getenv("GITHUB_ENTERPRISE_TOKEN")
os.Setenv("GITHUB_ENTERPRISE_TOKEN", tt.env["GITHUB_ENTERPRISE_TOKEN"])
defer func() {
os.Setenv("GH_TOKEN", old_GH_TOKEN)
os.Setenv("GITHUB_TOKEN", old_GITHUB_TOKEN)
os.Setenv("GH_ENTERPRISE_TOKEN", old_GH_ENTERPRISE_TOKEN)
os.Setenv("GITHUB_ENTERPRISE_TOKEN", old_GITHUB_ENTERPRISE_TOKEN)
}()
if tt.httpStubs != nil {
tt.httpStubs(reg)
} else {
reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("repo,read:org"))
}
mainBuf := bytes.Buffer{}
@ -289,7 +340,7 @@ func Test_loginRun_nontty(t *testing.T) {
}
assert.Equal(t, "", stdout.String())
assert.Equal(t, "", stderr.String())
assert.Equal(t, tt.wantStderr, stderr.String())
assert.Equal(t, tt.wantHosts, hostsBuf.String())
reg.Verify(t)
})
@ -325,7 +376,7 @@ func Test_loginRun_Survey(t *testing.T) {
as.StubOne(false) // do not continue
},
wantHosts: "", // nothing should have been written to hosts
wantErrOut: regexp.MustCompile("Logging into github.com"),
wantErrOut: nil,
},
{
name: "hostname set",

6
pkg/cmd/auth/logout/logout.go
View file

@ -105,6 +105,12 @@ func logoutRun(opts *LogoutOptions) error {
}
if err := cfg.CheckWriteable(hostname, "oauth_token"); err != nil {
var roErr *config.ReadOnlyEnvError
if errors.As(err, &roErr) {
fmt.Fprintf(opts.IO.ErrOut, "The value of the %s environment variable is being used for authentication.\n", roErr.Variable)
fmt.Fprint(opts.IO.ErrOut, "To erase credentials stored in GitHub CLI, first clear the value from the environment.\n")
return cmdutil.SilentError
}
return err
}

6
pkg/cmd/auth/refresh/refresh.go
View file

@ -112,6 +112,12 @@ func refreshRun(opts *RefreshOptions) error {
}
if err := cfg.CheckWriteable(hostname, "oauth_token"); err != nil {
var roErr *config.ReadOnlyEnvError
if errors.As(err, &roErr) {
fmt.Fprintf(opts.IO.ErrOut, "The value of the %s environment variable is being used for authentication.\n", roErr.Variable)
fmt.Fprint(opts.IO.ErrOut, "To refresh credentials stored in GitHub CLI, first clear the value from the environment.\n")
return cmdutil.SilentError
}
return err
}

14
pkg/cmd/auth/shared/client.go
View file

@ -8,20 +8,6 @@ import (
"github.com/cli/cli/internal/config"
)
func ValidateHostCfg(hostname string, cfg config.Config) error {
apiClient, err := ClientFromCfg(hostname, cfg)
if err != nil {
return err
}
err = apiClient.HasMinimumScopes(hostname)
if err != nil {
return fmt.Errorf("could not validate token: %w", err)
}
return nil
}
var ClientFromCfg = func(hostname string, cfg config.Config) (*api.Client, error) {
var opts []api.ClientOption