diff --git a/api/client.go b/api/client.go index d4a32d87d..09195181b 100644 --- a/api/client.go +++ b/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 } diff --git a/api/client_test.go b/api/client_test.go index d2925bd20..35a45af8c 100644 --- a/api/client_test.go +++ b/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) } }) } + } diff --git a/cmd/gh/main.go b/cmd/gh/main.go index 38ca1db84..728024079 100644 --- a/cmd/gh/main.go +++ b/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) } diff --git a/pkg/cmd/auth/status/status.go b/pkg/cmd/auth/status/status.go index 442b84afb..d5be70441 100644 --- a/pkg/cmd/auth/status/status.go +++ b/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"),