-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Autocreate autojoin rooms #3975
Changes from 10 commits
07340cd
8f646f2
5b68f29
23b6a05
faa462e
2dadc09
ed82043
a2bfb77
1ccafb0
c6584f4
a67d8ac
9f72c20
94a49e0
9532caf
9ec2186
f7f487e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Servers with auto-join rooms, should automatically create those rooms when first user registers | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,9 +15,10 @@ | |
|
||
from distutils.util import strtobool | ||
|
||
from synapse.types import RoomAlias | ||
from synapse.util.stringutils import random_string_with_symbols | ||
|
||
from ._base import Config | ||
from ._base import Config, ConfigError | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. again, absolute imports are preferred over relative ones. Please There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. right, misunderstood you |
||
|
||
|
||
class RegistrationConfig(Config): | ||
|
@@ -44,6 +45,10 @@ def read_config(self, config): | |
) | ||
|
||
self.auto_join_rooms = config.get("auto_join_rooms", []) | ||
for room_alias in self.auto_join_rooms: | ||
if not RoomAlias.is_valid(room_alias): | ||
raise ConfigError('Invalid auto_join_rooms entry %s' % (room_alias,)) | ||
self.autocreate_auto_join_rooms = config.get("autocreate_auto_join_rooms", True) | ||
|
||
def default_config(self, **kwargs): | ||
registration_shared_secret = random_string_with_symbols(50) | ||
|
@@ -98,6 +103,13 @@ def default_config(self, **kwargs): | |
# to these rooms | ||
#auto_join_rooms: | ||
# - "#example:example.com" | ||
|
||
# Where auto_join_rooms are specified, setting this flag ensures that the | ||
# the rooms exist by creating them when the first user on the | ||
# homeserver registers. | ||
# Setting to false means that if the rooms are not manually created, | ||
# users cannot be auto-joined since they do not exist. | ||
autocreate_auto_join_rooms: true | ||
""" % locals() | ||
|
||
def add_arguments(self, parser): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,6 +50,7 @@ def __init__(self, hs): | |
self._auth_handler = hs.get_auth_handler() | ||
self.profile_handler = hs.get_profile_handler() | ||
self.user_directory_handler = hs.get_user_directory_handler() | ||
self.room_creation_handler = self.hs.get_room_creation_handler() | ||
self.captcha_client = CaptchaServerHttpClient(hs) | ||
|
||
self._next_generated_user_id = None | ||
|
@@ -220,9 +221,35 @@ def register( | |
|
||
# auto-join the user to any rooms we're supposed to dump them into | ||
fake_requester = create_requester(user_id) | ||
|
||
# try to create the room if we're the first user on the server | ||
should_auto_create_rooms = False | ||
if self.hs.config.autocreate_auto_join_rooms: | ||
count = yield self.store.count_all_users() | ||
should_auto_create_rooms = count == 1 | ||
|
||
for r in self.hs.config.auto_join_rooms: | ||
try: | ||
yield self._join_user_to_room(fake_requester, r) | ||
if should_auto_create_rooms: | ||
if self.hs.hostname != RoomAlias.from_string(r).domain: | ||
richvdh marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. might be nice to do |
||
logger.warn( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fwiw (just a nit fyi): |
||
'Cannot create room alias %s, ' | ||
'it does not match server domain' % (r,) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the logger methods automatically interpolate their arguments (with the benefit of doing it after checking if logging is enabled), so, you can write:
rather than using % explicitly |
||
) | ||
raise SynapseError() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are we raising an exception here (and why does it have no message, but that's a side-issue) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is brain dead, sorry |
||
else: | ||
# create room expects the localpart of the room alias | ||
room_alias_localpart = RoomAlias.from_string(r).localpart | ||
yield self.room_creation_handler.create_room( | ||
fake_requester, | ||
config={ | ||
"preset": "public_chat", | ||
"room_alias_name": room_alias_localpart | ||
}, | ||
ratelimit=False, | ||
) | ||
else: | ||
yield self._join_user_to_room(fake_requester, r) | ||
except Exception as e: | ||
logger.error("Failed to join new user to %r: %r", r, e) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,7 +19,7 @@ | |
|
||
from synapse.api.errors import ResourceLimitError | ||
from synapse.handlers.register import RegistrationHandler | ||
from synapse.types import UserID, create_requester | ||
from synapse.types import RoomAlias, UserID, create_requester | ||
|
||
from tests.utils import setup_test_homeserver | ||
|
||
|
@@ -41,30 +41,27 @@ def setUp(self): | |
self.mock_captcha_client = Mock() | ||
self.hs = yield setup_test_homeserver( | ||
self.addCleanup, | ||
handlers=None, | ||
http_client=None, | ||
expire_access_token=True, | ||
profile_handler=Mock(), | ||
) | ||
self.macaroon_generator = Mock( | ||
generate_access_token=Mock(return_value='secret') | ||
) | ||
self.hs.get_macaroon_generator = Mock(return_value=self.macaroon_generator) | ||
self.hs.handlers = RegistrationHandlers(self.hs) | ||
self.handler = self.hs.get_handlers().registration_handler | ||
self.store = self.hs.get_datastore() | ||
self.hs.config.max_mau_value = 50 | ||
self.lots_of_users = 100 | ||
self.small_number_of_users = 1 | ||
|
||
self.requester = create_requester("@requester:test") | ||
|
||
@defer.inlineCallbacks | ||
def test_user_is_created_and_logged_in_if_doesnt_exist(self): | ||
local_part = "someone" | ||
display_name = "someone" | ||
user_id = "@someone:test" | ||
requester = create_requester("@as:test") | ||
frank = UserID.from_string("@frank:test") | ||
user_id = frank.to_string() | ||
requester = create_requester(user_id) | ||
result_user_id, result_token = yield self.handler.get_or_create_user( | ||
requester, local_part, display_name | ||
requester, frank.localpart, "Frankie" | ||
) | ||
self.assertEquals(result_user_id, user_id) | ||
self.assertEquals(result_token, 'secret') | ||
|
@@ -78,12 +75,11 @@ def test_if_user_exists(self): | |
token="jkv;g498752-43gj['eamb!-5", | ||
password_hash=None, | ||
) | ||
local_part = "frank" | ||
display_name = "Frank" | ||
user_id = "@frank:test" | ||
requester = create_requester("@as:test") | ||
local_part = frank.localpart | ||
user_id = frank.to_string() | ||
requester = create_requester(user_id) | ||
result_user_id, result_token = yield self.handler.get_or_create_user( | ||
requester, local_part, display_name | ||
requester, local_part, None | ||
) | ||
self.assertEquals(result_user_id, user_id) | ||
self.assertEquals(result_token, 'secret') | ||
|
@@ -92,7 +88,7 @@ def test_if_user_exists(self): | |
def test_mau_limits_when_disabled(self): | ||
self.hs.config.limit_usage_by_mau = False | ||
# Ensure does not throw exception | ||
yield self.handler.get_or_create_user("requester", 'a', "display_name") | ||
yield self.handler.get_or_create_user(self.requester, 'a', "display_name") | ||
|
||
@defer.inlineCallbacks | ||
def test_get_or_create_user_mau_not_blocked(self): | ||
|
@@ -101,7 +97,7 @@ def test_get_or_create_user_mau_not_blocked(self): | |
return_value=defer.succeed(self.hs.config.max_mau_value - 1) | ||
) | ||
# Ensure does not throw exception | ||
yield self.handler.get_or_create_user("@user:server", 'c', "User") | ||
yield self.handler.get_or_create_user(self.requester, 'c', "User") | ||
|
||
@defer.inlineCallbacks | ||
def test_get_or_create_user_mau_blocked(self): | ||
|
@@ -110,13 +106,13 @@ def test_get_or_create_user_mau_blocked(self): | |
return_value=defer.succeed(self.lots_of_users) | ||
) | ||
with self.assertRaises(ResourceLimitError): | ||
yield self.handler.get_or_create_user("requester", 'b', "display_name") | ||
yield self.handler.get_or_create_user(self.requester, 'b', "display_name") | ||
|
||
self.store.get_monthly_active_count = Mock( | ||
return_value=defer.succeed(self.hs.config.max_mau_value) | ||
) | ||
with self.assertRaises(ResourceLimitError): | ||
yield self.handler.get_or_create_user("requester", 'b', "display_name") | ||
yield self.handler.get_or_create_user(self.requester, 'b', "display_name") | ||
|
||
@defer.inlineCallbacks | ||
def test_register_mau_blocked(self): | ||
|
@@ -147,3 +143,44 @@ def test_register_saml2_mau_blocked(self): | |
) | ||
with self.assertRaises(ResourceLimitError): | ||
yield self.handler.register_saml2(localpart="local_part") | ||
|
||
@defer.inlineCallbacks | ||
def test_auto_create_auto_join_rooms(self): | ||
room_alias_str = "#room:test" | ||
self.hs.config.auto_join_rooms = [room_alias_str] | ||
res = yield self.handler.register(localpart='jeff') | ||
rooms = yield self.store.get_rooms_for_user(res[0]) | ||
|
||
directory_handler = self.hs.get_handlers().directory_handler | ||
room_alias = RoomAlias.from_string(room_alias_str) | ||
room_id = yield directory_handler.get_association(room_alias) | ||
|
||
self.assertTrue(room_id['room_id'] in rooms) | ||
self.assertEqual(len(rooms), 1) | ||
|
||
@defer.inlineCallbacks | ||
def test_auto_create_auto_join_rooms_with_no_rooms(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we also have a UT with |
||
self.hs.config.auto_join_rooms = [] | ||
frank = UserID.from_string("@frank:test") | ||
res = yield self.handler.register(frank.localpart) | ||
self.assertEqual(res[0], frank.to_string()) | ||
rooms = yield self.store.get_rooms_for_user(res[0]) | ||
self.assertEqual(len(rooms), 0) | ||
|
||
@defer.inlineCallbacks | ||
def test_auto_create_auto_join_where_room_is_another_domain(self): | ||
self.hs.config.auto_join_rooms = ["#room:another"] | ||
frank = UserID.from_string("@frank:test") | ||
res = yield self.handler.register(frank.localpart) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. erm, why does this not throw an exception? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. bare except in the try statement |
||
self.assertEqual(res[0], frank.to_string()) | ||
rooms = yield self.store.get_rooms_for_user(res[0]) | ||
self.assertEqual(len(rooms), 0) | ||
|
||
@defer.inlineCallbacks | ||
def test_auto_create_auto_join_where_auto_create_is_false(self): | ||
self.hs.config.autocreate_auto_join_rooms = False | ||
room_alias_str = "#room:test" | ||
self.hs.config.auto_join_rooms = [room_alias_str] | ||
res = yield self.handler.register(localpart='jeff') | ||
rooms = yield self.store.get_rooms_for_user(res[0]) | ||
self.assertEqual(len(rooms), 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still some problems here:
s/, should/will now/
s/first user/the first user/