Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Synapse does not respect event order of spec /createRoom #11731

Open
timokoesters opened this issue Jan 12, 2022 · 8 comments
Open

Synapse does not respect event order of spec /createRoom #11731

timokoesters opened this issue Jan 12, 2022 · 8 comments
Labels
A-Create-Room A-Spec-Compliance places where synapse does not conform to the spec T-Other Questions, user support, anything else.

Comments

@timokoesters
Copy link

# We treat the power levels override specially as this needs to be one
# of the first events that get sent into a room.
pl_content = initial_state.pop((EventTypes.PowerLevels, ""), None)

If no power_level_content_override event is given in step 3, one needs to create a default power levels event. The power levels event from initial_state can only be used in step 6 and needs to be auth checked against the power levels event from step 3.

https://spec.matrix.org/v1.1/client-server-api/#post_matrixclientv3createroom
image

@timokoesters timokoesters changed the title Synapse does not respect event order of spec Synapse does not respect event order of spec /createRoom Jan 12, 2022
@timokoesters
Copy link
Author

Also see matrix-org/matrix-spec-proposals#3214

@timokoesters
Copy link
Author

https://github.com/mautrix/signal currently depends on this implementation detail. This in turn means that the bridge doesn't work with conduit.

@ShadowJonathan
Copy link
Contributor

ShadowJonathan commented Jan 12, 2022

The problem here is that synapse short-circuits initial_content power_levels into as an override, while that is not documented anywhere.

While this is "fine" as an optimisation, this becomes a problem when software starts to rely on it, and other servers suddenly have to deal with this problem.

I think that, as a reference implementationthe largest and most prominent matrix server in the ecosystem, this should not exist in synapse, as it sets a bad standard and example.


(I'll create a complement test for this)

@ShadowJonathan
Copy link
Contributor

ShadowJonathan commented Jan 12, 2022

@turt2live mentioned in the #dev room that this is likely a copy oversight from when the spec was still being derived from synapse, this is the commit that added the change, 7 years ago.

@callahad
Copy link
Contributor

Our gut feeling is that Synapse's behavior should be preferred in this case, and a spec clarification / spec change should be raised to propose aligning the Spec with Synapse.

@callahad
Copy link
Contributor

(Or that could be handled in MSC3214 per Timo's comment?)

@H-Shay H-Shay added A-Spec-Compliance places where synapse does not conform to the spec T-Other Questions, user support, anything else. labels Jan 13, 2022
@ShadowJonathan
Copy link
Contributor

ShadowJonathan commented Jan 13, 2022

I believe 3214 would indeed solve it, I think focusing on that would be a good way forward.

@MTRNord
Copy link
Contributor

MTRNord commented Apr 3, 2022

Just want to note that this real scenario fails:

{
  "name": "mtrnord",
  "preset": "public_chat",
  "power_level_content_override": {
    "invite": 100,
    "kick": 100,
    "ban": 100,
    "redact": 50,
    "state_default": 50,
    "events_default": 50,
    "users_default": 0,
    "events": {
      "m.room.power_levels": 100,
      "m.room.history_visibility": 100,
      "m.room.tombstone": 100,
      "m.room.encryption": 100,
      "m.room.name": 50,
      "m.room.message": 50,
      "m.room.encrypted": 50,
      "m.sticker": 50
    },
    "users": {
      "@administrator:art.midnightthoughts.space": 100,
      "@mtrnord:art.midnightthoughts.space": 100
    }
  },
  "invite": [
    "@administrator:art.midnightthoughts.space"
  ],
  "creation_content": {
    "type": "m.space"
  },
  "initial_state": [
    {
      "type": "org.matrix.msc3088.purpose",
      "state_key": "org.matrix.msc3089.data_tree",
      "content": {
        "org.matrix.msc3088.enabled": true
      }
    },
    {
      "type": "m.room.encryption",
      "state_key": "",
      "content": {
        "algorithm": "m.megolm.v1.aes-sha2"
      }
    },
    {
      "type": "m.room.guest_access",
      "state_key": "",
      "content": {
        "guest_access": "can_join"
      }
    },
    {
      "type": "m.room.history_visibility",
      "state_key": "",
      "content": {
        "history_visibility": "world_readable"
      }
    },
    {
      "type": "m.room.power_levels",
      "state_key": "",
      "content": {
        "invite": 100,
        "kick": 100,
        "ban": 100,
        "redact": 50,
        "state_default": 50,
        "events_default": 50,
        "users_default": 0,
        "events": {
          "m.room.power_levels": 100,
          "m.room.history_visibility": 100,
          "m.room.tombstone": 100,
          "m.room.encryption": 100,
          "m.room.name": 50,
          "m.room.message": 50,
          "m.room.encrypted": 50,
          "m.sticker": 50
        },
        "users": {
          "@administrator:art.midnightthoughts.space": 100,
          "@mtrnord:art.midnightthoughts.space": 50
        }
      }
    }
  ]
}

Yes, it is fully intended behavior that here, the room creator is going to be PL 50 and not PL 100.

Note that room creator here is @mtrnord:art.midnightthoughts.space and NOT @administrator:art.midnightthoughts.space

The reason why I do this is mainly legal nature. As in Matrix Art needs to be easily moderateable. As long as the same PL can't ban each other, this needs to happen.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Create-Room A-Spec-Compliance places where synapse does not conform to the spec T-Other Questions, user support, anything else.
Projects
None yet
Development

No branches or pull requests

6 participants