[2.x] fix(realtime): honour user.viewLastSeenAt in the typing indicator - #4880
Conversation
Core treats `user.viewLastSeenAt` as the override for a user's `discloseOnline`
preference — a user who hides their online status still has `lastSeenAt`
disclosed to actors holding it. The typing indicator was the one place that
override wasn't applied: the display name was scrubbed at the sender, so it
never reached anyone, and a moderator saw `[Anonymous] is typing` for someone
whose last-seen time they can read on the profile page.
It can't be scrubbed at the receiver instead: `private-typing={id}` is
subscribable by everyone who can see the discussion, so a name broadcast there
is a name disclosed to all of them. So the relay splits the event by audience.
When the typist is hiding their online status:
- the full payload goes to `private-typingIdentified={id}`, which only holders
of the permission can subscribe to (AuthController::typingIdentified);
- an anonymised payload — no name at all — goes to the discussion channel,
skipping the identified channel's subscribers, so a privileged viewer sees
one event rather than a name and an `[Anonymous]` for the same person.
When the typist is disclosing, the existing single broadcast is unchanged. When
nobody privileged is listening the identified channel doesn't exist and the
behaviour is exactly as before. Permissions are evaluated once per subscription,
against a real actor in an ordinary request, so the websocket server does no
permission work per event.
The name and the disclosure preference are no longer read from the payload.
Gating a name behind a permission makes an authority claim about who is typing,
which a client-asserted string can't back, so both are resolved from the identity
the connection authenticated with: Manager now indexes socket to user id from the
`private-user={id}` subscription (authorised only for that user, and signature
verified), and TypingIdentity looks the pair up with a short TTL cache. Senders
that can't be identified fail closed to anonymous. This also closes the existing
ability for a modified client to broadcast an arbitrary display name.
A hidden user disclosed to a permitted viewer is labelled `{username} (hidden)`,
so it's clear they are invisible to everyone else.
Fixes flarum#4879
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An interpolated translation comes back as an array of parts, so String() on it joins with commas — the indicator rendered "alice, (hidden) is typing". The existing anonymous label happened to be safe because it has no placeholders. Caught running the feature against a live daemon in a browser. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A user who hides their online status is not anonymous — the forum knows exactly who they are, and holders of user.viewLastSeenAt can now see them. "[Hidden]" describes what is actually true (hidden from *you*), reads as a status rather than a pseudonym, and matches core's own vocabulary around online-status visibility. It also puts the two states of one preference in one word: member → "[Hidden] is typing" moderator → "alice (hidden) is typing" The key keeps its name so existing translations aren't orphaned; worth renaming in a translation sync if you'd rather it read consistently. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
imorland
left a comment
There was a problem hiding this comment.
Thanks — the audience split and the identity handling both look right, and the test coverage over the privacy properties is solid.
One defect that needs fixing before merge:
TypingIdentity is never bound as a singleton, so the cache never works.
WebsocketProvider::register() singletons the websocket server's stateful services (Manager, IndexTypingPresence, PresenceChannelAuthorizer), but TypingIdentity isn't among them. resolve(TypingIdentity::class) in Message::relayTyping() therefore constructs a fresh instance on every typing event — the 5s cache never hits, PRUNE_THRESHOLD is dead code, and every ping from every typist runs a User::find() in the event loop, which is exactly what the class docblock says it's there to prevent.
The unit tests don't catch it because they register the stub with $this->container->instance(...), which is shared — production resolution isn't.
Fix is one line in WebsocketProvider::register():
$this->container->singleton(TypingIdentity::class);Ideally with a test that pins the binding (resolving it twice from the container and asserting the same instance), so this can't regress silently.
Without the binding the container returns a fresh TypingIdentity per resolution, so the cache in Message::relayTyping() never survives a single event: every typing ping from every typist ran a User::find() inside the event loop, and PRUNE_THRESHOLD was unreachable. Exactly what the class exists to avoid. Pinned by an integration test over all five stateful websocket services, since the same omission would silently defeat any of them and unit tests can't catch it — they inject their own shared stub, while production resolution isn't shared. Verified the test fails on the typing-identity case with the binding removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Good catch, and thanks for pinpointing why the unit tests were blind to it — that was the part I'd have gone looking for last. Confirmed and fixed in fcc8fdc. On the test: I pinned all five stateful services rather than just this one, since the same omission would silently defeat any of them and the failure mode is invisible — the class keeps working, just with its cache permanently empty. It's a data-provider test asserting each resolves to the same instance twice from the real container. I checked it actually fails with the binding removed (only the typing-identity case, the other four stay green) rather than trusting that it would. Also re-ran the end-to-end check against a live daemon after the change — the audience split and the identity resolution still behave correctly, so the binding didn't shift any observable behaviour beyond making the cache real. |
imorland
left a comment
There was a problem hiding this comment.
The singleton binding is right, and the test is better than what I suggested — parameterising across every stateful websocket service rather than just TypingIdentity means the next service added to WebsocketProvider is covered by construction, and the docblock explains precisely why unit tests couldn't have caught it (they inject their own shared stub).
Verified: the new test fails on exactly the typing identity cache case with the binding removed, and only that case. Realtime suites green — 41 unit, 44 integration.
Everything from my earlier review still stands: the audience split, the identity resolution from the authenticated connection, and the fail-closed behaviour for unidentifiable senders. Nice work.
Fixes #4879.
Core treats
user.viewLastSeenAtas the override for a user'sdiscloseOnlinepreference — someone who hides their online status still haslastSeenAtdisclosed to actors holding it (UserResource). The typing indicator was the one place that override wasn't applied: the display name was scrubbed at the sender, so it never reached anyone, and a moderator saw[Anonymous] is typingfor a user whose last-seen time they can read on the profile page.Scrubbing at the receiver instead isn't an option:
private-typing={id}is subscribable by everyone who can see the discussion, so a name broadcast there is a name disclosed to all of them. So the relay splits the event by audience instead.How it works
When the typist is hiding their online status:
private-typingIdentified={id}, which only holders of the permission can subscribe to;[Anonymous]for the same person.When the typist is disclosing, the existing single broadcast is unchanged. When nobody privileged is listening, the identified channel doesn't exist and behaviour is exactly as before.
Authorisation happens in
AuthController::typingIdentified(), so the permission is evaluated once per subscription against a real actor in an ordinary request — the websocket server does no permission work per event, only set arithmetic over socket IDs. This follows the pattern established byprivate-index-typing-tag=in #4756, and the relay split followsrelayIndexTyping()/relayComposeTyping().Identity is no longer taken from the payload
Gating a name behind a permission makes an authority claim about who is typing, and a client-asserted string can't back that. Both the display name and the disclosure preference are now resolved from the identity the connection authenticated with:
Managerindexes socket → user id when a connection subscribes toprivate-user={id}— an authenticated identity claim, since that channel is only signed for the matching actor and the signature is verified against the socket ID. Same reasoningrelayComposeTyping()already relies on;TypingIdentityresolves name +discloseOnlinefrom that id, cached briefly (5s, deliberately short — it bounds how long someone who has just hidden their status keeps being announced by name);As a side effect this closes the existing ability for a modified client to broadcast an arbitrary
displayNameinto a discussion.The payload shape is unchanged, so existing
client-typinglisteners keep working — including flarum/messages, whose own dialog channel this doesn't touch (covered by a test).Notes
user.viewLastSeenAtis seeded to Moderators only, so on a default install this discloses nothing to a group that can't already read the same users' last-seen time. Happy to add a setting (mirroringindex-typing-indicator-restricted) if you'd prefer it opt-in.user.viewLastSeenAtshould imply "may see who is typing while hidden", or whether this deserves its own permission (flarum-realtime.view-hidden-typers). I assumed reuse: it's what admins already reach for when they want moderators to see through a hidden online status, and it needs no migration. Straightforward to switch.{username} (hidden)rather than bare, so a moderator can tell that person is invisible to everyone else.flarum/messageshas its own copy of the typing indicator (it currently drops non-disclosing typists rather than showing[Anonymous]). Deliberately out of scope; it could follow the same shape separately.Tests
tests/unit/Websocket/TypingRelayTest.php— 7 tests over the split: name reaches the whole discussion when disclosing; name withheld from the discussion channel when hidden; name reaches the identified channel (addressed to it, so pusher-js routes it correctly); identified subscribers excluded from the anonymised broadcast; payload-claimed identity ignored; unidentifiable sender falls back to anonymous; typing on other channels passes through verbatim.tests/integration/api/TypingIdentifiedAuthTest.php— 5 tests over channel auth: permitted user 200; user with view-who-types but withoutuser.viewLastSeenAt403; guest 403; permission doesn't bypass discussion visibility; the ordinary typing channel is unaffected.Realtime's full unit (41) and integration (39) suites pass locally, PHPStan is clean, and
tsctypechecks. Nojs/distordist-typingsin the commit.