Skip to content

Commit 7844396

Browse files
mislavmislav
authored
Merge pull request cli#4513 from cli/missing-oauth-scopes
Warn about missing OAuth scopes when reporting HTTP 4xx errors
2 parents 78ac771 + 89ad870 commit 7844396

17 files changed

Lines changed: 314 additions & 166 deletions

File tree

‎api/client.go‎

Lines changed: 64 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -142,11 +142,12 @@ func (gr GraphQLErrorResponse) Error() string {
142142

143143
// HTTPError is an error returned by a failed API call
144144
type HTTPError struct {
145-
StatusCode int
146-
RequestURL *url.URL
147-
Message string
148-
OAuthScopes string
149-
Errors []HTTPErrorItem
145+
StatusCode int
146+
RequestURL *url.URL
147+
Message string
148+
Errors []HTTPErrorItem
149+
150+
scopesSuggestion string
150151
}
151152

152153
type HTTPErrorItem struct {
@@ -165,6 +166,61 @@ func (err HTTPError) Error() string {
165166
return fmt.Sprintf("HTTP %d (%s)", err.StatusCode, err.RequestURL)
166167
}
167168

169+
func (err HTTPError) ScopesSuggestion() string {
170+
return err.scopesSuggestion
171+
}
172+
173+
// ScopesSuggestion is an error messaging utility that prints the suggestion to request additional OAuth
174+
// scopes in case a server response indicates that there are missing scopes.
175+
func ScopesSuggestion(resp *http.Response) string {
176+
if resp.StatusCode < 400 || resp.StatusCode > 499 {
177+
return ""
178+
}
179+
180+
endpointNeedsScopes := resp.Header.Get("X-Accepted-Oauth-Scopes")
181+
tokenHasScopes := resp.Header.Get("X-Oauth-Scopes")
182+
if tokenHasScopes == "" {
183+
return ""
184+
}
185+
186+
gotScopes := map[string]struct{}{}
187+
for _, s := range strings.Split(tokenHasScopes, ",") {
188+
s = strings.TrimSpace(s)
189+
gotScopes[s] = struct{}{}
190+
if strings.HasPrefix(s, "admin:") {
191+
gotScopes["read:"+strings.TrimPrefix(s, "admin:")] = struct{}{}
192+
gotScopes["write:"+strings.TrimPrefix(s, "admin:")] = struct{}{}
193+
} else if strings.HasPrefix(s, "write:") {
194+
gotScopes["read:"+strings.TrimPrefix(s, "write:")] = struct{}{}
195+
}
196+
}
197+
198+
for _, s := range strings.Split(endpointNeedsScopes, ",") {
199+
s = strings.TrimSpace(s)
200+
if _, gotScope := gotScopes[s]; s == "" || gotScope {
201+
continue
202+
}
203+
return fmt.Sprintf(
204+
"This API operation needs the %[1]q scope. To request it, run: gh auth refresh -h %[2]s -s %[1]s",
205+
s,
206+
ghinstance.NormalizeHostname(resp.Request.URL.Hostname()),
207+
)
208+
}
209+
210+
return ""
211+
}
212+
213+
// EndpointNeedsScopes adds additional OAuth scopes to an HTTP response as if they were returned from the
214+
// server endpoint. This improves HTTP 4xx error messaging for endpoints that don't explicitly list the
215+
// OAuth scopes they need.
216+
func EndpointNeedsScopes(resp *http.Response, s string) *http.Response {
217+
if resp.StatusCode >= 400 && resp.StatusCode < 500 {
218+
oldScopes := resp.Header.Get("X-Accepted-Oauth-Scopes")
219+
resp.Header.Set("X-Accepted-Oauth-Scopes", fmt.Sprintf("%s, %s", oldScopes, s))
220+
}
221+
return resp
222+
}
223+
168224
// GraphQL performs a GraphQL request and parses the response
169225
func (c Client) GraphQL(hostname string, query string, variables map[string]interface{}, data interface{}) error {
170226
reqBody, err := json.Marshal(map[string]interface{}{"query": query, "variables": variables})
@@ -261,9 +317,9 @@ func handleResponse(resp *http.Response, data interface{}) error {
261317

262318
func HandleHTTPError(resp *http.Response) error {
263319
httpError := HTTPError{
264-
StatusCode: resp.StatusCode,
265-
RequestURL: resp.Request.URL,
266-
OAuthScopes: resp.Header.Get("X-Oauth-Scopes"),
320+
StatusCode: resp.StatusCode,
321+
RequestURL: resp.Request.URL,
322+
scopesSuggestion: ScopesSuggestion(resp),
267323
}
268324

269325
if !jsonTypeRE.MatchString(resp.Header.Get("Content-Type")) {

‎api/client_test.go‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,3 +146,67 @@ func TestHandleHTTPError_GraphQL502(t *testing.T) {
146146
t.Errorf("got error: %v", err)
147147
}
148148
}
149+
150+
func TestHTTPError_ScopesSuggestion(t *testing.T) {
151+
makeResponse := func(s int, u, haveScopes, needScopes string) *http.Response {
152+
req, err := http.NewRequest("GET", u, nil)
153+
if err != nil {
154+
t.Fatal(err)
155+
}
156+
return &http.Response{
157+
Request: req,
158+
StatusCode: s,
159+
Body: ioutil.NopCloser(bytes.NewBufferString(`{}`)),
160+
Header: map[string][]string{
161+
"Content-Type": {"application/json"},
162+
"X-Oauth-Scopes": {haveScopes},
163+
"X-Accepted-Oauth-Scopes": {needScopes},
164+
},
165+
}
166+
}
167+
168+
tests := []struct {
169+
name string
170+
resp *http.Response
171+
want string
172+
}{
173+
{
174+
name: "has necessary scopes",
175+
resp: makeResponse(404, "https://api.github.com/gists", "repo, gist, read:org", "gist"),
176+
want: ``,
177+
},
178+
{
179+
name: "normalizes scopes",
180+
resp: makeResponse(404, "https://api.github.com/orgs/ORG/discussions", "admin:org, write:discussion", "read:org, read:discussion"),
181+
want: ``,
182+
},
183+
{
184+
name: "no scopes on endpoint",
185+
resp: makeResponse(404, "https://api.github.com/user", "repo", ""),
186+
want: ``,
187+
},
188+
{
189+
name: "missing a scope",
190+
resp: makeResponse(404, "https://api.github.com/gists", "repo, read:org", "gist, delete_repo"),
191+
want: `This API operation needs the "gist" scope. To request it, run: gh auth refresh -h github.com -s gist`,
192+
},
193+
{
194+
name: "server error",
195+
resp: makeResponse(500, "https://api.github.com/gists", "repo", "gist"),
196+
want: ``,
197+
},
198+
{
199+
name: "no scopes on token",
200+
resp: makeResponse(404, "https://api.github.com/gists", "", "gist, delete_repo"),
201+
want: ``,
202+
},
203+
}
204+
for _, tt := range tests {
205+
t.Run(tt.name, func(t *testing.T) {
206+
httpError := HandleHTTPError(tt.resp)
207+
if got := httpError.(HTTPError).ScopesSuggestion(); got != tt.want {
208+
t.Errorf("HTTPError.ScopesSuggestion() = %v, want %v", got, tt.want)
209+
}
210+
})
211+
}
212+
}

‎api/queries_repo.go‎

Lines changed: 28 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -241,12 +241,23 @@ func FetchRepository(client *Client, repo ghrepo.Interface, fields []string) (*R
241241
}
242242

243243
var result struct {
244-
Repository Repository
244+
Repository *Repository
245245
}
246246
if err := client.GraphQL(repo.RepoHost(), query, variables, &result); err != nil {
247247
return nil, err
248248
}
249-
return InitRepoHostname(&result.Repository, repo.RepoHost()), nil
249+
// The GraphQL API should have returned an error in case of a missing repository, but this isn't
250+
// guaranteed to happen when an authentication token with insufficient permissions is being used.
251+
if result.Repository == nil {
252+
return nil, GraphQLErrorResponse{
253+
Errors: []GraphQLError{{
254+
Type: "NOT_FOUND",
255+
Message: fmt.Sprintf("Could not resolve to a Repository with the name '%s/%s'.", repo.RepoOwner(), repo.RepoName()),
256+
}},
257+
}
258+
}
259+
260+
return InitRepoHostname(result.Repository, repo.RepoHost()), nil
250261
}
251262

252263
func GitHubRepo(client *Client, repo ghrepo.Interface) (*Repository, error) {
@@ -280,16 +291,24 @@ func GitHubRepo(client *Client, repo ghrepo.Interface) (*Repository, error) {
280291
"name": repo.RepoName(),
281292
}
282293

283-
result := struct {
284-
Repository Repository
285-
}{}
286-
err := client.GraphQL(repo.RepoHost(), query, variables, &result)
287-
288-
if err != nil {
294+
var result struct {
295+
Repository *Repository
296+
}
297+
if err := client.GraphQL(repo.RepoHost(), query, variables, &result); err != nil {
289298
return nil, err
290299
}
300+
// The GraphQL API should have returned an error in case of a missing repository, but this isn't
301+
// guaranteed to happen when an authentication token with insufficient permissions is being used.
302+
if result.Repository == nil {
303+
return nil, GraphQLErrorResponse{
304+
Errors: []GraphQLError{{
305+
Type: "NOT_FOUND",
306+
Message: fmt.Sprintf("Could not resolve to a Repository with the name '%s/%s'.", repo.RepoOwner(), repo.RepoName()),
307+
}},
308+
}
309+
}
291310

292-
return InitRepoHostname(&result.Repository, repo.RepoHost()), nil
311+
return InitRepoHostname(result.Repository, repo.RepoHost()), nil
293312
}
294313

295314
func RepoDefaultBranch(client *Client, repo ghrepo.Interface) (string, error) {

‎api/queries_repo_test.go‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,27 @@ import (
1010
"github.com/cli/cli/v2/pkg/httpmock"
1111
)
1212

13+
func TestGitHubRepo_notFound(t *testing.T) {
14+
httpReg := &httpmock.Registry{}
15+
defer httpReg.Verify(t)
16+
17+
httpReg.Register(
18+
httpmock.GraphQL(`query RepositoryInfo\b`),
19+
httpmock.StringResponse(`{ "data": { "repository": null } }`))
20+
21+
client := NewClient(ReplaceTripper(httpReg))
22+
repo, err := GitHubRepo(client, ghrepo.New("OWNER", "REPO"))
23+
if err == nil {
24+
t.Fatal("GitHubRepo did not return an error")
25+
}
26+
if wants := "GraphQL error: Could not resolve to a Repository with the name 'OWNER/REPO'."; err.Error() != wants {
27+
t.Errorf("GitHubRepo error: want %q, got %q", wants, err.Error())
28+
}
29+
if repo != nil {
30+
t.Errorf("GitHubRepo: expected nil repo, got %v", repo)
31+
}
32+
}
33+
1334
func Test_RepoMetadata(t *testing.T) {
1435
http := &httpmock.Registry{}
1536
client := NewClient(ReplaceTripper(http))

‎cmd/gh/main.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -226,6 +226,8 @@ func mainRun() exitCode {
226226
fmt.Fprintln(stderr, "Try authenticating with: gh auth login")
227227
} else if strings.Contains(err.Error(), "Resource protected by organization SAML enforcement") {
228228
fmt.Fprintln(stderr, "Try re-authenticating with: gh auth refresh")
229+
} else if msg := httpErr.ScopesSuggestion(); msg != "" {
230+
fmt.Fprintln(stderr, msg)
229231
}
230232

231233
return exitError

0 commit comments

Comments
 (0)