unbridled-41 opened a new pull request, #4899:
URL: https://github.com/apache/rocketmq-dashboard/pull/4899
<!-- Base branch: `rocketmq-studio`, the RocketMQ Studio trunk. -->
### Which Issue(s) This PR Fixes
- No issue tracks this defect; the searches listed under *Duplicate check*
below found none.
### Brief Description
`analyzeAclRisk` could not read the permission values its own backend
produces, so a valid
multi-action ACL rule was reported as malformed and a genuinely risky
default permission was
not reported at all.
The rule editor lets one rule carry several actions
(`web/src/pages/instance/acl.tsx:1515-1529`
is a `Checkbox.Group` offering PUB and SUB), and the repository stores them
as a single
comma-joined value:
```java
//
server/src/main/java/org/apache/rocketmq/studio/instance/acl/MybatisPlusAclRepository.java:366
String actions = joinNormalizedCsv(rule.getActions()); // "PUB,SUB"
...
topicPerms.add(rule.getResource() + "=" + actions); // :368 ->
"order-events=PUB,SUB"
```
`examineBrokerClusterAclConfig` (`MybatisPlusAclRepository.java:191-204`) is
what
`GET /api/acl/cluster-config` returns and what
`web/src/pages/instance/acl.tsx:965` feeds into
`analyzeAclRisk`, so `PUB,SUB` reaches the diagnostics verbatim.
`normalizePermission`
(`web/src/utils/aclRiskDiagnostics.ts:101-112` before this change) accepted
only a single exact
`DENY`/`PUB`/`SUB`/`ALL` token and otherwise returned `UNKNOWN`, which is
wrong three ways:
| # | Wrong behaviour |
|---|-----------------|
| 1 | An entry whose actions are valid is reported as
`INVALID_PERMISSION_ENTRY` — "权限条目格式无法识别" — on a page whose purpose is to flag
real risks. |
| 2 | `defaultTopicPerm`/`defaultGroupPerm` of `PUB,SUB` ranks `UNKNOWN` = 0
in `PERMISSION_ALLOW_RANK` (`:91-97`), so `hasDefaultAllow` (`:197-199`) is
false: `defaultAllowAccountCount` misses the account and neither
`DEFAULT_TOPIC_ALLOW` nor `DEFAULT_GROUP_ALLOW` is raised — a silent **false
negative** on an allow-by-default posture. |
| 3 | A `*=PUB,SUB` wildcard is scored `entry.permission === 'ALL' ?
'critical' : 'warning'` (`:294`, `:307`), i.e. downgraded from critical to
warning although PUB+SUB is full access. |
The fix parses the action list and ranks the strongest action in it, so
`PUB,SUB` is full access
(matching `*=ALL`), `PUB`/`SUB` alone keep their current rank, and a value
with no recognised
action still stays `UNKNOWN`. Single-action values behave exactly as before.
### Evidence (pre-fix, on the base commit `1ef5d860`, tests added and source
unreverted only for the snippets below)
```
$ cd web && npx vitest run src/utils/aclRiskDiagnostics.test.ts
✓ marks a least-privilege ACL config as healthy
✓ flags disabled ACL and missing accounts as critical risks
✓ finds duplicated access keys and over-broad admin access
✓ reports wildcard permissions and malformed permission entries
× reads the comma-joined action list the server writes for a
multi-action rule
AssertionError: expected [ 'INVALID_PERMISSION_ENTRY', …(1) ] to not include
'INVALID_PERMISSION_ENTRY'
× counts a comma-joined default permission as an allow
AssertionError: expected +0 to be 1 // defaultAllowAccountCount
× ranks a publish+subscribe wildcard as full access
AssertionError: expected [ { …(8) }, { …(8) } ] to deeply equal
ArrayContaining{…}
// WILDCARD_TOPIC_PERMISSION severity: "warning",
expected "critical"
Test Files 1 failed (1)
Tests 3 failed | 4 passed (7)
```
### Duplicate check
Searched issues and PRs (open and closed) for `aclRiskDiagnostics`, `acl
risk`, `PUB,SUB`,
`defaultTopicPerm`, `INVALID_PERMISSION_ENTRY`, `analyzeAclRisk`: the only
hits are
#4042 (a test-coverage issue for enabled-but-accountless clusters) and #3010
(the merged feature
that introduced the diagnostics) — neither covers multi-action values. No
open PR touches
`web/src/utils/aclRiskDiagnostics.ts`.
### Scoring (AGENTS.md)
`PRIORITY` = impact 22 + reach 14 + reproducibility 18 + maintenance value
14 = **68**;
`FIX_CONFIDENCE` = **92**. (Impact: a diagnostics page that both raises
false findings and misses
a real allow-by-default; reach: every ACL page view in both languages;
reproducibility: a pure
function, pinned by three deterministic tests.)
### How Did You Test This Change?
```
$ cd web && npx vitest run src/utils/aclRiskDiagnostics.test.ts
Test Files 1 passed (1)
Tests 7 passed (7)
$ cd web && npx vitest run src/utils
src/pages/instance/__tests__/AclPage.test.tsx
Test Files 21 passed (21)
Tests 156 passed (156)
$ cd web && npx tsc -b && npx eslint src/utils/aclRiskDiagnostics.ts
src/utils/aclRiskDiagnostics.test.ts
(no output, exit 0)
```
The three new cases fail on the base commit and pass with the change, so
they pin the defect
rather than the implementation.
### Checklist
- [x] One coherent change; unrelated modifications are not bundled in
- [x] Commit subject follows Conventional Commits (`fix:`)
- [x] Tests added for the changed behaviour (`aclRiskDiagnostics.test.ts`)
- [x] New UI text: none (no new user-visible string)
- [x] Architecture constraints stay green (no server change)
- [x] New source files: none (no license header needed)
- [x] Documentation: not touched — the fix aligns the code with the
documented format ("修正条目" recommendation text names `resource=PUB` etc.)
### Risk
Low. `normalizePermission` only feeds this diagnostics view. The only
intentional behaviour
changes are the three symptoms above: multi-action entries stop being
reported as malformed,
default-allow with several actions is now reported, and such a wildcard
becomes critical. A value
that genuinely has no recognised action is still `UNKNOWN`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
