btlqql opened a new pull request, #4686: URL: https://github.com/apache/rocketmq-dashboard/pull/4686
<!-- Base branch: `rocketmq-studio`, the RocketMQ Studio trunk. --> ### Brief Description `AuthService.changePassword` (server/src/main/java/org/apache/rocketmq/studio/auth/AuthService.java:346) makes two writes: it replaces `password_hash` and then revokes every open session of the account. The method was not transactional, so the two statements auto-committed independently. If the revoke failed - a transient connection or lock error, in the middle of a security-relevant operation - the request answered `500` while the new hash had already been stored and the old sessions were still valid: the password change was half applied, and the sessions the change was supposed to invalidate survived it. An operator retrying the call is not helped either, because the first half has already happened. `setUserEnabled`, the sibling mutation that also updates a user row and then revokes that user's sessions, is already annotated `@Transactional` for exactly this reason. `changePassword` now carries the same annotation, so either both writes are visible or neither is. The order of the two statements is unchanged: with one transaction around them, the order no longer decides the outcome. ### How Did You Test This Change? New test class `AuthServicePasswordChangeIntegrationTest`, which injects a failing session-mapper through `@MockitoBean` and lets the account row be written and read for real, on the `dev` profile (H2 in MySQL compatibility mode fed by `classpath:db/schema.sql`). That is what makes the rollback observable: a mock-only test has no transaction to roll back and no stored row to read back. The class also keeps the happy path and the wrong-current-password path as controls. Before the fix (red), on the unmodified tree: ``` $ cd server && mvn -B -ntp test -Dtest=AuthServicePasswordChangeIntegrationTest [ERROR] Tests run: 3, Failures: 1, Errors: 0, Skipped: 0 [ERROR] AuthServicePasswordChangeIntegrationTest.aFailedSessionRevokeMustNotReplaceTheStoredPassword:95 [a password change whose session revoke failed must not be half applied] Expecting value to be false but was true ``` The stored hash really was the replacement password after the revoke failed - the assertion is on a row read back from the database, not on a mock interaction. The other two cases passed on the base tree, as they must. After the fix (green) - the new class plus every class the change can reach: ``` $ cd server && mvn -B -ntp test -Dtest='AuthServicePasswordChangeIntegrationTest,AuthServiceDatabaseTest,AuthServiceTest,StudioUserControllerTest,AuthControllerTest' [INFO] Tests run: 9, Failures: 0, Errors: 0, Skipped: 0 -- in org.apache.rocketmq.studio.auth.AuthControllerTest [INFO] Tests run: 27, Failures: 0, Errors: 0, Skipped: 0 -- in org.apache.rocketmq.studio.auth.AuthServiceDatabaseTest [INFO] Tests run: 3, Failures: 0, Errors: 0, Skipped: 0 -- in org.apache.rocketmq.studio.auth.AuthServicePasswordChangeIntegrationTest [INFO] Tests run: 16, Failures: 0, Errors: 0, Skipped: 0 -- in org.apache.rocketmq.studio.auth.AuthServiceTest [INFO] Tests run: 5, Failures: 0, Errors: 0, Skipped: 0 -- in org.apache.rocketmq.studio.auth.StudioUserControllerTest [INFO] Tests run: 60, Failures: 0, Errors: 0, Skipped: 0 [INFO] BUILD SUCCESS [INFO] You have 0 Checkstyle violations. ``` Note on the full suite: on a clean `rocketmq-studio` checkout `mvn -B -ntp test` already reports `Tests run: 3051, Failures: 6, Errors: 25, Skipped: 4`. The 11 red classes are the MySQL 8 backed Spring integration tests (`AuthServiceBootstrapIntegrationTest`, `AuthServiceConcurrencyIntegrationTest`, `AuthServiceSessionOverviewIntegrationTest`, `HealthProbeIntegrationTest`, `QueryHistoryServiceIntegrationTest`, `NativeAlertEvaluationTransactionTest`, `NotificationOutboxMapperIntegrationTest`, `RmqAlertStateMapperIntegrationTest`, `StudioApplicationTest`) plus the external-CLI ones (`CliAgentProviderTest`, `ClaudeCodeAgentProviderTest`). None of them are touched by this change, and the new test class deliberately does not use the MySQL-backed test profile. ### Checklist - [x] One coherent change; unrelated modifications are not bundled in - [x] Commit subject follows Conventional Commits (`fix:`) - [x] Tests added or updated for non-trivial changes, test methods named `...Test` - [x] New UI text has both Chinese and English entries under `web/src/i18n/` (no UI text in this change) - [x] Architecture constraints stay green (`mvn test` runs the ArchUnit checks) - [x] New source files carry the ASF license header - [x] Documentation touched where behaviour changed (no user-visible contract change: a password change still replaces the hash and revokes the sessions, or fails as a whole) -- 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]
