diff --git a/cli/command/trust/signer_remove.go b/cli/command/trust/signer_remove.go index 27774632bf..d4e8eec42b 100644 --- a/cli/command/trust/signer_remove.go +++ b/cli/command/trust/signer_remove.go @@ -43,11 +43,9 @@ func removeSigner(cli command.Cli, options signerRemoveOptions) error { var errRepos []string for _, repo := range options.repos { fmt.Fprintf(cli.Out(), "Removing signer \"%s\" from %s...\n", options.signer, repo) - if err := removeSingleSigner(cli, repo, options.signer, options.forceYes); err != nil { + if _, err := removeSingleSigner(cli, repo, options.signer, options.forceYes); err != nil { fmt.Fprintln(cli.Err(), err.Error()+"\n") errRepos = append(errRepos, repo) - } else { - fmt.Fprintf(cli.Out(), "Successfully removed %s from %s\n\n", options.signer, repo) } } if len(errRepos) > 0 { @@ -78,24 +76,26 @@ func isLastSignerForReleases(roleWithSig data.Role, allRoles []client.RoleWithSi return counter < releasesRoleWithSigs.Threshold, nil } -func removeSingleSigner(cli command.Cli, repoName, signerName string, forceYes bool) error { +// removeSingleSigner attempts to remove a single signer and returns whether signer removal happened. +// The signer not being removed doesn't necessarily raise an error e.g. user choosing "No" when prompted for confirmation. +func removeSingleSigner(cli command.Cli, repoName, signerName string, forceYes bool) (bool, error) { ctx := context.Background() imgRefAndAuth, err := trust.GetImageReferencesAndAuth(ctx, nil, image.AuthResolver(cli), repoName) if err != nil { - return err + return false, err } signerDelegation := data.RoleName("targets/" + signerName) if signerDelegation == releasesRoleTUFName { - return fmt.Errorf("releases is a reserved keyword and cannot be removed") + return false, fmt.Errorf("releases is a reserved keyword and cannot be removed") } notaryRepo, err := cli.NotaryClient(imgRefAndAuth, trust.ActionsPushAndPull) if err != nil { - return trust.NotaryError(imgRefAndAuth.Reference().Name(), err) + return false, trust.NotaryError(imgRefAndAuth.Reference().Name(), err) } delegationRoles, err := notaryRepo.GetDelegationRoles() if err != nil { - return errors.Wrapf(err, "error retrieving signers for %s", repoName) + return false, errors.Wrapf(err, "error retrieving signers for %s", repoName) } var role data.Role for _, delRole := range delegationRoles { @@ -105,11 +105,11 @@ func removeSingleSigner(cli command.Cli, repoName, signerName string, forceYes b } } if role.Name == "" { - return fmt.Errorf("No signer %s for repository %s", signerName, repoName) + return false, fmt.Errorf("No signer %s for repository %s", signerName, repoName) } allRoles, err := notaryRepo.ListRoles() if err != nil { - return err + return false, err } if ok, err := isLastSignerForReleases(role, allRoles); ok && !forceYes { removeSigner := command.PromptForConfirmation(os.Stdin, cli.Out(), fmt.Sprintf("The signer \"%s\" signed the last released version of %s. "+ @@ -120,16 +120,23 @@ func removeSingleSigner(cli command.Cli, repoName, signerName string, forceYes b if !removeSigner { fmt.Fprintf(cli.Out(), "\nAborting action.\n") - return nil + return false, nil } } else if err != nil { - return err + return false, err } if err = notaryRepo.RemoveDelegationKeys(releasesRoleTUFName, role.KeyIDs); err != nil { - return err + return false, err } if err = notaryRepo.RemoveDelegationRole(signerDelegation); err != nil { - return err + return false, err } - return notaryRepo.Publish() + + if err = notaryRepo.Publish(); err != nil { + return false, err + } + + fmt.Fprintf(cli.Out(), "Successfully removed %s from %s\n\n", signerName, repoName) + + return true, nil } diff --git a/cli/command/trust/signer_remove_test.go b/cli/command/trust/signer_remove_test.go index f86bd84018..0feec87430 100644 --- a/cli/command/trust/signer_remove_test.go +++ b/cli/command/trust/signer_remove_test.go @@ -71,10 +71,13 @@ func TestTrustSignerRemoveErrors(t *testing.T) { func TestRemoveSingleSigner(t *testing.T) { cli := test.NewFakeCli(&fakeClient{}) cli.SetNotaryClient(notaryfake.GetLoadedNotaryRepository) - err := removeSingleSigner(cli, "signed-repo", "test", true) + removed, err := removeSingleSigner(cli, "signed-repo", "test", true) assert.Error(t, err, "No signer test for repository signed-repo") - err = removeSingleSigner(cli, "signed-repo", "releases", true) + assert.Equal(t, removed, false, "No signer should be removed") + + removed, err = removeSingleSigner(cli, "signed-repo", "releases", true) assert.Error(t, err, "releases is a reserved keyword and cannot be removed") + assert.Equal(t, removed, false, "No signer should be removed") } func TestRemoveMultipleSigners(t *testing.T) {