Skip to content

Historical string power level clarification #3688

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

Closed
wants to merge 4 commits into from
Closed

Historical string power level clarification #3688

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
56dd6a4
Comments about string power levels
neilalexander Feb 1, 2022
d1ebcfb
Review comments
neilalexander Feb 1, 2022
5a2e214
Update content/server-server-api.md
neilalexander Feb 1, 2022
6ec0ae6
Merge branch 'main' into neilalexander/powerlevels
neilalexander Feb 1, 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
27 changes: 24 additions & 3 deletions content/server-server-api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,22 +375,43 @@ them.

#### Definitions

**Required Power Level** \
##### Required Power Level
Copy link
Member

Choose a reason for hiding this comment

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

These shouldn't be promoted to headings - the bolding is fine


A given event type has an associated *required power level*. This is
given by the current `m.room.power_levels` event. The event type is
either listed explicitly in the `events` section or given by either
`state_default` or `events_default` depending on if the event is a state
event or not.

**Invite Level, Kick Level, Ban Level, Redact Level** \
##### Invite Level, Kick Level, Ban Level, Redact Level

The levels given by the `invite`, `kick`, `ban`, and `redact` properties
in the current `m.room.power_levels` state. Each defaults to 50 if
unspecified.

**Target User** \
##### Target User

For an `m.room.member` state event, the user given by the `state_key` of
the event.

##### Historical String Power Levels

Power level events in room versions up to and including room version 9
can be optionally represented in string format, in order to maintain
Comment on lines +399 to +400
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if it is worth mentioning but.... room versions 1 to 5 inclusive can unfortunately have float power-levels that get cast to int as well in Synapse. See MSC2540.

Again from the Python docs:

For floating point numbers, this truncates towards zero.

😢

compatibility with pre-1.0 implementations without breaking existing rooms.
A homeserver must be prepared to deal with this by parsing the power level
from the string. In these cases, the following formatting of the power level
string is allowed:
Comment on lines +399 to +404
Copy link
Member

Choose a reason for hiding this comment

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

We'll define a range when we pull this out of the s2s spec in favour of the room version specs (this section will get airlifted out of this doc).

For now, the following (or a variation therein) should be fine:

Suggested change
Power level events in room versions up to and including room version 9
can be optionally represented in string format, in order to maintain
compatibility with pre-1.0 implementations without breaking existing rooms.
A homeserver must be prepared to deal with this by parsing the power level
from the string. In these cases, the following formatting of the power level
string is allowed:
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:

Copy link
Member

Choose a reason for hiding this comment

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

something missing from this paragraph is a pointer to what value we're talking about though. Examples after the grammar would probably be best, demonstrating a scenario or two (kick power level being "12" and a user having PL "42" or something similar).


- a single Base10 integer, no float values or decimal points, optionally with leading zeroes;
Copy link
Contributor

@ShadowJonathan ShadowJonathan Feb 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

One thing i need to ask, and mention, is what the range of this integer will be.

Synapse does not correctly validate a string-passed integer, allowing it to become bigger than 64-bit, or the canonical JSON 2^53 limit.

I personally think this latter bit should not be included, and integers should be validated according to the canonical JSON limit, but it is still worth mentioning. (synapse issue: matrix-org/synapse#11873)

Copy link
Member

Choose a reason for hiding this comment

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

Limits are difficult because this dates all the way back to python2 which had a firm limit, but the bug has persisted through a python3 transition where no practical limits are applied.

This is way out of my depth as a reviewer, so will defer to the server-side folk of the SCT for comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

requiring bigints would significantly complicate server implementations, as in many languages using them isn't as simple as in python.

Copy link
Contributor

@deepbluev7 deepbluev7 Feb 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I would not relax the limit on the integers. Our database adventures found not a single event outside the range. So it should be safe to not also relax the limit on the integer range. If it actually breaks rooms, we could add such language to the spec then?

Copy link
Member

Choose a reason for hiding this comment

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

I would not relax the limit on the integers.

+1. This whole thing is a balance between not breaking too many existing rooms, whilst not making supporting those existing rooms too onerous for non-Synapse implementations.

Given that we don't know of any rooms which have stringy PLs outside [-2^53, 2^53), requiring non-synapse implementations to support such PLs seems an unnecessary burden.

Copy link
Member

Choose a reason for hiding this comment

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

@deepbluev7 says:

I suggest we break rooms with PLs outside the range in v6+ and add an appropriate error message to synapse that says to report a bug, if an existing room is broken because of that. And then when someone comes and actually complains about it, we can discuss relaxing the limit as well. We don't need to allow all synapse currently allows, just what is already in the wild. And those are from my search only PLs inside the range, but as strings, in v6+. As long as it is clear how to report, if someone finds a non-conforming room, I think it is fine to break those temporarily.

Copy link
Member

@richvdh richvdh Feb 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

(in other words, let's change synapse to reject such PL events if it sees them in the future)

Edit: in room v6+, obviously. room v5 and earlier are kinda stuck with bigint PLs.

Copy link
Member

@clokep clokep Feb 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Limits are difficult because this dates all the way back to python2 which had a firm limit, but the bug has persisted through a python3 transition where no practical limits are applied.

I don't think this is accurate. I just did some testing on Python 2:

>>> int("12345689012345678901234567890")
12345689012345678901234567890L
>>> type(_)
<type 'long'>

On python 3 this creates:

>>> int("12345689012345678901234567890")
12345689012345678901234567890
>>> type(_)
<class 'int'>

The difference is that Python 2 had a separate long class while Python 3 has an int class which expands to any size (essentially long from Python 2). json.dumps seems to work fine (and creates invalid JSON going over the 2 ^ 53 limit... for a number)

tl;dr I think both Python 2 & 3 didn't have a limit.

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

This behaviour is preserved strictly for backward compatibility only. A
homeserver should take reasonable precautions to prevent users from
uploading new power level events with string values and must never
populate the default power levels in a room as string values.
Comment on lines +410 to +413
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This behaviour is preserved strictly for backward compatibility only. A
homeserver should take reasonable precautions to prevent users from
uploading new power level events with string values and must never
populate the default power levels in a room as string values.
{{% 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