From 3e7a2585c5519f59c9eb0b30954e1b51160b5a1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 4 Sep 2020 16:18:26 +0200 Subject: [PATCH 1/4] Tighten git remote URL parsing We now only support git URLs that have one of the explicitly supported protocols. --- git/url.go | 29 ++++--- git/url_test.go | 195 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 215 insertions(+), 9 deletions(-) create mode 100644 git/url_test.go diff --git a/git/url.go b/git/url.go index 51938f934..7d4aac4d7 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 000000000..679739b41 --- /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) + } + }) + } +} From 2bb5e052d824f89995ba3c03585d428a6078557f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 4 Sep 2020 17:25:37 +0200 Subject: [PATCH 2/4] Send GITHUB_TOKEN to github.com and GITHUB_ENTERPRISE_TOKEN to GHES --- cmd/gh/main.go | 20 +++++++------------- pkg/cmd/factory/http.go | 8 ++++++-- pkg/cmd/root/root.go | 6 ++++-- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/cmd/gh/main.go b/cmd/gh/main.go index 456195314..44357ffd6 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/pkg/cmd/factory/http.go b/pkg/cmd/factory/http.go index 1cccfda9c..9daf6d36b 100644 --- a/pkg/cmd/factory/http.go +++ b/pkg/cmd/factory/http.go @@ -23,11 +23,15 @@ func httpClient(io *iostreams.IOStreams, cfg config.Config, appVersion string, s 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 != "" { + hostname := ghinstance.NormalizeHostname(req.URL.Hostname()) + + if token := os.Getenv("GITHUB_TOKEN"); hostname == ghinstance.Default() && 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 } - 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 diff --git a/pkg/cmd/root/root.go b/pkg/cmd/root/root.go index 252636e52..947ba73a8 100644 --- a/pkg/cmd/root/root.go +++ b/pkg/cmd/root/root.go @@ -41,8 +41,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. 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 3/4] 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 }), ) From ece17c4ce27bdcd74d487fb4e93347330c788200 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Mon, 7 Sep 2020 21:37:55 +0200 Subject: [PATCH 4/4] Add GITHUB_ENTERPRISE_TOKEN mention to `api` help --- pkg/cmd/api/api.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index 74fc44ea8..f051883a8 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),