MSC3667: Enforce integer power levels #3667

neilalexander · 2022-01-21T22:29:19Z

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

…c-proposals#3667)

turt2live

Generally seems fine to me - just a bit of clarity before I think this is ready for FCP.

turt2live · 2022-01-24T21:01:13Z

proposals/msc3667-enforce-integer-power-levels.md

+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.


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.

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.

Failing event auth sounds ideal - can we get words to that effect in the MSC? 😇

proposals/msc3667-enforce-integer-power-levels.md

turt2live · 2022-01-24T21:06:48Z

proposals/msc3667-enforce-integer-power-levels.md

@@ -0,0 +1,42 @@
+# MSC3667: Enforce integer power levels


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.

ShadowJonathan · 2022-01-31T20:10:11Z

proposals/msc3667-enforce-integer-power-levels.md

+All of this behaviour is exceptionally difficult for non-Python implementations to reproduce
+accurately, so we should not try to do so. 


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.

anoadragon453 · 2022-02-01T18:26:31Z

proposals/msc3667-enforce-integer-power-levels.md

+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


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.

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).

anoadragon453 · 2022-02-01T18:28:29Z

proposals/msc3667-enforce-integer-power-levels.md

+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.


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.

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.

(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)

proposals/msc3667-enforce-integer-power-levels.md

turt2live · 2022-02-10T10:23:04Z

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

mscbot · 2022-02-10T10:23:06Z

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.

mscbot · 2022-03-27T15:03:40Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

turt2live · 2022-05-04T20:38:01Z

This MSC is assigned to v10: #3604

turt2live · 2022-05-30T21:10:47Z

Spec PR: matrix-org/matrix-spec#1099

turt2live · 2022-06-08T21:23:25Z

Merged 🎉

Create mscxxxx-enforce-integer-power-levels.md

cd85016

neilalexander changed the title ~~MSCXXXX: Enforce integer power levels~~ MSC3667: Enforce integer power levels Jan 21, 2022

Update MSC number

fb65ed0

neilalexander mentioned this pull request Jan 24, 2022

MSC3667 implementation matrix-org/gomatrixserverlib#284

Merged

neilalexander added a commit to matrix-org/dendrite that referenced this pull request Jan 24, 2022

Update to matrix-org/gomatrixserverlib@f3e2ef8 (matrix-org/matrix-spe…

a4601bb

…c-proposals#3667)

Update MSC to mention implementation

feb6b92

neilalexander mentioned this pull request Jan 24, 2022

MSC3604: Room Version 10 #3604

Merged

2 tasks

turt2live self-requested a review January 24, 2022 14:47

turt2live reviewed Jan 24, 2022

View reviewed changes

turt2live removed the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Jan 24, 2022

neilalexander mentioned this pull request Jan 31, 2022

Trim whitespace from string power levels matrix-org/gomatrixserverlib#286

Merged

ShadowJonathan mentioned this pull request Jan 31, 2022

Normalising powerlevels ruma/ruma#835

Closed

ShadowJonathan reviewed Jan 31, 2022

View reviewed changes

neilalexander mentioned this pull request Feb 1, 2022

Historical string power level clarification #3688

Closed

anoadragon453 reviewed Feb 1, 2022

View reviewed changes

turt2live mentioned this pull request Feb 2, 2022

Support MSC3667 in a new room version matrix-org/synapse#11885

Closed

4 tasks

Sorunome reviewed Feb 2, 2022

View reviewed changes

proposals/msc3667-enforce-integer-power-levels.md Show resolved Hide resolved

mscbot added disposition-merge proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. and removed proposal-in-review labels Feb 10, 2022

turt2live added this to Needs idea feedback / initial review in Spec Core Team Backlog via automation Feb 10, 2022

turt2live moved this from Needs idea feedback / initial review to Proposed for FCP readiness in Spec Core Team Backlog Feb 10, 2022

mscbot added final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. and removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Mar 22, 2022

richvdh moved this from Ready for FCP ticks to In FCP in Spec Core Team Backlog Mar 22, 2022

mscbot added finished-final-comment-period and removed disposition-merge final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. labels Mar 27, 2022

turt2live merged commit 03f8ce9 into main Mar 27, 2022

turt2live added spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec and removed finished-final-comment-period labels Mar 27, 2022

turt2live moved this from In FCP to Done to some definition in Spec Core Team Backlog Mar 27, 2022

turt2live self-assigned this Mar 27, 2022

turt2live removed the unassigned-room-version Remove this label when things get versioned. label May 4, 2022

turt2live added this to Needs Spec PR in Matrix 1.3 hitlist via automation May 24, 2022

turt2live moved this from Needs Spec PR to Spec PR in progress in Matrix 1.3 hitlist May 24, 2022

This was referenced May 28, 2022

Power level values can be ints or strings matrix-org/matrix-spec#853

Closed

Specify room version 10: knock_restricted and int power levels matrix-org/matrix-spec#1099

Merged

turt2live added spec-pr-in-review A proposal which has been PR'd against the spec and is in review and removed spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec labels May 30, 2022

turt2live moved this from Spec PR in progress to Spec PR needs review in Matrix 1.3 hitlist May 30, 2022

turt2live added merged A proposal whose PR has merged into the spec! and removed spec-pr-in-review A proposal which has been PR'd against the spec and is in review labels Jun 8, 2022

turt2live moved this from Spec PR needs review to Merged to spec (done) in Matrix 1.3 hitlist Jun 8, 2022

This was referenced Jun 10, 2022

Implement MSC3604: Room Version 10 ruma/ruma#1174

Closed

Incorporate Matrix v1.3 changes ruma/ruma#1145

Closed

zecakeh mentioned this pull request Jun 21, 2022

state-res: Add RoomVersion::V10 ruma/ruma#1213

Merged

S7evinK mentioned this pull request Sep 29, 2023

Declare support for Matrix 1.3 matrix-org/dendrite#3219

Open

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MSC3667: Enforce integer power levels #3667

MSC3667: Enforce integer power levels #3667

neilalexander commented Jan 21, 2022 •

edited by github-actions bot

turt2live left a comment

turt2live Jan 24, 2022

neilalexander Jan 25, 2022

turt2live Feb 2, 2022

turt2live Jan 24, 2022

ShadowJonathan Jan 31, 2022

anoadragon453 Feb 1, 2022

neilalexander Feb 1, 2022

anoadragon453 Feb 1, 2022

neilalexander Feb 1, 2022

turt2live Feb 2, 2022

turt2live commented Feb 10, 2022

mscbot commented Feb 10, 2022 •

edited by ara4n

mscbot commented Mar 27, 2022

turt2live commented May 4, 2022

turt2live commented May 30, 2022

turt2live commented Jun 8, 2022

		All of this behaviour is exceptionally difficult for non-Python implementations to reproduce
		accurately, so we should not try to do so.

		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

MSC3667: Enforce integer power levels #3667

MSC3667: Enforce integer power levels #3667

Conversation

neilalexander commented Jan 21, 2022 • edited by github-actions bot

turt2live left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turt2live commented Feb 10, 2022

mscbot commented Feb 10, 2022 • edited by ara4n

mscbot commented Mar 27, 2022

turt2live commented May 4, 2022

turt2live commented May 30, 2022

turt2live commented Jun 8, 2022

neilalexander commented Jan 21, 2022 •

edited by github-actions bot

mscbot commented Feb 10, 2022 •

edited by ara4n