Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion frontend/src/components/pages/security/acls/create-acl.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import { useSupportedFeaturesStore } from 'state/supported-features';
import {
type AclRulesProps,
getIdFromRule,
getInitialRuleIdCounter,
getOperationsForResourceType,
getRuleDataTestId,
type HostType,
Expand Down Expand Up @@ -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 ?? [
{
Expand Down
34 changes: 34 additions & 0 deletions frontend/src/components/pages/security/shared/acl-model.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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);
});
});
3 changes: 3 additions & 0 deletions frontend/src/components/pages/security/shared/acl-model.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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> = {
Expand Down
Original file line number Diff line number Diff line change
@@ -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);
});
});
});
Loading