-
Notifications
You must be signed in to change notification settings - Fork 376
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MSC2375: Appservice Invite States #2375
base: old_master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems quite sensible to me.
60e323e
to
ff2c004
Compare
rooms, e.g. if a token is present and valid in the invite state. | ||
|
||
## Proposal | ||
This proposal is about adding `invite_room_state` to the `unsigned` block of invite events when sending |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the unsigned
block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
appservices only receive invites by receiving those events alongside all other events in transactions, so putting the state into those events sounded like it would make most sense. The unsigned block inside the event was chosen to not block the path for potentially sending the federation format to ASs in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, it appears that this is already the standard in the spec, as seen with the server-server invites, so creating multiple locations for the same thing seems wrong. That is also mentioned in the MSC
This way it would be in line with the Server-Server API to send invites.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is only true for the v1 invite endpoint, the v2 invite endpoint puts the invite_room_state next to the event instead of into it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must have slipped sorus eyes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think that with the current shape of the API, the unsigned block is the best position for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsigned is a general dumping ground for not-event-data, so this makes sense to me.
@@ -0,0 +1,73 @@ | |||
# MSC 2375: Proposal to add invite states to invites sent to Application Services |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need any of this? It sounds like a Synapse bug if it isn't sending invite_room_state
to the appservice. The spec says that appservices receive the same event format as the CS API, which includes invite_room_state
in m.room.member
events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't that mean that the invite room state should also be included when someone else is invited to a room the current user is already a part of? Because that does not seem to be the case.
From what I can see, the spec says that invite_room_state
may be included in the event. Maybe the way to solve this is to add conditions under which that may should instead be a should, such as when sending the event to the invitee.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, hmm. I might be misinterpreting what this MSC is trying to do: is it trying to solve invites sent towards it or other people who happen to be in the room? The reason why the invite state isn't included for people already in the room is because those people already have the context of the room.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is totally to solve invites send towards it, I just said that first part to make clear that the spec is very ambiguous about whether or not that invite_room_state
is present. Joined members can just query the room state directly, obviously.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The server should be sending it straight through to the target of the invite, whatever it is, given the current spec. The spec could probably clarify that it doesn't show up for other users though - that's not an issue for this MSC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but the spec doesn't say when it should show up either.
This event may also include an
invite_room_state
key inside the event's unsigned data. If present, this contains an array of StrippedState Events. These events provide information on a subset of state events such as the room name.
This is currently in the spec
This event may also include an
invite_room_state
key inside the event's unsigned data.invite_room_state
should be present if the member event is an invite that is being sent to the invitee, for example in a sync response or an appservice transaction. If present, this contains an array of StrippedState Events. These events provide information on a subset of state events such as the room name.
This is the clarification that would probably solve this MSC, if this is a Synapse bug and spec ambiguity instead of actually changing the intended behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
links, people. consider the next person to try to read the thread. The spec in question is https://matrix.org/docs/spec/client_server/r0.6.1#m-room-member.
I think probably all we need is some clarifications to the spec, rather than an MSC.
Rendered
Signed-off-by: Sorunome sorunome@famedly.com