Update group memberships when entity is deleted (#5786)

* Use common abstraction for entity deletion

* Update group memberships before deleting entity

* Added test

* Fix return statements

* Update comment

* Cleanup member entity IDs while loading groups

* Added test to ensure that upgrade happens properly

* Ensure that the group gets persisted if upgrade code modifies it

vault/identity_store_entities.go

@@ -412,39 +412,15 @@ func (i *IdentityStore) pathEntityIDDelete() framework.OperationFunc {
 		if err != nil {
 			return nil, err
 		}
-		// If there is no entity for the ID, do nothing
 		if entity == nil {
 			return nil, nil
 		}
 
-		ns, err := namespace.FromContext(ctx)
-		if err != nil {
-			return nil, err
-		}
-		if entity.NamespaceID != ns.ID {
-			return nil, nil
-		}
-
-		// Delete all the aliases in the entity. This function will also remove
-		// the corresponding alias indexes too.
-		err = i.deleteAliasesInEntityInTxn(txn, entity, entity.Aliases)
-		if err != nil {
-			return nil, err
-		}
-
-		// Delete the entity using the same transaction
-		err = i.MemDBDeleteEntityByIDInTxn(txn, entity.ID)
-		if err != nil {
-			return nil, err
-		}
-
-		// Delete the entity from storage
-		err = i.entityPacker.DeleteItem(entity.ID)
+		err = i.handleEntityDeleteCommon(ctx, txn, entity)
 		if err != nil {
 			return nil, err
 		}
 
-		// Committing the transaction *after* successfully deleting entity
 		txn.Commit()
 
 		return nil, nil
@@ -484,30 +460,60 @@ func (i *IdentityStore) pathEntityNameDelete() framework.OperationFunc {
 			return nil, nil
 		}
 
-		// Delete all the aliases in the entity. This function will also remove
-		// the corresponding alias indexes too.
-		err = i.deleteAliasesInEntityInTxn(txn, entity, entity.Aliases)
+		err = i.handleEntityDeleteCommon(ctx, txn, entity)
 		if err != nil {
 			return nil, err
 		}
 
-		// Delete the entity using the same transaction
-		err = i.MemDBDeleteEntityByIDInTxn(txn, entity.ID)
-		if err != nil {
-			return nil, err
-		}
+		txn.Commit()
+
+		return nil, nil
+	}
+}
+
+func (i *IdentityStore) handleEntityDeleteCommon(ctx context.Context, txn *memdb.Txn, entity *identity.Entity) error {
+	ns, err := namespace.FromContext(ctx)
+	if err != nil {
+		return err
+	}
+	if entity.NamespaceID != ns.ID {
+		return nil
+	}
+
+	// Remove entity ID as a member from all the groups it belongs, both
+	// internal and external
+	groups, err := i.MemDBGroupsByMemberEntityIDInTxn(txn, entity.ID, true, false)
+	if err != nil {
+		return nil
+	}
 
-		// Delete the entity from storage
-		err = i.entityPacker.DeleteItem(entity.ID)
+	for _, group := range groups {
+		group.MemberEntityIDs = strutil.StrListDelete(group.MemberEntityIDs, entity.ID)
+		err = i.UpsertGroupInTxn(txn, group, true)
 		if err != nil {
-			return nil, err
+			return err
 		}
+	}
 
-		// Committing the transaction *after* successfully deleting entity
-		txn.Commit()
+	// Delete all the aliases in the entity and the respective indexes
+	err = i.deleteAliasesInEntityInTxn(txn, entity, entity.Aliases)
+	if err != nil {
+		return err
+	}
 
-		return nil, nil
+	// Delete the entity using the same transaction
+	err = i.MemDBDeleteEntityByIDInTxn(txn, entity.ID)
+	if err != nil {
+		return err
 	}
+
+	// Delete the entity from storage
+	err = i.entityPacker.DeleteItem(entity.ID)
+	if err != nil {
+		return err
+	}
+
+	return nil
 }
 
 func (i *IdentityStore) pathEntityIDList() framework.OperationFunc {

vault/identity_store_entities_test.go

@@ -15,6 +15,73 @@ import (
 	"github.com/hashicorp/vault/logical"
 )
 
+func TestIdentityStore_EntityDeleteGroupMembershipUpdate(t *testing.T) {
+	i, _, _ := testIdentityStoreWithGithubAuth(namespace.RootContext(nil), t)
+
+	// Create an entity
+	resp, err := i.HandleRequest(namespace.RootContext(nil), &logical.Request{
+		Path:      "entity",
+		Operation: logical.UpdateOperation,
+		Data: map[string]interface{}{
+			"name": "testentity",
+		},
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err:%v\nresp: %#v", err, resp)
+	}
+	entityID := resp.Data["id"].(string)
+
+	// Create a group
+	resp, err = i.HandleRequest(namespace.RootContext(nil), &logical.Request{
+		Path:      "group",
+		Operation: logical.UpdateOperation,
+		Data: map[string]interface{}{
+			"name":              "testgroup",
+			"member_entity_ids": []string{entityID},
+		},
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err:%v\nresp: %#v", err, resp)
+	}
+
+	// Ensure that the group has entity ID as its member
+	resp, err = i.HandleRequest(namespace.RootContext(nil), &logical.Request{
+		Path:      "group/name/testgroup",
+		Operation: logical.ReadOperation,
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err:%v\nresp: %#v", err, resp)
+	}
+	expected := []string{entityID}
+	actual := resp.Data["member_entity_ids"].([]string)
+	if !reflect.DeepEqual(expected, actual) {
+		t.Fatalf("bad: member entity ids; expected: %#v\nactual: %#v", expected, actual)
+	}
+
+	// Delete the entity
+	resp, err = i.HandleRequest(namespace.RootContext(nil), &logical.Request{
+		Path:      "entity/name/testentity",
+		Operation: logical.DeleteOperation,
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err:%v\nresp: %#v", err, resp)
+	}
+
+	// Ensure that the group does not have entity ID as it's member anymore
+	resp, err = i.HandleRequest(namespace.RootContext(nil), &logical.Request{
+		Path:      "group/name/testgroup",
+		Operation: logical.ReadOperation,
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err:%v\nresp: %#v", err, resp)
+	}
+	expected = []string{}
+	actual = resp.Data["member_entity_ids"].([]string)
+	if !reflect.DeepEqual(expected, actual) {
+		t.Fatalf("bad: member entity ids; expected: %#v\nactual: %#v", expected, actual)
+	}
+}
+
 func TestIdentityStore_CaseInsensitiveEntityName(t *testing.T) {
 	ctx := namespace.RootContext(nil)
 	i, _, _ := testIdentityStoreWithGithubAuth(ctx, t)

vault/identity_store_groups_test.go

@@ -12,6 +12,68 @@ import (
 	"github.com/hashicorp/vault/logical"
 )
 
+func TestIdentityStore_GroupEntityMembershipUpgrade(t *testing.T) {
+	c, keys, rootToken := TestCoreUnsealed(t)
+
+	// Create a group
+	resp, err := c.identityStore.HandleRequest(namespace.RootContext(nil), &logical.Request{
+		Path:      "group",
+		Operation: logical.UpdateOperation,
+		Data: map[string]interface{}{
+			"name": "testgroup",
+		},
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err:%v\nresp: %#v", err, resp)
+	}
+
+	// Create a memdb transaction
+	txn := c.identityStore.db.Txn(true)
+	defer txn.Abort()
+
+	// Fetch the above created group
+	group, err := c.identityStore.MemDBGroupByNameInTxn(namespace.RootContext(nil), txn, "testgroup", true)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Manually add an invalid entity as the group's member
+	group.MemberEntityIDs = []string{"invalidentityid"}
+
+	// Persist the group
+	err = c.identityStore.UpsertGroupInTxn(txn, group, true)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	txn.Commit()
+
+	// Perform seal and unseal forcing an upgrade
+	err = c.Seal(rootToken)
+	if err != nil {
+		t.Fatal(err)
+	}
+	for i, key := range keys {
+		unseal, err := TestCoreUnseal(c, key)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if i+1 == len(keys) && !unseal {
+			t.Fatalf("failed to unseal")
+		}
+	}
+
+	// Read the group and ensure that invalid entity id is cleaned up
+	group, err = c.identityStore.MemDBGroupByName(namespace.RootContext(nil), "testgroup", false)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if len(group.MemberEntityIDs) != 0 {
+		t.Fatalf("bad: member entity IDs; expected: none, actual: %#v", group.MemberEntityIDs)
+	}
+}
+
 func TestIdentityStore_MemberGroupIDDelete(t *testing.T) {
 	ctx := namespace.RootContext(nil)
 	i, _, _ := testIdentityStoreWithGithubAuth(ctx, t)

vault/identity_store_util.go

@@ -118,7 +118,23 @@ func (i *IdentityStore) loadGroups(ctx context.Context) error {
 
 			txn := i.db.Txn(true)
 
-			err = i.UpsertGroupInTxn(txn, group, false)
+			// Before pull#5786, entity memberships in groups were not getting
+			// updated when respective entities were deleted. This is here to
+			// check that the entity IDs in the group are indeed valid, and if
+			// not remove them.
+			persist := false
+			for _, memberEntityID := range group.MemberEntityIDs {
+				entity, err := i.MemDBEntityByID(memberEntityID, false)
+				if err != nil {
+					return err
+				}
+				if entity == nil {
+					persist = true
+					group.MemberEntityIDs = strutil.StrListDelete(group.MemberEntityIDs, memberEntityID)
+				}
+			}
+
+			err = i.UpsertGroupInTxn(txn, group, persist)
 			if err != nil {
 				txn.Abort()
 				return errwrap.Wrapf("failed to update group in memdb: {{err}}", err)