Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Remove outlier check for retrieving membership events #15889

Conversation

jmartinesp
Copy link

@jmartinesp jmartinesp commented Jul 7, 2023

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Pull request includes a sign off
  • Code style is correct
    (run the linters)

Changes

Removed the check for the membership events for the current user being outliers when retrieving them from the /_matrix/client/r0/rooms/{roomId}/event/{eventId} endpoint.

Why

Fixes #15246.

This is specifically being spurred on to support Element X notifications for invites (element-hq/element-meta#1094, matrix-org/matrix-rust-sdk#2179)

It seems like the exception done in that check could have no reason to be applied only to out-of-band membership events (outlier), according to this comment.

Signed-off-by: Jorge Martín Espinosa <jorgem@element.io>
@jmartinesp jmartinesp requested a review from a team as a code owner July 7, 2023 13:08
@@ -207,15 +207,12 @@ async def filter_event_for_clients_with_state(
):
allowed_user_ids.discard(user_id)

if event.internal_metadata.outlier:
# Normally these can't be seen by clients, but we make an exception for
# for out-of-band membership events (eg, incoming invites, or rejections of
Copy link
Contributor

@MadLittleMods MadLittleMods Jul 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, out-of-band membership events has some docs written here: docs/development/room-dag-concepts.md#out-of-band-membership-events

We might want to better clarify it once we figure out what we want/can do. If we need a new label for the "membership events for the user themselves" we should also figure that out

@@ -0,0 +1 @@
Remove outlier check for getting membership events through the `/_matrix/client/r0/rooms/{roomId}/event/{eventId}` endpoint.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably create a spec issue to track clarifying the out-of-band membership behavior, https://github.com/matrix-org/matrix-spec/issues

@@ -0,0 +1 @@
Remove outlier check for getting membership events through the `/_matrix/client/r0/rooms/{roomId}/event/{eventId}` endpoint.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR probably needs more eyes on whether we want to do this so I've put it back in the review queue

@MadLittleMods MadLittleMods requested a review from a team July 7, 2023 16:58
Co-authored-by: Eric Eastwood <madlittlemods@gmail.com>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm failing to see how this fixes the bug -- is this saying that a user should always be allowed to see their own membership event where previously we would only check this if it was an outlier?

I'm guessing it might not be an outlier if there are other users in the room, but the user in question hasn't joined yet?

I'm really not sure this matches what the spec says:

Likewise, for the user’s own m.room.member events, the user should be allowed to see the event if their membership before or after the event would allow them to see it. (For example, a user can always see m.room.member events which set their membership to join, or which change their membership from join to any other value, even if history_visibility is joined.)

@clokep
Copy link
Member

clokep commented Sep 25, 2023

@jmartinesp Is this still wanted? There's a few pending questions above to answer!

@jmartinesp
Copy link
Author

Actually, I don't think this is needed anymore. I'll close it now.

@jmartinesp jmartinesp closed this Oct 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

/_matrix/client/r0/rooms/{roomId}/event/{eventId} return Http 404 for invitation Event
3 participants