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

Clarify that room ACLs apply to EDUs #1833

Closed
wants to merge 1 commit into from

Conversation

Kladki
Copy link
Contributor

@Kladki Kladki commented May 31, 2024

Pull Request Checklist

Preview: https://pr1833--matrix-spec-previews.netlify.app

Signed-off-by: Matthias Ahouansou <matthias@ahouansou.cz>
@Kladki Kladki requested a review from a team as a code owner May 31, 2024 11:26
@@ -1212,7 +1212,7 @@ of `M_FORBIDDEN`.

The following endpoint prefixes MUST be protected:

- `/_matrix/federation/v1/send` (on a per-PDU basis)
- `/_matrix/federation/v1/send` (on a per-PDU and EDU basis)
Copy link
Member

Choose a reason for hiding this comment

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

I don't see this in the original MSC1383 nor any evidence that Synapse has historically done this.

It probably makes sense, but might need an MSC.

I'd be interested in the opinion of those around when ACLs were added, however.

Copy link
Contributor Author

@Kladki Kladki May 31, 2024

Choose a reason for hiding this comment

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

I assumed that this would have been the intention of the original MSC, but I didn't know where to look to find historical MSCs.

I'm not sure when that MSC was written however, and if that came before or after (typing and/or read-receipt) EDUs were added.

Copy link
Member

Choose a reason for hiding this comment

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

Since the MSC doesn't say EDUs are forbidden and Synapse doesn't reject incoming EDUs, I don't think it can be added to the spec without a MSC

Copy link
Member

Choose a reason for hiding this comment

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

Typing and read-receipts certainly existed before server ACLs, and yes, this would probably make sense, but unfortunately it's not what the MSC says, and not what the Synapse implementation does (indeed, there is an open issue about it: element-hq/synapse#11456).

This seems like a great idea but we'd need an MSC before we can add it to the spec. Please do open one! But I'm going to have to close this for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please do open one!

I plan to do so in the coming weeks. 👍

@richvdh richvdh requested a review from a team June 11, 2024 15:21
@richvdh richvdh closed this Jun 11, 2024
@Kladki Kladki deleted the clarify-acls-apply-to-edus branch June 11, 2024 17:45
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.

4 participants