Support clearing an identity alias' custom_metadata (hashicorp#13395)

* Support clearing an identity alias' custom_metadata

Previously, an update to an entity alias supported updating the
custom_metadata as long as the update was not empty, which makes it
impossible to clear the metadata values completely.

Fixes:
- empty custom_metadata parameters are honoured on entity alias update
- update related tests
- drop dependency on mapstructure
- reformat with gofumpt

changelog/13395.txt

@@ -0,0 +1,3 @@
+```release-note:improvement
+core/identity: Support updating an alias' `custom_metadata` to be empty.
+```

vault/identity_store_aliases.go

@@ -8,18 +8,20 @@ import (
 	"github.com/golang/protobuf/ptypes"
 	"github.com/hashicorp/go-multierror"
 	"github.com/hashicorp/go-secure-stdlib/strutil"
+
 	"github.com/hashicorp/vault/helper/identity"
 	"github.com/hashicorp/vault/helper/namespace"
 	"github.com/hashicorp/vault/helper/storagepacker"
 	"github.com/hashicorp/vault/sdk/framework"
 	"github.com/hashicorp/vault/sdk/logical"
-	"github.com/mitchellh/mapstructure"
 )
 
-const maxCustomMetadataKeys = 64
-const maxCustomMetadataKeyLength = 128
-const maxCustomMetadataValueLength = 512
-const customMetadataValidationErrorPrefix = "custom_metadata validation failed"
+const (
+	maxCustomMetadataKeys               = 64
+	maxCustomMetadataKeyLength          = 128
+	maxCustomMetadataValueLength        = 512
+	customMetadataValidationErrorPrefix = "custom_metadata validation failed"
+)
 
 // aliasPaths returns the API endpoints to operate on aliases.
 // Following are the paths supported:
