[WPB-24977] Allow suspended users to keep their cookies.#5204
Conversation
4f5386a to
1137877
Compare
93e07a6 to
771c355
Compare
771c355 to
ad2c0b9
Compare
There was a problem hiding this comment.
Pull request overview
This PR changes brig’s session-cookie handling so that suspending a user/app no longer revokes their existing cookies, while still preventing suspended accounts from refreshing access tokens via POST /access.
Changes:
- Stop revoking all cookies as part of the “suspend account” flow; introduce a refresh-time status check to block suspended users from minting new access tokens.
- Add a new AuthenticationSubsystem operation intended to clean up stale cookies (expired and/or too old per
suspendInactiveUsers) when account status changes. - Add an integration test covering app token refresh behavior across suspend/unsuspend, and wire the new config field through test/canonical interpreters.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| services/brig/test/integration/API/User.hs | Extends test auth config with suspendInactiveUsers = Nothing. |
| services/brig/src/Brig/User/Auth/Cookie.hs | Minor formatting in inactivity-suspension logic. |
| services/brig/src/Brig/User/Auth.hs | Blocks access-token refresh for suspended users without revoking cookies; removes now-unused Concurrency constraints. |
| services/brig/src/Brig/Team/API.hs | Qualifies AuthenticationSubsystem import to avoid name clashes and updates constraints accordingly. |
| services/brig/src/Brig/CanonicalInterpreter.hs | Plumbs suspendInactiveUsers into AuthenticationSubsystemConfig. |
| services/brig/src/Brig/API/User.hs | Updates account-status change flow to stop revoking all cookies and instead call the new “expired/stale cookie cleanup” operation. |
| services/brig/src/Brig/API/Internal.hs | Removes now-unused Concurrency constraints from handlers. |
| services/brig/src/Brig/API/Auth.hs | Removes now-unused Concurrency constraints from handlers. |
| libs/wire-subsystems/test/unit/Wire/MiniBackend.hs | Updates default auth config with suspendInactiveUsers = Nothing. |
| libs/wire-subsystems/src/Wire/AuthenticationSubsystem/Interpreter.hs | Interprets the new RevokeAllExpiredCookies effect action. |
| libs/wire-subsystems/src/Wire/AuthenticationSubsystem/Cookie.hs | Implements the new cookie cleanup operation (currently has a logic bug vs doc/intent). |
| libs/wire-subsystems/src/Wire/AuthenticationSubsystem/Config.hs | Adds suspendInactiveUsers :: Maybe Timeout to the auth subsystem config. |
| libs/wire-subsystems/src/Wire/AuthenticationSubsystem.hs | Adds the new RevokeAllExpiredCookies effect constructor. |
| integration/test/Test/Apps.hs | Adds an integration test validating refresh behavior across suspend/unsuspend for apps. |
| changelog.d/2-features/WPB-24977-allow-suspended-apps-to-keep-their-cookies | Changelog entry for the behavior change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6d9f16f to
6247492
Compare
edac51d to
fff372a
Compare
|
@akshay i remember now why i changed the logic: if a user gets suspended for more than suspendInactive time and then reactivated, the next login attempt will suspend that user again immediately. i don't think we want that, should i revert to the previous behavior and remove all stale cookies on suspend or activate, even the ones indicating inactivity? (happy to talk about this in person later.) |
|
Ah I see, our logic for inactivity suspension was based on cookies, so we deleted all the cookies. But now we want to keep around the cookies, so this logic breaks. |
Cookies can't be used by suspended accounts for refreshing access tokens (see last commit), so this is safe.
There is a way to auto-suspend inactive users after a configurable time span. This change makes sure that expired cookies that would trigger auto-suspend immediately are removed on re-activation.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Akshay Mankar <akshay@wire.com>
The code removed here was confused: we *don't need* to revoke cookies that have expired because they will not be considered valid credentials; and we *must not* change the way suspension on inactivity works. Now the PR should apply the same rules as before when deciding suspension due to inactivity.
3118ae0 to
67cf655
Compare
67cf655 to
49d2f65
Compare
Before this PR, users lose all their cookies when suspended. Problem: Apps can't login, they get their cookies through an awkward manual process in team-management and there currently is no way to get a fresh cookie for an already-registered app, so this is bad.
With this PR, suspended users keep their cookies, and instead we guard getting new session tokens with an account status check.
related ticket
Checklist
changelog.d