Merge pull request cli#786 from cli/oauth-read-org

Nate Smith · web-flow · commit d9083200504d · 2020-04-28T20:23:44.000-05:00
Ask for `read:org` OAuth scope, warn for outdated tokens
diff --git a/api/client.go b/api/client.go
@@ -37,6 +37,16 @@ func AddHeader(name, value string) ClientOption {
 	}
 }
 
+// AddHeaderFunc is an AddHeader that gets the string value from a function
+func AddHeaderFunc(name string, value func() string) ClientOption {
+	return func(tr http.RoundTripper) http.RoundTripper {
+		return &funcTripper{roundTrip: func(req *http.Request) (*http.Response, error) {
+			req.Header.Add(name, value())
+			return tr.RoundTrip(req)
+		}}
+	}
+}
+
 // VerboseLog enables request/response logging within a RoundTripper
 func VerboseLog(out io.Writer, logTraffic bool, colorize bool) ClientOption {
 	logger := &httpretty.Logger{
@@ -63,6 +73,40 @@ func ReplaceTripper(tr http.RoundTripper) ClientOption {
 	}
 }
 
+var issuedScopesWarning bool
+
+// CheckScopes checks whether an OAuth scope is present in a response
+func CheckScopes(wantedScope string, cb func(string) error) ClientOption {
+	return func(tr http.RoundTripper) http.RoundTripper {
+		return &funcTripper{roundTrip: func(req *http.Request) (*http.Response, error) {
+			res, err := tr.RoundTrip(req)
+			if err != nil || res.StatusCode > 299 || issuedScopesWarning {
+				return res, err
+			}
+
+			appID := res.Header.Get("X-Oauth-Client-Id")
+			hasScopes := strings.Split(res.Header.Get("X-Oauth-Scopes"), ",")
+
+			hasWanted := false
+			for _, s := range hasScopes {
+				if wantedScope == strings.TrimSpace(s) {
+					hasWanted = true
+					break
+				}
+			}
+
+			if !hasWanted {
+				if err := cb(appID); err != nil {
+					return res, err
+				}
+				issuedScopesWarning = true
+			}
+
+			return res, nil
+		}}
+	}
+}
+
 type funcTripper struct {
 	roundTrip func(*http.Request) (*http.Response, error)
 }
diff --git a/auth/oauth.go b/auth/oauth.go
@@ -11,6 +11,7 @@ import (
 	"net/http"
 	"net/url"
 	"os"
+	"strings"
 
 	"github.com/cli/cli/pkg/browser"
 )
@@ -29,6 +30,7 @@ type OAuthFlow struct {
 	Hostname         string
 	ClientID         string
 	ClientSecret     string
+	Scopes           []string
 	WriteSuccessHTML func(io.Writer)
 	VerboseStream    io.Writer
 }
@@ -45,11 +47,15 @@ func (oa *OAuthFlow) ObtainAccessToken() (accessToken string, err error) {
 	}
 	port := listener.Addr().(*net.TCPAddr).Port
 
+	scopes := "repo"
+	if oa.Scopes != nil {
+		scopes = strings.Join(oa.Scopes, " ")
+	}
+
 	q := url.Values{}
 	q.Set("client_id", oa.ClientID)
 	q.Set("redirect_uri", fmt.Sprintf("http://127.0.0.1:%d/callback", port))
-	// TODO: make scopes configurable
-	q.Set("scope", "repo")
+	q.Set("scope", scopes)
 	q.Set("state", state)
 
 	startURL := fmt.Sprintf("https://%s/login/oauth/authorize?%s", oa.Hostname, q.Encode())
diff --git a/command/root.go b/command/root.go
@@ -132,12 +132,47 @@ var apiClientForContext = func(ctx context.Context) (*api.Client, error) {
 	if err != nil {
 		return nil, err
 	}
+
 	var opts []api.ClientOption
 	if verbose := os.Getenv("DEBUG"); verbose != "" {
 		opts = append(opts, apiVerboseLog())
 	}
+
+	getAuthValue := func() string {
+		return fmt.Sprintf("token %s", token)
+	}
+
+	checkScopesFunc := func(appID string) error {
+		if config.IsGitHubApp(appID) && utils.IsTerminal(os.Stdin) && utils.IsTerminal(os.Stderr) {
+			newToken, loginHandle, err := config.AuthFlow("Notice: additional authorization required")
+			if err != nil {
+				return err
+			}
+			cfg, err := ctx.Config()
+			if err != nil {
+				return err
+			}
+			_ = cfg.Set(defaultHostname, "oauth_token", newToken)
+			_ = cfg.Set(defaultHostname, "user", loginHandle)
+			// update config file on disk
+			err = cfg.Write()
+			if err != nil {
+				return err
+			}
+			// update configuration in memory
+			token = newToken
+			config.AuthFlowComplete()
+		} else {
+			fmt.Fprintln(os.Stderr, "Warning: gh now requires the `read:org` OAuth scope.")
+			fmt.Fprintln(os.Stderr, "Visit https://github.com/settings/tokens and edit your token to enable `read:org`")
+			fmt.Fprintln(os.Stderr, "or generate a new token and paste it via `gh config set -h github.com oauth_token MYTOKEN`")
+		}
+		return nil
+	}
+
 	opts = append(opts,
-		api.AddHeader("Authorization", fmt.Sprintf("token %s", token)),
+		api.CheckScopes("read:org", checkScopesFunc),
+		api.AddHeaderFunc("Authorization", getAuthValue),
 		api.AddHeader("User-Agent", fmt.Sprintf("GitHub CLI %s", Version)),
 		// antiope-preview: Checks
 		api.AddHeader("Accept", "application/vnd.github.antiope-preview+json"),
diff --git a/internal/config/config_setup.go b/internal/config/config_setup.go
@@ -24,9 +24,14 @@ var (
 	oauthClientSecret = "34ddeff2b558a23d38fba8a6de74f086ede1cc0b"
 )
 
-// TODO: have a conversation about whether this belongs in the "context" package
-// FIXME: make testable
-func setupConfigFile(filename string) (Config, error) {
+// IsGitHubApp reports whether an OAuth app is "GitHub CLI" or "GitHub CLI (dev)"
+func IsGitHubApp(id string) bool {
+	// this intentionally doesn't use `oauthClientID` because that is a variable
+	// that can potentially be changed at build time via GH_OAUTH_CLIENT_ID
+	return id == "178c6fc778ccc68e1d6a" || id == "4d747ba5675d5d66553f"
+}
+
+func AuthFlow(notice string) (string, string, error) {
 	var verboseStream io.Writer
 	if strings.Contains(os.Getenv("DEBUG"), "oauth") {
 		verboseStream = os.Stderr
@@ -36,21 +41,37 @@ func setupConfigFile(filename string) (Config, error) {
 		Hostname:     oauthHost,
 		ClientID:     oauthClientID,
 		ClientSecret: oauthClientSecret,
+		Scopes:       []string{"repo", "read:org"},
 		WriteSuccessHTML: func(w io.Writer) {
 			fmt.Fprintln(w, oauthSuccessPage)
 		},
 		VerboseStream: verboseStream,
 	}
 
-	fmt.Fprintln(os.Stderr, "Notice: authentication required")
+	fmt.Fprintln(os.Stderr, notice)
 	fmt.Fprintf(os.Stderr, "Press Enter to open %s in your browser... ", flow.Hostname)
 	_ = waitForEnter(os.Stdin)
 	token, err := flow.ObtainAccessToken()
 	if err != nil {
-		return nil, err
+		return "", "", err
 	}
 
 	userLogin, err := getViewer(token)
+	if err != nil {
+		return "", "", err
+	}
+
+	return token, userLogin, nil
+}
+
+func AuthFlowComplete() {
+	fmt.Fprintln(os.Stderr, "Authentication complete. Press Enter to continue... ")
+	_ = waitForEnter(os.Stdin)
+}
+
+// FIXME: make testable
+func setupConfigFile(filename string) (Config, error) {
+	token, userLogin, err := AuthFlow("Notice: authentication required")
 	if err != nil {
 		return nil, err
 	}
@@ -61,9 +82,9 @@ func setupConfigFile(filename string) (Config, error) {
 	}
 
 	yamlHosts := map[string]map[string]string{}
-	yamlHosts[flow.Hostname] = map[string]string{}
-	yamlHosts[flow.Hostname]["user"] = userLogin
-	yamlHosts[flow.Hostname]["oauth_token"] = token
+	yamlHosts[oauthHost] = map[string]string{}
+	yamlHosts[oauthHost]["user"] = userLogin
+	yamlHosts[oauthHost]["oauth_token"] = token
 
 	defaultConfig := yamlConfig{
 		Hosts: yamlHosts,
@@ -84,14 +105,9 @@ func setupConfigFile(filename string) (Config, error) {
 	if err != nil {
 		return nil, err
 	}
-	n, err := cfgFile.Write(yamlData)
-	if err == nil && n < len(yamlData) {
-		err = io.ErrShortWrite
-	}
-
-	if err == nil {
-		fmt.Fprintln(os.Stderr, "Authentication complete. Press Enter to continue... ")
-		_ = waitForEnter(os.Stdin)
+	_, err = cfgFile.Write(yamlData)
+	if err != nil {
+		return nil, err
 	}
 
 	// TODO cleaner error handling? this "should" always work given that we /just/ wrote the file...
