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.

Sign up for GitHub

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

Spec unsigned.membership property, per MSC4115 #1847

Merged
merged 5 commits into from
Jun 10, 2024
Merged

Spec unsigned.membership property, per MSC4115 #1847

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0697c82
Spec `unsigned.membership` property, per MSC4115
richvdh Jun 7, 2024
31e575d
fix link
richvdh Jun 7, 2024
1c6fd8e
Add note about AS API.
richvdh Jun 7, 2024
a0244f6
Update data/api/client-server/definitions/client_event_without_room_i…
richvdh Jun 10, 2024
5136a35
Update data/api/client-server/definitions/client_event_without_room_i…
richvdh Jun 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelogs/client_server/newsfragments/1847.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add the new `unsigned.membership` property to events served over the client-server API, as per [MSC4115](https://github.com/matrix-org/matrix-spec-proposals/pull/4115).
20 changes: 20 additions & 0 deletions data/api/client-server/definitions/client_event_without_room_id.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ properties:
"origin_server_ts": 1632491098485,
"unsigned": {
"age": 1257,
"membership": "leave"
}
}
transaction_id:
Expand All @@ -112,3 +113,22 @@ properties:
this.
title: EventContent
type: object
membership:
description: |
The room membership of the user making the request, at the time of the event.

This property is the value of the `membership` property of the
requesting user's [`m.room.member`](/client-server-api#mroommember)
state at the point of the event, including any changes caused by the
event. If the user had yet to join the room at the time of the event
(i.e, they have no `m.room.member` state), this property is set to
`leave`.

It is recommended that homeservers SHOULD populate this property
richvdh marked this conversation as resolved.
Show resolved Hide resolved
wherever practical, but they MAY omit it if necessary (for example,
if calculating the value is expensive, servers might choose to only
implement it in encrypted rooms). The property is *not* normally populated
in the application service API (where there is no clear definition of "requesting user").
Copy link
Member

Choose a reason for hiding this comment

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

I couldn't find this bit about application services in the MSC -- I would think you'd populate it with whoever the AS is acting as?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, we didn't really think about this during the MSC; the "not normally populated" comes from the fact that the synapse impl doesn't populate it.

I would think you'd populate it with whoever the AS is acting as?

The problem is that, typically, an AS doesn't just act as one user. Sure it often has a "primary" mxid for the AS as a whole (@irc:matrix.org, or so), but I'm not sure if that mxid's membership of a given room is either here or there.

I guess it all ties into if/how e2ee works for ASes, which I now can't remember.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking the "virtual user" provided by the user_id query parameter: https://spec.matrix.org/v1.10/application-service-api/#identity-assertion

But now we're kind of making stuff up that's not in the MSC. So probably best to leave it and if someone else wants it for AS they can define that in a new MSC.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, I think we're at cross-purposes a bit.

By "the application service API", I am specifically referring to events sent to the AS via PUT /_matrix/app/v1/transactions/{txnId} requests to the AS; in that case there is no "virtual user". (And, having confirmed with @Half-Shot how E2EE works for ASes, I don't think it makes sense to include membership there).

Seems like I need to reword this a bit for clarity.

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
implement it in encrypted rooms). The property is *not* normally populated
in the application service API (where there is no clear definition of "requesting user").
implement it in encrypted rooms). The property is *not* normally populated
in events pushed to application services via the application service transaction API
(where there is no clear definition of "requesting user").

Copy link
Member

Choose a reason for hiding this comment

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

That sounds good, can you add a cross link to the AS docs?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeees, but then it looks even more ridiculous in the AS docs itself

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean that it looks ridiculous? I'm ok with it as is, if you think it looks best now.

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean that it looks ridiculous?

Currently, under https://pr1847--matrix-spec-previews.netlify.app/application-service-api/#put_matrixappv1transactionstxnid, we have:

image

That looks moderately ridiculous imho (why is it listed at all?), but it's hard to do much about. OTOH, if it links straight back to https://pr1847--matrix-spec-previews.netlify.app/application-service-api/#put_matrixappv1transactionstxnid, it might be even sillier.

Or maybe we should accept the awkwardness, and at least linking it would be clear?

Copy link
Member

Choose a reason for hiding this comment

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

Ah -- the same bit gets added in both endpoints. I understand now. 👍 So yes -- I agree that would be a silly link to include.

I think ideally it wouldn't even be listed on the AS spec, but I have no idea how much refactoring that would need.

type: string
example: join
x-addedInMatrixVersion: "1.11"
3 changes: 2 additions & 1 deletion data/event-schemas/examples/core/room_event.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
"sender": "@example:example.org",
"origin_server_ts": 1432735824653,
"unsigned": {
"age": 1234
"age": 1234,
"membership": "join"
}
}