return bad request instead of server error for identity group cycle detection (#15912)

* return bad request for identity group cycle detection

* add changelog entry

* use change release note instead of improvement

* fix err reference

* fix TestIdentityStore_GroupHierarchyCases

changelog/15912.txt

@@ -0,0 +1,3 @@
+```release-note:change
+identity: a request to `/identity/group` that includes `member_group_ids` that contains a cycle will now be responded to with a 400 rather than 500
+```

vault/identity_store_groups.go

@@ -255,6 +255,10 @@ func (i *IdentityStore) handleGroupUpdateCommon(ctx context.Context, req *logica
 
 	err = i.sanitizeAndUpsertGroup(ctx, group, nil, memberGroupIDs)
 	if err != nil {
+		if errStr := err.Error(); strings.HasPrefix(errStr, errCycleDetectedPrefix) {
+			return logical.ErrorResponse(errStr), nil
+		}
+
 		return nil, err
 	}
 

vault/identity_store_groups_test.go

@@ -1,6 +1,7 @@
 package vault
 
 import (
+	"fmt"
 	"reflect"
 	"sort"
 	"strings"
@@ -1252,7 +1253,7 @@ func TestIdentityStore_GroupHierarchyCases(t *testing.T) {
 	groupUpdateReq.Data = kubeGroupData
 	kubeGroupData["member_group_ids"] = []string{engGroupID}
 	resp, err = is.HandleRequest(ctx, groupUpdateReq)
-	if err == nil {
+	if err != nil || resp == nil || !resp.IsError() {
 		t.Fatalf("expected an error response")
 	}
 
@@ -1391,3 +1392,91 @@ func TestIdentityStore_GroupHierarchyCases(t *testing.T) {
 		t.Fatalf("bad: length of inheritedGroups; expected: 0, actual: %d", len(inheritedGroups))
 	}
 }
+
+func TestIdentityStore_GroupCycleDetection(t *testing.T) {
+	c, _, _ := TestCoreUnsealed(t)
+	ctx := namespace.RootContext(nil)
+
+	group1Name := "group1"
+	group2Name := "group2"
+	group3Name := "group3"
+
+	resp, err := c.identityStore.HandleRequest(ctx, &logical.Request{
+		Path:      "group",
+		Operation: logical.UpdateOperation,
+		Data: map[string]interface{}{
+			"name": group1Name,
+		},
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("failed to create group %q, err: %v, resp: %#v", group1Name, err, resp)
+	}
+
+	group1Id := resp.Data["id"].(string)
+
+	resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{
+		Path:      "group",
+		Operation: logical.UpdateOperation,
+		Data: map[string]interface{}{
+			"name": group2Name,
+		},
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("failed to create group %q, err: %v, resp: %#v", group2Name, err, resp)
+	}
+
+	group2Id := resp.Data["id"].(string)
+
+	resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{
+		Path:      "group",
+		Operation: logical.UpdateOperation,
+		Data: map[string]interface{}{
+			"name": group3Name,
+		},
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("failed to create group %q, err: %v, resp: %#v", group3Name, err, resp)
+	}
+
+	group3Id := resp.Data["id"].(string)
+
+	resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{
+		Path:      "group",
+		Operation: logical.UpdateOperation,
+		Data: map[string]interface{}{
+			"name":             group1Name,
+			"member_group_ids": []string{},
+		},
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("failed to update group %q, err: %v, resp: %#v", group1Name, err, resp)
+	}
+
+	resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{
+		Path:      "group",
+		Operation: logical.UpdateOperation,
+		Data: map[string]interface{}{
+			"name":             group2Name,
+			"member_group_ids": []string{group3Id},
+		},
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("failed to update group %q, err: %v, resp: %#v", group2Name, err, resp)
+	}
+
+	resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{
+		Path:      "group",
+		Operation: logical.UpdateOperation,
+		Data: map[string]interface{}{
+			"name":             group3Name,
+			"member_group_ids": []string{group1Id, group2Id},
+		},
+	})
+
+	if err != nil || resp == nil {
+		t.Fatalf("unexpected group update error for group %q, err: %v, resp: %#v", group3Name, err, resp)
+	}
+	if !resp.IsError() || resp.Error().Error() != fmt.Sprintf("%s %q", errCycleDetectedPrefix, group2Id) {
+		t.Fatalf("expected update to group %q to fail due to cycle, resp: %#v", group3Id, resp)
+	}
+}

vault/identity_store_util.go

@@ -24,6 +24,7 @@ import (
 
 var (
 	errDuplicateIdentityName = errors.New("duplicate identity name")
+	errCycleDetectedPrefix   = "cyclic relationship detected for member group ID"
 	tmpSuffix                = ".tmp"
 )
 
@@ -1578,7 +1579,7 @@ func (i *IdentityStore) sanitizeAndUpsertGroup(ctx context.Context, group *ident
 				return fmt.Errorf("failed to perform cyclic relationship detection for member group ID %q", memberGroupID)
 			}
 			if cycleDetected {
-				return fmt.Errorf("cyclic relationship detected for member group ID %q", memberGroupID)
+				return fmt.Errorf("%s %q", errCycleDetectedPrefix, memberGroupID)
 			}
 		}
 
@@ -2157,7 +2158,7 @@ func (i *IdentityStore) detectCycleDFS(visited map[string]bool, startingGroupID,
 			return false, fmt.Errorf("failed to perform cycle detection at member group ID %q", memberGroup.ID)
 		}
 		if cycleDetected {
-			return true, fmt.Errorf("cycle detected at member group ID %q", memberGroup.ID)
+			return true, nil
 		}
 	}