Fix entity group associations (#10085)

- When two entities are merged, remove the from entity ID in any
  associated groups.
- When two entities are merged, also merge their associated group
  memberships.

Fixes #10084

changelog/10085.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+identity: merge associated entity groups when merging entities
+```

vault/identity_store_entities.go

@@ -583,7 +583,7 @@ func (i *IdentityStore) handleEntityDeleteCommon(ctx context.Context, txn *memdb
 	// internal and external
 	groups, err := i.MemDBGroupsByMemberEntityIDInTxn(txn, entity.ID, true, false)
 	if err != nil {
-		return nil
+		return err
 	}
 
 	for _, group := range groups {
@@ -767,6 +767,7 @@ func (i *IdentityStore) mergeEntity(ctx context.Context, txn *memdb.Txn, toEntit
 
 	isPerfSecondaryOrStandby := i.localNode.ReplicationState().HasState(consts.ReplicationPerformanceSecondary) ||
 		i.localNode.HAState() == consts.PerfStandby
+	var fromEntityGroups []*identity.Group
 	for _, fromEntityID := range fromEntityIDs {
 		if fromEntityID == toEntity.ID {
 			return errors.New("to_entity_id should not be present in from_entity_ids"), nil
@@ -814,6 +815,22 @@ func (i *IdentityStore) mergeEntity(ctx context.Context, txn *memdb.Txn, toEntit
 		// the entity we are merging into is composed of.
 		toEntity.MergedEntityIDs = append(toEntity.MergedEntityIDs, fromEntity.ID)
 
+		// Remove entity ID as a member from all the groups it belongs, both
+		// internal and external
+		groups, err := i.MemDBGroupsByMemberEntityIDInTxn(txn, fromEntity.ID, true, false)
+		if err != nil {
+			return nil, err
+		}
+		for _, group := range groups {
+			group.MemberEntityIDs = strutil.StrListDelete(group.MemberEntityIDs, fromEntity.ID)
+			err = i.UpsertGroupInTxn(ctx, txn, group, persist && !isPerfSecondaryOrStandby)
+			if err != nil {
+				return nil, err
+			}
+
+			fromEntityGroups = append(fromEntityGroups, group)
+		}
+
 		// Delete the entity which we are merging from in MemDB using the same transaction
 		err = i.MemDBDeleteEntityByIDInTxn(txn, fromEntity.ID)
 		if err != nil {
@@ -835,6 +852,14 @@ func (i *IdentityStore) mergeEntity(ctx context.Context, txn *memdb.Txn, toEntit
 		return nil, err
 	}
 
+	for _, group := range fromEntityGroups {
+		group.MemberEntityIDs = append(group.MemberEntityIDs, toEntity.ID)
+		err = i.UpsertGroupInTxn(ctx, txn, group, persist && !isPerfSecondaryOrStandby)
+		if err != nil {
+			return nil, err
+		}
+	}
+
 	if persist && !isPerfSecondaryOrStandby {
 		// Persist the entity which we are merging to
 		toEntityAsAny, err := ptypes.MarshalAny(toEntity)

vault/identity_store_entities_test.go

@@ -12,6 +12,7 @@ import (
 	credGithub "github.com/hashicorp/vault/builtin/credential/github"
 	"github.com/hashicorp/vault/helper/identity"
 	"github.com/hashicorp/vault/helper/namespace"
+	"github.com/hashicorp/vault/sdk/helper/strutil"
 	"github.com/hashicorp/vault/sdk/logical"
 )
 
@@ -1071,6 +1072,19 @@ func TestIdentityStore_MergeEntitiesByID(t *testing.T) {
 		t.Fatalf("bad: number of aliases in entity; expected: 2, actual: %d", len(entity1.Aliases))
 	}
 
+	entity1GroupReq := &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      "group",
+		Data: map[string]interface{}{
+			"member_entity_ids": entityID1,
+		},
+	}
+	resp, err = is.HandleRequest(ctx, entity1GroupReq)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("err:%v resp:%#v", err, resp)
+	}
+	entity1GroupID := resp.Data["id"].(string)
+
 	registerReq.Data = registerData2
 	// Register another entity
 	resp, err = is.HandleRequest(ctx, registerReq)
@@ -1114,6 +1128,19 @@ func TestIdentityStore_MergeEntitiesByID(t *testing.T) {
 		t.Fatalf("bad: number of aliases in entity; expected: 2, actual: %d", len(entity2.Aliases))
 	}
 
+	entity2GroupReq := &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      "group",
+		Data: map[string]interface{}{
+			"member_entity_ids": entityID2,
+		},
+	}
+	resp, err = is.HandleRequest(ctx, entity2GroupReq)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("err:%v resp:%#v", err, resp)
+	}
+	entity2GroupID := resp.Data["id"].(string)
+
 	mergeData := map[string]interface{}{
 		"to_entity_id":    entityID1,
 		"from_entity_ids": []string{entityID2},
@@ -1147,12 +1174,12 @@ func TestIdentityStore_MergeEntitiesByID(t *testing.T) {
 		t.Fatalf("err:%v resp:%#v", err, resp)
 	}
 
-	entity2Aliases := resp.Data["aliases"].([]interface{})
-	if len(entity2Aliases) != 4 {
-		t.Fatalf("bad: number of aliases in entity; expected: 4, actual: %d", len(entity2Aliases))
+	entity1Aliases := resp.Data["aliases"].([]interface{})
+	if len(entity1Aliases) != 4 {
+		t.Fatalf("bad: number of aliases in entity; expected: 4, actual: %d", len(entity1Aliases))
 	}
 
-	for _, aliasRaw := range entity2Aliases {
+	for _, aliasRaw := range entity1Aliases {
 		alias := aliasRaw.(map[string]interface{})
 		aliasLookedUp, err := is.MemDBAliasByID(alias["id"].(string), false, false)
 		if err != nil {
@@ -1162,4 +1189,24 @@ func TestIdentityStore_MergeEntitiesByID(t *testing.T) {
 			t.Fatalf("index for alias id %q is not updated", alias["id"].(string))
 		}
 	}
+
+	entity1Groups := resp.Data["direct_group_ids"].([]string)
+	if len(entity1Groups) != 2 {
+		t.Fatalf("bad: number of groups in entity; expected: 2, actual: %d", len(entity1Groups))
+	}
+
+	for _, group := range []string{entity1GroupID, entity2GroupID} {
+		if !strutil.StrListContains(entity1Groups, group) {
+			t.Fatalf("group id %q not found in merged entity direct groups %q", group, entity1Groups)
+		}
+
+		groupLookedUp, err := is.MemDBGroupByID(group, false)
+		if err != nil {
+			t.Fatal(err)
+		}
+		expectedEntityIDs := []string{entity1.ID}
+		if !strutil.EquivalentSlices(groupLookedUp.MemberEntityIDs, expectedEntityIDs) {
+			t.Fatalf("group id %q should contain %q but contains %q", group, expectedEntityIDs, groupLookedUp.MemberEntityIDs)
+		}
+	}
 }

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

@@ -395,7 +395,7 @@ $ curl \
 
 ## Merge Entities
 
-This endpoint merges many entities into one entity.
+This endpoint merges many entities into one entity. Additionally, all groups associated with `from_entity_ids` are merged with those of `to_entity_id`.
 
 | Method | Path                     |
 | :----- | :----------------------- |