Skip to content

Commit 3b6602c

Browse files
committed
fix(acls): prevent React key collision on edit-mode rule counter (UX-1219, UX-1198)
The `ruleIdCounter` in create-acl.tsx was hardcoded to start at 2. In edit mode, propRules arrive with IDs assigned from an external counter (0, 1, 2, ...), so adding a new rule via addRule() would emit id=2 — colliding with an existing rule's ID. React key collisions in list reconciliation can corrupt form state: field values bleed between rows, operation settings silently swap, state updates target the wrong row. Combined with the silent Promise.allSettled error path fixed in UX-1218, this can cause the sort of data-loss symptoms reported in UX-1217 (customer-facing ACLs-wiped-on-edit bug). Fix: initialize the counter from max(propRules.id) + 1 when editing, else keep the fresh-create default of 2. Also includes the e2e regression spec acl-edit-preserves-rules.spec.ts authored during UX-1217 investigation, which exercises the step-3 repro (create 3 topic ACLs, open edit, add a consumer-group rule, save) and asserts no ACLs are lost. Spec runs happy-path only against the real stack (no mocks). Related tickets: - UX-1217 (ACL wipe bug — this fix may resolve it; diagnostic handoff in ticket comments) - UX-1220 (duplicate of UX-1219, closed)
1 parent b42ca33 commit 3b6602c

2 files changed

Lines changed: 144 additions & 1 deletion

File tree

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -774,7 +774,10 @@ export default function CreateACL({
774774
}
775775
}, [propSharedConfig?.principal, propSharedConfig?.host]);
776776

