This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
-
-
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
Merged
Merged
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
be618e0
Only count real users when checking for auto-creation of auto-join room
jaywink 62fac9d
Auto-fix a few code style issues
jaywink 8c03cd0
Simplify is_real_user_txn check to trust user_type is null if real user
jaywink e89fea4
Simplify count_real_users SQL to only count user_type is null rows
jaywink aaed6b3
Fix code style, again
jaywink File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 res is None | ||
|
||
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): | ||
""" | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.