From a4814359ecdddeaa98513ac1150c23ac33b612db Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Sat, 13 Oct 2018 22:58:23 +0100 Subject: [PATCH 01/40] WIP creating and filtering support user --- synapse/app/homeserver.py | 15 +++++++++++++++ synapse/config/server.py | 2 ++ synapse/storage/monthly_active_users.py | 5 +++++ synapse/storage/user_directory.py | 3 +++ 4 files changed, 25 insertions(+) diff --git a/synapse/app/homeserver.py b/synapse/app/homeserver.py index e3f0d99a3ff2..7f9eef227ff7 100755 --- a/synapse/app/homeserver.py +++ b/synapse/app/homeserver.py @@ -555,6 +555,21 @@ def generate_monthly_active_users(): clock.looping_call(generate_monthly_active_users, 5 * 60 * 1000) # End of monthly active user settings + if hs.config.autocreate_support_user: + localpart = hs.config.autocreate_support_user['localpart'] + password = hs.config.autocreate_support_user['password'] + user_id = UserID(localpart, hs.hostname).to_string() + # check not already created + support_user = yield hs.get_datastore().get_users_by_id_case_insensitive(user_id) + # if not create + if not support_user: + registration_handler = hs.get_handlers().registration_handler + (user_id, token) = yield registration_handler.register( + localpart=localpart, + password=password, + ) + + if hs.config.report_stats: logger.info("Scheduling stats reporting for 3 hour intervals") clock.looping_call(start_phone_stats_home, 3 * 60 * 60 * 1000) diff --git a/synapse/config/server.py b/synapse/config/server.py index c1c7c0105e41..e045edd5cda9 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -86,6 +86,8 @@ def read_config(self, config): "mau_trial_days", 0, ) + self.autocreate_support_user = config.get('autocreate_support_user', None) + # Options to disable HS self.hs_disabled = config.get("hs_disabled", False) self.hs_disabled_message = config.get("hs_disabled_message", "") diff --git a/synapse/storage/monthly_active_users.py b/synapse/storage/monthly_active_users.py index 0fe8c8e24c2e..0cb08b088446 100644 --- a/synapse/storage/monthly_active_users.py +++ b/synapse/storage/monthly_active_users.py @@ -33,6 +33,8 @@ def __init__(self, dbconn, hs): self._clock = hs.get_clock() self.hs = hs self.reserved_users = () + localpart = self.hs.config.autocreate_support_user['localpart'] + self.support_user = UserId(localpart, hs.hostName).to_sting() @defer.inlineCallbacks def initialise_reserved_users(self, threepids): @@ -172,6 +174,9 @@ def upsert_monthly_active_user(self, user_id): Deferred[bool]: True if a new entry was created, False if an existing one was updated. """ + # Support user never to be included in MAU stats + if user_id is self.support_user: + return False # Am consciously deciding to lock the table on the basis that is ought # never be a big table and alternative approaches (batching multiple # upserts into a single txn) introduced a lot of extra complexity. diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index a8781b0e5d5b..b8981cdb6d37 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -86,6 +86,9 @@ def add_profiles_to_user_dir(self, room_id, users_with_profile): users_with_profile (dict): Users to add to directory in the form of mapping of user_id -> ProfileInfo """ + # TODO Filter out support user + + if isinstance(self.database_engine, PostgresEngine): # We weight the loclpart most highly, then display name and finally # server name From c8e1bb60043a33d13b0e2258a059760c7f3c9ce0 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 23 Oct 2018 18:09:35 +0100 Subject: [PATCH 02/40] move to using config.support_user_id --- synapse/app/homeserver.py | 1 - synapse/config/server.py | 9 +++++++-- synapse/storage/monthly_active_users.py | 7 +++---- tests/storage/test_monthly_active_users.py | 13 +++++++++++++ tests/utils.py | 3 +++ 5 files changed, 26 insertions(+), 7 deletions(-) diff --git a/synapse/app/homeserver.py b/synapse/app/homeserver.py index 3762a67b28bf..7a494489657a 100755 --- a/synapse/app/homeserver.py +++ b/synapse/app/homeserver.py @@ -580,7 +580,6 @@ def start_generate_monthly_active_users(): password=password, ) - if hs.config.report_stats: logger.info("Scheduling stats reporting for 3 hour intervals") clock.looping_call(start_phone_stats_home, 3 * 60 * 60 * 1000) diff --git a/synapse/config/server.py b/synapse/config/server.py index e045edd5cda9..8fd2d855174c 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -85,8 +85,13 @@ def read_config(self, config): self.mau_trial_days = config.get( "mau_trial_days", 0, ) - - self.autocreate_support_user = config.get('autocreate_support_user', None) + self.support_user_pass = None + self.support_user_id = None + autocreate_support_user = config.get('autocreate_support_user', None) + if autocreate_support_user: + self.support_user_pass = autocreate_support_user['password'] + localpart = autocreate_support_user['localpart'] + self.support_user_id = UserID(localpart, hs.hostname).to_string() # Options to disable HS self.hs_disabled = config.get("hs_disabled", False) diff --git a/synapse/storage/monthly_active_users.py b/synapse/storage/monthly_active_users.py index 0cb08b088446..569e08cc541f 100644 --- a/synapse/storage/monthly_active_users.py +++ b/synapse/storage/monthly_active_users.py @@ -33,8 +33,7 @@ def __init__(self, dbconn, hs): self._clock = hs.get_clock() self.hs = hs self.reserved_users = () - localpart = self.hs.config.autocreate_support_user['localpart'] - self.support_user = UserId(localpart, hs.hostName).to_sting() + self.support_user = None @defer.inlineCallbacks def initialise_reserved_users(self, threepids): @@ -175,8 +174,8 @@ def upsert_monthly_active_user(self, user_id): existing one was updated. """ # Support user never to be included in MAU stats - if user_id is self.support_user: - return False + if user_id is self.hs.config.support_user: + return # Am consciously deciding to lock the table on the basis that is ought # never be a big table and alternative approaches (batching multiple # upserts into a single txn) introduced a lot of extra complexity. diff --git a/tests/storage/test_monthly_active_users.py b/tests/storage/test_monthly_active_users.py index 686f12a0dcdc..1d9119a8de6f 100644 --- a/tests/storage/test_monthly_active_users.py +++ b/tests/storage/test_monthly_active_users.py @@ -28,6 +28,8 @@ def make_homeserver(self, reactor, clock): self.store = hs.get_datastore() hs.config.limit_usage_by_mau = True hs.config.max_mau_value = 50 + hs.config.support_user_id = "@support:test" + hs.config.support_user_pass = "password" # Advance the clock a bit reactor.advance(FORTY_DAYS) @@ -214,3 +216,14 @@ def test_get_reserved_real_user_account(self): self.store.user_add_threepid(user2, "email", user2_email, now, now) count = self.store.get_registered_reserved_users_count() self.assertEquals(self.get_success(count), len(threepids)) + + def test_support_user_not_add_to_mau_limits(self): + count = self.store.get_monthly_active_count() + self.pump() + self.assertEqual(self.get_success(count), 0) + self.store.upsert_monthly_active_user( + self.hs.config.support_user + ) + count = self.store.get_monthly_active_count() + self.pump() + self.assertEqual(self.get_success(count), 0) diff --git a/tests/utils.py b/tests/utils.py index dd347a0c5986..f4ade8892954 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -133,6 +133,9 @@ def default_config(name): config.mau_trial_days = 0 config.mau_limits_reserved_threepids = [] config.admin_contact = None + config.autocreate_support_user = None + config.support_user_id = None + config.support_user_pass = None config.rc_messages_per_second = 10000 config.rc_message_burst_count = 10000 From 0ee8d1b64149205de65e3f81ecc8bd38fc71ae95 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Mon, 29 Oct 2018 06:46:00 +0000 Subject: [PATCH 03/40] wip tests to filter out support user --- synapse/storage/user_directory.py | 6 ++--- tests/storage/test_user_directory.py | 39 ++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index 5699cea855d3..cd25e0771936 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -339,7 +339,7 @@ def get_all_local_users(self): rows = yield self._execute("get_all_local_users", None, sql) defer.returnValue([name for name, in rows]) - def add_users_who_share_room(self, room_id, share_private, user_id_tuples): + def add_users_who_share_room(self, room_id, share_private, user_id_tuples_x): """Insert entries into the users_who_share_rooms table. The first user should be a local user. @@ -350,9 +350,7 @@ def add_users_who_share_room(self, room_id, share_private, user_id_tuples): """ def _add_users_who_share_room_txn(txn): support_user = self.hs.config.support_user_id - for ut in user_id_tuples: - if support_user in ut: - user_id_tuples.remove(ut) + user_id_tuples = filter(lambda x: support_user not in x, user_id_tuples_x) self._simple_insert_many_txn( txn, diff --git a/tests/storage/test_user_directory.py b/tests/storage/test_user_directory.py index 0dde1ab2fea3..12f64de69119 100644 --- a/tests/storage/test_user_directory.py +++ b/tests/storage/test_user_directory.py @@ -75,3 +75,42 @@ def test_search_user_dir_all_users(self): ) finally: self.hs.config.user_directory_search_all_users = False + + @defer.inlineCallbacks + def test_cannot_add_support_user_to_directory(self): + self.hs.config.user_directory_search_all_users = True + self.hs.config.support_user_id = "@support:test" + SUPPORT_USER = self.hs.config.support_user_id + yield self.store.add_profiles_to_user_dir( + "!room:id", + {SUPPORT_USER: ProfileInfo(None, "support")}, + ) + yield self.store.add_users_to_public_room("!room:id", [SUPPORT_USER]) + yield self.store.add_users_who_share_room( + "!room:id", False, ((ALICE, SUPPORT_USER),) + ) + + r = yield self.store.search_user_dir(ALICE, "support", 10) + self.assertFalse(r["limited"]) + self.assertEqual(0, len(r["results"])) + + # add_users_who_share_room + # add_users_to_public_room + # add_profiles_to_user_dir + # update_user_in_user_dir + # update_profile_in_user_dir + # update_user_in_public_user_list + + # yield self.store.add_profiles_to_user_dir( + # "!room:id", + # {SUPPORT_USER: ProfileInfo(None, "support")}, + # ) + # yield self.store.add_profiles_to_user_dir(SUPPORT_USER, + # + # + # + # yield self.store.add_users_to_public_room("!room:id", [SUPPORT_USER]) + # + # yield self.store.add_users_who_share_room( + # "!room:id", False, ((ALICE, SUPPORT_USER), (BOB, SUPPORT_USER)) + # ) From 158eccdfb11c029d2f9698bbde85974e520da16a Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Fri, 2 Nov 2018 15:51:33 +0000 Subject: [PATCH 04/40] test suppoer user filtering --- synapse/storage/user_directory.py | 4 +-- tests/storage/test_user_directory.py | 50 +++++++++++++--------------- 2 files changed, 26 insertions(+), 28 deletions(-) diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index cd25e0771936..c91996b79912 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -164,7 +164,7 @@ def update_user_in_user_dir(self, user_id, room_id): def update_profile_in_user_dir(self, user_id, display_name, avatar_url, room_id): def _update_profile_in_user_dir_txn(txn): - if user_is is self.hs.config.support_user_id: + if user_id is self.hs.config.support_user_id: return new_entry = self._simple_upsert_txn( txn, @@ -229,7 +229,7 @@ def _update_profile_in_user_dir_txn(txn): @defer.inlineCallbacks def update_user_in_public_user_list(self, user_id, room_id): - if user_is is not self.hs.config.support_user_id: + if user_id is not self.hs.config.support_user_id: yield self._simple_update_one( table="users_in_public_rooms", keyvalues={"user_id": user_id}, diff --git a/tests/storage/test_user_directory.py b/tests/storage/test_user_directory.py index 12f64de69119..290a3fa8e5b3 100644 --- a/tests/storage/test_user_directory.py +++ b/tests/storage/test_user_directory.py @@ -24,6 +24,7 @@ ALICE = "@alice:a" BOB = "@bob:b" BOBBY = "@bobby:a" +ROOM = "!room:id" class UserDirectoryStoreTestCase(unittest.TestCase): @@ -42,7 +43,7 @@ def setUp(self): BOBBY: ProfileInfo(None, "bobby"), }, ) - yield self.store.add_users_to_public_room("!room:id", [ALICE, BOB]) + yield self.store.add_users_to_public_room(ROOM, [ALICE, BOB]) yield self.store.add_users_who_share_room( "!room:id", False, ((ALICE, BOB), (BOB, ALICE)) ) @@ -81,36 +82,33 @@ def test_cannot_add_support_user_to_directory(self): self.hs.config.user_directory_search_all_users = True self.hs.config.support_user_id = "@support:test" SUPPORT_USER = self.hs.config.support_user_id + support_screen_name = "Support" + yield self.store.add_profiles_to_user_dir( - "!room:id", - {SUPPORT_USER: ProfileInfo(None, "support")}, + ROOM, + {SUPPORT_USER: ProfileInfo(None, support_screen_name)}, ) - yield self.store.add_users_to_public_room("!room:id", [SUPPORT_USER]) + yield self.store.add_users_to_public_room(ROOM, [SUPPORT_USER]) yield self.store.add_users_who_share_room( - "!room:id", False, ((ALICE, SUPPORT_USER),) + ROOM, False, ((ALICE, SUPPORT_USER),) + ) + + r = yield self.store.search_user_dir(ALICE, support_screen_name, 10) + self.assertFalse(r["limited"]) + self.assertEqual(0, len(r["results"])) + + yield self.store.update_user_in_user_dir(SUPPORT_USER, ROOM) + yield self.store.update_profile_in_user_dir( + SUPPORT_USER, support_screen_name, None, ROOM ) + yield self.store.update_user_in_public_user_list(SUPPORT_USER, ROOM) - r = yield self.store.search_user_dir(ALICE, "support", 10) + r = yield self.store.search_user_dir(ALICE, support_screen_name, 10) self.assertFalse(r["limited"]) self.assertEqual(0, len(r["results"])) - # add_users_who_share_room - # add_users_to_public_room - # add_profiles_to_user_dir - # update_user_in_user_dir - # update_profile_in_user_dir - # update_user_in_public_user_list - - # yield self.store.add_profiles_to_user_dir( - # "!room:id", - # {SUPPORT_USER: ProfileInfo(None, "support")}, - # ) - # yield self.store.add_profiles_to_user_dir(SUPPORT_USER, - # - # - # - # yield self.store.add_users_to_public_room("!room:id", [SUPPORT_USER]) - # - # yield self.store.add_users_who_share_room( - # "!room:id", False, ((ALICE, SUPPORT_USER), (BOB, SUPPORT_USER)) - # ) + r = yield self.store.get_user_in_directory(SUPPORT_USER) + self.assertEqual(r, None) + + r = yield self.store.get_user_in_public_room(SUPPORT_USER) + self.assertEqual(r, None) From 9d045fef8ee00eb06e204a612192e45ab6170446 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Fri, 2 Nov 2018 15:59:15 +0000 Subject: [PATCH 05/40] tc --- changelog.d/4141.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/4141.feature diff --git a/changelog.d/4141.feature b/changelog.d/4141.feature new file mode 100644 index 000000000000..f0287df05c8f --- /dev/null +++ b/changelog.d/4141.feature @@ -0,0 +1 @@ +Optionally autocreate a support user via config for use in verifying user behaviour of a given server From 76081eb5bcd0dc19335d764646021eb724d418a3 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Fri, 2 Nov 2018 17:32:19 +0000 Subject: [PATCH 06/40] ensure support user created if does not exist --- synapse/app/homeserver.py | 31 +++++++++++++--------- synapse/config/server.py | 5 ++-- synapse/storage/monthly_active_users.py | 2 +- tests/storage/test_monthly_active_users.py | 2 +- 4 files changed, 24 insertions(+), 16 deletions(-) diff --git a/synapse/app/homeserver.py b/synapse/app/homeserver.py index 1453e76fa057..22e4f13c3582 100755 --- a/synapse/app/homeserver.py +++ b/synapse/app/homeserver.py @@ -64,6 +64,7 @@ from synapse.storage import DataStore, are_all_users_on_domain from synapse.storage.engines import IncorrectDatabaseSetup, create_engine from synapse.storage.prepare_database import UpgradeDatabaseException, prepare_database +from synapse.types import UserID from synapse.util.caches import CACHE_SIZE_FACTOR from synapse.util.httpresourcetree import create_resource_tree from synapse.util.logcontext import LoggingContext @@ -553,19 +554,25 @@ def start_generate_monthly_active_users(): clock.looping_call(start_generate_monthly_active_users, 5 * 60 * 1000) # End of monthly active user settings - if hs.config.autocreate_support_user: - localpart = hs.config.autocreate_support_user['localpart'] - password = hs.config.autocreate_support_user['password'] - user_id = UserID(localpart, hs.hostname).to_string() - # check not already created - support_user = yield hs.get_datastore().get_users_by_id_case_insensitive(user_id) - # if not create - if not support_user: - registration_handler = hs.get_handlers().registration_handler - (user_id, token) = yield registration_handler.register( - localpart=localpart, - password=password, + @defer.inlineCallbacks + def create_support_user(): + if hs.config.support_user_id: + # check not already created + support_user = yield hs.get_datastore().get_users_by_id_case_insensitive( + hs.config.support_user_id ) + # if not create + localpart = UserID.from_string(hs.config.support_user_id) + if not support_user: + registration_handler = hs.get_handlers().registration_handler + (user_id, token) = yield registration_handler.register( + localpart=UserID.from_string(hs.config.support_user_id).localpart, + password=hs.config.support_user_pass, + ) + run_as_background_process( + "create_support_user", + create_support_user, + ) if hs.config.report_stats: logger.info("Scheduling stats reporting for 3 hour intervals") diff --git a/synapse/config/server.py b/synapse/config/server.py index 8fd2d855174c..026ad511d2fa 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -17,6 +17,7 @@ import logging from synapse.http.endpoint import parse_and_validate_server_name +from synapse.types import UserID from ._base import Config, ConfigError @@ -89,9 +90,9 @@ def read_config(self, config): self.support_user_id = None autocreate_support_user = config.get('autocreate_support_user', None) if autocreate_support_user: - self.support_user_pass = autocreate_support_user['password'] + self.support_user_pass = unicode(autocreate_support_user['password'], "utf-8") localpart = autocreate_support_user['localpart'] - self.support_user_id = UserID(localpart, hs.hostname).to_string() + self.support_user_id = UserID(localpart, self.server_name).to_string() # Options to disable HS self.hs_disabled = config.get("hs_disabled", False) diff --git a/synapse/storage/monthly_active_users.py b/synapse/storage/monthly_active_users.py index 5e34aa91ac83..829d878617fb 100644 --- a/synapse/storage/monthly_active_users.py +++ b/synapse/storage/monthly_active_users.py @@ -207,7 +207,7 @@ def upsert_monthly_active_user_txn(self, txn, user_id): existing one was updated. """ # Support user never to be included in MAU stats - if user_id is self.hs.config.support_user: + if user_id is self.hs.config.support_user_id: return # Am consciously deciding to lock the table on the basis that is ought # never be a big table and alternative approaches (batching multiple diff --git a/tests/storage/test_monthly_active_users.py b/tests/storage/test_monthly_active_users.py index 79ce092cbaa9..cf33b90b2cff 100644 --- a/tests/storage/test_monthly_active_users.py +++ b/tests/storage/test_monthly_active_users.py @@ -228,7 +228,7 @@ def test_support_user_not_add_to_mau_limits(self): self.pump() self.assertEqual(self.get_success(count), 0) self.store.upsert_monthly_active_user( - self.hs.config.support_user + self.hs.config.support_user_id ) count = self.store.get_monthly_active_count() self.pump() From 89640096b87fa3c46cd6a30d3955c2bf00128516 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Mon, 5 Nov 2018 21:28:42 +0000 Subject: [PATCH 07/40] remove unused variable localpart --- synapse/app/homeserver.py | 1 - 1 file changed, 1 deletion(-) diff --git a/synapse/app/homeserver.py b/synapse/app/homeserver.py index 22e4f13c3582..186ebed2fa27 100755 --- a/synapse/app/homeserver.py +++ b/synapse/app/homeserver.py @@ -562,7 +562,6 @@ def create_support_user(): hs.config.support_user_id ) # if not create - localpart = UserID.from_string(hs.config.support_user_id) if not support_user: registration_handler = hs.get_handlers().registration_handler (user_id, token) = yield registration_handler.register( From 0e962f5de27ed7fd8bacfdd4f37cb675398c24df Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Mon, 5 Nov 2018 22:35:42 +0000 Subject: [PATCH 08/40] fix py2/3 incompatibility --- synapse/config/server.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/config/server.py b/synapse/config/server.py index 026ad511d2fa..38d862698d9f 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -13,8 +13,8 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. - import logging +from builtins import str from synapse.http.endpoint import parse_and_validate_server_name from synapse.types import UserID @@ -90,7 +90,7 @@ def read_config(self, config): self.support_user_id = None autocreate_support_user = config.get('autocreate_support_user', None) if autocreate_support_user: - self.support_user_pass = unicode(autocreate_support_user['password'], "utf-8") + self.support_user_pass = str(autocreate_support_user['password']) localpart = autocreate_support_user['localpart'] self.support_user_id = UserID(localpart, self.server_name).to_string() From 373a4610889bcf3b240ffbffc49be45480f3dded Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 6 Nov 2018 21:42:27 +0000 Subject: [PATCH 09/40] remove need to create support user in homeserver --- synapse/app/homeserver.py | 19 ------------------- synapse/config/server.py | 6 ++---- tests/storage/test_monthly_active_users.py | 1 - 3 files changed, 2 insertions(+), 24 deletions(-) diff --git a/synapse/app/homeserver.py b/synapse/app/homeserver.py index 186ebed2fa27..e2853972beb4 100755 --- a/synapse/app/homeserver.py +++ b/synapse/app/homeserver.py @@ -554,25 +554,6 @@ def start_generate_monthly_active_users(): clock.looping_call(start_generate_monthly_active_users, 5 * 60 * 1000) # End of monthly active user settings - @defer.inlineCallbacks - def create_support_user(): - if hs.config.support_user_id: - # check not already created - support_user = yield hs.get_datastore().get_users_by_id_case_insensitive( - hs.config.support_user_id - ) - # if not create - if not support_user: - registration_handler = hs.get_handlers().registration_handler - (user_id, token) = yield registration_handler.register( - localpart=UserID.from_string(hs.config.support_user_id).localpart, - password=hs.config.support_user_pass, - ) - run_as_background_process( - "create_support_user", - create_support_user, - ) - if hs.config.report_stats: logger.info("Scheduling stats reporting for 3 hour intervals") clock.looping_call(start_phone_stats_home, 3 * 60 * 60 * 1000) diff --git a/synapse/config/server.py b/synapse/config/server.py index 38d862698d9f..159a611e5868 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -86,13 +86,11 @@ def read_config(self, config): self.mau_trial_days = config.get( "mau_trial_days", 0, ) - self.support_user_pass = None self.support_user_id = None autocreate_support_user = config.get('autocreate_support_user', None) if autocreate_support_user: - self.support_user_pass = str(autocreate_support_user['password']) - localpart = autocreate_support_user['localpart'] - self.support_user_id = UserID(localpart, self.server_name).to_string() + autocreate_support_userID = UserID(local_part, self.server_name) + self.support_user_id = autocreate_support_userID.to_string() # Options to disable HS self.hs_disabled = config.get("hs_disabled", False) diff --git a/tests/storage/test_monthly_active_users.py b/tests/storage/test_monthly_active_users.py index cf33b90b2cff..b46727cba072 100644 --- a/tests/storage/test_monthly_active_users.py +++ b/tests/storage/test_monthly_active_users.py @@ -29,7 +29,6 @@ def make_homeserver(self, reactor, clock): hs.config.limit_usage_by_mau = True hs.config.max_mau_value = 50 hs.config.support_user_id = "@support:test" - hs.config.support_user_pass = "password" # Advance the clock a bit reactor.advance(FORTY_DAYS) From 6f19edbda5fb283793f2cd3094ef5c96de0f02d7 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 6 Nov 2018 22:46:40 +0000 Subject: [PATCH 10/40] fix misnamed var --- synapse/config/server.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/synapse/config/server.py b/synapse/config/server.py index 159a611e5868..4d296704bc28 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -87,10 +87,9 @@ def read_config(self, config): "mau_trial_days", 0, ) self.support_user_id = None - autocreate_support_user = config.get('autocreate_support_user', None) - if autocreate_support_user: - autocreate_support_userID = UserID(local_part, self.server_name) - self.support_user_id = autocreate_support_userID.to_string() + support_user = config.get('support_user', None) + if support_user: + self.support_user_id = UserID(support_user, self.server_name).to_string() # Options to disable HS self.hs_disabled = config.get("hs_disabled", False) From bccbdb85a839a1905d46a9fd70f8a8a61d099522 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 6 Nov 2018 23:08:49 +0000 Subject: [PATCH 11/40] replace is with == --- synapse/storage/user_directory.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index c91996b79912..67ab11b02689 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -153,7 +153,7 @@ def _add_profiles_to_user_dir_txn(txn): @defer.inlineCallbacks def update_user_in_user_dir(self, user_id, room_id): - if user_id is not self.hs.config.support_user_id: + if user_id != self.hs.config.support_user_id: yield self._simple_update_one( table="user_directory", keyvalues={"user_id": user_id}, @@ -164,7 +164,7 @@ def update_user_in_user_dir(self, user_id, room_id): def update_profile_in_user_dir(self, user_id, display_name, avatar_url, room_id): def _update_profile_in_user_dir_txn(txn): - if user_id is self.hs.config.support_user_id: + if user_id == self.hs.config.support_user_id: return new_entry = self._simple_upsert_txn( txn, @@ -229,7 +229,7 @@ def _update_profile_in_user_dir_txn(txn): @defer.inlineCallbacks def update_user_in_public_user_list(self, user_id, room_id): - if user_id is not self.hs.config.support_user_id: + if user_id != self.hs.config.support_user_id: yield self._simple_update_one( table="users_in_public_rooms", keyvalues={"user_id": user_id}, From d8b1f519e2faa4efe78c142be48add74a0c81d31 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 6 Nov 2018 23:21:07 +0000 Subject: [PATCH 12/40] remove unused dependency --- synapse/config/server.py | 1 - 1 file changed, 1 deletion(-) diff --git a/synapse/config/server.py b/synapse/config/server.py index 4d296704bc28..59c8bb673488 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -14,7 +14,6 @@ # See the License for the specific language governing permissions and # limitations under the License. import logging -from builtins import str from synapse.http.endpoint import parse_and_validate_server_name from synapse.types import UserID From b7d1f0dffdf3ab8f5d633cf0af6f46e458d6ad4f Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 6 Nov 2018 23:23:36 +0000 Subject: [PATCH 13/40] remove unused import --- synapse/app/homeserver.py | 1 - 1 file changed, 1 deletion(-) diff --git a/synapse/app/homeserver.py b/synapse/app/homeserver.py index e2853972beb4..415374a2ce91 100755 --- a/synapse/app/homeserver.py +++ b/synapse/app/homeserver.py @@ -64,7 +64,6 @@ from synapse.storage import DataStore, are_all_users_on_domain from synapse.storage.engines import IncorrectDatabaseSetup, create_engine from synapse.storage.prepare_database import UpgradeDatabaseException, prepare_database -from synapse.types import UserID from synapse.util.caches import CACHE_SIZE_FACTOR from synapse.util.httpresourcetree import create_resource_tree from synapse.util.logcontext import LoggingContext From 425403398e25afc36cdc84a7dc2db01db82355da Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 6 Nov 2018 23:35:08 +0000 Subject: [PATCH 14/40] update description due to change in desired behaviour --- changelog.d/4141.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/4141.feature b/changelog.d/4141.feature index f0287df05c8f..7df8118d3098 100644 --- a/changelog.d/4141.feature +++ b/changelog.d/4141.feature @@ -1 +1 @@ -Optionally autocreate a support user via config for use in verifying user behaviour of a given server +Optionally configure a support user via config for use in verifying user behaviour of a given server, the support user does not appear in user directory or monthly active user counts. From 06a3ec83e25d62a2bc6ef148c61714e33284774d Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Wed, 7 Nov 2018 22:28:21 +0000 Subject: [PATCH 15/40] replace filter with list comp to aid py2 py3 compat, make test more paranoid --- synapse/storage/user_directory.py | 12 +++++++----- tests/storage/test_user_directory.py | 5 +++++ 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index 67ab11b02689..a9c6cba346e1 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -339,18 +339,20 @@ def get_all_local_users(self): rows = yield self._execute("get_all_local_users", None, sql) defer.returnValue([name for name, in rows]) - def add_users_who_share_room(self, room_id, share_private, user_id_tuples_x): + def add_users_who_share_room(self, room_id, share_private, user_id_tuples): """Insert entries into the users_who_share_rooms table. The first user should be a local user. Args: room_id (str) share_private (bool): Is the room private - user_id_tuples([(str, str)]): iterable of 2-tuple of user IDs. + user_id_tuples_uf([(str, str)]): iterable of 2-tuple of user IDs. """ - def _add_users_who_share_room_txn(txn): + def _add_users_who_share_room_txn(txn, user_id_tuples): support_user = self.hs.config.support_user_id - user_id_tuples = filter(lambda x: support_user not in x, user_id_tuples_x) + + if support_user is not None: + user_id_tuples = [u for u in user_id_tuples if support_user not in u] self._simple_insert_many_txn( txn, @@ -375,7 +377,7 @@ def _add_users_who_share_room_txn(txn): (user_id, other_user_id), ) return self.runInteraction( - "add_users_who_share_room", _add_users_who_share_room_txn + "add_users_who_share_room", _add_users_who_share_room_txn, user_id_tuples ) def update_users_who_share_room(self, room_id, share_private, user_id_sets): diff --git a/tests/storage/test_user_directory.py b/tests/storage/test_user_directory.py index 290a3fa8e5b3..fdeb32f74850 100644 --- a/tests/storage/test_user_directory.py +++ b/tests/storage/test_user_directory.py @@ -97,6 +97,11 @@ def test_cannot_add_support_user_to_directory(self): self.assertFalse(r["limited"]) self.assertEqual(0, len(r["results"])) + # Check that enabled support user does not prevent all users being added + r = yield self.store.search_user_dir(ALICE, ALICE, 10) + self.assertFalse(r["limited"]) + self.assertEqual(1, len(r["results"])) + yield self.store.update_user_in_user_dir(SUPPORT_USER, ROOM) yield self.store.update_profile_in_user_dir( SUPPORT_USER, support_screen_name, None, ROOM From f217b5fd7a74a19b0c1dae3ac64c70fa7e5aa65a Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Wed, 7 Nov 2018 22:29:17 +0000 Subject: [PATCH 16/40] fix case where auto creation of rooms should never auto create for support user --- synapse/handlers/register.py | 12 +++++++++--- tests/handlers/test_register.py | 29 +++++++++++++++++++++++------ 2 files changed, 32 insertions(+), 9 deletions(-) diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index d2beb275cf5d..093da8d3ff2c 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -221,11 +221,17 @@ def register( # auto-join the user to any rooms we're supposed to dump them into fake_requester = create_requester(user_id) - # try to create the room if we're the first user on the server + # try to create the room if we're the first real user on the server. Note + # there maybe a support user present that should autojoin rooms should_auto_create_rooms = False - if self.hs.config.autocreate_auto_join_rooms: + if (self.hs.config.autocreate_auto_join_rooms and + self.hs.config.support_user_id != user_id): count = yield self.store.count_all_users() - should_auto_create_rooms = count == 1 + if self.hs.config.support_user_id is None: + should_auto_create_rooms = count == 1 + elif self.hs.config.support_user_id != user_id: + # assume that support user has been created first + should_auto_create_rooms = count == 2 for r in self.hs.config.auto_join_rooms: try: diff --git a/tests/handlers/test_register.py b/tests/handlers/test_register.py index 3e9a19072701..701bcb7f35e9 100644 --- a/tests/handlers/test_register.py +++ b/tests/handlers/test_register.py @@ -43,10 +43,7 @@ def setUp(self): self.addCleanup, expire_access_token=True, ) - self.macaroon_generator = Mock( - generate_access_token=Mock(return_value='secret') - ) - self.hs.get_macaroon_generator = Mock(return_value=self.macaroon_generator) + self.handler = self.hs.get_handlers().registration_handler self.store = self.hs.get_datastore() self.hs.config.max_mau_value = 50 @@ -64,7 +61,7 @@ def test_user_is_created_and_logged_in_if_doesnt_exist(self): requester, frank.localpart, "Frankie" ) self.assertEquals(result_user_id, user_id) - self.assertEquals(result_token, 'secret') + self.assertTrue(result_token is not None) @defer.inlineCallbacks def test_if_user_exists(self): @@ -82,7 +79,8 @@ def test_if_user_exists(self): requester, local_part, None ) self.assertEquals(result_user_id, user_id) - self.assertEquals(result_token, 'secret') + print("result token is %s" % result_token) + self.assertTrue(result_token is not None) @defer.inlineCallbacks def test_mau_limits_when_disabled(self): @@ -184,3 +182,22 @@ def test_auto_create_auto_join_where_auto_create_is_false(self): res = yield self.handler.register(localpart='jeff') rooms = yield self.store.get_rooms_for_user(res[0]) self.assertEqual(len(rooms), 0) + + @defer.inlineCallbacks + def test_auto_create_auto_join_rooms_when_support_user_exists(self): + room_alias_str = "#room:test" + self.hs.config.auto_join_rooms = [room_alias_str] + self.hs.config.support_user_id = "@support:test" + res_support = yield self.handler.register(localpart='support') + rooms = yield self.store.get_rooms_for_user(res_support[0]) + self.assertTrue(len(rooms) == 0) + + res = yield self.handler.register(localpart='jeff') + + rooms = yield self.store.get_rooms_for_user(res[0]) + directory_handler = self.hs.get_handlers().directory_handler + room_alias = RoomAlias.from_string(room_alias_str) + room_id = yield directory_handler.get_association(room_alias) + + self.assertTrue(room_id['room_id'] in rooms) + self.assertEqual(len(rooms), 1) From 12d09ac132c6ddea068eb950ad1c30a68c70b306 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Thu, 8 Nov 2018 12:42:17 +0000 Subject: [PATCH 17/40] tiday up cruft --- synapse/handlers/register.py | 5 +++-- synapse/storage/monthly_active_users.py | 2 +- synapse/storage/user_directory.py | 2 +- tests/handlers/test_register.py | 2 -- tests/utils.py | 3 --- 5 files changed, 5 insertions(+), 9 deletions(-) diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 093da8d3ff2c..2a2706b78c94 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -222,14 +222,15 @@ def register( fake_requester = create_requester(user_id) # try to create the room if we're the first real user on the server. Note - # there maybe a support user present that should autojoin rooms + # that an auto generated support user is not a real user and never be + # the user to create the room should_auto_create_rooms = False if (self.hs.config.autocreate_auto_join_rooms and self.hs.config.support_user_id != user_id): count = yield self.store.count_all_users() if self.hs.config.support_user_id is None: should_auto_create_rooms = count == 1 - elif self.hs.config.support_user_id != user_id: + else: # assume that support user has been created first should_auto_create_rooms = count == 2 diff --git a/synapse/storage/monthly_active_users.py b/synapse/storage/monthly_active_users.py index 829d878617fb..5c4e7852229a 100644 --- a/synapse/storage/monthly_active_users.py +++ b/synapse/storage/monthly_active_users.py @@ -207,7 +207,7 @@ def upsert_monthly_active_user_txn(self, txn, user_id): existing one was updated. """ # Support user never to be included in MAU stats - if user_id is self.hs.config.support_user_id: + if user_id == self.hs.config.support_user_id: return # Am consciously deciding to lock the table on the basis that is ought # never be a big table and alternative approaches (batching multiple diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index a9c6cba346e1..4502ac49c40e 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -346,7 +346,7 @@ def add_users_who_share_room(self, room_id, share_private, user_id_tuples): Args: room_id (str) share_private (bool): Is the room private - user_id_tuples_uf([(str, str)]): iterable of 2-tuple of user IDs. + user_id_tuples([(str, str)]): iterable of 2-tuple of user IDs. """ def _add_users_who_share_room_txn(txn, user_id_tuples): support_user = self.hs.config.support_user_id diff --git a/tests/handlers/test_register.py b/tests/handlers/test_register.py index 701bcb7f35e9..aa33a3de5197 100644 --- a/tests/handlers/test_register.py +++ b/tests/handlers/test_register.py @@ -43,7 +43,6 @@ def setUp(self): self.addCleanup, expire_access_token=True, ) - self.handler = self.hs.get_handlers().registration_handler self.store = self.hs.get_datastore() self.hs.config.max_mau_value = 50 @@ -79,7 +78,6 @@ def test_if_user_exists(self): requester, local_part, None ) self.assertEquals(result_user_id, user_id) - print("result token is %s" % result_token) self.assertTrue(result_token is not None) @defer.inlineCallbacks diff --git a/tests/utils.py b/tests/utils.py index 806b49944948..4eaf9c0cf661 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -134,12 +134,9 @@ def default_config(name): config.mau_trial_days = 0 config.mau_limits_reserved_threepids = [] config.admin_contact = None - config.autocreate_support_user = None config.support_user_id = None - config.support_user_pass = None config.rc_messages_per_second = 10000 config.rc_message_burst_count = 10000 - config.use_frozen_dicts = False # we need a sane default_room_version, otherwise attempts to create rooms will From 7430a879a01162c1a873a334ff8d19648619c43d Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Thu, 8 Nov 2018 13:03:44 +0000 Subject: [PATCH 18/40] remove white space --- synapse/handlers/register.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 2a2706b78c94..1137606932a9 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -223,7 +223,7 @@ def register( # try to create the room if we're the first real user on the server. Note # that an auto generated support user is not a real user and never be - # the user to create the room + # the user to create the room should_auto_create_rooms = False if (self.hs.config.autocreate_auto_join_rooms and self.hs.config.support_user_id != user_id): From eaac29f7ef3fbc90512dd6509a761c040e2d7840 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Mon, 12 Nov 2018 17:58:49 +0000 Subject: [PATCH 19/40] move to db backed support user --- synapse/api/auth.py | 5 +++-- synapse/api/constants.py | 7 ++++++- synapse/storage/prepare_database.py | 2 +- synapse/storage/registration.py | 14 +++++++++++++ synapse/storage/user_directory.py | 31 +++++++++++++++++++---------- 5 files changed, 45 insertions(+), 14 deletions(-) diff --git a/synapse/api/auth.py b/synapse/api/auth.py index 34382e4e3c8d..8250157ebd3d 100644 --- a/synapse/api/auth.py +++ b/synapse/api/auth.py @@ -791,9 +791,10 @@ def check_auth_blocking(self, user_id=None, threepid=None): threepid should never be set at the same time. """ - # Never fail an auth check for the server notices users + # Never fail an auth check for the server notices users or support user # This can be a problem where event creation is prohibited due to blocking - if user_id == self.hs.config.server_notices_mxid: + if (user_id == self.hs.config.server_notices_mxid or + user_id == self.hs.config.support_user_id): return if self.hs.config.hs_disabled: diff --git a/synapse/api/constants.py b/synapse/api/constants.py index f20e0fcf0bfb..f6f2d6bd6f48 100644 --- a/synapse/api/constants.py +++ b/synapse/api/constants.py @@ -105,7 +105,6 @@ class RoomVersions(object): VDH_TEST = "vdh-test-version" STATE_V2_TEST = "state-v2-test" - # the version we will give rooms which are created on this server DEFAULT_ROOM_VERSION = RoomVersions.V1 @@ -119,3 +118,9 @@ class RoomVersions(object): ServerNoticeMsgType = "m.server_notice" ServerNoticeLimitReached = "m.server_notice.usage_limit_reached" + + +# Allows for user type specific behaviour, if we'd had a crystal ball would +# probably have included admin and guest, normal users are type None +class UserTypes(object): + SUPPORT = "support" diff --git a/synapse/storage/prepare_database.py b/synapse/storage/prepare_database.py index bd740e1e4553..94b07ce64154 100644 --- a/synapse/storage/prepare_database.py +++ b/synapse/storage/prepare_database.py @@ -25,7 +25,7 @@ # Remember to update this number every time a change is made to database # schema files, so the users will be informed on server restarts. -SCHEMA_VERSION = 52 +SCHEMA_VERSION = 53 dir_path = os.path.abspath(os.path.dirname(__file__)) diff --git a/synapse/storage/registration.py b/synapse/storage/registration.py index 80d76bf9d789..46322ba7d4f0 100644 --- a/synapse/storage/registration.py +++ b/synapse/storage/registration.py @@ -24,6 +24,8 @@ from synapse.storage._base import SQLBaseStore from synapse.util.caches.descriptors import cached, cachedInlineCallbacks +from synapse.api.constants import UserTypes + class RegistrationWorkerStore(SQLBaseStore): def __init__(self, db_conn, hs): @@ -450,6 +452,18 @@ def is_guest(self, user_id): defer.returnValue(res if res else False) + @cachedInlineCallbacks() + def is_support_user(self, user_id): + res = yield self._simple_select_one_onecol( + table="users", + keyvalues={"name": user_id}, + retcol="user_type", + allow_none=True, + desc="is_support_user", + ) + + defer.returnValue(res if res == UserTypes.SUPPORT else False) + @defer.inlineCallbacks def user_add_threepid(self, user_id, medium, address, validated_at, added_at): yield self._simple_upsert("user_threepids", { diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index 4502ac49c40e..c8a780c048ae 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -16,7 +16,7 @@ import logging import re -from six import iteritems +from six import iteritems, iterkeys from twisted.internet import defer @@ -31,6 +31,10 @@ class UserDirectoryStore(SQLBaseStore): + def __init__(self, dbconn, hs): + super(UserDirectoryStore, self).__init__(dbconn, hs) + self.store = hs.get_datastore() + @cachedInlineCallbacks(cache_context=True) def is_room_world_readable_or_publicly_joinable(self, room_id, cache_context): """Check if the room is either world_readable or publically joinable @@ -64,10 +68,9 @@ def add_users_to_public_room(self, room_id, user_ids): or publically joinable user_ids (list(str)): Users to add """ - - support_id = self.hs.config.support_user_id - if support_id in user_ids: - user_ids.remove(support_id) + for u in user_ids: + if self.store.is_support_user(u): + user_ids.remove(u) yield self._simple_insert_many( table="users_in_public_rooms", @@ -91,7 +94,13 @@ def add_profiles_to_user_dir(self, room_id, users_with_profile): users_with_profile (dict): Users to add to directory in the form of mapping of user_id -> ProfileInfo """ - users_with_profile.pop(self.hs.config.support_user_id, None) + # remove users of type UserType.SUPPORT + user_ids_to_pop = [] + for user_id in iterkeys(users_with_profile): + if self.store.is_support_user(user_id): + user_ids_to_pop.append(user_id) + for u in user_ids_to_pop: + users_with_profile.pop(u, None) if isinstance(self.database_engine, PostgresEngine): # We weight the loclpart most highly, then display name and finally @@ -153,7 +162,8 @@ def _add_profiles_to_user_dir_txn(txn): @defer.inlineCallbacks def update_user_in_user_dir(self, user_id, room_id): - if user_id != self.hs.config.support_user_id: + + if not self.store.is_support_user(user_id): yield self._simple_update_one( table="user_directory", keyvalues={"user_id": user_id}, @@ -164,7 +174,7 @@ def update_user_in_user_dir(self, user_id, room_id): def update_profile_in_user_dir(self, user_id, display_name, avatar_url, room_id): def _update_profile_in_user_dir_txn(txn): - if user_id == self.hs.config.support_user_id: + if self.store.is_support_user(user_id): return new_entry = self._simple_upsert_txn( txn, @@ -229,7 +239,7 @@ def _update_profile_in_user_dir_txn(txn): @defer.inlineCallbacks def update_user_in_public_user_list(self, user_id, room_id): - if user_id != self.hs.config.support_user_id: + if not self.store.is_support_user(user_id): yield self._simple_update_one( table="users_in_public_rooms", keyvalues={"user_id": user_id}, @@ -352,7 +362,8 @@ def _add_users_who_share_room_txn(txn, user_id_tuples): support_user = self.hs.config.support_user_id if support_user is not None: - user_id_tuples = [u for u in user_id_tuples if support_user not in u] + user_id_tuples = [u for u in user_id_tuples + if not self.store.is_support_user(u)] self._simple_insert_many_txn( txn, From eae8d4a3883ea2f6e89edfffbf258990524b9ca3 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 13 Nov 2018 10:07:03 +0000 Subject: [PATCH 20/40] add db support for support user --- add_user_type_to_users.sql | 18 +++++++++++++++++ .../delta/53/add_user_type_to_users.sql | 20 +++++++++++++++++++ 2 files changed, 38 insertions(+) create mode 100644 add_user_type_to_users.sql create mode 100644 synapse/storage/schema/delta/53/add_user_type_to_users.sql diff --git a/add_user_type_to_users.sql b/add_user_type_to_users.sql new file mode 100644 index 000000000000..eb8d9b19dc78 --- /dev/null +++ b/add_user_type_to_users.sql @@ -0,0 +1,18 @@ +/* Copyright 2018 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + -- To allow for arbitrary account types in future, initially to be used to denote + -- designated support users + +ALTER TABLE users ADD COLUMN user_type TEXT DEFAULT NULL; diff --git a/synapse/storage/schema/delta/53/add_user_type_to_users.sql b/synapse/storage/schema/delta/53/add_user_type_to_users.sql new file mode 100644 index 000000000000..9044e0152c07 --- /dev/null +++ b/synapse/storage/schema/delta/53/add_user_type_to_users.sql @@ -0,0 +1,20 @@ +/* Copyright 2018 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/* record whether we have sent a server notice about consenting to the + * privacy policy. Specifically records the version of the policy we sent + * a message about. + */ +ALTER TABLE users ADD COLUMN user_type TEXT DEFAULT NULL; From 45e0b9dc1e605517711e96d8fa31582c0470660e Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 13 Nov 2018 15:35:20 +0000 Subject: [PATCH 21/40] implementation and tests for db backed support user --- synapse/api/auth.py | 4 +- synapse/api/constants.py | 1 + synapse/config/server.py | 5 -- synapse/handlers/register.py | 18 +++--- synapse/storage/registration.py | 22 ++------ synapse/storage/user_directory.py | 65 +++++++++++++++------- tests/api/test_auth.py | 2 + tests/handlers/test_register.py | 37 ++++++------ tests/storage/test_monthly_active_users.py | 14 ++++- tests/storage/test_user_directory.py | 39 ++++++++++--- tests/utils.py | 1 - 11 files changed, 129 insertions(+), 79 deletions(-) diff --git a/synapse/api/auth.py b/synapse/api/auth.py index 8250157ebd3d..f3a9aeb93462 100644 --- a/synapse/api/auth.py +++ b/synapse/api/auth.py @@ -793,8 +793,8 @@ def check_auth_blocking(self, user_id=None, threepid=None): # Never fail an auth check for the server notices users or support user # This can be a problem where event creation is prohibited due to blocking - if (user_id == self.hs.config.server_notices_mxid or - user_id == self.hs.config.support_user_id): + is_support = yield self.store.is_support_user(user_id) + if user_id == self.hs.config.server_notices_mxid or is_support: return if self.hs.config.hs_disabled: diff --git a/synapse/api/constants.py b/synapse/api/constants.py index f6f2d6bd6f48..a9fb6e73623e 100644 --- a/synapse/api/constants.py +++ b/synapse/api/constants.py @@ -105,6 +105,7 @@ class RoomVersions(object): VDH_TEST = "vdh-test-version" STATE_V2_TEST = "state-v2-test" + # the version we will give rooms which are created on this server DEFAULT_ROOM_VERSION = RoomVersions.V1 diff --git a/synapse/config/server.py b/synapse/config/server.py index 59c8bb673488..c4d660bb61c2 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -16,7 +16,6 @@ import logging from synapse.http.endpoint import parse_and_validate_server_name -from synapse.types import UserID from ._base import Config, ConfigError @@ -85,10 +84,6 @@ def read_config(self, config): self.mau_trial_days = config.get( "mau_trial_days", 0, ) - self.support_user_id = None - support_user = config.get('support_user', None) - if support_user: - self.support_user_id = UserID(support_user, self.server_name).to_string() # Options to disable HS self.hs_disabled = config.get("hs_disabled", False) diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 1137606932a9..9f663ae9b60f 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -225,15 +225,17 @@ def register( # that an auto generated support user is not a real user and never be # the user to create the room should_auto_create_rooms = False - if (self.hs.config.autocreate_auto_join_rooms and - self.hs.config.support_user_id != user_id): + is_support = yield self.store.is_support_user(user_id) + # There is an edge case where the first user is the support user, then + # the room is never created, though this seems unlikely and + # recoverable from given the support user being involved in the first + # place. + print ("is_support is %r" % is_support) + if (self.hs.config.autocreate_auto_join_rooms and not is_support): count = yield self.store.count_all_users() - if self.hs.config.support_user_id is None: - should_auto_create_rooms = count == 1 - else: - # assume that support user has been created first - should_auto_create_rooms = count == 2 - + print ("count %d" % count) + should_auto_create_rooms = count == 1 + print "should_auto_create_rooms is %r" % should_auto_create_rooms for r in self.hs.config.auto_join_rooms: try: if should_auto_create_rooms: diff --git a/synapse/storage/registration.py b/synapse/storage/registration.py index 46322ba7d4f0..9f1dddb5ac1b 100644 --- a/synapse/storage/registration.py +++ b/synapse/storage/registration.py @@ -24,8 +24,6 @@ from synapse.storage._base import SQLBaseStore from synapse.util.caches.descriptors import cached, cachedInlineCallbacks -from synapse.api.constants import UserTypes - class RegistrationWorkerStore(SQLBaseStore): def __init__(self, db_conn, hs): @@ -169,7 +167,7 @@ def add_access_token_to_user(self, user_id, token, device_id=None): def register(self, user_id, token=None, password_hash=None, was_guest=False, make_guest=False, appservice_id=None, - create_profile_with_localpart=None, admin=False): + create_profile_with_localpart=None, admin=False, user_type=None): """Attempts to register an account. Args: @@ -198,7 +196,8 @@ def register(self, user_id, token=None, password_hash=None, make_guest, appservice_id, create_profile_with_localpart, - admin + admin, + user_type ) def _register( @@ -212,6 +211,7 @@ def _register( appservice_id, create_profile_with_localpart, admin, + user_type, ): now = int(self.clock.time()) @@ -246,6 +246,7 @@ def _register( "is_guest": 1 if make_guest else 0, "appservice_id": appservice_id, "admin": 1 if admin else 0, + "user_type": user_type } ) else: @@ -259,6 +260,7 @@ def _register( "is_guest": 1 if make_guest else 0, "appservice_id": appservice_id, "admin": 1 if admin else 0, + "user_type": user_type, } ) except self.database_engine.module.IntegrityError: @@ -452,18 +454,6 @@ def is_guest(self, user_id): defer.returnValue(res if res else False) - @cachedInlineCallbacks() - def is_support_user(self, user_id): - res = yield self._simple_select_one_onecol( - table="users", - keyvalues={"name": user_id}, - retcol="user_type", - allow_none=True, - desc="is_support_user", - ) - - defer.returnValue(res if res == UserTypes.SUPPORT else False) - @defer.inlineCallbacks def user_add_threepid(self, user_id, medium, address, validated_at, added_at): yield self._simple_upsert("user_threepids", { diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index c8a780c048ae..ae7ff5ec73e8 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -20,7 +20,7 @@ from twisted.internet import defer -from synapse.api.constants import EventTypes, JoinRules +from synapse.api.constants import EventTypes, JoinRules, UserTypes from synapse.storage.engines import PostgresEngine, Sqlite3Engine from synapse.types import get_domain_from_id, get_localpart_from_id from synapse.util.caches.descriptors import cached, cachedInlineCallbacks @@ -69,7 +69,8 @@ def add_users_to_public_room(self, room_id, user_ids): user_ids (list(str)): Users to add """ for u in user_ids: - if self.store.is_support_user(u): + is_support = yield self.store.is_support_user(u) + if is_support: user_ids.remove(u) yield self._simple_insert_many( @@ -86,6 +87,7 @@ def add_users_to_public_room(self, room_id, user_ids): for user_id in user_ids: self.get_user_in_public_room.invalidate((user_id,)) + @defer.inlineCallbacks def add_profiles_to_user_dir(self, room_id, users_with_profile): """Add profiles to the user directory @@ -94,10 +96,10 @@ def add_profiles_to_user_dir(self, room_id, users_with_profile): users_with_profile (dict): Users to add to directory in the form of mapping of user_id -> ProfileInfo """ - # remove users of type UserType.SUPPORT user_ids_to_pop = [] for user_id in iterkeys(users_with_profile): - if self.store.is_support_user(user_id): + is_support = yield self.store.is_support_user(user_id) + if is_support: user_ids_to_pop.append(user_id) for u in user_ids_to_pop: users_with_profile.pop(u, None) @@ -156,14 +158,14 @@ def _add_profiles_to_user_dir_txn(txn): self.get_user_in_directory.invalidate, (user_id,) ) - return self.runInteraction( + self.runInteraction( "add_profiles_to_user_dir", _add_profiles_to_user_dir_txn ) @defer.inlineCallbacks def update_user_in_user_dir(self, user_id, room_id): - - if not self.store.is_support_user(user_id): + is_support = yield self.store.is_support_user(user_id) + if not is_support: yield self._simple_update_one( table="user_directory", keyvalues={"user_id": user_id}, @@ -174,8 +176,10 @@ def update_user_in_user_dir(self, user_id, room_id): def update_profile_in_user_dir(self, user_id, display_name, avatar_url, room_id): def _update_profile_in_user_dir_txn(txn): - if self.store.is_support_user(user_id): + is_support = yield self.store.is_support_user(user_id) + if is_support: return + new_entry = self._simple_upsert_txn( txn, table="user_directory", @@ -239,7 +243,8 @@ def _update_profile_in_user_dir_txn(txn): @defer.inlineCallbacks def update_user_in_public_user_list(self, user_id, room_id): - if not self.store.is_support_user(user_id): + is_support = yield self.store.is_support_user(user_id) + if not is_support: yield self._simple_update_one( table="users_in_public_rooms", keyvalues={"user_id": user_id}, @@ -349,6 +354,7 @@ def get_all_local_users(self): rows = yield self._execute("get_all_local_users", None, sql) defer.returnValue([name for name, in rows]) + @defer.inlineCallbacks def add_users_who_share_room(self, room_id, share_private, user_id_tuples): """Insert entries into the users_who_share_rooms table. The first user should be a local user. @@ -358,12 +364,19 @@ def add_users_who_share_room(self, room_id, share_private, user_id_tuples): share_private (bool): Is the room private user_id_tuples([(str, str)]): iterable of 2-tuple of user IDs. """ - def _add_users_who_share_room_txn(txn, user_id_tuples): - support_user = self.hs.config.support_user_id - - if support_user is not None: - user_id_tuples = [u for u in user_id_tuples - if not self.store.is_support_user(u)] + # user_id_tuples = + # [u for u in user_id_tuples if not self.store.is_support_user(u)] + user_id_tuples_filtered = [] + for ut in user_id_tuples: + safe = True + for u in ut: + is_support = yield self.store.is_support_user(u) + if is_support: + safe = False + if safe: + user_id_tuples_filtered.append(ut) + + def _add_users_who_share_room_txn(txn, user_id_tuples_filtered): self._simple_insert_many_txn( txn, @@ -375,10 +388,10 @@ def _add_users_who_share_room_txn(txn, user_id_tuples): "room_id": room_id, "share_private": share_private, } - for user_id, other_user_id in user_id_tuples + for user_id, other_user_id in user_id_tuples_filtered ], ) - for user_id, other_user_id in user_id_tuples: + for user_id, other_user_id in user_id_tuples_filtered: txn.call_after( self.get_users_who_share_room_from_dir.invalidate, (user_id,), @@ -387,8 +400,10 @@ def _add_users_who_share_room_txn(txn, user_id_tuples): self.get_if_users_share_a_room.invalidate, (user_id, other_user_id), ) - return self.runInteraction( - "add_users_who_share_room", _add_users_who_share_room_txn, user_id_tuples + self.runInteraction( + "add_users_who_share_room", + _add_users_who_share_room_txn, + user_id_tuples_filtered, ) def update_users_who_share_room(self, room_id, share_private, user_id_sets): @@ -760,6 +775,18 @@ def search_user_dir(self, user_id, search_term, limit): "results": results, }) + @cachedInlineCallbacks() + def is_support_user(self, user_id): + res = yield self._simple_select_one_onecol( + table="users", + keyvalues={"name": user_id}, + retcol="user_type", + allow_none=True, + desc="is_support_user", + ) + + defer.returnValue(res if res == UserTypes.SUPPORT else False) + def _parse_query_sqlite(search_term): """Takes a plain unicode string from the user and converts it into a form diff --git a/tests/api/test_auth.py b/tests/api/test_auth.py index 379e9c4ab198..69dc40428b1b 100644 --- a/tests/api/test_auth.py +++ b/tests/api/test_auth.py @@ -50,6 +50,8 @@ def setUp(self): # this is overridden for the appservice tests self.store.get_app_service_by_token = Mock(return_value=None) + self.store.is_support_user = Mock(return_value=defer.succeed(False)) + @defer.inlineCallbacks def test_get_user_by_req_user_valid_token(self): user_info = {"name": self.test_user, "token_id": "ditto", "device_id": "device"} diff --git a/tests/handlers/test_register.py b/tests/handlers/test_register.py index aa33a3de5197..d0512f599cab 100644 --- a/tests/handlers/test_register.py +++ b/tests/handlers/test_register.py @@ -181,21 +181,22 @@ def test_auto_create_auto_join_where_auto_create_is_false(self): rooms = yield self.store.get_rooms_for_user(res[0]) self.assertEqual(len(rooms), 0) - @defer.inlineCallbacks - def test_auto_create_auto_join_rooms_when_support_user_exists(self): - room_alias_str = "#room:test" - self.hs.config.auto_join_rooms = [room_alias_str] - self.hs.config.support_user_id = "@support:test" - res_support = yield self.handler.register(localpart='support') - rooms = yield self.store.get_rooms_for_user(res_support[0]) - self.assertTrue(len(rooms) == 0) - - res = yield self.handler.register(localpart='jeff') - - rooms = yield self.store.get_rooms_for_user(res[0]) - directory_handler = self.hs.get_handlers().directory_handler - room_alias = RoomAlias.from_string(room_alias_str) - room_id = yield directory_handler.get_association(room_alias) - - self.assertTrue(room_id['room_id'] in rooms) - self.assertEqual(len(rooms), 1) + # @defer.inlineCallbacks + # def test_auto_create_auto_join_rooms_when_support_user_exists(self): + # room_alias_str = "#room:test" + # self.hs.config.auto_join_rooms = [room_alias_str] + # + # res_support = yield self.handler.register(localpart='support', + # user_type=UserTypes.SUPPORT) + # rooms = yield self.store.get_rooms_for_user(res_support[0]) + # self.assertTrue(len(rooms) == 0) + # + # res = yield self.handler.register(localpart='jeff') + # + # rooms = yield self.store.get_rooms_for_user(res[0]) + # directory_handler = self.hs.get_handlers().directory_handler + # room_alias = RoomAlias.from_string(room_alias_str) + # room_id = yield directory_handler.get_association(room_alias) + # + # self.assertTrue(room_id['room_id'] in rooms) + # self.assertEqual(len(rooms), 1) diff --git a/tests/storage/test_monthly_active_users.py b/tests/storage/test_monthly_active_users.py index b46727cba072..65e1e9a6287a 100644 --- a/tests/storage/test_monthly_active_users.py +++ b/tests/storage/test_monthly_active_users.py @@ -16,6 +16,8 @@ from twisted.internet import defer +from synapse.api.constants import UserTypes + from tests.unittest import HomeserverTestCase FORTY_DAYS = 40 * 24 * 60 * 60 @@ -28,7 +30,7 @@ def make_homeserver(self, reactor, clock): self.store = hs.get_datastore() hs.config.limit_usage_by_mau = True hs.config.max_mau_value = 50 - hs.config.support_user_id = "@support:test" + # Advance the clock a bit reactor.advance(FORTY_DAYS) @@ -226,9 +228,15 @@ def test_support_user_not_add_to_mau_limits(self): count = self.store.get_monthly_active_count() self.pump() self.assertEqual(self.get_success(count), 0) - self.store.upsert_monthly_active_user( - self.hs.config.support_user_id + support_user_id = '@support:test' + self.store.register( + user_id=support_user_id, + token="123", + password_hash=None, + user_type=UserTypes.SUPPORT ) + + self.store.upsert_monthly_active_user(support_user_id) count = self.store.get_monthly_active_count() self.pump() self.assertEqual(self.get_success(count), 0) diff --git a/tests/storage/test_user_directory.py b/tests/storage/test_user_directory.py index fdeb32f74850..6cf6d4a48a6b 100644 --- a/tests/storage/test_user_directory.py +++ b/tests/storage/test_user_directory.py @@ -15,6 +15,7 @@ from twisted.internet import defer +from synapse.api.constants import UserTypes from synapse.storage import UserDirectoryStore from synapse.storage.roommember import ProfileInfo @@ -31,6 +32,7 @@ class UserDirectoryStoreTestCase(unittest.TestCase): @defer.inlineCallbacks def setUp(self): self.hs = yield setup_test_homeserver(self.addCleanup) + self.datastore = self.hs.get_datastore() self.store = UserDirectoryStore(self.hs.get_db_conn(), self.hs) # alice and bob are both in !room_id. bobby is not but shares @@ -80,20 +82,23 @@ def test_search_user_dir_all_users(self): @defer.inlineCallbacks def test_cannot_add_support_user_to_directory(self): self.hs.config.user_directory_search_all_users = True - self.hs.config.support_user_id = "@support:test" - SUPPORT_USER = self.hs.config.support_user_id - support_screen_name = "Support" + SUPPORT_USER = "@support:test" + SUPPOER_USER_SCREEN_NAME = "Support" + + yield self.datastore.register(user_id=SUPPORT_USER, token="123", + password_hash=None, user_type=UserTypes.SUPPORT) + yield self.datastore.register(user_id=ALICE, token="456", password_hash=None) yield self.store.add_profiles_to_user_dir( ROOM, - {SUPPORT_USER: ProfileInfo(None, support_screen_name)}, + {SUPPORT_USER: ProfileInfo(None, SUPPOER_USER_SCREEN_NAME)}, ) yield self.store.add_users_to_public_room(ROOM, [SUPPORT_USER]) yield self.store.add_users_who_share_room( ROOM, False, ((ALICE, SUPPORT_USER),) ) - r = yield self.store.search_user_dir(ALICE, support_screen_name, 10) + r = yield self.store.search_user_dir(ALICE, SUPPOER_USER_SCREEN_NAME, 10) self.assertFalse(r["limited"]) self.assertEqual(0, len(r["results"])) @@ -104,11 +109,11 @@ def test_cannot_add_support_user_to_directory(self): yield self.store.update_user_in_user_dir(SUPPORT_USER, ROOM) yield self.store.update_profile_in_user_dir( - SUPPORT_USER, support_screen_name, None, ROOM + SUPPORT_USER, SUPPOER_USER_SCREEN_NAME, None, ROOM ) yield self.store.update_user_in_public_user_list(SUPPORT_USER, ROOM) - r = yield self.store.search_user_dir(ALICE, support_screen_name, 10) + r = yield self.store.search_user_dir(ALICE, SUPPOER_USER_SCREEN_NAME, 10) self.assertFalse(r["limited"]) self.assertEqual(0, len(r["results"])) @@ -117,3 +122,23 @@ def test_cannot_add_support_user_to_directory(self): r = yield self.store.get_user_in_public_room(SUPPORT_USER) self.assertEqual(r, None) + + @defer.inlineCallbacks + def test_is_support_user(self): + TEST_USER = "@test:test" + SUPPORT_USER = "@support:test" + + res = yield self.store.is_support_user(None) + self.assertFalse(res) + yield self.datastore.register(user_id=TEST_USER, token="123", password_hash=None) + res = yield self.store.is_support_user(TEST_USER) + self.assertFalse(res) + + self.datastore.register( + user_id=SUPPORT_USER, + token="456", + password_hash=None, + user_type=UserTypes.SUPPORT + ) + res = yield self.store.is_support_user(SUPPORT_USER) + self.assertTrue(res) diff --git a/tests/utils.py b/tests/utils.py index 4eaf9c0cf661..cabe29d46130 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -134,7 +134,6 @@ def default_config(name): config.mau_trial_days = 0 config.mau_limits_reserved_threepids = [] config.admin_contact = None - config.support_user_id = None config.rc_messages_per_second = 10000 config.rc_message_burst_count = 10000 config.use_frozen_dicts = False From cf10ca9c5314a173dc53f5c12f367fa5c08a074d Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 13 Nov 2018 15:55:29 +0000 Subject: [PATCH 22/40] remove errant prints --- synapse/handlers/register.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 9f663ae9b60f..70fd19171703 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -230,12 +230,11 @@ def register( # the room is never created, though this seems unlikely and # recoverable from given the support user being involved in the first # place. - print ("is_support is %r" % is_support) + if (self.hs.config.autocreate_auto_join_rooms and not is_support): count = yield self.store.count_all_users() - print ("count %d" % count) should_auto_create_rooms = count == 1 - print "should_auto_create_rooms is %r" % should_auto_create_rooms + for r in self.hs.config.auto_join_rooms: try: if should_auto_create_rooms: From a54aaf42add9e32f82c1fa8a60323bb350e63362 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 13 Nov 2018 16:15:24 +0000 Subject: [PATCH 23/40] fix boolean typing --- synapse/storage/user_directory.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index ae7ff5ec73e8..a77c9a057928 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -785,7 +785,7 @@ def is_support_user(self, user_id): desc="is_support_user", ) - defer.returnValue(res if res == UserTypes.SUPPORT else False) + defer.returnValue(True if res == UserTypes.SUPPORT else False) def _parse_query_sqlite(search_term): From 22418bc494ec4a413cb37ab004dd60502a679e7e Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 13 Nov 2018 16:47:57 +0000 Subject: [PATCH 24/40] remove unneeded sql --- add_user_type_to_users.sql | 18 ------------------ 1 file changed, 18 deletions(-) delete mode 100644 add_user_type_to_users.sql diff --git a/add_user_type_to_users.sql b/add_user_type_to_users.sql deleted file mode 100644 index eb8d9b19dc78..000000000000 --- a/add_user_type_to_users.sql +++ /dev/null @@ -1,18 +0,0 @@ -/* Copyright 2018 New Vector Ltd - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -- To allow for arbitrary account types in future, initially to be used to denote - -- designated support users - -ALTER TABLE users ADD COLUMN user_type TEXT DEFAULT NULL; From 3121f041c6da62284dd0a7afe84249454169b930 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 13 Nov 2018 17:56:01 +0000 Subject: [PATCH 25/40] wip - move support user logic into handler from storage --- synapse/handlers/user_directory.py | 31 +++++--- synapse/storage/registration.py | 12 +++ synapse/storage/user_directory.py | 86 ++++++--------------- tests/storage/test_registration.py | 20 +++++ tests/storage/test_user_directory.py | 111 +++++++++++---------------- 5 files changed, 120 insertions(+), 140 deletions(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index f11b430126e3..1e233e044fe4 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -125,9 +125,11 @@ def handle_local_profile_change(self, user_id, profile): """ # FIXME(#3714): We should probably do this in the same worker as all # the other changes. - yield self.store.update_profile_in_user_dir( - user_id, profile.display_name, profile.avatar_url, None, - ) + is_support = yield self.store.is_support_user(user_id) + if not is_support: + yield self.store.update_profile_in_user_dir( + user_id, profile.display_name, profile.avatar_url, None, + ) @defer.inlineCallbacks def handle_user_deactivated(self, user_id): @@ -323,18 +325,14 @@ def _handle_deltas(self, deltas): room_id, prev_event_id, event_id, typ, ) elif typ == EventTypes.Member: + change = yield self._get_key_change( prev_event_id, event_id, key_name="membership", public_value=Membership.JOIN, ) - if change is None: - # Handle any profile changes - yield self._handle_profile_change( - state_key, room_id, prev_event_id, event_id, - ) - continue + if not change: # Need to check if the server left the room entirely, if so @@ -349,12 +347,23 @@ def _handle_deltas(self, deltas): # need to remove those users or not user_ids = yield self.store.get_users_in_dir_due_to_room(room_id) for user_id in user_ids: - yield self._handle_remove_user(room_id, user_id) + is_support = yield self.store.is_support_user(state_key) + if not is_support: + yield self._handle_remove_user(room_id, user_id) return else: logger.debug("Server is still in room: %r", room_id) - if change: # The user joined + is_support = yield self.store.is_support_user(state_key) + + if change is None and not is_support: + # Handle any profile changes + yield self._handle_profile_change( + state_key, room_id, prev_event_id, event_id, + ) + continue + + if change and not is_support: # The user joined event = yield self.store.get_event(event_id, allow_none=True) profile = ProfileInfo( avatar_url=event.content.get("avatar_url"), diff --git a/synapse/storage/registration.py b/synapse/storage/registration.py index 9f1dddb5ac1b..4d5ffe375e4e 100644 --- a/synapse/storage/registration.py +++ b/synapse/storage/registration.py @@ -454,6 +454,18 @@ def is_guest(self, user_id): defer.returnValue(res if res else False) + @cachedInlineCallbacks() + def is_support_user(self, user_id): + res = yield self._simple_select_one_onecol( + table="users", + keyvalues={"name": user_id}, + retcol="user_type", + allow_none=True, + desc="is_support_user", + ) + + defer.returnValue(True if res == UserTypes.SUPPORT else False) + @defer.inlineCallbacks def user_add_threepid(self, user_id, medium, address, validated_at, added_at): yield self._simple_upsert("user_threepids", { diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index a77c9a057928..b8f3bfdc9767 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -68,10 +68,6 @@ def add_users_to_public_room(self, room_id, user_ids): or publically joinable user_ids (list(str)): Users to add """ - for u in user_ids: - is_support = yield self.store.is_support_user(u) - if is_support: - user_ids.remove(u) yield self._simple_insert_many( table="users_in_public_rooms", @@ -87,7 +83,7 @@ def add_users_to_public_room(self, room_id, user_ids): for user_id in user_ids: self.get_user_in_public_room.invalidate((user_id,)) - @defer.inlineCallbacks + def add_profiles_to_user_dir(self, room_id, users_with_profile): """Add profiles to the user directory @@ -96,13 +92,6 @@ def add_profiles_to_user_dir(self, room_id, users_with_profile): users_with_profile (dict): Users to add to directory in the form of mapping of user_id -> ProfileInfo """ - user_ids_to_pop = [] - for user_id in iterkeys(users_with_profile): - is_support = yield self.store.is_support_user(user_id) - if is_support: - user_ids_to_pop.append(user_id) - for u in user_ids_to_pop: - users_with_profile.pop(u, None) if isinstance(self.database_engine, PostgresEngine): # We weight the loclpart most highly, then display name and finally @@ -158,27 +147,22 @@ def _add_profiles_to_user_dir_txn(txn): self.get_user_in_directory.invalidate, (user_id,) ) - self.runInteraction( + return self.runInteraction( "add_profiles_to_user_dir", _add_profiles_to_user_dir_txn ) @defer.inlineCallbacks def update_user_in_user_dir(self, user_id, room_id): - is_support = yield self.store.is_support_user(user_id) - if not is_support: - yield self._simple_update_one( - table="user_directory", - keyvalues={"user_id": user_id}, - updatevalues={"room_id": room_id}, - desc="update_user_in_user_dir", - ) - self.get_user_in_directory.invalidate((user_id,)) + yield self._simple_update_one( + table="user_directory", + keyvalues={"user_id": user_id}, + updatevalues={"room_id": room_id}, + desc="update_user_in_user_dir", + ) + self.get_user_in_directory.invalidate((user_id,)) def update_profile_in_user_dir(self, user_id, display_name, avatar_url, room_id): def _update_profile_in_user_dir_txn(txn): - is_support = yield self.store.is_support_user(user_id) - if is_support: - return new_entry = self._simple_upsert_txn( txn, @@ -243,15 +227,13 @@ def _update_profile_in_user_dir_txn(txn): @defer.inlineCallbacks def update_user_in_public_user_list(self, user_id, room_id): - is_support = yield self.store.is_support_user(user_id) - if not is_support: - yield self._simple_update_one( - table="users_in_public_rooms", - keyvalues={"user_id": user_id}, - updatevalues={"room_id": room_id}, - desc="update_user_in_public_user_list", - ) - self.get_user_in_public_room.invalidate((user_id,)) + yield self._simple_update_one( + table="users_in_public_rooms", + keyvalues={"user_id": user_id}, + updatevalues={"room_id": room_id}, + desc="update_user_in_public_user_list", + ) + self.get_user_in_public_room.invalidate((user_id,)) def remove_from_user_dir(self, user_id): def _remove_from_user_dir_txn(txn): @@ -354,7 +336,6 @@ def get_all_local_users(self): rows = yield self._execute("get_all_local_users", None, sql) defer.returnValue([name for name, in rows]) - @defer.inlineCallbacks def add_users_who_share_room(self, room_id, share_private, user_id_tuples): """Insert entries into the users_who_share_rooms table. The first user should be a local user. @@ -364,19 +345,8 @@ def add_users_who_share_room(self, room_id, share_private, user_id_tuples): share_private (bool): Is the room private user_id_tuples([(str, str)]): iterable of 2-tuple of user IDs. """ - # user_id_tuples = - # [u for u in user_id_tuples if not self.store.is_support_user(u)] - user_id_tuples_filtered = [] - for ut in user_id_tuples: - safe = True - for u in ut: - is_support = yield self.store.is_support_user(u) - if is_support: - safe = False - if safe: - user_id_tuples_filtered.append(ut) - - def _add_users_who_share_room_txn(txn, user_id_tuples_filtered): + + def _add_users_who_share_room_txn(txn, user_id_tuples): self._simple_insert_many_txn( txn, @@ -388,10 +358,10 @@ def _add_users_who_share_room_txn(txn, user_id_tuples_filtered): "room_id": room_id, "share_private": share_private, } - for user_id, other_user_id in user_id_tuples_filtered + for user_id, other_user_id in user_id_tuples ], ) - for user_id, other_user_id in user_id_tuples_filtered: + for user_id, other_user_id in user_id_tuples: txn.call_after( self.get_users_who_share_room_from_dir.invalidate, (user_id,), @@ -400,10 +370,10 @@ def _add_users_who_share_room_txn(txn, user_id_tuples_filtered): self.get_if_users_share_a_room.invalidate, (user_id, other_user_id), ) - self.runInteraction( + return self.runInteraction( "add_users_who_share_room", _add_users_who_share_room_txn, - user_id_tuples_filtered, + user_id_tuples, ) def update_users_who_share_room(self, room_id, share_private, user_id_sets): @@ -775,18 +745,6 @@ def search_user_dir(self, user_id, search_term, limit): "results": results, }) - @cachedInlineCallbacks() - def is_support_user(self, user_id): - res = yield self._simple_select_one_onecol( - table="users", - keyvalues={"name": user_id}, - retcol="user_type", - allow_none=True, - desc="is_support_user", - ) - - defer.returnValue(True if res == UserTypes.SUPPORT else False) - def _parse_query_sqlite(search_term): """Takes a plain unicode string from the user and converts it into a form diff --git a/tests/storage/test_registration.py b/tests/storage/test_registration.py index 3dfb7b903a3a..ec2032ea634c 100644 --- a/tests/storage/test_registration.py +++ b/tests/storage/test_registration.py @@ -99,6 +99,26 @@ def test_user_delete_access_tokens(self): user = yield self.store.get_user_by_access_token(self.tokens[0]) self.assertIsNone(user, "access token was not deleted without device_id") + @defer.inlineCallbacks + def test_is_support_user(self): + TEST_USER = "@test:test" + SUPPORT_USER = "@support:test" + + res = yield self.store.is_support_user(None) + self.assertFalse(res) + yield self.datastore.register(user_id=TEST_USER, token="123", password_hash=None) + res = yield self.store.is_support_user(TEST_USER) + self.assertFalse(res) + + self.datastore.register( + user_id=SUPPORT_USER, + token="456", + password_hash=None, + user_type=UserTypes.SUPPORT + ) + res = yield self.store.is_support_user(SUPPORT_USER) + self.assertTrue(res) + class TokenGenerator: def __init__(self): diff --git a/tests/storage/test_user_directory.py b/tests/storage/test_user_directory.py index 6cf6d4a48a6b..04cfa8c6911e 100644 --- a/tests/storage/test_user_directory.py +++ b/tests/storage/test_user_directory.py @@ -32,8 +32,8 @@ class UserDirectoryStoreTestCase(unittest.TestCase): @defer.inlineCallbacks def setUp(self): self.hs = yield setup_test_homeserver(self.addCleanup) - self.datastore = self.hs.get_datastore() - self.store = UserDirectoryStore(self.hs.get_db_conn(), self.hs) + self.store = self.hs.get_datastore() + # self.store = UserDirectoryStore(self.hs.get_db_conn(), self.hs) # alice and bob are both in !room_id. bobby is not but shares # a homeserver with alice. @@ -79,66 +79,47 @@ def test_search_user_dir_all_users(self): finally: self.hs.config.user_directory_search_all_users = False - @defer.inlineCallbacks - def test_cannot_add_support_user_to_directory(self): - self.hs.config.user_directory_search_all_users = True - SUPPORT_USER = "@support:test" - SUPPOER_USER_SCREEN_NAME = "Support" - - yield self.datastore.register(user_id=SUPPORT_USER, token="123", - password_hash=None, user_type=UserTypes.SUPPORT) - yield self.datastore.register(user_id=ALICE, token="456", password_hash=None) - - yield self.store.add_profiles_to_user_dir( - ROOM, - {SUPPORT_USER: ProfileInfo(None, SUPPOER_USER_SCREEN_NAME)}, - ) - yield self.store.add_users_to_public_room(ROOM, [SUPPORT_USER]) - yield self.store.add_users_who_share_room( - ROOM, False, ((ALICE, SUPPORT_USER),) - ) - - r = yield self.store.search_user_dir(ALICE, SUPPOER_USER_SCREEN_NAME, 10) - self.assertFalse(r["limited"]) - self.assertEqual(0, len(r["results"])) - - # Check that enabled support user does not prevent all users being added - r = yield self.store.search_user_dir(ALICE, ALICE, 10) - self.assertFalse(r["limited"]) - self.assertEqual(1, len(r["results"])) - - yield self.store.update_user_in_user_dir(SUPPORT_USER, ROOM) - yield self.store.update_profile_in_user_dir( - SUPPORT_USER, SUPPOER_USER_SCREEN_NAME, None, ROOM - ) - yield self.store.update_user_in_public_user_list(SUPPORT_USER, ROOM) - - r = yield self.store.search_user_dir(ALICE, SUPPOER_USER_SCREEN_NAME, 10) - self.assertFalse(r["limited"]) - self.assertEqual(0, len(r["results"])) - - r = yield self.store.get_user_in_directory(SUPPORT_USER) - self.assertEqual(r, None) - - r = yield self.store.get_user_in_public_room(SUPPORT_USER) - self.assertEqual(r, None) - - @defer.inlineCallbacks - def test_is_support_user(self): - TEST_USER = "@test:test" - SUPPORT_USER = "@support:test" - - res = yield self.store.is_support_user(None) - self.assertFalse(res) - yield self.datastore.register(user_id=TEST_USER, token="123", password_hash=None) - res = yield self.store.is_support_user(TEST_USER) - self.assertFalse(res) - - self.datastore.register( - user_id=SUPPORT_USER, - token="456", - password_hash=None, - user_type=UserTypes.SUPPORT - ) - res = yield self.store.is_support_user(SUPPORT_USER) - self.assertTrue(res) + # @defer.inlineCallbacks + # def test_cannot_add_support_user_to_directory(self): + # self.hs.config.user_directory_search_all_users = True + # SUPPORT_USER = "@support:test" + # SUPPOER_USER_SCREEN_NAME = "Support" + # + # yield self.store.register(user_id=SUPPORT_USER, token="123", + # password_hash=None, + # user_type=UserTypes.SUPPORT) + # yield self.store.register(user_id=ALICE, token="456", password_hash=None) + # + # yield self.store.add_profiles_to_user_dir( + # ROOM, + # {SUPPORT_USER: ProfileInfo(None, SUPPOER_USER_SCREEN_NAME)}, + # ) + # yield self.store.add_users_to_public_room(ROOM, [SUPPORT_USER]) + # yield self.store.add_users_who_share_room( + # ROOM, False, ((ALICE, SUPPORT_USER),) + # ) + # + # r = yield self.store.search_user_dir(ALICE, SUPPOER_USER_SCREEN_NAME, 10) + # self.assertFalse(r["limited"]) + # self.assertEqual(0, len(r["results"])) + # + # # Check that enabled support user does not prevent all users being added + # r = yield self.store.search_user_dir(ALICE, ALICE, 10) + # self.assertFalse(r["limited"]) + # self.assertEqual(1, len(r["results"])) + # + # yield self.store.update_user_in_user_dir(SUPPORT_USER, ROOM) + # yield self.store.update_profile_in_user_dir( + # SUPPORT_USER, SUPPOER_USER_SCREEN_NAME, None, ROOM + # ) + # yield self.store.update_user_in_public_user_list(SUPPORT_USER, ROOM) + # + # r = yield self.store.search_user_dir(ALICE, SUPPOER_USER_SCREEN_NAME, 10) + # self.assertFalse(r["limited"]) + # self.assertEqual(0, len(r["results"])) + # + # r = yield self.store.get_user_in_directory(SUPPORT_USER) + # self.assertEqual(r, None) + # + # r = yield self.store.get_user_in_public_room(SUPPORT_USER) + # self.assertEqual(r, None) From c7082966f3cedc697216987318c0213a72971eff Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 13 Nov 2018 22:07:58 +0000 Subject: [PATCH 26/40] fix unit tests --- synapse/storage/registration.py | 1 + tests/storage/test_registration.py | 5 +++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/synapse/storage/registration.py b/synapse/storage/registration.py index 4d5ffe375e4e..4af32ece1368 100644 --- a/synapse/storage/registration.py +++ b/synapse/storage/registration.py @@ -19,6 +19,7 @@ from twisted.internet import defer +from synapse.api.constants import UserTypes from synapse.api.errors import Codes, StoreError from synapse.storage import background_updates from synapse.storage._base import SQLBaseStore diff --git a/tests/storage/test_registration.py b/tests/storage/test_registration.py index ec2032ea634c..c16b8b6ea427 100644 --- a/tests/storage/test_registration.py +++ b/tests/storage/test_registration.py @@ -18,6 +18,7 @@ from tests import unittest from tests.utils import setup_test_homeserver +from synapse.api.constants import UserTypes class RegistrationStoreTestCase(unittest.TestCase): @@ -106,11 +107,11 @@ def test_is_support_user(self): res = yield self.store.is_support_user(None) self.assertFalse(res) - yield self.datastore.register(user_id=TEST_USER, token="123", password_hash=None) + yield self.store.register(user_id=TEST_USER, token="123", password_hash=None) res = yield self.store.is_support_user(TEST_USER) self.assertFalse(res) - self.datastore.register( + self.store.register( user_id=SUPPORT_USER, token="456", password_hash=None, From b01271a12be164d5f8286aa3c3106a32c6ac6be3 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 13 Nov 2018 23:12:58 +0000 Subject: [PATCH 27/40] tweak tests and tidy --- synapse/handlers/register.py | 2 -- synapse/handlers/user_directory.py | 2 -- synapse/storage/user_directory.py | 10 ++------ tests/handlers/test_register.py | 34 ++++++++++++---------------- tests/storage/test_registration.py | 3 ++- tests/storage/test_user_directory.py | 3 --- 6 files changed, 18 insertions(+), 36 deletions(-) diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 70fd19171703..ceab38f18c35 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -230,11 +230,9 @@ def register( # the room is never created, though this seems unlikely and # recoverable from given the support user being involved in the first # place. - if (self.hs.config.autocreate_auto_join_rooms and not is_support): count = yield self.store.count_all_users() should_auto_create_rooms = count == 1 - for r in self.hs.config.auto_join_rooms: try: if should_auto_create_rooms: diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index 1e233e044fe4..821058cfcb99 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -332,8 +332,6 @@ def _handle_deltas(self, deltas): public_value=Membership.JOIN, ) - - if not change: # Need to check if the server left the room entirely, if so # we might need to remove all the users in that room diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index b8f3bfdc9767..4fbfe8d90cd2 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -16,11 +16,11 @@ import logging import re -from six import iteritems, iterkeys +from six import iteritems from twisted.internet import defer -from synapse.api.constants import EventTypes, JoinRules, UserTypes +from synapse.api.constants import EventTypes, JoinRules from synapse.storage.engines import PostgresEngine, Sqlite3Engine from synapse.types import get_domain_from_id, get_localpart_from_id from synapse.util.caches.descriptors import cached, cachedInlineCallbacks @@ -31,9 +31,6 @@ class UserDirectoryStore(SQLBaseStore): - def __init__(self, dbconn, hs): - super(UserDirectoryStore, self).__init__(dbconn, hs) - self.store = hs.get_datastore() @cachedInlineCallbacks(cache_context=True) def is_room_world_readable_or_publicly_joinable(self, room_id, cache_context): @@ -68,7 +65,6 @@ def add_users_to_public_room(self, room_id, user_ids): or publically joinable user_ids (list(str)): Users to add """ - yield self._simple_insert_many( table="users_in_public_rooms", values=[ @@ -83,7 +79,6 @@ def add_users_to_public_room(self, room_id, user_ids): for user_id in user_ids: self.get_user_in_public_room.invalidate((user_id,)) - def add_profiles_to_user_dir(self, room_id, users_with_profile): """Add profiles to the user directory @@ -92,7 +87,6 @@ def add_profiles_to_user_dir(self, room_id, users_with_profile): users_with_profile (dict): Users to add to directory in the form of mapping of user_id -> ProfileInfo """ - if isinstance(self.database_engine, PostgresEngine): # We weight the loclpart most highly, then display name and finally # server name diff --git a/tests/handlers/test_register.py b/tests/handlers/test_register.py index d0512f599cab..dc297c030b3f 100644 --- a/tests/handlers/test_register.py +++ b/tests/handlers/test_register.py @@ -17,7 +17,7 @@ from twisted.internet import defer -from synapse.api.errors import ResourceLimitError +from synapse.api.errors import ResourceLimitError, SynapseError from synapse.handlers.register import RegistrationHandler from synapse.types import RoomAlias, UserID, create_requester @@ -181,22 +181,16 @@ def test_auto_create_auto_join_where_auto_create_is_false(self): rooms = yield self.store.get_rooms_for_user(res[0]) self.assertEqual(len(rooms), 0) - # @defer.inlineCallbacks - # def test_auto_create_auto_join_rooms_when_support_user_exists(self): - # room_alias_str = "#room:test" - # self.hs.config.auto_join_rooms = [room_alias_str] - # - # res_support = yield self.handler.register(localpart='support', - # user_type=UserTypes.SUPPORT) - # rooms = yield self.store.get_rooms_for_user(res_support[0]) - # self.assertTrue(len(rooms) == 0) - # - # res = yield self.handler.register(localpart='jeff') - # - # rooms = yield self.store.get_rooms_for_user(res[0]) - # directory_handler = self.hs.get_handlers().directory_handler - # room_alias = RoomAlias.from_string(room_alias_str) - # room_id = yield directory_handler.get_association(room_alias) - # - # self.assertTrue(room_id['room_id'] in rooms) - # self.assertEqual(len(rooms), 1) + @defer.inlineCallbacks + def test_auto_create_auto_join_rooms_when_support_user_exists(self): + room_alias_str = "#room:test" + self.hs.config.auto_join_rooms = [room_alias_str] + + self.store.is_support_user = Mock(return_value=True) + res = yield self.handler.register(localpart='support') + rooms = yield self.store.get_rooms_for_user(res[0]) + self.assertEqual(len(rooms), 0) + directory_handler = self.hs.get_handlers().directory_handler + room_alias = RoomAlias.from_string(room_alias_str) + with self.assertRaises(SynapseError): + yield directory_handler.get_association(room_alias) diff --git a/tests/storage/test_registration.py b/tests/storage/test_registration.py index c16b8b6ea427..421713cb32a2 100644 --- a/tests/storage/test_registration.py +++ b/tests/storage/test_registration.py @@ -16,9 +16,10 @@ from twisted.internet import defer +from synapse.api.constants import UserTypes + from tests import unittest from tests.utils import setup_test_homeserver -from synapse.api.constants import UserTypes class RegistrationStoreTestCase(unittest.TestCase): diff --git a/tests/storage/test_user_directory.py b/tests/storage/test_user_directory.py index 04cfa8c6911e..eff62e42e497 100644 --- a/tests/storage/test_user_directory.py +++ b/tests/storage/test_user_directory.py @@ -15,8 +15,6 @@ from twisted.internet import defer -from synapse.api.constants import UserTypes -from synapse.storage import UserDirectoryStore from synapse.storage.roommember import ProfileInfo from tests import unittest @@ -33,7 +31,6 @@ class UserDirectoryStoreTestCase(unittest.TestCase): def setUp(self): self.hs = yield setup_test_homeserver(self.addCleanup) self.store = self.hs.get_datastore() - # self.store = UserDirectoryStore(self.hs.get_db_conn(), self.hs) # alice and bob are both in !room_id. bobby is not but shares # a homeserver with alice. From b58bf8d85010e402e8f78e4323bf7f955605966a Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Wed, 14 Nov 2018 14:21:41 +0000 Subject: [PATCH 28/40] tests for support user behaviour --- changelog.d/4141.feature | 2 +- synapse/handlers/user_directory.py | 38 ++++++++++++++++-------------- 2 files changed, 21 insertions(+), 19 deletions(-) diff --git a/changelog.d/4141.feature b/changelog.d/4141.feature index 7df8118d3098..34ec4115a703 100644 --- a/changelog.d/4141.feature +++ b/changelog.d/4141.feature @@ -1 +1 @@ -Optionally configure a support user via config for use in verifying user behaviour of a given server, the support user does not appear in user directory or monthly active user counts. +Special case a support user for use in verifying user behaviour of a given server, the support user does not appear in user directory or monthly active user counts. diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index 821058cfcb99..a1f5356268f2 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -137,8 +137,10 @@ def handle_user_deactivated(self, user_id): """ # FIXME(#3714): We should probably do this in the same worker as all # the other changes. - yield self.store.remove_from_user_dir(user_id) - yield self.store.remove_from_user_in_public_room(user_id) + is_support = yield self.store.is_support_user(user_id) + if not is_support: + yield self.store.remove_from_user_dir(user_id) + yield self.store.remove_from_user_in_public_room(user_id) @defer.inlineCallbacks def _unsafe_process(self): @@ -353,24 +355,24 @@ def _handle_deltas(self, deltas): logger.debug("Server is still in room: %r", room_id) is_support = yield self.store.is_support_user(state_key) + if not is_support: + if change is None: + # Handle any profile changes + yield self._handle_profile_change( + state_key, room_id, prev_event_id, event_id, + ) + continue - if change is None and not is_support: - # Handle any profile changes - yield self._handle_profile_change( - state_key, room_id, prev_event_id, event_id, - ) - continue - - if change and not is_support: # The user joined - event = yield self.store.get_event(event_id, allow_none=True) - profile = ProfileInfo( - avatar_url=event.content.get("avatar_url"), - display_name=event.content.get("displayname"), - ) + if change: # The user joined + event = yield self.store.get_event(event_id, allow_none=True) + profile = ProfileInfo( + avatar_url=event.content.get("avatar_url"), + display_name=event.content.get("displayname"), + ) - yield self._handle_new_user(room_id, state_key, profile) - else: # The user left - yield self._handle_remove_user(room_id, state_key) + yield self._handle_new_user(room_id, state_key, profile) + else: # The user left + yield self._handle_remove_user(room_id, state_key) else: logger.debug("Ignoring irrelevant type: %r", typ) From 3f5fe16955fad2d5e41561bbcf2290308a3c7edf Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Wed, 14 Nov 2018 14:31:51 +0000 Subject: [PATCH 29/40] test support user behaviour --- tests/handlers/test_user_directory.py | 91 +++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) create mode 100644 tests/handlers/test_user_directory.py diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py new file mode 100644 index 000000000000..22b602abefcd --- /dev/null +++ b/tests/handlers/test_user_directory.py @@ -0,0 +1,91 @@ +# -*- coding: utf-8 -*- +# Copyright 2018 New Vector +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +from unittest.mock import Mock + +from twisted.internet import defer + +from synapse.api.constants import UserTypes +from synapse.handlers.user_directory import UserDirectoryHandler +from synapse.storage.roommember import ProfileInfo + +from tests import unittest +from tests.utils import setup_test_homeserver + + +class UserDirectoryHandlers(object): + def __init__(self, hs): + self.user_directory_handler = UserDirectoryHandler(hs) + + +class UserDirectoryTestCase(unittest.TestCase): + """ Tests the UserDirectoryHandler. """ + + @defer.inlineCallbacks + def setUp(self): + hs = yield setup_test_homeserver(self.addCleanup) + self.store = hs.get_datastore() + hs.handlers = UserDirectoryHandlers(hs) + + self.handler = hs.get_handlers().user_directory_handler + + @defer.inlineCallbacks + def test_handle_local_profile_change_with_support_user(self): + support_user_id = "@support:test" + self.store.register( + user_id=support_user_id, + token="123", + password_hash=None, + user_type=UserTypes.SUPPORT + ) + + yield self.handler.handle_local_profile_change(support_user_id, None) + profile = yield self.store.get_user_in_directory(support_user_id) + self.assertTrue(profile is None) + display_name = 'display_name' + + profile_info = ProfileInfo( + avatar_url='avatar_url', + display_name=display_name, + ) + regular_user_id = '@regular:test' + yield self.handler.handle_local_profile_change(regular_user_id, profile_info) + profile = yield self.store.get_user_in_directory(regular_user_id) + self.assertTrue(profile['display_name'] == display_name) + + @defer.inlineCallbacks + def test_handle_user_deactivated_support_user(self): + s_user_id = "@support:test" + self.store.register( + user_id=s_user_id, + token="123", + password_hash=None, + user_type=UserTypes.SUPPORT + ) + + self.store.remove_from_user_dir = Mock() + self.store.remove_from_user_in_public_room = Mock() + yield self.handler.handle_user_deactivated(s_user_id) + self.store.remove_from_user_dir.not_called() + self.store.remove_from_user_in_public_room.not_called() + + @defer.inlineCallbacks + def test_handle_user_deactivated_regular_user(self): + r_user_id = "@regular:test" + self.store.register(user_id=r_user_id, token="123", password_hash=None) + self.store.remove_from_user_dir = Mock() + self.store.remove_from_user_in_public_room = Mock() + yield self.handler.handle_user_deactivated(r_user_id) + self.store.remove_from_user_dir.called_once_with(r_user_id) + self.store.remove_from_user_in_public_room.assert_called_once_with(r_user_id) From ce27b1c9c1b984110da1af258ea22a409eea2aed Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Wed, 14 Nov 2018 14:32:07 +0000 Subject: [PATCH 30/40] clean up --- synapse/config/server.py | 1 + synapse/storage/user_directory.py | 12 ++----- tests/storage/test_user_directory.py | 51 ++-------------------------- 3 files changed, 7 insertions(+), 57 deletions(-) diff --git a/synapse/config/server.py b/synapse/config/server.py index c4d660bb61c2..c1c7c0105e41 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -13,6 +13,7 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. + import logging from synapse.http.endpoint import parse_and_validate_server_name diff --git a/synapse/storage/user_directory.py b/synapse/storage/user_directory.py index 4fbfe8d90cd2..a8781b0e5d5b 100644 --- a/synapse/storage/user_directory.py +++ b/synapse/storage/user_directory.py @@ -31,7 +31,6 @@ class UserDirectoryStore(SQLBaseStore): - @cachedInlineCallbacks(cache_context=True) def is_room_world_readable_or_publicly_joinable(self, room_id, cache_context): """Check if the room is either world_readable or publically joinable @@ -157,7 +156,6 @@ def update_user_in_user_dir(self, user_id, room_id): def update_profile_in_user_dir(self, user_id, display_name, avatar_url, room_id): def _update_profile_in_user_dir_txn(txn): - new_entry = self._simple_upsert_txn( txn, table="user_directory", @@ -215,6 +213,7 @@ def _update_profile_in_user_dir_txn(txn): raise Exception("Unrecognized database engine") txn.call_after(self.get_user_in_directory.invalidate, (user_id,)) + return self.runInteraction( "update_profile_in_user_dir", _update_profile_in_user_dir_txn ) @@ -339,9 +338,7 @@ def add_users_who_share_room(self, room_id, share_private, user_id_tuples): share_private (bool): Is the room private user_id_tuples([(str, str)]): iterable of 2-tuple of user IDs. """ - - def _add_users_who_share_room_txn(txn, user_id_tuples): - + def _add_users_who_share_room_txn(txn): self._simple_insert_many_txn( txn, table="users_who_share_rooms", @@ -365,9 +362,7 @@ def _add_users_who_share_room_txn(txn, user_id_tuples): (user_id, other_user_id), ) return self.runInteraction( - "add_users_who_share_room", - _add_users_who_share_room_txn, - user_id_tuples, + "add_users_who_share_room", _add_users_who_share_room_txn ) def update_users_who_share_room(self, room_id, share_private, user_id_sets): @@ -380,7 +375,6 @@ def update_users_who_share_room(self, room_id, share_private, user_id_sets): user_id_tuples([(str, str)]): iterable of 2-tuple of user IDs. """ def _update_users_who_share_room_txn(txn): - sql = """ UPDATE users_who_share_rooms SET room_id = ?, share_private = ? diff --git a/tests/storage/test_user_directory.py b/tests/storage/test_user_directory.py index eff62e42e497..0dde1ab2fea3 100644 --- a/tests/storage/test_user_directory.py +++ b/tests/storage/test_user_directory.py @@ -15,6 +15,7 @@ from twisted.internet import defer +from synapse.storage import UserDirectoryStore from synapse.storage.roommember import ProfileInfo from tests import unittest @@ -23,14 +24,13 @@ ALICE = "@alice:a" BOB = "@bob:b" BOBBY = "@bobby:a" -ROOM = "!room:id" class UserDirectoryStoreTestCase(unittest.TestCase): @defer.inlineCallbacks def setUp(self): self.hs = yield setup_test_homeserver(self.addCleanup) - self.store = self.hs.get_datastore() + self.store = UserDirectoryStore(self.hs.get_db_conn(), self.hs) # alice and bob are both in !room_id. bobby is not but shares # a homeserver with alice. @@ -42,7 +42,7 @@ def setUp(self): BOBBY: ProfileInfo(None, "bobby"), }, ) - yield self.store.add_users_to_public_room(ROOM, [ALICE, BOB]) + yield self.store.add_users_to_public_room("!room:id", [ALICE, BOB]) yield self.store.add_users_who_share_room( "!room:id", False, ((ALICE, BOB), (BOB, ALICE)) ) @@ -75,48 +75,3 @@ def test_search_user_dir_all_users(self): ) finally: self.hs.config.user_directory_search_all_users = False - - # @defer.inlineCallbacks - # def test_cannot_add_support_user_to_directory(self): - # self.hs.config.user_directory_search_all_users = True - # SUPPORT_USER = "@support:test" - # SUPPOER_USER_SCREEN_NAME = "Support" - # - # yield self.store.register(user_id=SUPPORT_USER, token="123", - # password_hash=None, - # user_type=UserTypes.SUPPORT) - # yield self.store.register(user_id=ALICE, token="456", password_hash=None) - # - # yield self.store.add_profiles_to_user_dir( - # ROOM, - # {SUPPORT_USER: ProfileInfo(None, SUPPOER_USER_SCREEN_NAME)}, - # ) - # yield self.store.add_users_to_public_room(ROOM, [SUPPORT_USER]) - # yield self.store.add_users_who_share_room( - # ROOM, False, ((ALICE, SUPPORT_USER),) - # ) - # - # r = yield self.store.search_user_dir(ALICE, SUPPOER_USER_SCREEN_NAME, 10) - # self.assertFalse(r["limited"]) - # self.assertEqual(0, len(r["results"])) - # - # # Check that enabled support user does not prevent all users being added - # r = yield self.store.search_user_dir(ALICE, ALICE, 10) - # self.assertFalse(r["limited"]) - # self.assertEqual(1, len(r["results"])) - # - # yield self.store.update_user_in_user_dir(SUPPORT_USER, ROOM) - # yield self.store.update_profile_in_user_dir( - # SUPPORT_USER, SUPPOER_USER_SCREEN_NAME, None, ROOM - # ) - # yield self.store.update_user_in_public_user_list(SUPPORT_USER, ROOM) - # - # r = yield self.store.search_user_dir(ALICE, SUPPOER_USER_SCREEN_NAME, 10) - # self.assertFalse(r["limited"]) - # self.assertEqual(0, len(r["results"])) - # - # r = yield self.store.get_user_in_directory(SUPPORT_USER) - # self.assertEqual(r, None) - # - # r = yield self.store.get_user_in_public_room(SUPPORT_USER) - # self.assertEqual(r, None) From eab8843c7095310a6245a37483728cf030256e74 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Wed, 14 Nov 2018 14:51:38 +0000 Subject: [PATCH 31/40] improve docstring --- synapse/handlers/user_directory.py | 1 - synapse/storage/registration.py | 2 ++ 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index a1f5356268f2..fbfaea8aeb71 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -327,7 +327,6 @@ def _handle_deltas(self, deltas): room_id, prev_event_id, event_id, typ, ) elif typ == EventTypes.Member: - change = yield self._get_key_change( prev_event_id, event_id, key_name="membership", diff --git a/synapse/storage/registration.py b/synapse/storage/registration.py index 4af32ece1368..2c6a417c79ad 100644 --- a/synapse/storage/registration.py +++ b/synapse/storage/registration.py @@ -184,6 +184,8 @@ def register(self, user_id, token=None, password_hash=None, appservice_id (str): The ID of the appservice registering the user. create_profile_with_localpart (str): Optionally create a profile for the given localpart. + admin (boolean): is an admin user? + user_type (synapse.api.constants import UserTypes): type of user Raises: StoreError if the user_id could not be registered. """ From ded57746373a90203cd171ab3842a7ef8e5418a2 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Wed, 14 Nov 2018 15:04:28 +0000 Subject: [PATCH 32/40] fix py2 Mock dep --- tests/handlers/test_user_directory.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index 22b602abefcd..4448d15642ef 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -12,7 +12,7 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -from unittest.mock import Mock +from mock import Mock from twisted.internet import defer From 255515babfffa82adae54c8b5559ff64df12bd6b Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 27 Nov 2018 14:47:16 +0000 Subject: [PATCH 33/40] isort --- tests/storage/test_monthly_active_users.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/storage/test_monthly_active_users.py b/tests/storage/test_monthly_active_users.py index 490e33573405..cbae424404a7 100644 --- a/tests/storage/test_monthly_active_users.py +++ b/tests/storage/test_monthly_active_users.py @@ -266,4 +266,3 @@ def test_no_users_when_not_tracking(self): self.pump() self.store.upsert_monthly_active_user.assert_not_called() - From 40771fff82f74798ebea023ac32731603f9815d1 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 27 Nov 2018 15:09:47 +0000 Subject: [PATCH 34/40] remove line --- tests/storage/test_monthly_active_users.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/storage/test_monthly_active_users.py b/tests/storage/test_monthly_active_users.py index cbae424404a7..38fadf32bd4a 100644 --- a/tests/storage/test_monthly_active_users.py +++ b/tests/storage/test_monthly_active_users.py @@ -224,7 +224,6 @@ def test_get_reserved_real_user_account(self): count = self.store.get_registered_reserved_users_count() self.assertEquals(self.get_success(count), len(threepids)) - def test_support_user_not_add_to_mau_limits(self): count = self.store.get_monthly_active_count() self.pump() From 6ae725a9d8e2f25f2daa3d10b34733dbd6b8e599 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 27 Nov 2018 15:34:26 +0000 Subject: [PATCH 35/40] fix race condition --- tests/handlers/test_user_directory.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index 4448d15642ef..11f2bae69808 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -43,7 +43,7 @@ def setUp(self): @defer.inlineCallbacks def test_handle_local_profile_change_with_support_user(self): support_user_id = "@support:test" - self.store.register( + yield self.store.register( user_id=support_user_id, token="123", password_hash=None, From b620f0ad14868cfae57c23957340590fe5400fd1 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Wed, 28 Nov 2018 11:49:42 +0000 Subject: [PATCH 36/40] fix reference to unused config.support_user_id --- synapse/storage/monthly_active_users.py | 16 +++++++++++++--- tests/storage/test_monthly_active_users.py | 3 ++- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/synapse/storage/monthly_active_users.py b/synapse/storage/monthly_active_users.py index 6e162a5fcdbc..e08c111253e8 100644 --- a/synapse/storage/monthly_active_users.py +++ b/synapse/storage/monthly_active_users.py @@ -182,6 +182,18 @@ def upsert_monthly_active_user(self, user_id): Args: user_id (str): user to add/update """ + # Support user never to be included in MAU stats. Note I can't easily call this + # from upsert_monthly_active_user_txn because then I need a _txn form of + # is_support_user which is complicated because I want to cache the result. + # Therefore I call it here and ignore the case where + # upsert_monthly_active_user_txn is called directly from + # _initialise_reserved_users reasoning that it would be very strange to + # include a support user in this context. + + is_support = yield self.is_support_user(user_id) + if is_support: + return + is_insert = yield self.runInteraction( "upsert_monthly_active_user", self.upsert_monthly_active_user_txn, user_id @@ -208,9 +220,7 @@ def upsert_monthly_active_user_txn(self, txn, user_id): bool: True if a new entry was created, False if an existing one was updated. """ - # Support user never to be included in MAU stats - if user_id == self.hs.config.support_user_id: - return + # Am consciously deciding to lock the table on the basis that is ought # never be a big table and alternative approaches (batching multiple # upserts into a single txn) introduced a lot of extra complexity. diff --git a/tests/storage/test_monthly_active_users.py b/tests/storage/test_monthly_active_users.py index 38fadf32bd4a..a1a02ab61215 100644 --- a/tests/storage/test_monthly_active_users.py +++ b/tests/storage/test_monthly_active_users.py @@ -225,10 +225,11 @@ def test_get_reserved_real_user_account(self): self.assertEquals(self.get_success(count), len(threepids)) def test_support_user_not_add_to_mau_limits(self): + support_user_id = "@support:test" count = self.store.get_monthly_active_count() self.pump() self.assertEqual(self.get_success(count), 0) - support_user_id = '@support:test' + self.store.register( user_id=support_user_id, token="123", From 6574fdd3149163c8a91eab7ee356d4f2f6579a6a Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Wed, 28 Nov 2018 12:05:20 +0000 Subject: [PATCH 37/40] add in missing @defer.inlineCallbacks to test_auto_create_auto_join_where_no_consent --- tests/handlers/test_register.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/handlers/test_register.py b/tests/handlers/test_register.py index 82f24ab1bcef..84d89b3c54fe 100644 --- a/tests/handlers/test_register.py +++ b/tests/handlers/test_register.py @@ -194,6 +194,7 @@ def test_auto_create_auto_join_rooms_when_support_user_exists(self): with self.assertRaises(SynapseError): yield directory_handler.get_association(room_alias) + @defer.inlineCallbacks def test_auto_create_auto_join_where_no_consent(self): self.hs.config.user_consent_at_registration = True self.hs.config.block_events_without_consent_error = "Error" From 66c4fec6130375ee5cc0da072f2e598949de78f6 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Wed, 28 Nov 2018 12:21:18 +0000 Subject: [PATCH 38/40] Fix synapse_admin_mau:current metric is not updating when config.mau_stats_only is True --- synapse/app/homeserver.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/app/homeserver.py b/synapse/app/homeserver.py index 3e4dea2f196a..15370587792e 100755 --- a/synapse/app/homeserver.py +++ b/synapse/app/homeserver.py @@ -549,7 +549,7 @@ def start_generate_monthly_active_users(): ) start_generate_monthly_active_users() - if hs.config.limit_usage_by_mau: + if hs.config.limit_usage_by_mau or or hs.config.mau_stats_only: clock.looping_call(start_generate_monthly_active_users, 5 * 60 * 1000) # End of monthly active user settings From aab59d6fff8cb439eef18e3aff6718092c874d82 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Wed, 28 Nov 2018 12:30:25 +0000 Subject: [PATCH 39/40] towncrier --- changelog.d/4233.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/4233.bugfix diff --git a/changelog.d/4233.bugfix b/changelog.d/4233.bugfix new file mode 100644 index 000000000000..eb567cc03eaf --- /dev/null +++ b/changelog.d/4233.bugfix @@ -0,0 +1 @@ +Fix synapse_admin_mau:current metric is not updating when config.mau_stats_only is True From 0a6a2a81a3d5121b5e2e4cac0c8f85b5d18ad8b1 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Wed, 28 Nov 2018 12:35:20 +0000 Subject: [PATCH 40/40] typo --- synapse/app/homeserver.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/app/homeserver.py b/synapse/app/homeserver.py index 15370587792e..ed6f84f4f4db 100755 --- a/synapse/app/homeserver.py +++ b/synapse/app/homeserver.py @@ -549,7 +549,7 @@ def start_generate_monthly_active_users(): ) start_generate_monthly_active_users() - if hs.config.limit_usage_by_mau or or hs.config.mau_stats_only: + if hs.config.limit_usage_by_mau or hs.config.mau_stats_only: clock.looping_call(start_generate_monthly_active_users, 5 * 60 * 1000) # End of monthly active user settings