Merge pull request #2392 from redpanda-data/fix/ux-1217-acl-rule-id-collision

fix(acls): prevent React key collision wiping ACLs on edit (UX-1217, UX-1219)

Author: c-julin

frontend/src/components/pages/security/acls/create-acl.tsx

@@ -28,6 +28,7 @@ import { useSupportedFeaturesStore } from 'state/supported-features';
 import {
   type AclRulesProps,
   getIdFromRule,
+  getInitialRuleIdCounter,
   getOperationsForResourceType,
   getRuleDataTestId,
   type HostType,
@@ -774,7 +775,7 @@ export default function CreateACL({
     }
   }, [propSharedConfig?.principal, propSharedConfig?.host]);
 
-  const ruleIdCounter = useRef(2);
+  const ruleIdCounter = useRef(getInitialRuleIdCounter(propRules));
   const [rules, setRules] = useState<Rule[]>(
     propRules ?? [
       {

frontend/src/components/pages/security/shared/acl-model.test.tsx

@@ -23,15 +23,27 @@ import { describe, expect, test } from 'vitest';
 
 import {
   getAclFromAclListResponse,
+  getInitialRuleIdCounter,
   ModeAllowAll,
   ModeCustom,
   ModeDenyAll,
   OperationTypeAllow,
   OperationTypeDeny,
   ResourcePatternTypeLiteral,
   ResourcePatternTypePrefix,
+  ResourceTypeTopic,
+  type Rule,
 } from './acl-model';
 
+const buildRule = (id: number): Rule => ({
+  id,
+  resourceType: ResourceTypeTopic,
+  mode: ModeCustom,
+  selectorType: ResourcePatternTypeLiteral,
+  selectorValue: `topic-${id}`,
+  operations: {},
+});
+
 describe('getAclFromAclListResponse', () => {
   describe('Single host scenarios', () => {
     test('should return single AclDetail for single host', () => {
@@ -531,3 +543,25 @@ describe('getAclFromAclListResponse', () => {
     });
   });
 });
+
+describe('getInitialRuleIdCounter', () => {
+  test('returns 2 for a fresh create (undefined rules)', () => {
+    expect(getInitialRuleIdCounter()).toBe(2);
+  });
+
+  test('returns 2 for an empty rule array', () => {
+    expect(getInitialRuleIdCounter([])).toBe(2);
+  });
+
+  test('returns max(id) + 1 for sequential edit-mode rules', () => {
+    expect(getInitialRuleIdCounter([buildRule(0), buildRule(1), buildRule(2)])).toBe(3);
+  });
+
+  test('returns max(id) + 1 for non-sequential ids', () => {
+    expect(getInitialRuleIdCounter([buildRule(0), buildRule(7), buildRule(3)])).toBe(8);
+  });
+
+  test('handles a single rule with id greater than 2', () => {
+    expect(getInitialRuleIdCounter([buildRule(9)])).toBe(10);
+  });
+});

frontend/src/components/pages/security/shared/acl-model.tsx

@@ -446,6 +446,9 @@ export const getAclFromAclListResponse = (aclList: ListACLsResponse): AclDetail[
 export const getOperationsForResourceType = (resourceType: ResourceType): Record<string, OperationType> =>
   operationSets[resourceType] || {};
 
+export const getInitialRuleIdCounter = (rules?: Rule[]): number =>
+  rules && rules.length > 0 ? Math.max(...rules.map((r) => r.id)) + 1 : 2;
+
 // Helper function to get resource name
 export const getResourceName = (resourceType: string): string => {
   const resourceNames: Record<string, string> = {

frontend/tests/test-variant-console/acls/acl-edit-preserves-rules.spec.ts

@@ -0,0 +1,108 @@
+/**
+ * Regression test for UX-1217 / UX-1219 — adding a rule on edit must not wipe existing ACLs.
+ *
+ * Pre-fix bug: `create-acl.tsx` used `useRef(2)` for the new-rule id counter. On edit-load,
+ * `processResourceAcls` assigns rule ids 0, 1, 2, ... When the user clicked "Add rule",
+ * the new rule got id=2 and collided with an already-loaded rule's React key, causing
+ * list-reconciliation state bleed that dropped ACLs on save.
+ *
+ * Reproducing the collision requires at least 3 distinct rule groups on edit-load
+ * (so that id=2 is already taken). The form groups ACLs by (resourceType, pattern, name),
+ * so three separate rule groups = three different (resourceType/name) combinations.
+ */
+import { test } from '@playwright/test';
+
+import {
+  ModeCustom,
+  OperationTypeAllow,
+  ResourcePatternTypeLiteral,
+  ResourceTypeCluster,
+  ResourceTypeConsumerGroup,
+  ResourceTypeTopic,
+  ResourceTypeTransactionalId,
+  type Rule,
+} from '../../../src/components/pages/security/shared/acl-model';
+import { AclPage } from '../utils/acl-page';
+
+const initialRules: Rule[] = [
+  {
+    id: 0,
+    resourceType: ResourceTypeCluster,
+    mode: ModeCustom,
+    selectorType: ResourcePatternTypeLiteral,
+    selectorValue: 'kafka-cluster',
+    operations: {
+      DESCRIBE: OperationTypeAllow,
+    },
+  },
+  {
+    id: 1,
+    resourceType: ResourceTypeTopic,
+    mode: ModeCustom,
+    selectorType: ResourcePatternTypeLiteral,
+    selectorValue: 'topic-acl2',
+    operations: {
+      DESCRIBE: OperationTypeAllow,
+      READ: OperationTypeAllow,
+      WRITE: OperationTypeAllow,
+    },
+  },
+  {
+    id: 2,
+    resourceType: ResourceTypeConsumerGroup,
+    mode: ModeCustom,
+    selectorType: ResourcePatternTypeLiteral,
+    selectorValue: 'cg-a',
+    operations: {
+      READ: OperationTypeAllow,
+    },
+  },
+];
+
+const addedRule: Rule = {
+  id: 3,
+  resourceType: ResourceTypeTransactionalId,
+  mode: ModeCustom,
+  selectorType: ResourcePatternTypeLiteral,
+  selectorValue: 'tx-1',
+  operations: {
+    DESCRIBE: OperationTypeAllow,
+  },
+};
+
+test.describe('ACL edit preserves existing rules (UX-1217)', () => {
+  test('adding a rule on edit does not wipe existing ACLs', async ({ page }) => {
+    test.setTimeout(180_000);
+
+    const principal = `edit-preserve-${Date.now()}`;
+    const host = '*';
+    const aclPage = new AclPage(page);
+
+    await test.step('Create initial ACL with 3 distinct rule groups', async () => {
+      await aclPage.goto();
+      await aclPage.setPrincipal(principal);
+      await aclPage.setHost(host);
+      await aclPage.configureRules(initialRules);
+      await aclPage.submitForm();
+      await aclPage.waitForDetailPage();
+      await aclPage.validateRulesCount(initialRules.length);
+    });
+
+    await test.step('Navigate to edit page', async () => {
+      await aclPage.clickUpdateButtonFromDetailPage();
+      await aclPage.waitForUpdatePage();
+    });
+
+    await test.step('Add a new rule — pre-fix this collides with the loaded id=2 rule', async () => {
+      await aclPage.updateRules([addedRule]);
+      await aclPage.submitForm();
+      await aclPage.waitForDetailPage();
+    });
+
+    await test.step('Verify every original rule plus the added rule is preserved', async () => {
+      const finalRules = [...initialRules, addedRule];
+      await aclPage.validateRulesCount(finalRules.length);
+      await aclPage.validateAllDetailRules(finalRules, principal, host);
+    });
+  });
+});