unbridled-41 opened a new pull request, #4339: URL: https://github.com/apache/rocketmq-dashboard/pull/4339
Fixes #4159 (rework of #4160 following @lizhimins's review). ## Problem / Evidence `AuthService.loginDatabaseUser` (`server/src/main/java/org/apache/rocketmq/studio/auth/AuthService.java`) rejected disabled accounts with `403 "User account is disabled"` **before** any credential check. An unauthenticated caller could therefore distinguish *unknown user / wrong password* (401) from *existing disabled account* (403) — account enumeration on the login path. As a side effect those attempts never reach the login rate limiter, which records failures for 401s only (`AuthService.login`, the `catch (BusinessException … code == 401)` branch). ## Design (per the review on #4160) The review asked to keep the disabled short-circuit ahead of the expensive hash — e.g. *verify against a dummy hash* — and to cover the disabled+correct-password case. This PR implements exactly that: - The `if (!enabled)` short-circuit stays in place, so a disabled account's **stored hash is never used** for login verification (satisfies "keep the disabled short-circuit ahead of the expensive hash"). - Inside the short-circuit, the submitted password is verified against `DUMMY_PASSWORD_HASH` — a fixed, well-formed PBKDF2 hash (210 000 iterations, same parameters as real hashes) of a random preimage that ships nowhere. The result is discarded. This burns one full derivation so the response timing matches a wrong-password attempt on an enabled account (no timing oracle for the disabled state), while the dummy never matches a real credential. - The response is then the generic `401 "Invalid username or password"` — byte-identical to unknown users and enabled+wrong-password. **Conscious behavior change (please confirm this is acceptable):** "disabled account + correct password" now also returns the uniform 401 instead of the previous `403 "User account is disabled"`. Both options offered in the review (dummy-hash verification and a uniform 401) produce this outcome — a 403 that fires only for the *correct* password is impossible without running the full PBKDF2 derivation against the account's real hash, which the review asked to avoid. The new behavior is pinned by a dedicated test; if you'd rather keep the 403 for correct passwords, the alternative is verifying against the real hash before revealing state, and I'm happy to rework. **Rate limiter:** `LoginRateLimiter` code and policy are untouched. Because disabled attempts now produce the same uniform 401, they flow through the existing `code == 401` failure recording like any other failed login, so the per-username lockout applies to them too. Note this also closes the old asymmetry where the 6th attempt (401/429 for unknown and enabled vs 403 for disabled) itself revealed the disabled state; after this change the limiter behaves identically for every failure class. ## Tests (`AuthServiceDatabaseTest`, 6 new) 1. `disabledAccountsWithWrongPasswordsGetTheUniformInvalidCredentialsResponse` — case (a): 401 generic, not 403. 2. `disabledAccountsWithCorrectPasswordsGetTheUniformInvalidCredentialsResponse` — case (b), the review's requested case: uniform 401 and `sessionMapper.insert` never called (no token issued). 3. `disabledAccountVerificationNeverTouchesTheAccountPasswordHash` — with a mocked `PasswordHasher`: `matches` is **never** called with the account's stored hash, and the one call it receives uses a `pbkdf2$210000$…` hash (proving the dummy verification is a full-cost derivation, not a fast reject). 4. `unknownUsersShareTheUniformInvalidCredentialsResponse` — case (c). 5. `enabledAccountsWithWrongPasswordsGetTheUniformInvalidCredentialsResponse` — case (d). 6. `disabledAccountFailuresAreRateLimitedLikeOtherFailedLogins` — case (e): five failures reach `LoginRateLimiter.MAX_FAILED_ATTEMPTS` and the sixth attempt gets `429 Too many failed login attempts…`; the pre-existing unknown-user lockout test (`repeatedFailedLoginsAreRejectedWithTooManyRequestsTest`) is untouched and still passes. Results: - Red on unmodified base `25132a0d` (fix stashed): `Tests run: 27, Failures: 4` — the disabled+wrong, disabled+correct, never-touches-real-hash and disabled-lockout tests fail with `expected: 401 but was: 403`; the unknown-user and enabled+wrong guard tests pass on both base and fix. - Green with the fix: `mvn -o test -Dtest=AuthServiceDatabaseTest` → `Tests run: 27, Failures: 0`. - Related auth modules: `AuthServiceDatabaseTest,AuthServiceTest,LoginRateLimiterTest,AuthControllerTest` → `Tests run: 64, Failures: 0, Errors: 0`, BUILD SUCCESS. - Full backend suite `mvn -o test` → `Tests run: 2351, Failures: 0, Errors: 13`. All 13 errors are Spring `ApplicationContext` load failures in the same 8 integration-test classes (`HealthProbeIntegrationTest`, `StudioApplicationTest`, `AuthServiceBootstrap/Concurrency/SessionOverviewIntegrationTest`, `QueryHistoryServiceIntegrationTest`, `NativeAlertEvaluationTransactionTest`, `NotificationOutboxMapperIntegrationTest`); running exactly those classes on pristine base with the fix stashed reproduces `Tests run: 13, Failures: 0, Errors: 13` — a pre-existing environment limitation of this sandbox, zero new failures. ## Risk Low. One branch of one method changes its response from a pre-credential 403 to a post-dummy-verification uniform 401; no API shape, persistence, or limiter code changes. The one visible product change (disabled+correct-password users now see the generic invalid-credentials response instead of "User account is disabled") is intentional, tested, and called out above for review. -- 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]
