New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Autocreate autojoin rooms #3975

Merged
merged 16 commits into from Oct 25, 2018

Conversation

6 participants
@ara4n
Member

ara4n commented Sep 28, 2018

Needed for HHS.

ara4n added some commits Sep 28, 2018

@ara4n ara4n changed the title from WIP: autocreate autojoin rooms to Autocreate autojoin rooms Sep 29, 2018

@ara4n ara4n requested a review from matrix-org/synapse-core Sep 29, 2018

# 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()
if count == 1 and RoomAlias.is_valid(room_identifier):

This comment has been minimized.

@richvdh

richvdh Oct 1, 2018

Member

it feels very random to be doing this at this point. (Which is when the first user is registered? when they log in?). what if two users register at once? what if the first user didn't qualify for autojoining for some reason?

Can we not do this when the HS first starts, somehow?

This comment has been minimized.

@ara4n

ara4n Oct 1, 2018

Member

the only way to do it at start would be if we also autocreated an admin user, which in turn then means shared-secret registering them and then setting up their profile and 3pid and passwords etc correctly. agreed this would be cleaner, but this is intended as a fast fix to avoid having to do that in the provisioning process, on the basis that the first user who signs in is 99% always going to be the server admin, and if it isn't, then you can always repoint the alias.

This comment has been minimized.

@richvdh

richvdh Oct 1, 2018

Member

ok, fair enough, but still, having _join_user_to_room magically do something different if there is now exactly one registered user feels very magical.

can we pull out the is this the first user logic to register (where we only have to do it once per user, rather than once per user per room)? and have a separate _create_auto_join_room method?

This comment has been minimized.

@richvdh

richvdh Oct 1, 2018

Member

and have a separate _create_auto_join_room method

or just call room_creation_handler.create_room directly from the for r in self.hs.config.auto_join_rooms loop in register.

)
room_id = info["room_id"]
directory_handler = self.hs.get_handlers().directory_handler

This comment has been minimized.

@richvdh

richvdh Oct 1, 2018

Member

can we not do the alias bits by passing room_alias_name to create_room ?

This comment has been minimized.

@ara4n

ara4n Oct 1, 2018

Member

oh, possibly.

@richvdh

This comment has been minimized.

Member

richvdh commented Oct 1, 2018

Also: we're not happy with this going in with zero tests to make sure it doesn't suddenly break in the future. It's going to be hard to sytest, but please can it have some unit tests to run the code?

@ara4n

This comment has been minimized.

Member

ara4n commented Oct 1, 2018

before I go rewrite this, it'd be good to get aligned on the proposed behaviour in general; the current implementation was a quick fix i suggested to @neilisfragile so we could get this feature out quickly rather than getting entangled with complicated provisioning/auth rejigs.

@hawkowl hawkowl added this to To Do in Backend Core Team via automation Oct 1, 2018

@maxidor

This comment has been minimized.

Contributor

maxidor commented Oct 1, 2018

What is HHS ?

@ara4n

This comment has been minimized.

Member

ara4n commented Oct 1, 2018

HHS === modular.im

@richvdh richvdh moved this from To Do to In Progress: Operational/bug fixes in Backend Core Team Oct 1, 2018

@neilisfragile

This comment has been minimized.

Contributor

neilisfragile commented Oct 3, 2018

Need to think about gdpr consent and ability to create rooms, could be dependent on vector-im/riot-web#7168

@neilisfragile neilisfragile self-assigned this Oct 3, 2018

neilisfragile added some commits Oct 4, 2018

@neilisfragile

This comment has been minimized.

Contributor

neilisfragile commented Oct 5, 2018

Blocked on #4004

@turt2live

This comment has been minimized.

Member

turt2live commented Oct 5, 2018

Blocked how? This and #4004 are unrelated changes

@neilisfragile

This comment has been minimized.

Contributor

neilisfragile commented Oct 6, 2018

