Merge pull request #2207 from cli/codespaces

[Codespaces] Support "integration" tokens

api/client.go

@@ -3,7 +3,6 @@ package api
 import (
 	"bytes"
 	"encoding/json"
-	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -99,58 +98,6 @@ func ReplaceTripper(tr http.RoundTripper) ClientOption {
 	}
 }
 
-var issuedScopesWarning bool
-
-const (
-	httpOAuthAppID  = "X-Oauth-Client-Id"
-	httpOAuthScopes = "X-Oauth-Scopes"
-)
-
-// CheckScopes checks whether an OAuth scope is present in a response
-func CheckScopes(wantedScope string, cb func(string) error) ClientOption {
-	wantedCandidates := []string{wantedScope}
-	if strings.HasPrefix(wantedScope, "read:") {
-		wantedCandidates = append(wantedCandidates, "admin:"+strings.TrimPrefix(wantedScope, "read:"))
-	}
-
-	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
-			}
-
-			_, hasHeader := res.Header[httpOAuthAppID]
-			if !hasHeader {
-				return res, nil
-			}
-
-			appID := res.Header.Get(httpOAuthAppID)
-			hasScopes := strings.Split(res.Header.Get(httpOAuthScopes), ",")
-
-			hasWanted := false
-		outer:
-			for _, s := range hasScopes {
-				for _, w := range wantedCandidates {
-					if w == strings.TrimSpace(s) {
-						hasWanted = true
-						break outer
-					}
-				}
-			}
-
-			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)
 }