777-
const ruleIdCounter = useRef(2);
777+
// Start counter beyond any existing rule id so new rules never collide with
778+
// propRules coming from edit-mode loads (UX-1219, UX-1198). Default 2 matches
779+
// the fresh-create default rule id=1 below.
780+
const ruleIdCounter = useRef(propRules && propRules.length > 0 ? Math.max(...propRules.map((r) => r.id)) + 1 : 2);
778781
const [rules, setRules] = useState<Rule[]>(
779782
propRules ?? [
780783
{
Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
/** biome-ignore-all lint/performance/useTopLevelRegex: e2e test */
2+
/** biome-ignore-all lint/suspicious/noConsole: diagnostic tracing for UX-1217 */
3+
/**
4+
* Regression test for UX-1217 — ACLs are wiped during edit when adding a new rule.
5+
*
6+
* Ticket repro:
7+
* 1. Create 3 topic ACLs with same topic-name (3 operations on topic-acl2)
8+
* 2. Open edit — loads as one Rule with 3 operations
9+
* 3. Add a consumer-group rule — SAVE triggers the bug: topic ACLs lost
10+
*
11+
* This spec automates steps 1-3 and asserts that after saving:
12+
* - The original 3 topic ACLs still exist
13+
* - The new consumer-group ACL was also created
14+
*/
15+
16+
import { expect, test } from '@playwright/test';
17+
18+
import { appendFileSync, writeFileSync } from 'node:fs';
19+
import {
20+
ModeAllowAll,
21+
ModeCustom,
22+
OperationTypeAllow,
23+
ResourcePatternTypeAny,
24+
ResourcePatternTypeLiteral,
25+
ResourceTypeConsumerGroup,
26+
ResourceTypeTopic,
27+
type Rule,
28+
} from '../../../src/components/pages/security/shared/acl-model';
29+
import { AclPage } from '../utils/acl-page';
30+
31+
const REQUEST_LOG = '/tmp/ux-1217-repro.txt';
32+
33+
function attachRequestLogger(page: import('@playwright/test').Page, label: string) {
34+
writeFileSync(REQUEST_LOG, `=== ${label} @ ${new Date().toISOString()} ===\n`, { flag: 'a' });
35+
page.on('request', (req) => {
36+
const url = req.url();
37+
if (!url.includes('/redpanda.api.')) {
38+
return;
39+
}
40+
appendFileSync(REQUEST_LOG, `REQ ${req.method()} ${url}\n`);
41+
const body = req.postData();
42+
if (body && body.length < 500) {
43+
appendFileSync(REQUEST_LOG, ` body=${body}\n`);
44+
}
45+
});
46+
page.on('response', (res) => {
47+
if (!res.url().includes('/redpanda.api.')) {
48+
return;
49+
}
50+
appendFileSync(REQUEST_LOG, `RES ${res.status()} ${res.url()}\n`);
51+
});
52+
}
53+
54+
// Per the ticket: 3 separate topic ACLs, same topic name, different operations.
55+
// Modeled as 3 separate rule-cards in the form (not one card with 3 ops).
56+
const topicRuleDescribe: Rule = {
57+
id: 0,
58+
resourceType: ResourceTypeTopic,
59+
mode: ModeCustom,
60+
selectorType: ResourcePatternTypeLiteral,
61+
selectorValue: 'topic-acl2',
62+
operations: { DESCRIBE: OperationTypeAllow },
63+
};
64+
const topicRuleRead: Rule = {
65+
id: 1,
66+
resourceType: ResourceTypeTopic,
67+
mode: ModeCustom,
68+
selectorType: ResourcePatternTypeLiteral,
69+
selectorValue: 'topic-acl2',
70+
operations: { READ: OperationTypeAllow },
71+
};
72+
const topicRuleWrite: Rule = {
73+
id: 2,
74+
resourceType: ResourceTypeTopic,
75+
mode: ModeCustom,
76+
selectorType: ResourcePatternTypeLiteral,
77+
selectorValue: 'topic-acl2',
78+
operations: { WRITE: OperationTypeAllow },
79+
};
80+
81+
const consumerGroupRule: Rule = {
82+
id: 1,
83+
resourceType: ResourceTypeConsumerGroup,
84+
mode: ModeAllowAll,
85+
selectorType: ResourcePatternTypeAny,
86+
selectorValue: '',
87+
operations: {},
88+
};
89+
90+
test.describe('ACL edit preserves existing rules (UX-1217)', () => {
91+
test('adding a cg rule on edit does not wipe existing topic ACLs', async ({ page }) => {
92+
test.setTimeout(180_000);
93+
attachRequestLogger(page, 'ux-1217 add-rule repro');
94+
95+
const principal = `edit-preserve-${Date.now()}`;
96+
97+
// Step 0: Seed a SCRAM user so the principal is navigable.
98+
await page.goto('/security/users', { waitUntil: 'domcontentloaded' });
99+
await expect(page.getByTestId('create-user-button')).toBeEnabled({ timeout: 10_000 });
100+
await page.getByTestId('create-user-button').click();
101+
await page.getByTestId('create-user-name').fill(principal);
102+
await page.getByTestId('create-user-submit').click();
103+
await expect(page.getByTestId('user-created-successfully')).toBeVisible();
104+
await page.getByTestId('done-button').click();
105+
106+
// Step 1: Create 1 topic Rule with 3 operations → 3 Kafka ACLs on topic-acl2.
107+
const aclPage = new AclPage(page);
108+
await aclPage.goto();
109+
await aclPage.setPrincipal(principal);
110+
await aclPage.setHost('*');
111+
await aclPage.configureRules([topicRuleDescribe, topicRuleRead, topicRuleWrite]);
112+
await aclPage.submitForm();
113+
await aclPage.waitForDetailPage();
114+
115+
appendFileSync(REQUEST_LOG, '\n=== post-create, navigating to edit ===\n');
116+
117+
// Step 2: Open edit page — triggers ListACLs which populates the form.
118+
await page.getByTestId('update-acl-button').click();
119+
await page.waitForURL((url) => url.href.includes('/update'));
120+
121+
appendFileSync(REQUEST_LOG, '\n=== edit page loaded, about to add consumer-group rule ===\n');
122+
123+
// Step 3: Add a new rule (consumer-group, allow-all) then submit.
124+
// This is the scenario that reproduces the bug per the ticket.
125+
// Note: updateRules adds new rules starting at the next card index.
126+
await aclPage.updateRules([consumerGroupRule]);
127+
await aclPage.submitForm();
128+
await aclPage.waitForDetailPage();
129+
130+
appendFileSync(REQUEST_LOG, '\n=== post-edit-save with added cg rule ===\n');
131+
132+
// Assertion: the detail page should show BOTH the topic rule AND the consumer-group rule.
133+
// Rule count = 2 because operations are grouped per (resourceType, pattern, name).
134+
await aclPage.validateRulesCount(2);
135+
136+
// Additional structural check: the topic rule's 3 operations should still be present.
137+
// getByTestId matches the detail-page rule entries.
138+
await expect(page.getByText('topic-acl2')).toBeVisible();
139+
});
140+
});

0 commit comments

Comments
 (0)