-
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
MSC3667: Enforce integer power levels #3667
Conversation
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.
Generally seems fine to me - just a bit of clarity before I think this is ready for FCP.
In a future room version, we should enforce the letter of the spec and only allow power levels | ||
as integers and reject events which try to define them as any other type. This removes all of the | ||
associated headaches with string parsing. |
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.
For clarity, which system are we hoping to enforce this within? If it's the auth rules then they might be complicated to represent, but if it's just evaluation-time parsing (ie: non-int is default power level) then that might be a bit easier to write.
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.
I don't think we need to modify the auth rules themselves here. I think an acceptable outcome is that parsing the power level content at the beginning of event auth altogether fails, therefore auth fails with it. This is what the Dendrite implementation does currently. We don't attempt to fall back to a default level, although if anyone has feelings on whether that approach would be better, I'd welcome opinions.
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.
Failing event auth sounds ideal - can we get words to that effect in the MSC? 😇
@@ -0,0 +1,42 @@ | |||
# MSC3667: Enforce integer power levels |
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.
just to pre-empt some potential discussion: other event types, like join rules, aren't really subject to the same nuances that integer-based power levels are. As a power event itself in state res v2, the power levels are critical for determining whether or not a user can do something - the same issue can theoretically arise from incorrect membership or join rule, however the handling of those two events is targeted at the specific enum values whereas power levels are a range.
All of this behaviour is exceptionally difficult for non-Python implementations to reproduce | ||
accurately, so we should not try to do so. |
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.
It is not specifically, i've drawn an algorithm to come close; (original comment)
- trim all whitespace
- remove leading
+
- if leading character is
-
, remove and remember - remove leading zeroes (except the last one, if remaining string is all-zeroes)
- interpret string as an integer
- if
-
was encountered, negate the integer
I'm plenty sure this is how python does it, and i think at least a mention/help for the spec on how to start to parse the old values would help a ton.
The spec defines power levels in `m.room.power_levels` events as integers, but due to legacy | ||
behaviour in Synapse, string power levels are also accepted and parsed. The string parsing itself |
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.
This first sentence is a little confusing. String power levels are not accepted by the spec - but in practice are often accepted by homeserver implementations as not doing so would break existing rooms.
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.
Spec PR #3688 will formally allow today's behaviour into the spec for all room versions up to and including version 9, since this issue was still not yet fixed in Synapse. That will allow us and future implementations to preserve compatibility with existing rooms.
This MSC, on the other hand, will fix the problem in future room versions (hopefully from room version 10).
We can't avoid the string parsing behaviour altogether because we need to continue to do so for | ||
existing room versions so that we do not break existing rooms. However, there are already documented | ||
cases of this causing problems across implementations. For example, Synapse will accept `" +2 "` as | ||
a power level but Dendrite will outright fail to parse it. |
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.
It would be good to have a line here explicitly stating that: as a result, homeserver implementations may need to update their existing room version implementations to remain compatible with the spec.
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.
It is my hope that the wording in #3688 will be sufficient to cover what implementations should do today and then this MSC can focus on what we do to plug the hole going forward.
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.
(directionally, I think it's best to work on this MSC and that spec PR almost concurrently: the MSC can make reference to what the spec intended to say, as we know we'd accept a clarification about how stringy power levels work, and the spec PR can either continue working on inclusion or wait for this MSC to land and be assigned a room version)
This seems possible to implement in at least two implementations (Synapse and Dendrite), and overall this MSC seems ready to go. I'm not an expert on where the validation should be applied, but hopefully one of these folks is: @mscbot fcp merge |
Team member @turt2live has proposed to merge this. The next step is review by the rest of the tagged people: Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for information about what commands tagged team members can give me. |
The final comment period, with a disposition to merge, as per the review above, is now complete. |
This MSC is assigned to v10: #3604 |
Spec PR: matrix-org/matrix-spec#1099 |
Merged 🎉 |
Rendered.
Related: matrix-org/matrix-spec#853, matrix-org/synapse#1237, matrix-org/synapse#10232.
Implementations:
FCP Proposal
Preview: https://pr3667--matrix-org-previews.netlify.app