From a820457b098ea3843f976f9ca39849fbf9461a98 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Mon, 28 Oct 2024 11:47:31 -0600 Subject: [PATCH 1/8] clean up skipped online tests Signed-off-by: Meredith Lancaster --- .../verify/verify_integration_test.go | 27 ++++++++ pkg/cmd/attestation/verify/verify_test.go | 69 ------------------- 2 files changed, 27 insertions(+), 69 deletions(-) diff --git a/pkg/cmd/attestation/verify/verify_integration_test.go b/pkg/cmd/attestation/verify/verify_integration_test.go index 4b0f0adfb..0b15e823e 100644 --- a/pkg/cmd/attestation/verify/verify_integration_test.go +++ b/pkg/cmd/attestation/verify/verify_integration_test.go @@ -83,6 +83,33 @@ func TestVerifyIntegration(t *testing.T) { require.Error(t, err) require.ErrorContains(t, err, "expected SourceRepositoryURI to be https://github.com/fakeowner/fakerepo, got https://github.com/sigstore/sigstore-js") }) + + t.Run("with no matching OIDC issuer", func(t *testing.T) { + opts := publicGoodOpts + opts.OIDCIssuer = "some-other-issuer" + + err := runVerify(&opts) + require.Error(t, err) + require.ErrorContains(t, err, "expected Issuer to be some-other-issuer, got https://token.actions.githubusercontent.com") + }) + + t.Run("with invalid SAN", func(t *testing.T) { + opts := publicGoodOpts + opts.SAN = "fake san" + + err := runVerify(&opts) + require.Error(t, err) + require.ErrorContains(t, err, "verifying with issuer \"sigstore.dev\"") + }) + + t.Run("with invalid SAN regex", func(t *testing.T) { + opts := publicGoodOpts + opts.SANRegex = "^https://github.com/sigstore/not-real/" + + err := runVerify(&opts) + require.Error(t, err) + require.ErrorContains(t, err, "verifying with issuer \"sigstore.dev\"") + }) } func TestVerifyIntegrationCustomIssuer(t *testing.T) { diff --git a/pkg/cmd/attestation/verify/verify_test.go b/pkg/cmd/attestation/verify/verify_test.go index 306ff8b35..93ad4bdbc 100644 --- a/pkg/cmd/attestation/verify/verify_test.go +++ b/pkg/cmd/attestation/verify/verify_test.go @@ -453,75 +453,6 @@ func TestRunVerify(t *testing.T) { require.ErrorContains(t, err, "failed to fetch attestations from wrong-owner") }) - // TODO: this test can only be tested with a live SigstoreVerifier - // add integration tests or HTTP mocked sigstore verifier tests - // to test this case - t.Run("with invalid OIDC issuer", func(t *testing.T) { - t.Skip() - opts := publicGoodOpts - opts.OIDCIssuer = "not-a-real-issuer" - require.Error(t, runVerify(&opts)) - }) - - // TODO: this test can only be tested with a live SigstoreVerifier - // add integration tests or HTTP mocked sigstore verifier tests - // to test this case - t.Run("with SAN enforcement", func(t *testing.T) { - t.Skip() - opts := Options{ - ArtifactPath: artifactPath, - BundlePath: bundlePath, - APIClient: api.NewTestClient(), - DigestAlgorithm: "sha512", - Logger: logger, - OIDCIssuer: verification.GitHubOIDCIssuer, - Owner: "sigstore", - SAN: SigstoreSanValue, - SigstoreVerifier: verification.NewMockSigstoreVerifier(t), - } - require.Nil(t, runVerify(&opts)) - }) - - // TODO: this test can only be tested with a live SigstoreVerifier - // add integration tests or HTTP mocked sigstore verifier tests - // to test this case - t.Run("with invalid SAN", func(t *testing.T) { - t.Skip() - opts := publicGoodOpts - opts.SAN = "fake san" - require.Error(t, runVerify(&opts)) - }) - - // TODO: this test can only be tested with a live SigstoreVerifier - // add integration tests or HTTP mocked sigstore verifier tests - // to test this case - t.Run("with SAN regex enforcement", func(t *testing.T) { - t.Skip() - opts := publicGoodOpts - opts.SANRegex = SigstoreSanRegex - require.Nil(t, runVerify(&opts)) - }) - - // TODO: this test can only be tested with a live SigstoreVerifier - // add integration tests or HTTP mocked sigstore verifier tests - // to test this case - t.Run("with invalid SAN regex", func(t *testing.T) { - t.Skip() - opts := publicGoodOpts - opts.SANRegex = "^https://github.com/sigstore/not-real/" - require.Error(t, runVerify(&opts)) - }) - - // TODO: this test can only be tested with a live SigstoreVerifier - // add integration tests or HTTP mocked sigstore verifier tests - // to test this case - t.Run("with no matching OIDC issuer", func(t *testing.T) { - t.Skip() - opts := publicGoodOpts - opts.OIDCIssuer = "some-other-issuer" - require.Error(t, runVerify(&opts)) - }) - t.Run("with missing API client", func(t *testing.T) { customOpts := publicGoodOpts customOpts.APIClient = nil From ce5bde4379e4ce7c9ba3ba520e39d7b14258c98d Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Mon, 28 Oct 2024 12:59:04 -0600 Subject: [PATCH 2/8] simplify signer workflow validation tests Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verify/policy_test.go | 46 ++++++++++------------- 1 file changed, 19 insertions(+), 27 deletions(-) diff --git a/pkg/cmd/attestation/verify/policy_test.go b/pkg/cmd/attestation/verify/policy_test.go index ae1e52955..bc831e46d 100644 --- a/pkg/cmd/attestation/verify/policy_test.go +++ b/pkg/cmd/attestation/verify/policy_test.go @@ -36,18 +36,28 @@ func TestValidateSignerWorkflow(t *testing.T) { providedSignerWorkflow string expectedWorkflowRegex string host string + expectErr bool + errContains string } testcases := []testcase{ { name: "workflow with no host specified", providedSignerWorkflow: "github/artifact-attestations-workflows/.github/workflows/attest.yml", - expectedWorkflowRegex: "^https://github.com/github/artifact-attestations-workflows/.github/workflows/attest.yml", + expectErr: true, + errContains: "unknown host", }, { - name: "workflow with host specified", + name: "workflow with default host", + providedSignerWorkflow: "github/artifact-attestations-workflows/.github/workflows/attest.yml", + expectedWorkflowRegex: "^https://github.com/github/artifact-attestations-workflows/.github/workflows/attest.yml", + host: "github.com", + }, + { + name: "workflow with workflow URL included", providedSignerWorkflow: "github.com/github/artifact-attestations-workflows/.github/workflows/attest.yml", expectedWorkflowRegex: "^https://github.com/github/artifact-attestations-workflows/.github/workflows/attest.yml", + host: "github.com", }, { name: "workflow with GH_HOST set", @@ -61,12 +71,6 @@ func TestValidateSignerWorkflow(t *testing.T) { expectedWorkflowRegex: "^https://authedhost.github.com/github/artifact-attestations-workflows/.github/workflows/attest.yml", host: "authedhost.github.com", }, - { - name: "workflow with authenticated host", - providedSignerWorkflow: "github/artifact-attestations-workflows/.github/workflows/attest.yml", - expectedWorkflowRegex: "^https://authedhost.github.com/github/artifact-attestations-workflows/.github/workflows/attest.yml", - host: "authedhost.github.com", - }, } for _, tc := range testcases { @@ -78,28 +82,16 @@ func TestValidateSignerWorkflow(t *testing.T) { } // All host resolution is done verify.go:RunE - if tc.host == "" { - // Set to default host - tc.host = "github.com" - } opts.Hostname = tc.host - workflowRegex, err := validateSignerWorkflow(opts) - require.NoError(t, err) require.Equal(t, tc.expectedWorkflowRegex, workflowRegex) + if tc.expectErr { + require.Error(t, err) + require.ErrorContains(t, err, tc.errContains) + } else { + require.NoError(t, err) + require.Equal(t, tc.expectedWorkflowRegex, workflowRegex) + } } } - -func TestValidateSignerWorkflowNoHost(t *testing.T) { - cmdFactory := factory.New("test") - opts := &Options{ - Config: cmdFactory.Config, - SignerWorkflow: "github/artifact-attestations-workflows/.github/workflows/attest.yml", - } - - workflowRegex, err := validateSignerWorkflow(opts) - require.Error(t, err) - require.ErrorContains(t, err, "unknown host") - require.Equal(t, "", workflowRegex) -} From f8b0f5e68738623a0bf3dc31718adb1be3a3cd58 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Mon, 28 Oct 2024 13:02:12 -0600 Subject: [PATCH 3/8] clean up test Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verify/policy_test.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/pkg/cmd/attestation/verify/policy_test.go b/pkg/cmd/attestation/verify/policy_test.go index bc831e46d..39e5d3707 100644 --- a/pkg/cmd/attestation/verify/policy_test.go +++ b/pkg/cmd/attestation/verify/policy_test.go @@ -74,10 +74,8 @@ func TestValidateSignerWorkflow(t *testing.T) { } for _, tc := range testcases { - cmdFactory := factory.New("test") - opts := &Options{ - Config: cmdFactory.Config, + Config: factory.New("test").Config, SignerWorkflow: tc.providedSignerWorkflow, } From 502856082e7a1afda89b57da8040ce5d31cbba27 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Mon, 28 Oct 2024 13:40:23 -0600 Subject: [PATCH 4/8] table tests Signed-off-by: Meredith Lancaster --- .../verification/sigstore_integration_test.go | 88 +++++++++---------- 1 file changed, 40 insertions(+), 48 deletions(-) diff --git a/pkg/cmd/attestation/verification/sigstore_integration_test.go b/pkg/cmd/attestation/verification/sigstore_integration_test.go index 1d3ec2d75..d4a92c541 100644 --- a/pkg/cmd/attestation/verification/sigstore_integration_test.go +++ b/pkg/cmd/attestation/verification/sigstore_integration_test.go @@ -15,32 +15,52 @@ import ( ) func TestLiveSigstoreVerifier(t *testing.T) { - t.Run("with invalid signature", func(t *testing.T) { - attestations := getAttestationsFor(t, "../test/data/sigstoreBundle-invalid-signature.json") - require.NotNil(t, attestations) + type testcase struct { + name string + attestations []*api.Attestation + expectErr bool + errContains string + } + testcases := []testcase{ + { + name: "with invalid signature", + attestations: getAttestationsFor(t, "../test/data/sigstoreBundle-invalid-signature.json"), + expectErr: true, + errContains: "verifying with issuer \"sigstore.dev\"", + }, + { + name: "with valid artifact and JSON lines file containing multiple Sigstore bundles", + attestations: getAttestationsFor(t, "../test/data/sigstore-js-2.1.0_with_2_bundles.jsonl"), + }, + { + name: "with invalid bundle version", + attestations: getAttestationsFor(t, "../test/data/sigstore-js-2.1.0-bundle-v0.1.json"), + expectErr: true, + errContains: "unsupported bundle version", + }, + { + name: "with no attestations", + attestations: []*api.Attestation{}, + }, + } + + for _, tc := range testcases { verifier := NewLiveSigstoreVerifier(SigstoreConfig{ Logger: io.NewTestHandler(), }) - res := verifier.Verify(attestations, publicGoodPolicy(t)) - require.Error(t, res.Error) - require.ErrorContains(t, res.Error, "verifying with issuer \"sigstore.dev\"") - require.Nil(t, res.VerifyResults) - }) + res := verifier.Verify(tc.attestations, publicGoodPolicy(t)) - t.Run("with valid artifact and JSON lines file containing multiple Sigstore bundles", func(t *testing.T) { - attestations := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0_with_2_bundles.jsonl") - require.Len(t, attestations, 2) - - verifier := NewLiveSigstoreVerifier(SigstoreConfig{ - Logger: io.NewTestHandler(), - }) - - res := verifier.Verify(attestations, publicGoodPolicy(t)) - require.Len(t, res.VerifyResults, 2) - require.NoError(t, res.Error) - }) + if tc.expectErr { + require.Error(t, res.Error) + require.ErrorContains(t, res.Error, "verifying with issuer \"sigstore.dev\"") + require.Nil(t, res.VerifyResults) + } else { + require.Len(t, len(tc.attestations), len(res.VerifyResults)) + require.NoError(t, res.Error) + } + } t.Run("with GitHub Sigstore artifact", func(t *testing.T) { githubArtifactPath := test.NormalizeRelativePath("../test/data/github_provenance_demo-0.0.12-py3-none-any.whl") @@ -72,34 +92,6 @@ func TestLiveSigstoreVerifier(t *testing.T) { require.Len(t, res.VerifyResults, 2) require.NoError(t, res.Error) }) - - t.Run("with invalid bundle version", func(t *testing.T) { - attestations := getAttestationsFor(t, "../test/data/sigstore-js-2.1.0-bundle-v0.1.json") - require.Len(t, attestations, 1) - - verifier := NewLiveSigstoreVerifier(SigstoreConfig{ - Logger: io.NewTestHandler(), - }) - - res := verifier.Verify(attestations, publicGoodPolicy(t)) - require.Len(t, res.VerifyResults, 0) - require.ErrorContains(t, res.Error, "unsupported bundle version") - }) - - t.Run("with no attestations", func(t *testing.T) { - attestations := []*api.Attestation{} - require.Len(t, attestations, 0) - - verifier := NewLiveSigstoreVerifier(SigstoreConfig{ - Logger: io.NewTestHandler(), - TrustedRoot: test.NormalizeRelativePath("../test/data/trusted_root.json"), - }) - - res := verifier.Verify(attestations, publicGoodPolicy(t)) - require.Len(t, res.VerifyResults, 0) - require.NotNil(t, res.Error) - }) - } func publicGoodPolicy(t *testing.T) verify.PolicyBuilder { From 4ec696dacd7a387f658c139a538ac414f2379033 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Mon, 28 Oct 2024 13:40:48 -0600 Subject: [PATCH 5/8] create common test fixture, organize tests Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/verify/options_test.go | 112 +++++++++------------ 1 file changed, 46 insertions(+), 66 deletions(-) diff --git a/pkg/cmd/attestation/verify/options_test.go b/pkg/cmd/attestation/verify/options_test.go index b9be054a5..77c0e3b23 100644 --- a/pkg/cmd/attestation/verify/options_test.go +++ b/pkg/cmd/attestation/verify/options_test.go @@ -13,29 +13,28 @@ var ( publicGoodBundlePath = test.NormalizeRelativePath("../test/data/psigstore-js-2.1.0-bundle.json") ) +var baseOptions = Options{ + ArtifactPath: publicGoodArtifactPath, + BundlePath: publicGoodBundlePath, + DigestAlgorithm: "sha512", + Limit: 1, + Owner: "sigstore", + OIDCIssuer: "some issuer", +} + func TestAreFlagsValid(t *testing.T) { t.Run("has invalid Repo value", func(t *testing.T) { - opts := Options{ - ArtifactPath: publicGoodArtifactPath, - DigestAlgorithm: "sha512", - OIDCIssuer: "some issuer", - Repo: "sigstoresigstore-js", - } + opts := baseOptions + opts.Repo = "sigstoresigstore-js" err := opts.AreFlagsValid() require.Error(t, err) require.ErrorContains(t, err, "invalid value provided for repo") }) - t.Run("invalid limit < 0", func(t *testing.T) { - opts := Options{ - ArtifactPath: publicGoodArtifactPath, - BundlePath: publicGoodBundlePath, - DigestAlgorithm: "sha512", - Owner: "sigstore", - OIDCIssuer: "some issuer", - Limit: 0, - } + t.Run("invalid limit == 0", func(t *testing.T) { + opts := baseOptions + opts.Limit = 0 err := opts.AreFlagsValid() require.Error(t, err) @@ -43,19 +42,43 @@ func TestAreFlagsValid(t *testing.T) { }) t.Run("invalid limit > 1000", func(t *testing.T) { - opts := Options{ - ArtifactPath: publicGoodArtifactPath, - BundlePath: publicGoodBundlePath, - DigestAlgorithm: "sha512", - Owner: "sigstore", - OIDCIssuer: "some issuer", - Limit: 1001, - } + opts := baseOptions + opts.Limit = 1001 err := opts.AreFlagsValid() require.Error(t, err) require.ErrorContains(t, err, "limit 1001 not allowed, must be between 1 and 1000") }) + + t.Run("returns error when UseBundleFromRegistry is true and ArtifactPath is not an OCI path", func(t *testing.T) { + opts := baseOptions + opts.BundlePath = "" + opts.UseBundleFromRegistry = true + + err := opts.AreFlagsValid() + require.Error(t, err) + require.ErrorContains(t, err, "bundle-from-oci flag can only be used with OCI artifact paths") + }) + + t.Run("does not return error when UseBundleFromRegistry is true and ArtifactPath is an OCI path", func(t *testing.T) { + opts := baseOptions + opts.ArtifactPath = "oci://sigstore/sigstore-js:2.1.0" + opts.BundlePath = "" + opts.UseBundleFromRegistry = true + + err := opts.AreFlagsValid() + require.NoError(t, err) + }) + + t.Run("returns error when UseBundleFromRegistry is true and BundlePath is provided", func(t *testing.T) { + opts := baseOptions + opts.ArtifactPath = "oci://sigstore/sigstore-js:2.1.0" + opts.UseBundleFromRegistry = true + + err := opts.AreFlagsValid() + require.Error(t, err) + require.ErrorContains(t, err, "bundle-from-oci flag cannot be used with bundle-path flag") + }) } func TestSetPolicyFlags(t *testing.T) { @@ -116,47 +139,4 @@ func TestSetPolicyFlags(t *testing.T) { require.Equal(t, "sigstore", opts.Owner) require.Equal(t, "^https://github/foo", opts.SANRegex) }) - - t.Run("returns error when UseBundleFromRegistry is true and ArtifactPath is not an OCI path", func(t *testing.T) { - opts := Options{ - ArtifactPath: publicGoodArtifactPath, - DigestAlgorithm: "sha512", - Owner: "sigstore", - UseBundleFromRegistry: true, - Limit: 1, - } - - err := opts.AreFlagsValid() - require.Error(t, err) - require.ErrorContains(t, err, "bundle-from-oci flag can only be used with OCI artifact paths") - }) - - t.Run("does not return error when UseBundleFromRegistry is true and ArtifactPath is an OCI path", func(t *testing.T) { - opts := Options{ - ArtifactPath: "oci://sigstore/sigstore-js:2.1.0", - DigestAlgorithm: "sha512", - OIDCIssuer: "some issuer", - Owner: "sigstore", - UseBundleFromRegistry: true, - Limit: 1, - } - - err := opts.AreFlagsValid() - require.NoError(t, err) - }) - - t.Run("returns error when UseBundleFromRegistry is true and BundlePath is provided", func(t *testing.T) { - opts := Options{ - ArtifactPath: "oci://sigstore/sigstore-js:2.1.0", - BundlePath: publicGoodBundlePath, - DigestAlgorithm: "sha512", - Owner: "sigstore", - UseBundleFromRegistry: true, - Limit: 1, - } - - err := opts.AreFlagsValid() - require.Error(t, err) - require.ErrorContains(t, err, "bundle-from-oci flag cannot be used with bundle-path flag") - }) } From 8a8f224a7ae36e01f15c48898f0f7be7625ba621 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Mon, 28 Oct 2024 15:28:00 -0600 Subject: [PATCH 6/8] fix test Signed-off-by: Meredith Lancaster --- .../verification/sigstore_integration_test.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/pkg/cmd/attestation/verification/sigstore_integration_test.go b/pkg/cmd/attestation/verification/sigstore_integration_test.go index d4a92c541..b7057505e 100644 --- a/pkg/cmd/attestation/verification/sigstore_integration_test.go +++ b/pkg/cmd/attestation/verification/sigstore_integration_test.go @@ -42,6 +42,8 @@ func TestLiveSigstoreVerifier(t *testing.T) { { name: "with no attestations", attestations: []*api.Attestation{}, + expectErr: true, + errContains: "no attestations were verified", }, } @@ -53,12 +55,12 @@ func TestLiveSigstoreVerifier(t *testing.T) { res := verifier.Verify(tc.attestations, publicGoodPolicy(t)) if tc.expectErr { - require.Error(t, res.Error) - require.ErrorContains(t, res.Error, "verifying with issuer \"sigstore.dev\"") - require.Nil(t, res.VerifyResults) + require.Error(t, res.Error, "test case: %s", tc.name) + require.ErrorContains(t, res.Error, tc.errContains, "test case: %s", tc.name) + require.Nil(t, res.VerifyResults, "test case: %s", tc.name) } else { - require.Len(t, len(tc.attestations), len(res.VerifyResults)) - require.NoError(t, res.Error) + require.Equal(t, len(tc.attestations), len(res.VerifyResults), "test case: %s", tc.name) + require.NoError(t, res.Error, "test case: %s", tc.name) } } From 15b2db9277735c7cfd715db9c05134b7bb64d23c Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Tue, 29 Oct 2024 10:25:04 -0400 Subject: [PATCH 7/8] Require visibility confirmation in `gh repo edit` This commit modifies interactive and non-interactive behaviors around `gh repo edit` as well as providing greater information about the impact. 1. `--help` usage is expanded to highlight the most significant consequences of changing visibility 1. `--help` usage and interactive experience call out GitHub Docs content that act as source of truth about full consequences of various changes 1. `gh repo edit` interactive experience will require confirmation for any visibility change 1. `gh repo edit` interactive experience will output potential stars and watchers lose regardless of visibility transition 1. `gh repo edit` will require `--visibility` flag to include new `--accept-visibility-change-consequences` flag regardless of interactivity --- pkg/cmd/repo/edit/edit.go | 59 ++++++++---- pkg/cmd/repo/edit/edit_test.go | 165 +++++++++++++++++++++++++++++++-- 2 files changed, 195 insertions(+), 29 deletions(-) diff --git a/pkg/cmd/repo/edit/edit.go b/pkg/cmd/repo/edit/edit.go index 508dea06c..29a149cf9 100644 --- a/pkg/cmd/repo/edit/edit.go +++ b/pkg/cmd/repo/edit/edit.go @@ -15,6 +15,7 @@ import ( fd "github.com/cli/cli/v2/internal/featuredetection" "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/internal/text" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" "github.com/cli/cli/v2/pkg/set" @@ -49,15 +50,16 @@ const ( ) type EditOptions struct { - HTTPClient *http.Client - Repository ghrepo.Interface - IO *iostreams.IOStreams - Edits EditRepositoryInput - AddTopics []string - RemoveTopics []string - InteractiveMode bool - Detector fd.Detector - Prompter iprompter + HTTPClient *http.Client + Repository ghrepo.Interface + IO *iostreams.IOStreams + Edits EditRepositoryInput + AddTopics []string + RemoveTopics []string + AcceptVisibilityChangeConsequences bool + InteractiveMode bool + Detector fd.Detector + Prompter iprompter // Cache of current repo topics to avoid retrieving them // in multiple flows. topicsCache []string @@ -103,7 +105,16 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(options *EditOptions) error) *cobr To toggle a setting off, use the %[1]s--=false%[1]s syntax. - Note that changing repository visibility to private will cause loss of stars and watchers. + Changing repository visibility can have unexpected consequences including but not limited to: + + - Losing stars and watchers, affecting repository ranking + - Detaching public forks from the network + - Disabling push rulesets + - Allowing access to GitHub Actions history and logs + + When the %[1]s--visibility%[1]s flag is used, %[1]s--accept-visibility-change-consequences%[1]s flag is required. + + For information on all the potential consequences, see `, "`"), Args: cobra.MaximumNArgs(1), Example: heredoc.Doc(` @@ -142,6 +153,10 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(options *EditOptions) error) *cobr return cmdutil.FlagErrorf("specify properties to edit when not running interactively") } + if opts.Edits.Visibility != nil && !opts.AcceptVisibilityChangeConsequences { + return cmdutil.FlagErrorf("use of --visibility flag requires --accept-visibility-change-consequences flag") + } + if runF != nil { return runF(opts) } @@ -167,6 +182,7 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(options *EditOptions) error) *cobr cmdutil.NilBoolFlag(cmd, &opts.Edits.AllowUpdateBranch, "allow-update-branch", "", "Allow a pull request head branch that is behind its base branch to be updated") cmd.Flags().StringSliceVar(&opts.AddTopics, "add-topic", nil, "Add repository topic") cmd.Flags().StringSliceVar(&opts.RemoveTopics, "remove-topic", nil, "Remove repository topic") + cmd.Flags().BoolVar(&opts.AcceptVisibilityChangeConsequences, "accept-visibility-change-consequences", false, "Accept the consequences of changing the repository visibility") return cmd } @@ -379,23 +395,26 @@ func interactiveRepoEdit(opts *EditOptions, r *api.Repository) error { } opts.Edits.EnableProjects = &a case optionVisibility: + cs := opts.IO.ColorScheme() + fmt.Fprintf(opts.IO.ErrOut, "%s Danger zone: changing repository visibility can have unexpected consequences; consult https://gh.io/setting-repository-visibility before continuing.\n", cs.WarningIcon()) + visibilityOptions := []string{"public", "private", "internal"} selected, err := p.Select("Visibility", strings.ToLower(r.Visibility), visibilityOptions) if err != nil { return err } - confirmed := true - if visibilityOptions[selected] == "private" && - (r.StargazerCount > 0 || r.Watchers.TotalCount > 0) { - cs := opts.IO.ColorScheme() - fmt.Fprintf(opts.IO.ErrOut, "%s Changing the repository visibility to private will cause permanent loss of stars and watchers.\n", cs.WarningIcon()) - confirmed, err = p.Confirm("Do you want to change visibility to private?", false) - if err != nil { - return err - } + selectedVisibility := visibilityOptions[selected] + + if selectedVisibility != r.Visibility && (r.StargazerCount > 0 || r.Watchers.TotalCount > 0) { + fmt.Fprintf(opts.IO.ErrOut, "%s Changing the repository visibility to %s will cause permanent loss of %s and %s.\n", cs.WarningIcon(), selectedVisibility, text.Pluralize(r.StargazerCount, "star"), text.Pluralize(r.Watchers.TotalCount, "watcher")) + } + + confirmed, err := p.Confirm(fmt.Sprintf("Do you want to change visibility to %s?", selectedVisibility), false) + if err != nil { + return err } if confirmed { - opts.Edits.Visibility = &visibilityOptions[selected] + opts.Edits.Visibility = &selectedVisibility } case optionMergeOptions: var defaultMergeOptions []string diff --git a/pkg/cmd/repo/edit/edit_test.go b/pkg/cmd/repo/edit/edit_test.go index 07f7d4d55..728f19d45 100644 --- a/pkg/cmd/repo/edit/edit_test.go +++ b/pkg/cmd/repo/edit/edit_test.go @@ -34,6 +34,63 @@ func TestNewCmdEdit(t *testing.T) { }, }, }, + { + name: "deny public visibility change without accepting consequences", + args: "--visibility public", + wantOpts: EditOptions{ + Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), + Edits: EditRepositoryInput{}, + }, + wantErr: "use of --visibility flag requires --accept-visibility-change-consequences flag", + }, + { + name: "allow public visibility change with accepting consequences", + args: "--visibility public --accept-visibility-change-consequences", + wantOpts: EditOptions{ + Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), + Edits: EditRepositoryInput{ + Visibility: sp("public"), + }, + }, + }, + { + name: "deny private visibility change without accepting consequences", + args: "--visibility private", + wantOpts: EditOptions{ + Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), + Edits: EditRepositoryInput{}, + }, + wantErr: "use of --visibility flag requires --accept-visibility-change-consequences flag", + }, + { + name: "allow private visibility change with accepting consequences", + args: "--visibility private --accept-visibility-change-consequences", + wantOpts: EditOptions{ + Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), + Edits: EditRepositoryInput{ + Visibility: sp("private"), + }, + }, + }, + { + name: "deny internal visibility change without accepting consequences", + args: "--visibility internal", + wantOpts: EditOptions{ + Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), + Edits: EditRepositoryInput{}, + }, + wantErr: "use of --visibility flag requires --accept-visibility-change-consequences flag", + }, + { + name: "allow internal visibility change with accepting consequences", + args: "--visibility internal --accept-visibility-change-consequences", + wantOpts: EditOptions{ + Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), + Edits: EditRepositoryInput{ + Visibility: sp("internal"), + }, + }, + }, } for _, tt := range tests { @@ -241,6 +298,104 @@ func Test_editRun_interactive(t *testing.T) { })) }, }, + { + name: "skipping visibility without confirmation", + opts: EditOptions{ + Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), + InteractiveMode: true, + }, + promptStubs: func(pm *prompter.MockPrompter) { + pm.RegisterMultiSelect("What do you want to edit?", nil, editList, + func(_ string, _, opts []string) ([]int, error) { + return []int{8}, nil + }) + pm.RegisterSelect("Visibility", []string{"public", "private", "internal"}, + func(_, _ string, opts []string) (int, error) { + return prompter.IndexFor(opts, "private") + }) + pm.RegisterConfirm("Do you want to change visibility to private?", func(_ string, _ bool) (bool, error) { + return false, nil + }) + }, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query RepositoryInfo\b`), + httpmock.StringResponse(` + { + "data": { + "repository": { + "visibility": "public", + "description": "description", + "homePageUrl": "https://url.com", + "defaultBranchRef": { + "name": "main" + }, + "stargazerCount": 10, + "isInOrganization": false, + "repositoryTopics": { + "nodes": [{ + "topic": { + "name": "x" + } + }] + } + } + } + }`)) + reg.Exclude(t, httpmock.REST("PATCH", "repos/OWNER/REPO")) + }, + }, + { + name: "changing visibility with confirmation", + opts: EditOptions{ + Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), + InteractiveMode: true, + }, + promptStubs: func(pm *prompter.MockPrompter) { + pm.RegisterMultiSelect("What do you want to edit?", nil, editList, + func(_ string, _, opts []string) ([]int, error) { + return []int{8}, nil + }) + pm.RegisterSelect("Visibility", []string{"public", "private", "internal"}, + func(_, _ string, opts []string) (int, error) { + return prompter.IndexFor(opts, "private") + }) + pm.RegisterConfirm("Do you want to change visibility to private?", func(_ string, _ bool) (bool, error) { + return true, nil + }) + }, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL(`query RepositoryInfo\b`), + httpmock.StringResponse(` + { + "data": { + "repository": { + "visibility": "public", + "description": "description", + "homePageUrl": "https://url.com", + "defaultBranchRef": { + "name": "main" + }, + "stargazerCount": 10, + "isInOrganization": false, + "repositoryTopics": { + "nodes": [{ + "topic": { + "name": "x" + } + }] + } + } + } + }`)) + reg.Register( + httpmock.REST("PATCH", "repos/OWNER/REPO"), + httpmock.RESTPayload(200, `{}`, func(payload map[string]interface{}) { + assert.Equal(t, "private", payload["visibility"]) + })) + }, + }, { name: "the rest", opts: EditOptions{ @@ -250,7 +405,7 @@ func Test_editRun_interactive(t *testing.T) { promptStubs: func(pm *prompter.MockPrompter) { pm.RegisterMultiSelect("What do you want to edit?", nil, editList, func(_ string, _, opts []string) ([]int, error) { - return []int{0, 2, 3, 5, 6, 8, 9}, nil + return []int{0, 2, 3, 5, 6, 9}, nil }) pm.RegisterInput("Default branch name", func(_, _ string) (string, error) { return "trunk", nil @@ -267,13 +422,6 @@ func Test_editRun_interactive(t *testing.T) { pm.RegisterConfirm("Convert into a template repository?", func(_ string, _ bool) (bool, error) { return true, nil }) - pm.RegisterSelect("Visibility", []string{"public", "private", "internal"}, - func(_, _ string, opts []string) (int, error) { - return prompter.IndexFor(opts, "private") - }) - pm.RegisterConfirm("Do you want to change visibility to private?", func(_ string, _ bool) (bool, error) { - return true, nil - }) pm.RegisterConfirm("Enable Wikis?", func(_ string, _ bool) (bool, error) { return true, nil }) @@ -310,7 +458,6 @@ func Test_editRun_interactive(t *testing.T) { assert.Equal(t, "https://zombo.com", payload["homepage"]) assert.Equal(t, true, payload["has_issues"]) assert.Equal(t, true, payload["has_projects"]) - assert.Equal(t, "private", payload["visibility"]) assert.Equal(t, true, payload["is_template"]) assert.Equal(t, true, payload["has_wiki"]) })) From 3f5fc85e41e8b664bed033cbf79e2c0104ffbff3 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Wed, 30 Oct 2024 13:31:00 -0400 Subject: [PATCH 8/8] Assert stderr for gh repo edit visibility tests --- pkg/cmd/repo/edit/edit_test.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/repo/edit/edit_test.go b/pkg/cmd/repo/edit/edit_test.go index 728f19d45..217c1dce4 100644 --- a/pkg/cmd/repo/edit/edit_test.go +++ b/pkg/cmd/repo/edit/edit_test.go @@ -344,6 +344,7 @@ func Test_editRun_interactive(t *testing.T) { }`)) reg.Exclude(t, httpmock.REST("PATCH", "repos/OWNER/REPO")) }, + wantsStderr: "Changing the repository visibility to private will cause permanent loss of 10 stars and 0 watchers.", }, { name: "changing visibility with confirmation", @@ -378,6 +379,9 @@ func Test_editRun_interactive(t *testing.T) { "name": "main" }, "stargazerCount": 10, + "watchers": { + "totalCount": 15 + }, "isInOrganization": false, "repositoryTopics": { "nodes": [{ @@ -395,6 +399,7 @@ func Test_editRun_interactive(t *testing.T) { assert.Equal(t, "private", payload["visibility"]) })) }, + wantsStderr: "Changing the repository visibility to private will cause permanent loss of 10 stars and 15 watchers", }, { name: "the rest", @@ -631,7 +636,7 @@ func Test_editRun_interactive(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ios, _, _, _ := iostreams.Test() + ios, _, _, stderr := iostreams.Test() ios.SetStdoutTTY(true) ios.SetStdinTTY(true) ios.SetStderrTTY(true) @@ -656,9 +661,11 @@ func Test_editRun_interactive(t *testing.T) { if tt.wantsErr == "" { require.NoError(t, err) } else { - assert.EqualError(t, err, tt.wantsErr) + require.EqualError(t, err, tt.wantsErr) return } + + assert.Contains(t, stderr.String(), tt.wantsStderr) }) } }