Update spec to say what the default state / events defaults are

[dbkr]

From inspecting the behaviour of synapse (which I'm not sure was intentional).

Are we sure this is necessary? I thought we always sent a power_levels event at room creation time with synapse, so in practice, no rooms will actually be affected?

[erikjohnston]

Are we sure this is necessary? I thought we always sent a power_levels event at room creation time with synapse, so in practice, no rooms will actually be affected?

We do need to document this in the cases where there are no power level events, but yeah, in general synapse will always set them at creation time.

[erikjohnston]

If it matches what's in synapse, LGTM

[dbkr]

@kegsay - could you second-review this?

[richvdh]

All I can say is that having two different default state_defaults is horrible. Can we really not retrospectively make the default default default 50? What is going to be affected by this in practice?

[kegsay]

I concur with the horrible-ness of two different state_default values depending on if the m.room.power_levels event is present vs present with the key missing. This smells like a bug in Synapse which should be fixed, rather than baking this behaviour into the spec. Given:

in general synapse will always set them at creation time

I would be happy to do a cursory sanity check to see which rooms will be affected on matrix.org (I suspect none?) and then actually fix this. I'm not happy to LGTM this in its current form.

[dbkr]

OK, good. Leaving this open so it doesn't get lost.

[dbkr]

Personally I'm becoming more inclined to say this is something that can be fixed in auth-events v2, but @kegsay , will leave it it up to you.

[kegsay]

I'd really prefer if we didn't spec this. We expect the number of rooms which will be affected by fixing Synapse to be low if not 0 (since it shouldn't happen due to m.room.power_levels always being set on room creation). I would prefer it if we just fixed Synapse and put something in the changelog along with the associated rationale. I don't like the precedent this sets otherwise (that bugs in Synapse will affect what is in the Spec).

[erikjohnston]

This is not a bug. The fact that it probably wasn't an intentional decision, and is one that we don't like, doesn't change the fact that this isn't a bug. This is now part of the core API, and thus changing it is technically a breaking change that would require a major version bump (if we versioned the core API).

Now, if you can show that no rooms on matrix.org are relying on the current behaviour then I'd be amenable to waving our hands in the air and changing the behaviour.

However, I have a suspicion that this may very well affect every room. In particular, the authorization of the first m.room.power_levels may depend on the fact that the effective state_default is 0 at that point.

I'd also like to reiterate that there are a surprisingly large number of home servers in the wild, some on old versions, and thus changing the core API is something that needs to be done with an extraordinary amount of care to ensure we don't screw over those servers.

@kegsay

I don't like the precedent this sets otherwise (that bugs in Synapse will affect what is in the Spec).

This is a valid concern, but:

1. Historically the authorization rules for events was defined by Synapse, and thus what Synapse does is by definition correct. I agree this is a suboptimal situation, but is nonetheless the situation we are in.
2. I believe the spec should specify what the current APIs are, warts and all. If we want to change those APIs then we should go through the appropriate process.

[ara4n]

So, I'd really like to unblock this so that matrix-org/matrix-js-sdk#94 can land (which in turn blocks about 5 other bugs).

@erikjohnston - does the m.room.power_levels auth actually depend on the implicit state_default power level being 0? If so, you have a case - if not, i very much agree with @kegsay that we should fix up the spec to be correct and fix the reference implementation to match, rather than being coerced into aligning the spec to the broken behaviour we accidentally released into the wild.

[erikjohnston]

So, I'd really like to unblock this so that matrix-org/matrix-js-sdk#94 can land (which in turn blocks about 5 other bugs).

I don't see why this is blocking that PR? If we do decide to change the behaviour, a) it won't instantly happen and get deployed everywhere, so you probably don't want to wait anyway, and b) we can change the default in the JS SDK later if we choose to change this.

If people's argument in favour of change is that this effects nobody, then what's the problem?

@erikjohnston - does the m.room.power_levels auth actually depend on the implicit state_default power level being 0? If so, you have a case - if not, i very much agree with @kegsay that we should fix up the spec to be correct and fix the reference implementation to match, rather than being coerced into aligning the spec to the broken behaviour we accidentally released into the wild.

(The following is written on the assumption it affects no one, but is equally valid as a general point regarding changes to federation)

This is my point, the spec as it is in this PR is, in my view, correct. Its the rules that servers are using. As far as I'm aware its not causing an issue anywhere, so I don't really see how its broken, or a bug, or an issue. You may not like, you may think we should change it, but it is correct. Yes, its a counter intuitive bit of the spec, yes it should be changed when we get to next changing the auth rules, but "its horrible" seems to be the sole argument for changing it.

I'd rather we just spec what we have today, so tomorrow we can see where all the "horrible" bits are and change them in the usual (and safe) fashion of bumping API versions when backwards incompatible changes are made. (And I'm generally in favour of spec'ing what we have now - even if we immediately change it - so that have a spec of how it did work, rather than a spec of how we wished it had worked)

Not changing this now does not mean we can't change this later. I am all in favour of changing this later. We just need a way of changing the auth rules in a safe fashion.

My concern is that we have played really quite fast and loose with changes to the auth rules in the past, but we absolutely need to get away from doing that now we're seeing increasing numbers of home servers. We have no way of knowing if this change will actually affect anyone, the best we can do is say that it won't effect anyone on matrix.org.
If this was actually causing a problem then I would be more inclined to say that we can take the risk of changing it, since the percentage of people effected by it would seem to be so low. But it is not causing an issue.

If we change it and it turns out to effect people, then a) we will need to justify why we changed it, and i don't believe that "it was a bit icky and no one on matrix.org was effected" is an acceptable answer and b) the failure mode of this would be bifurcated rooms which would be hell to debug for anyone not familiar with this PR (and even for those that are), and cause mass confusion to the users in the room as it will silently bifurcate with no indication in the UI that that has happened.

In summary:

• Arguments for: This is a bit horrible, we don't like it, lets change it. Since nothing relies on the difference then it doesn't matter if we change it.
• Arguments against: Why the hurry? This is already deployed to a large number of servers. This is not causing an actual issue. Changing it has non-zero risk of complicated breakages to peoples rooms, without the users in the room being aware that things are broken. We can, and should, change it later (in a safer manner).

[richvdh]

We agreed on the offsite to spec what is rather than what should have been, so this is a valid change.