Frun1na opened a new pull request, #6809:
URL: https://github.com/apache/rocketmq-dashboard/pull/6809
### Which Issue(s) This PR Fixes
- Fixes #6807
### Brief Description
The ACL write paths accepted an identity longer than the column that stores
it, so the write failed inside the insert and the caller received a
persistence-layer 500 instead of a validation error.
`rmq_acl_user.username` and `rmq_acl_rule.principal` are both `VARCHAR(128)`
(`server/src/main/resources/db/schema.sql:284`, `:267`), and nothing bounded
the values reaching them: `AclService.createUser` / `updateUser` / `createRule`
/ `updateRule` checked only that the value had text, and
`GlobalExceptionHandler` does not translate `DataIntegrityViolationException`.
The four local-store paths now bound the identity to the column through one
helper, counted in code points — the unit MySQL counts a `varchar` in — and
report the field name: `username must not exceed 128 characters` / `principal
must not exceed 128 characters`. That is the same shape `#6087` used for the
topic and group registration text, and the same reasoning
`InstanceService.requireTextWithin` documents for the instance fields.
The bound is enforced in the service rather than only on the request DTOs
because the tool handlers bypass bean validation:
`UserCreateToolHandler.execute` builds an `AclUserVO` in code and calls
`AclService.createUser` directly, and the tool catalog declares only
`minLength: 1` for that field. A DTO `@Size` would also have been the wrong
unit: it counts UTF-16 units, so it would reject a 128-code-point value made of
surrogate pairs that the column holds.
User-visible change: an over-long username or rule principal answers `400`
naming the field instead of `500`, on the REST endpoints and on the AI tools
alike.
### Other consumers
The identity columns are written in four places, and each was checked:
- `AclService.createUser` / `updateUser` — guarded here, on the local store
path.
- `AclService.createRule` / `updateRule` — guarded here; a rule principal
carries the username (or the plain-access access key), so it is the same value
in the second 128-character column.
- The Tencent branch of all four methods — deliberately **not** bounded: it
delegates to `TencentAclService` and writes a vendor role, not a row of these
tables.
- `web/src/pages/instance/acl.tsx` — the form has no length rule either, so
the console path reaches the same service; no UI change is needed because the
service answers 400 and the page already routes a rejected write to its failure
notice.
- The request DTOs (`CreateAclUserDTO`, `UpdateAclUserDTO`,
`CreateAclRuleDTO`, `UpdateAclRuleDTO`) — deliberately left as presence-only:
the guard is the code-point one in the service, and adding a UTF-16 `@Size`
beside it would refuse values the column accepts.
- `tool-catalog/tools/user.yaml` declares `minLength: 1` for `username` and
no maximum; documenting the bound in the catalog is a separate change (it would
regenerate `rmqctl/internal/catalog/catalog_gen.go`), not part of this one.
- The other text fields of the same DTOs are a different class of value and
are **not** part of this PR: `resource` (`VARCHAR(255)`), `resourceType` /
`resourcePattern` (`VARCHAR(32)`), `actions` (`VARCHAR(128)`), `decision`
(`VARCHAR(16)`), `scope` (`VARCHAR(64)`), `aclVersion` (`VARCHAR(16)`). None of
them carries a username, and bounding them needs its own per-field reasoning.
- Open PR #5878 adds the *blank* checks to `updateRule`'s principal and
resource. It validates presence and this change validates length, so the two do
not overlap in what they refuse; the guard is placed before `updateRule`'s id
check so the two edits do not sit on the same lines.
### How Did You Test This Change?
```
$ ./wb test --filter
'AclServiceTest,AclControllerTest,MybatisPlusAclRepositoryTest,IpRangeMatcherTest'
✓ 20.4s mvn -B -ntp test -DforkCount=1
-Dsurefire.failIfNoSpecifiedTests=false \
-Dtest=AclServiceTest,AclControllerTest,MybatisPlusAclRepositoryTest,IpRangeMatcherTest
[INFO] Tests run: 87, Failures: 0, Errors: 0, Skipped: 0 -- in AclServiceTest
[INFO] Tests run: 27, Failures: 0, Errors: 0, Skipped: 0 -- in
AclControllerTest
[INFO] Tests run: 26, Failures: 0, Errors: 0, Skipped: 0 -- in
MybatisPlusAclRepositoryTest
[INFO] Tests run: 15, Failures: 0, Errors: 0, Skipped: 0 -- in
IpRangeMatcherTest
[INFO] Tests run: 155, Failures: 0, Errors: 0, Skipped: 0
```
The new tests fail without the fix:
```
$ ./wb test --filter AclServiceTest
✗ mvn -B -ntp test -DforkCount=1 -Dsurefire.failIfNoSpecifiedTests=false
-Dtest=AclServiceTest
[ERROR] Tests run: 87, Failures: 4, Errors: 0, Skipped: 0
AclServiceTest.createUserShouldRejectAUsernameThatCannotFitItsColumnTest
java.lang.AssertionError: Expecting actual throwable to be an instance
of:
org.apache.rocketmq.studio.common.exception.BusinessException
but was:
java.lang.NullPointerException: Cannot invoke "…AclUserVO.getId()"
because "user" is null
```
The four assertions go red because they ask each write path to refuse a
129-code-point identity with `400` and the field's name; with the guard absent
the value passes every check and reaches the repository — the `createUser` case
shows it as the unstubbed `saveUser` returning `null` and the audit call then
dereferencing it, i.e. the value was never refused. Reverting only the guard
turns exactly those four of the 87 tests red and leaves the other 83 green, and
the positive case inside
`createUserShouldRejectAUsernameThatCannotFitItsColumnTest` — 128 code points
made of surrogate pairs — passes both with and without the guard, which is what
shows the assertions pin the column bound rather than "the method throws".
### Checklist
- [x] One coherent change; unrelated modifications are not bundled in
- [x] Commit subject follows Conventional Commits (`feat:` / `fix:` /
`refactor:` / `chore:` / `docs:` / `perf:`)
- [x] Tests added or updated for non-trivial changes, test methods named
`...Test`
- [ ] New UI text has both Chinese and English entries under `web/src/i18n/`
- [ ] Architecture constraints stay green (`mvn test` runs the ArchUnit
checks)
- [ ] New source files carry the ASF license header
- [ ] Documentation touched where behaviour changed (README / `docs/` /
in-app help)
--
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]
