Merge pull request cli#1548 from cli/auth-check

Nate Smith · web-flow · commit 82c6830849c1 · 2020-08-25T10:16:23.000-05:00
new auth flow UX
diff --git a/cmd/gh/main.go b/cmd/gh/main.go
@@ -10,6 +10,7 @@ import (
 	"path"
 	"strings"
 
+	"github.com/cli/cli/api"
 	"github.com/cli/cli/command"
 	"github.com/cli/cli/internal/config"
 	"github.com/cli/cli/internal/ghinstance"
@@ -91,10 +92,41 @@ func main() {
 		}
 	}
 
+	authCheckEnabled := cmdutil.IsAuthCheckEnabled(cmd)
+
+	// TODO support other names
+	ghtoken := os.Getenv("GITHUB_TOKEN")
+	if ghtoken != "" {
+		authCheckEnabled = false
+	}
+
+	if authCheckEnabled {
+		hasAuth := false
+
+		cfg, err := cmdFactory.Config()
+		if err == nil {
+			hasAuth = cmdutil.CheckAuth(cfg)
+		}
+
+		if !hasAuth {
+			fmt.Fprintln(stderr, utils.Bold("Welcome to GitHub CLI!"))
+			fmt.Fprintln(stderr)
+			fmt.Fprintln(stderr, "To authenticate, please run `gh auth login`.")
+			fmt.Fprintln(stderr, "You can also set the GITHUB_TOKEN environment variable, if preferred.")
+			os.Exit(4)
+		}
+	}
+
 	rootCmd.SetArgs(expandedArgs)
 
 	if cmd, err := rootCmd.ExecuteC(); err != nil {
 		printError(stderr, err, cmd, hasDebug)
+
+		var httpErr api.HTTPError
+		if errors.As(err, &httpErr) && httpErr.StatusCode == 401 {
+			fmt.Println("hint: try authenticating with `gh auth login`")
+		}
+
 		os.Exit(1)
 	}
 	if root.HasFailed() {
diff --git a/pkg/cmd/alias/alias.go b/pkg/cmd/alias/alias.go
@@ -20,6 +20,8 @@ func NewCmdAlias(f *cmdutil.Factory) *cobra.Command {
 		`),
 	}
 
+	cmdutil.DisableAuthCheck(cmd)
+
 	cmd.AddCommand(deleteCmd.NewCmdDelete(f, nil))
 	cmd.AddCommand(listCmd.NewCmdList(f, nil))
 	cmd.AddCommand(setCmd.NewCmdSet(f, nil))
diff --git a/pkg/cmd/auth/auth.go b/pkg/cmd/auth/auth.go
@@ -16,6 +16,8 @@ func NewCmdAuth(f *cmdutil.Factory) *cobra.Command {
 		Long:  `Manage gh's authentication state.`,
 	}
 
+	cmdutil.DisableAuthCheck(cmd)
+
 	cmd.AddCommand(authLoginCmd.NewCmdLogin(f, nil))
 	cmd.AddCommand(authLogoutCmd.NewCmdLogout(f, nil))
 	cmd.AddCommand(authStatusCmd.NewCmdStatus(f, nil))
diff --git a/pkg/cmd/auth/login/login.go b/pkg/cmd/auth/login/login.go
@@ -54,7 +54,7 @@ func NewCmdLogin(f *cmdutil.Factory, runF func(*LoginOptions) error) *cobra.Comm
 			# => read token from mytoken.txt and authenticate against github.com
 
 			$ gh auth login --hostname enterprise.internal --with-token < mytoken.txt
-			# => read token from mytoken.txt and authenticate against a GitHub Enterprise instance
+			# => 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()
@@ -142,7 +142,7 @@ func loginRun(opts *LoginOptions) error {
 			Message: "What account do you want to log into?",
 			Options: []string{
 				"GitHub.com",
-				"GitHub Enterprise",
+				"GitHub Enterprise Server",
 			},
 		}, &hostType)
 
