yyqdbngt opened a new pull request, #5353:
URL: https://github.com/apache/rocketmq-dashboard/pull/5353
### Which Issue(s) This PR Fixes
- Fixes #5311
### Brief Description
The operation audit recorded every business mutation — topics, ACLs,
credentials, settings — and none of the **authentication surface**.
`AuthController` (`/api/auth/login`, `/logout`, `/password`) never referenced
`OperationAuditService`, and neither did the user-management endpoints in
`StudioUserController` (`POST /api/studio-users`, `/{id}/status`,
`/{id}/password`, `/{id}/sessions/revoke`). The audit feed could prove an
operator changed a topic but not how that operator authenticated, nor who
handed out, disabled or re-credentialed accounts.
This change records those events, at the controller boundary the issue
points at:
| Endpoint | Operation | Operator | Detail |
| --- | --- | --- | --- |
| `POST /api/auth/login` (success) | `LOGIN` / `SUCCESS` | attempted
username | — |
| `POST /api/auth/login` (failure) | `LOGIN` / `FAILURE` | attempted
username | the message the API answered |
| `POST /api/auth/logout` | `LOGOUT` / `SUCCESS` | owner of the presented
token | — |
| `POST /api/auth/password` | `UPDATE_OWN_PASSWORD` / `SUCCESS` | the
account | — |
| `POST /api/studio-users` | `CREATE_USER` / `SUCCESS` | acting admin |
`admin=` |
| `POST /api/studio-users/{id}/status` | `UPDATE_USER_STATUS` / `SUCCESS` |
acting admin | `enabled=` |
| `POST /api/studio-users/{id}/password` | `RESET_USER_PASSWORD` / `SUCCESS`
| acting admin | — |
| `POST /api/studio-users/{id}/sessions/revoke` | `REVOKE_USER_SESSIONS` /
`SUCCESS` | acting admin | `revoked=` |
Notes on the parts that are not obvious from the diff:
- **Failure rows carry the answered message verbatim**, for the whole
`BusinessException` family: the uniform `Invalid username or password` for the
401 family (so the audit table leaks account existence no more than the API
does) and `LoginRateLimiter`'s 429 text, which `AuthService.login` raises
through the same exception type. No password material is ever recorded: `LOGIN`
rows carry the username and the answered message only, and the password
endpoints record no request body at all.
- **The actor is published for the duration of the audit call** rather than
read from the request: a login attempt happens before there is a principal, and
a logout has just given its token up. `AuthController.recordAs` sets
`AuthenticatedUserContext.setUsername(operator)` around
`operationAuditService.record(...)` and clears it in a `finally`. This is the
carrier `OperationAuditService.record` already reads (it is what
`AuthInterceptor` populates), so the alternative the issue floated — an
operator-explicit `record` overload on `OperationAuditService` — turned out to
be unnecessary API surface for a single call site. The user-management rows
need no such helper: those requests are authenticated, so the operator comes
from the request context as it does for every other audited service.
- **Logout resolves the username before revoking**:
`authService.getAuthenticatedUser(...)` is read *before*
`authService.logout(...)`, because after the revoke there is no principal left
to name. If the token was already invalid, no row is written (there is nobody
to attribute it to).
- **Audit failures stay swallowed**: `OperationAuditService.record` already
catches and logs an insert failure, so an audit-table outage cannot turn a
successful login into an error. That is asserted, not assumed.
- Two existing controller slices needed a one-line `@MockitoBean private
OperationAuditService` each (`AuthControllerTest`, `StudioUserControllerTest`):
those slices instantiate the controllers directly, so the new constructor
dependency has to be satisfied. No expectation in either file changed.
### How Did You Test This Change?
`AuthAuditTrailTest` (new, 10 tests) drives the real controllers through
`MockMvc`, with a real `OperationAuditService` writing into a mocked
`RmqOperationAuditMapper`, and captures the inserted rows. It asserts the
operator, resource type/name, result and detail of all eight endpoint/outcome
combinations above; that a 401 and a 429 login failure both record `FAILURE`
with the answered message and without the submitted password; that the
self-service and the admin password reset record no password material; and that
an `insert` failure leaves the login response (status, user, token) untouched.
The slice is registered with the production `AuthWebConfig` so the admin paths
are exercised through the real `AuthInterceptor`.
```powershell
$ cd server && mvn -B -ntp test -Dtest=AuthAuditTrailTest
[ERROR] Tests run: 10, Failures: 9, Errors: 0, Skipped: 0 <- before the
fix
[INFO] Tests run: 10, Failures: 0, Errors: 0, Skipped: 0 <- after the fix
[INFO] BUILD SUCCESS
```
Raw red run (production change stashed, test file only):
```
[ERROR] Tests run: 10, Failures: 9, Errors: 0, Skipped: 0, Time elapsed:
11.12 s <<< FAILURE! -- in org.apache.rocketmq.studio.auth.AuthAuditTrailTest
Wanted but not invoked:
org.apache.rocketmq.studio.persistence.mapper.RmqOperationAuditMapper#0.insert(
<Capturing argument: RmqOperationAudit>
);
-> at
org.apache.rocketmq.studio.auth.AuthAuditTrailTest.auditRows(AuthAuditTrailTest.java:275)
Actually, there were zero interactions with this mock.
[ERROR]
AuthAuditTrailTest.aRateLimitedLoginIsRecordedWithTheSameMessageTheApiAnswered:144->onlyRow:267->auditRows:275
[ERROR]
AuthAuditTrailTest.aSelfServicePasswordChangeIsRecordedWithoutThePassword:177->onlyRow:267->auditRows:275
[ERROR]
AuthAuditTrailTest.anAdminPasswordResetIsRecordedWithTheAffectedAccount:229->onlyRow:267->auditRows:275
[ERROR]
AuthAuditTrailTest.creatingAUserIsRecordedWithTheAffectedAccount:195->onlyRow:267->auditRows:275
[ERROR]
AuthAuditTrailTest.disablingAnAccountIsRecordedWithTheOutcome:213->onlyRow:267->auditRows:275
[ERROR]
AuthAuditTrailTest.failedLoginIsRecordedWithTheMessageTheApiAnswered:125->onlyRow:267->auditRows:275
[ERROR]
AuthAuditTrailTest.logoutIsRecordedForTheOwnerOfTheToken:159->onlyRow:267->auditRows:275
[ERROR]
AuthAuditTrailTest.revokingAUsersSessionsIsRecordedWithTheRevokedCount:245->onlyRow:267->auditRows:275
[ERROR]
AuthAuditTrailTest.successfulLoginIsRecordedWithTheAttemptedUsername:108->onlyRow:267->auditRows:275
[ERROR] Tests run: 10, Failures: 9, Errors: 0, Skipped: 0
[INFO] BUILD FAILURE
```
(The tenth test — audit-sink failure — passes before and after: it asserts
an unchanged login response, which is true on the base revision too.)
Green run after the change (same test file), with the neighbouring auth
slices:
```powershell
$ cd server && mvn -B -ntp test
"-Dtest=AuthControllerTest,StudioUserControllerTest,AuthAuditTrailTest,AuthInterceptorTest,AuthCredentialAuthorizationIntegrationTest,AuthCorsIntegrationTest"
[INFO] You have 0 Checkstyle violations.
[INFO] Tests run: 83, Failures: 0, Errors: 0, Skipped: 0
[INFO] BUILD SUCCESS
```
The full server suite was also run to check for regressions in the sessions
that were already green:
```powershell
$ cd server && mvn -B -ntp test
[INFO] Tests run: 3332, Failures: 8, Errors: 27, Skipped: 4
```
(3332 = the 3322 tests of the base revision plus the 10 new ones; the 8
failures and 27 errors are the pre-existing MySQL-8 / external-CLI classes,
identical to the base revision.)
### Checklist
- [x] One coherent change; unrelated modifications are not bundled in
- [x] Commit subject follows Conventional Commits
- [x] Tests added or updated for non-trivial changes
- [ ] New UI text has both Chinese and English entries under `web/src/i18n/`
— does not apply, this change adds no UI text; the new rows surface in the
existing audit page, which renders operation/resource/detail generically
- [ ] Architecture constraints stay green — no ArchUnit rule class exists in
the repository and this change adds none; the maven runs above report 0
checkstyle violations
- [x] New source files carry the ASF license header
- [ ] Documentation touched where behaviour changed — not ticked: the new
operations are documented in the table and notes above; `docs/` and
`server/src/main/resources/application.yml` are outside the file set this
change was scoped to, so no document was edited there
--
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]
