Skip to content

fix(admin): reject JWTs issued before a password change or revocation - #8235

Merged
Aias00 merged 4 commits into
apache:masterfrom
hanzhe-one:fix/admin-jwt-revocation
Oct 30, 2026
Merged

Aias00 merged 4 commits into
apache:masterfrom
hanzhe-one:fix/admin-jwt-revocation

Conversation

@hanzhe-one

@hanzhe-one hanzhe-one commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

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. modifyPassword updates 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_version in 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.
  • JwtUtils writes the tokenVersion claim on login and reads it back; tokens without the claim are reported as LEGACY_TOKEN_VERSION (0).
  • ShiroRealm rejects a token whose version differs from the stored one, after signature and expiry verification.
  • A self-service password change, an administrative password reset and an explicit revoke increment the version in SQL (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, and UserChangedEvent now reports the version change.
  • The full insert/update mapper 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

  • Tokens issued before this change have no tokenVersion claim and are treated as version 0, so existing sessions survive the upgrade and keep working while the account's version is 0.
  • After the first password change or revoke the account moves to version 1; legacy tokens and tokens minted by a not-yet-upgraded Admin instance (which cannot write the claim) are then rejected.
  • Operational rule: complete the upgrade before performing password resets or revocations. An old Admin instance cannot increment the version, so a reset it handles leaves the column at 0 and pre-reset version-0 tokens remain valid. Old instances also do not check the version, so a revoked token is accepted by them until they are upgraded. No per-user key rotation is involved.

Verifying this change

  • JwtUtilsTest 27/27 — claim round-trip and a legacy (no-claim) token reported as version 0.
  • ShiroRealmTest 6/6 — matching version accepted; bumped version rejected; legacy token accepted at 0 and rejected after the account moves to 1; testPasswordChangeInvalidatesOldTokenAndAcceptsFreshLogin covers the acceptance path end to end.
  • DashboardUserServiceTest 13/13 — password change and admin reset bump the version through the atomic statement; revokeTokens never writes an absolute version and publishes the stored row; non-admin revoke is rejected.
  • DashboardUserControllerTest 10/10 — new endpoint wiring.
  • DashboardUserMapperTest#testIncrementTokenVersion and #testConcurrentIncrementsAreAtomic (8 threads × 5 increments) prove the SQL-side increment loses no updates.
  • ./mvnw -pl shenyu-admin validate — checkstyle green.
  • Master's own test compile is broken (InstanceCheckService gained an InstanceRetentionProperties constructor 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:

  • You have read the contribution guidelines.
  • You submit test cases (unit or integration tests) that back your changes.
  • Your local test passed ./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.

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.
@hanzhe-one
hanzhe-one force-pushed the fix/admin-jwt-revocation branch from 2a0f2a7 to 86489f4 Compare October 29, 2026 12:41

@wy471x wy471x left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Lost revocation under concurrent resets/revokes. nextTokenVersion() (DashboardUserServiceImpl.java:405) is a read-modify-write, and updateSelective writes an absolute value. Two concurrent resets can both read 0 and write 1, 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.

  2. 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.

  3. revokeTokens publishes a partial user to the audit log. DashboardUserServiceImpl.java:391-397 builds a DO with only id/tokenVersion/dateUpdated, then calls publisher.onUpdated. UserChangedEvent.buildContext() reads after.getUserName() and diffs fields, so the operation log will read like the 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

  1. dashboard-user-sqlmap.xml adds token_version to the selective statements but not to the full insert/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.
  2. The clientId addition to equals/hashCode in DashboardUserDO and DashboardUserVO is unrelated to this fix; please split it out or explain why it belongs in this PR.
  3. DashboardUserMapperTest#testTokenVersionRoundTrip is 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 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@hanzhe-one

Copy link
Copy Markdown
Contributor Author

Review follow-up pushed in a8e3660e6:

  1. Atomic increment (P1). The version now moves in SQL — UPDATE dashboard_user SET token_version = token_version + 1 ... via DashboardUserMapper#incrementTokenVersion — used by the self-service change, the administrative reset and the revoke action. There is no read-modify-write left, so a delayed update cannot restore a stale value. The credential write and the increment run in one transaction (@Transactional added to modifyPassword and update). DashboardUserMapperTest#testConcurrentIncrementsAreAtomic runs 8 threads × 5 increments against H2 and asserts all 40 land; the service tests assert the atomic statement is used and that no absolute version is written.
  2. Rolling-upgrade policy. The PR description now states the operational rule: complete the upgrade before performing resets/revokes, because an old instance cannot increment the column, and old instances do not check the version either.
  3. Audit event. revokeTokens re-reads the stored row after the increment and publishes that row, so UserChangedEvent.buildContext() no longer sees null account fields; it also reports token version[x => y].
  4. Full insert/update. Documented in the mapper XML why they intentionally leave the column alone (new rows take the default 0; a full update must not reset revocation state).
  5. clientId in equals/hashCode. Reverted — the diff now only adds tokenVersion.
  6. Acceptance path. ShiroRealmTest#testPasswordChangeInvalidatesOldTokenAndAcceptsFreshLogin covers old-token-rejected / fresh-login-accepted; the H2 mapper round-trip test runs in CI.

Note on CI: pr_build currently fails at master's own test compile (InstanceCheckService constructor changed in #7971, fixture fix is #8247). With that fix applied locally, the 62 targeted tests and checkstyle pass.

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.
@hanzhe-one

Copy link
Copy Markdown
Contributor Author

Summarising where the review points stand after a8e3660e (fix(admin): increment the token version atomically), which landed after both reviews.

  1. Lost revocation under concurrent resets/revokes — fixed by incrementing in SQL: DashboardUserMapper#incrementTokenVersion runs SET token_version = token_version + 1 in the same transaction as the credential write, so there is no read-modify-write left to lose. DashboardUserMapperTest#testConcurrentIncrementsAreAtomic (8 threads × 5 increments) asserts no update is lost, and #testIncrementTokenVersion covers the single-statement path.
  2. Rolling-upgrade policy — the PR description now states the operational rule (complete the upgrade before performing resets or revokes) and covers both directions: an old instance neither writes nor checks the column.
  3. revokeTokens published a partial user — it now re-reads the stored row with selectById before publisher.onUpdated(after, before), so the audit event carries the full account rather than userName=null.
  4. Full insert/update omit token_version — documented in dashboard-user-sqlmap.xml: a new row takes the column default 0, and a full update must not reset revocation state.
  5. clientId in equals/hashCode — removed; those methods now differ only by tokenVersion.
  6. Acceptance-path coverage — ShiroRealmTest#testPasswordChangeInvalidatesOldTokenAndAcceptsFreshLogin covers change password → old token rejected → fresh login accepted. The mapper round-trip test needs a working CI run.

The checks currently failing on this PR are master's own Admin test compile (InstanceCheckService gained an InstanceRetentionProperties constructor argument in #7971; the fixture fix is #8247) and are unrelated to these changes.

@hanzhe-one

Copy link
Copy Markdown
Contributor Author

Pushed 22b3896d4 to unblock CI: master's shenyu-admin test compile is broken by #7971 (InstanceCheckService gained an InstanceRetentionProperties argument without a fixture update), which failed all six checks (build and sql-matrix are gates on list-contract). The commit carries the same fixture change as #8247 and merges cleanly once #8247 lands. Locally: HttpLongPollingDataChangedListenerTest 43/43 green, checkstyle green.

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Aias00
Aias00 merged commit 21de6be into apache:master Oct 30, 2026
56 checks passed
@Aias00 Aias00 added this to the 2.7.2 milestone Nov 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Task][Admin] Invalidate previously issued JWTs after password changes

3 participants