/createRoom results in a 403 + created room when mixing up the power levels

[turt2live]

Creating a room with a name and power_level_content_override which doesn't list the creator as having power results in the correct "403 you don't have enough power" response, however the room is still created and synced down to the user. It should not be created.

[t3chguy]

Why

It should not be created

as opposed to defaulting to 100 if not provided

[t3chguy]

The spec in unclear in the granularity of the override, it does not say whether to do it on a depth=1 only or not:

Overridden by the power_level_content_override parameter.

[turt2live]

I'd argue that https://matrix.org/docs/spec/client_server/r0.5.0#post-matrix-client-r0-createroom is pretty clear about the order of application.

[t3chguy]

Yes, but it doesn't say how to interpret power_level_content_override

Whether

power_level_content_override:

"users": {
    "@not_creator:server": 100
}

when overriding

...
"users": {
    "@creator:server": 100
}
...

ends up including both of the users, or it merged on a depth of 1 only,

synapse implementation overrides each key, so does not merge users, and therefore the creator disappears

[turt2live]

The intention is that the spec says it overrides the entire structure, not just at the root level. If that's unclear, please open a spec clarification.

[t3chguy]

Oh right, in synapse it uses a dict update,which is why it confused me. I'll fix it in synapse tomorrow

[t3chguy]

Corrected Synapse to match the above:

The intention is that the spec says it overrides the entire structure, not just at the root level. If that's unclear, please open a spec clarification.

[richvdh]

The intention is that the spec says it overrides the entire structure, not just at the root level. If that's unclear, please open a spec clarification.

The spec currently says of power_level_content_override:

This object is applied on top of the generated m.room.power_levels event content prior to it being sent to the room

"on top of" definitely suggests a merge, not an overwrite, to me, though I agree it's unclear how deep the merge should go. It feels like synapse kinda matches the spec at the moment, though.

@turt2live: are you sure about this?

[turt2live]

I was sure, but that quote leads me to believe that my life has been a lie. We should probably have a quick poll as the spec team and standardize on the winner.

[richvdh]

my main feeling on that is that the spec says what it says, and synapse seems to basically match it. There may be valid reasons to change it, but they probably need an MSC and I'm unconvinced they are worthwhile.

To return to the topic of this bug: there are probably hundreds of ways of specifying broken parameters to a createRoom call, all of which lead to a broken room being created... :/

[t3chguy]

We should probably have a quick poll as the spec team and standardize on the winner.

Any update on this?

[turt2live]

@t3chguy no. #5579 (comment) is the last comment since then.

It was also a suggestion, not an action I plan on taking right now (hence "probably"). The core team is a bit involved with too many issues to worry about the subtleties of /createRoom at the moment. I would recommend at the very least opening a clarification PR to the spec as I said here: #5579 (comment)

[t3chguy]

Fixed by #5633