diff --git a/pkg/cmd/config/config.go b/pkg/cmd/config/config.go
@@ -21,6 +21,8 @@ func NewCmdConfig(f *cmdutil.Factory) *cobra.Command {
 		`),
 	}
 
+	cmdutil.DisableAuthCheck(cmd)
+
 	cmd.AddCommand(NewCmdConfigGet(f))
 	cmd.AddCommand(NewCmdConfigSet(f))
 
diff --git a/pkg/cmd/factory/http.go b/pkg/cmd/factory/http.go
@@ -1,7 +1,6 @@
 package factory
 
 import (
-	"errors"
 	"fmt"
 	"net/http"
 	"os"
@@ -30,20 +29,9 @@ func httpClient(io *iostreams.IOStreams, cfg config.Config, appVersion string, s
 
 			hostname := ghinstance.NormalizeHostname(req.URL.Hostname())
 			token, err := cfg.Get(hostname, "oauth_token")
-			if token == "" {
-				var notFound *config.NotFoundError
-				// TODO: check if stdout is TTY too
-				if errors.As(err, &notFound) && io.IsStdinTTY() {
-					// interactive OAuth flow
-					token, err = config.AuthFlowWithConfig(cfg, hostname, "Notice: authentication required", nil)
-				}
-				if err != nil {
-					return "", err
-				}
-				if token == "" {
-					// TODO: instruct user how to manually authenticate
-					return "", fmt.Errorf("authentication required for %s", hostname)
-				}
+			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
diff --git a/pkg/cmd/factory/remote_resolver.go b/pkg/cmd/factory/remote_resolver.go
@@ -72,7 +72,7 @@ func (rr *remoteResolver) Resolver() func() (context.Remotes, error) {
 		}
 
 		if len(cachedRemotes) == 0 {
-			remotesError = errors.New("none of the git remotes point to a known GitHub host")
+			remotesError = errors.New("none of the git remotes configured for this repository point to a known GitHub host. To tell gh about a new GitHub host, please use `gh auth login`")
 			return nil, remotesError
 		}
 		return cachedRemotes, nil
diff --git a/pkg/cmd/root/completion.go b/pkg/cmd/root/completion.go
@@ -56,6 +56,8 @@ func NewCmdCompletion(io *iostreams.IOStreams) *cobra.Command {
 		},
 	}
 
+	cmdutil.DisableAuthCheck(cmd)
+
 	cmd.Flags().StringVarP(&shellType, "shell", "s", "", "Shell type: {bash|zsh|fish|powershell}")
 
 	return cmd
diff --git a/pkg/cmd/root/root.go b/pkg/cmd/root/root.go
@@ -98,6 +98,8 @@ func NewCmdRoot(f *cmdutil.Factory, version, buildDate string) *cobra.Command {
 		return &cmdutil.FlagError{Err: err}
 	})
 
+	cmdutil.DisableAuthCheck(cmd)
+
 	// CHILD COMMANDS
 
 	cmd.AddCommand(aliasCmd.NewCmdAlias(f))
diff --git a/pkg/cmdutil/auth_check.go b/pkg/cmdutil/auth_check.go
@@ -0,0 +1,46 @@
+package cmdutil
+
+import (
+	"github.com/cli/cli/internal/config"
+	"github.com/spf13/cobra"
+)
+
+// TODO can have this set a PersistentPreRun so we don't have to set for all child commands of auth,
+// config
+
+func DisableAuthCheck(cmd *cobra.Command) {
+	if cmd.Annotations == nil {
+		cmd.Annotations = map[string]string{}
+	}
+
+	cmd.Annotations["skipAuthCheck"] = "true"
+}
+
+func CheckAuth(cfg config.Config) bool {
+	hosts, err := cfg.Hosts()
+	if err != nil {
+		return false
+	}
+
+	for _, hostname := range hosts {
+		token, _ := cfg.Get(hostname, "oauth_token")
+		if token != "" {
+			return true
+		}
+	}
+
+	return false
+}
+
+func IsAuthCheckEnabled(cmd *cobra.Command) bool {
+	if !cmd.Runnable() {
+		return false
+	}
+	for c := cmd; c.Parent() != nil; c = c.Parent() {
+		if c.Annotations != nil && c.Annotations["skipAuthCheck"] == "true" {
+			return false
+		}
+	}
+
+	return true
+}
diff --git a/pkg/cmdutil/auth_check_test.go b/pkg/cmdutil/auth_check_test.go
@@ -0,0 +1,45 @@
+package cmdutil
+
+import (
+	"testing"
+
+	"github.com/cli/cli/internal/config"
+	"github.com/stretchr/testify/assert"
+)
+
+func Test_CheckAuth(t *testing.T) {
+	tests := []struct {
+		name     string
+		cfg      func(config.Config)
+		expected bool
+	}{
+		{
+			name:     "no hosts",
+			cfg:      func(c config.Config) {},
+			expected: false,
+		},
+		{
+			name: "host, no token",
+			cfg: func(c config.Config) {
+				_ = c.Set("github.com", "oauth_token", "")
+			},
+			expected: false,
+		},
+		{
+			name: "host, token",
+			cfg: func(c config.Config) {
+				_ = c.Set("github.com", "oauth_token", "a token")
+			},
+			expected: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			cfg := config.NewBlankConfig()
+			tt.cfg(cfg)
+			result := CheckAuth(cfg)
+			assert.Equal(t, tt.expected, result)
+		})
+	}
+}

Original file line number	Diff line number	Diff line change
`@@ -20,6 +20,8 @@ func NewCmdAlias(f cmdutil.Factory) cobra.Command {`
`20`	`20`	`),
`21`	`21`	`}`
`22`	`22`
	`23`	`+ cmdutil.DisableAuthCheck(cmd)`
	`24`	`+`
`23`	`25`	`cmd.AddCommand(deleteCmd.NewCmdDelete(f, nil))`
`24`	`26`	`cmd.AddCommand(listCmd.NewCmdList(f, nil))`
`25`	`27`	`cmd.AddCommand(setCmd.NewCmdSet(f, nil))`
Original file line number	Diff line number	Diff line change
`@@ -16,6 +16,8 @@ func NewCmdAuth(f cmdutil.Factory) cobra.Command {`
`16`	`16`	Long: `Manage gh's authentication state.`,
`17`	`17`	`}`
`18`	`18`
	`19`	`+ cmdutil.DisableAuthCheck(cmd)`
	`20`	`+`
`19`	`21`	`cmd.AddCommand(authLoginCmd.NewCmdLogin(f, nil))`
`20`	`22`	`cmd.AddCommand(authLogoutCmd.NewCmdLogout(f, nil))`
`21`	`23`	`cmd.AddCommand(authStatusCmd.NewCmdStatus(f, nil))`
Original file line number	Diff line number	Diff line change
`@@ -21,6 +21,8 @@ func NewCmdConfig(f cmdutil.Factory) cobra.Command {`
`21`	`21`	`),
`22`	`22`	`}`
`23`	`23`
	`24`	`+ cmdutil.DisableAuthCheck(cmd)`
	`25`	`+`
`24`	`26`	`cmd.AddCommand(NewCmdConfigGet(f))`
`25`	`27`	`cmd.AddCommand(NewCmdConfigSet(f))`
`26`	`28`
Original file line number	Diff line number	Diff line change
`@@ -72,7 +72,7 @@ func (rr *remoteResolver) Resolver() func() (context.Remotes, error) {`
`72`	`72`	`}`
`73`	`73`
`74`	`74`	`if len(cachedRemotes) == 0 {`
`75`		`- remotesError = errors.New("none of the git remotes point to a known GitHub host")`
	`75`	+ remotesError = errors.New("none of the git remotes configured for this repository point to a known GitHub host. To tell gh about a new GitHub host, please use `gh auth login`")
`76`	`76`	`return nil, remotesError`
`77`	`77`	`}`
`78`	`78`	`return cachedRemotes, nil`
Original file line number	Diff line number	Diff line change
`@@ -56,6 +56,8 @@ func NewCmdCompletion(io iostreams.IOStreams) cobra.Command {`
`56`	`56`	`},`
`57`	`57`	`}`
`58`	`58`
	`59`	`+ cmdutil.DisableAuthCheck(cmd)`
	`60`	`+`
`59`	`61`	`cmd.Flags().StringVarP(&shellType, "shell", "s", "", "Shell type: {bash\|zsh\|fish\|powershell}")`
`60`	`62`
`61`	`63`	`return cmd`