lizhimins commented on PR #5351:
URL:
https://github.com/apache/rocketmq-dashboard/pull/5351#issuecomment-6094445308
Verified against trunk: the endpoint gap is real
(AclController.java:89-110), AclUserVO/CsvUtil/
PageResult/pageUsers all exist as referenced, maskCredentials
(AclService.java:498-511) does mask
both credentials on both paths, and the three tests are genuinely
mutation-sensitive (I could name
a reverting line for every assertion). Nice work on the secret-leak
regression test.
Three things before this can land:
1. docs/api-spec.md: the maintenance contract at the top of that file (rules
6 and 7) requires the
接口速查 table and the module section to stay in sync. A new public endpoint
needs a row plus a
§7.x entry (params, text/csv response, the 10,000 bound and its 400).
2. AclService.pageUsers' Tencent branch (:192-206) calls
tencentAclService.listUsers(instanceId)
on *every* page and sublists in memory, so a full 10,000-row export issues
100 full cloud list
calls. Both paths are also non-transactional, so rows can duplicate or
vanish between pages —
in a document meant for access review. Your cited precedent
(CloudCredentialService:67) does a
single findPage(1, MAX) precisely to avoid this; please either add a
one-shot export method on
AclService or document the amplification and lower the bound for Tencent
instances.
3. Layering/shape: the page walk, the bound and the CSV rendering all live
in the @RestController.
Every other CSV export renders in the Service
(CloudCredentialService:72-78, MetadataService:728,
AuditService:88) and returns Result<String>, which the frontend reads as {
data: string } +
downloadCsv. ResponseEntity<byte[]> has precedent here but only for binary
(DLQ xlsx, Grafana
zip, alert-rule yaml). Please pick one and move the logic down.
One question for maintainers, not a defect in this PR: /api/acl/users/export
is not in
AuthInterceptor.isAdminOnlyGetPath (:174-182) and isCredentialRevealPath
(:194-196) only matches
*/credentials, so any authenticated reader can pull the whole inventory. No
new data is exposed
(/users/page is equally reader-accessible and already masked), and all 10
existing export endpoints
behave the same — but bulk-downloading admin flags plus IP whitelists is
worth an explicit call.
--
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]
