-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Only count real users when checking for auto-creation of auto-join room #6004
Changes from 2 commits
be618e0
62fac9d
8c03cd0
e89fea4
aaed6b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Only count real users when checking for auto-creation of auto-join room. |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -322,6 +322,19 @@ def _query_for_auth(self, txn, token): | |||||
|
||||||
return None | ||||||
|
||||||
@cachedInlineCallbacks() | ||||||
def is_real_user(self, user_id): | ||||||
"""Determines if the user is a real user, ie does not have a 'user_type'. | ||||||
|
||||||
Args: | ||||||
user_id (str): user id to test | ||||||
|
||||||
Returns: | ||||||
Deferred[bool]: True if user 'user_type' is null or empty string | ||||||
""" | ||||||
res = yield self.runInteraction("is_real_user", self.is_real_user_txn, user_id) | ||||||
return res | ||||||
|
||||||
@cachedInlineCallbacks() | ||||||
def is_support_user(self, user_id): | ||||||
"""Determines if the user is of type UserTypes.SUPPORT | ||||||
|
@@ -337,6 +350,16 @@ def is_support_user(self, user_id): | |||||
) | ||||||
return res | ||||||
|
||||||
def is_real_user_txn(self, txn, user_id): | ||||||
res = self._simple_select_one_onecol_txn( | ||||||
txn=txn, | ||||||
table="users", | ||||||
keyvalues={"name": user_id}, | ||||||
retcol="user_type", | ||||||
allow_none=True, | ||||||
) | ||||||
return True if res is None or res == "" else False | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, why do we have empty strings here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Can we guarantee There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Somebody would have had to explicitly set it to the empty string, at which point they could have equally set it to something weird anyway. I'd just handle it as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @erikjohnston fixed |
||||||
|
||||||
def is_support_user_txn(self, txn, user_id): | ||||||
res = self._simple_select_one_onecol_txn( | ||||||
txn=txn, | ||||||
|
@@ -421,6 +444,22 @@ def _count_users(txn): | |||||
ret = yield self.runInteraction("count_users", _count_users) | ||||||
return ret | ||||||
|
||||||
@defer.inlineCallbacks | ||||||
def count_real_users(self): | ||||||
"""Counts all users without a special user_type registered on the homeserver.""" | ||||||
|
||||||
def _count_users(txn): | ||||||
txn.execute( | ||||||
"SELECT COUNT(*) AS users FROM users where user_type is null or user_type = ''" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose I will change this too There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmh, seems I broke linting again :( |
||||||
) | ||||||
rows = self.cursor_to_dict(txn) | ||||||
if rows: | ||||||
return rows[0]["users"] | ||||||
return 0 | ||||||
|
||||||
ret = yield self.runInteraction("count_real_users", _count_users) | ||||||
return ret | ||||||
|
||||||
@defer.inlineCallbacks | ||||||
def find_next_generated_user_id_localpart(self): | ||||||
""" | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether it'd be easier to just check if the alias exists, rather than trying to count?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the reason for the count, AFAICT, is that we only want to auto-create rooms when the first (real) user registers. I was initially going to suggest a change that rooms would always be auto-created if missing, but the problem there is that the next registering user would own any room added to the Synapse configuration.
Ideally rooms would be created on Synapse start-up to avoid this. But then the problem is who do you make the creator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum, true. It sort of feels like we're using the "first real user" as a proxy for whether that user is an admin or not. I wonder if we should change it to check for the admin bit? I dunno.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/me throws the "I agree things could be better but since this is modifying existing functionality and things are generally broken for our use case it might be good to fix first, then refactor" -card out there ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But seriously, one possible solution would be to simplify things to not auto-create rooms, making admins who configure auto-join to pre-create the rooms. This would be simplest and avoid the problems and would probably not be too much to ask for admins to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or in fact have a configuration option for a "homeserver" account, e.g.
@admin:example.com
which gets used to create the roomThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When this was first implemented, using the first user was considered a 'good enough' hack to get going, definitely could be improved.