Skip to content
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.

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

MSC3760: State sub-keys #3760

Closed
wants to merge 1 commit into from
Closed

MSC3760: State sub-keys #3760

wants to merge 1 commit into from

Conversation

andybalaam
Copy link
Contributor

@andybalaam andybalaam commented Mar 31, 2022

@andybalaam andybalaam changed the title Add 'Restricting who can overwrite a state event' MSC3760 Restricting who can overwrite a state event Mar 31, 2022
@andybalaam andybalaam changed the title MSC3760 Restricting who can overwrite a state event MSC3760: Restricting who can overwrite a state event Mar 31, 2022
@andybalaam andybalaam changed the title MSC3760: Restricting who can overwrite a state event MSC3760: State sub-keys Mar 31, 2022
@anoadragon453 anoadragon453 added proposal-in-review proposal A matrix spec change proposal labels Mar 31, 2022
Copy link
Contributor

@ShadowJonathan ShadowJonathan left a comment

Choose a reason for hiding this comment

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

No obvious soundness issues on first glance. I personally like this one more than MSC3757 due to this being semantically more “clean” (though opinions obviously differ).

One point of discussion though.

@@ -0,0 +1,74 @@
# MSC3760: State sub-keys
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing I want to see is how much change this requires on the API level to query, filter, and fetch specific state with a specific subkey.

Currently matrix is indexing state events with a “tuple” of (type, key), this might change it to (type, key, subkey) semantically, but with most apis (and implementations) being geared towards the older model.

This MSC may need to detail the necessary API changes required to properly address state events with a subkey.


## Unstable prefix

* `state_subkey` should be referred to as `org.matrix.mscxxxx.state_subkey` until this MSC lands.
Copy link
Contributor

Choose a reason for hiding this comment

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

At risk of immutably requiring servers to be backwards compatible with this for any stable room version, I suggest adding a qualifier that this event-level key should only appear on rooms versions which experiment with this.

@uhoreg uhoreg added requires-room-version An idea which will require a bump in room version unassigned-room-version Remove this label when things get versioned. kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Mar 31, 2022

State events that are missing `state_subkey` should behave exactly as if `state_subkey` had a value of `""`.

Two state events with identical `type` and `state_key` should be treated as independent if they have different values of `state_subkey`.
Copy link
Member

Choose a reason for hiding this comment

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

Since this complicates things, perhaps it would make sense to also spell it out in terms of operational semantics. That is, to say that before this change, state events behaved like an assignment of the form

room_state[type, key] = content

And this MSC changes it to

room_state[type, key, subkey] = content

And that a missing subkey is equivalent to

room_state[type, key, ""] = content


Two state events with identical `type` and `state_key` should be treated as independent if they have different values of `state_subkey`.

## Potential issues
Copy link
Member

Choose a reason for hiding this comment

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

This change isn't backwards compatible for clients which aren't updated as frequently. Not sure how much of a problem it is necessarily, though as a general case we aim to keep those clients functional to the best of our ability (notable exceptions being introducing new join_rules, but not taking away the join_rule field).


## Security considerations

The rules on who can modify a state event are unchanged by this proposal, which should simplify security concerns relative to the alternatives.

Choose a reason for hiding this comment

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

Here's one small change that might increase the impact of this proposal, at the cost of a little bit of extra implementation complexity. Why not add the restriction that "If a state event has a state subkey which begins with an @, then the sender's mxid must match that state subkey."?

This would make it possible to use state subkeys for access control with state events whose state key already has a meaning - for example m.space.child. And that would solve some in-the-wild access control problems with state updates.

Copy link

@gleachkr gleachkr Apr 1, 2022

Choose a reason for hiding this comment

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

OK, after some discussion, I'm not sure the above is right. It's not clear that it would actually be possible to add access controls to existing state event types this way without further MSCs. For example, it's not transparent what it would mean to have multiple copies of m.space.child for a single room, differentiated by their subkeys, and dealing with this scenario could mean extra work for server implementations to rejigger the hierarchy endpoint.

