Allow downloading release assets without authentication by BagToad · Pull Request #13723 · cli/cli · GitHub
Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions pkg/cmd/release/download/download.go
34 changes: 34 additions & 0 deletions pkg/cmd/release/download/download_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,40 @@ func Test_downloadRun(t *testing.T) {
"windows-64bit.zip",
},
},
{
name: "downloads published release when the draft lookup is unauthorized",
isTTY: true,
opts: DownloadOptions{
TagName: "v1.2.3",
Destination: ".",
Concurrency: 2,
},
httpStubs: func(reg *httpmock.Registry) {
reg.Register(
httpmock.REST("GET", "repos/OWNER/REPO/releases/tags/v1.2.3"),
httpmock.StringResponse(`{
"assets": [
{ "name": "linux.tgz", "size": 56,
"url": "https://api.github.com/assets/5678" }
],
"tarball_url": "https://api.github.com/repos/OWNER/REPO/tarball/v1.2.3",
"zipball_url": "https://api.github.com/repos/OWNER/REPO/zipball/v1.2.3"
}`),
)
// An unauthenticated draft lookup fails over GraphQL and must not
// mask the published release found over REST.
reg.Register(
httpmock.GraphQL(`query RepositoryReleaseByTag\b`),
httpmock.StatusStringResponse(403, `{"message":"This endpoint requires you to be authenticated."}`),
)
reg.Register(httpmock.REST("GET", "assets/5678"), httpmock.StringResponse(`5678`))
},
wantStdout: ``,
wantStderr: ``,
wantFiles: []string{
"linux.tgz",
},
},
{
name: "download assets matching pattern into destination directory",
isTTY: true,
Expand Down
24 changes: 18 additions & 6 deletions pkg/cmd/release/shared/fetch.go
Original file line number Diff line number Diff line change
Expand Up @@ -201,15 +201,27 @@ func FetchRelease(ctx context.Context, httpClient *http.Client, repo ghrepo.Inte
results <- fetchResult{release: release, error: err}
}()

res := <-results
if errors.Is(res.error, ErrReleaseNotFound) {
res = <-results
cancel() // satisfy the linter even though no goroutines are running anymore
} else {
// Prefer a release found by either lookup. A single failed lookup, such as
// the draft lookup when unauthenticated, must not mask a release found by
// the other; only report an error when both lookups fail.
first := <-results
if first.error == nil {
cancel()
<-results // drain the channel
return first.release, nil
}
return res.release, res.error

second := <-results
cancel() // satisfy the linter even though no goroutines are running anymore
if second.error == nil {
return second.release, nil
}

// Both lookups failed; prefer reporting the release as not found.
if errors.Is(second.error, ErrReleaseNotFound) {
return nil, second.error
}
return nil, first.error
Comment thread
BagToad marked this conversation as resolved.
}

// FetchLatestRelease finds the latest published release for a repository.
Expand Down
27 changes: 21 additions & 6 deletions pkg/cmdutil/repo_override.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,25 @@ import (
"github.com/spf13/cobra"
)

func executeParentHooks(cmd *cobra.Command, args []string) error {
for cmd.HasParent() {
cmd = cmd.Parent()
if cmd.PersistentPreRunE != nil {
return cmd.PersistentPreRunE(cmd, args)
// executeParentHook re-runs the nearest ancestor's persistent pre-run hook,
// which the hook installed by EnableRepoOverride would otherwise shadow. By
// default cobra runs only the nearest PersistentPreRunE found walking up from
// the invoked command, so without this the nearest ancestor hook, such as the
// root auth gate, would never run for a repo-override command.
//
// That ancestor hook receives the invoked leaf cmd, not the ancestor, matching
// how cobra passes the leaf to every persistent hook:
// https://github.com/spf13/cobra/blob/v1.10.2/command.go#L984-L986
//
// cobra's EnableTraverseRunHooks global is the native equivalent and runs the
// whole root-to-leaf chain for us, but it is global. Enabling it would change
// pre-run behavior for every command: double-running the parents that issue
// develop and EnableRepoOverride re-run by hand, and un-suppressing the root
// gate that agent-task and skills intentionally shadow.
func executeParentHook(overrideCmd, cmd *cobra.Command, args []string) error {
for p := overrideCmd.Parent(); p != nil; p = p.Parent() {
if p.PersistentPreRunE != nil {
return p.PersistentPreRunE(cmd, args)
}
}
return nil
Expand Down Expand Up @@ -47,8 +61,9 @@ func EnableRepoOverride(cmd *cobra.Command, f *Factory) {
return results, cobra.ShellCompDirectiveNoFileComp
})

overrideCmd := cmd
cmd.PersistentPreRunE = func(cmd *cobra.Command, args []string) error {
if err := executeParentHooks(cmd, args); err != nil {
if err := executeParentHook(overrideCmd, cmd, args); err != nil {
return err
}
repoOverride, _ := cmd.Flags().GetString("repo")
Expand Down
70 changes: 70 additions & 0 deletions pkg/cmdutil/repo_override_test.go
Loading