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

Clarification on historical power level handling #1082

Closed
wants to merge 5 commits into from
Closed

Clarification on historical power level handling #1082

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f443b3d
Clarification on historical power level handling
neilalexander May 24, 2022
6b175d6
Revert "Clarification on historical power level handling"
neilalexander May 24, 2022
7fd54c4
Clean up
neilalexander May 24, 2022
666baf6
Let us try this again not using VS Code
neilalexander May 24, 2022
a5a4c1f
Markdown is full of mysteries
neilalexander May 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelogs/server_server/newsfragments/1082.clarification
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Clarifies the historical handling of non-integer power levels.
18 changes: 18 additions & 0 deletions content/server-server-api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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** \
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mostly figured it made sense here because it affects all current room versions and nothing else documents that behaviour anywhere.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

process-wise we increment by one, but the spec is much more strict on how it words things, for better or worse.

In order to maintain backwards compatibility with early implementations,
power levels can optionally be represented in string format instead of
integer format. A homeserver must be prepared to deal with this by parsing
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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@jplatte that was fixed in matrix-org/matrix-spec-proposals#2540

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

😭 well that's frustrating

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

- optionally with leading or trailing whitespace characters;
- optionally prefixed with a single `-` or `+` character before the integer but after leading whitespace padding.

{{% boxes/warning %}}
This behaviour is preserved strictly for backward compatibility only. A
homeserver should take reasonable precautions to prevent users from
sending new power level events with string values and must never
populate the default power levels in a room as string values.
{{% /boxes/warning %}}

#### Authorization rules

The rules governing whether an event is authorized depends on a set of
Expand Down