diff --git a/frontend/src/components/pages/security/acls/create-acl.tsx b/frontend/src/components/pages/security/acls/create-acl.tsx index 53c76840fe..56ac9b78d9 100644 --- a/frontend/src/components/pages/security/acls/create-acl.tsx +++ b/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( propRules ?? [ { diff --git a/frontend/src/components/pages/security/shared/acl-model.test.tsx b/frontend/src/components/pages/security/shared/acl-model.test.tsx index acb5ac75d6..24fafaed12 100644 --- a/frontend/src/components/pages/security/shared/acl-model.test.tsx +++ b/frontend/src/components/pages/security/shared/acl-model.test.tsx @@ -23,6 +23,7 @@ import { describe, expect, test } from 'vitest'; import { getAclFromAclListResponse, + getInitialRuleIdCounter, ModeAllowAll, ModeCustom, ModeDenyAll, @@ -30,8 +31,19 @@ import { 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); + }); +}); diff --git a/frontend/src/components/pages/security/shared/acl-model.tsx b/frontend/src/components/pages/security/shared/acl-model.tsx index f57754b401..482c2f2858 100644 --- a/frontend/src/components/pages/security/shared/acl-model.tsx +++ b/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 => 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 = { diff --git a/frontend/tests/test-variant-console/acls/acl-edit-preserves-rules.spec.ts b/frontend/tests/test-variant-console/acls/acl-edit-preserves-rules.spec.ts new file mode 100644 index 0000000000..2e80a20239 --- /dev/null +++ b/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); + }); + }); +});