Merge pull request cli#13723 from cli/bagtoad/disable-auth-check-release-clone

Allow downloading release assets without authentication

Author: BagToad

pkg/cmd/release/download/download.go

@@ -111,6 +111,8 @@ func NewCmdDownload(f *cmdutil.Factory, runF func(*DownloadOptions) error) *cobr
 	cmd.Flags().BoolVar(&opts.OverwriteExisting, "clobber", false, "Overwrite existing files of the same name")
 	cmd.Flags().BoolVar(&opts.SkipExisting, "skip-existing", false, "Skip downloading when files of the same name exist")
 
+	cmdutil.DisableAuthCheck(cmd)
+
 	return cmd
 }
 

pkg/cmd/release/download/download_test.go

@@ -218,6 +218,37 @@ 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" }
+						]
+					}`),
+				)
+				// 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,

pkg/cmd/release/shared/fetch.go

@@ -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
 }
 
 // FetchLatestRelease finds the latest published release for a repository.

pkg/cmdutil/repo_override.go

@@ -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
@@ -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")

pkg/cmdutil/repo_override_test.go

@@ -0,0 +1,70 @@
+package cmdutil
+
+import (
+	"testing"
+
+	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/require"
+)
+
+// Test_EnableRepoOverride_authCheckIntegration is an integration test for the
+// coupling between repo override and the root auth gate, wired through cobra's
+// persistent pre-run hooks. EnableRepoOverride replaces a command's
+// PersistentPreRunE, so cobra no longer reaches the root auth gate on its own;
+// the override hook re-runs the ancestor hooks itself. That re-run must evaluate
+// the command the user actually invoked, the same way cobra hands the target
+// command to parent hooks:
+// https://github.com/spf13/cobra/blob/v1.10.2/command.go#L984-L986
+// This pins that a leaf's DisableAuthCheck is honored under a repo-override
+// parent.
+func Test_EnableRepoOverride_authCheckIntegration(t *testing.T) {
+	tests := []struct {
+		name            string
+		disableAuthLeaf bool
+		wantAuthChecked bool
+	}{
+		{
+			name:            "leaf opts out, honored under repo-override parent",
+			disableAuthLeaf: true,
+			wantAuthChecked: false,
+		},
+		{
+			name:            "leaf does not opt out, auth still checked",
+			disableAuthLeaf: false,
+			wantAuthChecked: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var gotAuthChecked, ranLeaf bool
+
+			// Stand in for the real root auth gate: it judges whatever command
+			// it is handed, so it must receive the invoked leaf command.
+			root := &cobra.Command{Use: "root"}
+			root.PersistentPreRunE = func(cmd *cobra.Command, args []string) error {
+				gotAuthChecked = IsAuthCheckEnabled(cmd)
+				return nil
+			}
+
+			parent := &cobra.Command{Use: "parent"}
+			EnableRepoOverride(parent, &Factory{})
+			root.AddCommand(parent)
+
+			leaf := &cobra.Command{
+				Use:  "leaf",
+				RunE: func(cmd *cobra.Command, args []string) error { ranLeaf = true; return nil },
+			}
+			if tt.disableAuthLeaf {
+				DisableAuthCheck(leaf)
+			}
+			parent.AddCommand(leaf)
+
+			root.SetArgs([]string{"parent", "leaf"})
+			require.NoError(t, root.Execute())
+
+			require.True(t, ranLeaf, "leaf command should have executed")
+			require.Equal(t, tt.wantAuthChecked, gotAuthChecked)
+		})
+	}
+}