[WPB-24977] Allow suspended users to keep their cookies. by fisx · Pull Request #5204 · wireapp/wire-server

fisx · 2026-04-26T19:18:42Z

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

Add a new entry in an appropriate subdirectory of changelog.d
Read and follow the PR guidelines

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.

Copilot

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.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

akshaymankar · 2026-05-13T14:54:53Z

@@ -0,0 +1 @@
+Allow suspended users to keep their cookies.  (This has become necessary to allow apps to keep their cookies during (sufficiently brief) suspend periods.  It works because instead, there is a new guard now preventing session token refresh for suspended users with valid cookies.)


Suggested change

Allow suspended users to keep their cookies. (This has become necessary to allow apps to keep their cookies during (sufficiently brief) suspend periods. It works because instead, there is a new guard now preventing session token refresh for suspended users with valid cookies.)

Allow suspended users to keep their cookies, but disallow them to create/refresh access tokens.

IMO this is simpler to understand for people who don't work on the codebase. People who work here can read the JIRA.

akshaymankar · 2026-05-13T14:57:25Z

+testZauthAndApps :: (HasCallStack) => App ()
+testZauthAndApps = do
+  (owner, tid, []) <- createTeam OwnDomain 1
+  (app, cookie) <- createIt owner tid


"createIt" is a really vague name, can we instead just inline this function?

akshaymankar · 2026-05-13T14:59:20Z

+  refreshSucceeds app cookie
+  suspendApp app >> refreshFails app cookie
+  unsuspendApp app >> refreshSucceeds app cookie


This seems extremely cryptic. Can we please just inline these function?

akshaymankar · 2026-05-13T14:59:45Z

+    refreshFails :: (HasCallStack, MakesValue app) => app -> String -> App ()
+    refreshFails app cookie =
+      renewToken app cookie `bindResponse` \resp -> do
+        resp.status `shouldMatchInt` 403


We should always assert on labels when we expect failures.

akshaymankar · 2026-05-13T15:05:22Z

+-- (2) cookie creation time is further in the past than
+-- `env.suspendInactiveUsers` allows.


How is suspendInactiveUsers related to cookie being stale?

Also does this have impact on existing users? If so, that must be part of changelog and we should clear it by product.

akshaymankar · 2026-05-13T15:14:07Z

+  -- It is safe to *not* revoke any cookies here; if no valid access
+  -- token is available, cookies are only validated when calling `POST
+  -- /access`, and access token refresh only works on unsuspended
+  -- users.
+  --
+  -- Evidence: `git grep -Hn --color=never 'UserToken\b' | grep libs/wire-api/src/Wire/API/Routes/Public/`.
+  --
+  -- Having that said, we need to remove cookies here that are no
+  -- longer valid for login/inactivity handling (including both
+  -- expired cookies and cookies older than the inactivity threshold),
+  -- otherwise /login considers the user inactive, see
+  -- 'mustSuspendInactiveUser'.
+  --
+  -- The intuition is that every change of account status can be
+  -- considered an account activity, so users that have their status
+  -- changed recently should not be considered inactive, even if they
+  -- haven't taken any action themselves.


I'm very confused by this reasoning. So far any user with only old cookies was considered inactive and hence suspended. You're saying if we suspend a user they've become active. How have they become active?

akshaymankar · 2026-05-13T15:15:32Z

+  -- NB: no need to call `catchSuspendedUsers` here.  `newAccess` is
+  -- called in 3 places (login, ssoLogin, legalHoldLogin), and all of
+  -- them reject suspended users before calling it.


This sounds like a reason to move the call here, instead of making it some hope based contract that every caller should call that function first.

zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Apr 26, 2026

fisx force-pushed the WPB-24977-allow-suspended-apps-to-keep-their-cookies branch 3 times, most recently from 4f5386a to 1137877 Compare April 26, 2026 21:43

fisx changed the title ~~[WPB-24977] Allow suspended apps to keep their cookies.~~ [WPB-24977] Allow suspended users to keep their cookies. Apr 27, 2026

fisx force-pushed the WPB-24977-allow-suspended-apps-to-keep-their-cookies branch from 93e07a6 to 771c355 Compare April 27, 2026 11:55

fisx added 5 commits May 13, 2026 10:31

Changelog.

48f03d1

Integration test.

bf6ddfe

Reject access token refresh requests from suspended users.

d767794

Do not evict cookies on suspend.

9467cc6

Cookies can't be used by suspended accounts for refreshing access tokens (see last commit), so this is safe.

Allow unsuspended users to login even if inactive.

ad2c0b9

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.

fisx force-pushed the WPB-24977-allow-suspended-apps-to-keep-their-cookies branch from 771c355 to ad2c0b9 Compare May 13, 2026 08:43

fisx requested a review from Copilot May 13, 2026 08:43

Copilot started reviewing on behalf of fisx May 13, 2026 08:44 View session

Copilot AI reviewed May 13, 2026

View reviewed changes

Comment thread libs/wire-subsystems/src/Wire/AuthenticationSubsystem/Cookie.hs Outdated

Comment thread libs/wire-subsystems/src/Wire/AuthenticationSubsystem.hs Outdated

Comment thread services/brig/src/Brig/API/User.hs Outdated

fisx and others added 3 commits May 13, 2026 11:47

More precise haddocks

4ca1b4a

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Fix boolean logic bug.

3b9fd8b

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Better effect action name.

6247492

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

fisx force-pushed the WPB-24977-allow-suspended-apps-to-keep-their-cookies branch from 6d9f16f to 6247492 Compare May 13, 2026 11:01

Extend changelog entry.

6766e49

fisx marked this pull request as ready for review May 13, 2026 12:53

fisx requested review from a team as code owners May 13, 2026 12:53

akshaymankar reviewed May 13, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WPB-24977] Allow suspended users to keep their cookies.#5204

[WPB-24977] Allow suspended users to keep their cookies.#5204
fisx wants to merge 9 commits into
developfrom
WPB-24977-allow-suspended-apps-to-keep-their-cookies

fisx commented Apr 26, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

akshaymankar May 13, 2026

Uh oh!

akshaymankar May 13, 2026

Uh oh!

akshaymankar May 13, 2026

Uh oh!

akshaymankar May 13, 2026

Uh oh!

akshaymankar May 13, 2026

Uh oh!

akshaymankar May 13, 2026

Uh oh!

akshaymankar May 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		@@ -0,0 +1 @@
		Allow suspended users to keep their cookies. (This has become necessary to allow apps to keep their cookies during (sufficiently brief) suspend periods. It works because instead, there is a new guard now preventing session token refresh for suspended users with valid cookies.)

		-- (2) cookie creation time is further in the past than
		-- `env.suspendInactiveUsers` allows.

Conversation

fisx commented Apr 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

akshaymankar May 13, 2026

Choose a reason for hiding this comment

Uh oh!

akshaymankar May 13, 2026

Choose a reason for hiding this comment

Uh oh!

akshaymankar May 13, 2026

Choose a reason for hiding this comment

Uh oh!

akshaymankar May 13, 2026

Choose a reason for hiding this comment

Uh oh!

akshaymankar May 13, 2026

Choose a reason for hiding this comment

Uh oh!

akshaymankar May 13, 2026

Choose a reason for hiding this comment

Uh oh!

akshaymankar May 13, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fisx commented Apr 26, 2026 •

edited

Loading