@@ -138,10 +140,7 @@ func (i *IdentityStore) handleAliasCreateUpdate() framework.OperationFunc {
 		customMetadata := make(map[string]string)
 		data, customMetadataExists := d.GetOk("custom_metadata")
 		if customMetadataExists {
-			err = mapstructure.Decode(data, &customMetadata)
-			if err != nil {
-				return nil, err
-			}
+			customMetadata = data.(map[string]string)
 		}
 
 		// Get entity id
@@ -151,11 +150,9 @@ func (i *IdentityStore) handleAliasCreateUpdate() framework.OperationFunc {
 			canonicalID = d.Get("entity_id").(string)
 		}
 
-		//validate customMetadata if provided
+		// validate customMetadata if provided
 		if len(customMetadata) != 0 {
-
-			err := validateCustomMetadata(customMetadata)
-			if err != nil {
+			if err := validateCustomMetadata(customMetadata); err != nil {
 				return nil, err
 			}
 		}
@@ -178,25 +175,24 @@ func (i *IdentityStore) handleAliasCreateUpdate() framework.OperationFunc {
 				if alias.NamespaceID != ns.ID {
 					return logical.ErrorResponse("cannot modify aliases across namespaces"), logical.ErrPermissionDenied
 				}
+				if !customMetadataExists {
+					customMetadata = alias.CustomMetadata
+				}
 				switch {
-				case mountAccessor == "" && name == "" && len(customMetadata) == 0:
+				case mountAccessor == "" && name == "":
 					// Just a canonical ID update, maybe
 					if canonicalID == "" {
 						// Nothing to do, so be idempotent
 						return nil, nil
 					}
 					name = alias.Name
 					mountAccessor = alias.MountAccessor
-					customMetadata = alias.CustomMetadata
 				case mountAccessor == "":
 					// No change to mount accessor
 					mountAccessor = alias.MountAccessor
 				case name == "":
 					// No change to mount name
 					name = alias.Name
-				case len(customMetadata) == 0:
-					// No change to custom metadata
-					customMetadata = alias.CustomMetadata
 				default:
 					// mountAccessor, name and customMetadata  provided
 				}

vault/identity_store_aliases_test.go

@@ -355,55 +355,173 @@ func TestIdentityStore_AliasRegister(t *testing.T) {
 }
 
 func TestIdentityStore_AliasUpdate(t *testing.T) {
-	var err error
-	var resp *logical.Response
 	ctx := namespace.RootContext(nil)
 	is, githubAccessor, _ := testIdentityStoreWithGithubAuth(ctx, t)
 
-	aliasData := map[string]interface{}{
-		"name":           "testaliasname",
-		"mount_accessor": githubAccessor,
-	}
-
-	aliasReq := &logical.Request{
-		Operation: logical.UpdateOperation,
-		Path:      "entity-alias",
-		Data:      aliasData,
-	}
-
-	// This will create an alias and a corresponding entity
-	resp, err = is.HandleRequest(ctx, aliasReq)
-	if err != nil || (resp != nil && resp.IsError()) {
-		t.Fatalf("err:%v resp:%#v", err, resp)
-	}
-	aliasID := resp.Data["id"].(string)
-	customMetadata := make(map[string]string)
-	customMetadata["foo"] = "abc"
-
-	updateData := map[string]interface{}{
-		"name":            "updatedaliasname",
-		"mount_accessor":  githubAccessor,
-		"custom_metadata": customMetadata,
-	}
-
-	aliasReq.Data = updateData
-	aliasReq.Path = "entity-alias/id/" + aliasID
-	resp, err = is.HandleRequest(ctx, aliasReq)
-	if err != nil || (resp != nil && resp.IsError()) {
-		t.Fatalf("err:%v resp:%#v", err, resp)
-	}
-
-	aliasReq.Operation = logical.ReadOperation
-	resp, err = is.HandleRequest(ctx, aliasReq)
-	if err != nil || (resp != nil && resp.IsError()) {
-		t.Fatalf("err:%v resp:%#v", err, resp)
+	tests := []struct {
+		name       string
+		createData map[string]interface{}
+		updateData map[string]interface{}
+	}{
+		{
+			name: "noop",
+			createData: map[string]interface{}{
+				"name":           "noop",
+				"mount_accessor": githubAccessor,
+				"custom_metadata": map[string]string{
+					"foo": "baz",
+					"bar": "qux",
+				},
+			},
+			updateData: map[string]interface{}{
+				"name":           "noop",
+				"mount_accessor": githubAccessor,
+				"custom_metadata": map[string]string{
+					"foo": "baz",
+					"bar": "qux",
+				},
+			},
+		},
+		{
+			name: "no-custom-metadata",
+			createData: map[string]interface{}{
+				"name":           "no-custom-metadata",
+				"mount_accessor": githubAccessor,
+			},
+			updateData: map[string]interface{}{
+				"name":           "no-custom-metadata",
+				"mount_accessor": githubAccessor,
+			},
+		},
+		{
+			name: "update-name-custom-metadata",
+			createData: map[string]interface{}{
+				"name":            "update-name-custom-metadata",
+				"mount_accessor":  githubAccessor,
+				"custom_metadata": make(map[string]string),
+			},
+			updateData: map[string]interface{}{
+				"name":           "update-name-custom-metadata-2",
+				"mount_accessor": githubAccessor,
+				"custom_metadata": map[string]string{
+					"bar": "qux",
+				},
+			},
+		},
+		{
+			name: "update-name",
+			createData: map[string]interface{}{
+				"name":           "update-name",
+				"mount_accessor": githubAccessor,
+				"custom_metadata": map[string]string{
+					"foo": "baz",
+				},
+			},
+			updateData: map[string]interface{}{
+				"name":           "update-name-2",
+				"mount_accessor": githubAccessor,
+				"custom_metadata": map[string]string{
+					"foo": "baz",
+				},
+			},
+		},
+		{
+			name: "update-metadata",
+			createData: map[string]interface{}{
+				"name":           "update-metadata",
+				"mount_accessor": githubAccessor,
+				"custom_metadata": map[string]string{
+					"foo": "bar",
+				},
+			},
+			updateData: map[string]interface{}{
+				"name":           "update-metadata",
+				"mount_accessor": githubAccessor,
+				"custom_metadata": map[string]string{
+					"foo": "baz",
+					"bar": "qux",
+				},
+			},
+		},
+		{
+			name: "clear-metadata",
+			createData: map[string]interface{}{
+				"name":           "clear",
+				"mount_accessor": githubAccessor,
+				"custom_metadata": map[string]string{
+					"foo": "bar",
+				},
+			},
+			updateData: map[string]interface{}{
+				"name":            "clear",
+				"mount_accessor":  githubAccessor,
+				"custom_metadata": map[string]string{},
+			},
+		},
 	}
 
-	if resp.Data["name"] != "updatedaliasname" {
-		t.Fatalf("failed to update alias information; \n response data: %#v\n", resp.Data)
-	}
-	if !reflect.DeepEqual(resp.Data["custom_metadata"], customMetadata) {
-		t.Fatalf("failed to update alias information; \n response data: %#v\n", resp.Data)
+	handleRequest := func(t *testing.T, req *logical.Request) *logical.Response {
+		t.Helper()
+		resp, err := is.HandleRequest(ctx, req)
+		if err != nil || (resp != nil && resp.IsError()) {
+			t.Fatalf("err:%v resp:%#v", err, resp)
+		}
+		return resp
+	}
+
+	respDefaults := map[string]interface{}{
+		"custom_metadata": map[string]string{},
+	}
+
+	checkValues := func(t *testing.T, reqData, respData map[string]interface{}) {
+		t.Helper()
+		expected := make(map[string]interface{})
+		for k := range reqData {
+			expected[k] = reqData[k]
+		}
+
+		for k := range respDefaults {
+			if _, ok := expected[k]; !ok {
+				expected[k] = respDefaults[k]
+			}
+		}
+
+		actual := make(map[string]interface{}, len(expected))
+		for k := range expected {
+			actual[k] = respData[k]
+		}
+
+		if !reflect.DeepEqual(expected, actual) {
+			t.Fatalf("expected data %#v, actual %#v", expected, actual)
+		}
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			resp := handleRequest(t, &logical.Request{
+				Operation: logical.UpdateOperation,
+				Path:      "entity-alias",
+				Data:      tt.createData,
+			})
+
+			readReq := &logical.Request{
+				Operation: logical.ReadOperation,
+				Path:      "entity-alias/id/" + resp.Data["id"].(string),
+			}
+
+			// check create
+			resp = handleRequest(t, readReq)
+			checkValues(t, tt.createData, resp.Data)
+
+			// check update
+			resp = handleRequest(t, &logical.Request{
+				Operation: logical.UpdateOperation,
+				Path:      readReq.Path,
+				Data:      tt.updateData,
+			})
+			resp = handleRequest(t, readReq)
+			checkValues(t, tt.updateData, resp.Data)
+		})
 	}
 }
 
@@ -489,7 +607,6 @@ func TestIdentityStore_AliasMove_DuplicateAccessor(t *testing.T) {
 	if resp == nil || !resp.IsError() {
 		t.Fatalf("expected an error as alias on the github accessor exists for testentity1")
 	}
-
 }
 
 // Test that the alias cannot be changed to a mount for which
@@ -563,7 +680,6 @@ func TestIdentityStore_AliasUpdate_DuplicateAccessor(t *testing.T) {
 	if resp == nil || !resp.IsError() {
 		t.Fatalf("expected an error as an alias on the github accessor already exists for testentity")
 	}
-
 }
 
 // Test that alias creation fails if an alias for the specified mount