diff --git a/cmd/gh/main.go b/cmd/gh/main.go index 456195314ae..44357ffd6f6 100644 --- a/cmd/gh/main.go +++ b/cmd/gh/main.go @@ -92,23 +92,17 @@ func main() { } } - authCheckEnabled := cmdutil.IsAuthCheckEnabled(cmd) - - // TODO support other names - ghtoken := os.Getenv("GITHUB_TOKEN") - if ghtoken != "" { - authCheckEnabled = false - } - + authCheckEnabled := os.Getenv("GITHUB_TOKEN") == "" && + os.Getenv("GITHUB_ENTERPRISE_TOKEN") == "" && + cmdutil.IsAuthCheckEnabled(cmd) if authCheckEnabled { - hasAuth := false - cfg, err := cmdFactory.Config() - if err == nil { - hasAuth = cmdutil.CheckAuth(cfg) + if err != nil { + fmt.Fprintf(stderr, "failed to read configuration: %s\n", err) + os.Exit(2) } - if !hasAuth { + if !cmdutil.CheckAuth(cfg) { fmt.Fprintln(stderr, utils.Bold("Welcome to GitHub CLI!")) fmt.Fprintln(stderr) fmt.Fprintln(stderr, "To authenticate, please run `gh auth login`.") diff --git a/git/url.go b/git/url.go index 51938f93461..7d4aac4d77e 100644 --- a/git/url.go +++ b/git/url.go @@ -2,24 +2,35 @@ package git import ( "net/url" - "regexp" "strings" ) -var ( - protocolRe = regexp.MustCompile("^[a-zA-Z_+-]+://") -) - func IsURL(u string) bool { - return strings.HasPrefix(u, "git@") || protocolRe.MatchString(u) + return strings.HasPrefix(u, "git@") || isSupportedProtocol(u) +} + +func isSupportedProtocol(u string) bool { + return strings.HasPrefix(u, "ssh:") || + strings.HasPrefix(u, "git+ssh:") || + strings.HasPrefix(u, "git:") || + strings.HasPrefix(u, "http:") || + strings.HasPrefix(u, "https:") +} + +func isPossibleProtocol(u string) bool { + return isSupportedProtocol(u) || + strings.HasPrefix(u, "ftp:") || + strings.HasPrefix(u, "ftps:") || + strings.HasPrefix(u, "file:") } // ParseURL normalizes git remote urls func ParseURL(rawURL string) (u *url.URL, err error) { - if !protocolRe.MatchString(rawURL) && - strings.Contains(rawURL, ":") && + if !isPossibleProtocol(rawURL) && + strings.ContainsRune(rawURL, ':') && // not a Windows path - !strings.Contains(rawURL, "\\") { + !strings.ContainsRune(rawURL, '\\') { + // support scp-like syntax for ssh protocol rawURL = "ssh://" + strings.Replace(rawURL, ":", "/", 1) } diff --git a/git/url_test.go b/git/url_test.go new file mode 100644 index 00000000000..679739b4111 --- /dev/null +++ b/git/url_test.go @@ -0,0 +1,195 @@ +package git + +import "testing" + +func TestIsURL(t *testing.T) { + tests := []struct { + name string + url string + want bool + }{ + { + name: "scp-like", + url: "git@example.com:owner/repo", + want: true, + }, + { + name: "scp-like with no user", + url: "example.com:owner/repo", + want: false, + }, + { + name: "ssh", + url: "ssh://git@example.com/owner/repo", + want: true, + }, + { + name: "git", + url: "git://example.com/owner/repo", + want: true, + }, + { + name: "https", + url: "https://example.com/owner/repo.git", + want: true, + }, + { + name: "no protocol", + url: "example.com/owner/repo", + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := IsURL(tt.url); got != tt.want { + t.Errorf("IsURL() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestParseURL(t *testing.T) { + type url struct { + Scheme string + User string + Host string + Path string + } + tests := []struct { + name string + url string + want url + wantErr bool + }{ + { + name: "HTTPS", + url: "https://example.com/owner/repo.git", + want: url{ + Scheme: "https", + User: "", + Host: "example.com", + Path: "/owner/repo.git", + }, + }, + { + name: "HTTP", + url: "http://example.com/owner/repo.git", + want: url{ + Scheme: "http", + User: "", + Host: "example.com", + Path: "/owner/repo.git", + }, + }, + { + name: "git", + url: "git://example.com/owner/repo.git", + want: url{ + Scheme: "git", + User: "", + Host: "example.com", + Path: "/owner/repo.git", + }, + }, + { + name: "ssh", + url: "ssh://git@example.com/owner/repo.git", + want: url{ + Scheme: "ssh", + User: "git", + Host: "example.com", + Path: "/owner/repo.git", + }, + }, + { + name: "ssh with port", + url: "ssh://git@example.com:443/owner/repo.git", + want: url{ + Scheme: "ssh", + User: "git", + Host: "example.com", + Path: "/owner/repo.git", + }, + }, + { + name: "git+ssh", + url: "git+ssh://example.com/owner/repo.git", + want: url{ + Scheme: "ssh", + User: "", + Host: "example.com", + Path: "/owner/repo.git", + }, + }, + { + name: "scp-like", + url: "git@example.com:owner/repo.git", + want: url{ + Scheme: "ssh", + User: "git", + Host: "example.com", + Path: "/owner/repo.git", + }, + }, + { + name: "scp-like, leading slash", + url: "git@example.com:/owner/repo.git", + want: url{ + Scheme: "ssh", + User: "git", + Host: "example.com", + Path: "/owner/repo.git", + }, + }, + { + name: "file protocol", + url: "file:///example.com/owner/repo.git", + want: url{ + Scheme: "file", + User: "", + Host: "", + Path: "/example.com/owner/repo.git", + }, + }, + { + name: "file path", + url: "/example.com/owner/repo.git", + want: url{ + Scheme: "", + User: "", + Host: "", + Path: "/example.com/owner/repo.git", + }, + }, + { + name: "Windows file path", + url: "C:\\example.com\\owner\\repo.git", + want: url{ + Scheme: "c", + User: "", + Host: "", + Path: "", + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + u, err := ParseURL(tt.url) + if (err != nil) != tt.wantErr { + t.Fatalf("got error: %v", err) + } + if u.Scheme != tt.want.Scheme { + t.Errorf("expected scheme %q, got %q", tt.want.Scheme, u.Scheme) + } + if u.User.Username() != tt.want.User { + t.Errorf("expected user %q, got %q", tt.want.User, u.User.Username()) + } + if u.Host != tt.want.Host { + t.Errorf("expected host %q, got %q", tt.want.Host, u.Host) + } + if u.Path != tt.want.Path { + t.Errorf("expected path %q, got %q", tt.want.Path, u.Path) + } + }) + } +} diff --git a/internal/config/config_type.go b/internal/config/config_type.go index 28d91f9b0d1..8bb20df43ea 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 00000000000..fa930da0668 --- /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 00000000000..412d37248f1 --- /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/api/api.go b/pkg/cmd/api/api.go index 74fc44ea8a2..f051883a8c1 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -117,7 +117,9 @@ original query accepts an '$endCursor: String' variable and that it fetches the `), Annotations: map[string]string{ "help:environment": heredoc.Doc(` - GITHUB_TOKEN: an authentication token for API requests. + GITHUB_TOKEN: an authentication token for github.com API requests. + + GITHUB_ENTERPRISE_TOKEN: an authentication token for API requests to GitHub Enterprise. `), }, Args: cobra.ExactArgs(1), diff --git a/pkg/cmd/auth/login/login.go b/pkg/cmd/auth/login/login.go index 07e64a6dcb3..eb58484ecd1 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 4af891b6654..d90a0a2d66e 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 6f047a07957..c5810f5b881 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 50e1bed2a5a..7247057763d 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 8a69ea0e2f2..757903bdba2 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 3a22d507f6e..faacb0aa7d8 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 09d2f2fde6d..78b4b074efa 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 4ab0d1b2837..9aabf4fefaf 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 dc790623213..20c7dd5432b 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 f667acb6baa..ae8622b2aae 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 f87bfa423f9..0adc87a126f 100644 --- a/pkg/cmd/factory/http.go +++ b/pkg/cmd/factory/http.go @@ -23,18 +23,11 @@ func NewHTTPClient(io *iostreams.IOStreams, cfg config.Config, appVersion string opts = append(opts, api.AddHeader("User-Agent", fmt.Sprintf("GitHub CLI %s", appVersion)), api.AddHeaderFunc("Authorization", func(req *http.Request) (string, error) { - if token := os.Getenv("GITHUB_TOKEN"); token != "" { - return fmt.Sprintf("token %s", token), nil - } - hostname := ghinstance.NormalizeHostname(req.URL.Hostname()) - 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) + if token, err := cfg.Get(hostname, "oauth_token"); err == nil || token != "" { + return fmt.Sprintf("token %s", token), nil } - - return fmt.Sprintf("token %s", token), nil + return "", nil }), ) diff --git a/pkg/cmd/root/root.go b/pkg/cmd/root/root.go index 986976121b1..de14c681b5e 100644 --- a/pkg/cmd/root/root.go +++ b/pkg/cmd/root/root.go @@ -43,8 +43,10 @@ func NewCmdRoot(f *cmdutil.Factory, version, buildDate string) *cobra.Command { Open an issue using “gh issue create -R cli/cli” `), "help:environment": heredoc.Doc(` - GITHUB_TOKEN: an authentication token for API requests. Setting this avoids being - prompted to authenticate and overrides any previously stored credentials. + GITHUB_TOKEN: an authentication token for github.com API requests. Setting this avoids + being prompted to authenticate and takes precedence over previously stored credentials. + + GITHUB_ENTERPRISE_TOKEN: an authentication token for API requests to GitHub Enterprise. GH_REPO: specify the GitHub repository in the "[HOST/]OWNER/REPO" format for commands that otherwise operate on a local repository.