CalvinKirs commented on PR #67792:
URL: https://github.com/apache/doris/pull/67792#issuecomment-5708061323
<!-- doris-repo-review:v1:begin -->
### Local pipeline review — ✅ PASS
```yaml
schema: doris-repo-review/v1
status: PASS
pr: apache/doris#67792
commit: b17c6f3bf11226cda9657f557dfe565ab3c476ea
base: 92b46f325eee7339ad659520142c01762302d4cc
reviewed_at: 2026-09-17T11:32:55+08:00
reviewer: CalvinKirs
model: claude-fable-5-1
effort: xhigh
findings: {blocker: 0, major: 0, minor: 0, nit: 1}
rounds: 3
converged: true
```
**Scope**
Durable, MySQL-compatible `ACCOUNT_LOCK` / `ACCOUNT_UNLOCK`. A persisted
`manuallyLocked` flag on `PasswordPolicy.FailedLoginPolicy`, `ALTER USER ...
ACCOUNT_LOCK` as a real `LOCK_ACCOUNT` op through the existing `OP_ALTER_USER`
journal, `CREATE USER ... ACCOUNT_LOCK` honored via `PasswordPolicy.update`,
one shared predicate `Auth.checkAccountLocked` enforced on every authentication
path, new `ERR_ACCOUNT_HAS_BEEN_LOCKED` (3118), `SHOW CREATE USER` output. 11
files, +524/−7, FE only.
**Review history**
Three rounds. Round 1 (efedba8): `ACCOUNT_LOCK` combined with policy options
was half-applied, root could lock itself, and LDAP / integration / plugin
logins bypassed the lock. Round 2 (af3f513): the certificate-only early return
in `AuthenticatorManager.authenticate` still skipped the check, and the LDAP
branch read `currentUser.get(size - 1)`. Round 3 (b17c6f3): all five points
verified fixed, nothing new introduced.
**Verification at the PR head**
- Every login path now runs the same lock predicate: local password inside
`checkAccountLockedAndPasswordExpiration`; LDAP, integrations, plugins and the
certificate-only path through `AuthenticatorManager.refuseIfAccountLocked`
(both the `shouldSkipPasswordVerification` return and
`finishSuccessfulAuthentication`); HTTP / stream load / thrift / Arrow Flight
through `Auth.checkPlainPassword`, whose LDAP branch checks
`currentUser.get(0)`. An identity with no Doris policy entry is skipped by the
`hasUser` guard, so LDAP-only users are unaffected. The refusal happens before
`setCurrentUserIdentity`, sends the 3118 packet once, and is not counted as a
failed login.
- `AlterUserInfo.validate`: lock/unlock and policy options are independent
`if`s, so the combined statement hits the existing one-operation error; `ALTER
USER root ACCOUNT_LOCK` is refused.
- Persistence: `@SerializedName` on the existing GSON image path; an image
without the field reads as unlocked, an image with it is ignored by older
binaries. Replay goes through the same `alterUserInternal`. An older FE
replaying `LOCK_ACCOUNT` logs an ERROR and stays unlocked, which the PR body
now states correctly.
- Locally at b17c6f3: `AccountLockTest` 7/7, `AuthenticatorManagerTest`
25/25, `AlterUserCommandTest` 1/1, checkstyle clean.
**Checkpoints (Part 1.3)**
- Goal / test proof: yes — unit tests cover lock/unlock via SQL, `CREATE
USER ... ACCOUNT_LOCK`, policy edits keeping the lock, unlock clearing the
failed-login lock, journal replay, GSON round trip both directions, combined
options rejected without partial state, root refused, the shared predicate, and
the certificate-only path; the regression suite covers the end-to-end login
refusals.
- Small, clear, focused: yes; the follow-ups folded the two duplicate
refusals into one helper.
- Concurrency: flag read/written under `PasswordPolicy`'s read/write lock;
`policyMap` is a ConcurrentMap with `putIfAbsent`; only a boolean inside the
lock; no new lock order.
- Lifecycle / static init: none. `DROP USER` removes the policy entry, so a
re-created user starts unlocked.
- Configuration items: none.
- Incompatible changes / rolling upgrade: image and journal both backward
and forward compatible; mixed-version window documented in the PR body.
- Functionally parallel paths: all authentication entry points enumerated
above are covered.
- Special conditional checks: `policies.size() > 8`, the `hasUser` guard and
the split `if`s each carry a comment.
- Test coverage: positive, negative (combined options, root, correct
password refused, error text), replay, compatibility.
- Test results (.out): none; the suite asserts in Groovy.
- Observability: refused logins land in the ConnectProcessor failure log
with "Account is locked"; consistent with the other `alterUserInternal`
branches having no INFO log.
- Transaction / persistence / EditLog: existing `OP_ALTER_USER` path, replay
equivalent.
- Data writes / FE↔BE variables: n/a, no BE change.
- Performance: one map lookup and a boolean read per authentication.
**Notes for maintainers**
- nit: the `skip_localhost_auth_check` bypass for root/admin in
`Auth.checkPassword` skips the policy check, but
`finishSuccessfulAuthentication` now applies the administrative lock anyway, so
a locked `admin` cannot use the 127.0.0.1 bypass and must be unlocked by root.
That is the right behavior for an administrative lock; worth one line in the
docs when the docs PR lands.
- Docs PR is still to follow, per the author.
<!-- doris-repo-review:v1:end -->
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
