Clarification on historical power level handling #1082
Conversation
| @@ -394,6 +394,24 @@ unspecified. | |||
| For an `m.room.member` state event, the user given by the `state_key` of | |||
| the event. | |||
|
|
|||
| **Historical String Power Levels** \ | |||
There was a problem hiding this comment.
shouldn't this be in the room version spec? (it'll also get adopted as part of v10 being specced if you'd prefer to make it someone else's problem to land, btw)
There was a problem hiding this comment.
I mostly figured it made sense here because it affects all current room versions and nothing else documents that behaviour anywhere.
There was a problem hiding this comment.
ah, yea, if we didn't have a room version on the way then it'd land here for sure. v10 is currently in FCP and will land in Matrix 1.3 though (a few weeks away), so time delay seems to have bit us here :(
if you're okay with it, I'm happy to take over the inclusion of this via v10 spec.
There was a problem hiding this comment.
My feeling is that a "historical" section probably still makes sense here regardless of v10 because it's behaviour that a server implementor really needs to know exists from the past, although maybe we want to modify the text to clarify that the behaviour only applies to room versions 9 and below?
I'm more than happy for you to take over rolling something into v10 though either way.
There was a problem hiding this comment.
The spec language is incredibly frustrating because we can't say "v9 or below", because they aren't linear. A note here is certainly worthwhile, though it might get moved into the room version spec and a note left here to say "note that power level parsing is dependent on the room version" or something.
There was a problem hiding this comment.
because they aren't linear
In terms of specced versions and completely ignoring unspecced/unstable ones, they are linear-ish enough aren't they? (In that we should never spec a new room version after v10 that doesn't require integer power levels because that would be a regression.)
There was a problem hiding this comment.
process-wise we increment by one, but the spec is much more strict on how it words things, for better or worse.
| the power level from a string. In these cases, the following formatting of the | ||
| power level string is allowed: | ||
|
|
||
| - a single Base10 integer, no float values or decimal points, optionally with leading zeroes; |
There was a problem hiding this comment.
For maximum clarity, it should be noted here that this integer has to be within the usual [MIN_SAFE_INTEGER, MAX_SAFE_INTEGER] range (i.e. Synapse [still? / previously?] accepting smaller / larger ones is "just" a bug).
There was a problem hiding this comment.
@jplatte that was fixed in matrix-org/matrix-spec-proposals#2540
There was a problem hiding this comment.
I'm talking specifically about stringified integers outside of that range, which were still accepted by Synapse in all room versions (i.e. including v6+) recently according to a comment on one of the enforce-integer-powerlevels related issues / PRs.
There was a problem hiding this comment.
I think it's fine actually, it wasn't found to be used / exploited in the wild and can really only happen if somebody intentionally creates such a PL event. It would actually be near impossible to support current Synapse's behavior in other HSes anyways, since Synapse converts to Python's int, which even allows values >64bit.
|
closing in favour of #1099 which defines it as part of v10 (backporting to v1-9 and leaving a note in the s2s spec). |
This PR replaces and therefore closes matrix-org/matrix-spec-proposals#3688 and includes the changes suggested from the review comments.
Preview: https://pr1082--matrix-spec-previews.netlify.app