unbridled-41 opened a new pull request, #4160:
URL: https://github.com/apache/rocketmq-dashboard/pull/4160
Fixes #4159.
## Problem / Evidence
`AuthService.loginDatabaseUser` checks whether the Studio user is enabled
**before** comparing the password
(`server/src/main/java/org/apache/rocketmq/studio/auth/AuthService.java`, lines
352-359 on base `0a596661`):
```java
RmqStudioUser user = findUserByUsername(request.getUsername())
.orElseThrow(() -> new BusinessException(401, "Invalid username or
password"));
if (!Boolean.TRUE.equals(user.getEnabled())) {
throw new BusinessException(403, "User account is disabled"); //
before password check
}
if (!passwordHasher.matches(request.getPassword(), user.getPasswordHash())) {
throw new BusinessException(401, "Invalid username or password");
}
```
Two consequences, both reproduced by new regression tests that fail on the
unmodified base:
1. **User enumeration** —
`loginShouldNotRevealDisabledAccountsBeforeThePasswordIsVerified`: a disabled
user with a *wrong* password gets `403 "User account is disabled"` instead of
the generic `401`. Actual red output: `Expecting message to be: "Invalid
username or password" but was: "User account is disabled"`.
2. **Brute-force bypass** —
`disabledAccountLoginsAreRateLimitedLikeWrongPasswords`: `AuthService.login`
records rate-limiter failures only for `401` (lines 138-143), so five
wrong-password attempts against a disabled account never reach
`LoginRateLimiter.MAX_FAILED_ATTEMPTS`; the sixth attempt returns `403` again
instead of the expected `429 Too many failed login attempts...`.
## Root cause / Fix
Root cause: the enabled check runs before credential verification, and the
rate limiter only counts `401` failures.
Fix: swap the two checks so the password is compared first. A disabled
account with a wrong password now gets the generic `401 "Invalid username or
password"` (identical to unknown users), which also routes those attempts
through the existing `LoginRateLimiter`. After a *correct* password, the
existing `403 "User account is disabled"` response is preserved unchanged, so
the legitimate UX for disabled accounts is intact.
## Priority & scoring
- Impact 34/40 (unauthenticated credential-hygiene defect on the login path
of every database-backed deployment) + breadth 12/20 (only the DB-backed login
flow; configured-user login unaffected) + reproducibility 20/20 (deterministic
unit-level reproduction) + maintainability value 14/20 (aligns login ordering
with the limiter's existing contract) = **PRIORITY 80** ≥ 70.
- Minimal, ordering-only change with full module test coverage →
**FIX_CONFIDENCE 95** ≥ 80.
## Tests
- `mvn -o test -Dtest=AuthServiceDatabaseTest -DfailIfNoTests=true` on base:
`Tests run: 23, Failures: 2` (the two new regressions above; the 21
pre-existing tests all pass).
- Same command after the fix: `Tests run: 23, Failures: 0, Errors: 0,
Skipped: 0`.
- Related module suite after the fix: `AuthServiceDatabaseTest,
AuthServiceTest, LoginRateLimiterTest, AuthControllerTest` → `Tests run: 60,
Failures: 0, Errors: 0` — BUILD SUCCESS.
- Full backend suite (`mvn -o test`) after the fix: `Tests run: 2134,
Failures: 3` — the 3 failures are the pre-existing baseline set
(`AuthCorsIntegrationTest` ×2,
`AliyunInstanceProviderTest.getGroupProgressShouldMapLagRowsTest` ×1),
identical to the pristine base run; **zero new failures**.
## Risk
Low. The change reorders two `if` statements in `loginDatabaseUser`; the
only observable differences are intended (wrong-password + disabled → 401
generic message instead of 403, and those attempts now count toward the
limiter). Correct-password logins against disabled accounts still return `403
"User account is disabled"`. No API shape changes, no persistence changes.
Bootstrapping/`ensureBootstrapUsers` behavior is untouched.
--
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]
