-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Autocreate autojoin rooms #3975
Changes from 7 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 @@ | ||
First user should autocreate autojoin rooms | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
|
@@ -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 | ||
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. 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): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
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. you need to initialise |
||
|
||
for r in self.hs.config.auto_join_rooms: | ||
try: | ||
if auto_create_rooms and RoomAlias.is_valid(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. 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() | ||
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 you put this in the constructor? 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 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. 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. 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) | ||
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. 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) | ||
|
@@ -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): | ||
|
||
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. please no 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. it was oversight I promise! |
||
room_id = None | ||
room_member_handler = self.hs.get_room_member_handler() | ||
if RoomID.is_valid(room_identifier): | ||
|
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,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) | ||
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. 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') | ||
|
@@ -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') | ||
|
@@ -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): | ||
|
@@ -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): | ||
|
@@ -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): | ||
|
@@ -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 | ||
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 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): | ||
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.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) |
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.
could you expand on this a bit so that it makes sense for users, please?