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 1/5] 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{}, } } From 2c3f02ee62fe18e6472d6ff159ef24a4d42c1309 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 14 Oct 2021 17:30:05 +0200 Subject: [PATCH 2/5] Ensure NOT_FOUND error when querying private repos using insufficient scope --- api/queries_repo.go | 37 ++++++++++++++++++++++++++++--------- api/queries_repo_test.go | 21 +++++++++++++++++++++ 2 files changed, 49 insertions(+), 9 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index c9d8b46f9..8832aae7d 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -241,12 +241,23 @@ func FetchRepository(client *Client, repo ghrepo.Interface, fields []string) (*R } var result struct { - Repository Repository + Repository *Repository } if err := client.GraphQL(repo.RepoHost(), query, variables, &result); err != nil { return nil, err } - return InitRepoHostname(&result.Repository, repo.RepoHost()), nil + // The GraphQL API should have returned an error in case of a missing repository, but this isn't + // guaranteed to happen when an authentication token with insufficient permissions is being used. + if result.Repository == nil { + return nil, GraphQLErrorResponse{ + Errors: []GraphQLError{{ + Type: "NOT_FOUND", + Message: fmt.Sprintf("Could not resolve to a Repository with the name '%s/%s'.", repo.RepoOwner(), repo.RepoName()), + }}, + } + } + + return InitRepoHostname(result.Repository, repo.RepoHost()), nil } func GitHubRepo(client *Client, repo ghrepo.Interface) (*Repository, error) { @@ -280,16 +291,24 @@ func GitHubRepo(client *Client, repo ghrepo.Interface) (*Repository, error) { "name": repo.RepoName(), } - result := struct { - Repository Repository - }{} - err := client.GraphQL(repo.RepoHost(), query, variables, &result) - - if err != nil { + var result struct { + Repository *Repository + } + if err := client.GraphQL(repo.RepoHost(), query, variables, &result); err != nil { return nil, err } + // The GraphQL API should have returned an error in case of a missing repository, but this isn't + // guaranteed to happen when an authentication token with insufficient permissions is being used. + if result.Repository == nil { + return nil, GraphQLErrorResponse{ + Errors: []GraphQLError{{ + Type: "NOT_FOUND", + Message: fmt.Sprintf("Could not resolve to a Repository with the name '%s/%s'.", repo.RepoOwner(), repo.RepoName()), + }}, + } + } - return InitRepoHostname(&result.Repository, repo.RepoHost()), nil + return InitRepoHostname(result.Repository, repo.RepoHost()), nil } func RepoDefaultBranch(client *Client, repo ghrepo.Interface) (string, error) { diff --git a/api/queries_repo_test.go b/api/queries_repo_test.go index 5fadf7cfc..8846e16cc 100644 --- a/api/queries_repo_test.go +++ b/api/queries_repo_test.go @@ -10,6 +10,27 @@ import ( "github.com/cli/cli/v2/pkg/httpmock" ) +func TestGitHubRepo_notFound(t *testing.T) { + httpReg := &httpmock.Registry{} + defer httpReg.Verify(t) + + httpReg.Register( + httpmock.GraphQL(`query RepositoryInfo\b`), + httpmock.StringResponse(`{ "data": { "repository": null } }`)) + + client := NewClient(ReplaceTripper(httpReg)) + repo, err := GitHubRepo(client, ghrepo.New("OWNER", "REPO")) + if err == nil { + t.Fatal("GitHubRepo did not return an error") + } + if wants := "GraphQL error: Could not resolve to a Repository with the name 'OWNER/REPO'."; err.Error() != wants { + t.Errorf("GitHubRepo error: want %q, got %q", wants, err.Error()) + } + if repo != nil { + t.Errorf("GitHubRepo: expected nil repo, got %v", repo) + } +} + func Test_RepoMetadata(t *testing.T) { http := &httpmock.Registry{} client := NewClient(ReplaceTripper(http)) From 693193fe847879d09e60525fc1e180eb94587025 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 14 Oct 2021 18:16:04 +0200 Subject: [PATCH 3/5] Consistent error handling in codespaces API operations Using HandleHTTPError ensures that hints regarding insufficient OAuth scopes will be properly reported on stderr. --- internal/codespaces/api/api.go | 103 +++++++++++---------------------- 1 file changed, 35 insertions(+), 68 deletions(-) diff --git a/internal/codespaces/api/api.go b/internal/codespaces/api/api.go index 1087e5828..35d823b3d 100644 --- a/internal/codespaces/api/api.go +++ b/internal/codespaces/api/api.go @@ -85,15 +85,15 @@ func (a *API) GetUser(ctx context.Context) (*User, error) { } defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return nil, api.HandleHTTPError(resp) + } + b, err := ioutil.ReadAll(resp.Body) if err != nil { return nil, fmt.Errorf("error reading response body: %w", err) } - if resp.StatusCode != http.StatusOK { - return nil, jsonErrorResponse(b) - } - var response User if err := json.Unmarshal(b, &response); err != nil { return nil, fmt.Errorf("error unmarshaling response: %w", err) @@ -102,18 +102,6 @@ func (a *API) GetUser(ctx context.Context) (*User, error) { return &response, nil } -// jsonErrorResponse returns the error message from a JSON response. -func jsonErrorResponse(b []byte) error { - var response struct { - Message string `json:"message"` - } - if err := json.Unmarshal(b, &response); err != nil { - return fmt.Errorf("error unmarshaling error response: %w", err) - } - - return errors.New(response.Message) -} - // Repository represents a GitHub repository. type Repository struct { ID int `json:"id"` @@ -133,15 +121,15 @@ func (a *API) GetRepository(ctx context.Context, nwo string) (*Repository, error } defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return nil, api.HandleHTTPError(resp) + } + b, err := ioutil.ReadAll(resp.Body) if err != nil { return nil, fmt.Errorf("error reading response body: %w", err) } - if resp.StatusCode != http.StatusOK { - return nil, jsonErrorResponse(b) - } - var response Repository if err := json.Unmarshal(b, &response); err != nil { return nil, fmt.Errorf("error unmarshaling response: %w", err) @@ -286,15 +274,15 @@ func (a *API) GetCodespace(ctx context.Context, codespaceName string, includeCon } defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return nil, api.HandleHTTPError(resp) + } + b, err := ioutil.ReadAll(resp.Body) if err != nil { return nil, fmt.Errorf("error reading response body: %w", err) } - if resp.StatusCode != http.StatusOK { - return nil, jsonErrorResponse(b) - } - var response Codespace if err := json.Unmarshal(b, &response); err != nil { return nil, fmt.Errorf("error unmarshaling response: %w", err) @@ -322,26 +310,12 @@ func (a *API) StartCodespace(ctx context.Context, codespaceName string) error { } defer resp.Body.Close() - b, err := ioutil.ReadAll(resp.Body) - if err != nil { - return fmt.Errorf("error reading response body: %w", err) - } - if resp.StatusCode != http.StatusOK { if resp.StatusCode == http.StatusConflict { // 409 means the codespace is already running which we can safely ignore return nil } - - // Error response may be a numeric code or a JSON {"message": "..."}. - if bytes.HasPrefix(b, []byte("{")) { - return jsonErrorResponse(b) // probably JSON - } - - if len(b) > 100 { - b = append(b[:97], "..."...) - } - return fmt.Errorf("failed to start codespace: %s (%s)", b, resp.Status) + return api.HandleHTTPError(resp) } return nil @@ -364,15 +338,15 @@ func (a *API) GetCodespaceRegionLocation(ctx context.Context) (string, error) { } defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return "", api.HandleHTTPError(resp) + } + b, err := ioutil.ReadAll(resp.Body) if err != nil { return "", fmt.Errorf("error reading response body: %w", err) } - if resp.StatusCode != http.StatusOK { - return "", jsonErrorResponse(b) - } - var response getCodespaceRegionLocationResponse if err := json.Unmarshal(b, &response); err != nil { return "", fmt.Errorf("error unmarshaling response: %w", err) @@ -406,15 +380,15 @@ func (a *API) GetCodespacesMachines(ctx context.Context, repoID int, branch, loc } defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return nil, api.HandleHTTPError(resp) + } + b, err := ioutil.ReadAll(resp.Body) if err != nil { return nil, fmt.Errorf("error reading response body: %w", err) } - if resp.StatusCode != http.StatusOK { - return nil, jsonErrorResponse(b) - } - var response struct { Machines []*Machine `json:"machines"` } @@ -499,18 +473,17 @@ func (a *API) startCreate(ctx context.Context, repoID int, machine, branch, loca } defer resp.Body.Close() + if resp.StatusCode == http.StatusAccepted { + return nil, errProvisioningInProgress // RPC finished before result of creation known + } else if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated { + return nil, api.HandleHTTPError(resp) + } + b, err := ioutil.ReadAll(resp.Body) if err != nil { return nil, fmt.Errorf("error reading response body: %w", err) } - switch { - case resp.StatusCode > http.StatusAccepted: - return nil, jsonErrorResponse(b) - case resp.StatusCode == http.StatusAccepted: - return nil, errProvisioningInProgress // RPC finished before result of creation known - } - var response Codespace if err := json.Unmarshal(b, &response); err != nil { return nil, fmt.Errorf("error unmarshaling response: %w", err) @@ -533,12 +506,8 @@ func (a *API) DeleteCodespace(ctx context.Context, codespaceName string) error { } defer resp.Body.Close() - if resp.StatusCode > http.StatusAccepted { - b, err := ioutil.ReadAll(resp.Body) - if err != nil { - return fmt.Errorf("error reading response body: %w", err) - } - return jsonErrorResponse(b) + if resp.StatusCode != http.StatusOK { + return api.HandleHTTPError(resp) } return nil @@ -567,6 +536,8 @@ func (a *API) GetCodespaceRepositoryContents(ctx context.Context, codespace *Cod if resp.StatusCode == http.StatusNotFound { return nil, nil + } else if resp.StatusCode != http.StatusOK { + return nil, api.HandleHTTPError(resp) } b, err := ioutil.ReadAll(resp.Body) @@ -574,10 +545,6 @@ func (a *API) GetCodespaceRepositoryContents(ctx context.Context, codespace *Cod return nil, fmt.Errorf("error reading response body: %w", err) } - if resp.StatusCode != http.StatusOK { - return nil, jsonErrorResponse(b) - } - var response getCodespaceRepositoryContentsResponse if err := json.Unmarshal(b, &response); err != nil { return nil, fmt.Errorf("error unmarshaling response: %w", err) @@ -605,14 +572,14 @@ func (a *API) AuthorizedKeys(ctx context.Context, user string) ([]byte, error) { } defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("server returned %s", resp.Status) + } + b, err := ioutil.ReadAll(resp.Body) if err != nil { return nil, fmt.Errorf("error reading response body: %w", err) } - - if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("server returned %s", resp.Status) - } return b, nil } From 64a19ee71feb0b5d1024227ff87e4104e36e896f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 14 Oct 2021 18:36:55 +0200 Subject: [PATCH 4/5] Remove OAuth scopes checking logic from `ssh-key` commands Scopes checking is now handled on the HTTP client level for all commands. --- pkg/cmd/ssh-key/add/add.go | 6 ------ pkg/cmd/ssh-key/add/http.go | 6 +----- pkg/cmd/ssh-key/list/http.go | 7 +------ pkg/cmd/ssh-key/list/list.go | 7 ------- 4 files changed, 2 insertions(+), 24 deletions(-) diff --git a/pkg/cmd/ssh-key/add/add.go b/pkg/cmd/ssh-key/add/add.go index 72da863de..53759acfb 100644 --- a/pkg/cmd/ssh-key/add/add.go +++ b/pkg/cmd/ssh-key/add/add.go @@ -85,12 +85,6 @@ func runAdd(opts *AddOptions) error { err = SSHKeyUpload(httpClient, hostname, keyReader, opts.Title) if err != nil { - if errors.Is(err, scopesError) { - cs := opts.IO.ColorScheme() - fmt.Fprint(opts.IO.ErrOut, "Error: insufficient OAuth scopes to list SSH keys\n") - fmt.Fprintf(opts.IO.ErrOut, "Run the following to grant scopes: %s\n", cs.Bold("gh auth refresh -s write:public_key")) - return cmdutil.SilentError - } return err } diff --git a/pkg/cmd/ssh-key/add/http.go b/pkg/cmd/ssh-key/add/http.go index c85b00c2a..28a70acc8 100644 --- a/pkg/cmd/ssh-key/add/http.go +++ b/pkg/cmd/ssh-key/add/http.go @@ -12,8 +12,6 @@ import ( "github.com/cli/cli/v2/internal/ghinstance" ) -var scopesError = errors.New("insufficient OAuth scopes") - func SSHKeyUpload(httpClient *http.Client, hostname string, keyFile io.Reader, title string) error { url := ghinstance.RESTPrefix(hostname) + "user/keys" @@ -43,9 +41,7 @@ func SSHKeyUpload(httpClient *http.Client, hostname string, keyFile io.Reader, t } defer resp.Body.Close() - if resp.StatusCode == 404 { - return scopesError - } else if resp.StatusCode > 299 { + if resp.StatusCode > 299 { var httpError api.HTTPError err := api.HandleHTTPError(resp) if errors.As(err, &httpError) && isDuplicateError(&httpError) { diff --git a/pkg/cmd/ssh-key/list/http.go b/pkg/cmd/ssh-key/list/http.go index 9a6d6fc0e..539e55bac 100644 --- a/pkg/cmd/ssh-key/list/http.go +++ b/pkg/cmd/ssh-key/list/http.go @@ -2,7 +2,6 @@ package list import ( "encoding/json" - "errors" "fmt" "io/ioutil" "net/http" @@ -12,8 +11,6 @@ import ( "github.com/cli/cli/v2/internal/ghinstance" ) -var scopesError = errors.New("insufficient OAuth scopes") - type sshKey struct { Key string Title string @@ -37,9 +34,7 @@ func userKeys(httpClient *http.Client, host, userHandle string) ([]sshKey, error } defer resp.Body.Close() - if resp.StatusCode == 404 { - return nil, scopesError - } else if resp.StatusCode > 299 { + if resp.StatusCode > 299 { return nil, api.HandleHTTPError(resp) } diff --git a/pkg/cmd/ssh-key/list/list.go b/pkg/cmd/ssh-key/list/list.go index fc35cf650..e743f2818 100644 --- a/pkg/cmd/ssh-key/list/list.go +++ b/pkg/cmd/ssh-key/list/list.go @@ -1,7 +1,6 @@ package list import ( - "errors" "fmt" "net/http" "time" @@ -59,12 +58,6 @@ func listRun(opts *ListOptions) error { sshKeys, err := userKeys(apiClient, host, "") if err != nil { - if errors.Is(err, scopesError) { - cs := opts.IO.ColorScheme() - fmt.Fprint(opts.IO.ErrOut, "Error: insufficient OAuth scopes to list SSH keys\n") - fmt.Fprintf(opts.IO.ErrOut, "Run the following to grant scopes: %s\n", cs.Bold("gh auth refresh -s read:public_key")) - return cmdutil.SilentError - } return err } From 89ad87019043642fc8ef144fa51eb9b8e0601a5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 14 Oct 2021 19:52:59 +0200 Subject: [PATCH 5/5] auth refresh: preserve existing scopes when requesting new ones When there was a previously valid token that was granted some scopes, ensure all those scopes will be re-requested when doing the authentication flow for the new token. --- pkg/cmd/auth/refresh/refresh.go | 18 +++++++++-- pkg/cmd/auth/refresh/refresh_test.go | 46 ++++++++++++++++++++++++---- pkg/cmd/auth/shared/oauth_scopes.go | 18 ++++++++--- 3 files changed, 69 insertions(+), 13 deletions(-) diff --git a/pkg/cmd/auth/refresh/refresh.go b/pkg/cmd/auth/refresh/refresh.go index 6244fabf5..4137341a9 100644 --- a/pkg/cmd/auth/refresh/refresh.go +++ b/pkg/cmd/auth/refresh/refresh.go @@ -3,6 +3,8 @@ package refresh import ( "errors" "fmt" + "net/http" + "strings" "github.com/AlecAivazis/survey/v2" "github.com/MakeNowJust/heredoc" @@ -16,8 +18,9 @@ import ( ) type RefreshOptions struct { - IO *iostreams.IOStreams - Config func() (config.Config, error) + IO *iostreams.IOStreams + Config func() (config.Config, error) + httpClient *http.Client MainExecutable string @@ -36,6 +39,7 @@ func NewCmdRefresh(f *cmdutil.Factory, runF func(*RefreshOptions) error) *cobra. _, err := authflow.AuthFlowWithConfig(cfg, io, hostname, "", scopes) return err }, + httpClient: http.DefaultClient, } cmd := &cobra.Command{ @@ -128,6 +132,16 @@ func refreshRun(opts *RefreshOptions) error { } var additionalScopes []string + if oldToken, _ := cfg.Get(hostname, "oauth_token"); oldToken != "" { + if oldScopes, err := shared.GetScopes(opts.httpClient, hostname, oldToken); err == nil { + for _, s := range strings.Split(oldScopes, ",") { + s = strings.TrimSpace(s) + if s != "" { + additionalScopes = append(additionalScopes, s) + } + } + } + } credentialFlow := &shared.GitCredentialFlow{ Executable: opts.MainExecutable, diff --git a/pkg/cmd/auth/refresh/refresh_test.go b/pkg/cmd/auth/refresh/refresh_test.go index 943864424..dbdae26af 100644 --- a/pkg/cmd/auth/refresh/refresh_test.go +++ b/pkg/cmd/auth/refresh/refresh_test.go @@ -2,6 +2,9 @@ package refresh import ( "bytes" + "io/ioutil" + "net/http" + "strings" "testing" "github.com/cli/cli/v2/internal/config" @@ -134,6 +137,7 @@ func Test_refreshRun(t *testing.T) { opts *RefreshOptions askStubs func(*prompt.AskStubber) cfgHosts []string + oldScopes string wantErr string nontty bool wantAuthArgs authArgs @@ -211,6 +215,20 @@ func Test_refreshRun(t *testing.T) { scopes: []string{"repo:invite", "public_key:read"}, }, }, + { + name: "scopes provided", + cfgHosts: []string{ + "github.com", + }, + oldScopes: "delete_repo, codespace", + opts: &RefreshOptions{ + Scopes: []string{"repo:invite", "public_key:read"}, + }, + wantAuthArgs: authArgs{ + hostname: "github.com", + scopes: []string{"repo:invite", "public_key:read", "delete_repo", "codespace"}, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -234,10 +252,26 @@ func Test_refreshRun(t *testing.T) { for _, hostname := range tt.cfgHosts { _ = cfg.Set(hostname, "oauth_token", "abc123") } - reg := &httpmock.Registry{} - reg.Register( - httpmock.GraphQL(`query UserCurrent\b`), - httpmock.StringResponse(`{"data":{"viewer":{"login":"cybilb"}}}`)) + + httpReg := &httpmock.Registry{} + httpReg.Register( + httpmock.REST("GET", ""), + func(req *http.Request) (*http.Response, error) { + statusCode := 200 + if req.Header.Get("Authorization") != "token abc123" { + statusCode = 400 + } + return &http.Response{ + Request: req, + StatusCode: statusCode, + Body: ioutil.NopCloser(strings.NewReader(``)), + Header: http.Header{ + "X-Oauth-Scopes": {tt.oldScopes}, + }, + }, nil + }, + ) + tt.opts.httpClient = &http.Client{Transport: httpReg} mainBuf := bytes.Buffer{} hostsBuf := bytes.Buffer{} @@ -258,8 +292,8 @@ func Test_refreshRun(t *testing.T) { assert.NoError(t, err) } - assert.Equal(t, aa.hostname, tt.wantAuthArgs.hostname) - assert.Equal(t, aa.scopes, tt.wantAuthArgs.scopes) + assert.Equal(t, tt.wantAuthArgs.hostname, aa.hostname) + assert.Equal(t, tt.wantAuthArgs.scopes, aa.scopes) }) } } diff --git a/pkg/cmd/auth/shared/oauth_scopes.go b/pkg/cmd/auth/shared/oauth_scopes.go index 35619ea7b..c076722b2 100644 --- a/pkg/cmd/auth/shared/oauth_scopes.go +++ b/pkg/cmd/auth/shared/oauth_scopes.go @@ -32,19 +32,19 @@ type httpClient interface { Do(*http.Request) (*http.Response, error) } -func HasMinimumScopes(httpClient httpClient, hostname, authToken string) error { +func GetScopes(httpClient httpClient, hostname, authToken string) (string, error) { apiEndpoint := ghinstance.RESTPrefix(hostname) req, err := http.NewRequest("GET", apiEndpoint, nil) if err != nil { - return err + return "", err } req.Header.Set("Authorization", "token "+authToken) res, err := httpClient.Do(req) if err != nil { - return err + return "", err } defer func() { @@ -55,10 +55,18 @@ func HasMinimumScopes(httpClient httpClient, hostname, authToken string) error { }() if res.StatusCode != 200 { - return api.HandleHTTPError(res) + return "", api.HandleHTTPError(res) + } + + return res.Header.Get("X-Oauth-Scopes"), nil +} + +func HasMinimumScopes(httpClient httpClient, hostname, authToken string) error { + scopesHeader, err := GetScopes(httpClient, hostname, authToken) + if err != nil { + return err } - 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