From 01021c812f86466a5f35db8afd443b9cb17bd092 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Thu, 9 Aug 2018 22:16:00 +0100 Subject: [PATCH 01/26] wip at implementing MSC 7075 --- .../resource_limits_server_notices.py | 84 +++++++++++++++++++ 1 file changed, 84 insertions(+) create mode 100644 synapse/server_notices/resource_limits_server_notices.py diff --git a/synapse/server_notices/resource_limits_server_notices.py b/synapse/server_notices/resource_limits_server_notices.py new file mode 100644 index 0000000000..33b1d80cd9 --- /dev/null +++ b/synapse/server_notices/resource_limits_server_notices.py @@ -0,0 +1,84 @@ +# -*- 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. +import logging + +from six import iteritems, string_types + +from twisted.internet import defer + +from synapse.api.errors import SynapseError +from synapse.api.urls import ConsentURIBuilder +from synapse.config import ConfigError +from synapse.types import get_localpart_from_id + +logger = logging.getLogger(__name__) + + +class ResourceLimitsServerNotices(object): + """ + """ + def __init__(self, hs): + """ + + Args: + hs (synapse.server.HomeServer): + """ + self._server_notices_manager = hs.get_server_notices_manager() + self._store = hs.get_datastore() + self._api = hs.get_api() + self._server_notice_content = hs.config.user_consent_server_notice_content + self._limit_usage_by_mau = config.limit_usage_by_mau = False + self._hs_disabled.config.hs_disabled = False + + self._notified = set() + self._resouce_limited = False + # Config checks? + + @defer.inlineCallbacks + def maybe_send_server_notice_to_user(self, user_id): + """Check if we need to send a notice to this user, and does so if so + + Args: + user_id (str): user to check + + Returns: + Deferred + """ + if self._limit_usage_by_mau is False and self._hs_disabled is False: + # not enabled + return + + timestamp = yield self.store.user_last_seen_monthly_active(user_id) + if timestamp is None: + # This user will be blocked from receiving the notice anyway + return + try: + yield self.api.check_auth_blocking() + if self._resouce_limited: + # Need to start removing notices + pass + except AuthError as e: + # Need to start notifying of blocking + if not self._resouce_limited: + pass + + # need to send a message. + try: + yield self._server_notices_manager.send_notice( + user_id, content, + ) + + except SynapseError as e: + logger.error("Error sending server notice about resource limits: %s", e) From 6c6aba76e1059f103ed48487e74ab4161347638b Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Fri, 10 Aug 2018 15:12:59 +0100 Subject: [PATCH 02/26] implementation of server notices to alert on hitting resource limits --- .../resource_limits_server_notices.py | 79 ++++++----- .../server_notices/server_notices_sender.py | 33 +++-- .../test_resource_limits_server_notices.py | 125 ++++++++++++++++++ 3 files changed, 191 insertions(+), 46 deletions(-) create mode 100644 tests/server_notices/test_resource_limits_server_notices.py diff --git a/synapse/server_notices/resource_limits_server_notices.py b/synapse/server_notices/resource_limits_server_notices.py index 33b1d80cd9..017828f81e 100644 --- a/synapse/server_notices/resource_limits_server_notices.py +++ b/synapse/server_notices/resource_limits_server_notices.py @@ -14,14 +14,9 @@ # limitations under the License. import logging -from six import iteritems, string_types - from twisted.internet import defer -from synapse.api.errors import SynapseError -from synapse.api.urls import ConsentURIBuilder -from synapse.config import ConfigError -from synapse.types import get_localpart_from_id +from synapse.api.errors import AuthError, SynapseError logger = logging.getLogger(__name__) @@ -37,12 +32,12 @@ class ResourceLimitsServerNotices(object): """ self._server_notices_manager = hs.get_server_notices_manager() self._store = hs.get_datastore() - self._api = hs.get_api() + self.auth = hs.get_auth() self._server_notice_content = hs.config.user_consent_server_notice_content - self._limit_usage_by_mau = config.limit_usage_by_mau = False - self._hs_disabled.config.hs_disabled = False + self._limit_usage_by_mau = hs.config.limit_usage_by_mau = False + self._hs_disabled = hs.config.hs_disabled - self._notified = set() + self._notified_of_blocking = set() self._resouce_limited = False # Config checks? @@ -56,29 +51,49 @@ class ResourceLimitsServerNotices(object): Returns: Deferred """ - if self._limit_usage_by_mau is False and self._hs_disabled is False: - # not enabled + if self._hs_disabled is True: return - timestamp = yield self.store.user_last_seen_monthly_active(user_id) - if timestamp is None: - # This user will be blocked from receiving the notice anyway - return - try: - yield self.api.check_auth_blocking() - if self._resouce_limited: - # Need to start removing notices - pass - except AuthError as e: - # Need to start notifying of blocking - if not self._resouce_limited: - pass - - # need to send a message. + if self._limit_usage_by_mau is True: + timestamp = yield self._store.user_last_seen_monthly_active(user_id) + if timestamp is None: + # This user will be blocked from receiving the notice anyway. + # In practice, not sure we can ever get here + return try: - yield self._server_notices_manager.send_notice( - user_id, content, - ) + yield self.auth.check_auth_blocking() + self._resouce_limited = False + # Need to start removing notices + if user_id in self._notified_of_blocking: + # Send message to remove warning - needs updating + content = "remove warning" + self._send_server_notice(user_id, content) + self._notified_of_blocking.remove(user_id) - except SynapseError as e: - logger.error("Error sending server notice about resource limits: %s", e) + except AuthError: + # Need to start notifying of blocking + + self._resouce_limited = True + if user_id not in self._notified_of_blocking: + # Send message to add warning - needs updating + content = "add warning" + self._send_server_notice(user_id, content) + self._notified_of_blocking.add(user_id) + + @defer.inlineCallbacks + def _send_server_notice(self, user_id, content): + """Sends Server notice + + Args: + user_id(str): The user to send to + content(str): The content of the message + + Returns: + Deferred[] + """ + try: + yield self._server_notices_manager.send_notice( + user_id, content, + ) + except SynapseError as e: + logger.error("Error sending server notice about resource limits: %s", e) diff --git a/synapse/server_notices/server_notices_sender.py b/synapse/server_notices/server_notices_sender.py index 5d23965f34..6121b2f267 100644 --- a/synapse/server_notices/server_notices_sender.py +++ b/synapse/server_notices/server_notices_sender.py @@ -12,7 +12,12 @@ # 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 + from synapse.server_notices.consent_server_notices import ConsentServerNotices +from synapse.server_notices.resource_limits_server_notices import ( + ResourceLimitsServerNotices, +) class ServerNoticesSender(object): @@ -25,34 +30,34 @@ class ServerNoticesSender(object): Args: hs (synapse.server.HomeServer): """ - # todo: it would be nice to make this more dynamic - self._consent_server_notices = ConsentServerNotices(hs) + self._server_notices = ( + ConsentServerNotices(hs), + ResourceLimitsServerNotices(hs) + ) + @defer.inlineCallbacks def on_user_syncing(self, user_id): """Called when the user performs a sync operation. Args: user_id (str): mxid of user who synced - - Returns: - Deferred """ - return self._consent_server_notices.maybe_send_server_notice_to_user( - user_id, - ) + for sn in self._server_notices: + yield sn.maybe_send_server_notice_to_user( + user_id, + ) + @defer.inlineCallbacks def on_user_ip(self, user_id): """Called on the master when a worker process saw a client request. Args: user_id (str): mxid - - Returns: - Deferred """ # The synchrotrons use a stubbed version of ServerNoticesSender, so # we check for notices to send to the user in on_user_ip as well as # in on_user_syncing - return self._consent_server_notices.maybe_send_server_notice_to_user( - user_id, - ) + for sn in self._server_notices: + yield sn.maybe_send_server_notice_to_user( + user_id, + ) diff --git a/tests/server_notices/test_resource_limits_server_notices.py b/tests/server_notices/test_resource_limits_server_notices.py new file mode 100644 index 0000000000..a69253f1b6 --- /dev/null +++ b/tests/server_notices/test_resource_limits_server_notices.py @@ -0,0 +1,125 @@ +from mock import Mock + +from twisted.internet import defer + +from synapse.api.errors import AuthError +from synapse.handlers.auth import AuthHandler +from synapse.server_notices.resource_limits_server_notices import ( + ResourceLimitsServerNotices, +) + +from tests import unittest +from tests.utils import setup_test_homeserver + + +class AuthHandlers(object): + def __init__(self, hs): + self.auth_handler = AuthHandler(hs) + + +class TestResourceLimitsServerNotices(unittest.TestCase): + @defer.inlineCallbacks + def setUp(self): + self.hs = yield setup_test_homeserver(handlers=None) + self.hs.handlers = AuthHandlers(self.hs) + self.auth_handler = self.hs.handlers.auth_handler + self.server_notices_sender = self.hs.get_server_notices_sender() + + # relying on [1] is far from ideal, but the only case where + # ResourceLimitsServerNotices class needs to be isolated is this test, + # general code should never have a reason to do so ... + self._rlsn = self.server_notices_sender._server_notices[1] + if not isinstance(self._rlsn, ResourceLimitsServerNotices): + raise Exception("Failed to find reference to ResourceLimitsServerNotices") + + self._rlsn._store.user_last_seen_monthly_active = Mock( + return_value=defer.succeed(1000) + ) + self._send_notice = self._rlsn._server_notices_manager.send_notice + self._rlsn._server_notices_manager.send_notice = Mock() + self._send_notice = self._rlsn._server_notices_manager.send_notice + + self._rlsn._limit_usage_by_mau = True + self.user_id = "user_id" + + @defer.inlineCallbacks + def test_maybe_send_server_notice_to_user_flag_off(self): + """Tests cases where the flags indicate nothing to do""" + # test hs disabled case + self._hs_disabled = True + + yield self._rlsn.maybe_send_server_notice_to_user("user_id") + + self._send_notice.assert_not_called() + # Test when mau limiting disabled + self._hs_disabled = False + self._rlsn._limit_usage_by_mau = False + yield self._rlsn.maybe_send_server_notice_to_user("user_id") + + self._send_notice.assert_not_called() + + @defer.inlineCallbacks + def test_maybe_send_server_notice_to_user_remove_blocked_notice(self): + """Test when user has blocked notice, but should have it removed""" + + self._rlsn._notified_of_blocking.add(self.user_id) + self._rlsn.auth.check_auth_blocking = Mock() + + yield self._rlsn.maybe_send_server_notice_to_user(self.user_id) + # "remove warning" obviously aweful, but test will start failing when code + # actually sends a real event, and then it can be updated + + self._send_notice.assert_called_once_with(self.user_id, "remove warning") + self.assertFalse(self.user_id in self._rlsn._notified_of_blocking) + + @defer.inlineCallbacks + def test_maybe_send_server_notice_to_user_remove_blocked_notice_noop(self): + """Test when user has blocked notice, but notice ought to be there (NOOP)""" + self._rlsn._notified_of_blocking.add(self.user_id) + self._rlsn.auth.check_auth_blocking = Mock( + side_effect=AuthError(403, 'foo') + ) + + yield self._rlsn.maybe_send_server_notice_to_user("user_id") + + self._send_notice.assert_not_called() + self.assertTrue(self.user_id in self._rlsn._notified_of_blocking) + + @defer.inlineCallbacks + def test_maybe_send_server_notice_to_user_add_blocked_notice(self): + """Test when user does not have blocked notice, but should have one""" + + self._rlsn.auth.check_auth_blocking = Mock(side_effect=AuthError(403, 'foo')) + yield self._rlsn.maybe_send_server_notice_to_user("user_id") + + # "add warning" obviously awful, but test will start failing when code + # actually sends a real event, and then it can be updated + self._send_notice.assert_called_once_with(self.user_id, "add warning") + self.assertTrue(self.user_id in self._rlsn._notified_of_blocking) + + @defer.inlineCallbacks + def test_maybe_send_server_notice_to_user_add_blocked_notice_noop(self): + """Test when user does not have blocked notice, nor should they (NOOP)""" + + self._rlsn.auth.check_auth_blocking = Mock() + + yield self._rlsn.maybe_send_server_notice_to_user(self.user_id) + + self._send_notice.assert_not_called() + self.assertFalse(self.user_id in self._rlsn._notified_of_blocking) + + @defer.inlineCallbacks + def test_maybe_send_server_notice_to_user_not_in_mau_cohort(self): + + """Test when user is not part of the MAU cohort - this should not ever + happen - but ... + """ + + self._rlsn.auth.check_auth_blocking = Mock() + self._rlsn._store.user_last_seen_monthly_active = Mock( + return_value=defer.succeed(None) + ) + yield self._rlsn.maybe_send_server_notice_to_user(self.user_id) + + self._send_notice.assert_not_called() + self.assertFalse(self.user_id in self._rlsn._notified_of_blocking) From c79c4c0a7d4887c24050df72417b1be9afe2992c Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Fri, 10 Aug 2018 15:25:47 +0100 Subject: [PATCH 03/26] server notices for resource limit blocking --- changelog.d/3680.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/3680.feature diff --git a/changelog.d/3680.feature b/changelog.d/3680.feature new file mode 100644 index 0000000000..4edaaf76a8 --- /dev/null +++ b/changelog.d/3680.feature @@ -0,0 +1 @@ +Server notices for resource limit blocking From 63417c31e9d32992fbb422b630024946494ff26b Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Mon, 13 Aug 2018 22:36:52 +0100 Subject: [PATCH 04/26] fix typo --- synapse/server_notices/resource_limits_server_notices.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/server_notices/resource_limits_server_notices.py b/synapse/server_notices/resource_limits_server_notices.py index 017828f81e..e4be836439 100644 --- a/synapse/server_notices/resource_limits_server_notices.py +++ b/synapse/server_notices/resource_limits_server_notices.py @@ -34,7 +34,7 @@ class ResourceLimitsServerNotices(object): self._store = hs.get_datastore() self.auth = hs.get_auth() self._server_notice_content = hs.config.user_consent_server_notice_content - self._limit_usage_by_mau = hs.config.limit_usage_by_mau = False + self._limit_usage_by_mau = hs.config.limit_usage_by_mau self._hs_disabled = hs.config.hs_disabled self._notified_of_blocking = set() From 9b75c78b4df276fe40d3f1b92a57481c3e970886 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 14 Aug 2018 11:20:41 +0100 Subject: [PATCH 05/26] support server notice state events for resource limits --- synapse/api/constants.py | 2 ++ .../server_notices/consent_server_notices.py | 3 ++- .../resource_limits_server_notices.py | 21 ++++++++++++++----- .../server_notices/server_notices_manager.py | 4 ++-- 4 files changed, 22 insertions(+), 8 deletions(-) diff --git a/synapse/api/constants.py b/synapse/api/constants.py index b0da506f6d..ef60841995 100644 --- a/synapse/api/constants.py +++ b/synapse/api/constants.py @@ -79,6 +79,8 @@ class EventTypes(object): ServerACL = "m.room.server_acl" + ServerNoticeLimitReached = "m.server_notice.usage_limit_reached" + class RejectedReason(object): AUTH_ERROR = "auth_error" diff --git a/synapse/server_notices/consent_server_notices.py b/synapse/server_notices/consent_server_notices.py index 5e3044d164..6d5caedb08 100644 --- a/synapse/server_notices/consent_server_notices.py +++ b/synapse/server_notices/consent_server_notices.py @@ -22,6 +22,7 @@ from synapse.api.errors import SynapseError from synapse.api.urls import ConsentURIBuilder from synapse.config import ConfigError from synapse.types import get_localpart_from_id +from synapse.api.constants import EventTypes logger = logging.getLogger(__name__) @@ -103,7 +104,7 @@ class ConsentServerNotices(object): }, ) yield self._server_notices_manager.send_notice( - user_id, content, + user_id, content, EventTypes.Message ) yield self._store.user_set_consent_server_notice_sent( user_id, self._current_consent_version, diff --git a/synapse/server_notices/resource_limits_server_notices.py b/synapse/server_notices/resource_limits_server_notices.py index e4be836439..2f49dae168 100644 --- a/synapse/server_notices/resource_limits_server_notices.py +++ b/synapse/server_notices/resource_limits_server_notices.py @@ -17,6 +17,7 @@ import logging from twisted.internet import defer from synapse.api.errors import AuthError, SynapseError +from synapse.api.constants import EventTypes logger = logging.getLogger(__name__) @@ -61,22 +62,32 @@ class ResourceLimitsServerNotices(object): # In practice, not sure we can ever get here return try: + # Normally should always pass in user_id if you have it, but in + # this case are checking what would happen to other users if they + # were to arrive. yield self.auth.check_auth_blocking() self._resouce_limited = False # Need to start removing notices if user_id in self._notified_of_blocking: # Send message to remove warning - needs updating - content = "remove warning" + content = { + 'body': '', + 'admin_email': '', + } self._send_server_notice(user_id, content) self._notified_of_blocking.remove(user_id) - except AuthError: + except AuthError as e: # Need to start notifying of blocking self._resouce_limited = True if user_id not in self._notified_of_blocking: - # Send message to add warning - needs updating - content = "add warning" + # TODO use admin email contained in error once PR lands + content = { + 'body': e.msg, + 'admin_email': 'stunt@adminemail.com', + 'msgtype': 'm.text' + } self._send_server_notice(user_id, content) self._notified_of_blocking.add(user_id) @@ -93,7 +104,7 @@ class ResourceLimitsServerNotices(object): """ try: yield self._server_notices_manager.send_notice( - user_id, content, + user_id, content, EventTypes.ServerNoticeLimitReached ) except SynapseError as e: logger.error("Error sending server notice about resource limits: %s", e) diff --git a/synapse/server_notices/server_notices_manager.py b/synapse/server_notices/server_notices_manager.py index a26deace53..4806977a89 100644 --- a/synapse/server_notices/server_notices_manager.py +++ b/synapse/server_notices/server_notices_manager.py @@ -46,7 +46,7 @@ class ServerNoticesManager(object): return self._config.server_notices_mxid is not None @defer.inlineCallbacks - def send_notice(self, user_id, event_content): + def send_notice(self, user_id, event_content, type): """Send a notice to the given user Creates the server notices room, if none exists. @@ -67,7 +67,7 @@ class ServerNoticesManager(object): yield self._event_creation_handler.create_and_send_nonmember_event( requester, { - "type": EventTypes.Message, + "type": type, "room_id": room_id, "sender": system_mxid, "content": event_content, From e2c9fe0a6a6dbe4f81402b79b6605e17fb05e5d4 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Tue, 14 Aug 2018 13:32:56 +0100 Subject: [PATCH 06/26] backout ability to pass in event type to server notices --- synapse/server_notices/consent_server_notices.py | 2 +- synapse/server_notices/resource_limits_server_notices.py | 3 ++- synapse/server_notices/server_notices_manager.py | 4 ++-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/synapse/server_notices/consent_server_notices.py b/synapse/server_notices/consent_server_notices.py index 6d5caedb08..783ceae1ad 100644 --- a/synapse/server_notices/consent_server_notices.py +++ b/synapse/server_notices/consent_server_notices.py @@ -104,7 +104,7 @@ class ConsentServerNotices(object): }, ) yield self._server_notices_manager.send_notice( - user_id, content, EventTypes.Message + user_id, content ) yield self._store.user_set_consent_server_notice_sent( user_id, self._current_consent_version, diff --git a/synapse/server_notices/resource_limits_server_notices.py b/synapse/server_notices/resource_limits_server_notices.py index 2f49dae168..0470f66205 100644 --- a/synapse/server_notices/resource_limits_server_notices.py +++ b/synapse/server_notices/resource_limits_server_notices.py @@ -91,6 +91,7 @@ class ResourceLimitsServerNotices(object): self._send_server_notice(user_id, content) self._notified_of_blocking.add(user_id) + @defer.inlineCallbacks def _send_server_notice(self, user_id, content): """Sends Server notice @@ -104,7 +105,7 @@ class ResourceLimitsServerNotices(object): """ try: yield self._server_notices_manager.send_notice( - user_id, content, EventTypes.ServerNoticeLimitReached + user_id, content ) except SynapseError as e: logger.error("Error sending server notice about resource limits: %s", e) diff --git a/synapse/server_notices/server_notices_manager.py b/synapse/server_notices/server_notices_manager.py index 4806977a89..a26deace53 100644 --- a/synapse/server_notices/server_notices_manager.py +++ b/synapse/server_notices/server_notices_manager.py @@ -46,7 +46,7 @@ class ServerNoticesManager(object): return self._config.server_notices_mxid is not None @defer.inlineCallbacks - def send_notice(self, user_id, event_content, type): + def send_notice(self, user_id, event_content): """Send a notice to the given user Creates the server notices room, if none exists. @@ -67,7 +67,7 @@ class ServerNoticesManager(object): yield self._event_creation_handler.create_and_send_nonmember_event( requester, { - "type": type, + "type": EventTypes.Message, "room_id": room_id, "sender": system_mxid, "content": event_content, From c24fc9797bc0dd59c0d8131ea09c1178e39b76f6 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Wed, 15 Aug 2018 15:04:30 +0100 Subject: [PATCH 07/26] add new event types --- synapse/api/constants.py | 1 + 1 file changed, 1 insertion(+) diff --git a/synapse/api/constants.py b/synapse/api/constants.py index ef60841995..1fb24578e2 100644 --- a/synapse/api/constants.py +++ b/synapse/api/constants.py @@ -78,6 +78,7 @@ class EventTypes(object): Name = "m.room.name" ServerACL = "m.room.server_acl" + Pinned = "m.room.pinned_events" ServerNoticeLimitReached = "m.server_notice.usage_limit_reached" From eabc5f827107a24717655e0eec376adf88b574c4 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Wed, 15 Aug 2018 15:04:52 +0100 Subject: [PATCH 08/26] wip cut at sending resource server notices --- .../resource_limits_server_notices.py | 65 ++++++++++--------- .../server_notices/server_notices_manager.py | 30 ++++++--- 2 files changed, 53 insertions(+), 42 deletions(-) diff --git a/synapse/server_notices/resource_limits_server_notices.py b/synapse/server_notices/resource_limits_server_notices.py index 0470f66205..94d0f98189 100644 --- a/synapse/server_notices/resource_limits_server_notices.py +++ b/synapse/server_notices/resource_limits_server_notices.py @@ -40,6 +40,7 @@ class ResourceLimitsServerNotices(object): self._notified_of_blocking = set() self._resouce_limited = False + # Config checks? @defer.inlineCallbacks @@ -69,43 +70,43 @@ class ResourceLimitsServerNotices(object): self._resouce_limited = False # Need to start removing notices if user_id in self._notified_of_blocking: - # Send message to remove warning - needs updating + # Send message to remove warning + # send state event here + # How do I do this? if drop the id, how to refer to it? content = { - 'body': '', - 'admin_email': '', + "pinned":[] } - self._send_server_notice(user_id, content) + yield self._server_notices_manager.send_notice( + user_id, content, EventTypes.Pinned, '', + ) + self._notified_of_blocking.remove(user_id) except AuthError as e: # Need to start notifying of blocking + try: + self._resouce_limited = True + if user_id not in self._notified_of_blocking: + # TODO use admin email contained in error once PR lands + content = { + 'body': e.msg, + 'admin_email': 'stunt@adminemail.com', + } + event = yield self._server_notices_manager.send_notice( + user_id, content, EventTypes.ServerNoticeLimitReached + ) - self._resouce_limited = True - if user_id not in self._notified_of_blocking: - # TODO use admin email contained in error once PR lands - content = { - 'body': e.msg, - 'admin_email': 'stunt@adminemail.com', - 'msgtype': 'm.text' - } - self._send_server_notice(user_id, content) - self._notified_of_blocking.add(user_id) + # send server notices state event here + # TODO Over writing pinned events + content = { + "pinned":[ + event.event_id, + ] + } + yield self._server_notices_manager.send_notice( + user_id, content, EventTypes.Pinned, '', + ) - - @defer.inlineCallbacks - def _send_server_notice(self, user_id, content): - """Sends Server notice - - Args: - user_id(str): The user to send to - content(str): The content of the message - - Returns: - Deferred[] - """ - try: - yield self._server_notices_manager.send_notice( - user_id, content - ) - except SynapseError as e: - logger.error("Error sending server notice about resource limits: %s", e) + self._notified_of_blocking.add(user_id) + except SynapseError as e: + logger.error("Error sending server notice about resource limits: %s", e) diff --git a/synapse/server_notices/server_notices_manager.py b/synapse/server_notices/server_notices_manager.py index a26deace53..3e57f8211a 100644 --- a/synapse/server_notices/server_notices_manager.py +++ b/synapse/server_notices/server_notices_manager.py @@ -46,7 +46,10 @@ class ServerNoticesManager(object): return self._config.server_notices_mxid is not None @defer.inlineCallbacks - def send_notice(self, user_id, event_content): + def send_notice( + self, user_id, event_content, + type=EventTypes.Message, state_key=None + ): """Send a notice to the given user Creates the server notices room, if none exists. @@ -54,9 +57,11 @@ class ServerNoticesManager(object): Args: user_id (str): mxid of user to send event to. event_content (dict): content of event to send + type(EventTypes): type of event + is_state_event(bool): Is the event a state event Returns: - Deferred[None] + Deferred[FrozenEvent] """ room_id = yield self.get_notice_room_for_user(user_id) @@ -65,15 +70,19 @@ class ServerNoticesManager(object): logger.info("Sending server notice to %s", user_id) - yield self._event_creation_handler.create_and_send_nonmember_event( - requester, { - "type": EventTypes.Message, - "room_id": room_id, - "sender": system_mxid, - "content": event_content, - }, - ratelimit=False, + event_dict = { + "type": type, + "room_id": room_id, + "sender": system_mxid, + "content": event_content, + } + if state_key: + event_dict['state_key'] = state_key + + res = yield self._event_creation_handler.create_and_send_nonmember_event( + requester, event_dict, ratelimit=False, ) + defer.returnValue(res) @cachedInlineCallbacks() def get_notice_room_for_user(self, user_id): @@ -141,6 +150,7 @@ class ServerNoticesManager(object): creator_join_profile=join_profile, ) room_id = info['room_id'] + yield self._store.add_tag_to_room(user_id, room_id, 'm.server_notice', None) logger.info("Created server notices room %s for %s", room_id, user_id) defer.returnValue(room_id) From c055c91655cbe5d4e6b36d86a00dafb02c585b6d Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Thu, 16 Aug 2018 11:10:19 +0100 Subject: [PATCH 09/26] fix case where empty string state check is evaulated as False --- synapse/server_notices/server_notices_manager.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/synapse/server_notices/server_notices_manager.py b/synapse/server_notices/server_notices_manager.py index 3e57f8211a..22c819cc5b 100644 --- a/synapse/server_notices/server_notices_manager.py +++ b/synapse/server_notices/server_notices_manager.py @@ -76,7 +76,8 @@ class ServerNoticesManager(object): "sender": system_mxid, "content": event_content, } - if state_key: + + if state_key is not None: event_dict['state_key'] = state_key res = yield self._event_creation_handler.create_and_send_nonmember_event( From df1e4f259fda3009dacbb3211e999f7d89adaf87 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Thu, 16 Aug 2018 11:10:53 +0100 Subject: [PATCH 10/26] WIP impl commiting to get feedback --- .../resource_limits_server_notices.py | 80 +++++++++++++++++-- 1 file changed, 74 insertions(+), 6 deletions(-) diff --git a/synapse/server_notices/resource_limits_server_notices.py b/synapse/server_notices/resource_limits_server_notices.py index 94d0f98189..2b714a1016 100644 --- a/synapse/server_notices/resource_limits_server_notices.py +++ b/synapse/server_notices/resource_limits_server_notices.py @@ -40,7 +40,8 @@ class ResourceLimitsServerNotices(object): self._notified_of_blocking = set() self._resouce_limited = False - + self._message_handler = hs.get_message_handler() + self._state = hs.get_state_handler() # Config checks? @defer.inlineCallbacks @@ -57,6 +58,72 @@ class ResourceLimitsServerNotices(object): return if self._limit_usage_by_mau is True: + room_id = yield self._server_notices_manager.get_notice_room_for_user(user_id) + + + # Alternate impl - currently inlcuded because I'm not sure I am on + # the right track and want to share WIP + + # logger.info("GET STATE EVENTS") + # currently_blocked = False + # events = [] + # try: + # events = yield self._message_handler.get_state_events(user_id, room_id, types=[(EventTypes.Pinned, None)]) + # except AuthError as e: + # # The user has yet to join the server notices room + # pass + # + # pinned_event_refs = [] + # for e in events: + # logger.info('events %s' % e) + # logger.info(type(e)) + # for key, event_ids in e['content'].items(): + # logger.info('Key Event %s %s' % (key, event_ids)) + # if key == 'pinned': + # pinned_event_refs = event_ids + # + # logger.info('pinned_event_refs %s' % pinned_event_refs) + # + # events = yield self._store.get_events(pinned_event_refs) + # logger.info(events) + # for event_id, event in events.items(): + # logger.info("event_id, event event.type %s %s %s" % (event_id, event, event.type)) + # if event.type == 'm.server_notice.usage_limit_reached': + # currently_blocked = True + # + # logger.info('Currently Blocked is %r' % currently_blocked) + + #for e in events: + # logger.info(e) + currently_blocked = False + logger.info("GET CURRENT STATE") + pinned_state_event = yield self._state.get_current_state(room_id, event_type=EventTypes.Pinned) + logger.info(events) + logger.info(events.get('content')) + + referenced_events = [] + if pinned_state_event is not None: + content = pinned_state_event.get('content') + if content is not None: + referenced_events = content.get('pinned') + + events = yield self._store.get_events(referenced_events) + logger.info(events) + for event_id, event in events.items(): + logger.info("event_id, event event.type %s %s %s" % (event_id, event, event.type)) + if event.type == 'm.server_notice.usage_limit_reached': + currently_blocked = True + + logger.info("currently_blocked is %r" % currently_blocked) + + #event = yield self._store.get_event(events.event_id) + #logger.info(event) + + #logger.info("GET CURRENT STATE IDs") + #events = yield self._state.get_current_state_ids(room_id) + #for k,v in events.items(): + # logger.info('%s %s' % (k,v)) + timestamp = yield self._store.user_last_seen_monthly_active(user_id) if timestamp is None: # This user will be blocked from receiving the notice anyway. @@ -69,7 +136,8 @@ class ResourceLimitsServerNotices(object): yield self.auth.check_auth_blocking() self._resouce_limited = False # Need to start removing notices - if user_id in self._notified_of_blocking: + # if user_id in self._notified_of_blocking: + if currently_blocked: # Send message to remove warning # send state event here # How do I do this? if drop the id, how to refer to it? @@ -79,14 +147,14 @@ class ResourceLimitsServerNotices(object): yield self._server_notices_manager.send_notice( user_id, content, EventTypes.Pinned, '', ) - - self._notified_of_blocking.remove(user_id) + logger.info('deactivate block') except AuthError as e: # Need to start notifying of blocking try: self._resouce_limited = True - if user_id not in self._notified_of_blocking: + #if user_id not in self._notified_of_blocking: + if not currently_blocked: # TODO use admin email contained in error once PR lands content = { 'body': e.msg, @@ -103,10 +171,10 @@ class ResourceLimitsServerNotices(object): event.event_id, ] } + logger.info("active block") yield self._server_notices_manager.send_notice( user_id, content, EventTypes.Pinned, '', ) - self._notified_of_blocking.add(user_id) except SynapseError as e: logger.error("Error sending server notice about resource limits: %s", e) From a675f9c5560a16655a9337992499690c9f27a7eb Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Thu, 16 Aug 2018 14:53:35 +0100 Subject: [PATCH 11/26] check for room state before deciding on action --- .../server_notices/consent_server_notices.py | 1 - .../resource_limits_server_notices.py | 110 ++++++------------ .../test_resource_limits_server_notices.py | 52 +++++---- 3 files changed, 67 insertions(+), 96 deletions(-) diff --git a/synapse/server_notices/consent_server_notices.py b/synapse/server_notices/consent_server_notices.py index 783ceae1ad..a9fe58d9ff 100644 --- a/synapse/server_notices/consent_server_notices.py +++ b/synapse/server_notices/consent_server_notices.py @@ -22,7 +22,6 @@ from synapse.api.errors import SynapseError from synapse.api.urls import ConsentURIBuilder from synapse.config import ConfigError from synapse.types import get_localpart_from_id -from synapse.api.constants import EventTypes logger = logging.getLogger(__name__) diff --git a/synapse/server_notices/resource_limits_server_notices.py b/synapse/server_notices/resource_limits_server_notices.py index 2b714a1016..43a926fc79 100644 --- a/synapse/server_notices/resource_limits_server_notices.py +++ b/synapse/server_notices/resource_limits_server_notices.py @@ -16,8 +16,8 @@ import logging from twisted.internet import defer -from synapse.api.errors import AuthError, SynapseError from synapse.api.constants import EventTypes +from synapse.api.errors import AuthError, SynapseError logger = logging.getLogger(__name__) @@ -35,10 +35,10 @@ class ResourceLimitsServerNotices(object): self._store = hs.get_datastore() self.auth = hs.get_auth() self._server_notice_content = hs.config.user_consent_server_notice_content + self._admin_uri = hs.config.admin_uri self._limit_usage_by_mau = hs.config.limit_usage_by_mau self._hs_disabled = hs.config.hs_disabled - self._notified_of_blocking = set() self._resouce_limited = False self._message_handler = hs.get_message_handler() self._state = hs.get_state_handler() @@ -58,83 +58,45 @@ class ResourceLimitsServerNotices(object): return if self._limit_usage_by_mau is True: - room_id = yield self._server_notices_manager.get_notice_room_for_user(user_id) - - - # Alternate impl - currently inlcuded because I'm not sure I am on - # the right track and want to share WIP - - # logger.info("GET STATE EVENTS") - # currently_blocked = False - # events = [] - # try: - # events = yield self._message_handler.get_state_events(user_id, room_id, types=[(EventTypes.Pinned, None)]) - # except AuthError as e: - # # The user has yet to join the server notices room - # pass - # - # pinned_event_refs = [] - # for e in events: - # logger.info('events %s' % e) - # logger.info(type(e)) - # for key, event_ids in e['content'].items(): - # logger.info('Key Event %s %s' % (key, event_ids)) - # if key == 'pinned': - # pinned_event_refs = event_ids - # - # logger.info('pinned_event_refs %s' % pinned_event_refs) - # - # events = yield self._store.get_events(pinned_event_refs) - # logger.info(events) - # for event_id, event in events.items(): - # logger.info("event_id, event event.type %s %s %s" % (event_id, event, event.type)) - # if event.type == 'm.server_notice.usage_limit_reached': - # currently_blocked = True - # - # logger.info('Currently Blocked is %r' % currently_blocked) - - #for e in events: - # logger.info(e) - currently_blocked = False - logger.info("GET CURRENT STATE") - pinned_state_event = yield self._state.get_current_state(room_id, event_type=EventTypes.Pinned) - logger.info(events) - logger.info(events.get('content')) - - referenced_events = [] - if pinned_state_event is not None: - content = pinned_state_event.get('content') - if content is not None: - referenced_events = content.get('pinned') - - events = yield self._store.get_events(referenced_events) - logger.info(events) - for event_id, event in events.items(): - logger.info("event_id, event event.type %s %s %s" % (event_id, event, event.type)) - if event.type == 'm.server_notice.usage_limit_reached': - currently_blocked = True - - logger.info("currently_blocked is %r" % currently_blocked) - - #event = yield self._store.get_event(events.event_id) - #logger.info(event) - - #logger.info("GET CURRENT STATE IDs") - #events = yield self._state.get_current_state_ids(room_id) - #for k,v in events.items(): - # logger.info('%s %s' % (k,v)) - timestamp = yield self._store.user_last_seen_monthly_active(user_id) if timestamp is None: # This user will be blocked from receiving the notice anyway. # In practice, not sure we can ever get here return + + room_id = yield self._server_notices_manager.get_notice_room_for_user(user_id) + + currently_blocked = False + logger.info("GET CURRENT STATE") + pinned_state_event = None + try: + pinned_state_event = yield self._state.get_current_state( + room_id, event_type=EventTypes.Pinned + ) + except AuthError as e: + # The user has yet to join the server notices room + pass + + referenced_events = [] + if pinned_state_event is not None: + referenced_events = pinned_state_event.content.get('pinned') + + events = yield self._store.get_events(referenced_events) + logger.info(events) + for event_id, event in events.items(): + logger.info("event_id, event event.type %s %s %s" % ( + event_id, event, event.type) + ) + if event.type == EventTypes.ServerNoticeLimitReached: + currently_blocked = True + + logger.info("currently_blocked is %r" % currently_blocked) try: # Normally should always pass in user_id if you have it, but in # this case are checking what would happen to other users if they # were to arrive. yield self.auth.check_auth_blocking() - self._resouce_limited = False + # Need to start removing notices # if user_id in self._notified_of_blocking: if currently_blocked: @@ -142,7 +104,7 @@ class ResourceLimitsServerNotices(object): # send state event here # How do I do this? if drop the id, how to refer to it? content = { - "pinned":[] + "pinned": [] } yield self._server_notices_manager.send_notice( user_id, content, EventTypes.Pinned, '', @@ -152,13 +114,11 @@ class ResourceLimitsServerNotices(object): except AuthError as e: # Need to start notifying of blocking try: - self._resouce_limited = True - #if user_id not in self._notified_of_blocking: if not currently_blocked: # TODO use admin email contained in error once PR lands content = { 'body': e.msg, - 'admin_email': 'stunt@adminemail.com', + 'admin_uri': self._admin_uri, } event = yield self._server_notices_manager.send_notice( user_id, content, EventTypes.ServerNoticeLimitReached @@ -167,7 +127,7 @@ class ResourceLimitsServerNotices(object): # send server notices state event here # TODO Over writing pinned events content = { - "pinned":[ + "pinned": [ event.event_id, ] } @@ -177,4 +137,4 @@ class ResourceLimitsServerNotices(object): ) except SynapseError as e: - logger.error("Error sending server notice about resource limits: %s", e) + logger.error("Error sending resource limits server notice: %s", e) diff --git a/tests/server_notices/test_resource_limits_server_notices.py b/tests/server_notices/test_resource_limits_server_notices.py index a69253f1b6..fb8c593a75 100644 --- a/tests/server_notices/test_resource_limits_server_notices.py +++ b/tests/server_notices/test_resource_limits_server_notices.py @@ -2,6 +2,7 @@ from mock import Mock from twisted.internet import defer +from synapse.api.constants import EventTypes from synapse.api.errors import AuthError from synapse.handlers.auth import AuthHandler from synapse.server_notices.resource_limits_server_notices import ( @@ -20,7 +21,7 @@ class AuthHandlers(object): class TestResourceLimitsServerNotices(unittest.TestCase): @defer.inlineCallbacks def setUp(self): - self.hs = yield setup_test_homeserver(handlers=None) + self.hs = yield setup_test_homeserver(self.addCleanup, handlers=None) self.hs.handlers = AuthHandlers(self.hs) self.auth_handler = self.hs.handlers.auth_handler self.server_notices_sender = self.hs.get_server_notices_sender() @@ -37,10 +38,23 @@ class TestResourceLimitsServerNotices(unittest.TestCase): ) self._send_notice = self._rlsn._server_notices_manager.send_notice self._rlsn._server_notices_manager.send_notice = Mock() + self._rlsn._state.get_current_state = Mock(return_value=defer.succeed(None)) + self._rlsn._store.get_events = Mock(return_value=defer.succeed({})) + self._send_notice = self._rlsn._server_notices_manager.send_notice self._rlsn._limit_usage_by_mau = True - self.user_id = "user_id" + self.user_id = "@user_id:test" + + self.server_notices_mxid = "@server:test" + self.server_notices_mxid_display_name = None + self.server_notices_mxid_avatar_url = None + self.server_notices_room_name = "Server Notices" + + self._rlsn._server_notices_manager.get_notice_room_for_user = Mock( + returnValue="" + ) + self.hs.config.admin_uri = "mailto:user@test.com" @defer.inlineCallbacks def test_maybe_send_server_notice_to_user_flag_off(self): @@ -48,13 +62,13 @@ class TestResourceLimitsServerNotices(unittest.TestCase): # test hs disabled case self._hs_disabled = True - yield self._rlsn.maybe_send_server_notice_to_user("user_id") + yield self._rlsn.maybe_send_server_notice_to_user(self.user_id) self._send_notice.assert_not_called() # Test when mau limiting disabled self._hs_disabled = False self._rlsn._limit_usage_by_mau = False - yield self._rlsn.maybe_send_server_notice_to_user("user_id") + yield self._rlsn.maybe_send_server_notice_to_user(self.user_id) self._send_notice.assert_not_called() @@ -62,40 +76,40 @@ class TestResourceLimitsServerNotices(unittest.TestCase): def test_maybe_send_server_notice_to_user_remove_blocked_notice(self): """Test when user has blocked notice, but should have it removed""" - self._rlsn._notified_of_blocking.add(self.user_id) self._rlsn.auth.check_auth_blocking = Mock() + mock_event = Mock(type=EventTypes.ServerNoticeLimitReached) + self._rlsn._store.get_events = Mock(return_value=defer.succeed( + {"123": mock_event} + )) yield self._rlsn.maybe_send_server_notice_to_user(self.user_id) - # "remove warning" obviously aweful, but test will start failing when code - # actually sends a real event, and then it can be updated - - self._send_notice.assert_called_once_with(self.user_id, "remove warning") - self.assertFalse(self.user_id in self._rlsn._notified_of_blocking) + # Would be better to check the content, but once == remove blocking event + self._send_notice.assert_called_once() @defer.inlineCallbacks def test_maybe_send_server_notice_to_user_remove_blocked_notice_noop(self): """Test when user has blocked notice, but notice ought to be there (NOOP)""" - self._rlsn._notified_of_blocking.add(self.user_id) self._rlsn.auth.check_auth_blocking = Mock( side_effect=AuthError(403, 'foo') ) - yield self._rlsn.maybe_send_server_notice_to_user("user_id") + mock_event = Mock(type=EventTypes.ServerNoticeLimitReached) + self._rlsn._store.get_events = Mock(return_value=defer.succeed( + {"123": mock_event} + )) + yield self._rlsn.maybe_send_server_notice_to_user(self.user_id) self._send_notice.assert_not_called() - self.assertTrue(self.user_id in self._rlsn._notified_of_blocking) @defer.inlineCallbacks def test_maybe_send_server_notice_to_user_add_blocked_notice(self): """Test when user does not have blocked notice, but should have one""" self._rlsn.auth.check_auth_blocking = Mock(side_effect=AuthError(403, 'foo')) - yield self._rlsn.maybe_send_server_notice_to_user("user_id") + yield self._rlsn.maybe_send_server_notice_to_user(self.user_id) - # "add warning" obviously awful, but test will start failing when code - # actually sends a real event, and then it can be updated - self._send_notice.assert_called_once_with(self.user_id, "add warning") - self.assertTrue(self.user_id in self._rlsn._notified_of_blocking) + # Would be better to check contents, but 2 calls == set blocking event + self.assertTrue(self._send_notice.call_count == 2) @defer.inlineCallbacks def test_maybe_send_server_notice_to_user_add_blocked_notice_noop(self): @@ -106,7 +120,6 @@ class TestResourceLimitsServerNotices(unittest.TestCase): yield self._rlsn.maybe_send_server_notice_to_user(self.user_id) self._send_notice.assert_not_called() - self.assertFalse(self.user_id in self._rlsn._notified_of_blocking) @defer.inlineCallbacks def test_maybe_send_server_notice_to_user_not_in_mau_cohort(self): @@ -122,4 +135,3 @@ class TestResourceLimitsServerNotices(unittest.TestCase): yield self._rlsn.maybe_send_server_notice_to_user(self.user_id) self._send_notice.assert_not_called() - self.assertFalse(self.user_id in self._rlsn._notified_of_blocking) From eff3ae3b9a8ab558a106ce3fe69774a2e594cf72 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Thu, 16 Aug 2018 15:48:34 +0100 Subject: [PATCH 12/26] add room tagging --- .../resource_limits_server_notices.py | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/synapse/server_notices/resource_limits_server_notices.py b/synapse/server_notices/resource_limits_server_notices.py index 43a926fc79..570e8307cc 100644 --- a/synapse/server_notices/resource_limits_server_notices.py +++ b/synapse/server_notices/resource_limits_server_notices.py @@ -66,8 +66,12 @@ class ResourceLimitsServerNotices(object): room_id = yield self._server_notices_manager.get_notice_room_for_user(user_id) + # Need to set tag here because room may have been created prior to + # tagging being set on creation. Ideally would set something to read + # room tags first, and cache that aggressively/ + yield self._store.add_tag_to_room(user_id, room_id, 'm.server_notice', None) + currently_blocked = False - logger.info("GET CURRENT STATE") pinned_state_event = None try: pinned_state_event = yield self._state.get_current_state( @@ -82,15 +86,9 @@ class ResourceLimitsServerNotices(object): referenced_events = pinned_state_event.content.get('pinned') events = yield self._store.get_events(referenced_events) - logger.info(events) for event_id, event in events.items(): - logger.info("event_id, event event.type %s %s %s" % ( - event_id, event, event.type) - ) if event.type == EventTypes.ServerNoticeLimitReached: currently_blocked = True - - logger.info("currently_blocked is %r" % currently_blocked) try: # Normally should always pass in user_id if you have it, but in # this case are checking what would happen to other users if they @@ -109,7 +107,6 @@ class ResourceLimitsServerNotices(object): yield self._server_notices_manager.send_notice( user_id, content, EventTypes.Pinned, '', ) - logger.info('deactivate block') except AuthError as e: # Need to start notifying of blocking @@ -131,7 +128,6 @@ class ResourceLimitsServerNotices(object): event.event_id, ] } - logger.info("active block") yield self._server_notices_manager.send_notice( user_id, content, EventTypes.Pinned, '', ) From 3c1080b6e4a4d7c80d67bd9bee45d845aa145088 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Thu, 16 Aug 2018 17:02:04 +0100 Subject: [PATCH 13/26] refactor for readability, and reuse caching for setting tags --- .../resource_limits_server_notices.py | 166 +++++++++++------- .../server_notices/server_notices_manager.py | 6 +- .../test_resource_limits_server_notices.py | 1 + 3 files changed, 110 insertions(+), 63 deletions(-) diff --git a/synapse/server_notices/resource_limits_server_notices.py b/synapse/server_notices/resource_limits_server_notices.py index 570e8307cc..61becca758 100644 --- a/synapse/server_notices/resource_limits_server_notices.py +++ b/synapse/server_notices/resource_limits_server_notices.py @@ -18,6 +18,7 @@ from twisted.internet import defer from synapse.api.constants import EventTypes from synapse.api.errors import AuthError, SynapseError +from synapse.server_notices.server_notices_manager import SERVER_NOTICE_ROOM_TAG logger = logging.getLogger(__name__) @@ -57,80 +58,121 @@ class ResourceLimitsServerNotices(object): if self._hs_disabled is True: return - if self._limit_usage_by_mau is True: - timestamp = yield self._store.user_last_seen_monthly_active(user_id) - if timestamp is None: - # This user will be blocked from receiving the notice anyway. - # In practice, not sure we can ever get here - return + if self._limit_usage_by_mau is False: + return - room_id = yield self._server_notices_manager.get_notice_room_for_user(user_id) + timestamp = yield self._store.user_last_seen_monthly_active(user_id) + if timestamp is None: + # This user will be blocked from receiving the notice anyway. + # In practice, not sure we can ever get here + return - # Need to set tag here because room may have been created prior to - # tagging being set on creation. Ideally would set something to read - # room tags first, and cache that aggressively/ - yield self._store.add_tag_to_room(user_id, room_id, 'm.server_notice', None) + # Determine current state of room - currently_blocked = False - pinned_state_event = None - try: - pinned_state_event = yield self._state.get_current_state( - room_id, event_type=EventTypes.Pinned + room_id = yield self._server_notices_manager.get_notice_room_for_user(user_id) + + yield self._check_and_set_tags(user_id, room_id) + currently_blocked, ref_events = yield self._is_room_currently_blocked(room_id) + + try: + # Normally should always pass in user_id if you have it, but in + # this case are checking what would happen to other users if they + # were to arrive. + yield self.auth.check_auth_blocking() + if currently_blocked: + # Room is notifying of a block, when it ought not to be. + # Remove block notification + content = { + "pinned": ref_events + } + yield self._server_notices_manager.send_notice( + user_id, content, EventTypes.Pinned, '', ) - except AuthError as e: - # The user has yet to join the server notices room - pass - referenced_events = [] - if pinned_state_event is not None: - referenced_events = pinned_state_event.content.get('pinned') + except AuthError as e: - events = yield self._store.get_events(referenced_events) - for event_id, event in events.items(): - if event.type == EventTypes.ServerNoticeLimitReached: - currently_blocked = True try: - # Normally should always pass in user_id if you have it, but in - # this case are checking what would happen to other users if they - # were to arrive. - yield self.auth.check_auth_blocking() - - # Need to start removing notices - # if user_id in self._notified_of_blocking: - if currently_blocked: - # Send message to remove warning - # send state event here - # How do I do this? if drop the id, how to refer to it? + if not currently_blocked: + # Room is not notifying of a block, when it ought to be. + # Add block notification content = { - "pinned": [] + 'body': e.msg, + 'admin_uri': self._admin_uri, + } + event = yield self._server_notices_manager.send_notice( + user_id, content, EventTypes.ServerNoticeLimitReached + ) + + content = { + "pinned": [ + event.event_id, + ] } yield self._server_notices_manager.send_notice( user_id, content, EventTypes.Pinned, '', ) - except AuthError as e: - # Need to start notifying of blocking - try: - if not currently_blocked: - # TODO use admin email contained in error once PR lands - content = { - 'body': e.msg, - 'admin_uri': self._admin_uri, - } - event = yield self._server_notices_manager.send_notice( - user_id, content, EventTypes.ServerNoticeLimitReached - ) + except SynapseError as e: + logger.error("Error sending resource limits server notice: %s", e) - # send server notices state event here - # TODO Over writing pinned events - content = { - "pinned": [ - event.event_id, - ] - } - yield self._server_notices_manager.send_notice( - user_id, content, EventTypes.Pinned, '', - ) + @defer.inlineCallbacks + def _check_and_set_tags(self, user_id, room_id): + """ + Since server notices rooms were originally not with tags, + important to check that tags have been set correctly + Args: + user_id(str): the user in question + room_id(str): the server notices room for that user + """ + tags = yield self._store.get_tags_for_user(user_id) + server_notices_tags = tags.get(room_id) + need_to_set_tag = True + if server_notices_tags: + if server_notice_tags.get(SERVER_NOTICE_ROOM_TAG): + # tag already present, nothing to do here + need_to_set_tag = False + if need_to_set_tag: + yield self._store.add_tag_to_room( + user_id, room_id, SERVER_NOTICE_ROOM_TAG, None + ) - except SynapseError as e: - logger.error("Error sending resource limits server notice: %s", e) + @defer.inlineCallbacks + def _is_room_currently_blocked(self, room_id): + """ + Determines if the room is currently blocked + + Args: + room_id(str): The room id of the server notices room + + Returns: + + bool: Is the room currently blocked + list: The list of pinned events that are unrelated to limit blocking + This list can be used as a convenience in the case where the block + is to be lifted and the remaining pinned event references need to be + preserved + """ + currently_blocked = False + pinned_state_event = None + try: + pinned_state_event = yield self._state.get_current_state( + room_id, event_type=EventTypes.Pinned + ) + except AuthError as e: + # The user has yet to join the server notices room + pass + + referenced_events = [] + if pinned_state_event is not None: + referenced_events = pinned_state_event.content.get('pinned') + + events = yield self._store.get_events(referenced_events) + event_to_remove = None + for event_id, event in events.items(): + if event.type == EventTypes.ServerNoticeLimitReached: + currently_blocked = True + # remove event in case we need to disable blocking later on. + if event_id in referenced_events: + referenced_events.remove(event.event_id) + + defer.returnValue((currently_blocked, referenced_events)) diff --git a/synapse/server_notices/server_notices_manager.py b/synapse/server_notices/server_notices_manager.py index 22c819cc5b..5968104a99 100644 --- a/synapse/server_notices/server_notices_manager.py +++ b/synapse/server_notices/server_notices_manager.py @@ -22,6 +22,8 @@ from synapse.util.caches.descriptors import cachedInlineCallbacks logger = logging.getLogger(__name__) +SERVER_NOTICE_ROOM_TAG = "m.server_notice" + class ServerNoticesManager(object): def __init__(self, hs): @@ -151,7 +153,9 @@ class ServerNoticesManager(object): creator_join_profile=join_profile, ) room_id = info['room_id'] - yield self._store.add_tag_to_room(user_id, room_id, 'm.server_notice', None) + yield self._store.add_tag_to_room( + user_id, room_id, SERVER_NOTICE_ROOM_TAG, None + ) logger.info("Created server notices room %s for %s", room_id, user_id) defer.returnValue(room_id) diff --git a/tests/server_notices/test_resource_limits_server_notices.py b/tests/server_notices/test_resource_limits_server_notices.py index fb8c593a75..ccb69097b1 100644 --- a/tests/server_notices/test_resource_limits_server_notices.py +++ b/tests/server_notices/test_resource_limits_server_notices.py @@ -54,6 +54,7 @@ class TestResourceLimitsServerNotices(unittest.TestCase): self._rlsn._server_notices_manager.get_notice_room_for_user = Mock( returnValue="" ) + self._rlsn._store.add_tag_to_room = Mock() self.hs.config.admin_uri = "mailto:user@test.com" @defer.inlineCallbacks From 51b17ec56615b710f4937e3c20208841a6591477 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Thu, 16 Aug 2018 17:32:22 +0100 Subject: [PATCH 14/26] flake8 --- synapse/server_notices/resource_limits_server_notices.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/synapse/server_notices/resource_limits_server_notices.py b/synapse/server_notices/resource_limits_server_notices.py index 61becca758..33625fba72 100644 --- a/synapse/server_notices/resource_limits_server_notices.py +++ b/synapse/server_notices/resource_limits_server_notices.py @@ -128,7 +128,7 @@ class ResourceLimitsServerNotices(object): server_notices_tags = tags.get(room_id) need_to_set_tag = True if server_notices_tags: - if server_notice_tags.get(SERVER_NOTICE_ROOM_TAG): + if server_notices_tags.get(SERVER_NOTICE_ROOM_TAG): # tag already present, nothing to do here need_to_set_tag = False if need_to_set_tag: @@ -158,7 +158,7 @@ class ResourceLimitsServerNotices(object): pinned_state_event = yield self._state.get_current_state( room_id, event_type=EventTypes.Pinned ) - except AuthError as e: + except AuthError: # The user has yet to join the server notices room pass @@ -167,7 +167,6 @@ class ResourceLimitsServerNotices(object): referenced_events = pinned_state_event.content.get('pinned') events = yield self._store.get_events(referenced_events) - event_to_remove = None for event_id, event in events.items(): if event.type == EventTypes.ServerNoticeLimitReached: currently_blocked = True From d49b77404beab498ad8c16bd55e811b423851240 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Fri, 17 Aug 2018 15:21:34 +0100 Subject: [PATCH 15/26] clean up, no functional changes --- .../server_notices/consent_server_notices.py | 2 +- .../resource_limits_server_notices.py | 84 ++++++++++--------- .../test_resource_limits_server_notices.py | 32 +++---- 3 files changed, 63 insertions(+), 55 deletions(-) diff --git a/synapse/server_notices/consent_server_notices.py b/synapse/server_notices/consent_server_notices.py index a9fe58d9ff..5e3044d164 100644 --- a/synapse/server_notices/consent_server_notices.py +++ b/synapse/server_notices/consent_server_notices.py @@ -103,7 +103,7 @@ class ConsentServerNotices(object): }, ) yield self._server_notices_manager.send_notice( - user_id, content + user_id, content, ) yield self._store.user_set_consent_server_notice_sent( user_id, self._current_consent_version, diff --git a/synapse/server_notices/resource_limits_server_notices.py b/synapse/server_notices/resource_limits_server_notices.py index 33625fba72..5987e448b2 100644 --- a/synapse/server_notices/resource_limits_server_notices.py +++ b/synapse/server_notices/resource_limits_server_notices.py @@ -14,40 +14,41 @@ # limitations under the License. import logging +from six import iteritems + from twisted.internet import defer from synapse.api.constants import EventTypes -from synapse.api.errors import AuthError, SynapseError +from synapse.api.errors import AuthError, ResourceLimitError, SynapseError from synapse.server_notices.server_notices_manager import SERVER_NOTICE_ROOM_TAG logger = logging.getLogger(__name__) class ResourceLimitsServerNotices(object): - """ + """ Keeps track of whether the server has reached it's resource limit and + ensures that the client is kept up to date. """ def __init__(self, hs): """ - Args: hs (synapse.server.HomeServer): """ self._server_notices_manager = hs.get_server_notices_manager() self._store = hs.get_datastore() - self.auth = hs.get_auth() - self._server_notice_content = hs.config.user_consent_server_notice_content - self._admin_uri = hs.config.admin_uri - self._limit_usage_by_mau = hs.config.limit_usage_by_mau - self._hs_disabled = hs.config.hs_disabled - + self._auth = hs.get_auth() + self._config = hs.config self._resouce_limited = False self._message_handler = hs.get_message_handler() self._state = hs.get_state_handler() - # Config checks? @defer.inlineCallbacks def maybe_send_server_notice_to_user(self, user_id): - """Check if we need to send a notice to this user, and does so if so + """Check if we need to send a notice to this user, this will be true in + two cases. + 1. The server has reached its limit does not reflect this + 2. The room state indicates that the server has reached its limit when + actually the server is fine Args: user_id (str): user to check @@ -55,10 +56,10 @@ class ResourceLimitsServerNotices(object): Returns: Deferred """ - if self._hs_disabled is True: + if self._config.hs_disabled is True: return - if self._limit_usage_by_mau is False: + if self._config.limit_usage_by_mau is False: return timestamp = yield self._store.user_last_seen_monthly_active(user_id) @@ -78,8 +79,15 @@ class ResourceLimitsServerNotices(object): # Normally should always pass in user_id if you have it, but in # this case are checking what would happen to other users if they # were to arrive. - yield self.auth.check_auth_blocking() - if currently_blocked: + try: + yield self._auth.check_auth_blocking() + is_auth_blocking = False + except ResourceLimitError as e: + is_auth_blocking = True + event_content = e.msg + event_limit_type = e.limit_type + + if currently_blocked and not is_auth_blocking: # Room is notifying of a block, when it ought not to be. # Remove block notification content = { @@ -89,31 +97,29 @@ class ResourceLimitsServerNotices(object): user_id, content, EventTypes.Pinned, '', ) - except AuthError as e: + elif not currently_blocked and is_auth_blocking: + # Room is not notifying of a block, when it ought to be. + # Add block notification + content = { + 'body': event_content, + 'admin_uri': self._config.admin_uri, + 'limit_type': event_limit_type + } + event = yield self._server_notices_manager.send_notice( + user_id, content, EventTypes.ServerNoticeLimitReached + ) - try: - if not currently_blocked: - # Room is not notifying of a block, when it ought to be. - # Add block notification - content = { - 'body': e.msg, - 'admin_uri': self._admin_uri, - } - event = yield self._server_notices_manager.send_notice( - user_id, content, EventTypes.ServerNoticeLimitReached - ) + content = { + "pinned": [ + event.event_id, + ] + } + yield self._server_notices_manager.send_notice( + user_id, content, EventTypes.Pinned, '', + ) - content = { - "pinned": [ - event.event_id, - ] - } - yield self._server_notices_manager.send_notice( - user_id, content, EventTypes.Pinned, '', - ) - - except SynapseError as e: - logger.error("Error sending resource limits server notice: %s", e) + except SynapseError as e: + logger.error("Error sending resource limits server notice: %s", e) @defer.inlineCallbacks def _check_and_set_tags(self, user_id, room_id): @@ -167,7 +173,7 @@ class ResourceLimitsServerNotices(object): referenced_events = pinned_state_event.content.get('pinned') events = yield self._store.get_events(referenced_events) - for event_id, event in events.items(): + for event_id, event in events.iteritems(): if event.type == EventTypes.ServerNoticeLimitReached: currently_blocked = True # remove event in case we need to disable blocking later on. diff --git a/tests/server_notices/test_resource_limits_server_notices.py b/tests/server_notices/test_resource_limits_server_notices.py index ccb69097b1..cc8d2f539c 100644 --- a/tests/server_notices/test_resource_limits_server_notices.py +++ b/tests/server_notices/test_resource_limits_server_notices.py @@ -3,7 +3,7 @@ from mock import Mock from twisted.internet import defer from synapse.api.constants import EventTypes -from synapse.api.errors import AuthError +from synapse.api.errors import ResourceLimitError from synapse.handlers.auth import AuthHandler from synapse.server_notices.resource_limits_server_notices import ( ResourceLimitsServerNotices, @@ -43,13 +43,13 @@ class TestResourceLimitsServerNotices(unittest.TestCase): self._send_notice = self._rlsn._server_notices_manager.send_notice - self._rlsn._limit_usage_by_mau = True + self.hs.config.limit_usage_by_mau = True self.user_id = "@user_id:test" - self.server_notices_mxid = "@server:test" - self.server_notices_mxid_display_name = None - self.server_notices_mxid_avatar_url = None - self.server_notices_room_name = "Server Notices" + # self.server_notices_mxid = "@server:test" + # self.server_notices_mxid_display_name = None + # self.server_notices_mxid_avatar_url = None + # self.server_notices_room_name = "Server Notices" self._rlsn._server_notices_manager.get_notice_room_for_user = Mock( returnValue="" @@ -61,14 +61,14 @@ class TestResourceLimitsServerNotices(unittest.TestCase): def test_maybe_send_server_notice_to_user_flag_off(self): """Tests cases where the flags indicate nothing to do""" # test hs disabled case - self._hs_disabled = True + self.hs.config.hs_disabled = True yield self._rlsn.maybe_send_server_notice_to_user(self.user_id) self._send_notice.assert_not_called() # Test when mau limiting disabled - self._hs_disabled = False - self._rlsn._limit_usage_by_mau = False + self.hs.config.hs_disabled = False + self.hs.limit_usage_by_mau = False yield self._rlsn.maybe_send_server_notice_to_user(self.user_id) self._send_notice.assert_not_called() @@ -77,7 +77,7 @@ class TestResourceLimitsServerNotices(unittest.TestCase): def test_maybe_send_server_notice_to_user_remove_blocked_notice(self): """Test when user has blocked notice, but should have it removed""" - self._rlsn.auth.check_auth_blocking = Mock() + self._rlsn._auth.check_auth_blocking = Mock() mock_event = Mock(type=EventTypes.ServerNoticeLimitReached) self._rlsn._store.get_events = Mock(return_value=defer.succeed( {"123": mock_event} @@ -90,8 +90,8 @@ class TestResourceLimitsServerNotices(unittest.TestCase): @defer.inlineCallbacks def test_maybe_send_server_notice_to_user_remove_blocked_notice_noop(self): """Test when user has blocked notice, but notice ought to be there (NOOP)""" - self._rlsn.auth.check_auth_blocking = Mock( - side_effect=AuthError(403, 'foo') + self._rlsn._auth.check_auth_blocking = Mock( + side_effect=ResourceLimitError(403, 'foo') ) mock_event = Mock(type=EventTypes.ServerNoticeLimitReached) @@ -106,7 +106,9 @@ class TestResourceLimitsServerNotices(unittest.TestCase): def test_maybe_send_server_notice_to_user_add_blocked_notice(self): """Test when user does not have blocked notice, but should have one""" - self._rlsn.auth.check_auth_blocking = Mock(side_effect=AuthError(403, 'foo')) + self._rlsn._auth.check_auth_blocking = Mock( + side_effect=ResourceLimitError(403, 'foo') + ) yield self._rlsn.maybe_send_server_notice_to_user(self.user_id) # Would be better to check contents, but 2 calls == set blocking event @@ -116,7 +118,7 @@ class TestResourceLimitsServerNotices(unittest.TestCase): def test_maybe_send_server_notice_to_user_add_blocked_notice_noop(self): """Test when user does not have blocked notice, nor should they (NOOP)""" - self._rlsn.auth.check_auth_blocking = Mock() + self._rlsn._auth.check_auth_blocking = Mock() yield self._rlsn.maybe_send_server_notice_to_user(self.user_id) @@ -129,7 +131,7 @@ class TestResourceLimitsServerNotices(unittest.TestCase): happen - but ... """ - self._rlsn.auth.check_auth_blocking = Mock() + self._rlsn._auth.check_auth_blocking = Mock() self._rlsn._store.user_last_seen_monthly_active = Mock( return_value=defer.succeed(None) ) From ba1fbf7d5bd8807e892de6de4bc82cb5e49581bd Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Sat, 18 Aug 2018 12:31:08 +0100 Subject: [PATCH 16/26] special case server_notices_mxid --- synapse/server_notices/resource_limits_server_notices.py | 2 +- tests/api/test_auth.py | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/synapse/server_notices/resource_limits_server_notices.py b/synapse/server_notices/resource_limits_server_notices.py index 5987e448b2..84b91aeb5d 100644 --- a/synapse/server_notices/resource_limits_server_notices.py +++ b/synapse/server_notices/resource_limits_server_notices.py @@ -173,7 +173,7 @@ class ResourceLimitsServerNotices(object): referenced_events = pinned_state_event.content.get('pinned') events = yield self._store.get_events(referenced_events) - for event_id, event in events.iteritems(): + for event_id, event in iteritems(events): if event.type == EventTypes.ServerNoticeLimitReached: currently_blocked = True # remove event in case we need to disable blocking later on. diff --git a/tests/api/test_auth.py b/tests/api/test_auth.py index 022d81ce3e..c4cbff4e8d 100644 --- a/tests/api/test_auth.py +++ b/tests/api/test_auth.py @@ -476,3 +476,11 @@ class AuthTestCase(unittest.TestCase): self.assertEquals(e.exception.admin_uri, self.hs.config.admin_uri) self.assertEquals(e.exception.errcode, Codes.RESOURCE_LIMIT_EXCEED) self.assertEquals(e.exception.code, 403) + + @defer.inlineCallbacks + def test_server_notices_mxid_special_cased(self): + self.hs.config.hs_disabled = True + user = "@user:server" + self.hs.config.server_notices_mxid = user + self.hs.config.hs_disabled_message = "Reason for being disabled" + yield self.auth.check_auth_blocking(user) From c5171bf171ae28596c4e101e9dcc61bad7bcae63 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Sat, 18 Aug 2018 12:33:07 +0100 Subject: [PATCH 17/26] special case server_notices_mxid --- synapse/api/auth.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/synapse/api/auth.py b/synapse/api/auth.py index 022211e34e..55384d6ffe 100644 --- a/synapse/api/auth.py +++ b/synapse/api/auth.py @@ -783,6 +783,12 @@ class Auth(object): user_id(str|None): If present, checks for presence against existing MAU cohort """ + + # Never fail an auth check for the server notices users + # This can be a problem where event creation is prohibited due to blocking + if user_id == self.hs.config.server_notices_mxid: + return + if self.hs.config.hs_disabled: raise ResourceLimitError( 403, self.hs.config.hs_disabled_message, From e07970165f852ccbc4542f1aaf0fd1b2bc54b973 Mon Sep 17 00:00:00 2001 From: Neil Johnson Date: Sat, 18 Aug 2018 14:39:45 +0100 Subject: [PATCH 18/26] rename error code --- synapse/api/auth.py | 4 ++-- synapse/api/errors.py | 4 ++-- tests/api/test_auth.py | 4 ++-- tests/handlers/test_sync.py | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/synapse/api/auth.py b/synapse/api/auth.py index 55384d6ffe..4207a48afd 100644 --- a/synapse/api/auth.py +++ b/synapse/api/auth.py @@ -792,7 +792,7 @@ class Auth(object): if self.hs.config.hs_disabled: raise ResourceLimitError( 403, self.hs.config.hs_disabled_message, - errcode=Codes.RESOURCE_LIMIT_EXCEED, + errcode=Codes.RESOURCE_LIMIT_EXCEEDED, admin_uri=self.hs.config.admin_uri, limit_type=self.hs.config.hs_disabled_limit_type ) @@ -809,6 +809,6 @@ class Auth(object): 403, "Monthly Active User Limit Exceeded", admin_uri=self.hs.config.admin_uri, - errcode=Codes.RESOURCE_LIMIT_EXCEED, + errcode=Codes.RESOURCE_LIMIT_EXCEEDED, limit_type="monthly_active_user" ) diff --git a/synapse/api/errors.py b/synapse/api/errors.py index e26001ab12..c4ddba9889 100644 --- a/synapse/api/errors.py +++ b/synapse/api/errors.py @@ -56,7 +56,7 @@ class Codes(object): SERVER_NOT_TRUSTED = "M_SERVER_NOT_TRUSTED" CONSENT_NOT_GIVEN = "M_CONSENT_NOT_GIVEN" CANNOT_LEAVE_SERVER_NOTICE_ROOM = "M_CANNOT_LEAVE_SERVER_NOTICE_ROOM" - RESOURCE_LIMIT_EXCEED = "M_RESOURCE_LIMIT_EXCEED" + RESOURCE_LIMIT_EXCEEDED = "M_RESOURCE_LIMIT_EXCEEDED" UNSUPPORTED_ROOM_VERSION = "M_UNSUPPORTED_ROOM_VERSION" INCOMPATIBLE_ROOM_VERSION = "M_INCOMPATIBLE_ROOM_VERSION" @@ -238,7 +238,7 @@ class ResourceLimitError(SynapseError): """ def __init__( self, code, msg, - errcode=Codes.RESOURCE_LIMIT_EXCEED, + errcode=Codes.RESOURCE_LIMIT_EXCEEDED, admin_uri=None, limit_type=None, ): diff --git a/tests/api/test_auth.py b/tests/api/test_auth.py index c4cbff4e8d..ed960090c4 100644 --- a/tests/api/test_auth.py +++ b/tests/api/test_auth.py @@ -458,7 +458,7 @@ class AuthTestCase(unittest.TestCase): with self.assertRaises(ResourceLimitError) as e: yield self.auth.check_auth_blocking() self.assertEquals(e.exception.admin_uri, self.hs.config.admin_uri) - self.assertEquals(e.exception.errcode, Codes.RESOURCE_LIMIT_EXCEED) + self.assertEquals(e.exception.errcode, Codes.RESOURCE_LIMIT_EXCEEDED) self.assertEquals(e.exception.code, 403) # Ensure does not throw an error @@ -474,7 +474,7 @@ class AuthTestCase(unittest.TestCase): with self.assertRaises(ResourceLimitError) as e: yield self.auth.check_auth_blocking() self.assertEquals(e.exception.admin_uri, self.hs.config.admin_uri) - self.assertEquals(e.exception.errcode, Codes.RESOURCE_LIMIT_EXCEED) + self.assertEquals(e.exception.errcode, Codes.RESOURCE_LIMIT_EXCEEDED) self.assertEquals(e.exception.code, 403) @defer.inlineCallbacks diff --git a/tests/handlers/test_sync.py b/tests/handlers/test_sync.py index a01ab471f5..31f54bbd7d 100644 --- a/tests/handlers/test_sync.py +++ b/tests/handlers/test_sync.py @@ -51,7 +51,7 @@ class SyncTestCase(tests.unittest.TestCase): self.hs.config.hs_disabled = True with self.assertRaises(ResourceLimitError) as e: yield self.sync_handler.wait_for_sync_for_user(sync_config) - self.assertEquals(e.exception.errcode, Codes.RESOURCE_LIMIT_EXCEED) + self.assertEquals(e.exception.errcode, Codes.RESOURCE_LIMIT_EXCEEDED) self.hs.config.hs_disabled = False @@ -59,7 +59,7 @@ class SyncTestCase(tests.unittest.TestCase): with self.assertRaises(ResourceLimitError) as e: yield self.sync_handler.wait_for_sync_for_user(sync_config) - self.assertEquals(e.exception.errcode, Codes.RESOURCE_LIMIT_EXCEED) + self.assertEquals(e.exception.errcode, Codes.RESOURCE_LIMIT_EXCEEDED) def _generate_sync_config(self, user_id): return SyncConfig( From dd0ac1614cc9649936830bc1efd1f6208528d504 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Tue, 21 Aug 2018 23:35:50 -0600 Subject: [PATCH 19/26] Reference that the federation_reader needs the HTTP replication port set --- docs/workers.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/workers.rst b/docs/workers.rst index aec319dd84..5435f36331 100644 --- a/docs/workers.rst +++ b/docs/workers.rst @@ -74,7 +74,7 @@ replication endpoints that it's talking to on the main synapse process. ``worker_replication_port`` should point to the TCP replication listener port and ``worker_replication_http_port`` should point to the HTTP replication port. -Currently, only the ``event_creator`` worker requires specifying +Currently, the ``event_creator`` and ``federation_reader`` workers require specifying ``worker_replication_http_port``. For instance:: From 365f588d987d531bac9506216f12c44b28c8c5b9 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Tue, 21 Aug 2018 23:38:38 -0600 Subject: [PATCH 20/26] Create 3734.misc --- changelog.d/3734.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/3734.misc diff --git a/changelog.d/3734.misc b/changelog.d/3734.misc new file mode 100644 index 0000000000..4f6e4b3848 --- /dev/null +++ b/changelog.d/3734.misc @@ -0,0 +1 @@ +Reference the need for an HTTP replication port when using the federation_reader worker From 9643a6f7f208f52febc4be7edb1e38f4ff077b4d Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 22 Aug 2018 17:00:29 +0100 Subject: [PATCH 21/26] Update notice format --- synapse/api/constants.py | 6 ++++-- .../resource_limits_server_notices.py | 14 +++++++++++--- .../test_resource_limits_server_notices.py | 12 +++++++++--- 3 files changed, 24 insertions(+), 8 deletions(-) diff --git a/synapse/api/constants.py b/synapse/api/constants.py index 1fb24578e2..a67862f4ed 100644 --- a/synapse/api/constants.py +++ b/synapse/api/constants.py @@ -80,8 +80,6 @@ class EventTypes(object): ServerACL = "m.room.server_acl" Pinned = "m.room.pinned_events" - ServerNoticeLimitReached = "m.server_notice.usage_limit_reached" - class RejectedReason(object): AUTH_ERROR = "auth_error" @@ -106,3 +104,7 @@ DEFAULT_ROOM_VERSION = "1" # vdh-test-version is a placeholder to get room versioning support working and tested # until we have a working v2. KNOWN_ROOM_VERSIONS = {"1", "vdh-test-version"} + + +ServerNoticeMsgType = "m.server_notice" +ServerNoticeLimitReached = "m.server_notice.usage_limit_reached" diff --git a/synapse/server_notices/resource_limits_server_notices.py b/synapse/server_notices/resource_limits_server_notices.py index 84b91aeb5d..575697e54b 100644 --- a/synapse/server_notices/resource_limits_server_notices.py +++ b/synapse/server_notices/resource_limits_server_notices.py @@ -18,7 +18,11 @@ from six import iteritems from twisted.internet import defer -from synapse.api.constants import EventTypes +from synapse.api.constants import ( + EventTypes, + ServerNoticeLimitReached, + ServerNoticeMsgType, +) from synapse.api.errors import AuthError, ResourceLimitError, SynapseError from synapse.server_notices.server_notices_manager import SERVER_NOTICE_ROOM_TAG @@ -102,11 +106,13 @@ class ResourceLimitsServerNotices(object): # Add block notification content = { 'body': event_content, + 'msgtype': ServerNoticeMsgType, + 'server_notice_type': ServerNoticeLimitReached, 'admin_uri': self._config.admin_uri, 'limit_type': event_limit_type } event = yield self._server_notices_manager.send_notice( - user_id, content, EventTypes.ServerNoticeLimitReached + user_id, content, EventTypes.Message, ) content = { @@ -174,7 +180,9 @@ class ResourceLimitsServerNotices(object): events = yield self._store.get_events(referenced_events) for event_id, event in iteritems(events): - if event.type == EventTypes.ServerNoticeLimitReached: + if event.type != EventTypes.Message: + continue + if event.content.get("msgtype") == ServerNoticeMsgType: currently_blocked = True # remove event in case we need to disable blocking later on. if event_id in referenced_events: diff --git a/tests/server_notices/test_resource_limits_server_notices.py b/tests/server_notices/test_resource_limits_server_notices.py index cc8d2f539c..ca9b31128a 100644 --- a/tests/server_notices/test_resource_limits_server_notices.py +++ b/tests/server_notices/test_resource_limits_server_notices.py @@ -2,7 +2,7 @@ from mock import Mock from twisted.internet import defer -from synapse.api.constants import EventTypes +from synapse.api.constants import EventTypes, ServerNoticeMsgType from synapse.api.errors import ResourceLimitError from synapse.handlers.auth import AuthHandler from synapse.server_notices.resource_limits_server_notices import ( @@ -78,7 +78,10 @@ class TestResourceLimitsServerNotices(unittest.TestCase): """Test when user has blocked notice, but should have it removed""" self._rlsn._auth.check_auth_blocking = Mock() - mock_event = Mock(type=EventTypes.ServerNoticeLimitReached) + mock_event = Mock( + type=EventTypes.Message, + content={"msgtype": ServerNoticeMsgType}, + ) self._rlsn._store.get_events = Mock(return_value=defer.succeed( {"123": mock_event} )) @@ -94,7 +97,10 @@ class TestResourceLimitsServerNotices(unittest.TestCase): side_effect=ResourceLimitError(403, 'foo') ) - mock_event = Mock(type=EventTypes.ServerNoticeLimitReached) + mock_event = Mock( + type=EventTypes.Message, + content={"msgtype": ServerNoticeMsgType}, + ) self._rlsn._store.get_events = Mock(return_value=defer.succeed( {"123": mock_event} )) From 7a0da69eee49ce84ddd8a433288d7ce9cb76e14e Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 23 Aug 2018 10:28:10 +0100 Subject: [PATCH 22/26] Add missing yield --- synapse/storage/monthly_active_users.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/synapse/storage/monthly_active_users.py b/synapse/storage/monthly_active_users.py index 06f9a75a97..fd3b630bd2 100644 --- a/synapse/storage/monthly_active_users.py +++ b/synapse/storage/monthly_active_users.py @@ -147,6 +147,7 @@ class MonthlyActiveUsersStore(SQLBaseStore): return count return self.runInteraction("count_users", _count_users) + @defer.inlineCallbacks def upsert_monthly_active_user(self, user_id): """ Updates or inserts monthly active user member @@ -155,7 +156,7 @@ class MonthlyActiveUsersStore(SQLBaseStore): Deferred[bool]: True if a new entry was created, False if an existing one was updated. """ - is_insert = self._simple_upsert( + is_insert = yield self._simple_upsert( desc="upsert_monthly_active_user", table="monthly_active_users", keyvalues={ From c5842dff1a792843365becfa3ca666e42fdce869 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 23 Aug 2018 10:35:54 +0100 Subject: [PATCH 23/26] Actually run the tests --- tests/server_notices/__init__.py | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 tests/server_notices/__init__.py diff --git a/tests/server_notices/__init__.py b/tests/server_notices/__init__.py new file mode 100644 index 0000000000..e69de29bb2 From 4eb8408ed2331bf4a180364e966cb9f93b416202 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 23 Aug 2018 10:46:13 +0100 Subject: [PATCH 24/26] Newsfile --- changelog.d/3746.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/3746.misc diff --git a/changelog.d/3746.misc b/changelog.d/3746.misc new file mode 100644 index 0000000000..fc00ee773a --- /dev/null +++ b/changelog.d/3746.misc @@ -0,0 +1 @@ +Fix MAU cache invalidation due to missing yield From 7e6e588e60708be3e58f026bd65bd4680f9cd969 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 23 Aug 2018 16:20:51 +0100 Subject: [PATCH 25/26] Fix bug where we resent "limit exceeded" server notices This was due to a bug where we mutated a cached event's contents --- .../resource_limits_server_notices.py | 6 +- .../test_resource_limits_server_notices.py | 66 +++++++++++++++++++ 2 files changed, 71 insertions(+), 1 deletion(-) diff --git a/synapse/server_notices/resource_limits_server_notices.py b/synapse/server_notices/resource_limits_server_notices.py index 575697e54b..96eb97771f 100644 --- a/synapse/server_notices/resource_limits_server_notices.py +++ b/synapse/server_notices/resource_limits_server_notices.py @@ -76,6 +76,10 @@ class ResourceLimitsServerNotices(object): room_id = yield self._server_notices_manager.get_notice_room_for_user(user_id) + if not room_id: + logger.warn("Failed to get server notices room") + return + yield self._check_and_set_tags(user_id, room_id) currently_blocked, ref_events = yield self._is_room_currently_blocked(room_id) @@ -176,7 +180,7 @@ class ResourceLimitsServerNotices(object): referenced_events = [] if pinned_state_event is not None: - referenced_events = pinned_state_event.content.get('pinned') + referenced_events = list(pinned_state_event.content.get('pinned', [])) events = yield self._store.get_events(referenced_events) for event_id, event in iteritems(events): diff --git a/tests/server_notices/test_resource_limits_server_notices.py b/tests/server_notices/test_resource_limits_server_notices.py index ca9b31128a..fede3f52a9 100644 --- a/tests/server_notices/test_resource_limits_server_notices.py +++ b/tests/server_notices/test_resource_limits_server_notices.py @@ -144,3 +144,69 @@ class TestResourceLimitsServerNotices(unittest.TestCase): yield self._rlsn.maybe_send_server_notice_to_user(self.user_id) self._send_notice.assert_not_called() + + +class TestResourceLimitsServerNoticesWithRealRooms(unittest.TestCase): + @defer.inlineCallbacks + def setUp(self): + self.hs = yield setup_test_homeserver(self.addCleanup) + self.store = self.hs.get_datastore() + self.server_notices_sender = self.hs.get_server_notices_sender() + self.server_notices_manager = self.hs.get_server_notices_manager() + self.event_source = self.hs.get_event_sources() + + # relying on [1] is far from ideal, but the only case where + # ResourceLimitsServerNotices class needs to be isolated is this test, + # general code should never have a reason to do so ... + self._rlsn = self.server_notices_sender._server_notices[1] + if not isinstance(self._rlsn, ResourceLimitsServerNotices): + raise Exception("Failed to find reference to ResourceLimitsServerNotices") + + self.hs.config.limit_usage_by_mau = True + self.hs.config.hs_disabled = False + self.hs.config.max_mau_value = 5 + self.hs.config.server_notices_mxid = "@server:test" + self.hs.config.server_notices_mxid_display_name = None + self.hs.config.server_notices_mxid_avatar_url = None + self.hs.config.server_notices_room_name = "Test Server Notice Room" + + self.user_id = "@user_id:test" + + self.hs.config.admin_uri = "mailto:user@test.com" + + @defer.inlineCallbacks + def test_server_notice_only_sent_once(self): + self.store.get_monthly_active_count = Mock( + return_value=1000, + ) + + self.store.user_last_seen_monthly_active = Mock( + return_value=1000, + ) + + # Call the function multiple times to ensure we only send the notice once + yield self._rlsn.maybe_send_server_notice_to_user(self.user_id) + yield self._rlsn.maybe_send_server_notice_to_user(self.user_id) + yield self._rlsn.maybe_send_server_notice_to_user(self.user_id) + + # Now lets get the last load of messages in the service notice room and + # check that there is only one server notice + room_id = yield self.server_notices_manager.get_notice_room_for_user( + self.user_id, + ) + + token = yield self.event_source.get_current_token() + events, _ = yield self.store.get_recent_events_for_room( + room_id, limit=100, end_token=token.room_key, + ) + + count = 0 + for event in events: + if event.type != EventTypes.Message: + continue + if event.content.get("msgtype") != ServerNoticeMsgType: + continue + + count += 1 + + self.assertEqual(count, 1) From 60cf1c6e164c153cc98903a2c0cf102825416b03 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 23 Aug 2018 16:34:32 +0100 Subject: [PATCH 26/26] Newsfile --- changelog.d/3747.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/3747.bugfix diff --git a/changelog.d/3747.bugfix b/changelog.d/3747.bugfix new file mode 100644 index 0000000000..c41e2a1213 --- /dev/null +++ b/changelog.d/3747.bugfix @@ -0,0 +1 @@ +Fix bug where we resent "limit exceeded" server notices repeatedly