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

MSC3757: Restricting who can overwrite a state event #3757

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

andybalaam
Copy link
Contributor

@andybalaam andybalaam commented Mar 25, 2022

@andybalaam andybalaam changed the title Restricting who can overwrite a state event MSC3757: Restricting who can overwrite a state event Mar 25, 2022
@turt2live turt2live added requires-room-version An idea which will require a bump in room version proposal-in-review proposal A matrix spec change proposal s2s Server-to-Server API (federation) client-server Client-Server API unassigned-room-version Remove this label when things get versioned. 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. labels Mar 25, 2022
@gleachkr

This comment was marked as duplicate.

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.

One nit, else this looks sound.

andybalaam and others added 5 commits March 31, 2022 11:44
Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
Co-authored-by: Travis Ralston <travisr@matrix.org>
@jplatte jplatte mentioned this pull request Apr 4, 2022
@ara4n
Copy link
Member

ara4n commented Feb 12, 2023

Does anyone actually object to this MSC? Its absence is currently screwing over my attempt to track multiple beacons via MSC3489 from a given user :(


None yet.

## Alternatives
Copy link
Member

Choose a reason for hiding this comment

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

String-based matching still feels like a bandaid solution for a larger problem. The problem statement is reasonable (beacons, calls, and integrations all want an ability to narrow power levels), however there are non-state events where it'd be worth having a similar access mechanism (namely for edits).

At the point where we're considering string-packing, a top level field on the event works just as well and is more extensible, capable, and presentable than a packed string. It can be populated with query parameters to the PUT /state or PUT /send calls, and servers are easily able to retrieve information from the event. Further, since we're modifying the auth rules to accomplish this, there is no material difference to the implementation effort of the proposal itself.

Copy link

Choose a reason for hiding this comment

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

I am wondering if a top level field implies that state_key and access mechanism are independent.

If this is what is proposed I have the concern that this would allow blocking of state keys by not allowing to write to them anymore. (I think I saw this being brought up somewhere else as well. cannot find it right now)

But the concept of merging key + access level makes a lot of sense to me since it would behave more like a namespace than an add-on permission system. And I think the idea of namespaces (kind of like inverse domains where each user owns the domain starting with their username) is a very good fit for a key value based state like the room state.

Copy link
Contributor Author

@andybalaam andybalaam Jun 14, 2024

Choose a reason for hiding this comment

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

(edited out to avoid confusion)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, the above was wrong - I was mixing this up with #3779 which I think replaces this, and which I prefer.

Copy link
Member

Choose a reason for hiding this comment

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

This alternative appears to be missing from the MSC.

@toger5
Copy link

toger5 commented May 7, 2024

I would like to add another solution.
State keys could be of type: string | string[]
Where state_key: string = "hello" is a shortcut for state_key: string[] = ["hello"].
Our access rule would become:

If the events the state_key contains a user id that does not match the sender, reject.

  • unless the sender's powerlevel is greater than the event type's required power level.

Treating state keys as a path also mirrors a nice mental model to split bigger state events into objects build up by multiple state events so we would not need a state change request. Also I am worried that a state subkey (like in MSC3760 could quickly get its own rules like: "only allow to chnage by the device with that deviceId" and we might need a state-sub-sub-key. Making the state key a path solves this. It would be trivial to add rules like [$userId, $deviceId, something] can only be edited by the defined device...

Here is a visualization of how the state path can be thought of as a path to a subsection of an object.

This array of stat events

[
{
 "state_key": "a",
 "content": {
  "my_property": "a"
 }
},

{
 "state_key": ["b", "a"],
 "content":{"test":1}
},

{
 "state_key": ["b","@myId:domain.com"],
 "content": {
   "data": "data",
   "note": "this object owned by the user: @myId:domain.com"
  }
},

{
 "state_key": ["b", "@myId:domain.com", "subElement"],
 "content": {
   "note": "also owned by: @myId:domain.com",
   "editProps": "but can be edited independently as a sub state object"
  }
}
]

Can be thought of/parsed as this room state object.

{
 "a": {
  "content":{"my_property": "a"}
 },
 "b": {
  "@myId:domain.com":{
   "content": {"other": "data"},
   "subElement"{
    "content":{
        "note": "also owned by: @myId:domain.com",
        "editProps": "but can be edited independently as a sub state object"
    }
   }
  },
  "a":{
   "content":{"test":1}
  }
 }
}

A concrete example for b could be:
["ephemeralLocations",$userId], ["presistentLocations",$userId]
Or b could be the user Id and we inside each user we have a list of devices for the call member event example

One can think of the state event type as the first element in the array/path.
state_property_path = [$state_event_type] + state_key
It still makes sense however, that the type is its own field since we always need a type. Which is better then requireing the key array to be of min length one.

Additinally the state_key = "" would be equivalent to state_key=[] which makes much more sense thinking of it as a property path: path = [$state_event_type] + state_key = [$state_event_type]

@richvdh
Copy link
Member

richvdh commented May 21, 2024

I'm not necessarily against this proposal per-se, but I remain very worried that we have no practical means of deleting state from a room, and it seems like most applications that would make use of this proposal would tend towards large amounts of state being added to a room which then has to hang around forever.

(#3901 is a proposal to deal with deleting state, but it needs implementation work)

@AndrewFerr
Copy link
Member

I remain very worried that we have no practical means of deleting state from a room, and it seems like most applications that would make use of this proposal would tend towards large amounts of state being added to a room which then has to hang around forever.

What could mitigate this somewhat is #4140 working together with #3901 to allow for state events that expire into obsoleted events. That would allow for applications of this MSC to set state that would eventually be marked for cleanup/deletion.


becomes:

> If the event has a `state_key` that starts with an `@`, and the substring before the first `_` that follows the first `:` (or end of string) does not match the `sender`, reject - unless the sender's powerlevel is greater than the event type's *required power level*.
Copy link
Member

Choose a reason for hiding this comment

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

To overwrite another user's state event, must your power level exceed the event type's PL, or the PL of the user who owns the event?

i.e. should this be reworded to the following:

Suggested change
> If the event has a `state_key` that starts with an `@`, and the substring before the first `_` that follows the first `:` (or end of string) does not match the `sender`, reject - unless the sender's powerlevel is greater than the event type's *required power level*.
> If the event has a `state_key` that starts with an `@`, and the substring before the first `_` that follows the first `:` (or end of string) does not match the `sender`, reject - unless the sender's powerlevel is greater than the powerlevel of the user whose matrix ID is the prefix of the `state_key`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To overwrite another user's state event, must your power level exceed the event type's PL, or the PL of the user who owns the event?

I was thinking of the event type's power level i.e. we're not changing anything here except expanding the existing exception where a user can create a state event beyond their power level if they use a specific state key.

Could the wording be improved to clarify this?

Copy link
Member

Choose a reason for hiding this comment

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

a user can create a state event beyond their power level if they use a specific state key

This isn't mentioned anywhere, at least not explicitly. If the only intended change to the auth rules is the change to step 8, then the preceding step still says to reject if the sender's PL is less than the event type’s required power level.

For the exception to hold, step 7 could simply be removed, meaning the PL restriction would be applied by the proposed change to rule 8.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, no -- that would remove PL restrictions for non-state events.

How about replacing steps 7 and 8 with this:

  • If the event type’s required power level is greater than the sender’s power level, and the event does not have a state_key matching the sender optionally followed by a substring prefixed with _, reject.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hang on, I was getting confused with #3779 - sorry for the confusion!

I think this MSC restricts who can overwrite certain state events, and I agree that the wording as it is now doesn't make sense, because it actually allows everyone to overwrite, because we won't reach rule 8 without first passing rule 7 which already checks power level.

I think I just prefer #3779 over this one, and if I understand right, we don't need both.

Copy link
Member

Choose a reason for hiding this comment

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

Circling back on this: since it's not this MSC that bypasses event type power levels, I believe my proposed rewording to the auth rule is correct.

Also, with that rewording, perhaps this rule would do the same job as some of the auth rules related to the PLs of the sender & target users of m.room.member events (like the sender being able to kick/ban only target users of a lower PL than them), in which case they could be removed to simplify the auth rules somewhat. (The rules for invite & join would need to stay the same though, lest a powerful user could force anyone to join).

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this makes sense as currently written. We can only get to rule 8 if we've already passed rule 7, which means that the sender's power level is not less than the required power level.

Assuming you mean "not less than" where you've written "greater than", then this whole rule becomes a no-op.

Copy link
Member

Choose a reason for hiding this comment

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

/me rereads comment thread properly. @AndrewFerr's proposal sounds correct.

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 a bit confused as to what the rule is, and the thread context isn't really helping, sorry. Can we try writing the rule into an if-else ladder without keywords like "unless" please?

Copy link
Member

Choose a reason for hiding this comment

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

I've reworded the auth rule in 5e95ff3. I also added a condition to reject state keys beginning with @ that are not valid MXIDs, which prevents the power level comparison from looking up the PL of something that is not a user.

@andybalaam
Copy link
Contributor Author

I would like to add another solution. State keys could be of type: string | string[] Where state_key: string = "hello" is a shortcut for state_key: string[] = ["hello"]. Our access rule would become:

I would buy into this if others felt it was better. I think it would make @turt2live happier?

I feel a little concerned about the ambiguity of whether "" = [""] or [] but that is a minor detail.

@erikjohnston
Copy link
Member

I would like to add another solution. State keys could be of type: string | string[] Where state_key: string = "hello" is a shortcut for state_key: string[] = ["hello"]. Our access rule would become:

I would buy into this if others felt it was better. I think it would make @turt2live happier?

I feel a little concerned about the ambiguity of whether "" = [""] or [] but that is a minor detail.

I think changing the format of state_key is a pretty big change, and personally not something I would want to block this proposal on (e.g. we'd need to update DB tables in Synapse, update state resolution code, etc etc)

@erikjohnston
Copy link
Member

It feels like this is a fairly useful thing to support, with no great alternative way of doing it in the short/medium term.

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Aug 15, 2024

Team member @mscbot has proposed to merge this. The next step is review by the rest of the tagged people:

Concerns:

  • changes to auth rules are unclear
  • issue of state event proliferation requires discussion
  • Does not work with all MXIDs
  • Introduction does not sufficiently list benefiting MSCs/features.
  • Insufficient implementation - Complement tests are required for this MSC.
  • Alternatives section is missing some alternatives.
  • Specific alternative of top-level access control is not sufficiently discounted.

Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for information about what commands tagged team members can give me.

@mscbot mscbot added proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. disposition-merge labels Aug 15, 2024
Comment on lines +5 to +9
Currently there are three main ways to limit who can overwrite a state event:

* If a user's PL is greater than the `m.room.power_levels` `state_default` field
* If a user's PL is greater than the `m.room.power_levels` `events` field for that event type
* If a state event has a state key which begins with an `@`, then the sender's mxid must match that state key.
Copy link
Member

Choose a reason for hiding this comment

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

I find this a confusing way of phrasing the current restrictions.

I'd say that there are essentially two current restrictions, imposed by rules 7 and 8 in the auth rules.

In short:

  1. The user's PL must not be less than the "required power level" for that event.
  2. In addition, if a state event has a state key which begins with an @, then the sender's mxid must match that state key.

The spec for m.room.power_levels defines required power level, but in summary, it is determined by the setting in m.room.power_levels events field for that event type if present; otherwise, it is determined by the state_default field.

@@ -0,0 +1,118 @@
# MSC3757: Restricting who can overwrite a state event.
Copy link
Member

Choose a reason for hiding this comment

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

I think this is actually derestricting who can overwrite a state event.

Suggested change
# MSC3757: Restricting who can overwrite a state event.
# MSC3757: Relaxing the restrictions on who can overwrite a state event.

Copy link
Member

Choose a reason for hiding this comment

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

That's true for MSC3779, but not as much for this one; the former bypasses PL restrictions for setting state that belongs to the sender (where "belongs to" = the state key starts with the sender's MXID), but this MSC does not.

The restriction proposed by this MSC is to prevent state that belongs to a particular user from being overwritten by other, equally-powerful users.

The only PL restriction that's relaxed by this MSC is for allowing more powerful users to overwrite state that doesn't belong to them, for the sake of having a tool against state graffiti.

Copy link
Member

Choose a reason for hiding this comment

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

You're right. Still, I find the title a bit confusing. How about:

Suggested change
# MSC3757: Restricting who can overwrite a state event.
# MSC3757: Extending the set of write-protected state keys

or something


becomes:

> If the event has a `state_key` that starts with an `@`, and the substring before the first `_` that follows the first `:` (or end of string) does not match the `sender`, reject - unless the sender's powerlevel is greater than the event type's *required power level*.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this makes sense as currently written. We can only get to rule 8 if we've already passed rule 7, which means that the sender's power level is not less than the required power level.

Assuming you mean "not less than" where you've written "greater than", then this whole rule becomes a no-op.

Comment on lines 37 to 43
Practically speaking, this means modifying the [authorization rules](https://spec.matrix.org/v1.2/rooms/v9/#authorization-rules) such that rule 8:

> If the event has a `state_key` that starts with an `@` and does not match the `sender`, reject.

becomes:

> If the event has a `state_key` that starts with an `@`, and the substring before the first `_` that follows the first `:` (or end of string) does not match the `sender`, reject - unless the sender's powerlevel is greater than the event type's *required power level*.
Copy link
Member

Choose a reason for hiding this comment

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

Last time we changed the auth rules, there was a lot of discussion around "let's never do this again without test vectors for the auth rules" (cf matrix-org/matrix-spec#1710, matrix-org/matrix-spec#1642).

The apparent confusion over what the current auth rules mean, and what they should say after the change, makes me think that we should start some progress in that direction.

Comment on lines +69 to +71
## Potential issues

None yet.
Copy link
Member

Choose a reason for hiding this comment

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

I remain concerned that this MSC opens the door to a proliferation of state events that we cannot delete, and that faster-joins can't help with. I would like to see some discussion of this in the MSC.

Admittedly, we have this problem to some extent today, in that there is nothing stopping users creating lots of state events with different types, but:

  • (a) such behaviour is currently restricted to room moderators, because it relies on their power level being state_default or higher. This MSC would change that, by allowing any user to (say) create millions of m.beacon_info events.
  • (b) there is currently no specced usecase that requires multiple state events per user, and introducing one feels like a step-change. Arguably that is not a problem of this MSC per se (and rather MSCs that make use of it such as MSC3672), but I think we'll need to at least set some expectations here.

This is not a veto from me, but I do think we need to consider this properly and at least set out guidelines for future MSCs that build on this.

Copy link
Member

Choose a reason for hiding this comment

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

  • (a) ... This MSC would change that, by allowing any user to (say) create millions of m.beacon_info events.

This is what MSC3779 proposes, not this one.

To clarify, the effective auth rule changes proposed by this MSC are to:

  • grant write access on state events with state keys of the form <sender>_<anySuffix> to all users
    • N.B. <sender> is the MXID of the user sending the state event
  • grant write access on state events with state keys of the form <mxid> & <mxid>_<anySuffix> to users whose PL is greater than that of <mxid>'s PL
    • N.B. <mxid> is any MXID

The event-sending restrictions imposed by m.room.power_levels remain in effect.

What this MSC does allow is creating lots of state events with different state key suffixes -- but that would be limited to room moderators, just like how creating lots of state events with different types currently is.

  • (b) there is currently no specced usecase that requires multiple state events per user, and introducing one feels like a step-change.

As a start, MSC4143 makes an explicit mention to this one (namely, in the section regarding MatrixRTC room state), and is what has driven interest in this MSC.

@richvdh
Copy link
Member

richvdh commented Aug 15, 2024

@mscbot concern changes to auth rules are unclear
@mscbot concern issue of state event proliferation requires discussion

@mscbot mscbot added the unresolved-concerns This proposal has at least one outstanding concern label Aug 15, 2024
Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

@mscbot concern Does not work with all MXIDs


## Potential issues

None yet.
Copy link
Member

Choose a reason for hiding this comment

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

This is going to be incompatible with some Matrix IDs:

state_key MUST NOT exceed 255 bytes. source

The length of a user ID, including the @ sigil and the domain, MUST NOT exceed 255 bytes. source

If you have a user at the maximum length (or close to it) then there's no room for a suffix.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's also my main concern. There is no guarantee on how large the suffix can be, so users will get varying experiences depending on how long their mxid is. This MSC should specify new sizes for mxids, state keys and as a result the suffix, that can be appended in a predictable manner. Alternatively, since this requires a new room version anyway and would be a breaking change for clients, #3760 would probably have the same impact on implementations.

Copy link
Member

Choose a reason for hiding this comment

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

to be honest, I'm less concerned about this particular issue. Users which make use of the full 255 bytes available to them are usually banned on sight for having silly names. The vast majority of users are well within the 255 byte limit.

I'd be supportive of making user IDs shorter generally, but that's a very different discussion for another MSC. This MSC creating further implicit limits on participation for "silly" user IDs is a +1 from me.

Copy link
Member

Choose a reason for hiding this comment

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

This MSC creating further implicit limits on participation for "silly" user IDs is a +1 from me.

I'm having trouble parsing this sentence. You think that it isn't a problem to have parts of the protocol incompatible with each other?

Having MXIDs "usually" banned in those situations is not a good enough reason IMO. I feel strongly about that more internal inconsistency is bad for the project.

Copy link
Member

Choose a reason for hiding this comment

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

The inconsistency doesn't bother me because I would favour shortening the allowable length anyways. I see this MSC as a very small stepping stone in that direction (we'd resolve the inconsistency in favour of shorter user IDs, in my opinion/dream).

@clokep
Copy link
Member

clokep commented Aug 15, 2024

@mscbot concern Does not work with all MXIDs

Reposting due to matrix-org/mscbot-python#26 not yet being deployed.

@turt2live turt2live self-requested a review August 15, 2024 16:47
}
```

Since `:` is not permitted in the localpart and `_` is not permitted in the domain part of an mxid (see [Historical User IDs](https://spec.matrix.org/v1.2/appendices/#historical-user-ids)), it is not possible to craft an mxid that matches the beginning of a state key constructed for another user's mxid, so state keys restricted to one owner can never be overwritten by another user.
Copy link
Contributor

Choose a reason for hiding this comment

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

While the spec specifies a grammar for server names, are we SURE that servers verify that grammar? While officially underscores are invalid in domain names and you can't order a TLS certificate for it nowadays, such domains still exist in the wild and wildcard certificates even allow using TLS with them. For example: https://my_sarisari_store.typepad.com/

I think relying on underscores as a separator is a rather scary trap and I wouldn't bet on all currently developed Matrix servers rejecting such server names.

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

Quick process note: there appear to be several lengthy comments not contained to threads on this MSC. I haven't reviewed those because github makes it difficult to track replies - please move them to the MSC text or into a thread for better visibility.


On the Apache voting scale, I'm at a -1.0 for how the MSC is currently written. A stronger rationale for why this is needed now and strong discounting of a top-level ACL structure would change this to a -0.9 (sorry, I'm just not in favour of string packing here).

Comment on lines +11 to +17
This is problematic if a user needs to publish multiple state
events of the same type in a room, but would like to set access control so
that only they can subsequently update the event. An example of this is if a
user wishes to publish multiple live location share beacons as per [MSC3489](https://github.com/matrix-org/matrix-spec-proposals/pull/3489)
and [MSC3672](https://github.com/matrix-org/matrix-spec-proposals/pull/3672), for instance one per device. They will typically not want
other users in the room to be able to overwrite the state event,
so we need a mechanism to prevent other peers from doing so.
Copy link
Member

Choose a reason for hiding this comment

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

MSC3489 and MSC3672 are not prioritized for inclusion right now, which raises questions about why this MSC is put up for FCP if we're not going to use it for a while. A question around whether the implementation is suitably deployed also comes to mind, though I'm not certain enough to raise that as a FCP-blocking concern.

An update to the introduction to better list out all the features which benefit from this MSC feels needed.

Copy link
Member

Choose a reason for hiding this comment

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

I'll let @AndrewFerr talk more about his use case, but broadly I think its for per-device call state within a room.

Copy link
Member

Choose a reason for hiding this comment

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

its for per-device call state within a room.

That's correct; more precisely, it's for per-device call membership state (as a user can join a call from multiple devices at once).

Currently, the call membership state is an array, with one element per participating device. The problem is that updating that state array is prone to race conditions, as adding/removing an entry to the array is dependent on the current value of the array.

This MSC allows for using one state event per device, which each device can freely update without risk of any other device (or user, for that matter) overwriting it at the same time.

Comment on lines 19 to 25
[MSC3489](https://github.com/matrix-org/matrix-spec-proposals/pull/3489) originally proposed that the event type could be made variable,
appending an ID to each separately posted event so that each one could
separately be locked to the same mxid in the state_key. However, this is
problematic because you can't proactively refer to these event types in the
`events` field of the `m.room.power_levels` event to allow users to post
them - and they also are awkward for some client implementations to
manipulate.
Copy link
Member

Choose a reason for hiding this comment

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

This paragraph should be in the alternatives section.

## Proposal

Therefore, we need a different way to state that a given state event may only
be written by its owner. **We propose that if a state event's state_key *starts with* a matrix ID (followed by an underscore), only the sender with that matrix ID (or higher PL users) can set the state event.** This is an extension of the current behaviour where state events may be overwritten only if the sender's mxid *exactly equals* the state_key.
Copy link
Member

Choose a reason for hiding this comment

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

Proposal language should generally be assertive: "The state_key restrictions are changed to..." rather than "The state key should be fixed". This would also resolve a question about who 'we' is.

(please also limit lines to ~100 characters for easier review)


becomes:

> If the event has a `state_key` that starts with an `@`, and the substring before the first `_` that follows the first `:` (or end of string) does not match the `sender`, reject - unless the sender's powerlevel is greater than the event type's *required power level*.
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 a bit confused as to what the rule is, and the thread context isn't really helping, sorry. Can we try writing the rule into an if-else ladder without keywords like "unless" please?


## Potential issues

None yet.
Copy link
Member

Choose a reason for hiding this comment

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

to be honest, I'm less concerned about this particular issue. Users which make use of the full 255 bytes available to them are usually banned on sight for having silly names. The vast majority of users are well within the 255 byte limit.

I'd be supportive of making user IDs shorter generally, but that's a very different discussion for another MSC. This MSC creating further implicit limits on participation for "silly" user IDs is a +1 from me.


None yet.

## Alternatives
Copy link
Member

Choose a reason for hiding this comment

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

This alternative appears to be missing from the MSC.


* If a user's PL is greater than the `m.room.power_levels` `state_default` field
* If a user's PL is greater than the `m.room.power_levels` `events` field for that event type
* If a state event has a state key which begins with an `@`, then the sender's mxid must match that state key.
Copy link
Member

Choose a reason for hiding this comment

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

I find the @ prefix restriction a bit questionable, for the record. The only specified state event which uses it is m.room.member, which the auth rules validate state_key well before the restriction is checked. Non-spec usage of the state key already applies tricks like prefixing user IDs with _ to ensure they aren't hit by the restriction, and I'm not really convinced that location sharing/beacons need to pack a user ID into the state key anyways (see https://github.com/matrix-org/matrix-spec-proposals/pull/3757/files#r1103877363 ).

This has me leaning towards removing the @ prefix restriction entirely, but I recognize that's not exactly a helpful opinion for this MSC to tackle. Creating stronger arguments for why the dependent features require the user ID in the state key I think would help, though.


## Security considerations

This change requires a new room version, so will not affect old events.
Copy link
Member

Choose a reason for hiding this comment

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

The mention of requiring a new room version should be at the top of the Proposal section. How I've done this in the past is literally:

In a future room version...

[actual proposal]


## Unstable prefix

While this MSC is not considered stable, implementations should apply the behaviours of this MSC on top of room version 9 as `org.matrix.msc3757`.
Copy link
Member

Choose a reason for hiding this comment

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

The Synapse implementation appears to also have identifiers for v10 and v11. Those should be noted here.

@turt2live
Copy link
Member

@mscbot concern Introduction does not sufficiently list benefiting MSCs/features.
@mscbot concern Insufficient implementation - Complement tests are required for this MSC.
@mscbot concern Alternatives section is missing some alternatives.
@mscbot concern Specific alternative of top-level access control is not sufficiently discounted.

@AndrewFerr
Copy link
Member

Complement tests are added by matrix-org/complement#733.

@AndrewFerr
Copy link
Member

There've been some issues/concerns raised about the state key string-packing:

Adding a top-level event field to signify the "owner" of an event would help solve these issues, and is akin to the idea of state sub-keys proposed by MSC3760.

However, that approach was dismissed by https://github.com/matrix-org/matrix-spec-proposals/pull/3757/files#r848454293 & #3757 (comment) due to the friction of redefining state events & state resolution.

So it seems like either option (string-packed state keys vs a top-level owner field) has problems. Should the goal be to decide which option is less problematic, or to arrive at a compromise / a different option?

- Recommend version 10 as it is the current default
- Allow higher versions as this MSC isn't tied to any single version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API disposition-merge 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. proposal A matrix spec change proposal proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. requires-room-version An idea which will require a bump in room version s2s Server-to-Server API (federation) unassigned-room-version Remove this label when things get versioned. unresolved-concerns This proposal has at least one outstanding concern
Projects
Status: Ready for FCP ticks
Development

Successfully merging this pull request may close these issues.