Skip to content
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

Open
wants to merge 4 commits into
base: old_master
Choose a base branch
from

Conversation

Sorunome
Copy link
Contributor

@Sorunome Sorunome commented Dec 3, 2019

Rendered

Signed-off-by: Sorunome sorunome@famedly.com

proposals/2375-appservices-invite-states.md Outdated Show resolved Hide resolved
proposals/2375-appservices-invite-states.md Show resolved Hide resolved
proposals/2375-appservices-invite-states.md Outdated Show resolved Hide resolved
proposals/2375-appservices-invite-states.md Outdated Show resolved Hide resolved
proposals/2375-appservices-invite-states.md Outdated Show resolved Hide resolved
proposals/2375-appservices-invite-states.md Show resolved Hide resolved
proposals/2375-appservices-invite-states.md Show resolved Hide resolved
Copy link
Contributor

@Half-Shot Half-Shot left a 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.

@turt2live turt2live added proposal A matrix spec change proposal proposal-in-review labels Dec 3, 2019
@turt2live turt2live self-requested a review December 3, 2019 17:27
proposals/2375-appservices-invite-states.md Outdated Show resolved Hide resolved
proposals/2375-appservices-invite-states.md Outdated Show resolved Hide resolved
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
Copy link
Member

Choose a reason for hiding this comment

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

Why the unsigned block?

Copy link
Contributor

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.

Copy link
Contributor Author

@Sorunome Sorunome Dec 3, 2019

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.

Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

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

Copy link
Contributor

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
Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Member

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.

@turt2live turt2live added the kind:maintenance MSC which clarifies/updates existing spec label Apr 20, 2020
@turt2live turt2live added the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:maintenance MSC which clarifies/updates existing spec needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants