fix: revoking device does not invalidate RememberMe session#8372
fix: revoking device does not invalidate RememberMe session#8372
Conversation
f2c-ci-robot
bot
added
do-not-merge/work-in-progress
|
|
When user logs in with RememberMe ("keep login session") and admin revokes
the device, the RememberMe token was not being properly removed, allowing
the user to remain logged in.
Two root causes fixed:
1. Race condition: `rememberMeServices.loginSuccess` and
`deviceService.loginSuccess` ran concurrently, so the
REMEMBER_ME_SERIES_REQUEST_NAME attribute wasn't set yet when
createDevice tried to read it. Fixed by running rememberMeServices
sequentially first.
2. updateExistingDevice didn't update rememberMeSeriesId to the new
series when user re-logged in with RememberMe. Fixed by reading the
new series from exchange attribute after removing the old token.
Co-authored-by: ruibaby <21301288+ruibaby@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
f2c-ci-robot
bot
added
release-note
|
There was a problem hiding this comment.
Pull request overview
Fixes a login/device revocation edge case where a revoked device could still re-authenticate via a surviving RememberMe token, by ensuring the device record always tracks the current RememberMe series and by removing an ordering race during login.
Changes:
- Serialize
rememberMeServices.loginSuccess(...)ahead of other login-success side effects to ensure the RememberMe series attribute is available when device creation/update reads it. - Update existing devices to refresh
rememberMeSeriesIdfrom the exchange after invalidating the previous token.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
application/src/main/java/run/halo/app/security/device/DeviceServiceImpl.java |
Updates device update path to persist the new RememberMe series id after removing the old token. |
application/src/main/java/run/halo/app/security/LoginHandlerEnhancerImpl.java |
Enforces sequential ordering so RememberMe series attribute is set before device login success processing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var newSeriesId = | ||
| exchange.<String>getAttribute(REMEMBER_ME_SERIES_REQUEST_NAME); | ||
| if (StringUtils.isNotBlank(newSeriesId)) { | ||
| device.getSpec().setRememberMeSeriesId(newSeriesId); |
There was a problem hiding this comment.
After removing the previous remember-me token, the device spec can still retain the old rememberMeSeriesId when no new series is set on the exchange (e.g., user re-logs in without RememberMe). This leaves the persisted Device pointing at a non-existent series and causes redundant token-removal lookups on later logins/revocations. Consider clearing device.spec.rememberMeSeriesId when the exchange attribute is blank/missing so the Device state stays consistent with the token repository.
| device.getSpec().setRememberMeSeriesId(newSeriesId); | |
| device.getSpec().setRememberMeSeriesId(newSeriesId); | |
| } else { | |
| // No new remember-me series; clear any stale series id | |
| device.getSpec().setRememberMeSeriesId(null); |
| // rememberMeServices.loginSuccess must run first to set the REMEMBER_ME_SERIES_REQUEST_NAME | ||
| // attribute on the exchange before deviceService.loginSuccess reads it. | ||
| return rememberMeServices.loginSuccess(exchange, successfulAuthentication) | ||
| .then(Mono.when( | ||
| deviceService.loginSuccess(exchange, successfulAuthentication), | ||
| rememberMeRequestCache.removeRememberMe(exchange), | ||
| oauth2LoginHandlerEnhancer.loginSuccess(exchange, successfulAuthentication), | ||
| userLoginOrLogoutProcessing.loginProcessing(successfulAuthentication.getName()) | ||
| )); |
There was a problem hiding this comment.
This change fixes a subtle ordering/race issue between remember-me token creation and device persistence. There are existing remember-me unit tests in the codebase, but no coverage that asserts (a) rememberMeServices runs before deviceService reads the series attribute, and (b) re-login updates the persisted Device.rememberMeSeriesId so revocation removes the correct token. Adding a focused test around the login flow (or DeviceServiceImpl updateExistingDevice) would help prevent regressions.
When a user logs in with "keep login session" (RememberMe) selected, revoking their device from the admin panel does not terminate the session — the RememberMe cookie re-authenticates them automatically.
Two bugs contribute to this:
1. Race condition:
rememberMeSeriesIdis null on newly created devicesIn
LoginHandlerEnhancerImpl.onLoginSuccess,rememberMeServices.loginSuccessanddeviceService.loginSuccessran concurrently viaMono.when(). ThecreateDevicepath synchronously readsREMEMBER_ME_SERIES_REQUEST_NAMEfrom the exchange at subscription time, butrememberMeServices.loginSuccesssets that attribute only after an async session read + DB write complete — so it's always unset when read.Fix: run
rememberMeServices.loginSuccesssequentially first, then the rest concurrently.2.
updateExistingDevicedoes not updaterememberMeSeriesIdon re-loginWhen a user re-logs in with RememberMe,
rememberMeServices.loginSuccesscreates a new token with a new series ID.updateExistingDevicecorrectly removes the old token but saves the device still referencing the old series ID. Subsequent device revocation removes the already-gone old token while the new token survives.Fix: after removing the old token in
updateExistingDevice, updaterememberMeSeriesIdfrom the exchange attribute if a new series was set.What type of PR is this?
/kind bug
What this PR does / why we need it:
Ensures that when a device is revoked, the associated RememberMe token is correctly identified and removed, so the user cannot silently re-authenticate via their cookie.
Which issue(s) this PR fixes:
Special notes for your reviewer:
The sequential ordering change in
LoginHandlerEnhancerImpladds a small latency to the login path (waiting for RememberMe token creation before proceeding with device/OAuth2/audit operations), but correctness requires this ordering.Does this PR introduce a user-facing change?
Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
checkstyle.org/opt/hostedtoolcache/CodeQL/2.24.2/x64/codeql/tools/linux64/java/bin/java /opt/hostedtoolcache/CodeQL/2.24.2/x64/codeql/tools/linux64/java/bin/java -jar /opt/hostedtoolcache/CodeQL/2.24.2/x64/codeql/xml/tools/xml-extractor.jar --fileList=/tmp/codeql-scratch-dd1d0a2b113c8104/dbs/java/working/files-to-index8078197461987019615.list --sourceArchiveDir=/tmp/codeql-scratch-dd1d0a2b113c8104/dbs/java/src --outputDir=/tmp/codeql-scratch-dd1d0a2b113c8104/dbs/java/trap/java(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.