Fix use of identity/group endpoint to edit group by name (#10812)

* Updates identity/group to allow updating a group by name (#10223)
* Now that lookup by name is outside handleGroupUpdateCommon, do not
use the second name lookup as the object to update.
* Added changelog.

Co-authored-by: dr-db <25711615+dr-db@users.noreply.github.com>

changelog/10812.txt

@@ -0,0 +1,3 @@
+```changelog:bug
+identity: When the identity/group endpoint is used to modify a group by name, correctly update its policy and member entities.
+```

vault/identity_store_groups.go

@@ -120,6 +120,11 @@ func (i *IdentityStore) pathGroupRegister() framework.OperationFunc {
 			return i.pathGroupIDUpdate()(ctx, req, d)
 		}
 
+		_, ok = d.GetOk("name")
+		if ok {
+			return i.pathGroupNameUpdate()(ctx, req, d)
+		}
+
 		i.groupLock.Lock()
 		defer i.groupLock.Unlock()
 
@@ -212,16 +217,13 @@ func (i *IdentityStore) handleGroupUpdateCommon(ctx context.Context, req *logica
 			return nil, err
 		}
 
-		// If this is a new group and if there already exists a group by this
-		// name, error out. If the name of an existing group is about to be
-		// modified into something which is already tied to a different group,
-		// error out.
+		// If no existing group has this name, go ahead with the creation or rename.
+		// If there is a group, it must match the group passed in; groupByName
+		// should not be modified as it's in memdb.
 		switch {
 		case groupByName == nil:
 			// Allowed
-		case group.ID == "":
-			group = groupByName
-		case group.ID != "" && groupByName.ID != group.ID:
+		case groupByName.ID != group.ID:
 			return logical.ErrorResponse("group name is already in use"), nil
 		}
 		group.Name = groupName

vault/identity_store_groups_test.go

@@ -12,6 +12,65 @@ import (
 	"github.com/hashicorp/vault/sdk/logical"
 )
 
+func TestIdentityStore_Groups_AddByNameEntityUpdate(t *testing.T) {
+	c, _, _ := TestCoreUnsealed(t)
+	ctx := namespace.RootContext(nil)
+
+	// Create an entity and get its ID
+	entityRegisterReq := &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      "entity",
+	}
+	resp, err := c.identityStore.HandleRequest(ctx, entityRegisterReq)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: resp: %#v, err: %v", resp, err)
+	}
+	entityID := resp.Data["id"].(string)
+
+	// Create a group containing the entity
+	groupName := "group-name"
+	expectedMemberEntityIDs := []string{entityID}
+	resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{
+		Path:      "group",
+		Operation: logical.UpdateOperation,
+		Data: map[string]interface{}{
+			"name":              groupName,
+			"member_entity_ids": expectedMemberEntityIDs,
+		},
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err: %v\nresp: %#v", err, resp)
+	}
+
+	// Remove the entity from the group
+	resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{
+		Path:      "group",
+		Operation: logical.UpdateOperation,
+		Data: map[string]interface{}{
+			"name":              groupName,
+			"member_entity_ids": []string{},
+		},
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err: %v\nresp: %#v", err, resp)
+	}
+
+	// Make sure the member no longer thinks it's in the group
+	entityIDReq := &logical.Request{
+		Operation: logical.ReadOperation,
+		Path:      "entity/id/" + entityID,
+	}
+	resp, err = c.identityStore.HandleRequest(ctx, entityIDReq)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: resp: %#v, err: %v", resp, err)
+	}
+	expectedGroupIDs := []string{}
+	actualGroupIDs := resp.Data["direct_group_ids"]
+	if !reflect.DeepEqual(expectedGroupIDs, actualGroupIDs) {
+		t.Fatalf("bad: direct_group_ids:\nexpected: %#v\nactual: %#v", expectedGroupIDs, actualGroupIDs)
+	}
+}
+
 func TestIdentityStore_FixOverwrittenMemberGroupIDs(t *testing.T) {
 	c, _, _ := TestCoreUnsealed(t)
 	ctx := namespace.RootContext(nil)

website/content/api-docs/secret/identity/group.mdx

@@ -15,7 +15,8 @@ This endpoint creates or updates a Group.
 
 ### Parameters
 
-- `name` `(string: entity-<UUID>)` – Name of the group.
+- `name` `(string: entity-<UUID>)` – Name of the group.  If set (and
+  ID is not set), updates the corresponding existing group.
 
 - `id` `(string: <optional>)` - ID of the group. If set, updates the
   corresponding existing group.