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 7 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
1 change: 1 addition & 0 deletions changelog.d/3975.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
First user should autocreate autojoin rooms
Copy link
Member

Choose a reason for hiding this comment

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

could you expand on this a bit so that it makes sense for users, please?

4 changes: 4 additions & 0 deletions synapse/config/registration.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ def read_config(self, config):
)

self.auto_join_rooms = config.get("auto_join_rooms", [])
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)
Expand Down Expand Up @@ -98,6 +99,9 @@ def default_config(self, **kwargs):
# to these rooms
#auto_join_rooms:
# - "#example:example.com"

# Have first user on server autocreate autojoin rooms
Copy link
Member

Choose a reason for hiding this comment

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

maybe explain what will happen if it is false, and conversely what happens if it is true?

autocreate_auto_join_rooms: true
""" % locals()

def add_arguments(self, parser):
Expand Down
19 changes: 19 additions & 0 deletions synapse/handlers/register.py
Original file line number Diff line number Diff line change
Expand Up @@ -220,8 +220,26 @@ 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
if self.hs.config.autocreate_auto_join_rooms:
count = yield self.store.count_all_users()
auto_create_rooms = count == 1
Copy link
Member

Choose a reason for hiding this comment

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

you need to initialise auto_create_rooms for the case where autocreate_auto_join_rooms is False.


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

Choose a reason for hiding this comment

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

if the alias is invalid, we silently ignore it? If you're going to sanity check it, put the sanity check in the config module, and raise a ConfigError if it's wrong.

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.

# 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(
fake_requester,
config={
"preset": "public_chat",
"room_alias_name": room_alias_localpart
},
ratelimit=False,
)
yield self._join_user_to_room(fake_requester, r)
Copy link
Member

Choose a reason for hiding this comment

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

I think if you've created the room, you don't need to join it.

except Exception as e:
logger.error("Failed to join new user to %r: %r", r, e)
Expand Down Expand Up @@ -513,6 +531,7 @@ def get_or_register_3pid_guest(self, medium, address, inviter_user_id):

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

Copy link
Member

Choose a reason for hiding this comment

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

please no

Copy link
Contributor

Choose a reason for hiding this comment

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

it was oversight I promise!

room_id = None
room_member_handler = self.hs.get_room_member_handler()
if RoomID.is_valid(room_identifier):
Expand Down
2 changes: 1 addition & 1 deletion synapse/storage/directory.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ def get_aliases_for_room(self, room_id):
class DirectoryStore(DirectoryWorkerStore):
@defer.inlineCallbacks
def create_room_alias_association(self, room_alias, room_id, servers, creator=None):
""" Creates an associatin between a room alias and room_id/servers
""" Creates an association between a room alias and room_id/servers

Args:
room_alias (RoomAlias)
Expand Down
62 changes: 43 additions & 19 deletions tests/handlers/test_register.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -41,30 +41,28 @@ 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.hs.handlers = RegistrationHandlers(self.hs)
Copy link
Member

Choose a reason for hiding this comment

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

if it's redundant, remove it, don't comment it out

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')
Expand All @@ -78,12 +76,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')
Expand All @@ -92,7 +89,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):
Expand All @@ -101,7 +98,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):
Expand All @@ -110,13 +107,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):
Expand Down Expand Up @@ -147,3 +144,30 @@ 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.autocreate_auto_join_rooms = True
Copy link
Member

Choose a reason for hiding this comment

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

this seems to be the default; you could argue it should be set here for certainty but in that case let's have a comment. else just remove it

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):
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)
1 change: 1 addition & 0 deletions tests/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ def default_config(name):
config.user_consent_server_notice_content = None
config.block_events_without_consent_error = None
config.media_storage_providers = []
config.autocreate_auto_join_rooms = True
config.auto_join_rooms = []
config.limit_usage_by_mau = False
config.hs_disabled = False
Expand Down