Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Autocreate autojoin rooms #3975

Merged
merged 16 commits into from
Oct 25, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion changelog.d/3975.feature
Original file line number Diff line number Diff line change
@@ -1 +1 @@
First user should autocreate autojoin rooms
Servers with auto join rooms, should autocreate those rooms when first user registers
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/auto join/auto-join/

s/should/will now/

s/first user/the first user/

possibly, s/autocreate/automatically create/

11 changes: 10 additions & 1 deletion synapse/config/registration.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@

from distutils.util import strtobool

from synapse.config._base import ConfigError
from synapse.types import RoomAlias
from synapse.util.stringutils import random_string_with_symbols

from ._base import Config
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it'd be nice to merge this into line 18 while you're at it (absolute imports are preferred over relative ones)

Expand Down Expand Up @@ -44,6 +46,9 @@ 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally prefer % (room_alias, ) in case room_alias turns out to be a tuple

self.autocreate_auto_join_rooms = config.get("autocreate_auto_join_rooms", True)

def default_config(self, **kwargs):
Expand Down Expand Up @@ -100,7 +105,11 @@ def default_config(self, **kwargs):
#auto_join_rooms:
# - "#example:example.com"

# Have first user on server autocreate autojoin rooms
# Where auto_join_rooms are specified, setting this flag ensures that the
# the rooms exists by creating them when the first user on the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/exists/exist/

# homeserver registers.
# Setting to false means that if the rooms are not manually created,
# users cannot be auto joined since they do not exist.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/auto joined/auto-joined/

autocreate_auto_join_rooms: true
""" % locals()

Expand Down
14 changes: 10 additions & 4 deletions synapse/handlers/register.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
RegistrationError,
SynapseError,
)
from synapse.config._base import ConfigError
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you import this from synapse.config rather than the private _base module, please? (not that it's really appropriate to be raising ConfigErrors at runtime imho)

from synapse.http.client import CaptchaServerHttpClient
from synapse.types import RoomAlias, RoomID, UserID, create_requester
from synapse.util.async_helpers import Linearizer
Expand Down Expand Up @@ -222,14 +223,19 @@ def register(
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()
auto_create_rooms = count == 1
should_auto_create_rooms = count == 1

for r in self.hs.config.auto_join_rooms:
try:
if auto_create_rooms and RoomAlias.is_valid(r):
if should_auto_create_rooms:
room_creation_handler = self.hs.get_room_creation_handler()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you put this in the constructor?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first part needs to be on a request basis - I guess the self.hs.get_room_creation_handler() could but this is the only place it is used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's the only place it's used, but we prefer to get the handlers in the constructors where possible, so that we detect problems earlier.

if self.hs.hostname != RoomAlias.from_string(r).domain:
richvdh marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be nice to do RoomAlias.from_string(r) once and use a local var, rather than twice

raise ConfigError(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and if we can't check it during startup, it's probably more appropriate to warn and move on than to 500 reject the whole registration.

'Cannot create room alias %s, it does not match server domain'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is missing its argument too.

)
# create room expects the localpart of the room alias
room_alias_localpart = RoomAlias.from_string(r).localpart
richvdh marked this conversation as resolved.
Show resolved Hide resolved
yield room_creation_handler.create_room(
Expand All @@ -240,7 +246,8 @@ def register(
},
ratelimit=False,
)
yield self._join_user_to_room(fake_requester, r)
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)

Expand Down Expand Up @@ -531,7 +538,6 @@ def get_or_register_3pid_guest(self, medium, address, inviter_user_id):

@defer.inlineCallbacks
def _join_user_to_room(self, requester, room_identifier):

room_id = None
room_member_handler = self.hs.get_room_member_handler()
if RoomID.is_valid(room_identifier):
Expand Down
21 changes: 17 additions & 4 deletions tests/handlers/test_register.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ def setUp(self):
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
Expand Down Expand Up @@ -148,9 +147,7 @@ def test_register_saml2_mau_blocked(self):
@defer.inlineCallbacks
def test_auto_create_auto_join_rooms(self):
room_alias_str = "#room:test"
self.hs.config.autocreate_auto_join_rooms = True
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])

Expand All @@ -163,11 +160,27 @@ def test_auto_create_auto_join_rooms(self):

@defer.inlineCallbacks
def test_auto_create_auto_join_rooms_with_no_rooms(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we also have a UT with autocreate_auto_join_rooms=False please?

self.hs.config.autocreate_auto_join_rooms = True
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

erm, why does this not throw an exception?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bare except in the try statement
Have since removed the exception as per comments above

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)