From 2ca18e0600223d8e2e8588a75cf693d17598fe43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 13 Oct 2021 23:24:14 +0200 Subject: [PATCH] Warn about missing OAuth scopes when reporting HTTP 4xx errors If a 4xx server response lists scopes in the X-Accepted-Oauth-Scopes header that are not present in the X-Oauth-Scopes header, the final error messaging on stderr will now include a hint for the user that they might need to request the additional scope: $ gh codespace list error getting codespaces: HTTP 403: Must have admin rights to Repository. (https://api.github.com/user/codespaces?per_page=30) This API operation needs the "codespace" scope. To request it, run: gh auth refresh -h github.com -s codespace --- api/client.go | 72 ++++++++++++++++++++++++++---- api/client_test.go | 64 ++++++++++++++++++++++++++ cmd/gh/main.go | 2 + pkg/cmd/api/api.go | 10 +++-- pkg/cmd/gist/create/create.go | 45 ++++++++++++------- pkg/cmd/gist/create/create_test.go | 29 ------------ pkg/httpmock/stub.go | 1 + 7 files changed, 165 insertions(+), 58 deletions(-) diff --git a/api/client.go b/api/client.go index a5741a42e..e3e48f57d 100644 --- a/api/client.go +++ b/api/client.go @@ -142,11 +142,12 @@ func (gr GraphQLErrorResponse) Error() string { // HTTPError is an error returned by a failed API call type HTTPError struct { - StatusCode int - RequestURL *url.URL - Message string - OAuthScopes string - Errors []HTTPErrorItem + StatusCode int + RequestURL *url.URL + Message string + Errors []HTTPErrorItem + + scopesSuggestion string } type HTTPErrorItem struct { @@ -165,6 +166,61 @@ func (err HTTPError) Error() string { return fmt.Sprintf("HTTP %d (%s)", err.StatusCode, err.RequestURL) } +func (err HTTPError) ScopesSuggestion() string { + return err.scopesSuggestion +} + +// ScopesSuggestion is an error messaging utility that prints the suggestion to request additional OAuth +// scopes in case a server response indicates that there are missing scopes. +func ScopesSuggestion(resp *http.Response) string { + if resp.StatusCode < 400 || resp.StatusCode > 499 { + return "" + } + + endpointNeedsScopes := resp.Header.Get("X-Accepted-Oauth-Scopes") + tokenHasScopes := resp.Header.Get("X-Oauth-Scopes") + if tokenHasScopes == "" { + return "" + } + + gotScopes := map[string]struct{}{} + for _, s := range strings.Split(tokenHasScopes, ",") { + s = strings.TrimSpace(s) + gotScopes[s] = struct{}{} + if strings.HasPrefix(s, "admin:") { + gotScopes["read:"+strings.TrimPrefix(s, "admin:")] = struct{}{} + gotScopes["write:"+strings.TrimPrefix(s, "admin:")] = struct{}{} + } else if strings.HasPrefix(s, "write:") { + gotScopes["read:"+strings.TrimPrefix(s, "write:")] = struct{}{} + } + } + + for _, s := range strings.Split(endpointNeedsScopes, ",") { + s = strings.TrimSpace(s) + if _, gotScope := gotScopes[s]; s == "" || gotScope { + continue + } + return fmt.Sprintf( + "This API operation needs the %[1]q scope. To request it, run: gh auth refresh -h %[2]s -s %[1]s", + s, + ghinstance.NormalizeHostname(resp.Request.URL.Hostname()), + ) + } + + return "" +} + +// EndpointNeedsScopes adds additional OAuth scopes to an HTTP response as if they were returned from the +// server endpoint. This improves HTTP 4xx error messaging for endpoints that don't explicitly list the +// OAuth scopes they need. +func EndpointNeedsScopes(resp *http.Response, s string) *http.Response { + if resp.StatusCode >= 400 && resp.StatusCode < 500 { + oldScopes := resp.Header.Get("X-Accepted-Oauth-Scopes") + resp.Header.Set("X-Accepted-Oauth-Scopes", fmt.Sprintf("%s, %s", oldScopes, s)) + } + return resp +} + // GraphQL performs a GraphQL request and parses the response func (c Client) GraphQL(hostname string, query string, variables map[string]interface{}, data interface{}) error { reqBody, err := json.Marshal(map[string]interface{}{"query": query, "variables": variables}) @@ -261,9 +317,9 @@ func handleResponse(resp *http.Response, data interface{}) error { func HandleHTTPError(resp *http.Response) error { httpError := HTTPError{ - StatusCode: resp.StatusCode, - RequestURL: resp.Request.URL, - OAuthScopes: resp.Header.Get("X-Oauth-Scopes"), + StatusCode: resp.StatusCode, + RequestURL: resp.Request.URL, + scopesSuggestion: ScopesSuggestion(resp), } if !jsonTypeRE.MatchString(resp.Header.Get("Content-Type")) { diff --git a/api/client_test.go b/api/client_test.go index 50665fddb..c7848d242 100644 --- a/api/client_test.go +++ b/api/client_test.go @@ -146,3 +146,67 @@ func TestHandleHTTPError_GraphQL502(t *testing.T) { t.Errorf("got error: %v", err) } } + +func TestHTTPError_ScopesSuggestion(t *testing.T) { + makeResponse := func(s int, u, haveScopes, needScopes string) *http.Response { + req, err := http.NewRequest("GET", u, nil) + if err != nil { + t.Fatal(err) + } + return &http.Response{ + Request: req, + StatusCode: s, + Body: ioutil.NopCloser(bytes.NewBufferString(`{}`)), + Header: map[string][]string{ + "Content-Type": {"application/json"}, + "X-Oauth-Scopes": {haveScopes}, + "X-Accepted-Oauth-Scopes": {needScopes}, + }, + } + } + + tests := []struct { + name string + resp *http.Response + want string + }{ + { + name: "has necessary scopes", + resp: makeResponse(404, "https://api.github.com/gists", "repo, gist, read:org", "gist"), + want: ``, + }, + { + name: "normalizes scopes", + resp: makeResponse(404, "https://api.github.com/orgs/ORG/discussions", "admin:org, write:discussion", "read:org, read:discussion"), + want: ``, + }, + { + name: "no scopes on endpoint", + resp: makeResponse(404, "https://api.github.com/user", "repo", ""), + want: ``, + }, + { + name: "missing a scope", + resp: makeResponse(404, "https://api.github.com/gists", "repo, read:org", "gist, delete_repo"), + want: `This API operation needs the "gist" scope. To request it, run: gh auth refresh -h github.com -s gist`, + }, + { + name: "server error", + resp: makeResponse(500, "https://api.github.com/gists", "repo", "gist"), + want: ``, + }, + { + name: "no scopes on token", + resp: makeResponse(404, "https://api.github.com/gists", "", "gist, delete_repo"), + want: ``, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + httpError := HandleHTTPError(tt.resp) + if got := httpError.(HTTPError).ScopesSuggestion(); got != tt.want { + t.Errorf("HTTPError.ScopesSuggestion() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/cmd/gh/main.go b/cmd/gh/main.go index 218f3ed22..50f8335a3 100644 --- a/cmd/gh/main.go +++ b/cmd/gh/main.go @@ -226,6 +226,8 @@ func mainRun() exitCode { fmt.Fprintln(stderr, "Try authenticating with: gh auth login") } else if strings.Contains(err.Error(), "Resource protected by organization SAML enforcement") { fmt.Fprintln(stderr, "Try re-authenticating with: gh auth refresh") + } else if msg := httpErr.ScopesSuggestion(); msg != "" { + fmt.Fprintln(stderr, msg) } return exitError diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index 22b42f33d..7d9faf9b6 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -384,12 +384,14 @@ func processResponse(resp *http.Response, opts *ApiOptions, headersOutputStream } } + if serverError == "" && resp.StatusCode > 299 { + serverError = fmt.Sprintf("HTTP %d", resp.StatusCode) + } if serverError != "" { fmt.Fprintf(opts.IO.ErrOut, "gh: %s\n", serverError) - err = cmdutil.SilentError - return - } else if resp.StatusCode > 299 { - fmt.Fprintf(opts.IO.ErrOut, "gh: HTTP %d\n", resp.StatusCode) + if msg := api.ScopesSuggestion(resp); msg != "" { + fmt.Fprintf(opts.IO.ErrOut, "gh: %s\n", msg) + } err = cmdutil.SilentError return } diff --git a/pkg/cmd/gist/create/create.go b/pkg/cmd/gist/create/create.go index 9ead354f4..098e13f0f 100644 --- a/pkg/cmd/gist/create/create.go +++ b/pkg/cmd/gist/create/create.go @@ -16,6 +16,7 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/config" + "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/pkg/cmd/gist/shared" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" @@ -150,9 +151,6 @@ func createRun(opts *CreateOptions) error { if err != nil { var httpError api.HTTPError if errors.As(err, &httpError) { - if httpError.OAuthScopes != "" && !strings.Contains(httpError.OAuthScopes, "gist") { - return fmt.Errorf("This command requires the 'gist' OAuth scope.\nPlease re-authenticate with: gh auth refresh -h %s -s gist", host) - } if httpError.StatusCode == http.StatusUnprocessableEntity { if detectEmptyFiles(files) { fmt.Fprintf(errOut, "%s Failed to create gist: %s\n", cs.FailureIcon(), "a gist file cannot be blank") @@ -248,29 +246,42 @@ func guessGistName(files map[string]*shared.GistFile) string { } func createGist(client *http.Client, hostname, description string, public bool, files map[string]*shared.GistFile) (*shared.Gist, error) { - path := "gists" - body := &shared.Gist{ Description: description, Public: public, Files: files, } - result := shared.Gist{} - - requestByte, err := json.Marshal(body) - if err != nil { - return nil, err - } - requestBody := bytes.NewReader(requestByte) - - apiClient := api.NewClientFromHTTP(client) - err = apiClient.REST(hostname, "POST", path, requestBody, &result) - if err != nil { + requestBody := &bytes.Buffer{} + enc := json.NewEncoder(requestBody) + if err := enc.Encode(body); err != nil { return nil, err } - return &result, nil + u := ghinstance.RESTPrefix(hostname) + "gists" + req, err := http.NewRequest(http.MethodPost, u, requestBody) + if err != nil { + return nil, err + } + req.Header.Set("Content-Type", "application/json; charset=utf-8") + + resp, err := client.Do(req) + if err != nil { + return nil, err + } + defer resp.Body.Close() + + if resp.StatusCode > 299 { + return nil, api.HandleHTTPError(api.EndpointNeedsScopes(resp, "gist")) + } + + result := &shared.Gist{} + dec := json.NewDecoder(resp.Body) + if err := dec.Decode(result); err != nil { + return nil, err + } + + return result, nil } func detectEmptyFiles(files map[string]*shared.GistFile) bool { diff --git a/pkg/cmd/gist/create/create_test.go b/pkg/cmd/gist/create/create_test.go index bbcc42707..450e20f99 100644 --- a/pkg/cmd/gist/create/create_test.go +++ b/pkg/cmd/gist/create/create_test.go @@ -388,32 +388,3 @@ func Test_detectEmptyFiles(t *testing.T) { assert.Equal(t, tt.isEmptyFile, isEmptyFile) } } - -func Test_CreateRun_reauth(t *testing.T) { - reg := &httpmock.Registry{} - reg.Register(httpmock.REST("POST", "gists"), func(req *http.Request) (*http.Response, error) { - return &http.Response{ - StatusCode: 404, - Request: req, - Header: map[string][]string{ - "X-Oauth-Scopes": {"repo, read:org"}, - }, - Body: ioutil.NopCloser(bytes.NewBufferString("oh no")), - }, nil - }) - - io, _, _, _ := iostreams.Test() - - opts := &CreateOptions{ - IO: io, - HttpClient: func() (*http.Client, error) { - return &http.Client{Transport: reg}, nil - }, - Config: func() (config.Config, error) { - return config.NewBlankConfig(), nil - }, - } - - err := createRun(opts) - assert.EqualError(t, err, "This command requires the 'gist' OAuth scope.\nPlease re-authenticate with: gh auth refresh -h github.com -s gist") -} diff --git a/pkg/httpmock/stub.go b/pkg/httpmock/stub.go index 2c17c7349..98edbcb58 100644 --- a/pkg/httpmock/stub.go +++ b/pkg/httpmock/stub.go @@ -162,5 +162,6 @@ func httpResponse(status int, req *http.Request, body io.Reader) *http.Response StatusCode: status, Request: req, Body: ioutil.NopCloser(body), + Header: http.Header{}, } }