diff --git a/changelog.d/5760.feature b/changelog.d/5760.feature
new file mode 100644
index 0000000000..90302d793e
--- /dev/null
+++ b/changelog.d/5760.feature
@@ -0,0 +1 @@
+Force the access rule to be "restricted" if the join rule is "public".
diff --git a/synapse/storage/events_worker.py b/synapse/storage/events_worker.py
index cc7df5cf14..5dc49822b5 100644
--- a/synapse/storage/events_worker.py
+++ b/synapse/storage/events_worker.py
@@ -255,6 +255,26 @@ class EventsWorkerStore(SQLBaseStore):
# didn't have the redacted event at the time, so we recheck on read
# instead.
if not allow_rejected and entry.event.type == EventTypes.Redaction:
+ orig_event_info = yield self._simple_select_one(
+ table="events",
+ keyvalues={"event_id": entry.event.redacts},
+ retcols=["sender", "room_id", "type"],
+ allow_none=True,
+ )
+
+ if not orig_event_info:
+ # We don't have the event that is being redacted, so we
+ # assume that the event isn't authorized for now. (If we
+ # later receive the event, then we will always redact
+ # it anyway, since we have this redaction)
+ continue
+
+ if orig_event_info["room_id"] != entry.event.room_id:
+ # Don't process redactions if the redacted event doesn't belong to the
+ # redaction's room.
+ logger.info("Ignoring redation in another room.")
+ continue
+
if entry.event.internal_metadata.need_to_check_redaction():
# XXX: we need to avoid calling get_event here.
#
@@ -277,27 +297,13 @@ class EventsWorkerStore(SQLBaseStore):
# 2. have _get_event_from_row just call the first half of
# that
- orig_sender = yield self._simple_select_one_onecol(
- table="events",
- keyvalues={"event_id": entry.event.redacts},
- retcol="sender",
- allow_none=True,
- )
-
expected_domain = get_domain_from_id(entry.event.sender)
if (
- orig_sender
- and get_domain_from_id(orig_sender) == expected_domain
+ get_domain_from_id(orig_event_info["sender"]) == expected_domain
):
# This redaction event is allowed. Mark as not needing a
# recheck.
entry.event.internal_metadata.recheck_redaction = False
- else:
- # We don't have the event that is being redacted, so we
- # assume that the event isn't authorized for now. (If we
- # later receive the event, then we will always redact
- # it anyway, since we have this redaction)
- continue
if allow_rejected or not entry.event.rejected_reason:
if check_redacted and entry.redacted_event:
@@ -532,7 +538,7 @@ class EventsWorkerStore(SQLBaseStore):
)
redacted_event = None
- if redacted:
+ if redacted and original_ev.type != EventTypes.Redaction:
redacted_event = prune_event(original_ev)
redaction_id = yield self._simple_select_one_onecol(
@@ -564,9 +570,18 @@ class EventsWorkerStore(SQLBaseStore):
# recheck.
because.internal_metadata.recheck_redaction = False
else:
- # Senders don't match, so the event isn't actually redacted
+ # Senders don't match, so the event isn't actually
+ # redacted
redacted_event = None
+ if because.room_id != original_ev.room_id:
+ redacted_event = None
+ else:
+ # The lack of a redaction likely means that the redaction is invalid
+ # and therefore not returned by get_event, so it should be safe to
+ # just ignore it here.
+ redacted_event = None
+
cache_entry = _EventCacheEntry(
event=original_ev, redacted_event=redacted_event
)
diff --git a/synapse/third_party_rules/access_rules.py b/synapse/third_party_rules/access_rules.py
index e3f97bdf3a..1a295ea7ce 100644
--- a/synapse/third_party_rules/access_rules.py
+++ b/synapse/third_party_rules/access_rules.py
@@ -17,7 +17,7 @@ import email.utils
from twisted.internet import defer
-from synapse.api.constants import EventTypes
+from synapse.api.constants import EventTypes, JoinRules, RoomCreationPreset
from synapse.api.errors import SynapseError
from synapse.config._base import ConfigError
from synapse.types import get_domain_from_id
@@ -94,35 +94,43 @@ class RoomAccessRules(object):
default rule to the initial state.
"""
is_direct = config.get("is_direct")
- rule = None
+ preset = config.get("preset")
+ access_rule = None
+ join_rule = None
# If there's a rules event in the initial state, check if it complies with the
# spec for im.vector.room.access_rules and deny the request if not.
for event in config.get("initial_state", []):
if event["type"] == ACCESS_RULES_TYPE:
- rule = event["content"].get("rule")
+ access_rule = event["content"].get("rule")
# Make sure the event has a valid content.
- if rule is None:
+ if access_rule is None:
raise SynapseError(400, "Invalid access rule")
# Make sure the rule name is valid.
- if rule not in VALID_ACCESS_RULES:
+ if access_rule not in VALID_ACCESS_RULES:
raise SynapseError(400, "Invalid access rule")
# Make sure the rule is "direct" if the room is a direct chat.
if (
- (is_direct and rule != ACCESS_RULE_DIRECT)
- or (rule == ACCESS_RULE_DIRECT and not is_direct)
+ (is_direct and access_rule != ACCESS_RULE_DIRECT)
+ or (access_rule == ACCESS_RULE_DIRECT and not is_direct)
):
raise SynapseError(400, "Invalid access rule")
- # If there's no rules event in the initial state, create one with the default
- # setting.
- if not rule:
+ if event["type"] == EventTypes.JoinRules:
+ join_rule = event["content"].get("join_rule")
+
+ if access_rule is None:
+ # If there's no access rules event in the initial state, create one with the
+ # default setting.
if is_direct:
default_rule = ACCESS_RULE_DIRECT
else:
+ # If the default value for non-direct chat changes, we should make another
+ # case here for rooms created with either a "public" join_rule or the
+ # "public_chat" preset to make sure those keep defaulting to "restricted"
default_rule = ACCESS_RULE_RESTRICTED
if not config.get("initial_state"):
@@ -136,11 +144,22 @@ class RoomAccessRules(object):
}
})
- rule = default_rule
+ access_rule = default_rule
+
+ # Check that the preset or the join rule in use is compatible with the access
+ # rule, whether it's a user-defined one or the default one (i.e. if it involves
+ # a "public" join rule, the access rule must be "restricted").
+ if (
+ (
+ join_rule == JoinRules.PUBLIC
+ or preset == RoomCreationPreset.PUBLIC_CHAT
+ ) and access_rule != ACCESS_RULE_RESTRICTED
+ ):
+ raise SynapseError(400, "Invalid access rule")
# Check if the creator can override values for the power levels.
allowed = self._is_power_level_content_allowed(
- config.get("power_level_content_override", {}), rule,
+ config.get("power_level_content_override", {}), access_rule,
)
if not allowed:
raise SynapseError(400, "Invalid power levels content override")
@@ -148,7 +167,9 @@ class RoomAccessRules(object):
# Second loop for events we need to know the current rule to process.
for event in config.get("initial_state", []):
if event["type"] == EventTypes.PowerLevels:
- allowed = self._is_power_level_content_allowed(event["content"], rule)
+ allowed = self._is_power_level_content_allowed(
+ event["content"], access_rule
+ )
if not allowed:
raise SynapseError(400, "Invalid power levels content")
@@ -213,6 +234,9 @@ class RoomAccessRules(object):
if event.type == EventTypes.Member or event.type == EventTypes.ThirdPartyInvite:
return self._on_membership_or_invite(event, rule, state_events)
+ if event.type == EventTypes.JoinRules:
+ return self._on_join_rule_change(event, rule)
+
return True
def _on_rules_change(self, event, state_events):
@@ -232,6 +256,12 @@ class RoomAccessRules(object):
if new_rule not in VALID_ACCESS_RULES:
return False
+ # We must not allow rooms with the "public" join rule to be given any other access
+ # rule than "restricted".
+ join_rule = self._get_join_rule_from_state(state_events)
+ if join_rule == JoinRules.PUBLIC and new_rule != ACCESS_RULE_RESTRICTED:
+ return False
+
# Make sure we don't apply "direct" if the room has more than two members.
if new_rule == ACCESS_RULE_DIRECT:
existing_members, threepid_tokens = self._get_members_and_tokens_from_state(
@@ -381,7 +411,6 @@ class RoomAccessRules(object):
access_rule (str): The access rule in place in this room.
Returns:
bool, True if the event can be allowed, False otherwise.
-
"""
# Check if we need to apply the restrictions with the current rule.
if access_rule not in RULES_WITH_RESTRICTED_POWER_LEVELS:
@@ -405,6 +434,33 @@ class RoomAccessRules(object):
return True
+ def _on_join_rule_change(self, event, rule):
+ """Check whether a join rule change is allowed. A join rule change is always
+ allowed unless the new join rule is "public" and the current access rule isn't
+ "restricted".
+ The rationale is that external users (those whose server would be denied access
+ to rooms enforcing the "restricted" access rule) should always rely on non-
+ external users for access to rooms, therefore they shouldn't be able to access
+ rooms that don't require an invite to be joined.
+
+ Note that we currently rely on the default access rule being "restricted": during
+ room creation, the m.room.join_rules event will be sent *before* the
+ im.vector.room.access_rules one, so the access rule that will be considered here
+ in this case will be the default "restricted" one. This is fine since the
+ "restricted" access rule allows any value for the join rule, but we should keep
+ that in mind if we need to change the default access rule in the future.
+
+ Args:
+ event (synapse.events.EventBase): The event to check.
+ rule (str): The name of the rule to apply.
+ Returns:
+ bool, True if the event can be allowed, False otherwise.
+ """
+ if event.content.get('join_rule') == JoinRules.PUBLIC:
+ return rule == ACCESS_RULE_RESTRICTED
+
+ return True
+
@staticmethod
def _get_rule_from_state(state_events):
"""Extract the rule to be applied from the given set of state events.
@@ -423,6 +479,21 @@ class RoomAccessRules(object):
return rule
@staticmethod
+ def _get_join_rule_from_state(state_events):
+ """Extract the room's join rule from the given set of state events.
+
+ Args:
+ state_events (dict[tuple[event type, state key], EventBase]): The set of state
+ events.
+ Returns:
+ str, the name of the join rule (either "public", or "invite")
+ """
+ join_rule_event = state_events.get((EventTypes.JoinRules, ""))
+ if join_rule_event is None:
+ return None
+ return join_rule_event.content.get("join_rule")
+
+ @staticmethod
def _get_members_and_tokens_from_state(state_events):
"""Retrieves from a list of state events the list of users that have a
m.room.member event in the room, and the tokens of 3PID invites in the room.
diff --git a/tests/rest/client/test_room_access_rules.py b/tests/rest/client/test_room_access_rules.py
index df48a89e93..7e23add6b7 100644
--- a/tests/rest/client/test_room_access_rules.py
+++ b/tests/rest/client/test_room_access_rules.py
@@ -22,7 +22,7 @@ from mock import Mock
from twisted.internet import defer
-from synapse.api.constants import EventTypes
+from synapse.api.constants import EventTypes, JoinRules, RoomCreationPreset
from synapse.rest import admin
from synapse.rest.client.v1 import login, room
from synapse.third_party_rules.access_rules import (
@@ -156,6 +156,84 @@ class RoomAccessTestCase(unittest.HomeserverTestCase):
"""
self.create_room(direct=True, rule=ACCESS_RULE_RESTRICTED, expected_code=400)
+ def test_public_room(self):
+ """Tests that it's not possible to have a room with the public join rule and an
+ access rule that's not restricted.
+ """
+ # Creating a room with the public_chat preset should succeed and set the access
+ # rule to restricted.
+ preset_room_id = self.create_room(preset=RoomCreationPreset.PUBLIC_CHAT)
+ self.assertEqual(
+ self.current_rule_in_room(preset_room_id), ACCESS_RULE_RESTRICTED,
+ )
+
+ # Creating a room with the public join rule in its initial state should succeed
+ # and set the access rule to restricted.
+ init_state_room_id = self.create_room(initial_state=[{
+ "type": "m.room.join_rules",
+ "content": {
+ "join_rule": JoinRules.PUBLIC,
+ },
+ }])
+ self.assertEqual(
+ self.current_rule_in_room(init_state_room_id), ACCESS_RULE_RESTRICTED,
+ )
+
+ # Changing access rule to unrestricted should fail.
+ self.change_rule_in_room(
+ preset_room_id, ACCESS_RULE_UNRESTRICTED, expected_code=403,
+ )
+ self.change_rule_in_room(
+ init_state_room_id, ACCESS_RULE_UNRESTRICTED, expected_code=403,
+ )
+
+ # Changing access rule to direct should fail.
+ self.change_rule_in_room(
+ preset_room_id, ACCESS_RULE_DIRECT, expected_code=403,
+ )
+ self.change_rule_in_room(
+ init_state_room_id, ACCESS_RULE_DIRECT, expected_code=403,
+ )
+
+ # Changing join rule to public in an unrestricted room should fail.
+ self.change_join_rule_in_room(
+ self.unrestricted_room, JoinRules.PUBLIC, expected_code=403,
+ )
+ # Changing join rule to public in an direct room should fail.
+ self.change_join_rule_in_room(
+ self.direct_rooms[0], JoinRules.PUBLIC, expected_code=403,
+ )
+
+ # Creating a new room with the public_chat preset and an access rule that isn't
+ # restricted should fail.
+ self.create_room(
+ preset=RoomCreationPreset.PUBLIC_CHAT, rule=ACCESS_RULE_UNRESTRICTED,
+ expected_code=400,
+ )
+ self.create_room(
+ preset=RoomCreationPreset.PUBLIC_CHAT, rule=ACCESS_RULE_DIRECT,
+ expected_code=400,
+ )
+
+ # Creating a room with the public join rule in its initial state and an access
+ # rule that isn't restricted should fail.
+ self.create_room(
+ initial_state=[{
+ "type": "m.room.join_rules",
+ "content": {
+ "join_rule": JoinRules.PUBLIC,
+ },
+ }], rule=ACCESS_RULE_UNRESTRICTED, expected_code=400,
+ )
+ self.create_room(
+ initial_state=[{
+ "type": "m.room.join_rules",
+ "content": {
+ "join_rule": JoinRules.PUBLIC,
+ },
+ }], rule=ACCESS_RULE_DIRECT, expected_code=400,
+ )
+
def test_restricted(self):
"""Tests that in restricted mode we're unable to invite users from blacklisted
servers but can invite other users.
@@ -405,9 +483,13 @@ class RoomAccessTestCase(unittest.HomeserverTestCase):
expected_code=403,
)
- def create_room(self, direct=False, rule=None, expected_code=200):
+ def create_room(
+ self, direct=False, rule=None, preset=RoomCreationPreset.TRUSTED_PRIVATE_CHAT,
+ initial_state=None, expected_code=200,
+ ):
content = {
"is_direct": direct,
+ "preset": preset,
}
if rule:
@@ -419,6 +501,12 @@ class RoomAccessTestCase(unittest.HomeserverTestCase):
}
}]
+ if initial_state:
+ if "initial_state" not in content:
+ content["initial_state"] = []
+
+ content["initial_state"] += initial_state
+
request, channel = self.make_request(
"POST",
"/_matrix/client/r0/createRoom",
@@ -457,6 +545,20 @@ class RoomAccessTestCase(unittest.HomeserverTestCase):
self.assertEqual(channel.code, expected_code, channel.result)
+ def change_join_rule_in_room(self, room_id, new_join_rule, expected_code=200):
+ data = {
+ "join_rule": new_join_rule,
+ }
+ request, channel = self.make_request(
+ "PUT",
+ "/_matrix/client/r0/rooms/%s/state/%s" % (room_id, EventTypes.JoinRules),
+ json.dumps(data),
+ access_token=self.tok,
+ )
+ self.render(request)
+
+ self.assertEqual(channel.code, expected_code, channel.result)
+
def send_threepid_invite(self, address, room_id, expected_code=200):
params = {
"id_server": "testis",
|
