Let a caller name the scopes an endpoint needs · cli/cli@216199e · GitHub
Skip to content

Commit 216199e

Browse files
Let a caller name the scopes an endpoint needs
Some endpoints do not say which OAuth scopes they require, so gh supplies the answer itself: it calls EndpointNeedsScopes on the response before turning it into an error, which folds the scope into the suggestion the user is shown. That only works while the call site holds the response. A call site that hands request making to the shared client never sees a failed response, because the client has already converted it into an error and closed the body. So the scope has to travel with the request instead. Add WithEndpointScopes, and apply it on the error path: the scope is added to the error's headers before the suggestion is generated, which is the same mechanism as before, moved to where the response still exists. gh gist create is the first caller, and needed this to keep telling a user with an under-scoped token that they are missing the gist scope. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6dd1d13e-90bd-44b8-8bec-261795e282c3
1 parent 62a6e9e commit 216199e

6 files changed

Lines changed: 180 additions & 41 deletions

File tree

‎api/client.go‎

Lines changed: 46 additions & 4 deletions

‎api/request_test.go‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,8 +154,73 @@ func TestRequestErrorPreservesScopesSuggestion(t *testing.T) {
154154

155155
// Call sites used to mutate the response with EndpointNeedsScopes before converting it to an error.
156156
// Request converts internally, so WithEndpointScopes has to reproduce that suggestion.
157+
func TestRequestWithEndpointScopes(t *testing.T) {
158+
tests := []struct {
159+
name string
160+
statusCode int
161+
acceptedScopes string
162+
wantSuggestion string
163+
}{
164+
{
165+
name: "adds scopes the endpoint did not report",
166+
statusCode: 403,
167+
acceptedScopes: "",
168+
wantSuggestion: "delete_repo",
169+
},
170+
{
171+
name: "appends to scopes the endpoint did report",
172+
statusCode: 403,
173+
acceptedScopes: "repo",
174+
wantSuggestion: "delete_repo",
175+
},
176+
{
177+
name: "leaves 5xx errors alone",
178+
statusCode: 500,
179+
acceptedScopes: "",
180+
wantSuggestion: "",
181+
},
182+
}
183+
184+
for _, tt := range tests {
185+
t.Run(tt.name, func(t *testing.T) {
186+
reg := &httpmock.Registry{}
187+
defer reg.Verify(t)
188+
client := newTestClient(reg)
189+
190+
reg.Register(httpmock.MatchAny, func(req *http.Request) (*http.Response, error) {
191+
header := map[string][]string{
192+
"Content-Type": {"application/json; charset=utf-8"},
193+
"X-Oauth-Scopes": {"repo"},
194+
}
195+
if tt.acceptedScopes != "" {
196+
header["X-Accepted-Oauth-Scopes"] = []string{tt.acceptedScopes}
197+
}
198+
return &http.Response{
199+
Request: req,
200+
StatusCode: tt.statusCode,
201+
Body: io.NopCloser(bytes.NewBufferString(`{"message": "Forbidden"}`)),
202+
Header: header,
203+
}, nil
204+
})
205+
206+
_, err := client.Request("github.com", http.MethodDelete, "repos/OWNER/REPO", nil,
207+
WithEndpointScopes("delete_repo"))
208+
209+
var httpErr HTTPError
210+
require.ErrorAs(t, err, &httpErr)
211+
if tt.wantSuggestion == "" {
212+
assert.Empty(t, httpErr.ScopesSuggestion())
213+
} else {
214+
assert.Contains(t, httpErr.ScopesSuggestion(), tt.wantSuggestion)
215+
}
216+
})
217+
}
218+
}
219+
157220
type ctxKey struct{}
158221

222+
// Call sites such as release asset upload run inside a retry loop that owns a context, so the
223+
// context has to reach the outgoing request.
159224
func TestRequestWithContextPropagatesContext(t *testing.T) {
160225
reg := &httpmock.Registry{}
161226
defer reg.Verify(t)

‎docs/api-host-test-harness.md‎

Lines changed: 24 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -209,36 +209,37 @@ $ curl --cacert /tmp/ca.pem --resolve gh-gateway.internal:8443:127.0.0.1 \
209209

210210
## Current state
211211

212-
Phases 1, 2 and 3 pass, unchanged. That is the main thing this commit had to
213-
show: a dozen call sites changed how they make requests, and nothing that
214-
already worked stopped working.
212+
Phases 1, 2 and 3 pass, unchanged.
215213

216-
10 scripts are still red, but four of them now pass their entire body and fail
217-
only in deferred `gh repo delete` cleanup:
214+
`gist-create-view-delete` is green. It is the only script whose cleanup is
215+
`gh gist delete` rather than `gh repo delete`, which is why it can go green on
216+
its own while the rest cannot.
217+
218+
Nine scripts are still red, and the count overstates how much is broken. Five
219+
fail in their body, on a call site that is genuinely still unrouted. The other
220+
four pass their body in full and fail only in deferred cleanup:
218221

219222
| Script | Fails at |
220223
|---|---|
221-
| `repo-read-file` | `gh repo read-file` |
222-
| `extension` | `gh repo edit --add-topic` |
223-
| `search-issues` | `gh search issues` |
224-
| `auth-status` | scope checking |
225-
| `gist-create-view-delete` | `gh gist create` |
226-
| `repo-delete` | `gh repo delete` |
224+
| `repo-delete` | `gh repo delete`, line 9 |
225+
| `auth-status` | scope checking, line 2 |
226+
| `repo-read-file` | `gh repo read-file`, line 34 |
227+
| `extension` | `gh repo edit --add-topic`, line 29 |
228+
| `search-issues` | `gh search issues`, line 21 |
227229
| `release-upload-download`, `repo-list-rename`, `repo-rename-transfer-ownership`, `run-download` | nothing in the body; cleanup only |
228230

229-
`release-upload-download` joining that group is worth understanding, because it
230-
was not migrated by this commit. Uploads and downloads use the `upload_url` and
231-
asset URLs that the API itself returns, and now that the release is *created*
232-
through the gateway those URLs already point at the gateway. Routing propagates
233-
through response bodies, so fixing one call site can fix its dependents for
234-
free.
231+
Almost every script creates a repository and defers `gh repo delete`, so until
232+
that one call site is routed those four cannot report anything but red, however
233+
much of their own subject matter works. Script colour is a poor measure of
234+
progress on its own: read the step list above the failure, which shows how far
235+
the body got, and the `.txtar:NN` line, which says whether the failure is the
236+
script's subject or its cleanup.
235237

236238
## Remaining
237239

238-
`gh repo read-file`, `gh repo edit`, `gh search issues`, `gh auth status`, `gh
239-
gist create` and `gh repo delete`. Each is blocked on a capability the shared
240-
client does not have yet: per-request headers, endpoint scopes, or control over
241-
redirects.
240+
`gh repo delete`, which needs control over redirects, and `gh auth status`,
241+
`gh repo read-file`, `gh repo edit` and `gh search issues`, which need
242+
per-request headers.
242243

243244
## Transcript
244245

@@ -276,8 +277,8 @@ PHASE RESULT NAME
276277
4 FAIL extension.txtar
277278
4 FAIL search-issues.txtar
278279
4 FAIL auth-status.txtar
279-
5 FAIL gist-create-view-delete.txtar
280+
5 PASS gist-create-view-delete.txtar
280281
281282
== Summary
282-
10 subset script(s) red: release-upload-download.txtar repo-delete.txtar repo-list-rename.txtar repo-read-file.txtar repo-rename-transfer-ownership.txtar run-download.txtar extension.txtar search-issues.txtar auth-status.txtar gist-create-view-delete.txtar
283+
9 subset script(s) red: release-upload-download.txtar repo-delete.txtar repo-list-rename.txtar repo-read-file.txtar repo-rename-transfer-ownership.txtar run-download.txtar extension.txtar search-issues.txtar auth-status.txtar
283284
```

‎docs/api-host.md‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,9 +73,14 @@ report an error.
7373
## Known gaps
7474

7575
`api_host` is not yet honoured centrally. It works for requests that go through
76-
go-gh's client, and for `gh api`, but many call sites still build absolute
76+
go-gh's client, and for `gh api`, but some call sites still build absolute
7777
`api.github.com` URLs and never reach the gateway at all.
7878

79+
Those call sites are being migrated a capability at a time, because each one
80+
built its own request for a reason. Endpoint scopes were one such reason and
81+
are now expressible; per-request headers and control over redirects are not
82+
yet.
83+
7984
`gh api` is a wart worth naming. It does not use go-gh's client, so it resolves
8085
`api_host` itself with a second implementation of the same rule. Two
8186
implementations of "where does this request go" is exactly the shape of problem

‎pkg/cmd/gist/create/create.go‎

Lines changed: 8 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ import (
1717
"github.com/cli/cli/v2/api"
1818
"github.com/cli/cli/v2/internal/browser"
1919
"github.com/cli/cli/v2/internal/gh"
20-
"github.com/cli/cli/v2/internal/ghinstance"
2120
"github.com/cli/cli/v2/internal/safeurl"
2221
"github.com/cli/cli/v2/internal/text"
2322
"github.com/cli/cli/v2/pkg/cmd/gist/shared"
@@ -273,27 +272,23 @@ func createGist(client *http.Client, hostname, description string, public bool,
273272
return nil, err
274273
}
275274

276-
u, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(hostname), "gists")
275+
path, err := safeurl.JoinPath("gists")
277276
if err != nil {
278277
return nil, err
279278
}
280-
req, err := http.NewRequest(http.MethodPost, u.String(), requestBody)
281-
if err != nil {
282-
return nil, err
283-
}
284-
req.Header.Set("Content-Type", "application/json; charset=utf-8")
285279

286-
resp, err := client.Do(req)
280+
// The Content-Type this site used to set explicitly is the one the client already sends,
281+
// so it is no longer stated here.
282+
// TODO(api-client-rollout)
283+
// This line of code is part of a mechanical roll out of the api client.
284+
// As a follow up, consider whether the api client can be injected to this call site, rather than constructed
285+
resp, err := api.NewClientFromHTTP(client).Request(hostname, http.MethodPost, path.String(), requestBody,
286+
api.WithEndpointScopes("gist"))
287287
if err != nil {
288288
return nil, err
289289
}
290290
defer resp.Body.Close()
291291

292-
if resp.StatusCode > 299 {
293-
api.EndpointNeedsScopes(resp, "gist")
294-
return nil, api.HandleHTTPError(resp)
295-
}
296-
297292
result := &shared.Gist{}
298293
dec := json.NewDecoder(resp.Body)
299294
if err := dec.Decode(result); err != nil {

‎pkg/cmd/gist/create/create_test.go‎

Lines changed: 31 additions & 0 deletions

0 commit comments

Comments
 (0)