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

Spec unsigned.membership property, per MSC4115 #1847

Merged
merged 5 commits into from
Jun 10, 2024
Merged

Spec unsigned.membership property, per MSC4115 #1847

merged 5 commits into from
Jun 10, 2024

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Jun 7, 2024

@richvdh
Copy link
Member Author

richvdh commented Jun 7, 2024

ugh this has made it appear in the AS API, which is not the intention

@richvdh richvdh marked this pull request as draft June 7, 2024 11:02
@richvdh
Copy link
Member Author

richvdh commented Jun 7, 2024

oh, maybe it's fine. I'll add a couple more words.

@richvdh richvdh marked this pull request as ready for review June 7, 2024 11:11
Comment on lines 130 to 131
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.

@richvdh richvdh enabled auto-merge (squash) June 10, 2024 12:17
@richvdh richvdh merged commit 9605763 into main Jun 10, 2024
12 checks passed
@richvdh richvdh deleted the rav/msc4115 branch June 10, 2024 12:17
@Kladki Kladki mentioned this pull request Jun 15, 2024
53 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants