RemoveTarget now takes an optional variadic list of roles to remove from.

If none are provided, it defaults to the targets role, as before.

Signed-off-by: Ying Li <ying.li@docker.com>

client/client.go

@@ -259,9 +259,48 @@ func (r *NotaryRepository) Initialize(rootKeyID string, serverManagedRoles ...st
 	return r.saveMetadata(serverManagesSnapshot)
 }
 
-// AddTarget adds a new target to the repository to the given roles, forcing a
+// adds a TUF Change template to the given roles
-// timestamps check from TUF. If roles are unspecified, AddTarget the default
+func addChange(cl *changelist.FileChangelist, c changelist.Change,
-// role is "target"
+	roles ...string) error {
+	if len(roles) == 0 {
+		roles = []string{data.CanonicalTargetsRole}
+	}
+
+	var changes []changelist.Change
+	for _, role := range roles {
+		role = strings.ToLower(role)
+
+		if !data.ValidRole(role) {
+			return data.ErrInvalidRole{Role: role}
+		}
+
+		if _, ok := data.ValidRoles[role]; ok && role != data.CanonicalTargetsRole {
+			return data.ErrInvalidRole{
+				Role:   role,
+				Reason: "cannot add targets to this role",
+			}
+		}
+
+		changes = append(changes, changelist.NewTufChange(
+			c.Action(),
+			role,
+			c.Type(),
+			c.Path(),
+			c.Content(),
+		))
+	}
+
+	for _, c := range changes {
+		if err := cl.Add(c); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+// AddTarget creates new changelist entries to add a target to the given roles
+// in the repository when the changelist gets appied at publish time.
+// If roles are unspecified, the default role is "target".
 func (r *NotaryRepository) AddTarget(target *Target, roles ...string) error {
 	cl, err := changelist.NewFileChangelist(filepath.Join(r.tufRepoPath, "changelist"))
 	if err != nil {
@@ -276,58 +315,24 @@ func (r *NotaryRepository) AddTarget(target *Target, roles ...string) error {
 		return err
 	}
 
-	if len(roles) == 0 {
+	template := changelist.NewTufChange(
-		roles = []string{data.CanonicalTargetsRole}
+		changelist.ActionCreate, "", changelist.TypeTargetsTarget,
-	}
+		target.Name, metaJSON)
-
+	return addChange(cl, template, roles...)
-	var changes []*changelist.TufChange
-	for _, role := range roles {
-		role = strings.ToLower(role)
-
-		if !data.ValidRole(role) {
-			return data.ErrInvalidRole{Role: role}
-		}
-
-		_, ok := data.ValidRoles[role]
-		if ok && role != data.CanonicalTargetsRole {
-			return data.ErrInvalidRole{
-				Role:   role,
-				Reason: "cannot add targets to this role",
-			}
-		}
-
-		changes = append(changes, changelist.NewTufChange(
-			changelist.ActionCreate,
-			role,
-			changelist.TypeTargetsTarget,
-			target.Name,
-			metaJSON,
-		))
-	}
-
-	for _, c := range changes {
-		err = cl.Add(c)
-		if err != nil {
-			return err
-		}
-	}
-	return nil
 }
 
-// RemoveTarget creates a new changelist entry to remove a target from the repository
+// RemoveTarget creates new changelist entries to remove a target from the given
-// when the changelist gets applied at publish time
+// roles in the repository when the changelist gets applied at publish time.
-func (r *NotaryRepository) RemoveTarget(targetName string) error {
+// If roles are unspecified, the default role is "target".
+func (r *NotaryRepository) RemoveTarget(targetName string, roles ...string) error {
 	cl, err := changelist.NewFileChangelist(filepath.Join(r.tufRepoPath, "changelist"))
 	if err != nil {
 		return err
 	}
 	logrus.Debugf("Removing target \"%s\"", targetName)
-	c := changelist.NewTufChange(changelist.ActionDelete, changelist.ScopeTargets, "target", targetName, nil)
+	template := changelist.NewTufChange(changelist.ActionDelete, "",
-	err = cl.Add(c)
+		changelist.TypeTargetsTarget, targetName, nil)
-	if err != nil {
+	return addChange(cl, template, roles...)
-		return err
-	}
-	return nil
 }
 
 // ListTargets lists all targets for the current repository

client/client_test.go

@@ -427,17 +427,6 @@ func testInitRepoPasswordInvalid(t *testing.T, rootType string) {
 	assert.EqualError(t, err, trustmanager.ErrPasswordInvalid{}.Error())
 }
 
-// TestAddTargetToTargetRoleByDefault adds a target without specifying a role
-// to a repo without delegations.  Confirms that the changelist is created
-// correctly, for the targets scope.
-// We test this with both an RSA and ECDSA root key
-func TestAddTargetToTargetRoleByDefault(t *testing.T) {
-	testAddTargetToTargetRoleByDefault(t, data.ECDSAKey)
-	if !testing.Short() {
-		testAddTargetToTargetRoleByDefault(t, data.RSAKey)
-	}
-}
-
 func addTarget(t *testing.T, repo *NotaryRepository, targetName, targetFile string,
 	roles ...string) *Target {
 	target, err := NewTarget(targetName, targetFile)
@@ -454,7 +443,10 @@ func getChanges(t *testing.T, repo *NotaryRepository) []changelist.Change {
 	return changeList.List()
 }
 
-func testAddTargetToTargetRoleByDefault(t *testing.T, rootType string) {
+// TestAddTargetToTargetRoleByDefault adds a target without specifying a role
+// to a repo without delegations.  Confirms that the changelist is created
+// correctly, for the targets scope.
+func TestAddTargetToTargetRoleByDefault(t *testing.T) {
 	// Temporary directory where test files will be created
 	tempBaseDir, err := ioutil.TempDir("", "notary-test-")
 	defer os.RemoveAll(tempBaseDir)
@@ -466,45 +458,58 @@ func testAddTargetToTargetRoleByDefault(t *testing.T, rootType string) {
 	ts, _, _ := simpleTestServer(t)
 	defer ts.Close()
 
-	repo, _ := initializeRepo(t, rootType, tempBaseDir, gun, ts.URL, false)
+	repo, _ := initializeRepo(t, data.ECDSAKey, tempBaseDir, gun, ts.URL, false)
 
-	// tests need to manually boostrap timestamp as client doesn't generate it
+	testAddOrDeleteTarget(t, repo, changelist.ActionCreate, nil,
-	err = repo.tufRepo.InitTimestamp()
+		[]string{data.CanonicalTargetsRole})
-	assert.NoError(t, err, "error creating repository: %s", err)
-
-	testAddTargetToRepo(t, repo, nil, []string{data.CanonicalTargetsRole})
 }
 
-// Tests that adding a target to a repo, with the given roles, adds it to the
+// Tests that adding a target to a repo or deleting a target from a repo,
-// expected scopes
+// with the given roles, makes a change to the expected scopes
-func testAddTargetToRepo(t *testing.T, repo *NotaryRepository,
+func testAddOrDeleteTarget(t *testing.T, repo *NotaryRepository, action string,
-	addToRoles []string, expectedScopes []string) {
+	rolesToChange []string, expectedScopes []string) {
 
 	assert.Len(t, getChanges(t, repo), 0, "should start with zero changes")
 
-	// Add fixtures/intermediate-ca.crt as a target. There's no particular
+	if action == changelist.ActionCreate {
-	// reason for using this file except that it happens to be available as
+		// Add fixtures/intermediate-ca.crt as a target. There's no particular
-	// a fixture.
+		// reason for using this file except that it happens to be available as
-	addTarget(t, repo, "latest", "../fixtures/intermediate-ca.crt", addToRoles...)
+		// a fixture.
+		addTarget(t, repo, "latest", "../fixtures/intermediate-ca.crt", rolesToChange...)
+	} else {
+		err := repo.RemoveTarget("latest", rolesToChange...)
+		assert.NoError(t, err, "error removing target")
+	}
+
 	changes := getChanges(t, repo)
 	assert.Len(t, changes, len(expectedScopes), "wrong number of changes files found")
 
 	foundScopes := make(map[string]bool)
 	for _, c := range changes { // there is only one
-		assert.EqualValues(t, changelist.ActionCreate, c.Action())
+		assert.EqualValues(t, action, c.Action())
 		foundScopes[c.Scope()] = true
 		assert.Equal(t, "target", c.Type())
 		assert.Equal(t, "latest", c.Path())
-		assert.NotEmpty(t, c.Content())
+		if action == changelist.ActionCreate {
+			assert.NotEmpty(t, c.Content())
+		} else {
+			assert.Empty(t, c.Content())
+		}
 	}
 	assert.Len(t, foundScopes, len(expectedScopes))
 	for _, expectedScope := range expectedScopes {
 		_, ok := foundScopes[expectedScope]
-		assert.True(t, ok, "Target was not added to %s", expectedScope)
+		assert.True(t, ok, "Target was not added/removed from %s", expectedScope)
+	}
+
+	// add/delete a second time
+	if action == changelist.ActionCreate {
+		addTarget(t, repo, "current", "../fixtures/intermediate-ca.crt", rolesToChange...)
+	} else {
+		err := repo.RemoveTarget("current", rolesToChange...)
+		assert.NoError(t, err, "error removing target")
 	}
 
-	// Create a second target
-	addTarget(t, repo, "current", "../fixtures/intermediate-ca.crt", addToRoles...)
 	changes = getChanges(t, repo)
 	assert.Len(t, changes, 2*len(expectedScopes),
 		"wrong number of changelist files found")
@@ -513,11 +518,15 @@ func testAddTargetToRepo(t *testing.T, repo *NotaryRepository,
 	foundScopes = make(map[string]bool)
 	for _, c := range changes {
 		if c.Path() != "latest" {
-			assert.EqualValues(t, changelist.ActionCreate, c.Action())
+			assert.EqualValues(t, action, c.Action())
 			foundScopes[c.Scope()] = true
 			assert.Equal(t, "target", c.Type())
 			assert.Equal(t, "current", c.Path())
-			assert.NotEmpty(t, c.Content())
+			if action == changelist.ActionCreate {
+				assert.NotEmpty(t, c.Content())
+			} else {
+				assert.Empty(t, c.Content())
+			}
 
 			newFileFound = true
 		}
@@ -526,22 +535,14 @@ func testAddTargetToRepo(t *testing.T, repo *NotaryRepository,
 	assert.Len(t, foundScopes, len(expectedScopes))
 	for _, expectedScope := range expectedScopes {
 		_, ok := foundScopes[expectedScope]
-		assert.True(t, ok, "Target was not added to %s", expectedScope)
+		assert.True(t, ok, "Target was not added/removed from %s", expectedScope)
 	}
 }
 
 // TestAddTargetToSpecifiedValidRoles adds a target to the specified roles.
 // Confirms that the changelist is created correctly, one for each of the
 // the specified roles as scopes.
-// We test this with both an RSA and ECDSA root key
 func TestAddTargetToSpecifiedValidRoles(t *testing.T) {
-	testAddTargetToSpecifiedRoles(t, data.ECDSAKey)
-	if !testing.Short() {
-		testAddTargetToSpecifiedRoles(t, data.RSAKey)
-	}
-}
-
-func testAddTargetToSpecifiedRoles(t *testing.T, keyType string) {
 	// Temporary directory where test files will be created
 	tempBaseDir, err := ioutil.TempDir("", "notary-test-")
 	defer os.RemoveAll(tempBaseDir)
@@ -553,21 +554,10 @@ func testAddTargetToSpecifiedRoles(t *testing.T, keyType string) {
 	ts, _, _ := simpleTestServer(t)
 	defer ts.Close()
 
-	repo, _ := initializeRepo(t, keyType, tempBaseDir, gun, ts.URL, false)
+	repo, _ := initializeRepo(t, data.ECDSAKey, tempBaseDir, gun, ts.URL, false)
 
-	// tests need to manually boostrap timestamp as client doesn't generate it
-	err = repo.tufRepo.InitTimestamp()
-	assert.NoError(t, err, "error creating repository: %s", err)
-
-	// add a delegated role
 	roleName := filepath.Join(data.CanonicalTargetsRole, "a")
-	delegationKey, err := repo.CryptoService.Create(roleName, keyType)
+	testAddOrDeleteTarget(t, repo, changelist.ActionCreate,
-	assert.NoError(t, err)
-	newRole, err := data.NewRole(roleName, 1, nil, nil, nil)
-	assert.NoError(t, err)
-	repo.tufRepo.UpdateDelegations(newRole, []data.PublicKey{delegationKey})
-
-	testAddTargetToRepo(t, repo,
 		[]string{
 			strings.ToUpper(data.CanonicalTargetsRole),
 			strings.ToUpper(roleName),
@@ -617,6 +607,7 @@ func TestAddTargetToSpecifiedInvalidRoles(t *testing.T) {
 func TestAddTargetErrorWritingChanges(t *testing.T) {
 	// Temporary directory where test files will be created
 	tempBaseDir, err := ioutil.TempDir("", "notary-test-")
+	defer os.RemoveAll(tempBaseDir)
 	assert.NoError(t, err, "failed to create a temporary directory: %s", err)
 
 	gun := "docker.com/notary"
@@ -629,15 +620,152 @@ func TestAddTargetErrorWritingChanges(t *testing.T) {
 	target, err := NewTarget("latest", "../fixtures/intermediate-ca.crt")
 	assert.NoError(t, err, "error creating target")
 
-	os.RemoveAll(tempBaseDir)
+	// first, make the actual changefile unwritable by making the changelist
-	// ioutil.WriteFile seems to create all the directories necessary, so make
+	// directory unwritable
-	// it impossible to do so
+	changelistPath := filepath.Join(repo.tufRepoPath, "changelist")
-	ioutil.WriteFile(tempBaseDir, []byte("hi"), 0644)
+	err = os.MkdirAll(changelistPath, 0744)
-	defer os.Remove(tempBaseDir)
+	assert.NoError(t, err, "could not create changelist dir")
+	err = os.Chmod(changelistPath, 0600)
+	assert.NoError(t, err, "could not change permission of changelist dir")
 
 	err = repo.AddTarget(target, data.CanonicalTargetsRole)
 	assert.Error(t, err, "Expected an error writing the change")
 	assert.IsType(t, &os.PathError{}, err)
+
+	// then break prevent the changlist directory from being able to be created
+	err = os.Chmod(changelistPath, 0744)
+	assert.NoError(t, err, "could not change permission of temp dir")
+	err = os.RemoveAll(changelistPath)
+	assert.NoError(t, err, "could not remove changelist dir")
+	// creating a changelist file so the directory can't be created
+	err = ioutil.WriteFile(changelistPath, []byte("hi"), 0644)
+	assert.NoError(t, err, "could not write temporary file")
+
+	err = repo.AddTarget(target, data.CanonicalTargetsRole)
+	assert.Error(t, err, "Expected an error writing the change")
+	assert.IsType(t, &os.PathError{}, err)
+}
+
+// TestRemoveTargetToTargetRoleByDefault removes a target without specifying a
+// role from a repo.  Confirms that the changelist is created correctly for
+// the targets scope.
+func TestRemoveTargetToTargetRoleByDefault(t *testing.T) {
+	// Temporary directory where test files will be created
+	tempBaseDir, err := ioutil.TempDir("", "notary-test-")
+	defer os.RemoveAll(tempBaseDir)
+
+	assert.NoError(t, err, "failed to create a temporary directory: %s", err)
+
+	gun := "docker.com/notary"
+
+	ts, _, _ := simpleTestServer(t)
+	defer ts.Close()
+
+	repo, _ := initializeRepo(t, data.ECDSAKey, tempBaseDir, gun, ts.URL, false)
+
+	testAddOrDeleteTarget(t, repo, changelist.ActionDelete, nil,
+		[]string{data.CanonicalTargetsRole})
+}
+
+// TestRemoveTargetFromSpecifiedValidRoles removes a target from the specified
+// roles. Confirms that the changelist is created correctly, one for each of
+// the the specified roles as scopes.
+func TestRemoveTargetFromSpecifiedValidRoles(t *testing.T) {
+	// Temporary directory where test files will be created
+	tempBaseDir, err := ioutil.TempDir("", "notary-test-")
+	defer os.RemoveAll(tempBaseDir)
+
+	assert.NoError(t, err, "failed to create a temporary directory: %s", err)
+
+	gun := "docker.com/notary"
+
+	ts, _, _ := simpleTestServer(t)
+	defer ts.Close()
+
+	repo, _ := initializeRepo(t, data.ECDSAKey, tempBaseDir, gun, ts.URL, false)
+
+	roleName := filepath.Join(data.CanonicalTargetsRole, "a")
+	testAddOrDeleteTarget(t, repo, changelist.ActionDelete,
+		[]string{
+			strings.ToUpper(data.CanonicalTargetsRole),
+			strings.ToUpper(roleName),
+		},
+		[]string{data.CanonicalTargetsRole, roleName})
+}
+
+// TestRemoveTargetFromSpecifiedInvalidRoles expects errors to be returned if
+// removing a target to an invalid role.  If any of the roles are invalid,
+// no targets are removed from any roles.
+func TestRemoveTargetToSpecifiedInvalidRoles(t *testing.T) {
+	// Temporary directory where test files will be created
+	tempBaseDir, err := ioutil.TempDir("", "notary-test-")
+	assert.NoError(t, err, "failed to create a temporary directory: %s", err)
+	defer os.RemoveAll(tempBaseDir)
+
+	gun := "docker.com/notary"
+
+	ts, _, _ := simpleTestServer(t)
+	defer ts.Close()
+
+	repo, _ := initializeRepo(t, data.ECDSAKey, tempBaseDir, gun, ts.URL, false)
+
+	invalidRoles := []string{
+		data.CanonicalRootRole,
+		data.CanonicalSnapshotRole,
+		data.CanonicalTimestampRole,
+		"target/otherrole",
+		"otherrole",
+	}
+
+	for _, invalidRole := range invalidRoles {
+		err = repo.RemoveTarget(data.CanonicalTargetsRole, invalidRole)
+		assert.Error(t, err, "Expected an ErrInvalidRole error")
+		assert.IsType(t, data.ErrInvalidRole{}, err)
+
+		changes := getChanges(t, repo)
+		assert.Len(t, changes, 0)
+	}
+}
+
+// TestRemoveTargetErrorWritingChanges expects errors writing a change to file
+// to be propagated.
+func TestRemoveTargetErrorWritingChanges(t *testing.T) {
+	// Temporary directory where test files will be created
+	tempBaseDir, err := ioutil.TempDir("", "notary-test-")
+	defer os.Remove(tempBaseDir)
+	assert.NoError(t, err, "failed to create a temporary directory: %s", err)
+
+	gun := "docker.com/notary"
+
+	ts, _, _ := simpleTestServer(t)
+	defer ts.Close()
+
+	repo, _ := initializeRepo(t, data.ECDSAKey, tempBaseDir, gun, ts.URL, false)
+
+	// first, make the actual changefile unwritable by making the changelist
+	// directory unwritable
+	changelistPath := filepath.Join(repo.tufRepoPath, "changelist")
+	err = os.MkdirAll(changelistPath, 0744)
+	assert.NoError(t, err, "could not create changelist dir")
+	err = os.Chmod(changelistPath, 0600)
+	assert.NoError(t, err, "could not change permission of changelist dir")
+
+	err = repo.RemoveTarget(data.CanonicalTargetsRole)
+	assert.Error(t, err, "Expected an error writing the change")
+	assert.IsType(t, &os.PathError{}, err)
+
+	// then break prevent the changlist directory from being able to be created
+	err = os.Chmod(changelistPath, 0744)
+	assert.NoError(t, err, "could not change permission of temp dir")
+	err = os.RemoveAll(changelistPath)
+	assert.NoError(t, err, "could not remove changelist dir")
+	// creating a changelist file so the directory can't be created
+	err = ioutil.WriteFile(changelistPath, []byte("hi"), 0644)
+	assert.NoError(t, err, "could not write temporary file")
+
+	err = repo.RemoveTarget(data.CanonicalTargetsRole)
+	assert.Error(t, err, "Expected an error writing the change")
+	assert.IsType(t, &os.PathError{}, err)
 }
 
 // TestListTarget fakes serving signed metadata files over the test's