It's currently not possible to create the room prior to consent, so the room is never created on GDPR compliant servers. I suppose you could instead tie room creation into the first user to consent, but given you are already working on moving consent earlier in the flow it seems sensible just to wait for it to land then this PR will magically work.

neil seems to have fixed this

@richvdh richvdh self-requested a review Oct 10, 2018

@@ -0,0 +1 @@
First user should autocreate autojoin rooms

This comment has been minimized.

@richvdh

richvdh Oct 10, 2018

Member

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

@@ -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

This comment has been minimized.

@richvdh

richvdh Oct 10, 2018

Member

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

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

This comment has been minimized.

@richvdh

richvdh Oct 10, 2018

Member

can you put this in the constructor?

This comment has been minimized.

@neilisfragile

neilisfragile Oct 13, 2018

Contributor

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.

This comment has been minimized.

@richvdh

richvdh Oct 17, 2018

Member

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.

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

This comment has been minimized.

@richvdh

richvdh Oct 10, 2018

Member

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.

# 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

This comment has been minimized.

@richvdh

richvdh Oct 10, 2018

Member

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

Show resolved Hide resolved synapse/handlers/register.py Outdated
@@ -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):

This comment has been minimized.

@richvdh

richvdh Oct 10, 2018

Member

please no

This comment has been minimized.

@neilisfragile

neilisfragile Oct 12, 2018

Contributor

it was oversight I promise!

)
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)

This comment has been minimized.

@richvdh

richvdh Oct 10, 2018

Member

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

@defer.inlineCallbacks
def test_auto_create_auto_join_rooms(self):
room_alias_str = "#room:test"
self.hs.config.autocreate_auto_join_rooms = True

This comment has been minimized.

@richvdh

richvdh Oct 10, 2018

Member

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.assertEqual(len(rooms), 1)
@defer.inlineCallbacks
def test_auto_create_auto_join_rooms_with_no_rooms(self):

This comment has been minimized.

@richvdh

richvdh Oct 10, 2018

Member

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

@richvdh richvdh removed their assignment Oct 10, 2018

@martindale

This comment has been minimized.

martindale commented Oct 12, 2018

Could portions of this be used to auto-create a room for each user? Useful for creating notes channels, etc. for workflow-specific applications.

@@ -1 +1 @@
First user should autocreate autojoin rooms
Servers with auto join rooms, should autocreate those rooms when first user registers

This comment has been minimized.

@richvdh

richvdh Oct 17, 2018

Member

s/auto join/auto-join/

s/should/will now/

s/first user/the first user/

possibly, s/autocreate/automatically create/

@@ -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

This comment has been minimized.

@richvdh

richvdh Oct 17, 2018

Member

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

@@ -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)

This comment has been minimized.

@richvdh

richvdh Oct 17, 2018

Member

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

@@ -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

This comment has been minimized.

@richvdh

richvdh Oct 17, 2018

Member

s/exists/exist/

# the rooms exists 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.

This comment has been minimized.

@richvdh

richvdh Oct 17, 2018

Member

s/auto joined/auto-joined/

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

This comment has been minimized.

@richvdh

richvdh Oct 17, 2018

Member

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.

@@ -26,6 +26,7 @@
RegistrationError,
SynapseError,
)
from synapse.config._base import ConfigError

This comment has been minimized.

@richvdh

richvdh Oct 17, 2018

Member

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)

