unbridled-41 opened a new pull request, #5072:
URL: https://github.com/apache/rocketmq-dashboard/pull/5072
## Problem
Creating an ACL user whose username already exists — or renaming one onto an
existing username — is answered with HTTP 500 "Internal Server Error" instead
of a conflict.
`rmq_acl_user` declares two unique keys
(`server/src/main/resources/db/schema.sql:291-292`):
```sql
UNIQUE KEY uk_username (username),
UNIQUE KEY uk_access_key (access_key)
```
Neither write path translates the resulting `DuplicateKeyException`:
- create: `AclController.java:112-114` → `AclService.createUser` →
`aclRepository.saveUser(user)` (`AclService.java:226`) →
`userMapper.insert(entity)` (`MybatisPlusAclRepository.java:152`);
- rename: `AclController.java:117-120` → `AclService.updateUser` (the
console's edit form sends `username`) → `aclRepository.replaceUser(merged)`
(`AclService.java:256`) → `userMapper.updateById(entity)`
(`MybatisPlusAclRepository.java:167`).
The exception reaches `GlobalExceptionHandler`, which only maps
`BusinessException` and a few MVC exceptions; everything else falls through to
the catch-all (`:109-115`) and becomes 500 "Internal Server Error". The
operator gets no way to tell "that name is taken" from "the server is broken".
The same repository already treats this class of failure as a conflict for
the plain-access account, and says so in the comment: "surface it as a conflict
like the other duplicate-key paths (e.g. CloudCredentialService)"
(`MybatisPlusAclRepository.java:255-263`). The ACL user paths were simply left
out.
## Evidence
Pre-fix, on base `a562601d`, with the two new tests in place and only the
production file at its base revision:
```
$ cd server && mvn -o test -Dtest=MybatisPlusAclRepositoryTest
[ERROR] Tests run: 25, Failures: 2, Errors: 0, Skipped: 0
saveUserShouldReportADuplicateUsernameAsAConflict:
Expecting actual throwable to be an instance of:
org.apache.rocketmq.studio.common.exception.BusinessException
but was:
org.springframework.dao.DuplicateKeyException: uk_username
at
...MybatisPlusAclRepository.saveUser(MybatisPlusAclRepository.java:152)
replaceUserShouldReportARenameOntoAnExistingUsernameAsAConflict:
Expecting actual throwable to be an instance of:
org.apache.rocketmq.studio.common.exception.BusinessException
but was:
org.springframework.dao.DuplicateKeyException: uk_username
at
...MybatisPlusAclRepository.replaceUser(MybatisPlusAclRepository.java:167)
```
The `DuplicateKeyException` is exactly what the 500 path is fed: the
handler's catch-all is reached with a `DataIntegrityViolationException` and no
status mapping exists for it.
## Root cause
`saveUser`'s insert branch and `replaceUser`'s update branch let the
unique-key violation escape as a `DuplicateKeyException`, which the global
advice can only answer with its 500 catch-all. The visible contract ("a name
that is taken is a conflict") already exists in the sibling write path in the
same file.
## Fix
Both branches catch `DuplicateKeyException` and rethrow
`BusinessException(409, "ACL user already exists: <username>")` through one
small private helper, mirroring the wording and the reasoning of the
plain-access block a hundred lines below.
`GlobalExceptionHandler.handleBusinessException` maps the code straight to the
status (`:42-47`), so the console now receives a 409 with a message that names
the cause.
## Score
`PRIORITY = 64` (impact 22: an operator (or an API client) is told the
server failed while the request was simply a conflict, and has no way to act on
it; scope 12: ACL user create and rename; reproducibility 20: two deterministic
tests; maintenance value 10: the repository itself documents the intended
answer for this class).
`FIX_CONFIDENCE = 85`.
## Tests
```
$ cd server && mvn -o test -Dtest=MybatisPlusAclRepositoryTest #
post-fix
Tests run: 25, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS
$ cd server && mvn -o test
-Dtest='MybatisPlusAclRepositoryTest,AclServiceTest,AclControllerTest'
Tests run: 130, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS
```
The pre-fix run of the repository class (only
`MybatisPlusAclRepository.java` reverted to `a562601d`, tests kept) is the
failure quoted under Evidence. Both write paths are asserted, not one
representative: the insert branch (create) and the update branch (rename),
because the console's edit form submits `username` and can hit either.
Checkstyle runs in the `validate` phase of both commands.
## Risk
Low. The only behaviour that changes is the exception type and status for a
duplicate write; the insert branch's `user.setId(...)` is skipped exactly as it
was when the exception propagated. The successful paths are untouched (25
repository tests, 78 service tests and 27 controller tests pass unchanged), and
no caller catches `DuplicateKeyException` from these methods.
## Dedup
- `gh search prs --repo apache/rocketmq-dashboard "duplicate acl user"` →
only #1632 (merged, concurrent admin toggles, a different table); `gh search
issues "duplicate acl user"` → nothing.
- The files listed for the open ACL PRs (#4891 version filter, #4882
principal directory, #4982 rule-update validation, #4720 plain-access view) do
not include this path — `grep -F MybatisPlusAclRepository.java
/tmp/open_pr_files.txt` shows the repository only for PRs that change other
methods, and none adds conflict handling to `saveUser`/`replaceUser`.
- `git log --oneline a562601d -- .../MybatisPlusAclRepository.java` → the
plain-access conflict handling is the most recent relevant change; the user
write paths date back to the ACL pagination work.
--
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]
