diff --git a/changelog.d/4141.feature b/changelog.d/4141.feature new file mode 100644 index 000000000000..34ec4115a703 --- /dev/null +++ b/changelog.d/4141.feature @@ -0,0 +1 @@ +Special case a support user for use in verifying user behaviour of a given server, the support user does not appear in user directory or monthly active user counts. diff --git a/changelog.d/4233.bugfix b/changelog.d/4233.bugfix new file mode 100644 index 000000000000..eb567cc03eaf --- /dev/null +++ b/changelog.d/4233.bugfix @@ -0,0 +1 @@ +Fix synapse_admin_mau:current metric is not updating when config.mau_stats_only is True diff --git a/synapse/api/auth.py b/synapse/api/auth.py index 34382e4e3c8d..f3a9aeb93462 100644 --- a/synapse/api/auth.py +++ b/synapse/api/auth.py @@ -791,9 +791,10 @@ def check_auth_blocking(self, user_id=None, threepid=None): threepid should never be set at the same time. """ - # Never fail an auth check for the server notices users + # Never fail an auth check for the server notices users or support user # This can be a problem where event creation is prohibited due to blocking - if user_id == self.hs.config.server_notices_mxid: + is_support = yield self.store.is_support_user(user_id) + if user_id == self.hs.config.server_notices_mxid or is_support: return if self.hs.config.hs_disabled: diff --git a/synapse/api/constants.py b/synapse/api/constants.py index f20e0fcf0bfb..a9fb6e73623e 100644 --- a/synapse/api/constants.py +++ b/synapse/api/constants.py @@ -119,3 +119,9 @@ class RoomVersions(object): ServerNoticeMsgType = "m.server_notice" ServerNoticeLimitReached = "m.server_notice.usage_limit_reached" + + +# Allows for user type specific behaviour, if we'd had a crystal ball would +# probably have included admin and guest, normal users are type None +class UserTypes(object): + SUPPORT = "support" diff --git a/synapse/app/homeserver.py b/synapse/app/homeserver.py index 3e4dea2f196a..ed6f84f4f4db 100755 --- a/synapse/app/homeserver.py +++ b/synapse/app/homeserver.py @@ -549,7 +549,7 @@ def start_generate_monthly_active_users(): ) start_generate_monthly_active_users() - if hs.config.limit_usage_by_mau: + if hs.config.limit_usage_by_mau or hs.config.mau_stats_only: clock.looping_call(start_generate_monthly_active_users, 5 * 60 * 1000) # End of monthly active user settings diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 015909bb26b4..8341ee1ccffa 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -233,9 +233,16 @@ def _auto_join_rooms(self, user_id): # auto-join the user to any rooms we're supposed to dump them into fake_requester = create_requester(user_id) - # try to create the room if we're the first user on the server + # try to create the room if we're the first real user on the server. Note + # that an auto generated support user is not a real user and never be + # the user to create the room should_auto_create_rooms = False - if self.hs.config.autocreate_auto_join_rooms: + is_support = yield self.store.is_support_user(user_id) + # There is an edge case where the first user is the support user, then + # the room is never created, though this seems unlikely and + # recoverable from given the support user being involved in the first + # place. + if (self.hs.config.autocreate_auto_join_rooms and not is_support): count = yield self.store.count_all_users() should_auto_create_rooms = count == 1 for r in self.hs.config.auto_join_rooms: diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py index f11b430126e3..fbfaea8aeb71 100644 --- a/synapse/handlers/user_directory.py +++ b/synapse/handlers/user_directory.py @@ -125,9 +125,11 @@ def handle_local_profile_change(self, user_id, profile): """ # FIXME(#3714): We should probably do this in the same worker as all # the other changes. - yield self.store.update_profile_in_user_dir( - user_id, profile.display_name, profile.avatar_url, None, - ) + is_support = yield self.store.is_support_user(user_id) + if not is_support: + yield self.store.update_profile_in_user_dir( + user_id, profile.display_name, profile.avatar_url, None, + ) @defer.inlineCallbacks def handle_user_deactivated(self, user_id): @@ -135,8 +137,10 @@ def handle_user_deactivated(self, user_id): """ # FIXME(#3714): We should probably do this in the same worker as all # the other changes. - yield self.store.remove_from_user_dir(user_id) - yield self.store.remove_from_user_in_public_room(user_id) + is_support = yield self.store.is_support_user(user_id) + if not is_support: + yield self.store.remove_from_user_dir(user_id) + yield self.store.remove_from_user_in_public_room(user_id) @defer.inlineCallbacks def _unsafe_process(self): @@ -329,13 +333,6 @@ def _handle_deltas(self, deltas): public_value=Membership.JOIN, ) - if change is None: - # Handle any profile changes - yield self._handle_profile_change( - state_key, room_id, prev_event_id, event_id, - ) - continue - if not change: # Need to check if the server left the room entirely, if so # we might need to remove all the users in that room @@ -349,21 +346,32 @@ def _handle_deltas(self, deltas): # need to remove those users or not user_ids = yield self.store.get_users_in_dir_due_to_room(room_id) for user_id in user_ids: - yield self._handle_remove_user(room_id, user_id) + is_support = yield self.store.is_support_user(state_key) + if not is_support: + yield self._handle_remove_user(room_id, user_id) return else: logger.debug("Server is still in room: %r", room_id) - if change: # The user joined - event = yield self.store.get_event(event_id, allow_none=True) - profile = ProfileInfo( - avatar_url=event.content.get("avatar_url"), - display_name=event.content.get("displayname"), - ) + is_support = yield self.store.is_support_user(state_key) + if not is_support: + if change is None: + # Handle any profile changes + yield self._handle_profile_change( + state_key, room_id, prev_event_id, event_id, + ) + continue + + if change: # The user joined + event = yield self.store.get_event(event_id, allow_none=True) + profile = ProfileInfo( + avatar_url=event.content.get("avatar_url"), + display_name=event.content.get("displayname"), + ) - yield self._handle_new_user(room_id, state_key, profile) - else: # The user left - yield self._handle_remove_user(room_id, state_key) + yield self._handle_new_user(room_id, state_key, profile) + else: # The user left + yield self._handle_remove_user(room_id, state_key) else: logger.debug("Ignoring irrelevant type: %r", typ) diff --git a/synapse/storage/monthly_active_users.py b/synapse/storage/monthly_active_users.py index 479e01ddc16e..e08c111253e8 100644 --- a/synapse/storage/monthly_active_users.py +++ b/synapse/storage/monthly_active_users.py @@ -182,6 +182,18 @@ def upsert_monthly_active_user(self, user_id): Args: user_id (str): user to add/update """ + # Support user never to be included in MAU stats. Note I can't easily call this + # from upsert_monthly_active_user_txn because then I need a _txn form of + # is_support_user which is complicated because I want to cache the result. + # Therefore I call it here and ignore the case where + # upsert_monthly_active_user_txn is called directly from + # _initialise_reserved_users reasoning that it would be very strange to + # include a support user in this context. + + is_support = yield self.is_support_user(user_id) + if is_support: + return + is_insert = yield self.runInteraction( "upsert_monthly_active_user", self.upsert_monthly_active_user_txn, user_id @@ -208,6 +220,7 @@ def upsert_monthly_active_user_txn(self, txn, user_id): bool: True if a new entry was created, False if an existing one was updated. """ + # Am consciously deciding to lock the table on the basis that is ought # never be a big table and alternative approaches (batching multiple # upserts into a single txn) introduced a lot of extra complexity. diff --git a/synapse/storage/prepare_database.py b/synapse/storage/prepare_database.py index d5d2f89a7729..fa36daac524d 100644 --- a/synapse/storage/prepare_database.py +++ b/synapse/storage/prepare_database.py @@ -25,7 +25,7 @@ # Remember to update this number every time a change is made to database # schema files, so the users will be informed on server restarts. -SCHEMA_VERSION = 52 +SCHEMA_VERSION = 53 dir_path = os.path.abspath(os.path.dirname(__file__)) diff --git a/synapse/storage/registration.py b/synapse/storage/registration.py index 80d76bf9d789..2c6a417c79ad 100644 --- a/synapse/storage/registration.py +++ b/synapse/storage/registration.py @@ -19,6 +19,7 @@ from twisted.internet import defer +from synapse.api.constants import UserTypes from synapse.api.errors import Codes, StoreError from synapse.storage import background_updates from synapse.storage._base import SQLBaseStore @@ -167,7 +168,7 @@ def add_access_token_to_user(self, user_id, token, device_id=None): def register(self, user_id, token=None, password_hash=None, was_guest=False, make_guest=False, appservice_id=None, - create_profile_with_localpart=None, admin=False): + create_profile_with_localpart=None, admin=False, user_type=None): """Attempts to register an account. Args: @@ -183,6 +184,8 @@ def register(self, user_id, token=None, password_hash=None, appservice_id (str): The ID of the appservice registering the user. create_profile_with_localpart (str): Optionally create a profile for the given localpart. + admin (boolean): is an admin user? + user_type (synapse.api.constants import UserTypes): type of user Raises: StoreError if the user_id could not be registered. """ @@ -196,7 +199,8 @@ def register(self, user_id, token=None, password_hash=None, make_guest, appservice_id, create_profile_with_localpart, - admin + admin, + user_type ) def _register( @@ -210,6 +214,7 @@ def _register( appservice_id, create_profile_with_localpart, admin, + user_type, ): now = int(self.clock.time()) @@ -244,6 +249,7 @@ def _register( "is_guest": 1 if make_guest else 0, "appservice_id": appservice_id, "admin": 1 if admin else 0, + "user_type": user_type } ) else: @@ -257,6 +263,7 @@ def _register( "is_guest": 1 if make_guest else 0, "appservice_id": appservice_id, "admin": 1 if admin else 0, + "user_type": user_type, } ) except self.database_engine.module.IntegrityError: @@ -450,6 +457,18 @@ def is_guest(self, user_id): defer.returnValue(res if res else False) + @cachedInlineCallbacks() + def is_support_user(self, user_id): + res = yield self._simple_select_one_onecol( + table="users", + keyvalues={"name": user_id}, + retcol="user_type", + allow_none=True, + desc="is_support_user", + ) + + defer.returnValue(True if res == UserTypes.SUPPORT else False) + @defer.inlineCallbacks def user_add_threepid(self, user_id, medium, address, validated_at, added_at): yield self._simple_upsert("user_threepids", { diff --git a/synapse/storage/schema/delta/53/add_user_type_to_users.sql b/synapse/storage/schema/delta/53/add_user_type_to_users.sql new file mode 100644 index 000000000000..9044e0152c07 --- /dev/null +++ b/synapse/storage/schema/delta/53/add_user_type_to_users.sql @@ -0,0 +1,20 @@ +/* Copyright 2018 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/* record whether we have sent a server notice about consenting to the + * privacy policy. Specifically records the version of the policy we sent + * a message about. + */ +ALTER TABLE users ADD COLUMN user_type TEXT DEFAULT NULL; diff --git a/tests/api/test_auth.py b/tests/api/test_auth.py index 379e9c4ab198..69dc40428b1b 100644 --- a/tests/api/test_auth.py +++ b/tests/api/test_auth.py @@ -50,6 +50,8 @@ def setUp(self): # this is overridden for the appservice tests self.store.get_app_service_by_token = Mock(return_value=None) + self.store.is_support_user = Mock(return_value=defer.succeed(False)) + @defer.inlineCallbacks def test_get_user_by_req_user_valid_token(self): user_info = {"name": self.test_user, "token_id": "ditto", "device_id": "device"} diff --git a/tests/handlers/test_register.py b/tests/handlers/test_register.py index 90a2a7647512..84d89b3c54fe 100644 --- a/tests/handlers/test_register.py +++ b/tests/handlers/test_register.py @@ -17,7 +17,7 @@ from twisted.internet import defer -from synapse.api.errors import ResourceLimitError +from synapse.api.errors import ResourceLimitError, SynapseError from synapse.handlers.register import RegistrationHandler from synapse.types import RoomAlias, UserID, create_requester @@ -43,10 +43,6 @@ def setUp(self): self.addCleanup, expire_access_token=True, ) - self.macaroon_generator = Mock( - generate_access_token=Mock(return_value='secret') - ) - self.hs.get_macaroon_generator = Mock(return_value=self.macaroon_generator) self.handler = self.hs.get_handlers().registration_handler self.store = self.hs.get_datastore() self.hs.config.max_mau_value = 50 @@ -64,7 +60,7 @@ def test_user_is_created_and_logged_in_if_doesnt_exist(self): requester, frank.localpart, "Frankie" ) self.assertEquals(result_user_id, user_id) - self.assertEquals(result_token, 'secret') + self.assertTrue(result_token is not None) @defer.inlineCallbacks def test_if_user_exists(self): @@ -82,7 +78,7 @@ def test_if_user_exists(self): requester, local_part, None ) self.assertEquals(result_user_id, user_id) - self.assertEquals(result_token, 'secret') + self.assertTrue(result_token is not None) @defer.inlineCallbacks def test_mau_limits_when_disabled(self): @@ -184,6 +180,20 @@ def test_auto_create_auto_join_where_auto_create_is_false(self): rooms = yield self.store.get_rooms_for_user(res[0]) self.assertEqual(len(rooms), 0) + @defer.inlineCallbacks + def test_auto_create_auto_join_rooms_when_support_user_exists(self): + room_alias_str = "#room:test" + self.hs.config.auto_join_rooms = [room_alias_str] + + self.store.is_support_user = Mock(return_value=True) + res = yield self.handler.register(localpart='support') + rooms = yield self.store.get_rooms_for_user(res[0]) + self.assertEqual(len(rooms), 0) + directory_handler = self.hs.get_handlers().directory_handler + room_alias = RoomAlias.from_string(room_alias_str) + with self.assertRaises(SynapseError): + yield directory_handler.get_association(room_alias) + @defer.inlineCallbacks def test_auto_create_auto_join_where_no_consent(self): self.hs.config.user_consent_at_registration = True diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py new file mode 100644 index 000000000000..11f2bae69808 --- /dev/null +++ b/tests/handlers/test_user_directory.py @@ -0,0 +1,91 @@ +# -*- coding: utf-8 -*- +# Copyright 2018 New Vector +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +from mock import Mock + +from twisted.internet import defer + +from synapse.api.constants import UserTypes +from synapse.handlers.user_directory import UserDirectoryHandler +from synapse.storage.roommember import ProfileInfo + +from tests import unittest +from tests.utils import setup_test_homeserver + + +class UserDirectoryHandlers(object): + def __init__(self, hs): + self.user_directory_handler = UserDirectoryHandler(hs) + + +class UserDirectoryTestCase(unittest.TestCase): + """ Tests the UserDirectoryHandler. """ + + @defer.inlineCallbacks + def setUp(self): + hs = yield setup_test_homeserver(self.addCleanup) + self.store = hs.get_datastore() + hs.handlers = UserDirectoryHandlers(hs) + + self.handler = hs.get_handlers().user_directory_handler + + @defer.inlineCallbacks + def test_handle_local_profile_change_with_support_user(self): + support_user_id = "@support:test" + yield self.store.register( + user_id=support_user_id, + token="123", + password_hash=None, + user_type=UserTypes.SUPPORT + ) + + yield self.handler.handle_local_profile_change(support_user_id, None) + profile = yield self.store.get_user_in_directory(support_user_id) + self.assertTrue(profile is None) + display_name = 'display_name' + + profile_info = ProfileInfo( + avatar_url='avatar_url', + display_name=display_name, + ) + regular_user_id = '@regular:test' + yield self.handler.handle_local_profile_change(regular_user_id, profile_info) + profile = yield self.store.get_user_in_directory(regular_user_id) + self.assertTrue(profile['display_name'] == display_name) + + @defer.inlineCallbacks + def test_handle_user_deactivated_support_user(self): + s_user_id = "@support:test" + self.store.register( + user_id=s_user_id, + token="123", + password_hash=None, + user_type=UserTypes.SUPPORT + ) + + self.store.remove_from_user_dir = Mock() + self.store.remove_from_user_in_public_room = Mock() + yield self.handler.handle_user_deactivated(s_user_id) + self.store.remove_from_user_dir.not_called() + self.store.remove_from_user_in_public_room.not_called() + + @defer.inlineCallbacks + def test_handle_user_deactivated_regular_user(self): + r_user_id = "@regular:test" + self.store.register(user_id=r_user_id, token="123", password_hash=None) + self.store.remove_from_user_dir = Mock() + self.store.remove_from_user_in_public_room = Mock() + yield self.handler.handle_user_deactivated(r_user_id) + self.store.remove_from_user_dir.called_once_with(r_user_id) + self.store.remove_from_user_in_public_room.assert_called_once_with(r_user_id) diff --git a/tests/storage/test_monthly_active_users.py b/tests/storage/test_monthly_active_users.py index 8664bc3d54fc..a1a02ab61215 100644 --- a/tests/storage/test_monthly_active_users.py +++ b/tests/storage/test_monthly_active_users.py @@ -16,6 +16,8 @@ from twisted.internet import defer +from synapse.api.constants import UserTypes + from tests.unittest import HomeserverTestCase FORTY_DAYS = 40 * 24 * 60 * 60 @@ -28,6 +30,7 @@ def make_homeserver(self, reactor, clock): self.store = hs.get_datastore() hs.config.limit_usage_by_mau = True hs.config.max_mau_value = 50 + # Advance the clock a bit reactor.advance(FORTY_DAYS) @@ -221,6 +224,24 @@ def test_get_reserved_real_user_account(self): count = self.store.get_registered_reserved_users_count() self.assertEquals(self.get_success(count), len(threepids)) + def test_support_user_not_add_to_mau_limits(self): + support_user_id = "@support:test" + count = self.store.get_monthly_active_count() + self.pump() + self.assertEqual(self.get_success(count), 0) + + self.store.register( + user_id=support_user_id, + token="123", + password_hash=None, + user_type=UserTypes.SUPPORT + ) + + self.store.upsert_monthly_active_user(support_user_id) + count = self.store.get_monthly_active_count() + self.pump() + self.assertEqual(self.get_success(count), 0) + def test_track_monthly_users_without_cap(self): self.hs.config.limit_usage_by_mau = False self.hs.config.mau_stats_only = True diff --git a/tests/storage/test_registration.py b/tests/storage/test_registration.py index 3dfb7b903a3a..421713cb32a2 100644 --- a/tests/storage/test_registration.py +++ b/tests/storage/test_registration.py @@ -16,6 +16,8 @@ from twisted.internet import defer +from synapse.api.constants import UserTypes + from tests import unittest from tests.utils import setup_test_homeserver @@ -99,6 +101,26 @@ def test_user_delete_access_tokens(self): user = yield self.store.get_user_by_access_token(self.tokens[0]) self.assertIsNone(user, "access token was not deleted without device_id") + @defer.inlineCallbacks + def test_is_support_user(self): + TEST_USER = "@test:test" + SUPPORT_USER = "@support:test" + + res = yield self.store.is_support_user(None) + self.assertFalse(res) + yield self.store.register(user_id=TEST_USER, token="123", password_hash=None) + res = yield self.store.is_support_user(TEST_USER) + self.assertFalse(res) + + self.store.register( + user_id=SUPPORT_USER, + token="456", + password_hash=None, + user_type=UserTypes.SUPPORT + ) + res = yield self.store.is_support_user(SUPPORT_USER) + self.assertTrue(res) + class TokenGenerator: def __init__(self): diff --git a/tests/utils.py b/tests/utils.py index 52ab76201045..7771da242ecb 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -139,7 +139,6 @@ def default_config(name): config.admin_contact = None config.rc_messages_per_second = 10000 config.rc_message_burst_count = 10000 - config.use_frozen_dicts = False # we need a sane default_room_version, otherwise attempts to create rooms will