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

MSC3216: Synchronized access control for Spaces #3216

Open
wants to merge 3 commits into
base: old_master
Choose a base branch
from

Conversation

joepie91
Copy link

@joepie91 joepie91 commented May 24, 2021

Competing alternative for #2962.

Rendered

Signed-off-by: Sven Slootweg <admin@cryto.net>
Signed-off-by: Sven Slootweg <admin@cryto.net>
Signed-off-by: Sven Slootweg <admin@cryto.net>
@joepie91 joepie91 changed the title MSC: Synchronized access control for Spaces MSC3216: Synchronized access control for Spaces May 24, 2021
@turt2live turt2live added kind:core MSC which is critical to the protocol's success proposal-in-review proposal A matrix spec change proposal labels May 25, 2021
@eras
Copy link

eras commented May 25, 2021

This mechanism would be trivially extensible to also handle bans, right, and it would be a sensible extension? Invites are probably best let out of it.

@joepie91
Copy link
Author

This mechanism would be trivially extensible to also handle bans, right, and it would be a sensible extension?

Assuming that 'this mechanism' refers to the approach of having the homeserver 'amplify' an event across all rooms in a Space, then yes. I'm not sure whether a space_defaults key would be the right approach there as well, or whether they should just be replicated as-is.

But either way, I think that's probably best as a separate MSC - as there's already a serviceable solution for ban management (Mjolnir), and the functionality of this MSC does not depend on ban replication in any way, it's probably best to keep this MSC scoped to only powerlevel management so that it can be reviewed and merged ASAP. Replicated PL management is a frequently-requested feature :)


## Proposal

Whereas MSC2962 addresses the problem from an angle of a specially-privileged
Copy link
Member

@ara4n ara4n May 28, 2021

Choose a reason for hiding this comment

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

Firstly, thanks for writing this up and the amount of effort which has gone into the proposal here.

However, I think there is a core misunderstanding on 2962 (which is probably my fault in terms of not writing it up well, or the write-up having bugs).

The proposed pseudo-users in MSC2962 are no different to the admin user(s) which already exist in the room. Just as a malicious server operator could reach in and puppet a human admin in the room, they could equally reach in and puppet the pseudo-user. The pseudo-user(s) have no more or less permission than the room admin(s), and do not cause any more centralisation (given you could & should have one per server where an admin is present).

So I don't think this introduces a confused deputy problem which is any worse than the risk in this proposal of the admin itself being abused - and it shouldn't matter whether changes are made directly by an admin or by the pseudo-user. Meanwhile, it avoids the complexities of "what happens if the admin isn't in the child room".

That said, this MSC might well provide a viable solution too - but I just want to ensure that MSC2962 isn't misunderstood!

Copy link
Author

Choose a reason for hiding this comment

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

To clarify, with "specially-privileged", I don't mean that the pseudo-user has higher privileges than the admin. Rather, I mean that it independently has privileges, and would need to implement its own authorization system - separate from the usual state resolution algorithm - to allow users to use its administrative privileges on their behalf.

This sort of "double authorization" is a typical candidate for the confused deputy problem; the risk is in there being any mismatches whatsoever between the pseudo-user's authorization mechanism and the regular authorization mechanism in the protocol. This could allow a user to command the pseudo-user to make an administrative change that they, themselves, would not have been allowed to make directly (because the change itself is made on the authority of the pseudo-user, not the commanding user).

This is unlike the scenario of a server operator puppeting a user; there, trust has been explicitly placed in that server admin to use this power responsibly (which can include "not at all"), and authorization rule mismatches do not play a role.

Copy link
Member

Choose a reason for hiding this comment

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

sorry for lag on this - have been on holiday.

Rather, I mean that it independently has privileges, and would need to implement its own authorization system - separate from the usual state resolution algorithm - to allow users to use its administrative privileges on their behalf.

Ah, right, so you're worried that the power levels of the admins in a room could get out of sync with the power levels of the serverbots of their servers.

Copy link

@kevincox kevincox left a comment

Choose a reason for hiding this comment

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

I like this because it reuses the existing permissions system and largely could be implemented by a client if desired. I'm not saying that power levels are the ultimate permission system but as long as we are sticking with it I think it makes sense to reuse them. I also think that this model could be applied to a role-based permission system cleanly.

However I do feel that this is currently under-specified about how the space_defaults and m.space.power_levels data is formed making it hard to review without applying my interpretation of what is meant to be stored there.

property that defines the new power levels to apply to the Space's rooms (to be
inserted in the `space_defaults` property of each room's power levels event),
and an `allow_partial_update` property that implements the behaviour described
earlier.

Choose a reason for hiding this comment

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

I may make sense to provide a list of rooms to ignore instead of allowing all failures. This way you can try, get an error back with the rooms that you don't have permission to update, then post the update. This would provide a nice UX as they see what changes they will be making (or the list of rooms their changes won't affect) before actually making the atomic update.

Copy link
Author

Choose a reason for hiding this comment

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

Thinking this over for a bit, I think this is a good idea, also for another reason. It would prevent a sort-of race condition in the following scenario:

  1. User applies powerlevels to A,B,C,D
  2. Fails because of room A
  3. User loses permissions to change powerlevels in B
  4. User retries with allow_partial_update

Within specifying explicit rooms to allow failure for, this would result in the change being applied to C and D, whereas the client may have previously believed that it would also apply to B (which is no longer true).

This does introduce a tradeoff, though: clients need to be prepared to deal with potentially multiple failures in a row, UX-wise. Either by choosing to accept the change automatically and retry with an expanded allow-failures list, or by telling the user "hey something else has changed, now the list of unapplied rooms will be this list instead".

Does this sound right, or can you see some kind of UX issue with this?

explicitly-given ones; this works similarly to MSC2962, where the
explicitly-given settings take precedence.

### Technical changes

Choose a reason for hiding this comment

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

I think this is backwards. Right now it puts the inherited keys in a separate key. This means that it changes the permission check algorithm. I wonder if it makes more sense to add an overrides key, and maybe even local_overrides and child_overrides keys. (Shown below, but maybe we can skip them until we have specific use cases).

When you update one of these keys the homeserver should propagate the changes by recomputing the regular m.room.power_levels object. This way the m.room.power_levels is still all that is needed to check permissions, using the same algorithm. I think the significant downside is that a client that is not spaces aware will have their changes overwritten on the next inherited change.

So for example:

// Space
{
   overrides: { ... }, // Human editable.
   local_overrides: { ... }, // Human editable.
   child_overrides: { ... }, // Human editable.
   ... // Computed as overrides + local_overrides
}

// Sub-space
{
   overrides: { ... }, // Human editable.
   local_overrides: { ... }, // Human editable.
   child_overrides: { ... }, // Human editable.
   ... // Computed as:
       //   space.overrides + space.child_overrides
       //   + overrides + local_overrides
}

// Room
{
   overrides: { ... }, // Human editable.
   local_overrides: { ... }, // Human editable.
   child_overrides: { ... }, // Human editable.
   ... // Computed as:
       //   space.overrides + space.child_overrides
       //   + subspace.overrides + subspace.child_overrides
       //   + overrides + local_overrides
}

(For now I assume that each space has one "main" parent. It would be possible to do merging but the expected behaviour there is probably highly controversial).

Comment on lines +115 to +121
Additionally, an `m.space.power_levels` state event would be introduced, which
stores the most recent replicated powerlevels directly into the Space. This
event does not play a role in the state resolution algorithm, and only serves as
a way for clients to learn what the most recent Space-level powerlevels were set
to, for UI/display purposes. This way, clients do not need to pull the
`space_defaults` from multiple rooms and try to work out which of them actually
belong to this specific Space.

Choose a reason for hiding this comment

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

What is exactly in this event? Is is just a copy of the space_defaults from the parent space? What if there are nested parents? In that case is it the "merge" of all of them?

Also I'm not sure how this is not used in the resolution algorithm. How else does the algorithm get the space_defaults from parent spaces?

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 also finding this quite confusing - an example would go a long way here.

Copy link
Author

Choose a reason for hiding this comment

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

This is essentially just a bookkeeping event, that is notably stored in the Space, not in its rooms.

When a user opens up the "Space-wide powerlevels" UI, it will need to be pre-populated with the most recent set of Space-wide powerlevels (ie. "replicated powerlevels"), so that they can adjust them and execute a new replication. But where would it get these previously replicated PLs from?

That's what this event is for. It's essentially just a copy of whatever the last replicated PLs for the space were, for clients to work from UI-wise. If it didn't exist, clients would have to fish these powerlevels out of the space_defaults from rooms in the Space - and importantly, if the room is in multiple Spaces, the client would need to somehow work out whether those space_defaults were actually set by the specific Space that the user is trying to edit or not, which would get messy fast.

So, in summary:

  • space_defaults: Key within m.room.power_levels, stored in a child room. Used in state resolution to authorize room events.
  • m.space.power_levels: Event type, stored in a Space. Not used in state resolution, but used to populate client UIs when a user wishes to edit the Space-wide PLs.

As long as the space-default PLs in a child room are only set through a single parent space, these two things will always contain the exact same powerlevels.

Does that clarify it?

whether they would like to apply the change to those rooms where it is
possible anyway. If the user chooses yes, the operation is reattempted, but
this time with an "allow partial" flag, and it succeeds. This case will
typically occur when a Space is not correctly set up, or in private Spaces
Copy link
Contributor

Choose a reason for hiding this comment

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

This would also happen on a "correctly set up" space where the space is a public umbrella of independent thematic rooms, like "Country space" or "International project" space (like OSM).

@turt2live turt2live added the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Jun 8, 2021
@turt2live turt2live self-requested a review October 11, 2021 15:21
is not possible because the user has an insufficient role in some rooms, then
the operation will atomically fail unless an "allow partial" flag is set. If the
replication fails in *all* rooms, the operation will fail completely, with or
without the flag.
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this also mean it would fail if the user just didn't want to be in all of the rooms in that space?

Copy link
Author

@joepie91 joepie91 Oct 29, 2021

Choose a reason for hiding this comment

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

Hmm. I think it is technically possible allowed by the spec for a user to create an event in a room that they are not joined to, as long as they are not banned or kicked and they have a sufficient powerlevel assigned to them? I'm not 100% sure about this, though. If not, then yes, that would be a concern.

as a whole, how can this be replicated to all of its child rooms in a
semantically consistent and predictable manner?

This approach has three main advantages:
Copy link
Member

Choose a reason for hiding this comment

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

On a purely MSC-style note, it's better to tell the reader what the proposal is before telling them the advantages, then list the advantages along with the disadvantages later: it's not a sales pitch.

Copy link
Author

Choose a reason for hiding this comment

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

While the specific 'advantages' phrasing here would probably disappear if this MSC were not phrased as 'an alternative to ____', and it would be about 'motivation' instead, I disagree that the content itself is in the wrong place, and the ordering is deliberate.

In technical writing, it's generally good practice to lead with a motivation immediately after describing the high-level concept (as has been done here), before diving into the mechanical details; that way, the reader can not only more easily determine the relevance of something to what they are currently working on, but they also have a conceptual frame of reference to understand the specific design decisions and direction in.

(That having been said, in this case point 1 and most of point 2 would no longer be relevant to mention if the references to the other MSC were removed - so the motivation section would end up a lot shorter.)

Comment on lines +115 to +121
Additionally, an `m.space.power_levels` state event would be introduced, which
stores the most recent replicated powerlevels directly into the Space. This
event does not play a role in the state resolution algorithm, and only serves as
a way for clients to learn what the most recent Space-level powerlevels were set
to, for UI/display purposes. This way, clients do not need to pull the
`space_defaults` from multiple rooms and try to work out which of them actually
belong to this specific Space.
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 also finding this quite confusing - an example would go a long way here.

property that defines the new power levels to apply to the Space's rooms (to be
inserted in the `space_defaults` property of each room's power levels event),
and an `allow_partial_update` property that implements the behaviour described
earlier.
Copy link
Member

Choose a reason for hiding this comment

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

So, a client would call this API in addition to settings the power levels in the space directly? And if you used a client that didn't know about this API, your changes wouldn't propagate to the rooms?

Again, an example would be massively appreciated.

Copy link
Author

@joepie91 joepie91 Oct 29, 2021

Choose a reason for hiding this comment

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

In this proposal, the Space is treated as also being a room; therefore, "setting PLs to all rooms in a Space" is a separate operation from "setting PLs for a Space itself". That is why this is a separate endpoint, rather than using a general-purpose "set PL for room" approach.

Calling this endpoint results in two things happening on the server side:

  1. A m.space.power_levels event is created in the Space itself, for bookkeeping purposes (as explained in another thread).
  2. The m.room.power_levels events of all child rooms in the Space are updated with new space_defaults.

Both of these contain the same powerlevels, as specified in the power_levels property of the API call. Notably, no m.room.power_levels is created in the Space itself - as that would be a semantically different operation.


The high-level mechanism works as follows: when a user updates either the roles
or permissions of a Space, their homeserver will, on their behalf, translate
this to a role/permission update for each room that the Space contains. If this
Copy link
Member

Choose a reason for hiding this comment

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

"The space contains" ie. the spaces's sub-rooms? Doesn't it have to be the other way around, ie. the room needs to declare that it wants to delegate control to a space? Otherwise I can make a space and add any room to it, then trick an admin of that room into changing the power levels of that rooms by changing the power levels of my space.

Copy link
Author

Choose a reason for hiding this comment

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

A central point of this MSC is that the Space itself - deliberately - holds no authority over any room. Every Space-originating PL change in a room is actually made directly by the user initiating it through a Space, who therefore already needs to have the necessary administrative access in the room to apply those changes.

In other words: merely adding a room to a Space is a no-op from an access perspective. For a room PL change to succeed through a Space, the originating user would need to have the necessary permissions already anyway, and so could just have made the changes on the room directly, bypassing the Space entirely.

Under those conditions, do you still see a way in which this could be abused?

@turt2live turt2live added requires-room-version An idea which will require a bump in room version unassigned-room-version Remove this label when things get versioned. labels Oct 13, 2021
@turt2live turt2live removed their request for review October 18, 2021 14:26
operation is only semantically meaningful on a Space, it should perhaps *not* be
consistent in that way.

Subspaces should be accounted for, and treated as a part of the Space, for the
Copy link

Choose a reason for hiding this comment

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

How do the new space defaults interact with any existing space defaults, possibly one that came from a space containing this one, or from an intermediate subspace? In other words, is it possible to use this mechanism in a hierarchical way, or does it support only one "manager" level by design?

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. 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

9 participants