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

Jump to bottom

Support MSC3757: Restricting who can overwrite a state event #17513

Open
wants to merge 21 commits into
base: develop
Choose a base branch
from
Open

Support MSC3757: Restricting who can overwrite a state event #17513

wants to merge 21 commits into from

Conversation

AndrewFerr
Copy link
Member

@AndrewFerr AndrewFerr commented Aug 1, 2024 •
edited
Loading

Link to the MSC

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Code style is correct
    (run the linters)

@AndrewFerr
Support MSC3757
7855b97 
Restricting who can overwrite a state event
@AndrewFerr AndrewFerr requested a review from a team as a code owner August 1, 2024 03:16
@fkwp fkwp mentioned this pull request Aug 1, 2024
Closed
3 tasks
@AndrewFerr
Run power level tests on MSC3757 rooms
6a69213
@AndrewFerr
Check if @-prefixed state key is not a user ID
e481238
@AndrewFerr
Test MSC3757 auth rules
248025d
@AndrewFerr
Support MSC3757 for room versions 9 and 11
a12238d 
Support v9 because that is what the MSC says should be supported.
Keep supporting v10 because it is the current default room version.
Also support v11 because it is the latest room version.
@AndrewFerr
Copy link
Member Author

The new & edited unit tests cover the MSC's auth rules. Since they all pass, this PR is ready for review.

@clokep
Copy link
Contributor

clokep commented Aug 12, 2024

Can you expand the title of this PR? Each time I see a notification for it I wonder what MSC3757 is.

@AndrewFerr AndrewFerr changed the title Support MSC3757 Support MSC3757: Restricting who can overwrite a state event Aug 13, 2024
hughns
hughns previously requested changes Aug 13, 2024
View reviewed changes
Copy link
Member

@hughns hughns left a comment

Choose a reason for hiding this comment

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

@AndrewFerr as per conversation yesterday, I think the feature will need to be opt-in via configuration rather than enabled by default.

@hughns hughns mentioned this pull request Aug 13, 2024
Open
@AndrewFerr
Copy link
Member Author

I think the feature will need to be opt-in via configuration rather than enabled by default.

This is already opt-in by virtue of working only in rooms of a compatible version (org.matrix.msc3757.9 / .10 / .11).

erikjohnston
erikjohnston reviewed Aug 14, 2024
View reviewed changes
synapse/event_auth.py Outdated Show resolved Hide resolved
tests/rest/client/test_power_levels.py Outdated Show resolved Hide resolved
richvdh
richvdh reviewed Aug 15, 2024
View reviewed changes
synapse/event_auth.py Outdated Show resolved Hide resolved
@anoadragon453 anoadragon453 mentioned this pull request Aug 30, 2024
Open
sandhose
sandhose requested changes Sep 5, 2024
View reviewed changes
Copy link
Member

@sandhose sandhose left a comment

Choose a reason for hiding this comment

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

Looks good overall, this is mainly about code style

synapse/api/room_versions.py Show resolved Hide resolved
synapse/api/room_versions.py Outdated Show resolved Hide resolved
synapse/event_auth.py Outdated Show resolved Hide resolved
synapse/event_auth.py Outdated Show resolved Hide resolved
tests/rest/client/test_power_levels.py Outdated Show resolved Hide resolved
tests/rest/client/test_power_levels.py Outdated Show resolved Hide resolved
sandhose
sandhose reviewed Sep 9, 2024
View reviewed changes
Copy link
Member

@sandhose sandhose left a comment

Choose a reason for hiding this comment

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

Very minor nitpicks, otherwise looks good to me!

synapse/event_auth.py Show resolved Hide resolved
synapse/event_auth.py Show resolved Hide resolved
synapse/event_auth.py Outdated Show resolved Hide resolved
@sandhose sandhose dismissed hughns’s stale review September 11, 2024 13:59

This has been addressed

@AndrewFerr
Copy link
Member Author

Also note that this has Complement tests: matrix-org/complement#733

@AndrewFerr
Copy link
Member Author

(Complement CI is expected to fail until matrix-org/complement#733 is merged.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richvdh richvdh richvdh left review comments

@sandhose sandhose sandhose approved these changes

@hughns hughns Awaiting requested review from hughns

@erikjohnston erikjohnston Awaiting requested review from erikjohnston

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@AndrewFerr @clokep @richvdh @sandhose @hughns @erikjohnston