Keep the per-user index in sync on reauthentication

[MatthewHarrigan]

Fixes #342.

When auth.index is enabled, every successful authentication goes through Sockets.prototype.auth(), including reauth messages (_processReauth() → _authenticate() → _setCredentials()). On master, auth() pushes the socket onto _byUser[user] without checking whether it is already there, and nothing moves the socket when a reauthentication changes the user.

Effects on master:

• With maxConnectionsPerUser set, a client that reauthenticates periodically (for example on access token refresh) eventually hits the limit on its own socket. The reauth is rejected with 503 "Too many connections for the authenticated user" and the server disconnects the client.
• server.broadcast(message, { user }) iterates _byUser[user], so a reauthenticated socket receives the message once per entry.
• When a reauthentication switches the socket to a different user, the socket stays in the previous user's Sockets and Subscribers lists. Messages for the previous user still reach it and messages for the new user don't. On close, remove() only looks at the new user, so the previous user's entry is never removed and keeps using one of their maxConnectionsPerUser slots.

Changes:

• Sockets.prototype.auth() runs before the socket credentials are replaced and receives the previous user. If the user is unchanged it does nothing. Otherwise it checks the new user against maxConnectionsPerUser, removes the socket from the previous user's list and adds it to the new user's.
• Because the check runs first, a reauthentication rejected at the limit leaves the socket's credentials and index entries unchanged. The existing failed-reauth handling disconnects it, and remove() then cleans up the current user's entry.
• Subscribers.prototype.reauth() makes the same move for each indexed subscription route the socket holds. This also indexes subscriptions made before the socket authenticated.

Tests (all fail on master):

• does not count reauthentication towards the connections limit
• sends to a reauthenticated user socket once
• moves the socket to the new user when reauthenticating as a different user
• rejects reauthentication as a different user who is at the connections limit
• updates the user index when reauthenticating to and from credentials without a user
• publishes to the new user after reauthenticating as a different user
• publishes to a subscriber that authenticated after subscribing

npm test: 214 tests, 100% coverage, lint and types clean.

🤖 Generated with Claude Code

[mtharrison]

There's another subtle bug that this fixes, if a broadcast() is sent with a specific user as a target it will iterate over their sockets in _byUser sending to each. For a user that has reauthed that will result in receiving the same message several times. Worth an additional test I think @MatthewHarrigan

[damusix]

@MatthewHarrigan @mtharrison can you please file an issue for this with replication steps as to how it's failing for you today? We can look further into things from there. https://github.com/hapijs/.github/blob/master/MAINTENANCE.md

Also, thank you for disclosing AI usage.

[mtharrison]

Cheers @damusix I opened an issue #342

[MatthewHarrigan]

Changes in 030e021:

• Replaced the reauthentication test with the minimal version from the review: two reauthenticate() calls on one connection with maxConnectionsPerUser: 2. On master the second call rejects with Too many connections for the authenticated user.
• Added sends to a reauthenticated user socket once in test/listener.js. After two reauthentications, server.broadcast('x', { user: 'steve' }) delivers one update. On master it delivers three, because _broadcast() iterates _byUser[user], which holds the socket once per authentication.
• The description now references A client reauth over the same socket should not count against their max connections #342.

npm test: 209 tests, 100% coverage, lint and types clean.

[damusix]

Thanks, tests look good. Both fail on master and pass here.

Since this says Fixes #342, I'd like the user-switch case in here too, not a follow-up. Same stale index. If a socket reauths from john to ed, it stays in _byUser.john, so john's broadcasts still reach it. And remove() filters by ed on close, so the john entry never gets cleaned up and eats one of john's maxConnectionsPerUser slots for good.

Subscribers has the same bug. After the switch, publish(path, msg, { user: 'john' }) still reaches the socket, { user: 'ed' } doesn't, and the john entry sticks around after close.

Can you deindex from the old user on a switch, in both places, with tests? Watch the case where the new user is already at the limit.

Unfortunately, I can't push to you branch or I'd have done it myself. Here's what my agent generated zz-switch.js

Good find guys. @Marsup

[MatthewHarrigan]

Changes in 2bbd809:

• When a reauthentication changes the user, the socket is removed from the previous user's list and added to the new user's. This happens in Sockets and in every indexed Subscribers the socket belongs to, once per route, so two paths on the same route are not indexed twice.
• The maxConnectionsPerUser check now runs before the socket credentials are replaced. If the new user is already at the limit, the reauth is rejected with the 503 and the socket stays under its current user. The existing failed-reauth handling then disconnects it, and remove() cleans up that user's entry, so neither user's count is left wrong.
• The same code path indexes subscriptions made before the socket authenticated under the user it authenticates as.

New tests (all fail on master and on 030e021):

• moves the socket to the new user when reauthenticating as a different user: john → ed with maxConnectionsPerUser: 1. Only ed's broadcast arrives, a second john connection is accepted while the switched socket is still open, and _byUser is empty after both close.
• rejects reauthentication as a different user who is at the connections limit: ed holds the only slot and john → ed rejects with the 503. Afterwards john's slot is free and ed's socket still receives ed's broadcasts.
• updates the user index when reauthenticating to and from credentials without a user: john → app → john.
• publishes to the new user after reauthenticating as a different user covers the Subscribers side:
  • john → john leaves a single entry.
  • john → ed, where ed already has a subscriber on the route. Both sockets receive { user: 'ed' } and john's publish reaches neither.
  • Two paths on one route, and subscriptions without auth or without index.
  • ed → app → john, and an empty index after close.
• publishes to a subscriber that authenticated after subscribing

The three cases from zz-switch.js:

Case	master	2bbd809
1. broadcast(..., { user: 'john' }) after john → ed	delivered, _byUser keeps john after close	not delivered, _byUser empty after close
2. new john connection after the switched socket closed (maxConnectionsPerUser: 1)	503	accepted
3. publish('/feed', ..., { user }) after john → ed	john's arrives, ed's doesn't, _byUser keeps john after close	ed's arrives, john's doesn't, _byUser empty after close

The PR description is updated to match. npm test: 214 tests, 100% coverage, lint and types clean.

[Marsup]

I saw a possible race condition where reauth would have put back a dead socket into the dictionary, pushed a commit fixing it. If everyone is fine with it, I'll merge/release soon.