From 68f3ef79cadfda52a5096f2651132e9f68b03d5e Mon Sep 17 00:00:00 2001 From: bagtoad <47394200+BagToad@users.noreply.github.com> Date: Fri, 18 Oct 2024 21:21:47 -0600 Subject: [PATCH 1/6] Handle missing "workflow" scope in createRelease --- pkg/cmd/release/create/create_test.go | 67 +++++++++++++++++++++++++++ pkg/cmd/release/create/http.go | 45 ++++++++++++++++++ pkg/httpmock/stub.go | 14 ++++++ 3 files changed, 126 insertions(+) diff --git a/pkg/cmd/release/create/create_test.go b/pkg/cmd/release/create/create_test.go index 85c1c3f3f..aded1bea9 100644 --- a/pkg/cmd/release/create/create_test.go +++ b/pkg/cmd/release/create/create_test.go @@ -10,6 +10,7 @@ import ( "path/filepath" "testing" + "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/git" "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/gh" @@ -1082,6 +1083,72 @@ func Test_createRun(t *testing.T) { runStubs: defaultRunStubs, wantErr: "cannot generate release notes from tag v1.2.3 as it does not exist locally", }, + { + name: "API returns 404, OAuth token has no workflow scope", + isTTY: false, + opts: CreateOptions{ + TagName: "Does not matter", + }, + runStubs: func(rs *run.CommandStubber) { + rs.Register(contentCmd, 0, "some tag message") + rs.Register(signatureCmd, 0, "") + }, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL("RepositoryFindRef"), + httpmock.StringResponse(`{"data":{"repository":{"ref": {"id": "tag id"}}}}`), + ) + reg.Register( + httpmock.REST("POST", "repos/OWNER/REPO/releases"), + httpmock.StatusScopesResponder(404, `repo,read:org`)) + }, + wantErr: heredoc.Doc(` + HTTP 404: Failed to create release, "workflow" scope may be required + To request it, run gh auth refresh -h github.com -s workflow + `), + }, + { + name: "API returns 404, OAuth token has workflow scope", + isTTY: false, + opts: CreateOptions{ + TagName: "Does not matter", + }, + runStubs: func(rs *run.CommandStubber) { + rs.Register(contentCmd, 0, "some tag message") + rs.Register(signatureCmd, 0, "") + }, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL("RepositoryFindRef"), + httpmock.StringResponse(`{"data":{"repository":{"ref": {"id": "tag id"}}}}`), + ) + reg.Register( + httpmock.REST("POST", "repos/OWNER/REPO/releases"), + httpmock.StatusScopesResponder(404, `repo,read:org,workflow`)) + }, + wantErr: "HTTP 404 (https://api.github.com/repos/OWNER/REPO/releases)", + }, + { + name: "API returns 404, not an OAuth token", + isTTY: false, + opts: CreateOptions{ + TagName: "Does not matter", + }, + runStubs: func(rs *run.CommandStubber) { + rs.Register(contentCmd, 0, "some tag message") + rs.Register(signatureCmd, 0, "") + }, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL("RepositoryFindRef"), + httpmock.StringResponse(`{"data":{"repository":{"ref": {"id": "tag id"}}}}`), + ) + reg.Register( + httpmock.REST("POST", "repos/OWNER/REPO/releases"), + httpmock.StatusStringResponse(404, `HTTP 404 (https://api.github.com/repos/OWNER/REPO/releases)`)) + }, + wantErr: "HTTP 404 (https://api.github.com/repos/OWNER/REPO/releases)", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/cmd/release/create/http.go b/pkg/cmd/release/create/http.go index 84e95710f..1512b5341 100644 --- a/pkg/cmd/release/create/http.go +++ b/pkg/cmd/release/create/http.go @@ -8,12 +8,16 @@ import ( "io" "net/http" "net/url" + "strings" + "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/cmd/release/shared" "github.com/shurcooL/githubv4" + + ghauth "github.com/cli/go-gh/v2/pkg/auth" ) type tag struct { @@ -174,6 +178,26 @@ func createRelease(httpClient *http.Client, repo ghrepo.Interface, params map[st } defer resp.Body.Close() + // This code checks if we received a 404 while attempting to create a release without + // the workflow scope, and if so, returns an error message that explains a possible + // solution to the user. + // + // If the same file (with both the same path and contents) exists + // on another branch in the repo, releases with workflow file changes can be + // created without the workflow scope. Otherwise, the workflow scope is + // required to create the release. + // + // https://docs.github.com/en/apps/oauth-apps/building-oauth-apps/scopes-for-oauth-apps#available-scopes + if resp.StatusCode == http.StatusNotFound && tokenMissingWorkflowScope(resp) { + normalizedHostname := ghauth.NormalizeHostname(resp.Request.URL.Hostname()) + errMissingRequiredWorkflowScope := errors.New(heredoc.Docf(` + HTTP 404: Failed to create release, "workflow" scope may be required + To request it, run gh auth refresh -h %[1]s -s workflow + `, normalizedHostname)) + + return nil, errMissingRequiredWorkflowScope + } + success := resp.StatusCode >= 200 && resp.StatusCode < 300 if !success { return nil, api.HandleHTTPError(resp) @@ -254,3 +278,24 @@ func deleteRelease(httpClient *http.Client, release *shared.Release) error { } return nil } + +// Given an http.Response, check if the token used in +// the request is missing the workflow scope. +func tokenMissingWorkflowScope(resp *http.Response) bool { + scopes := resp.Header.Get("X-Oauth-Scopes") + + // Return false when no scopes are present - no scopes in this header + // means that the user is probably authenticating with a token type other + // than an OAuth token, and we don't know what this token's scopes actually are. + if scopes == "" { + return false + } + + for _, s := range strings.Split(scopes, ",") { + if s == "workflow" { + return false + } + } + + return true +} diff --git a/pkg/httpmock/stub.go b/pkg/httpmock/stub.go index 787cdcf9d..d1a27d835 100644 --- a/pkg/httpmock/stub.go +++ b/pkg/httpmock/stub.go @@ -238,6 +238,20 @@ func ScopesResponder(scopes string) func(*http.Request) (*http.Response, error) } } +// StatusScopesResponder returns a response with the given status code and OAuth scopes. +func StatusScopesResponder(status int, scopes string) func(*http.Request) (*http.Response, error) { + return func(req *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: status, + Request: req, + Header: map[string][]string{ + "X-Oauth-Scopes": {scopes}, + }, + Body: io.NopCloser(bytes.NewBufferString("")), + }, nil + } +} + func httpResponse(status int, req *http.Request, body io.Reader) *http.Response { return httpResponseWithHeader(status, req, body, http.Header{}) } From 677ed2cdcfd6cefef65f1a59a6f44b9a8f0ba1c4 Mon Sep 17 00:00:00 2001 From: bagtoad <47394200+BagToad@users.noreply.github.com> Date: Thu, 21 Nov 2024 13:20:30 -0700 Subject: [PATCH 2/6] Refactor command documentation to use heredoc --- pkg/cmd/cache/delete/delete.go | 24 ++++++++++++------------ pkg/cmd/codespace/rebuild.go | 10 +++++++--- pkg/cmd/repo/delete/delete.go | 13 ++++++++----- 3 files changed, 27 insertions(+), 20 deletions(-) diff --git a/pkg/cmd/cache/delete/delete.go b/pkg/cmd/cache/delete/delete.go index 4794d1fbe..1835d87b1 100644 --- a/pkg/cmd/cache/delete/delete.go +++ b/pkg/cmd/cache/delete/delete.go @@ -35,23 +35,23 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co cmd := &cobra.Command{ Use: "delete [| | --all]", Short: "Delete GitHub Actions caches", - Long: ` - Delete GitHub Actions caches. + Long: heredoc.Doc(` + Delete GitHub Actions caches. - Deletion requires authorization with the "repo" scope. -`, + Deletion requires authorization with the "repo" scope. + `), Example: heredoc.Doc(` - # Delete a cache by id - $ gh cache delete 1234 + # Delete a cache by id + $ gh cache delete 1234 - # Delete a cache by key - $ gh cache delete cache-key + # Delete a cache by key + $ gh cache delete cache-key - # Delete a cache by id in a specific repo - $ gh cache delete 1234 --repo cli/cli + # Delete a cache by id in a specific repo + $ gh cache delete 1234 --repo cli/cli - # Delete all caches - $ gh cache delete --all + # Delete all caches + $ gh cache delete --all `), Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { diff --git a/pkg/cmd/codespace/rebuild.go b/pkg/cmd/codespace/rebuild.go index 93d43bf29..565406edc 100644 --- a/pkg/cmd/codespace/rebuild.go +++ b/pkg/cmd/codespace/rebuild.go @@ -4,6 +4,7 @@ import ( "context" "fmt" + "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/internal/codespaces" "github.com/cli/cli/v2/internal/codespaces/api" "github.com/cli/cli/v2/internal/codespaces/portforwarder" @@ -20,9 +21,12 @@ func newRebuildCmd(app *App) *cobra.Command { rebuildCmd := &cobra.Command{ Use: "rebuild", Short: "Rebuild a codespace", - Long: `Rebuilding recreates your codespace. Your code and any current changes will be -preserved. Your codespace will be rebuilt using your working directory's -dev container. A full rebuild also removes cached Docker images.`, + Long: heredoc.Doc(` + Rebuilding recreates your codespace. + + Your code and any current changes will be preserved. Your codespace will be rebuilt using + your working directory's dev container. A full rebuild also removes cached Docker images. + `), Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, args []string) error { return app.Rebuild(cmd.Context(), selector, fullRebuild) diff --git a/pkg/cmd/repo/delete/delete.go b/pkg/cmd/repo/delete/delete.go index 722d748b9..e6fbf6363 100644 --- a/pkg/cmd/repo/delete/delete.go +++ b/pkg/cmd/repo/delete/delete.go @@ -6,6 +6,7 @@ import ( "net/http" "strings" + "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/cmdutil" @@ -38,12 +39,14 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co cmd := &cobra.Command{ Use: "delete []", Short: "Delete a repository", - Long: `Delete a GitHub repository. + Long: heredoc.Doc(` + Delete a GitHub repository. + + With no argument, deletes the current repository. Otherwise, deletes the specified repository. -With no argument, deletes the current repository. Otherwise, deletes the specified repository. - -Deletion requires authorization with the "delete_repo" scope. -To authorize, run "gh auth refresh -s delete_repo"`, + Deletion requires authorization with the "delete_repo" scope. + To authorize, run "gh auth refresh -s delete_repo" + `), Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { if len(args) > 0 { From 74f13a9b4f8e735a14ca227a21c31231db4fc41c Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 21 Nov 2024 13:55:35 -0700 Subject: [PATCH 3/6] Apply suggestions from code review Co-authored-by: Andy Feller --- pkg/cmd/cache/delete/delete.go | 6 +++--- pkg/cmd/repo/delete/delete.go | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/cmd/cache/delete/delete.go b/pkg/cmd/cache/delete/delete.go index 1835d87b1..65a9d696a 100644 --- a/pkg/cmd/cache/delete/delete.go +++ b/pkg/cmd/cache/delete/delete.go @@ -35,11 +35,11 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co cmd := &cobra.Command{ Use: "delete [| | --all]", Short: "Delete GitHub Actions caches", - Long: heredoc.Doc(` + Long: heredoc.Docf(` Delete GitHub Actions caches. - Deletion requires authorization with the "repo" scope. - `), + Deletion requires authorization with the %[1]srepo%[1]s scope. + `, "`"), Example: heredoc.Doc(` # Delete a cache by id $ gh cache delete 1234 diff --git a/pkg/cmd/repo/delete/delete.go b/pkg/cmd/repo/delete/delete.go index e6fbf6363..7c6476f1d 100644 --- a/pkg/cmd/repo/delete/delete.go +++ b/pkg/cmd/repo/delete/delete.go @@ -39,14 +39,14 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co cmd := &cobra.Command{ Use: "delete []", Short: "Delete a repository", - Long: heredoc.Doc(` + Long: heredoc.Docf(` Delete a GitHub repository. With no argument, deletes the current repository. Otherwise, deletes the specified repository. - Deletion requires authorization with the "delete_repo" scope. - To authorize, run "gh auth refresh -s delete_repo" - `), + Deletion requires authorization with the %[1]sdelete_repo%[1]s scope. + To authorize, run %[1]sgh auth refresh -s delete_repo%[1]s + `, "`"), Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { if len(args) > 0 { From a4f96d29e32214c13a0f5c5830a39a714bea94ba Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Thu, 21 Nov 2024 13:52:15 -0700 Subject: [PATCH 4/6] Refactor `workflow` scope checking Refactor the logic for checking `workflow` scope checking in releases to be in the positive - check if the scope is there, not check if it isn't there. Then, when the function is called we invert it. Also update comments to be more imperative. This refactor also incorporates @andyfeller's suggestion to use `slices`. Co-Authored-By: Andy Feller --- pkg/cmd/release/create/http.go | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/pkg/cmd/release/create/http.go b/pkg/cmd/release/create/http.go index 1512b5341..1162e7080 100644 --- a/pkg/cmd/release/create/http.go +++ b/pkg/cmd/release/create/http.go @@ -8,6 +8,7 @@ import ( "io" "net/http" "net/url" + "slices" "strings" "github.com/MakeNowJust/heredoc" @@ -178,17 +179,18 @@ func createRelease(httpClient *http.Client, repo ghrepo.Interface, params map[st } defer resp.Body.Close() - // This code checks if we received a 404 while attempting to create a release without - // the workflow scope, and if so, returns an error message that explains a possible + // Check if we received a 404 while attempting to create a release without + // the workflow scope, and if so, return an error message that explains a possible // solution to the user. // // If the same file (with both the same path and contents) exists // on another branch in the repo, releases with workflow file changes can be // created without the workflow scope. Otherwise, the workflow scope is - // required to create the release. + // required to create the release, but the API does not indicate this criteria + // beyond returning a 404. // // https://docs.github.com/en/apps/oauth-apps/building-oauth-apps/scopes-for-oauth-apps#available-scopes - if resp.StatusCode == http.StatusNotFound && tokenMissingWorkflowScope(resp) { + if resp.StatusCode == http.StatusNotFound && !tokenHasWorkflowScope(resp) { normalizedHostname := ghauth.NormalizeHostname(resp.Request.URL.Hostname()) errMissingRequiredWorkflowScope := errors.New(heredoc.Docf(` HTTP 404: Failed to create release, "workflow" scope may be required @@ -279,23 +281,17 @@ func deleteRelease(httpClient *http.Client, release *shared.Release) error { return nil } -// Given an http.Response, check if the token used in -// the request is missing the workflow scope. -func tokenMissingWorkflowScope(resp *http.Response) bool { +// tokenHasWorkflowScope checks if the given http.Response's token has the workflow scope. +// Tokens that do not have OAuth scopes are assumed to have the workflow scope. +func tokenHasWorkflowScope(resp *http.Response) bool { scopes := resp.Header.Get("X-Oauth-Scopes") - // Return false when no scopes are present - no scopes in this header + // Return true when no scopes are present - no scopes in this header // means that the user is probably authenticating with a token type other // than an OAuth token, and we don't know what this token's scopes actually are. if scopes == "" { - return false + return true } - for _, s := range strings.Split(scopes, ",") { - if s == "workflow" { - return false - } - } - - return true + return slices.Contains(strings.Split(scopes, ","), "workflow") } From 11dc6df88b93e5e767c4f82ca3245693afddc86a Mon Sep 17 00:00:00 2001 From: bagtoad <47394200+BagToad@users.noreply.github.com> Date: Sat, 23 Nov 2024 13:19:45 -0700 Subject: [PATCH 5/6] ScopesResponder wraps StatusScopesResponder --- pkg/httpmock/stub.go | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/pkg/httpmock/stub.go b/pkg/httpmock/stub.go index d1a27d835..196a047d8 100644 --- a/pkg/httpmock/stub.go +++ b/pkg/httpmock/stub.go @@ -225,17 +225,9 @@ func GraphQLQuery(body string, cb func(string, map[string]interface{})) Responde } } +// ScopesResponder returns a response with a 200 status code and the given OAuth scopes. func ScopesResponder(scopes string) func(*http.Request) (*http.Response, error) { - return func(req *http.Request) (*http.Response, error) { - return &http.Response{ - StatusCode: 200, - Request: req, - Header: map[string][]string{ - "X-Oauth-Scopes": {scopes}, - }, - Body: io.NopCloser(bytes.NewBufferString("")), - }, nil - } + return StatusScopesResponder(http.StatusOK, scopes) } // StatusScopesResponder returns a response with the given status code and OAuth scopes. From deb34d6456d14efe73ccd61fcb88e063da5f4794 Mon Sep 17 00:00:00 2001 From: bagtoad <47394200+BagToad@users.noreply.github.com> Date: Sat, 23 Nov 2024 16:59:49 -0700 Subject: [PATCH 6/6] Refactor error handling for missing "workflow" scope in createRelease --- pkg/cmd/release/create/create.go | 15 +++++++++++++++ pkg/cmd/release/create/create_test.go | 9 +++++---- pkg/cmd/release/create/http.go | 18 +++++++++++------- 3 files changed, 31 insertions(+), 11 deletions(-) diff --git a/pkg/cmd/release/create/create.go b/pkg/cmd/release/create/create.go index 0b7c99b23..8e77546d0 100644 --- a/pkg/cmd/release/create/create.go +++ b/pkg/cmd/release/create/create.go @@ -477,6 +477,21 @@ func createRun(opts *CreateOptions) error { } newRelease, err := createRelease(httpClient, baseRepo, params) + + var errMissingRequiredWorkflowScope *errMissingRequiredWorkflowScope + if errors.As(err, &errMissingRequiredWorkflowScope) { + host := errMissingRequiredWorkflowScope.Hostname + refreshInstructions := fmt.Sprintf("gh auth refresh -h %[1]s -s workflow", host) + cs := opts.IO.ColorScheme() + + var sb strings.Builder + sb.WriteString(fmt.Sprintf("%s Failed to create release, \"workflow\" scope may be required.\n", cs.WarningIcon())) + sb.WriteString(fmt.Sprintf("To request it, run:\n%s\n", cs.Bold(refreshInstructions))) + fmt.Fprint(opts.IO.ErrOut, sb.String()) + + return cmdutil.SilentError + } + if err != nil { return err } diff --git a/pkg/cmd/release/create/create_test.go b/pkg/cmd/release/create/create_test.go index aded1bea9..c9e9a8c8a 100644 --- a/pkg/cmd/release/create/create_test.go +++ b/pkg/cmd/release/create/create_test.go @@ -1102,10 +1102,12 @@ func Test_createRun(t *testing.T) { httpmock.REST("POST", "repos/OWNER/REPO/releases"), httpmock.StatusScopesResponder(404, `repo,read:org`)) }, - wantErr: heredoc.Doc(` - HTTP 404: Failed to create release, "workflow" scope may be required - To request it, run gh auth refresh -h github.com -s workflow + wantStderr: heredoc.Doc(` + ! Failed to create release, "workflow" scope may be required. + To request it, run: + gh auth refresh -h github.com -s workflow `), + wantErr: cmdutil.SilentError.Error(), }, { name: "API returns 404, OAuth token has workflow scope", @@ -1182,7 +1184,6 @@ func Test_createRun(t *testing.T) { err := createRun(&tt.opts) if tt.wantErr != "" { require.EqualError(t, err, tt.wantErr) - return } else { require.NoError(t, err) } diff --git a/pkg/cmd/release/create/http.go b/pkg/cmd/release/create/http.go index 1162e7080..3bb55f39e 100644 --- a/pkg/cmd/release/create/http.go +++ b/pkg/cmd/release/create/http.go @@ -11,7 +11,6 @@ import ( "slices" "strings" - "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" @@ -32,6 +31,14 @@ type releaseNotes struct { var notImplementedError = errors.New("not implemented") +type errMissingRequiredWorkflowScope struct { + Hostname string +} + +func (e errMissingRequiredWorkflowScope) Error() string { + return "workflow scope may be required" +} + func remoteTagExists(httpClient *http.Client, repo ghrepo.Interface, tagName string) (bool, error) { gql := api.NewClientFromHTTP(httpClient) qualifiedTagName := fmt.Sprintf("refs/tags/%s", tagName) @@ -192,12 +199,9 @@ func createRelease(httpClient *http.Client, repo ghrepo.Interface, params map[st // https://docs.github.com/en/apps/oauth-apps/building-oauth-apps/scopes-for-oauth-apps#available-scopes if resp.StatusCode == http.StatusNotFound && !tokenHasWorkflowScope(resp) { normalizedHostname := ghauth.NormalizeHostname(resp.Request.URL.Hostname()) - errMissingRequiredWorkflowScope := errors.New(heredoc.Docf(` - HTTP 404: Failed to create release, "workflow" scope may be required - To request it, run gh auth refresh -h %[1]s -s workflow - `, normalizedHostname)) - - return nil, errMissingRequiredWorkflowScope + return nil, &errMissingRequiredWorkflowScope{ + Hostname: normalizedHostname, + } } success := resp.StatusCode >= 200 && resp.StatusCode < 300