(Attempt to) Fix event authorization rules to allow first Power Levels events to be injected in the room

[maxidorius]

I believe this is the intended meaning of the sentence.

I just tripped over this as my initial implementation checked against default calculated PLs when injecting the very first m.room.power_levels, in which case it failed.

If you ignore keys which were not set previously, then the rules allow you to create a room and its initial state.

[matrixbot]

Can one of the admins verify this patch?

[richvdh]

Oh gods. It's certainly true that the rules as currently specified are insufficient to create the first power_levels event. However, I don't think that this is the correct interpretation: all of these fields have default values, which are used in this algorithm if no explicit value is given in the power_levels event.

However, the first power_levels event turns out to be special-cased in two separate ways:

• Firstly, until there is a power_levels event in the room, the room creator has a power level of 100 (and everybody else 0), as per this code. (Which raises the question of "why is state_default special-cased when there is no power_levels event" (as per here. This seems redundant.)
• Secondly the first power_levels event can do whatever it wants anyway. It doesn't even have to come from the room creator, afaict.

[richvdh]

I'm currently undecided as to whether we should spec both of those special-cases, or change synapse in some way. I'll probably have to poll @erikjohnston on the subject.

[maxidorius]

Yes, I assumed either synapse gave PL 100 to the creator, or the intended meaning of the rules had to be "for those existing before". I didn't check synapse code on purpose :)
So I did find a real issue :D

[maxidorius]

@richvdh if you were to change the rules/wording as per my PR, would it actually behave any different than the current behaviour? I'm not a Math/algo person, so I might have missed a case here...

[richvdh]

For example, suppose I have PL 50 (allowing me to send state_events), and you have no PL assigned. Under your wording, I can give you PL 1000 and we can take over the room.

[maxidorius]

you're right, that doesn't work then. thanks for the example!

[richvdh]

again, no. Even if the current value is not specified explicitly (so is inferred from the defaults), it may be higher than the sender's level, in which case it should be rejected.

[maxidorius]

That extra commit wasn't for review, I wanted to share with someone else and since the branch is in the PR, everything shows :(
There is no fixing the logic without handling it as a special case, for sure.

[maxidorius]

@richvdh What should be done with this PR? and the actual logic behind about rules?

[richvdh]

I'm currently undecided as to whether we should spec both of those special-cases, or change synapse in some way. I'll probably have to poll @erikjohnston on the subject.

Conclusion was that changes in this area really need to wait for auth rules v2, so we should just make sure that all the existing special-cases are specced.

[maxidorius]

This will be split in two PRs as described here. Leaving it open not to forget to do it.

[maxidorius]

Split done in #1080 & #1081