@@ -207,7 +154,20 @@ func (err HTTPError) Error() string {
 }
 
 type MissingScopesError struct {
-	error
+	MissingScopes []string
+}
+
+func (e MissingScopesError) Error() string {
+	var missing []string
+	for _, s := range e.MissingScopes {
+		missing = append(missing, fmt.Sprintf("'%s'", s))
+	}
+	scopes := strings.Join(missing, ", ")
+
+	if len(e.MissingScopes) == 1 {
+		return "missing required scope " + scopes
+	}
+	return "missing required scopes " + scopes
 }
 
 func (c Client) HasMinimumScopes(hostname string) error {
@@ -235,31 +195,34 @@ func (c Client) HasMinimumScopes(hostname string) error {
 		return HandleHTTPError(res)
 	}
 
-	hasScopes := strings.Split(res.Header.Get("X-Oauth-Scopes"), ",")
+	scopesHeader := res.Header.Get("X-Oauth-Scopes")
+	if scopesHeader == "" {
+		// if the token reports no scopes, assume that it's an integration token and give up on
+		// detecting its capabilities
+		return nil
+	}
 
 	search := map[string]bool{
 		"repo":      false,
 		"read:org":  false,
 		"admin:org": false,
 	}
-
-	for _, s := range hasScopes {
+	for _, s := range strings.Split(scopesHeader, ",") {
 		search[strings.TrimSpace(s)] = true
 	}
 
-	errorMsgs := []string{}
+	var missingScopes []string
 	if !search["repo"] {
-		errorMsgs = append(errorMsgs, "missing required scope 'repo'")
+		missingScopes = append(missingScopes, "repo")
 	}
 
 	if !search["read:org"] && !search["admin:org"] {
-		errorMsgs = append(errorMsgs, "missing required scope 'read:org'")
+		missingScopes = append(missingScopes, "read:org")
 	}
 
-	if len(errorMsgs) > 0 {
-		return &MissingScopesError{error: errors.New(strings.Join(errorMsgs, ";"))}
+	if len(missingScopes) > 0 {
+		return &MissingScopesError{MissingScopes: missingScopes}
 	}
-
 	return nil
 }
 

api/client_test.go

@@ -105,97 +105,66 @@ func TestRESTError(t *testing.T) {
 	}
 }
 
-func Test_CheckScopes(t *testing.T) {
+func Test_HasMinimumScopes(t *testing.T) {
 	tests := []struct {
-		name           string
-		wantScope      string
-		responseApp    string
-		responseScopes string
-		responseError  error
-		expectCallback bool
+		name    string
+		header  string
+		wantErr string
 	}{
 		{
-			name:           "missing read:org",
-			wantScope:      "read:org",
-			responseApp:    "APPID",
-			responseScopes: "repo, gist",
-			expectCallback: true,
+			name:    "no scopes",
+			header:  "",
+			wantErr: "",
 		},
 		{
-			name:           "has read:org",
-			wantScope:      "read:org",
-			responseApp:    "APPID",
-			responseScopes: "repo, read:org, gist",
-			expectCallback: false,
+			name:    "default scopes",
+			header:  "repo, read:org",
+			wantErr: "",
 		},
 		{
-			name:           "has admin:org",
-			wantScope:      "read:org",
-			responseApp:    "APPID",
-			responseScopes: "repo, admin:org, gist",
-			expectCallback: false,
+			name:    "admin:org satisfies read:org",
+			header:  "repo, admin:org",
+			wantErr: "",
 		},
 		{
-			name:           "no scopes in response",
-			wantScope:      "read:org",
-			responseApp:    "",
-			responseScopes: "",
-			expectCallback: false,
+			name:    "insufficient scope",
+			header:  "repo",
+			wantErr: "missing required scope 'read:org'",
 		},
 		{
-			name:           "errored response",
-			wantScope:      "read:org",
-			responseApp:    "",
-			responseScopes: "",
-			responseError:  errors.New("Network Failed"),
-			expectCallback: false,
+			name:    "insufficient scopes",
+			header:  "gist",
+			wantErr: "missing required scopes 'repo', 'read:org'",
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			tr := &httpmock.Registry{}
-			tr.Register(httpmock.MatchAny, func(*http.Request) (*http.Response, error) {
-				if tt.responseError != nil {
-					return nil, tt.responseError
-				}
-				if tt.responseScopes == "" {
-					return &http.Response{StatusCode: 200}, nil
-				}
+			fakehttp := &httpmock.Registry{}
+			client := NewClient(ReplaceTripper(fakehttp))
+
+			fakehttp.Register(httpmock.REST("GET", ""), func(req *http.Request) (*http.Response, error) {
 				return &http.Response{
+					Request:    req,
 					StatusCode: 200,
-					Header: http.Header{
-						"X-Oauth-Client-Id": []string{tt.responseApp},
-						"X-Oauth-Scopes":    []string{tt.responseScopes},
+					Body:       ioutil.NopCloser(&bytes.Buffer{}),
+					Header: map[string][]string{
+						"X-Oauth-Scopes": {tt.header},
 					},
 				}, nil
 			})
 
-			callbackInvoked := false
-			var gotAppID string
-			fn := CheckScopes(tt.wantScope, func(appID string) error {
-				callbackInvoked = true
-				gotAppID = appID
-				return nil
-			})
-
-			rt := fn(tr)
-			req, err := http.NewRequest("GET", "https://api.github.com/hello", nil)
-			if err != nil {
-				t.Fatalf("unexpected error: %v", err)
+			err := client.HasMinimumScopes("github.com")
+			if tt.wantErr == "" {
+				if err != nil {
+					t.Errorf("error: %v", err)
+				}
+				return
 			}
+			if err.Error() != tt.wantErr {
+				t.Errorf("want %q, got %q", tt.wantErr, err.Error())
 
-			issuedScopesWarning = false
-			_, err = rt.RoundTrip(req)
-			if err != nil && !errors.Is(err, tt.responseError) {
-				t.Fatalf("unexpected error: %v", err)
-			}
-
-			if tt.expectCallback != callbackInvoked {
-				t.Fatalf("expected CheckScopes callback: %v", tt.expectCallback)
-			}
-			if tt.expectCallback && gotAppID != tt.responseApp {
-				t.Errorf("unexpected app ID: %q", gotAppID)
 			}
 		})
 	}
+
 }

cmd/gh/main.go

@@ -191,6 +191,9 @@ func shouldCheckForUpdate() bool {
 	if os.Getenv("GH_NO_UPDATE_NOTIFIER") != "" {
 		return false
 	}
+	if os.Getenv("CODESPACES") != "" {
+		return false
+	}
 	return updaterEnabled != "" && !isCI() && !isCompletionCommand() && utils.IsTerminal(os.Stderr)
 }
 

pkg/cmd/auth/status/status.go

@@ -98,7 +98,7 @@ func statusRun(opts *StatusOptions) error {
 		if err != nil {
 			var missingScopes *api.MissingScopesError
 			if errors.As(err, &missingScopes) {
-				addMsg("%s %s: %s", utils.Red("X"), hostname, err)
+				addMsg("%s %s: the token in %s is %s", utils.Red("X"), hostname, tokenSource, err)
 				if tokenIsWriteable {
 					addMsg("- To request missing scopes, run: %s %s\n",
 						utils.Bold("gh auth refresh -h"),