From e92fb00f32c63de6ea50ba1cbbadf74060ea143d Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Wed, 8 Aug 2018 17:54:49 +0100 Subject: [PATCH 01/17] sync auth blocking --- synapse/handlers/sync.py | 16 +++++++++----- tests/handlers/test_sync.py | 42 +++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 5 deletions(-) create mode 100644 tests/handlers/test_sync.py diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py index dff1f67dc..f748d9afb 100644 --- a/synapse/handlers/sync.py +++ b/synapse/handlers/sync.py @@ -191,6 +191,7 @@ class SyncHandler(object): self.clock = hs.get_clock() self.response_cache = ResponseCache(hs, "sync") self.state = hs.get_state_handler() + self.auth = hs.get_auth() # ExpiringCache((User, Device)) -> LruCache(state_key => event_id) self.lazy_loaded_members_cache = ExpiringCache( @@ -198,18 +199,23 @@ class SyncHandler(object): max_len=0, expiry_ms=LAZY_LOADED_MEMBERS_CACHE_MAX_AGE, ) + @defer.inlineCallbacks def wait_for_sync_for_user(self, sync_config, since_token=None, timeout=0, full_state=False): """Get the sync for a client if we have new data for it now. Otherwise wait for new data to arrive on the server. If the timeout expires, then return an empty sync result. Returns: - A Deferred SyncResult. + Deferred[SyncResult] """ - return self.response_cache.wrap( - sync_config.request_key, - self._wait_for_sync_for_user, - sync_config, since_token, timeout, full_state, + yield self.auth.check_auth_blocking() + + defer.returnValue( + self.response_cache.wrap( + sync_config.request_key, + self._wait_for_sync_for_user, + sync_config, since_token, timeout, full_state, + ) ) @defer.inlineCallbacks diff --git a/tests/handlers/test_sync.py b/tests/handlers/test_sync.py new file mode 100644 index 000000000..3b1b4d492 --- /dev/null +++ b/tests/handlers/test_sync.py @@ -0,0 +1,42 @@ +# -*- coding: utf-8 -*- +# 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. +from twisted.internet import defer + +import tests.unittest +import tests.utils +from tests.utils import setup_test_homeserver +from synapse.handlers.sync import SyncHandler, SyncConfig +from synapse.types import UserID + + +class SyncTestCase(tests.unittest.TestCase): + """ Tests Sync Handler. """ + + @defer.inlineCallbacks + def setUp(self): + self.hs = yield setup_test_homeserver() + self.sync_handler = SyncHandler(self.hs) + + @defer.inlineCallbacks + def test_wait_for_sync_for_user_auth_blocking(self): + sync_config = SyncConfig( + user=UserID("@user","server"), + filter_collection=None, + is_guest=False, + request_key="request_key", + device_id="device_id", + ) + res = yield self.sync_handler.wait_for_sync_for_user(sync_config) + print res From 69ce057ea613f425d5ef6ace03d0019a8e4fdf49 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Thu, 9 Aug 2018 12:26:27 +0100 Subject: [PATCH 02/17] block sync if auth checks fail --- synapse/handlers/sync.py | 12 +++++------- tests/handlers/test_sync.py | 19 +++++++++++++------ 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py index f748d9afb..776ddca63 100644 --- a/synapse/handlers/sync.py +++ b/synapse/handlers/sync.py @@ -209,14 +209,12 @@ class SyncHandler(object): Deferred[SyncResult] """ yield self.auth.check_auth_blocking() - - defer.returnValue( - self.response_cache.wrap( - sync_config.request_key, - self._wait_for_sync_for_user, - sync_config, since_token, timeout, full_state, - ) + res = yield self.response_cache.wrap( + sync_config.request_key, + self._wait_for_sync_for_user, + sync_config, since_token, timeout, full_state, ) + defer.returnValue(res) @defer.inlineCallbacks def _wait_for_sync_for_user(self, sync_config, since_token, timeout, diff --git a/tests/handlers/test_sync.py b/tests/handlers/test_sync.py index 3b1b4d492..497e4bd93 100644 --- a/tests/handlers/test_sync.py +++ b/tests/handlers/test_sync.py @@ -14,11 +14,14 @@ # limitations under the License. from twisted.internet import defer +from synapse.api.errors import AuthError +from synapse.api.filtering import DEFAULT_FILTER_COLLECTION +from synapse.handlers.sync import SyncConfig, SyncHandler +from synapse.types import UserID + import tests.unittest import tests.utils from tests.utils import setup_test_homeserver -from synapse.handlers.sync import SyncHandler, SyncConfig -from synapse.types import UserID class SyncTestCase(tests.unittest.TestCase): @@ -32,11 +35,15 @@ class SyncTestCase(tests.unittest.TestCase): @defer.inlineCallbacks def test_wait_for_sync_for_user_auth_blocking(self): sync_config = SyncConfig( - user=UserID("@user","server"), - filter_collection=None, + user=UserID("@user", "server"), + filter_collection=DEFAULT_FILTER_COLLECTION, is_guest=False, request_key="request_key", device_id="device_id", ) - res = yield self.sync_handler.wait_for_sync_for_user(sync_config) - print res + # Ensure that an exception is not thrown + yield self.sync_handler.wait_for_sync_for_user(sync_config) + self.hs.config.hs_disabled = True + + with self.assertRaises(AuthError): + yield self.sync_handler.wait_for_sync_for_user(sync_config) From d967653705854dac98cde06cb3de54113e704e60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Christian=20Gr=C3=BCnhage?= Date: Thu, 9 Aug 2018 13:21:30 +0200 Subject: [PATCH 03/17] update docker base-image to alpine 3.8 --- changelog.d/3669.misc | 1 + docker/Dockerfile | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 changelog.d/3669.misc diff --git a/changelog.d/3669.misc b/changelog.d/3669.misc new file mode 100644 index 000000000..fc579ddc6 --- /dev/null +++ b/changelog.d/3669.misc @@ -0,0 +1 @@ +Update docker base image from alpine 3.7 to 3.8. diff --git a/docker/Dockerfile b/docker/Dockerfile index 26fb3a6bf..777976217 100644 --- a/docker/Dockerfile +++ b/docker/Dockerfile @@ -1,4 +1,4 @@ -FROM docker.io/python:2-alpine3.7 +FROM docker.io/python:2-alpine3.8 RUN apk add --no-cache --virtual .nacl_deps \ build-base \ From c6b28fb4791d88e064cd88686dd32e7ed7834525 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Thu, 9 Aug 2018 12:45:58 +0100 Subject: [PATCH 04/17] Where server is disabled, block ability for locked out users to read new messages --- changelog.d/3670.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/3670.feature diff --git a/changelog.d/3670.feature b/changelog.d/3670.feature new file mode 100644 index 000000000..ba00f2d2e --- /dev/null +++ b/changelog.d/3670.feature @@ -0,0 +1 @@ +Where server is disabled, block ability for locked out users to read new messages From 09cf13089858902f3cdcb49b9f9bc3d214ba6337 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Thu, 9 Aug 2018 17:39:12 +0100 Subject: [PATCH 05/17] only block on sync where user is not part of the mau cohort --- synapse/api/auth.py | 13 ++++++++++-- synapse/handlers/sync.py | 7 ++++++- tests/handlers/test_sync.py | 40 ++++++++++++++++++++++++++++--------- 3 files changed, 48 insertions(+), 12 deletions(-) diff --git a/synapse/api/auth.py b/synapse/api/auth.py index 9c62ec437..170039fc8 100644 --- a/synapse/api/auth.py +++ b/synapse/api/auth.py @@ -775,17 +775,26 @@ class Auth(object): ) @defer.inlineCallbacks - def check_auth_blocking(self): + def check_auth_blocking(self, user_id=None): """Checks if the user should be rejected for some external reason, such as monthly active user limiting or global disable flag + + Args: + user_id(str): If present, checks for presence against existing MAU cohort """ if self.hs.config.hs_disabled: raise AuthError( 403, self.hs.config.hs_disabled_message, errcode=Codes.HS_DISABLED ) if self.hs.config.limit_usage_by_mau is True: + # If the user is already part of the MAU cohort + if user_id: + timestamp = yield self.store._user_last_seen_monthly_active(user_id) + if timestamp: + return + # Else if there is no room in the MAU bucket, bail current_mau = yield self.store.get_monthly_active_count() if current_mau >= self.hs.config.max_mau_value: raise AuthError( 403, "MAU Limit Exceeded", errcode=Codes.MAU_LIMIT_EXCEEDED - ) + ) diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py index 776ddca63..d3b26a410 100644 --- a/synapse/handlers/sync.py +++ b/synapse/handlers/sync.py @@ -208,7 +208,12 @@ class SyncHandler(object): Returns: Deferred[SyncResult] """ - yield self.auth.check_auth_blocking() + # If the user is not part of the mau group, then check that limits have + # not been exceeded (if not part of the group by this point, almost certain + # auth_blocking will occur) + user_id = sync_config.user.to_string() + yield self.auth.check_auth_blocking(user_id) + res = yield self.response_cache.wrap( sync_config.request_key, self._wait_for_sync_for_user, diff --git a/tests/handlers/test_sync.py b/tests/handlers/test_sync.py index 497e4bd93..b95a8743a 100644 --- a/tests/handlers/test_sync.py +++ b/tests/handlers/test_sync.py @@ -13,8 +13,8 @@ # See the License for the specific language governing permissions and # limitations under the License. from twisted.internet import defer +from synapse.api.errors import AuthError, Codes -from synapse.api.errors import AuthError from synapse.api.filtering import DEFAULT_FILTER_COLLECTION from synapse.handlers.sync import SyncConfig, SyncHandler from synapse.types import UserID @@ -31,19 +31,41 @@ class SyncTestCase(tests.unittest.TestCase): def setUp(self): self.hs = yield setup_test_homeserver() self.sync_handler = SyncHandler(self.hs) + self.store = self.hs.get_datastore() @defer.inlineCallbacks def test_wait_for_sync_for_user_auth_blocking(self): - sync_config = SyncConfig( - user=UserID("@user", "server"), + + user_id1 = "@user1:server" + user_id2 = "@user2:server" + sync_config = self._generate_sync_config(user_id1) + + self.hs.config.limit_usage_by_mau = True + self.hs.config.max_mau_value = 1 + + # Check that the happy case does not throw errors + yield self.store.upsert_monthly_active_user(user_id1) + yield self.sync_handler.wait_for_sync_for_user(sync_config) + + # Test that global lock works + self.hs.config.hs_disabled = True + with self.assertRaises(AuthError) as e: + yield self.sync_handler.wait_for_sync_for_user(sync_config) + self.assertEquals(e.exception.errcode, Codes.HS_DISABLED) + + self.hs.config.hs_disabled = False + + sync_config = self._generate_sync_config(user_id2) + + with self.assertRaises(AuthError) as e: + yield self.sync_handler.wait_for_sync_for_user(sync_config) + self.assertEquals(e.exception.errcode, Codes.MAU_LIMIT_EXCEEDED) + + def _generate_sync_config(self, user_id): + return SyncConfig( + user=UserID(user_id.split(":")[0][1:], user_id.split(":")[1]), filter_collection=DEFAULT_FILTER_COLLECTION, is_guest=False, request_key="request_key", device_id="device_id", ) - # Ensure that an exception is not thrown - yield self.sync_handler.wait_for_sync_for_user(sync_config) - self.hs.config.hs_disabled = True - - with self.assertRaises(AuthError): - yield self.sync_handler.wait_for_sync_for_user(sync_config) From 04df7142598b531e8e400611e3f92b21afeabab6 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Thu, 9 Aug 2018 17:41:52 +0100 Subject: [PATCH 06/17] fix imports --- tests/handlers/test_sync.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/handlers/test_sync.py b/tests/handlers/test_sync.py index b95a8743a..cfd37f313 100644 --- a/tests/handlers/test_sync.py +++ b/tests/handlers/test_sync.py @@ -13,8 +13,8 @@ # See the License for the specific language governing permissions and # limitations under the License. from twisted.internet import defer -from synapse.api.errors import AuthError, Codes +from synapse.api.errors import AuthError, Codes from synapse.api.filtering import DEFAULT_FILTER_COLLECTION from synapse.handlers.sync import SyncConfig, SyncHandler from synapse.types import UserID From c1f9dec92ac8abaa693cc591438087d8282c6844 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Thu, 9 Aug 2018 17:43:26 +0100 Subject: [PATCH 07/17] fix errant parenthesis --- synapse/api/auth.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/api/auth.py b/synapse/api/auth.py index 170039fc8..df6022ff6 100644 --- a/synapse/api/auth.py +++ b/synapse/api/auth.py @@ -797,4 +797,4 @@ class Auth(object): if current_mau >= self.hs.config.max_mau_value: raise AuthError( 403, "MAU Limit Exceeded", errcode=Codes.MAU_LIMIT_EXCEEDED - ) + ) From 885ea9c602ca4002385a6d73c94c35e0958de809 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Thu, 9 Aug 2018 18:02:12 +0100 Subject: [PATCH 08/17] rename _user_last_seen_monthly_active --- synapse/api/auth.py | 2 +- synapse/storage/monthly_active_users.py | 10 +++++----- tests/storage/test_client_ips.py | 12 ++++++------ tests/storage/test_monthly_active_users.py | 13 +++++++------ 4 files changed, 19 insertions(+), 18 deletions(-) diff --git a/synapse/api/auth.py b/synapse/api/auth.py index df6022ff6..c31c6a6a0 100644 --- a/synapse/api/auth.py +++ b/synapse/api/auth.py @@ -789,7 +789,7 @@ class Auth(object): if self.hs.config.limit_usage_by_mau is True: # If the user is already part of the MAU cohort if user_id: - timestamp = yield self.store._user_last_seen_monthly_active(user_id) + timestamp = yield self.store.user_last_seen_monthly_active(user_id) if timestamp: return # Else if there is no room in the MAU bucket, bail diff --git a/synapse/storage/monthly_active_users.py b/synapse/storage/monthly_active_users.py index d47dcef3a..07211432a 100644 --- a/synapse/storage/monthly_active_users.py +++ b/synapse/storage/monthly_active_users.py @@ -113,7 +113,7 @@ class MonthlyActiveUsersStore(SQLBaseStore): # is racy. # Have resolved to invalidate the whole cache for now and do # something about it if and when the perf becomes significant - self._user_last_seen_monthly_active.invalidate_all() + self.user_last_seen_monthly_active.invalidate_all() self.get_monthly_active_count.invalidate_all() @cached(num_args=0) @@ -152,11 +152,11 @@ class MonthlyActiveUsersStore(SQLBaseStore): lock=False, ) if is_insert: - self._user_last_seen_monthly_active.invalidate((user_id,)) + self.user_last_seen_monthly_active.invalidate((user_id,)) self.get_monthly_active_count.invalidate(()) @cached(num_args=1) - def _user_last_seen_monthly_active(self, user_id): + def user_last_seen_monthly_active(self, user_id): """ Checks if a given user is part of the monthly active user group Arguments: @@ -173,7 +173,7 @@ class MonthlyActiveUsersStore(SQLBaseStore): }, retcol="timestamp", allow_none=True, - desc="_user_last_seen_monthly_active", + desc="user_last_seen_monthly_active", )) @defer.inlineCallbacks @@ -185,7 +185,7 @@ class MonthlyActiveUsersStore(SQLBaseStore): user_id(str): the user_id to query """ if self.hs.config.limit_usage_by_mau: - last_seen_timestamp = yield self._user_last_seen_monthly_active(user_id) + last_seen_timestamp = yield self.user_last_seen_monthly_active(user_id) now = self.hs.get_clock().time_msec() # We want to reduce to the total number of db writes, and are happy diff --git a/tests/storage/test_client_ips.py b/tests/storage/test_client_ips.py index 7a58c6eb2..6d2bb3058 100644 --- a/tests/storage/test_client_ips.py +++ b/tests/storage/test_client_ips.py @@ -64,7 +64,7 @@ class ClientIpStoreTestCase(tests.unittest.TestCase): yield self.store.insert_client_ip( user_id, "access_token", "ip", "user_agent", "device_id", ) - active = yield self.store._user_last_seen_monthly_active(user_id) + active = yield self.store.user_last_seen_monthly_active(user_id) self.assertFalse(active) @defer.inlineCallbacks @@ -80,7 +80,7 @@ class ClientIpStoreTestCase(tests.unittest.TestCase): yield self.store.insert_client_ip( user_id, "access_token", "ip", "user_agent", "device_id", ) - active = yield self.store._user_last_seen_monthly_active(user_id) + active = yield self.store.user_last_seen_monthly_active(user_id) self.assertFalse(active) @defer.inlineCallbacks @@ -88,13 +88,13 @@ class ClientIpStoreTestCase(tests.unittest.TestCase): self.hs.config.limit_usage_by_mau = True self.hs.config.max_mau_value = 50 user_id = "@user:server" - active = yield self.store._user_last_seen_monthly_active(user_id) + active = yield self.store.user_last_seen_monthly_active(user_id) self.assertFalse(active) yield self.store.insert_client_ip( user_id, "access_token", "ip", "user_agent", "device_id", ) - active = yield self.store._user_last_seen_monthly_active(user_id) + active = yield self.store.user_last_seen_monthly_active(user_id) self.assertTrue(active) @defer.inlineCallbacks @@ -103,7 +103,7 @@ class ClientIpStoreTestCase(tests.unittest.TestCase): self.hs.config.max_mau_value = 50 user_id = "@user:server" - active = yield self.store._user_last_seen_monthly_active(user_id) + active = yield self.store.user_last_seen_monthly_active(user_id) self.assertFalse(active) yield self.store.insert_client_ip( @@ -112,5 +112,5 @@ class ClientIpStoreTestCase(tests.unittest.TestCase): yield self.store.insert_client_ip( user_id, "access_token", "ip", "user_agent", "device_id", ) - active = yield self.store._user_last_seen_monthly_active(user_id) + active = yield self.store.user_last_seen_monthly_active(user_id) self.assertTrue(active) diff --git a/tests/storage/test_monthly_active_users.py b/tests/storage/test_monthly_active_users.py index cbd480cd4..be74c3021 100644 --- a/tests/storage/test_monthly_active_users.py +++ b/tests/storage/test_monthly_active_users.py @@ -66,9 +66,9 @@ class MonthlyActiveUsersTestCase(tests.unittest.TestCase): # Test user is marked as active - timestamp = yield self.store._user_last_seen_monthly_active(user1) + timestamp = yield self.store.user_last_seen_monthly_active(user1) self.assertTrue(timestamp) - timestamp = yield self.store._user_last_seen_monthly_active(user2) + timestamp = yield self.store.user_last_seen_monthly_active(user2) self.assertTrue(timestamp) # Test that users are never removed from the db. @@ -92,17 +92,18 @@ class MonthlyActiveUsersTestCase(tests.unittest.TestCase): self.assertEqual(1, count) @defer.inlineCallbacks - def test__user_last_seen_monthly_active(self): + def test_user_last_seen_monthly_active(self): user_id1 = "@user1:server" user_id2 = "@user2:server" user_id3 = "@user3:server" - result = yield self.store._user_last_seen_monthly_active(user_id1) + + result = yield self.store.user_last_seen_monthly_active(user_id1) self.assertFalse(result == 0) yield self.store.upsert_monthly_active_user(user_id1) yield self.store.upsert_monthly_active_user(user_id2) - result = yield self.store._user_last_seen_monthly_active(user_id1) + result = yield self.store.user_last_seen_monthly_active(user_id1) self.assertTrue(result > 0) - result = yield self.store._user_last_seen_monthly_active(user_id3) + result = yield self.store.user_last_seen_monthly_active(user_id3) self.assertFalse(result == 0) @defer.inlineCallbacks From 31fa743567c3f70c66c7af2f1332560b04b78f7a Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Sat, 11 Aug 2018 22:38:34 +0100 Subject: [PATCH 09/17] fix sqlite/postgres incompatibility in reap_monthly_active_users --- synapse/storage/monthly_active_users.py | 42 ++++++++++++++++--------- 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/synapse/storage/monthly_active_users.py b/synapse/storage/monthly_active_users.py index d47dcef3a..9a6e69938 100644 --- a/synapse/storage/monthly_active_users.py +++ b/synapse/storage/monthly_active_users.py @@ -64,23 +64,27 @@ class MonthlyActiveUsersStore(SQLBaseStore): Deferred[] """ def _reap_users(txn): + # Purge stale users thirty_days_ago = ( int(self._clock.time_msec()) - (1000 * 60 * 60 * 24 * 30) ) - # Purge stale users - - # questionmarks is a hack to overcome sqlite not supporting - # tuples in 'WHERE IN %s' - questionmarks = '?' * len(self.reserved_users) query_args = [thirty_days_ago] - query_args.extend(self.reserved_users) + base_sql = "DELETE FROM monthly_active_users WHERE timestamp < ?" - sql = """ - DELETE FROM monthly_active_users - WHERE timestamp < ? - AND user_id NOT IN ({}) - """.format(','.join(questionmarks)) + # Need if/else since 'AND user_id NOT IN ({})' fails on Postgres + # when len(reserved_users) == 0. Works fine on sqlite. + if len(self.reserved_users) > 0: + # questionmarks is a hack to overcome sqlite not supporting + # tuples in 'WHERE IN %s' + questionmarks = '?' * len(self.reserved_users) + + query_args.extend(self.reserved_users) + sql = base_sql + """ AND user_id NOT IN ({})""".format( + ','.join(questionmarks) + ) + else: + sql = base_sql txn.execute(sql, query_args) @@ -93,16 +97,24 @@ class MonthlyActiveUsersStore(SQLBaseStore): # negative LIMIT values. So there is no way to write it that both can # support query_args = [self.hs.config.max_mau_value] - query_args.extend(self.reserved_users) - sql = """ + + base_sql = """ DELETE FROM monthly_active_users WHERE user_id NOT IN ( SELECT user_id FROM monthly_active_users ORDER BY timestamp DESC LIMIT ? ) - AND user_id NOT IN ({}) - """.format(','.join(questionmarks)) + """ + # Need if/else since 'AND user_id NOT IN ({})' fails on Postgres + # when len(reserved_users) == 0. Works fine on sqlite. + if len(self.reserved_users) > 0: + query_args.extend(self.reserved_users) + sql = base_sql + """ AND user_id NOT IN ({})""".format( + ','.join(questionmarks) + ) + else: + sql = base_sql txn.execute(sql, query_args) yield self.runInteraction("reap_monthly_active_users", _reap_users) From ac205a54b22478726cbab4427a3370a58737d71f Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Sat, 11 Aug 2018 22:50:09 +0100 Subject: [PATCH 10/17] Fixes test_reap_monthly_active_users so it passes under postgres --- changelog.d/3681.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/3681.bugfix diff --git a/changelog.d/3681.bugfix b/changelog.d/3681.bugfix new file mode 100644 index 000000000..d18a69cd0 --- /dev/null +++ b/changelog.d/3681.bugfix @@ -0,0 +1 @@ +Fixes test_reap_monthly_active_users so it passes under postgres From bdfbd934d6eb42b899779abbe17f72ec4c77e720 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Tue, 14 Aug 2018 20:53:43 +1000 Subject: [PATCH 11/17] Implement a new test baseclass to cut down on boilerplate (#3684) --- changelog.d/3684.misc | 1 + tests/rest/client/v1/test_typing.py | 66 ++++++------- tests/unittest.py | 144 ++++++++++++++++++++++++++++ 3 files changed, 174 insertions(+), 37 deletions(-) create mode 100644 changelog.d/3684.misc diff --git a/changelog.d/3684.misc b/changelog.d/3684.misc new file mode 100644 index 000000000..4c013263c --- /dev/null +++ b/changelog.d/3684.misc @@ -0,0 +1 @@ +Implemented a new testing base class to reduce test boilerplate. diff --git a/tests/rest/client/v1/test_typing.py b/tests/rest/client/v1/test_typing.py index 677265edf..0ad814c5e 100644 --- a/tests/rest/client/v1/test_typing.py +++ b/tests/rest/client/v1/test_typing.py @@ -1,5 +1,6 @@ # -*- coding: utf-8 -*- # Copyright 2014-2016 OpenMarket Ltd +# 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. @@ -17,41 +18,32 @@ from mock import Mock, NonCallableMock -# twisted imports from twisted.internet import defer -import synapse.rest.client.v1.room +from synapse.rest.client.v1 import room from synapse.types import UserID -from ....utils import MockClock, MockHttpResource, setup_test_homeserver -from .utils import RestTestCase +from tests import unittest PATH_PREFIX = "/_matrix/client/api/v1" -class RoomTypingTestCase(RestTestCase): +class RoomTypingTestCase(unittest.HomeserverTestCase): """ Tests /rooms/$room_id/typing/$user_id REST API. """ user_id = "@sid:red" user = UserID.from_string(user_id) + servlets = [room.register_servlets] - @defer.inlineCallbacks - def setUp(self): - self.clock = MockClock() + def make_homeserver(self, reactor, clock): - self.mock_resource = MockHttpResource(prefix=PATH_PREFIX) - self.auth_user_id = self.user_id - - hs = yield setup_test_homeserver( - self.addCleanup, + hs = self.setup_test_homeserver( "red", - clock=self.clock, http_client=None, federation_client=Mock(), ratelimiter=NonCallableMock(spec_set=["send_message"]), ) - self.hs = hs self.event_source = hs.get_event_sources().sources["typing"] @@ -100,25 +92,24 @@ class RoomTypingTestCase(RestTestCase): fetch_room_distributions_into ) - synapse.rest.client.v1.room.register_servlets(hs, self.mock_resource) + return hs - self.room_id = yield self.create_room_as(self.user_id) + def prepare(self, reactor, clock, hs): + self.room_id = self.helper.create_room_as(self.user_id) # Need another user to make notifications actually work - yield self.join(self.room_id, user="@jim:red") + self.helper.join(self.room_id, user="@jim:red") - @defer.inlineCallbacks def test_set_typing(self): - (code, _) = yield self.mock_resource.trigger( + request, channel = self.make_request( "PUT", "/rooms/%s/typing/%s" % (self.room_id, self.user_id), - '{"typing": true, "timeout": 30000}', + b'{"typing": true, "timeout": 30000}', ) - self.assertEquals(200, code) + self.render(request) + self.assertEquals(200, channel.code) self.assertEquals(self.event_source.get_current_key(), 1) - events = yield self.event_source.get_new_events( - from_key=0, room_ids=[self.room_id] - ) + events = self.event_source.get_new_events(from_key=0, room_ids=[self.room_id]) self.assertEquals( events[0], [ @@ -130,35 +121,36 @@ class RoomTypingTestCase(RestTestCase): ], ) - @defer.inlineCallbacks def test_set_not_typing(self): - (code, _) = yield self.mock_resource.trigger( + request, channel = self.make_request( "PUT", "/rooms/%s/typing/%s" % (self.room_id, self.user_id), - '{"typing": false}', + b'{"typing": false}', ) - self.assertEquals(200, code) + self.render(request) + self.assertEquals(200, channel.code) - @defer.inlineCallbacks def test_typing_timeout(self): - (code, _) = yield self.mock_resource.trigger( + request, channel = self.make_request( "PUT", "/rooms/%s/typing/%s" % (self.room_id, self.user_id), - '{"typing": true, "timeout": 30000}', + b'{"typing": true, "timeout": 30000}', ) - self.assertEquals(200, code) + self.render(request) + self.assertEquals(200, channel.code) self.assertEquals(self.event_source.get_current_key(), 1) - self.clock.advance_time(36) + self.reactor.advance(36) self.assertEquals(self.event_source.get_current_key(), 2) - (code, _) = yield self.mock_resource.trigger( + request, channel = self.make_request( "PUT", "/rooms/%s/typing/%s" % (self.room_id, self.user_id), - '{"typing": true, "timeout": 30000}', + b'{"typing": true, "timeout": 30000}', ) - self.assertEquals(200, code) + self.render(request) + self.assertEquals(200, channel.code) self.assertEquals(self.event_source.get_current_key(), 3) diff --git a/tests/unittest.py b/tests/unittest.py index f448a6dfb..e6afe3b96 100644 --- a/tests/unittest.py +++ b/tests/unittest.py @@ -1,5 +1,6 @@ # -*- coding: utf-8 -*- # Copyright 2014-2016 OpenMarket Ltd +# 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. @@ -15,12 +16,19 @@ import logging +from mock import Mock + import twisted import twisted.logger from twisted.trial import unittest +from synapse.http.server import JsonResource +from synapse.server import HomeServer +from synapse.types import UserID, create_requester from synapse.util.logcontext import LoggingContextFilter +from tests.server import get_clock, make_request, render, setup_test_homeserver + # Set up putting Synapse's logs into Trial's. rootLogger = logging.getLogger() @@ -129,3 +137,139 @@ def DEBUG(target): Can apply to either a TestCase or an individual test method.""" target.loglevel = logging.DEBUG return target + + +class HomeserverTestCase(TestCase): + """ + A base TestCase that reduces boilerplate for HomeServer-using test cases. + + Attributes: + servlets (list[function]): List of servlet registration function. + user_id (str): The user ID to assume if auth is hijacked. + hijack_auth (bool): Whether to hijack auth to return the user specified + in user_id. + """ + servlets = [] + hijack_auth = True + + def setUp(self): + """ + Set up the TestCase by calling the homeserver constructor, optionally + hijacking the authentication system to return a fixed user, and then + calling the prepare function. + """ + self.reactor, self.clock = get_clock() + self._hs_args = {"clock": self.clock, "reactor": self.reactor} + self.hs = self.make_homeserver(self.reactor, self.clock) + + if self.hs is None: + raise Exception("No homeserver returned from make_homeserver.") + + if not isinstance(self.hs, HomeServer): + raise Exception("A homeserver wasn't returned, but %r" % (self.hs,)) + + # Register the resources + self.resource = JsonResource(self.hs) + + for servlet in self.servlets: + servlet(self.hs, self.resource) + + if hasattr(self, "user_id"): + from tests.rest.client.v1.utils import RestHelper + + self.helper = RestHelper(self.hs, self.resource, self.user_id) + + if self.hijack_auth: + + def get_user_by_access_token(token=None, allow_guest=False): + return { + "user": UserID.from_string(self.helper.auth_user_id), + "token_id": 1, + "is_guest": False, + } + + def get_user_by_req(request, allow_guest=False, rights="access"): + return create_requester( + UserID.from_string(self.helper.auth_user_id), 1, False, None + ) + + self.hs.get_auth().get_user_by_req = get_user_by_req + self.hs.get_auth().get_user_by_access_token = get_user_by_access_token + self.hs.get_auth().get_access_token_from_request = Mock( + return_value="1234" + ) + + if hasattr(self, "prepare"): + self.prepare(self.reactor, self.clock, self.hs) + + def make_homeserver(self, reactor, clock): + """ + Make and return a homeserver. + + Args: + reactor: A Twisted Reactor, or something that pretends to be one. + clock (synapse.util.Clock): The Clock, associated with the reactor. + + Returns: + A homeserver (synapse.server.HomeServer) suitable for testing. + + Function to be overridden in subclasses. + """ + raise NotImplementedError() + + def prepare(self, reactor, clock, homeserver): + """ + Prepare for the test. This involves things like mocking out parts of + the homeserver, or building test data common across the whole test + suite. + + Args: + reactor: A Twisted Reactor, or something that pretends to be one. + clock (synapse.util.Clock): The Clock, associated with the reactor. + homeserver (synapse.server.HomeServer): The HomeServer to test + against. + + Function to optionally be overridden in subclasses. + """ + + def make_request(self, method, path, content=b""): + """ + Create a SynapseRequest at the path using the method and containing the + given content. + + Args: + method (bytes/unicode): The HTTP request method ("verb"). + path (bytes/unicode): The HTTP path, suitably URL encoded (e.g. + escaped UTF-8 & spaces and such). + content (bytes): The body of the request. + + Returns: + A synapse.http.site.SynapseRequest. + """ + return make_request(method, path, content) + + def render(self, request): + """ + Render a request against the resources registered by the test class's + servlets. + + Args: + request (synapse.http.site.SynapseRequest): The request to render. + """ + render(request, self.resource, self.reactor) + + def setup_test_homeserver(self, *args, **kwargs): + """ + Set up the test homeserver, meant to be called by the overridable + make_homeserver. It automatically passes through the test class's + clock & reactor. + + Args: + See tests.utils.setup_test_homeserver. + + Returns: + synapse.server.HomeServer + """ + kwargs = dict(kwargs) + kwargs.update(self._hs_args) + return setup_test_homeserver(self.addCleanup, *args, **kwargs) From 614e6d517d90922cbc54e59d0117a4daaed9a9fc Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Tue, 14 Aug 2018 21:30:34 +1000 Subject: [PATCH 12/17] Dockerised sytest (#3660) --- .circleci/config.yml | 48 +++++++++++++++++++++++++++++++++++++++++++ .dockerignore | 3 +++ MANIFEST.in | 1 + changelog.d/3660.misc | 1 + 4 files changed, 53 insertions(+) create mode 100644 .circleci/config.yml create mode 100644 changelog.d/3660.misc diff --git a/.circleci/config.yml b/.circleci/config.yml new file mode 100644 index 000000000..e03f01b83 --- /dev/null +++ b/.circleci/config.yml @@ -0,0 +1,48 @@ +version: 2 +jobs: + sytestpy2: + machine: true + steps: + - checkout + - run: docker pull matrixdotorg/sytest-synapsepy2 + - run: docker run --rm -it -v $(pwd)\:/src -v $(pwd)/logs\:/logs matrixdotorg/sytest-synapsepy2 + - store_artifacts: + path: ~/project/logs + destination: logs + sytestpy2postgres: + machine: true + steps: + - checkout + - run: docker pull matrixdotorg/sytest-synapsepy2 + - run: docker run --rm -it -v $(pwd)\:/src -v $(pwd)/logs\:/logs -e POSTGRES=1 matrixdotorg/sytest-synapsepy2 + - store_artifacts: + path: ~/project/logs + destination: logs + sytestpy3: + machine: true + steps: + - checkout + - run: docker pull matrixdotorg/sytest-synapsepy3 + - run: docker run --rm -it -v $(pwd)\:/src -v $(pwd)/logs\:/logs hawkowl/sytestpy3 + - store_artifacts: + path: ~/project/logs + destination: logs + sytestpy3postgres: + machine: true + steps: + - checkout + - run: docker pull matrixdotorg/sytest-synapsepy3 + - run: docker run --rm -it -v $(pwd)\:/src -v $(pwd)/logs\:/logs -e POSTGRES=1 matrixdotorg/sytest-synapsepy3 + - store_artifacts: + path: ~/project/logs + destination: logs + +workflows: + version: 2 + build: + jobs: + - sytestpy2 + - sytestpy2postgres +# Currently broken while the Python 3 port is incomplete +# - sytestpy3 +# - sytestpy3postgres diff --git a/.dockerignore b/.dockerignore index f36f86fbb..6cdb8532d 100644 --- a/.dockerignore +++ b/.dockerignore @@ -3,3 +3,6 @@ Dockerfile .gitignore demo/etc tox.ini +synctl +.git/* +.tox/* diff --git a/MANIFEST.in b/MANIFEST.in index 1ff98d95d..e0826ba54 100644 --- a/MANIFEST.in +++ b/MANIFEST.in @@ -36,3 +36,4 @@ recursive-include changelog.d * prune .github prune demo/etc prune docker +prune .circleci diff --git a/changelog.d/3660.misc b/changelog.d/3660.misc new file mode 100644 index 000000000..acd814c27 --- /dev/null +++ b/changelog.d/3660.misc @@ -0,0 +1 @@ +Sytests can now be run inside a Docker container. From 2545993ce430606f76926723898c2de3840da94a Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 14 Aug 2018 15:48:12 +0100 Subject: [PATCH 13/17] make comments clearer --- synapse/api/auth.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/synapse/api/auth.py b/synapse/api/auth.py index c31c6a6a0..18c73f054 100644 --- a/synapse/api/auth.py +++ b/synapse/api/auth.py @@ -780,7 +780,8 @@ class Auth(object): such as monthly active user limiting or global disable flag Args: - user_id(str): If present, checks for presence against existing MAU cohort + user_id(str|None): If present, checks for presence against existing + MAU cohort """ if self.hs.config.hs_disabled: raise AuthError( From 1522ed9c07b11a97064a6642c6fd6c3e786f9cb5 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 14 Aug 2018 16:52:30 +0100 Subject: [PATCH 14/17] in case max_mau is less than I think --- tests/storage/test_monthly_active_users.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/storage/test_monthly_active_users.py b/tests/storage/test_monthly_active_users.py index 0d58dcebc..511acbde9 100644 --- a/tests/storage/test_monthly_active_users.py +++ b/tests/storage/test_monthly_active_users.py @@ -33,7 +33,7 @@ class MonthlyActiveUsersTestCase(tests.unittest.TestCase): @defer.inlineCallbacks def test_initialise_reserved_users(self): - + self.hs.config.max_mau_value = 5 user1 = "@user1:server" user1_email = "user1@matrix.org" user2 = "@user2:server" From 9ecbaf8ba8b12537862e3045a650fffd438ba8a5 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 14 Aug 2018 16:55:28 +0100 Subject: [PATCH 15/17] adding missing yield --- synapse/storage/monthly_active_users.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/monthly_active_users.py b/synapse/storage/monthly_active_users.py index 7b36accdb..7e417f811 100644 --- a/synapse/storage/monthly_active_users.py +++ b/synapse/storage/monthly_active_users.py @@ -46,7 +46,7 @@ class MonthlyActiveUsersStore(SQLBaseStore): tp["medium"], tp["address"] ) if user_id: - self.upsert_monthly_active_user(user_id) + yield self.upsert_monthly_active_user(user_id) reserved_user_list.append(user_id) else: logger.warning( From cd0c749c4f86e9350be1152a91958d0064d41c84 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 14 Aug 2018 17:06:51 +0100 Subject: [PATCH 16/17] Fix missing yield in synapse.storage.monthly_active_users.initialise_reserved_users --- changelog.d/3692.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/3692.bugfix diff --git a/changelog.d/3692.bugfix b/changelog.d/3692.bugfix new file mode 100644 index 000000000..f44e13dca --- /dev/null +++ b/changelog.d/3692.bugfix @@ -0,0 +1 @@ +Fix missing yield in synapse.storage.monthly_active_users.initialise_reserved_users From 7277216d01261f055886d0ac7b1ae5e5c5fc33cf Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 14 Aug 2018 17:14:39 +0100 Subject: [PATCH 17/17] fix setup_test_homeserver to be postgres compatible --- tests/handlers/test_sync.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/handlers/test_sync.py b/tests/handlers/test_sync.py index cfd37f313..8c8b65e04 100644 --- a/tests/handlers/test_sync.py +++ b/tests/handlers/test_sync.py @@ -29,7 +29,7 @@ class SyncTestCase(tests.unittest.TestCase): @defer.inlineCallbacks def setUp(self): - self.hs = yield setup_test_homeserver() + self.hs = yield setup_test_homeserver(self.addCleanup) self.sync_handler = SyncHandler(self.hs) self.store = self.hs.get_datastore()