room_creation_handler = self.hs.get_room_creation_handler()
if self.hs.hostname != RoomAlias.from_string(r).domain:
raise ConfigError(

This comment has been minimized.

@richvdh

richvdh Oct 17, 2018

Member

... 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.

room_creation_handler = self.hs.get_room_creation_handler()
if self.hs.hostname != RoomAlias.from_string(r).domain:
raise ConfigError(
'Cannot create room alias %s, it does not match server domain'

This comment has been minimized.

@richvdh

richvdh Oct 17, 2018

Member

this is missing its argument too.

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)

This comment has been minimized.

@richvdh

richvdh Oct 17, 2018

Member

erm, why does this not throw an exception?

This comment has been minimized.

@neilisfragile

neilisfragile Oct 17, 2018

Contributor

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

@richvdh richvdh assigned neilisfragile and unassigned richvdh Oct 17, 2018

@neilisfragile neilisfragile self-requested a review Oct 19, 2018

@neilisfragile neilisfragile requested review from richvdh and matrix-org/synapse-core and removed request for richvdh Oct 19, 2018

@@ -1 +1 @@
Servers with auto join rooms, should autocreate those rooms when first user registers
Servers with auto-join rooms, should automatically create those rooms when first user registers

This comment has been minimized.

@richvdh

richvdh Oct 23, 2018

Member

Still some problems here:

s/, should/will now/

s/first user/the first user/

from synapse.types import RoomAlias
from synapse.util.stringutils import random_string_with_symbols
from ._base import Config
from ._base import Config, ConfigError

This comment has been minimized.

@richvdh

richvdh Oct 23, 2018

Member

again, absolute imports are preferred over relative ones. Please s/._base/synapse.config._base/

This comment has been minimized.

@neilisfragile

neilisfragile Oct 23, 2018

Contributor

right, misunderstood you

'Cannot create room alias %s, '
'it does not match server domain' % (r,)
)
raise SynapseError()

This comment has been minimized.

@richvdh

richvdh Oct 23, 2018

Member

why are we raising an exception here (and why does it have no message, but that's a side-issue)

This comment has been minimized.

@neilisfragile

neilisfragile Oct 23, 2018

Contributor

this is brain dead, sorry

if self.hs.hostname != RoomAlias.from_string(r).domain:
raise ConfigError(
'Cannot create room alias %s, it does not match server domain'
logger.warn(

This comment has been minimized.

@richvdh

richvdh Oct 23, 2018

Member

fwiw (just a nit fyi): logger.warn is deprecated, prefer logger.warning

'Cannot create room alias %s, it does not match server domain'
logger.warn(
'Cannot create room alias %s, '
'it does not match server domain' % (r,)

This comment has been minimized.

@richvdh

richvdh Oct 23, 2018

Member

the logger methods automatically interpolate their arguments (with the benefit of doing it after checking if logging is enabled), so, you can write:

logger.warn('message with %s', r)

rather than using % explicitly

@@ -231,21 +231,23 @@ def register(
for r in self.hs.config.auto_join_rooms:
try:
if should_auto_create_rooms:
room_creation_handler = self.hs.get_room_creation_handler()
if self.hs.hostname != RoomAlias.from_string(r).domain:

This comment has been minimized.

@richvdh

richvdh Oct 23, 2018

Member

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

@richvdh richvdh assigned neilisfragile and unassigned richvdh Oct 23, 2018

@neilisfragile neilisfragile requested a review from matrix-org/synapse-core Oct 23, 2018

@richvdh

lgtm other than the nits below. Also your CI is failing.

Show resolved Hide resolved changelog.d/3975.feature Outdated
Show resolved Hide resolved synapse/handlers/register.py Outdated

@richvdh richvdh assigned neilisfragile and unassigned richvdh Oct 23, 2018

richvdh and others added some commits Oct 24, 2018

Update changelog.d/3975.feature
Co-Authored-By: neilisfragile <neil@matrix.org>
fix tuple
Co-Authored-By: neilisfragile <neil@matrix.org>

@neilisfragile neilisfragile merged commit c99b6c6 into develop Oct 25, 2018

5 checks passed

ci/circleci: sytestpy2merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgresmerged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgresmerged Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Backend Core Team automation moved this from In Progress: Operational/bug fixes to Done - Operations Oct 25, 2018

@neilisfragile neilisfragile deleted the matthew/autocreate_autojoin branch Oct 25, 2018

@neilisfragile neilisfragile moved this from Done - Operations to Done - Planned Project in Backend Core Team Oct 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment