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

MSC3021: Limits API — Part 1: per-User limits #3021

Closed
wants to merge 9 commits into from
Closed

MSC3021: Limits API — Part 1: per-User limits #3021

wants to merge 9 commits into from

Conversation

erkinalp
Copy link

@erkinalp erkinalp commented Feb 22, 2021

This is a series of MSCs that will allow servers to put artificial numeric limits on users and rooms. This MSC, in particular, deals with per-user limits.
Rendered

Signed-off by: Erkin Alp Güney <erkinalp9035@gmail.com>
@turt2live turt2live added kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal proposal-in-review labels Feb 22, 2021
proposals/3021-per-user-limits.md Outdated Show resolved Hide resolved
proposals/3021-per-user-limits.md Outdated Show resolved Hide resolved
proposals/3021-per-user-limits.md Outdated Show resolved Hide resolved
proposals/3021-per-user-limits.md Outdated Show resolved Hide resolved
erkinalp and others added 4 commits February 23, 2021 08:51
Signed-off by: Erkin Alp Güney <erkinalp9035@gmail.com>

Co-authored-by: Hubert Chathi <hubert@uhoreg.ca>
Signed-off by: Erkin Alp Güney <erkinalp9035@gmail.com>
Signed-off by: Erkin Alp Güney <erkinalp9035@gmail.com>
Signed-off by: Erkin Alp Güney <erkinalp9035@gmail.com>
Signed-off by: Erkin Alp Güney <erkinalp9035@gmail.com>
@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
Signed-off by: Erkin Alp Güney <erkinalp9035@gmail.com>
Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

I appreciate the general idea but I'm not sure if these particular limits are going to actually protect homeservers from overloading. Also, there's a big question how much this API should be unified across homeservers - do you expect client applications to be able to change these limits? If not, then adding that to Client-Server API doesn't have much merit.

proposals/3021-per-user-limits.md Outdated Show resolved Hide resolved
A user shall not be allowed to join more rooms once it joins as many rooms as
specified by the per-user room limit. The join requests will be rejected,
invites overturned and knocks ignored by the server. As soon as the number of
rooms user has joined drops below the limit, joins will be processed as usual.
Copy link
Member

Choose a reason for hiding this comment

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

It's all nice but the actual problem for servers, as I understand it, is not really the number of joined rooms but rather their "complexity" (basically and very roughly, how large is the room state from the stateres perspective). Joining Matrix HQ from a weak homeserver has every chance to load that homeserver beyond its capabilities; meanwhile, adding a thousand of 1:1 direct chats on the same server likely won't be as taxing, especially if it's spread across time (which can already be ensured by rate limits). So I wonder how much the fanout limit would actually help with the problem described in the introduction.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I know the number of rooms is not so directly correlated, but it is impossible for a regular user to predict the complexity of a room. However, user can easily know or query how many rooms it has joined and how many members those rooms have.

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 sorry to sound negative but that sounds like "this doesn't help with the actual bottleneck, but it's easy". Just because something's easy doesn't necessarily mean it should be implemented.
Did you look at any prior art on the subject? If not, it would be great if you do before proceeding. This, by the way, is another worthwhile addition to the MSC - what kind of prior art exists (in Matrix and even outside).


If a user has more room than it is allowed to be joined when the limit is put
in place, it is to be auto-kicked from the excess rooms by the server
starting from the last joined one. If a limit for a particular type of room
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 pretty drastic and really needs elaboration on how users can avoid this (grace periods, warnings from admins, anything).

Copy link
Author

Choose a reason for hiding this comment

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

I thought none will be required since the clients will be able to query the limit values and display the relevant warnings. Retroactivity is intentional to keep the implementations simple enough, especially while interacting with other parts of this series of proposals.

Copy link
Member

Choose a reason for hiding this comment

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

So, you prioritised the technology over the user ("it's fine to auto-kick users if it helps to keep implementations simple"). I'm afraid that doesn't add points to the proposal. Querying the limits doesn't help if an admin came and mistakenly reduced the limit down to 10 instead of, say, 100. You put a very high price on such mistakes; and not all rooms are easily (or at all) re-joinable. This is why I expected elaboration; part of the MSC writing rigour is in highlighting your design decisions and describing the rationale of them.

the limit, logins will be processed as usual. If a user has more open sessions
than it is allowed to be open when the limit is put in place, it is to be
logged out from the excess sessions by the server starting from the least
recently active session according to the last log-out time of said sessions.
Copy link
Member

Choose a reason for hiding this comment

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

This has very serious security implications - e.g. that session may be the last one from a verified device. Also, same as above, I wonder how much many user sessions are actually a performance problem - do you have any stats on that for any existing homeserver?

Signed-off by: Erkin Alp Güney <erkinalp9035@gmail.com>

Co-authored-by: Alexey Rusakov <Kitsune-Ral@users.sf.net>
@erkinalp
Copy link
Author

do you expect client applications to be able to change these limits? If not, then adding that to Client-Server API doesn't have much merit.

Yes, clients will be able to query and change limits. I am going to also clarify that not every user might be allowed to change own limits.

Signed-off by: Erkin Alp Güney <erkinalp9035@gmail.com>
A user shall not be allowed to join more rooms once it joins as many rooms as
specified by the per-user room limit. The join requests will be rejected,
invites overturned and knocks ignored by the server. As soon as the number of
rooms user has joined drops below the limit, joins will be processed as usual.
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 sorry to sound negative but that sounds like "this doesn't help with the actual bottleneck, but it's easy". Just because something's easy doesn't necessarily mean it should be implemented.
Did you look at any prior art on the subject? If not, it would be great if you do before proceeding. This, by the way, is another worthwhile addition to the MSC - what kind of prior art exists (in Matrix and even outside).


If a user has more room than it is allowed to be joined when the limit is put
in place, it is to be auto-kicked from the excess rooms by the server
starting from the last joined one. If a limit for a particular type of room
Copy link
Member

Choose a reason for hiding this comment

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

So, you prioritised the technology over the user ("it's fine to auto-kick users if it helps to keep implementations simple"). I'm afraid that doesn't add points to the proposal. Querying the limits doesn't help if an admin came and mistakenly reduced the limit down to 10 instead of, say, 100. You put a very high price on such mistakes; and not all rooms are easily (or at all) re-joinable. This is why I expected elaboration; part of the MSC writing rigour is in highlighting your design decisions and describing the rationale of them.

@KitsuneRal
Copy link
Member

do you expect client applications to be able to change these limits? If not, then adding that to Client-Server API doesn't have much merit.

Yes, clients will be able to query and change limits. I am going to also clarify that not every user might be allowed to change own limits.

That won't be enough. Again - you should explain WHY and HOW.

@erkinalp erkinalp marked this pull request as draft June 20, 2021 14:13
@richvdh richvdh deleted the branch matrix-org:master August 27, 2021 18:24
@richvdh richvdh closed this Aug 27, 2021
@turt2live turt2live added abandoned A proposal where the author/shepherd is not responsive and removed proposal-in-review labels Aug 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abandoned A proposal where the author/shepherd is not responsive 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. proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants