-
Notifications
You must be signed in to change notification settings - Fork 379
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
MSC4163: Make ACLs apply to EDUs #4163
Merged
Merged
Changes from 6 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
cc7d6e6
Proposal to make ACLs apply to ACLs
Kladki 481381e
fix spelling mistakes
Kladki a55cab1
make behaviour more closely align with current ACL conventions, by in…
Kladki e30a503
simplify recipt logic, making it apply to all possible recipts
Kladki 4631025
clarify that ACLs should apply to all EDUs which are local to a room
Kladki 4425176
fix spelling mistakes
Kladki c1cf8d2
were -> are
Kladki de9bb4d
explain what should happen when an EDU is ACLed
Kladki 07e5b42
should -> MUST
Kladki ab3342b
explain that ACLed typing EDUs should be dropped
Kladki 93521ac
room id is dropped before processing rather than before sending
Kladki fdefcbc
remove mention of req/resp
Kladki 3c1f349
remove confusing sentence about room_id field
Kladki File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
# MSC4163: Make ACLs apply to EDUs | ||
Kladki marked this conversation as resolved.
Show resolved
Hide resolved
Kladki marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
[Access Control Lists](https://spec.matrix.org/v1.11/client-server-api/#server-access-control-lists-acls-for-rooms) | ||
(also known as ACLs) are used to prevent other servers from participating in a room at a federation level, | ||
covering many federation API endpoints, including | ||
[`/send`](https://spec.matrix.org/v1.11/server-server-api/#put_matrixfederationv1sendtxnid). However, while ACLs | ||
are applied on a per-PDU basis on this endpoint, they are not applied to EDUs at all. Considering that some EDUs | ||
are specific to certain rooms (e.g. read receipts & typing indicators), it makes sense to apply ACLs to them as well. | ||
|
||
|
||
## Proposal | ||
|
||
All EDUs which are local to a specific room should have ACLs applied to them. This means that for the EDUs currently | ||
Kladki marked this conversation as resolved.
Show resolved
Hide resolved
|
||
in the spec, ACLs would only apply to receipts and typing notifications. Examples of how ACLs should be enforced for | ||
those two types of EDUs are as follows: | ||
- For | ||
[typing notifications (`m.typing`)](https://spec.matrix.org/v1.11/server-server-api/#typing-notifications), | ||
the `room_id` field inside `content` should be checked, with the typing notification rejected if the `origin` | ||
Kladki marked this conversation as resolved.
Show resolved
Hide resolved
|
||
of the request is a server which is forbidden by the room's ACL. | ||
- For [read receipts (`m.receipt`)](https://spec.matrix.org/v1.11/server-server-api/#receipts), all receipts | ||
inside a `room_id` inside `content` should be rejected if the `origin` of the request is forbidden by the | ||
room's ACL. | ||
|
||
## Potential issues | ||
|
||
None considered. | ||
|
||
## Alternatives | ||
|
||
Leave things as-is, which wouldn't be that big of a deal when you consider that this would only apply | ||
to typing notifications and read receipts currently, which don't allow for very significant disruption inside | ||
a room. However, as ACLs are meant to prevent certain servers from participating in a room at all, it makes | ||
sense to apply ACLs to EDUs which are local to certain rooms, as they are a form of participation. | ||
|
||
## Security considerations | ||
|
||
None considered. | ||
|
||
## Unstable prefix | ||
|
||
None required, as no new fields or endpoints were added. | ||
Kladki marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
## Dependencies | ||
|
||
None. |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Implementation requirements:
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.
See server implementations in thread @ https://github.com/matrix-org/matrix-spec-proposals/pull/4163/files#r1667727322