From 2d910406c690af0ce2ad39f79f948a713321c2b1 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Thu, 25 Apr 2024 09:28:49 -0400 Subject: [PATCH 1/3] proof of concept for flag-level disable auth check Building upon the existing command-level disable auth check logic, this commit adds flag-level disable auth check logic for any flag set with `-b,--bundle` flag of `gh attestation verify` being the first use case. Subsequent commit to build out testing is needed as IsAuthCheckEnabled does not have tests. --- pkg/cmd/attestation/verify/verify.go | 1 + pkg/cmdutil/auth_check.go | 29 ++++++++++++++++++++++++++-- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/attestation/verify/verify.go b/pkg/cmd/attestation/verify/verify.go index a323898ee..12d199e69 100644 --- a/pkg/cmd/attestation/verify/verify.go +++ b/pkg/cmd/attestation/verify/verify.go @@ -127,6 +127,7 @@ func NewVerifyCmd(f *cmdutil.Factory, runF func(*Options) error) *cobra.Command // general flags verifyCmd.Flags().StringVarP(&opts.BundlePath, "bundle", "b", "", "Path to bundle on disk, either a single bundle in a JSON file or a JSON lines file with multiple bundles") + cmdutil.DisableAuthCheckFlag(verifyCmd.Flags().Lookup("bundle")) cmdutil.StringEnumFlag(verifyCmd, &opts.DigestAlgorithm, "digest-alg", "d", "sha256", []string{"sha256", "sha512"}, "The algorithm used to compute a digest of the artifact") verifyCmd.Flags().StringVarP(&opts.Owner, "owner", "o", "", "GitHub organization to scope attestation lookup by") verifyCmd.Flags().StringVarP(&opts.Repo, "repo", "R", "", "Repository name in the format /") diff --git a/pkg/cmdutil/auth_check.go b/pkg/cmdutil/auth_check.go index 56ebb0c4d..fb269704f 100644 --- a/pkg/cmdutil/auth_check.go +++ b/pkg/cmdutil/auth_check.go @@ -1,16 +1,29 @@ package cmdutil import ( + "reflect" + "github.com/cli/cli/v2/internal/config" "github.com/spf13/cobra" + "github.com/spf13/pflag" ) +const skipAuthCheckAnnotation = "skipAuthCheck" + func DisableAuthCheck(cmd *cobra.Command) { if cmd.Annotations == nil { cmd.Annotations = map[string]string{} } - cmd.Annotations["skipAuthCheck"] = "true" + cmd.Annotations[skipAuthCheckAnnotation] = "true" +} + +func DisableAuthCheckFlag(flag *pflag.Flag) { + if flag.Annotations == nil { + flag.Annotations = map[string][]string{} + } + + flag.Annotations[skipAuthCheckAnnotation] = []string{"true"} } func CheckAuth(cfg config.Config) bool { @@ -32,7 +45,19 @@ func IsAuthCheckEnabled(cmd *cobra.Command) bool { } for c := cmd; c.Parent() != nil; c = c.Parent() { - if c.Annotations != nil && c.Annotations["skipAuthCheck"] == "true" { + // Check whether any command marked as DisableAuthCheck is set + if c.Annotations != nil && c.Annotations[skipAuthCheckAnnotation] == "true" { + return false + } + + // Check whether any flag marked as DisableAuthCheckFlag is set + var skipAuthCheck bool + c.Flags().Visit(func(f *pflag.Flag) { + if f.Annotations != nil && reflect.DeepEqual(f.Annotations[skipAuthCheckAnnotation], []string{"true"}) { + skipAuthCheck = true + } + }) + if skipAuthCheck { return false } } From 8e3afe55df03749343d3771c804106dc2a476f2a Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Mon, 29 Apr 2024 10:02:01 -0400 Subject: [PATCH 2/3] Test cmdutil.IsAuthCheckEnabled cases This commit adds various test cases around whether a command will require authentication based on Cobra annotation metadata. --- pkg/cmdutil/auth_check_test.go | 64 ++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/pkg/cmdutil/auth_check_test.go b/pkg/cmdutil/auth_check_test.go index 25cbae567..02d1fd775 100644 --- a/pkg/cmdutil/auth_check_test.go +++ b/pkg/cmdutil/auth_check_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/cli/cli/v2/internal/config" + "github.com/spf13/cobra" "github.com/stretchr/testify/require" ) @@ -53,3 +54,66 @@ func Test_CheckAuth(t *testing.T) { }) } } + +func Test_IsAuthCheckEnabled(t *testing.T) { + tests := []struct { + name string + init func() (*cobra.Command, error) + isAuthCheckEnabled bool + }{ + { + name: "no annotations", + init: func() (*cobra.Command, error) { + cmd := &cobra.Command{} + cmd.Flags().Bool("flag", false, "") + return cmd, nil + }, + isAuthCheckEnabled: true, + }, + { + name: "command-level disable", + init: func() (*cobra.Command, error) { + cmd := &cobra.Command{} + DisableAuthCheck(cmd) + return cmd, nil + }, + isAuthCheckEnabled: false, + }, + { + name: "command with flag-level disable, flag not set", + init: func() (*cobra.Command, error) { + cmd := &cobra.Command{} + cmd.Flags().Bool("flag", false, "") + DisableAuthCheckFlag(cmd.Flag("flag")) + return cmd, nil + }, + isAuthCheckEnabled: true, + }, + { + name: "command with flag-level disable, flag set", + init: func() (*cobra.Command, error) { + cmd := &cobra.Command{} + cmd.Flags().Bool("flag", false, "") + if err := cmd.Flags().Set("flag", "true"); err != nil { + return nil, err + } + + DisableAuthCheckFlag(cmd.Flag("flag")) + return cmd, nil + }, + isAuthCheckEnabled: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cmd, err := tt.init() + require.NoError(t, err) + + // IsAuthCheckEnabled assumes commands under test are subcommands + parent := &cobra.Command{Use: "root"} + parent.AddCommand(cmd) + require.Equal(t, tt.isAuthCheckEnabled, IsAuthCheckEnabled(cmd)) + }) + } +} From cc36d32a212a2b8b6611fb73549fe6d04fb6ec38 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Mon, 29 Apr 2024 12:02:41 -0400 Subject: [PATCH 3/3] Test `gh at verify -b` does not require auth Thanks to @williammartin, this completes the PR by ensuring the actual feature this new logic was added for actually works as expected :D --- pkg/cmd/attestation/verify/verify_test.go | 31 +++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/pkg/cmd/attestation/verify/verify_test.go b/pkg/cmd/attestation/verify/verify_test.go index 203f203b3..ef778f454 100644 --- a/pkg/cmd/attestation/verify/verify_test.go +++ b/pkg/cmd/attestation/verify/verify_test.go @@ -14,6 +14,7 @@ import ( "github.com/cli/cli/v2/pkg/cmd/attestation/test" "github.com/cli/cli/v2/pkg/cmd/attestation/verification" "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/spf13/cobra" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" @@ -237,6 +238,36 @@ func TestNewVerifyCmd(t *testing.T) { } } +func TestVerifyCmdAuthChecks(t *testing.T) { + f := &cmdutil.Factory{} + + t.Run("by default auth check is required", func(t *testing.T) { + cmd := NewVerifyCmd(f, func(o *Options) error { + return nil + }) + + // IsAuthCheckEnabled assumes commands under test are subcommands + parent := &cobra.Command{Use: "root"} + parent.AddCommand(cmd) + + require.NoError(t, cmd.ParseFlags([]string{})) + require.True(t, cmdutil.IsAuthCheckEnabled(cmd), "expected auth check to be required") + }) + + t.Run("when --bundle flag is provided, auth check is not required", func(t *testing.T) { + cmd := NewVerifyCmd(f, func(o *Options) error { + return nil + }) + + // IsAuthCheckEnabled assumes commands under test are subcommands + parent := &cobra.Command{Use: "root"} + parent.AddCommand(cmd) + + require.NoError(t, cmd.ParseFlags([]string{"--bundle", "not-important"})) + require.False(t, cmdutil.IsAuthCheckEnabled(cmd), "expected auth check not to be required due to --bundle flag") + }) +} + func TestJSONOutput(t *testing.T) { testIO, _, out, _ := iostreams.Test() opts := Options{