Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Support MSC3667 in a new room version #11885

Closed
wants to merge 8 commits into from
Closed

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Feb 2, 2022

Includes MSC3604's representation for testing.

MSC: matrix-org/matrix-spec-proposals#3667

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.
  • Pull request includes a sign off
  • Code style is correct
    (run the linters)

@@ -137,6 +137,22 @@ def check_auth_rules_for_event(
Raises:
AuthError if the checks fail
"""
# Before we get too far into event auth, validate that the event is even
# valid enough to be used
if event.type == EventTypes.PowerLevels:
Copy link
Member Author

Choose a reason for hiding this comment

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

this might be in the wrong place, but it's highly convenient to put it here

Copy link
Member

Choose a reason for hiding this comment

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

It seems like for similar code we usually do it in EventValidator.validate_new + event_from_pdu_json, but I'm unsure if that makes sense or not. (I was looking at where we call validate_canonicaljson)

Copy link
Member

Choose a reason for hiding this comment

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

We should really add a function to the Validator for inbound federation events, I guess

@turt2live turt2live marked this pull request as ready for review February 2, 2022 20:04
@turt2live turt2live requested a review from a team as a code owner February 2, 2022 20:04
Copy link
Member

@erikjohnston erikjohnston left a comment

Choose a reason for hiding this comment

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

Seems sane enough to convince me the MSC is implementable

if room_version_obj.msc3667_int_only_power_levels:
for k, v in event.content.items():
if k in ["events", "notifications", "users"]:
if type(v) is not dict:
Copy link
Member

Choose a reason for hiding this comment

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

One usually does isinstance(v, dict), but it doesn't really matter in this case (isinstance correctly handles subclasses and the like)

synapse/event_auth.py Outdated Show resolved Hide resolved
@@ -137,6 +137,22 @@ def check_auth_rules_for_event(
Raises:
AuthError if the checks fail
"""
# Before we get too far into event auth, validate that the event is even
# valid enough to be used
if event.type == EventTypes.PowerLevels:
Copy link
Member

Choose a reason for hiding this comment

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

We should really add a function to the Validator for inbound federation events, I guess

# If applicable, validate that the known power levels are integers
if room_version_obj.msc3667_int_only_power_levels:
for k, v in event.content.items():
if k in ["events", "notifications", "users"]:
Copy link
Member

Choose a reason for hiding this comment

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

I am somewhat tempted to just recursively check that all values are either dicts or ints? That way we don't need to remember to update this set for new fields. That would break experimental non-int fields, so maybe we could only do that on the redacted form of the power level?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm a little bit worried that'd treat ban: {} as valid :(

@erikjohnston erikjohnston added X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged z-blocked (Deprecated Label) labels Feb 23, 2022
@turt2live
Copy link
Member Author

turt2live commented May 4, 2022

This is now formally blocked behind #12623 as it needs to pull in those changes to properly support the MSC.

Edit: wrong PR :|

@richvdh
Copy link
Member

richvdh commented May 4, 2022

@turt2live what is the state of this PR?

@turt2live
Copy link
Member Author

Needs someone who isn't me to take it to merge. I can probably address some of the code quality issues, but tests are well beyond my capability.

@turt2live
Copy link
Member Author

Have modernized this to present day, though it still needs someone to take a proper crack at it.

if k in ["events", "notifications", "users"]:
if type(v) is not dict:
raise AuthError(403, "Not a valid object: %s" % (k,))
for v2 in v.values():
Copy link
Member Author

Choose a reason for hiding this comment

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

this check won't work because of how the auth rules are written: we have to check the keys explicitly.

@squahtx
Copy link
Contributor

squahtx commented Jul 11, 2022

superseded by #13220

@squahtx squahtx closed this Jul 11, 2022
@turt2live turt2live deleted the travis/msc3667 branch July 13, 2022 18:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged z-blocked (Deprecated Label)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants