From c80292c2e8c98c16897092249229e115ea98ae04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Mon, 7 Sep 2020 21:33:26 +0200 Subject: [PATCH] Extend Config object with GITHUB_TOKEN support Adding GITHUB_TOKEN & GITHUB_ENTERPRISE_TOKEN support orthogonal to Config was getting out of hand, especially in `auth` commands that adjust their messaging and error status based on the presence of these environment variables. The new approach builds in support for tokens from environment straight into Config object by composition. Thus, commands need not ever be concerned with any specific environment variables. --- internal/config/config_type.go | 30 +++-- internal/config/from_env.go | 71 ++++++++++++ internal/config/from_env_test.go | 160 +++++++++++++++++++++++++++ pkg/cmd/auth/login/login.go | 36 ++---- pkg/cmd/auth/login/login_test.go | 41 ++----- pkg/cmd/auth/logout/logout.go | 9 +- pkg/cmd/auth/logout/logout_test.go | 16 +-- pkg/cmd/auth/refresh/refresh.go | 9 +- pkg/cmd/auth/refresh/refresh_test.go | 13 --- pkg/cmd/auth/status/status.go | 93 ++++------------ pkg/cmd/auth/status/status_test.go | 74 +------------ pkg/cmd/config/config_test.go | 13 ++- pkg/cmd/factory/default.go | 1 + pkg/cmd/factory/http.go | 15 +-- 14 files changed, 325 insertions(+), 256 deletions(-) create mode 100644 internal/config/from_env.go create mode 100644 internal/config/from_env_test.go diff --git a/internal/config/config_type.go b/internal/config/config_type.go index 28d91f9b0..8bb20df43 100644 --- a/internal/config/config_type.go +++ b/internal/config/config_type.go @@ -15,10 +15,12 @@ const defaultGitProtocol = "https" // This interface describes interacting with some persistent configuration for gh. type Config interface { Get(string, string) (string, error) + GetWithSource(string, string) (string, string, error) Set(string, string, string) error UnsetHost(string) Hosts() ([]string, error) Aliases() (*AliasConfig, error) + CheckWriteable(string, string) error Write() error } @@ -200,42 +202,51 @@ func (c *fileConfig) Root() *yaml.Node { } func (c *fileConfig) Get(hostname, key string) (string, error) { + val, _, err := c.GetWithSource(hostname, key) + return val, err +} + +func (c *fileConfig) GetWithSource(hostname, key string) (string, string, error) { if hostname != "" { var notFound *NotFoundError hostCfg, err := c.configForHost(hostname) if err != nil && !errors.As(err, ¬Found) { - return "", err + return "", "", err } var hostValue string if hostCfg != nil { hostValue, err = hostCfg.GetStringValue(key) if err != nil && !errors.As(err, ¬Found) { - return "", err + return "", "", err } } if hostValue != "" { - return hostValue, nil + // TODO: avoid hardcoding this + return hostValue, "~/.config/gh/hosts.yml", nil } } + // TODO: avoid hardcoding this + defaultSource := "~/.config/gh/config.yml" + value, err := c.GetStringValue(key) var notFound *NotFoundError if err != nil && errors.As(err, ¬Found) { - return defaultFor(key), nil + return defaultFor(key), defaultSource, nil } else if err != nil { - return "", err + return "", defaultSource, err } if value == "" { - return defaultFor(key), nil + return defaultFor(key), defaultSource, nil } - return value, nil + return value, defaultSource, nil } func (c *fileConfig) Set(hostname, key, value string) error { @@ -281,6 +292,11 @@ func (c *fileConfig) configForHost(hostname string) (*HostConfig, error) { return nil, &NotFoundError{fmt.Errorf("could not find config entry for %q", hostname)} } +func (c *fileConfig) CheckWriteable(hostname, key string) error { + // TODO: check filesystem permissions + return nil +} + func (c *fileConfig) Write() error { mainData := yaml.Node{Kind: yaml.MappingNode} hostsData := yaml.Node{Kind: yaml.MappingNode} diff --git a/internal/config/from_env.go b/internal/config/from_env.go new file mode 100644 index 000000000..fa930da06 --- /dev/null +++ b/internal/config/from_env.go @@ -0,0 +1,71 @@ +package config + +import ( + "fmt" + "os" + + "github.com/cli/cli/internal/ghinstance" +) + +const ( + GITHUB_TOKEN = "GITHUB_TOKEN" + GITHUB_ENTERPRISE_TOKEN = "GITHUB_ENTERPRISE_TOKEN" +) + +func InheritEnv(c Config) Config { + return &envConfig{Config: c} +} + +type envConfig struct { + Config +} + +func (c *envConfig) Hosts() ([]string, error) { + hasDefault := false + hosts, err := c.Config.Hosts() + for _, h := range hosts { + if h == ghinstance.Default() { + hasDefault = true + } + } + if (err != nil || !hasDefault) && os.Getenv(GITHUB_TOKEN) != "" { + hosts = append([]string{ghinstance.Default()}, hosts...) + return hosts, nil + } + return hosts, err +} + +func (c *envConfig) Get(hostname, key string) (string, error) { + val, _, err := c.GetWithSource(hostname, key) + return val, err +} + +func (c *envConfig) GetWithSource(hostname, key string) (string, string, error) { + if hostname != "" && key == "oauth_token" { + envName := GITHUB_TOKEN + if ghinstance.IsEnterprise(hostname) { + envName = GITHUB_ENTERPRISE_TOKEN + } + + if value := os.Getenv(envName); value != "" { + return value, envName, nil + } + } + + return c.Config.GetWithSource(hostname, key) +} + +func (c *envConfig) CheckWriteable(hostname, key string) error { + if hostname != "" && key == "oauth_token" { + envName := GITHUB_TOKEN + if ghinstance.IsEnterprise(hostname) { + envName = GITHUB_ENTERPRISE_TOKEN + } + + if os.Getenv(envName) != "" { + return fmt.Errorf("read-only token in %s cannot be modified", envName) + } + } + + return c.Config.CheckWriteable(hostname, key) +} diff --git a/internal/config/from_env_test.go b/internal/config/from_env_test.go new file mode 100644 index 000000000..412d37248 --- /dev/null +++ b/internal/config/from_env_test.go @@ -0,0 +1,160 @@ +package config + +import ( + "os" + "testing" + + "github.com/MakeNowJust/heredoc" + "github.com/stretchr/testify/assert" +) + +func TestInheritEnv(t *testing.T) { + orig_GITHUB_TOKEN := os.Getenv("GITHUB_TOKEN") + orig_GITHUB_ENTERPRISE_TOKEN := os.Getenv("GITHUB_ENTERPRISE_TOKEN") + t.Cleanup(func() { + os.Setenv("GITHUB_TOKEN", orig_GITHUB_TOKEN) + os.Setenv("GITHUB_ENTERPRISE_TOKEN", orig_GITHUB_ENTERPRISE_TOKEN) + }) + + type wants struct { + hosts []string + token string + source string + writeable bool + } + + tests := []struct { + name string + baseConfig string + GITHUB_TOKEN string + GITHUB_ENTERPRISE_TOKEN string + hostname string + wants wants + }{ + { + name: "blank", + baseConfig: ``, + GITHUB_TOKEN: "", + GITHUB_ENTERPRISE_TOKEN: "", + hostname: "github.com", + wants: wants{ + hosts: []string(nil), + token: "", + source: "~/.config/gh/config.yml", + writeable: true, + }, + }, + { + name: "GITHUB_TOKEN over blank config", + baseConfig: ``, + GITHUB_TOKEN: "OTOKEN", + GITHUB_ENTERPRISE_TOKEN: "", + hostname: "github.com", + wants: wants{ + hosts: []string{"github.com"}, + token: "OTOKEN", + source: "GITHUB_TOKEN", + writeable: false, + }, + }, + { + name: "GITHUB_TOKEN not applicable to GHE", + baseConfig: ``, + GITHUB_TOKEN: "OTOKEN", + GITHUB_ENTERPRISE_TOKEN: "", + hostname: "example.org", + wants: wants{ + hosts: []string{"github.com"}, + token: "", + source: "~/.config/gh/config.yml", + writeable: true, + }, + }, + { + name: "GITHUB_ENTERPRISE_TOKEN over blank config", + baseConfig: ``, + GITHUB_TOKEN: "", + GITHUB_ENTERPRISE_TOKEN: "ENTOKEN", + hostname: "example.org", + wants: wants{ + hosts: []string(nil), + token: "ENTOKEN", + source: "GITHUB_ENTERPRISE_TOKEN", + writeable: false, + }, + }, + { + name: "token from file", + baseConfig: heredoc.Doc(` + hosts: + github.com: + oauth_token: OTOKEN + `), + GITHUB_TOKEN: "", + GITHUB_ENTERPRISE_TOKEN: "", + hostname: "github.com", + wants: wants{ + hosts: []string{"github.com"}, + token: "OTOKEN", + source: "~/.config/gh/hosts.yml", + writeable: true, + }, + }, + { + name: "GITHUB_TOKEN shadows token from file", + baseConfig: heredoc.Doc(` + hosts: + github.com: + oauth_token: OTOKEN + `), + GITHUB_TOKEN: "ENVTOKEN", + GITHUB_ENTERPRISE_TOKEN: "", + hostname: "github.com", + wants: wants{ + hosts: []string{"github.com"}, + token: "ENVTOKEN", + source: "GITHUB_TOKEN", + writeable: false, + }, + }, + { + name: "GITHUB_TOKEN adds host entry", + baseConfig: heredoc.Doc(` + hosts: + example.org: + oauth_token: OTOKEN + `), + GITHUB_TOKEN: "ENVTOKEN", + GITHUB_ENTERPRISE_TOKEN: "", + hostname: "github.com", + wants: wants{ + hosts: []string{"github.com", "example.org"}, + token: "ENVTOKEN", + source: "GITHUB_TOKEN", + writeable: false, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + os.Setenv("GITHUB_TOKEN", tt.GITHUB_TOKEN) + os.Setenv("GITHUB_ENTERPRISE_TOKEN", tt.GITHUB_ENTERPRISE_TOKEN) + + baseCfg := NewFromString(tt.baseConfig) + cfg := InheritEnv(baseCfg) + + hosts, _ := cfg.Hosts() + assert.Equal(t, tt.wants.hosts, hosts) + + val, source, _ := cfg.GetWithSource(tt.hostname, "oauth_token") + assert.Equal(t, tt.wants.token, val) + assert.Equal(t, tt.wants.source, source) + + val, _ = cfg.Get(tt.hostname, "oauth_token") + assert.Equal(t, tt.wants.token, val) + + err := cfg.CheckWriteable(tt.hostname, "oauth_token") + assert.Equal(t, tt.wants.writeable, err == nil) + }) + } +} diff --git a/pkg/cmd/auth/login/login.go b/pkg/cmd/auth/login/login.go index 07e64a6dc..eb58484ec 100644 --- a/pkg/cmd/auth/login/login.go +++ b/pkg/cmd/auth/login/login.go @@ -4,7 +4,6 @@ import ( "errors" "fmt" "io/ioutil" - "os" "strings" "github.com/AlecAivazis/survey/v2" @@ -24,9 +23,8 @@ type LoginOptions struct { IO *iostreams.IOStreams Config func() (config.Config, error) - Hostname string - Token string - OnlyValidate bool + Hostname string + Token string } func NewCmdLogin(f *cmdutil.Factory, runF func(*LoginOptions) error) *cobra.Command { @@ -35,6 +33,8 @@ func NewCmdLogin(f *cmdutil.Factory, runF func(*LoginOptions) error) *cobra.Comm Config: f.Config, } + var tokenStdin bool + cmd := &cobra.Command{ Use: "login", Args: cobra.ExactArgs(0), @@ -57,27 +57,13 @@ func NewCmdLogin(f *cmdutil.Factory, runF func(*LoginOptions) error) *cobra.Comm # => read token from mytoken.txt and authenticate against a GitHub Enterprise Server instance `), RunE: func(cmd *cobra.Command, args []string) error { - isTTY := opts.IO.IsStdinTTY() - - // TODO support other ways of naming - ghToken := os.Getenv("GITHUB_TOKEN") - - if !isTTY && (!cmd.Flags().Changed("with-token") && ghToken == "") { - return &cmdutil.FlagError{Err: errors.New("no terminal detected; please use '--with-token' or set GITHUB_TOKEN")} - } - - wt, _ := cmd.Flags().GetBool("with-token") - if wt { + if tokenStdin { defer opts.IO.In.Close() token, err := ioutil.ReadAll(opts.IO.In) if err != nil { - return &cmdutil.FlagError{Err: fmt.Errorf("failed to read token from STDIN: %w", err)} + return fmt.Errorf("failed to read token from STDIN: %w", err) } - opts.Token = strings.TrimSpace(string(token)) - } else if ghToken != "" { - opts.OnlyValidate = true - opts.Token = ghToken } if opts.Token != "" { @@ -102,7 +88,7 @@ func NewCmdLogin(f *cmdutil.Factory, runF func(*LoginOptions) error) *cobra.Comm } cmd.Flags().StringVarP(&opts.Hostname, "hostname", "h", "", "The hostname of the GitHub instance to authenticate with") - cmd.Flags().Bool("with-token", false, "Read token from standard input") + cmd.Flags().BoolVar(&tokenStdin, "with-token", false, "Read token from standard input") return cmd } @@ -131,10 +117,6 @@ func loginRun(opts *LoginOptions) error { return err } - if opts.OnlyValidate { - return nil - } - return cfg.Write() } @@ -203,6 +185,10 @@ func loginRun(opts *LoginOptions) error { } } + if err := cfg.CheckWriteable(hostname, "oauth_token"); err != nil { + return err + } + var authMode int err = prompt.SurveyAskOne(&survey.Select{ Message: "How would you like to authenticate?", diff --git a/pkg/cmd/auth/login/login_test.go b/pkg/cmd/auth/login/login_test.go index 4af891b66..d90a0a2d6 100644 --- a/pkg/cmd/auth/login/login_test.go +++ b/pkg/cmd/auth/login/login_test.go @@ -3,7 +3,6 @@ package login import ( "bytes" "net/http" - "os" "regexp" "testing" @@ -26,7 +25,6 @@ func Test_NewCmdLogin(t *testing.T) { stdinTTY bool wants LoginOptions wantsErr bool - ghtoken string }{ { name: "nontty, with-token", @@ -49,13 +47,21 @@ func Test_NewCmdLogin(t *testing.T) { }, { name: "nontty, hostname", + stdinTTY: false, cli: "--hostname claire.redfield", - wantsErr: true, + wants: LoginOptions{ + Hostname: "claire.redfield", + Token: "", + }, }, { name: "nontty", + stdinTTY: false, cli: "", - wantsErr: true, + wants: LoginOptions{ + Hostname: "", + Token: "", + }, }, { name: "nontty, with-token, hostname", @@ -94,37 +100,10 @@ func Test_NewCmdLogin(t *testing.T) { Token: "", }, }, - { - name: "tty, GITHUB_TOKEN", - stdinTTY: true, - cli: "", - ghtoken: "abc123", - wants: LoginOptions{ - Hostname: "github.com", - Token: "abc123", - OnlyValidate: true, - }, - }, - { - name: "nontty, GITHUB_TOKEN", - stdinTTY: false, - cli: "", - ghtoken: "abc123", - wants: LoginOptions{ - Hostname: "github.com", - Token: "abc123", - OnlyValidate: true, - }, - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ghtoken := os.Getenv("GITHUB_TOKEN") - defer func() { - os.Setenv("GITHUB_TOKEN", ghtoken) - }() - os.Setenv("GITHUB_TOKEN", tt.ghtoken) io, stdin, _, _ := iostreams.Test() f := &cmdutil.Factory{ IOStreams: io, diff --git a/pkg/cmd/auth/logout/logout.go b/pkg/cmd/auth/logout/logout.go index 6f047a079..c5810f5b8 100644 --- a/pkg/cmd/auth/logout/logout.go +++ b/pkg/cmd/auth/logout/logout.go @@ -4,7 +4,6 @@ import ( "errors" "fmt" "net/http" - "os" "github.com/AlecAivazis/survey/v2" "github.com/MakeNowJust/heredoc" @@ -63,10 +62,6 @@ func NewCmdLogout(f *cmdutil.Factory, runF func(*LogoutOptions) error) *cobra.Co } func logoutRun(opts *LogoutOptions) error { - if os.Getenv("GITHUB_TOKEN") != "" { - return errors.New("GITHUB_TOKEN is set in your environment. If you no longer want to use it with gh, please unset it.") - } - isTTY := opts.IO.IsStdinTTY() && opts.IO.IsStdoutTTY() hostname := opts.Hostname @@ -114,6 +109,10 @@ func logoutRun(opts *LogoutOptions) error { } } + if err := cfg.CheckWriteable(hostname, "oauth_token"); err != nil { + return err + } + httpClient, err := opts.HttpClient() if err != nil { return err diff --git a/pkg/cmd/auth/logout/logout_test.go b/pkg/cmd/auth/logout/logout_test.go index 50e1bed2a..724705776 100644 --- a/pkg/cmd/auth/logout/logout_test.go +++ b/pkg/cmd/auth/logout/logout_test.go @@ -3,7 +3,6 @@ package logout import ( "bytes" "net/http" - "os" "regexp" "testing" @@ -213,21 +212,10 @@ func Test_logoutRun_nontty(t *testing.T) { }, wantErr: regexp.MustCompile(`not logged in to any hosts`), }, - { - name: "gh token is set", - opts: &LogoutOptions{}, - ghtoken: "abc123", - wantErr: regexp.MustCompile(`GITHUB_TOKEN is set in your environment`), - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ghtoken := os.Getenv("GITHUB_TOKEN") - defer func() { - os.Setenv("GITHUB_TOKEN", ghtoken) - }() - os.Setenv("GITHUB_TOKEN", tt.ghtoken) io, _, _, stderr := iostreams.Test() io.SetStdinTTY(false) @@ -256,7 +244,9 @@ func Test_logoutRun_nontty(t *testing.T) { assert.Equal(t, tt.wantErr == nil, err == nil) if err != nil { if tt.wantErr != nil { - assert.True(t, tt.wantErr.MatchString(err.Error())) + if !tt.wantErr.MatchString(err.Error()) { + t.Errorf("got error: %v", err) + } return } else { t.Fatalf("unexpected error: %s", err) diff --git a/pkg/cmd/auth/refresh/refresh.go b/pkg/cmd/auth/refresh/refresh.go index 8a69ea0e2..757903bdb 100644 --- a/pkg/cmd/auth/refresh/refresh.go +++ b/pkg/cmd/auth/refresh/refresh.go @@ -2,7 +2,6 @@ package refresh import ( "fmt" - "os" "github.com/AlecAivazis/survey/v2" "github.com/MakeNowJust/heredoc" @@ -64,10 +63,6 @@ func NewCmdRefresh(f *cmdutil.Factory, runF func(*RefreshOptions) error) *cobra. } func refreshRun(opts *RefreshOptions) error { - if os.Getenv("GITHUB_TOKEN") != "" { - return fmt.Errorf("GITHUB_TOKEN is present in your environment and is incompatible with this command. If you'd like to modify a personal access token, see https://github.com/settings/tokens") - } - isTTY := opts.IO.IsStdinTTY() && opts.IO.IsStdoutTTY() if !isTTY { @@ -112,5 +107,9 @@ func refreshRun(opts *RefreshOptions) error { } } + if err := cfg.CheckWriteable(hostname, "oauth_token"); err != nil { + return err + } + return opts.AuthFlow(cfg, hostname, opts.Scopes) } diff --git a/pkg/cmd/auth/refresh/refresh_test.go b/pkg/cmd/auth/refresh/refresh_test.go index 3a22d507f..faacb0aa7 100644 --- a/pkg/cmd/auth/refresh/refresh_test.go +++ b/pkg/cmd/auth/refresh/refresh_test.go @@ -2,7 +2,6 @@ package refresh import ( "bytes" - "os" "regexp" "testing" @@ -94,16 +93,9 @@ func Test_refreshRun(t *testing.T) { askStubs func(*prompt.AskStubber) cfgHosts []string wantErr *regexp.Regexp - ghtoken string nontty bool wantAuthArgs authArgs }{ - { - name: "GITHUB_TOKEN set", - opts: &RefreshOptions{}, - ghtoken: "abc123", - wantErr: regexp.MustCompile(`GITHUB_TOKEN is present in your environment`), - }, { name: "non tty", opts: &RefreshOptions{}, @@ -193,11 +185,6 @@ func Test_refreshRun(t *testing.T) { return nil } - ghtoken := os.Getenv("GITHUB_TOKEN") - defer func() { - os.Setenv("GITHUB_TOKEN", ghtoken) - }() - os.Setenv("GITHUB_TOKEN", tt.ghtoken) io, _, _, _ := iostreams.Test() io.SetStdinTTY(!tt.nontty) diff --git a/pkg/cmd/auth/status/status.go b/pkg/cmd/auth/status/status.go index 09d2f2fde..78b4b074e 100644 --- a/pkg/cmd/auth/status/status.go +++ b/pkg/cmd/auth/status/status.go @@ -4,13 +4,10 @@ import ( "errors" "fmt" "net/http" - "os" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/api" "github.com/cli/cli/internal/config" - "github.com/cli/cli/internal/ghinstance" - "github.com/cli/cli/pkg/cmd/auth/client" "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/iostreams" "github.com/cli/cli/utils" @@ -21,8 +18,8 @@ type StatusOptions struct { HttpClient func() (*http.Client, error) IO *iostreams.IOStreams Config func() (config.Config, error) - Token string - Hostname string + + Hostname string } func NewCmdStatus(f *cmdutil.Factory, runF func(*StatusOptions) error) *cobra.Command { @@ -42,13 +39,6 @@ func NewCmdStatus(f *cmdutil.Factory, runF func(*StatusOptions) error) *cobra.Co report on any issues. `), RunE: func(cmd *cobra.Command, args []string) error { - // TODO support other names - opts.Token = os.Getenv("GITHUB_TOKEN") - - if opts.Token != "" && opts.Hostname == "" { - opts.Hostname = ghinstance.Default() - } - if runF != nil { return runF(opts) } @@ -72,52 +62,6 @@ func statusRun(opts *StatusOptions) error { stderr := opts.IO.ErrOut - if opts.Token != "" { - hostname := opts.Hostname - err := cfg.Set(opts.Hostname, "oauth_token", opts.Token) - if err != nil { - return err - } - - apiClient, err := client.ClientFromCfg(hostname, cfg) - if err != nil { - return err - } - - err = apiClient.HasMinimumScopes(hostname) - if err != nil { - var missingScopes *api.MissingScopesError - if errors.As(err, &missingScopes) { - fmt.Fprintf(stderr, "%s %s: %s\n", utils.Red("X"), hostname, err) - fmt.Fprintln(stderr, - "The token in GITHUB_TOKEN is valid but missing scopes that gh requires to function.") - } else { - fmt.Fprintf(stderr, "%s %s: authentication failed\n", utils.Red("X"), hostname) - fmt.Fprintln(stderr) - fmt.Fprintf(stderr, - "The token in GITHUB_TOKEN is invalid.\n") - } - fmt.Fprintf(stderr, - "Please visit https://%s/settings/tokens and create a new token with 'repo', 'read:org', and 'gist' scopes.\n", hostname) - return cmdutil.SilentError - } else { - username, err := api.CurrentLoginName(apiClient, hostname) - if err != nil { - return fmt.Errorf("%s %s: api call failed: %s\n", utils.Red("X"), hostname, err) - } - fmt.Fprintf(stderr, - "%s token valid for %s as %s\n", utils.GreenCheck(), hostname, utils.Bold(username)) - proto, _ := cfg.Get(hostname, "git_protocol") - if proto != "" { - fmt.Fprintln(stderr) - fmt.Fprintf(stderr, - "Git operations for %s configured to use %s protocol.\n", hostname, utils.Bold(proto)) - } - } - - return nil - } - statusInfo := map[string][]string{} hostnames, err := cfg.Hosts() @@ -140,6 +84,9 @@ func statusRun(opts *StatusOptions) error { continue } + _, tokenSource, _ := cfg.GetWithSource(hostname, "oauth_token") + tokenIsWriteable := cfg.CheckWriteable(hostname, "oauth_token") == nil + statusInfo[hostname] = []string{} addMsg := func(x string, ys ...interface{}) { statusInfo[hostname] = append(statusInfo[hostname], fmt.Sprintf(x, ys...)) @@ -149,32 +96,36 @@ func statusRun(opts *StatusOptions) error { if err != nil { var missingScopes *api.MissingScopesError if errors.As(err, &missingScopes) { - addMsg("%s %s: %s\n", utils.Red("X"), hostname, err) - addMsg("- To enable the missing scopes, please run %s %s\n", - utils.Bold("gh auth refresh -h"), - utils.Bold(hostname)) + addMsg("%s %s: %s", utils.Red("X"), hostname, err) + if tokenIsWriteable { + addMsg("- To request missing scopes, run: %s %s\n", + utils.Bold("gh auth refresh -h"), + utils.Bold(hostname)) + } } else { - addMsg("%s %s: authentication failed\n", utils.Red("X"), hostname) - addMsg("- The configured token for %s is no longer valid.", utils.Bold(hostname)) - addMsg("- To re-authenticate, please run %s %s", - utils.Bold("gh auth login -h"), utils.Bold(hostname)) - addMsg("- To forget about this host, please run %s %s", - utils.Bold("gh auth logout -h"), utils.Bold(hostname)) + addMsg("%s %s: authentication failed", utils.Red("X"), hostname) + addMsg("- The %s token in %s is no longer valid.", utils.Bold(hostname), tokenSource) + if tokenIsWriteable { + addMsg("- To re-authenticate, run: %s %s", + utils.Bold("gh auth login -h"), utils.Bold(hostname)) + addMsg("- To forget about this host, run: %s %s", + utils.Bold("gh auth logout -h"), utils.Bold(hostname)) + } } failed = true } else { username, err := api.CurrentLoginName(apiClient, hostname) if err != nil { - addMsg("%s %s: api call failed: %s\n", utils.Red("X"), hostname, err) + addMsg("%s %s: api call failed: %s", utils.Red("X"), hostname, err) } - addMsg("%s Logged in to %s as %s", utils.GreenCheck(), hostname, utils.Bold(username)) + addMsg("%s Logged in to %s as %s (%s)", utils.GreenCheck(), hostname, utils.Bold(username), tokenSource) proto, _ := cfg.Get(hostname, "git_protocol") if proto != "" { addMsg("%s Git operations for %s configured to use %s protocol.", utils.GreenCheck(), hostname, utils.Bold(proto)) } - addMsg("") } + addMsg("") // NB we could take this opportunity to add or fix the "user" key in the hosts config. I chose // not to since I wanted this command to be read-only. diff --git a/pkg/cmd/auth/status/status_test.go b/pkg/cmd/auth/status/status_test.go index 4ab0d1b28..9aabf4fef 100644 --- a/pkg/cmd/auth/status/status_test.go +++ b/pkg/cmd/auth/status/status_test.go @@ -3,7 +3,6 @@ package status import ( "bytes" "net/http" - "os" "regexp" "testing" @@ -19,29 +18,10 @@ import ( func Test_NewCmdStatus(t *testing.T) { tests := []struct { - name string - cli string - wants StatusOptions - ghtoken string + name string + cli string + wants StatusOptions }{ - { - name: "ghtoken set", - cli: "", - wants: StatusOptions{ - Token: "abc123", - Hostname: "github.com", - }, - ghtoken: "abc123", - }, - { - name: "ghtoken set", - cli: "--hostname joel.miller", - wants: StatusOptions{ - Token: "def456", - Hostname: "joel.miller", - }, - ghtoken: "def456", - }, { name: "no arguments", cli: "", @@ -58,12 +38,6 @@ func Test_NewCmdStatus(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ghtoken := os.Getenv("GITHUB_TOKEN") - defer func() { - os.Setenv("GITHUB_TOKEN", ghtoken) - }() - os.Setenv("GITHUB_TOKEN", tt.ghtoken) - f := &cmdutil.Factory{} argv, err := shlex.Split(tt.cli) @@ -86,7 +60,6 @@ func Test_NewCmdStatus(t *testing.T) { _, err = cmd.ExecuteC() assert.NoError(t, err) - assert.Equal(t, tt.wants.Token, gotOpts.Token) assert.Equal(t, tt.wants.Hostname, gotOpts.Hostname) }) } @@ -101,47 +74,6 @@ func Test_statusRun(t *testing.T) { wantErr *regexp.Regexp wantErrOut *regexp.Regexp }{ - { - name: "token set, bad token", - opts: &StatusOptions{ - Token: "abc123", - Hostname: "github.com", - }, - httpStubs: func(reg *httpmock.Registry) { - reg.Register( - httpmock.REST("GET", ""), - httpmock.StatusStringResponse(400, "no bueno"), - ) - }, - wantErr: regexp.MustCompile(``), - wantErrOut: regexp.MustCompile(`authentication failed`), - }, - { - name: "token set, missing scope", - opts: &StatusOptions{ - Token: "abc123", - Hostname: "github.com", - }, - httpStubs: func(reg *httpmock.Registry) { - reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("repo,")) - }, - wantErr: regexp.MustCompile(``), - wantErrOut: regexp.MustCompile(`missing required scope 'read:org'`), - }, - { - name: "token set, good token", - opts: &StatusOptions{ - Token: "abc123", - Hostname: "github.com", - }, - httpStubs: func(reg *httpmock.Registry) { - reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("repo,read:org,")) - reg.Register( - httpmock.GraphQL(`query UserCurrent\b`), - httpmock.StringResponse(`{"data":{"viewer":{"login":"tess"}}}`)) - }, - wantErrOut: regexp.MustCompile(`token valid for github.com as.*tess`), - }, { name: "hostname set", opts: &StatusOptions{ diff --git a/pkg/cmd/config/config_test.go b/pkg/cmd/config/config_test.go index dc7906232..20c7dd543 100644 --- a/pkg/cmd/config/config_test.go +++ b/pkg/cmd/config/config_test.go @@ -21,10 +21,15 @@ func genKey(host, key string) string { } func (c configStub) Get(host, key string) (string, error) { + val, _, err := c.GetWithSource(host, key) + return val, err +} + +func (c configStub) GetWithSource(host, key string) (string, string, error) { if v, found := c[genKey(host, key)]; found { - return v, nil + return v, "(memory)", nil } - return "", errors.New("not found") + return "", "", errors.New("not found") } func (c configStub) Set(host, key, value string) error { @@ -43,6 +48,10 @@ func (c configStub) Hosts() ([]string, error) { func (c configStub) UnsetHost(hostname string) { } +func (c configStub) CheckWriteable(host, key string) error { + return nil +} + func (c configStub) Write() error { c["_written"] = "true" return nil diff --git a/pkg/cmd/factory/default.go b/pkg/cmd/factory/default.go index f667acb6b..ae8622b2a 100644 --- a/pkg/cmd/factory/default.go +++ b/pkg/cmd/factory/default.go @@ -27,6 +27,7 @@ func New(appVersion string) *cmdutil.Factory { cachedConfig = config.NewBlankConfig() configError = nil } + cachedConfig = config.InheritEnv(cachedConfig) return cachedConfig, configError } diff --git a/pkg/cmd/factory/http.go b/pkg/cmd/factory/http.go index d36bffdb2..0adc87a12 100644 --- a/pkg/cmd/factory/http.go +++ b/pkg/cmd/factory/http.go @@ -24,21 +24,10 @@ func NewHTTPClient(io *iostreams.IOStreams, cfg config.Config, appVersion string api.AddHeader("User-Agent", fmt.Sprintf("GitHub CLI %s", appVersion)), api.AddHeaderFunc("Authorization", func(req *http.Request) (string, error) { hostname := ghinstance.NormalizeHostname(req.URL.Hostname()) - - if token := os.Getenv("GITHUB_TOKEN"); hostname == ghinstance.Default() && token != "" { + if token, err := cfg.Get(hostname, "oauth_token"); err == nil || token != "" { return fmt.Sprintf("token %s", token), nil } - if token := os.Getenv("GITHUB_ENTERPRISE_TOKEN"); ghinstance.IsEnterprise(hostname) && token != "" { - return fmt.Sprintf("token %s", token), nil - } - - token, err := cfg.Get(hostname, "oauth_token") - if err != nil || token == "" { - // Users shouldn't see this because of the pre-execute auth check on commands - return "", fmt.Errorf("authentication required for %s; please run `gh auth login -h %s`", hostname, hostname) - } - - return fmt.Sprintf("token %s", token), nil + return "", nil }), )