Use a single room per server-user pair for events#3910
Conversation
…realm-server-session
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7cd147b2c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 'JOIN realm_user_permissions rup', | ||
| 'ON rup.username = sr.matrix_user_id', | ||
| 'WHERE rup.realm_url =', | ||
| param(realmURL), | ||
| 'AND (rup.read = true OR rup.write = true)', |
There was a problem hiding this comment.
This query now only returns session rooms for users that have explicit realm_user_permissions rows (it joins on rup.username = sr.matrix_user_id). For realms that are world‑readable via the '*' permission row, users who only have public access will not match this join, so fetchAllSessionRooms returns no rooms for them. Because NodeAdapter.broadcastRealmEvent relies on this mapping, those public users will stop receiving realm events (e.g. incremental index invalidations) even though they can read the realm. This regression happens specifically for realms that are readable via '*' but have no per‑user permission rows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Yes. Only specifically mentioned users with read or write access are now going to be notified of updates. This is an expected and OK change. Pinging all users if we change catalog could be a huge issue, and should be solved in a different way so this drop of functionality is expected.
Preview deployments |
…realm-server-session
…realm-server-session
…realm-server-session
…users are created in the test setup
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 289cf14ced
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const content = event.content as RealmEventContent; | ||
| if (!content.realmURL) { | ||
| realmEventsLogger.debug( | ||
| 'Ignoring realm event because no realm found', | ||
| 'Ignoring realm event because no realm URL was provided', | ||
| event, | ||
| ); | ||
| } else { | ||
| if (realmResourceForEvent.info?.realmUserId !== event.sender) { | ||
| realmEventsLogger.warn( | ||
| `Realm event sender ${event.sender} is not the realm user ${realmResourceForEvent.info?.realmUserId}`, | ||
| event, | ||
| ); | ||
| } | ||
|
|
||
| (event.content as any).origin_server_ts = event.origin_server_ts; | ||
| this.messageService.relayRealmEvent( | ||
| realmResourceForEvent.url, | ||
| event.content as RealmEventContent, | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| (content as any).origin_server_ts = event.origin_server_ts; | ||
| this.messageService.relayRealmEvent(content); |
There was a problem hiding this comment.
processDecryptedEventFromAuthRoom now relays any APP_BOXEL_REALM_EVENT_TYPE event as long as content.realmURL exists, but this path is reached for every non-AI room event. That means a participant in any unrelated DM/group room can send a crafted realm event and trigger invalidations for whatever realm URL the client is subscribed to. Previously this code first resolved event.room_id to a known session room before relaying, so removing that check introduces spoofable cross-room realm updates.
Useful? React with 👍 / 👎.
| if (rows.length === 0) { | ||
| throw new Error( | ||
| `Cannot set session room for user ${matrixUserId}: user does not exist in the users table`, | ||
| ); |
There was a problem hiding this comment.
upsertSessionRoom now throws when the matrix user has no users row, and Realm's /_session login flow calls this path during session creation. For first-time/non-provisioned matrix users, this turns session creation into a 500 instead of issuing a JWT (or a controlled 4xx), which is a regression from the previous upsert behavior that created session-room mappings independently of users rows.
Useful? React with 👍 / 👎.
habdelra
left a comment
habdelra
left a comment
There was a problem hiding this comment.
looks good. I assume this change is not backwards compatible, meaning that we'll need to abandon AI conversations that were created from before this is merged?
I think it's mostly backwards compatible, this doesn't touch how the ai conversations work, and the session rooms are (relatively) ephemeral anyway. If the session room we create for login is in existing users dms then they should already be in the room we'll use for indexing events, if not they won't get them until they get a new jwt. |
…o-session-rooms.js
…r-session' of github.com:cardstack/boxel into cs-9552-refactor-realm-event-sending-to-use-realm-server-session
…realm-server-session
3 participants
Currently we require that every realm-user pair has a unique room.
Instead, we can have a single server user, and a single dm room for events between the server and user. Indexing events have realm URLs added so that the host can correctly identify which realm to update.
Since users have just one session room, this has been added as a column on the users table and the session_rooms table has been removed.
We also now only notify users that have read or write access to a realm if they are explicitly identified in the permissions.
Most of the changes are in tests to make sure that users are created and have the right access, partly to handle the hard coded details in the realm server about owners.