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

fix test_auto_create_auto_join_where_no_consent #4886

Merged
merged 9 commits into from
Mar 19, 2019
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
13 changes: 10 additions & 3 deletions synapse/handlers/message.py
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,14 @@ def __init__(self, hs):

self.spam_checker = hs.get_spam_checker()

if self.config.block_events_without_consent_error is not None:
self._block_events_without_consent_error = (
self.config.block_events_without_consent_error
)

# we need to construct a ConsentURIBuilder here, as it checks that the necessary
# config options, but *only* if we have a configuration for which we are
# going to need it.
if self._block_events_without_consent_error:
self._consent_uri_builder = ConsentURIBuilder(self.config)

@defer.inlineCallbacks
Expand Down Expand Up @@ -378,7 +385,7 @@ def assert_accepted_privacy_policy(self, requester):
Raises:
ConsentNotGivenError: if the user has not given consent yet
"""
if self.config.block_events_without_consent_error is None:
if self._block_events_without_consent_error is None:
return

# exempt AS users from needing consent
Expand All @@ -405,7 +412,7 @@ def assert_accepted_privacy_policy(self, requester):
consent_uri = self._consent_uri_builder.build_user_consent_uri(
requester.user.localpart,
)
msg = self.config.block_events_without_consent_error % {
msg = self._block_events_without_consent_error % {
'consent_uri': consent_uri,
}
raise ConsentNotGivenError(
Expand Down
6 changes: 6 additions & 0 deletions synapse/handlers/register.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,13 @@
from synapse.api.errors import (
AuthError,
Codes,
ConsentNotGivenError,
InvalidCaptchaError,
LimitExceededError,
RegistrationError,
SynapseError,
)

from synapse.config.server import is_threepid_reserved
from synapse.http.client import CaptchaServerHttpClient
from synapse.http.servlet import assert_params_in_dict
Expand Down Expand Up @@ -311,6 +313,10 @@ def _auto_join_rooms(self, user_id):
)
else:
yield self._join_user_to_room(fake_requester, r)
except ConsentNotGivenError as e:
# Technically not necessary to pull out this error though
# moving away from bare excepts is a good thing to do.
logger.error("Failed to join new user to %r: %r", r, e)
except Exception as e:
logger.error("Failed to join new user to %r: %r", r, e)

Expand Down
20 changes: 19 additions & 1 deletion tests/handlers/test_register.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
from synapse.api.errors import ResourceLimitError, SynapseError
from synapse.handlers.register import RegistrationHandler
from synapse.types import RoomAlias, UserID, create_requester

from synapse.api.urls import ConsentURIBuilder
from tests.utils import setup_test_homeserver

from .. import unittest
Expand Down Expand Up @@ -187,12 +187,30 @@ def test_auto_create_auto_join_rooms_when_support_user_exists(self):

@defer.inlineCallbacks
def test_auto_create_auto_join_where_no_consent(self):
"""Test to ensure that the first user is not auto-joined to a room if
they have not given general consent.
"""
self.hs.config.user_consent_at_registration = True
Copy link
Member

Choose a reason for hiding this comment

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

does this and the next line actually do anything? I'd expect them to be redundant now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope

self.hs.config.block_events_without_consent_error = "Error"

# Given:-
# * a user must give consent,
# * they have not given that consent
# * The server is configured to auto-join to a room
# (and autocreate if necessary)
event_creation_handler = self.hs.get_event_creation_handler()
event_creation_handler._block_events_without_consent_error = (
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit dubious about this poking into the guts of EventCreationHandler - it looks fragile. OTOH I can't really think of a better way of testing it, other than creating a new test class (possibly a subclass of RegistrationTestCase) which uses a different config - which might be a bit fiddly right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this really mings - I'll comment explicitly to make it clearer

self.hs.config.block_events_without_consent_error
)
event_creation_handler._consent_uri_builder = Mock()
room_alias_str = "#room:test"
self.hs.config.auto_join_rooms = [room_alias_str]

# When the user is registered, and post consent actions are called
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't seem to make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried to make it clearer with formatting

res = yield self.handler.register(localpart='jeff')
yield self.handler.post_consent_actions(res[0])

# Ensure that they have not been joined to the room
rooms = yield self.store.get_rooms_for_user(res[0])
self.assertEqual(len(rooms), 0)

Expand Down