More generally, this proposal seems to entail that you could legally have multiple copies of something like m.room.canonical_alias, which all have an empty string as key, but which have different subkeys. Probably it's obvious that in this case the state events with subkeys should be ignored, but maybe it'd be good to make this explicit, and also to think through what changes would need to be made across the client ecosystem to implement this behavior? (or maybe that's not a worry because of the room-version bump?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not add the restriction that "If a state event has a state subkey which begins with an @, then the sender's mxid must match that state subkey."?

We'll also have to decide how to handle the case were state key and subkey contain two different MXIDs then.

Copy link
Member

Choose a reason for hiding this comment

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

For example, it's not transparent what it would mean to have multiple copies of m.space.child for a single room, differentiated by their subkeys, and dealing with this scenario could mean extra work for server implementations to rejigger the hierarchy endpoint.

Emphasized part is what really complicates this idea from my POV. It's kind of obvious to me that any references to a particular state event type in older MSCs need to be taken as talking about the case where state_subkey == "", but you could still add new meaning to an m.space.child with a non-nil state_subkey client-side.

But in this case this doesn't help you very much, as the usefulness of this lies primarily in having a functioning /hierarchy endpoint. And for that you need server support. Bummer.

Copy link

Choose a reason for hiding this comment

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

And in having other clients acknowledge your space events, which you'd lose as well.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but that could eventually be spec'ed as normative client behaviour and would be relatively easy and straightforward to do. But making /hierarchy work needs significant design.

I also just had a random interesting thought: We currently put the intended interpretation of events into the C2S API spec, but that's not really correct. In the future we could imagine having clients speaking the S2S protocol directly without even implementing C2S (not even internally, like with current client + embedded Dendrite plans). It seems there should really be a separate spec specifying the intended meaning of events.

@turt2live turt2live added kind:core MSC which is critical to the protocol's success and removed kind:feature MSC for not-core and not-maintenance stuff labels Mar 31, 2022
@turt2live
Copy link
Member

turt2live commented Mar 31, 2022

this is kind:core because the related proposal, MSC3757, is also listed as core. That one is core because it interacts with a system which affects the ability for multiple MSCs/ideas to make forward movement.

Edited: word choice. "Markets" isn't entirely accurate.

Copy link
Member

@dkasak dkasak left a comment

Choose a reason for hiding this comment

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

I generally like this and it seems like it adds a lot of useful flexibility without overextending.

One other spot where I see this as being useful is for implementing per-room extensible profiles which I described here and were fleshed out some more here by @turt2live.

Having state subkeys would allow us to have a single m.profile state event type without making it a huge dictionary which would need to be replaced wholesale each time a single profile field is updated.

Using the operational notation I suggested, it would effectively allow us to do stuff like:

room_state["m.profile", @alice:example.com, "display_name"] = display_name_content
room_state["m.profile", @alice:example.com, "avatar_url"] = avatar_url_content
room_state["m.profile", @alice:example.com, "bio"] = bio_content

State events that are missing `state_subkey` should behave exactly as if `state_subkey` had a value of `""`.

Two state events with identical `type` and `state_key` should be treated as independent if they have different values of `state_subkey`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Potential implementation strategy: it's possible a server implementation should implement this by storing both state_key and state_subkey in a (large enough) single database column, concatenated together, separated by underscore. This might reduce the need to update existing code, and underscore is safe to use since it is not allowed in a domain part (see MSC3757).

@@ -0,0 +1,74 @@
# MSC3760: State sub-keys

Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of this MSC overall because it breaks client data structures which have always assumed replace semantics are (event type, state key) - this PR introduces a third value to the tuple. This breaks almost everything: server databases won't have an index for the subkey, and existing code which expects to pull out a single event for a (type, state key) will break.

This will require major changes on both clients and servers, which is a lot to ask for just for live location sharing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe as a middle-ground solution between this and MSC3757, state_key could be allowed to be an array of strings? That should slightly lower the amount of breakage while still maintaining the advantages of this proposal.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe changing that to an array would be even a bigger change, and makes a type variable in a schema (either string or array), which is... fairly difficult to correctly deserialize, especially in Rust/Ruma/Serde.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's definitely simple enough to deserialize in Rust / serde.

@benkuly
Copy link
Contributor

benkuly commented Apr 10, 2022

I don't think this is a good approach at all. I'm worried, that with new use-cases we will have MSC "State sub-sub-key", MSC "State sub-sub-sub-key", ... in a few months or years. We need a sustainable solution for state keys, even if this means, that there will be breaking changes (with a new room version).

@andybalaam
Copy link
Contributor Author

The spec core team discussed this as an alternative to MSC3757, and decided they preferred MSC3757. The reasoning is written up here: https://github.com/matrix-org/matrix-spec-proposals/pull/3757/files#r848454293

So, closing this in favour of MSC3757.

@andybalaam andybalaam closed this Apr 13, 2022
@turt2live turt2live added abandoned A proposal where the author/shepherd is not responsive obsolete A proposal which has been overtaken by other proposals and removed proposal-in-review abandoned A proposal where the author/shepherd is not responsive labels Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:core MSC which is critical to the protocol's success needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. obsolete A proposal which has been overtaken by other proposals proposal A matrix spec change proposal requires-room-version An idea which will require a bump in room version unassigned-room-version Remove this label when things get versioned.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet