Added test for verifying member group id deletion (#5469)

vault/identity_store_groups_test.go

@@ -11,6 +11,75 @@ import (
 	"github.com/hashicorp/vault/logical"
 )
 
+func TestIdentityStore_MemberGroupIDDelete(t *testing.T) {
+	ctx := namespace.RootContext(nil)
+	i, _, _ := testIdentityStoreWithGithubAuth(ctx, t)
+
+	// Create a child group
+	resp, err := i.HandleRequest(ctx, &logical.Request{
+		Path:      "group",
+		Operation: logical.UpdateOperation,
+		Data: map[string]interface{}{
+			"name": "child",
+		},
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: resp: %#v\nerr: %v", resp, err)
+	}
+	childGroupID := resp.Data["id"].(string)
+
+	// Create a parent group with the above group ID as its child
+	resp, err = i.HandleRequest(ctx, &logical.Request{
+		Path:      "group",
+		Operation: logical.UpdateOperation,
+		Data: map[string]interface{}{
+			"name":             "parent",
+			"member_group_ids": []string{childGroupID},
+		},
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: resp: %#v\nerr: %v", resp, err)
+	}
+
+	// Ensure that member group ID is properly updated
+	resp, err = i.HandleRequest(ctx, &logical.Request{
+		Path:      "group/name/parent",
+		Operation: logical.ReadOperation,
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: resp: %#v\nerr: %v", resp, err)
+	}
+	memberGroupIDs := resp.Data["member_group_ids"].([]string)
+	if len(memberGroupIDs) != 1 && memberGroupIDs[0] != childGroupID {
+		t.Fatalf("bad: member group ids; expected: %#v, actual: %#v", []string{childGroupID}, memberGroupIDs)
+	}
+
+	// Clear the member group IDs from the parent group
+	resp, err = i.HandleRequest(ctx, &logical.Request{
+		Path:      "group/name/parent",
+		Operation: logical.UpdateOperation,
+		Data: map[string]interface{}{
+			"member_group_ids": []string{},
+		},
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: resp: %#v\nerr: %v", resp, err)
+	}
+
+	// Ensure that member group ID is properly deleted
+	resp, err = i.HandleRequest(ctx, &logical.Request{
+		Path:      "group/name/parent",
+		Operation: logical.ReadOperation,
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: resp: %#v\nerr: %v", resp, err)
+	}
+	memberGroupIDs = resp.Data["member_group_ids"].([]string)
+	if len(memberGroupIDs) != 0 {
+		t.Fatalf("bad: length of member group ids; expected: %d, actual: %d", 0, len(memberGroupIDs))
+	}
+}
+
 func TestIdentityStore_GroupByName(t *testing.T) {
 	ctx := namespace.RootContext(nil)
 	i, _, _ := testIdentityStoreWithGithubAuth(ctx, t)

vault/identity_store_util.go

@@ -932,7 +932,9 @@ func (i *IdentityStore) sanitizeAndUpsertGroup(ctx context.Context, group *ident
 
 	memberGroupIDs = strutil.RemoveDuplicates(memberGroupIDs, false)
 
-	// Remove ParentGroupID from removed GroupMembers
+	// For those group member IDs that are removed from the list, remove current
+	// group ID as their respective ParentGroupID.
+
 	// Get the current MemberGroups IDs for this group
 	var currentMemberGroupIDs []string
 	currentMemberGroups, err := i.MemDBGroupsByParentGroupID(group.ID, false)
@@ -943,8 +945,12 @@ func (i *IdentityStore) sanitizeAndUpsertGroup(ctx context.Context, group *ident
 		currentMemberGroupIDs = append(currentMemberGroupIDs, currentMemberGroup.ID)
 	}
 
-	// Check if current MemberGroups should be removed
+	// Update parent group IDs in the removed members
 	for _, currentMemberGroupID := range currentMemberGroupIDs {
+		if strutil.StrListContains(memberGroupIDs, currentMemberGroupID) {
+			continue
+		}
+
 		currentMemberGroup, err := i.MemDBGroupByID(currentMemberGroupID, true)
 		if err != nil {
 			return err
@@ -953,19 +959,11 @@ func (i *IdentityStore) sanitizeAndUpsertGroup(ctx context.Context, group *ident
 			return fmt.Errorf("invalid member group ID %q", currentMemberGroupID)
 		}
 
-		// Remove ParentGroup Entry for this group from removed Group
+		// Remove group ID from the parent group IDs
-		if !strutil.StrListContains(memberGroupIDs, currentMemberGroupID) {
+		currentMemberGroup.ParentGroupIDs = strutil.StrListDelete(currentMemberGroup.ParentGroupIDs, group.ID)
-			currentMemberGroup.ParentGroupIDs = strutil.StrListDelete(currentMemberGroup.ParentGroupIDs, group.ID)
-		}
 
-		// This technically is not upsert. It is only update, only the method
-		// name is upsert here.
 		err = i.UpsertGroupInTxn(txn, currentMemberGroup, true)
 		if err != nil {
-			// Ideally we would want to revert the whole operation in case of
-			// errors while persisting in member groups. But there is no
-			// storage transaction support yet. When we do have it, this will need
-			// an update.
 			return err
 		}
 	}