Repository navigation
fix(admin): reject JWTs issued before a password change or revocation - #8235
Conversation
Store a per-user token version and embed it in the JWT. A self-service password change, an administrative password reset and an explicit revoke increment the stored version, so a token issued with the previous credential is rejected by ShiroRealm on every Admin instance without rotating the shared signing key. Tokens issued before this change carry no version claim and are treated as version 0, so rolling upgrades keep existing sessions working until the account's version is first incremented.
2a0f2a7 to
86489f4
Compare
wy471x
left a comment
There was a problem hiding this comment.
Review summary
The persisted per-user token version is the right shape for #8176: it works across Admin instances, keeps the shared signing key, and gives legacy tokens an explicit version-0 policy. I found a few correctness gaps to address before merge.
Should fix
-
Lost revocation under concurrent resets/revokes.
nextTokenVersion()(DashboardUserServiceImpl.java:405) is a read-modify-write, andupdateSelectivewrites an absolute value. Two concurrent resets can both read0and write1, so a token minted after the first commit (version 1) survives the second. That violates the acceptance criterion that a token issued before a successful reset is rejected afterward. Increment in SQL (token_version = token_version + 1) in the same statement as the password update, or lock the row (SELECT ... FOR UPDATE), and add a concurrency test. -
Rolling-upgrade policy is incomplete. Only the new code writes
token_version. An old Admin instance handling a password reset during the rolling window leaves the column at 0, so pre-reset version-0 tokens remain valid even after every instance is upgraded. The PR documents the opposite direction (old instances do not check the version) but not this one. Please document the operational rule (for example, complete the upgrade before performing resets/revokes) or add a DB-level safeguard; otherwise "revocation works across Admin instances" only holds after the window. -
revokeTokenspublishes a partial user to the audit log.DashboardUserServiceImpl.java:391-397builds a DO with only id/tokenVersion/dateUpdated, then callspublisher.onUpdated.UserChangedEvent.buildContext()readsafter.getUserName()and diffs fields, so the operation log will read likethe selector [null] ... name[admin => null] password is changed.... Populate the user name (and any fields the audit needs), or skip the event if revocation does not require an audit record.
Minor
dashboard-user-sqlmap.xmladdstoken_versionto the selective statements but not to the fullinsert/update; a DO carrying a version is silently ignored by those methods. Add it there as well, or document that only the selective methods support the field.- The
clientIdaddition toequals/hashCodeinDashboardUserDOandDashboardUserVOis unrelated to this fix; please split it out or explain why it belongs in this PR. DashboardUserMapperTest#testTokenVersionRoundTripis the only test covering the H2 column plus mapper, and the PR says it did not run locally. Please make sure CI runs it, and add a test for the full acceptance path: change password, old token rejected, fresh login accepted.
The account-disabled check is still correctly ordered before the version check, and the legacy-token behavior is well covered.
Aias00
left a comment
There was a problem hiding this comment.
Found a race in the persisted token-version update that can make a previously revoked token valid again. The existing targeted JWT, realm, user service, and controller tests pass. An additional service regression test, delayedRevocationMustNotRollBackTokenVersion, reproduces the rollback using controlled mapper interleaving.
…ation # Conflicts: # db/upgrade/2.7.2-upgrade-2.7.3-h2.sql # db/upgrade/2.7.2-upgrade-2.7.3-mysql.sql # db/upgrade/2.7.2-upgrade-2.7.3-ob.sql # db/upgrade/2.7.2-upgrade-2.7.3-og.sql # db/upgrade/2.7.2-upgrade-2.7.3-oracle.sql # db/upgrade/2.7.2-upgrade-2.7.3-pg.sql
Review follow-up for apache#8176: the version is now incremented in SQL (token_version = token_version + 1) instead of a read-modify-write, so a delayed password change or revocation cannot write back a stale value and revive a revoked token. The revoke path re-reads the stored row before publishing the audit event, and the full insert/update statements document that they intentionally leave the version alone.
|
Review follow-up pushed in
Note on CI: |
Master changed the InstanceCheckService constructor in apache#7971 without updating this fixture, so shenyu-admin test compilation fails and every PR check is blocked. Apply the same fixture change as apache#8247 to unblock CI; it merges cleanly once apache#8247 lands.
|
Summarising where the review points stand after
The checks currently failing on this PR are master's own Admin test compile ( |
|
Pushed |
Aias00
left a comment
There was a problem hiding this comment.
The earlier revocation race is addressed: password resets/changes and revocations increment token_version in SQL inside their transaction, rather than assigning a stale read-modify-write value. Reviewed login/token verification, mapper statements and schema/upgrade wiring; no remaining actionable finding. Focused Admin security, mapper, compatibility and migration tests: 190 tests passed.
Checkstyle and a separate Apache RAT check passed locally. This review covers source and the stated targeted verification; full Docker integration/E2E suites were not run locally.
Fixes #8176
Problem
The Admin realm verifies the JWT signature/expiry and the current account status, but never compares token state with the account's credentials.
modifyPasswordupdates the password hash without revoking tokens issued with the old password, so an old token stays accepted for up to its 24h lifetime — on every Admin instance.Modifications
A per-user token version is now persisted and carried in the JWT:
dashboard_user.token_versionin the MySQL/PostgreSQL/Oracle/H2 init scripts and in the 2.7.2→2.7.3 upgrade scripts (MySQL, PG, Oracle, H2, OceanBase, openGauss). New accounts start at 0.JwtUtilswrites thetokenVersionclaim on login and reads it back; tokens without the claim are reported asLEGACY_TOKEN_VERSION(0).ShiroRealmrejects a token whose version differs from the stored one, after signature and expiry verification.token_version = token_version + 1,DashboardUserMapper#incrementTokenVersion). Concurrent changes cannot overwrite each other with a stale read-modify-write value, and the credential write and the increment share one transaction.PUT /dashboardUser/revoke/{id}(system:manager:edit, root admin) revokes every token of an account without changing its credential. It re-reads the stored row before publishing the audit event, andUserChangedEventnow reports the version change.insert/updatemapper statements intentionally omit the column (documented in the XML): a new row takes the default 0, and a full update must not reset revocation state.Because the version lives in the database, revocation applies on every Admin instance and does not rotate the shared signing key. Other users' tokens are untouched, and the existing account-disabled rejection is unchanged and still runs before the version check.
Compatibility / rolling upgrade
tokenVersionclaim and are treated as version 0, so existing sessions survive the upgrade and keep working while the account's version is 0.Verifying this change
JwtUtilsTest27/27 — claim round-trip and a legacy (no-claim) token reported as version 0.ShiroRealmTest6/6 — matching version accepted; bumped version rejected; legacy token accepted at 0 and rejected after the account moves to 1;testPasswordChangeInvalidatesOldTokenAndAcceptsFreshLogincovers the acceptance path end to end.DashboardUserServiceTest13/13 — password change and admin reset bump the version through the atomic statement;revokeTokensnever writes an absolute version and publishes the stored row; non-admin revoke is rejected.DashboardUserControllerTest10/10 — new endpoint wiring.DashboardUserMapperTest#testIncrementTokenVersionand#testConcurrentIncrementsAreAtomic(8 threads × 5 increments) prove the SQL-side increment loses no updates../mvnw -pl shenyu-admin validate— checkstyle green.InstanceCheckServicegained anInstanceRetentionPropertiesconstructor argument in [ISSUE #7717] Add opt-in retention cleanup for expired instance records #7971 without a fixture update), so this branch carries the same fixture change as test(admin): adapt long-polling fixtures to instance retention #8247 (22b3896d4) to unblock CI; it merges cleanly once test(admin): adapt long-polling fixtures to instance retention #8247 lands.Make sure that:
./mvnw clean install -Dmaven.javadoc.skip=true— the module's targeted tests and checkstyle were run; the full install was not run in this environment.