Skip to content
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

Add configurable room list publishing rules #4647

Merged
merged 9 commits into from Feb 15, 2019
Copy path View file
@@ -0,0 +1 @@
Add configurable room list publishing rules
Copy path View file
@@ -23,70 +23,132 @@ def read_config(self, config):
alias_creation_rules = config["alias_creation_rules"]

self._alias_creation_rules = [
_AliasRule(rule)
_RoomDirectoryRule("alias_creation_rules", rule)
for rule in alias_creation_rules
]

room_list_publication_rules = config["room_list_publication_rules"]

self._room_list_publication_rules = [
_RoomDirectoryRule("room_list_publication_rules", rule)
for rule in room_list_publication_rules
]

def default_config(self, config_dir_path, server_name, **kwargs):
return """
# The `alias_creation` option controls who's allowed to create aliases
# on this server.
#
# The format of this option is a list of rules that contain globs that
# match against user_id and the new alias (fully qualified with server
# name). The action in the first rule that matches is taken, which can
# currently either be "allow" or "deny".
# match against user_id, room_id and the new alias (fully qualified with
# server name). The action in the first rule that matches is taken,
# which can currently either be "allow" or "deny".
#
# Missing user_id/room_id/alias fields default to "*".
#
# If no rules match the request is denied.
alias_creation_rules:
- user_id: "*"
alias: "*"
- user_id: "*" # Matches agaisnt the creator of the alias
This conversation was marked as resolved by erikjohnston

This comment has been minimized.

Copy link
@richvdh

richvdh Feb 14, 2019

Member

the convention we've used elsewhere is to document options at the top of the section rather than using inline comments, which feel a bit cluttered to me. - see https://github.com/matrix-org/synapse/blob/develop/synapse/config/server.py#L336 for example.

This comment has been minimized.

Copy link
@richvdh

richvdh Feb 14, 2019

Member

agaisnt

alias: "*" # Matches against the alias being created
room_id: "*" # Matches against the room ID the alias is being
# pointed at
action: allow
# The `room_list_publication_rules` option controls who can publish and
# which rooms can be published in the public room list.
This conversation was marked as resolved by erikjohnston

This comment has been minimized.

Copy link
@richvdh

richvdh Feb 14, 2019

Member

does an empty list mean that everyone can publish or nobody can publish?

#
# The format of this option is the same as that for
# `alias_creation_rules`.
#
# If the room has one or more aliases associated with it, only one of
# the aliases needs to match the alias rule. If there are no aliases

This comment has been minimized.

Copy link
@richvdh

richvdh Feb 14, 2019

Member

what does it even mean to publish a room in the room directory when it doesn't have an alias, ooi?

This comment has been minimized.

Copy link
@erikjohnston

erikjohnston Feb 15, 2019

Author Member

It's a perfectly legit thing to do, while remote users wouldn't be able to join the room local users still would happily be able to.

Though I have a suspicion we might require an alias before we actually publish the room

# then only rules with `alias: *` match.
room_list_publication_rules:
This conversation was marked as resolved by erikjohnston

This comment has been minimized.

Copy link
@richvdh

richvdh Feb 14, 2019

Member

I think we're adopting a convention that new sections should be commented out in the default config, which apart from anything else answers the question of "does it matter if I don't include this section in my config". See https://github.com/matrix-org/synapse/blob/develop/synapse/config/server.py#L461 etc.

This comment has been minimized.

Copy link
@erikjohnston

erikjohnston Feb 15, 2019

Author Member

Hmm, ok. It's somewhat annoying to have to comment about the config in the default config and have it in the read_config. FWIW what's in the default_config is the default if you omit the key

- user_id: "*" # Matches against the user publishing the room
alias: "*" # Matches against any current local or canonical
# aliases associated with the room
room_id: "*"
action: allow
"""

def is_alias_creation_allowed(self, user_id, alias):
def is_alias_creation_allowed(self, user_id, room_id, alias):
"""Checks if the given user is allowed to create the given alias
Args:
user_id (str)
room_id (str)
alias (str)
Returns:
boolean: True if user is allowed to crate the alias
"""
for rule in self._alias_creation_rules:
if rule.matches(user_id, alias):
if rule.matches(user_id, room_id, [alias]):
return rule.action == "allow"

return False

def is_publishing_room_allowed(self, user_id, room_id, aliases):
"""Checks if the given user is allowed to publish the room
Args:
user_id (str)
room_id (str)
aliases (list[str]): any local aliases associated with the room
Returns:
boolean: True if user can publish room
"""
for rule in self._room_list_publication_rules:
if rule.matches(user_id, room_id, aliases):
return rule.action == "allow"

return False


class _AliasRule(object):
def __init__(self, rule):
class _RoomDirectoryRule(object):
"""Helper class to test whether a room directory action is allowed, like
creating an alias or publishing a room.
"""

def __init__(self, option_name, rule):
This conversation was marked as resolved by erikjohnston

This comment has been minimized.

Copy link
@richvdh

richvdh Feb 14, 2019

Member

doc your args pls

"""
Args:
option_name (str): Name of the config option this rule belongs to
rule (dict): The rule as specified in the config
"""

action = rule["action"]
user_id = rule["user_id"]
alias = rule["alias"]
user_id = rule.get("user_id", "*")
room_id = rule.get("room_id", "*")
alias = rule.get("alias", "*")

if action in ("allow", "deny"):
self.action = action
else:
raise ConfigError(
"alias_creation_rules rules can only have action of 'allow'"
" or 'deny'"
"%s rules can only have action of 'allow'"
" or 'deny'" % (option_name,)
)

self._alias_matches_all = alias == "*"

try:
self._user_id_regex = glob_to_regex(user_id)
self._alias_regex = glob_to_regex(alias)
self._room_id_regex = glob_to_regex(room_id)
except Exception as e:
raise ConfigError("Failed to parse glob into regex: %s", e)

def matches(self, user_id, alias):
"""Tests if this rule matches the given user_id and alias.
def matches(self, user_id, room_id, aliases):
"""Tests if this rule matches the given user_id, room_id and aliases.
Args:
user_id (str)
alias (str)
room_id (str)
aliases (list[str]): The associated aliases to the room. Will be a
single element for testing alias creation, and can be empty for
testing room publishing.
Returns:
boolean
@@ -96,7 +158,24 @@ def matches(self, user_id, alias):
if not self._user_id_regex.match(user_id):
return False

if not self._alias_regex.match(alias):
# If we are not given any aliases then this rule only matches if the
# alias glob matches all aliases
matched = False
if not aliases:
if not self._alias_matches_all:
return False
else:
# Otherwise, we just need one alias to match
matched = False
for alias in aliases:
if self._alias_regex.match(alias):
matched = True
This conversation was marked as resolved by erikjohnston

This comment has been minimized.

Copy link
@richvdh

richvdh Feb 14, 2019

Member

can we hoist the room_id check higher up, and them do an early return here?

break

if not matched:
return False

if not self._room_id_regex.match(room_id):
return False

return True
Copy path View file
@@ -112,7 +112,9 @@ def create_association(self, requester, room_alias, room_id, servers=None,
403, "This user is not permitted to create this alias",
)

if not self.config.is_alias_creation_allowed(user_id, room_alias.to_string()):
if not self.config.is_alias_creation_allowed(
user_id, room_id, room_alias.to_string(),
):
# Lets just return a generic message, as there may be all sorts of
# reasons why we said no. TODO: Allow configurable error messages
# per alias creation rule?
@@ -395,9 +397,9 @@ def edit_published_room_list(self, requester, room_id, visibility):
room_id (str)
visibility (str): "public" or "private"
"""
if not self.spam_checker.user_may_publish_room(
requester.user.to_string(), room_id
):
user_id = requester.user.to_string()

if not self.spam_checker.user_may_publish_room(user_id, room_id):
raise AuthError(
403,
"This user is not permitted to publish rooms to the room list"
@@ -415,7 +417,24 @@ def edit_published_room_list(self, requester, room_id, visibility):

yield self.auth.check_can_change_room_list(room_id, requester.user)

yield self.store.set_room_is_public(room_id, visibility == "public")
making_public = visibility == "public"
if making_public:
room_aliases = yield self.store.get_aliases_for_room(room_id)
canonical_alias = yield self.store.get_canonical_alias_for_room(room_id)
if canonical_alias:
room_aliases.append(canonical_alias)

if not self.config.is_publishing_room_allowed(
user_id, room_id, room_aliases,
):
# Lets just return a generic message, as there may be all sorts of
# reasons why we said no. TODO: Allow configurable error messages
# per alias creation rule?
raise SynapseError(
403, "Not allowed to publish room",
)

yield self.store.set_room_is_public(room_id, making_public)

@defer.inlineCallbacks
def edit_published_appservice_room_list(self, appservice_id, network_id,
Copy path View file
@@ -548,6 +548,31 @@ def _get_filtered_current_state_ids_txn(txn):
_get_filtered_current_state_ids_txn,
)

@defer.inlineCallbacks
def get_canonical_alias_for_room(self, room_id):
"""Get canonical alias for room, if any
Args:
room_id (str)
Returns:
Deferred[str|None]: The canonical alias, if any
"""

state = yield self.get_filtered_current_state_ids(room_id, StateFilter.from_types(
[(EventTypes.CanonicalAlias, "")]
))

event_id = state.get((EventTypes.CanonicalAlias, ""))
if not event_id:
return

event = yield self.get_event(event_id, allow_none=True)
if not event:
return

defer.returnValue(event.content.get("canonical_alias"))

@cached(max_entries=10000, iterable=True)
def get_state_group_delta(self, state_group):
"""Given a state group try to return a previous group and a delta between
@@ -36,32 +36,111 @@ def test_alias_creation_acl(self):
- user_id: "@gah:example.com"
alias: "#goo:example.com"
action: "allow"
room_list_publication_rules: []
""")

rd_config = RoomDirectoryConfig()
rd_config.read_config(config)

self.assertFalse(rd_config.is_alias_creation_allowed(
user_id="@bob:example.com",
room_id="!test",
alias="#test:example.com",
))

self.assertTrue(rd_config.is_alias_creation_allowed(
user_id="@test:example.com",
room_id="!test",
alias="#unofficial_st:example.com",
))

self.assertTrue(rd_config.is_alias_creation_allowed(
user_id="@foobar:example.com",
room_id="!test",
alias="#test:example.com",
))

self.assertTrue(rd_config.is_alias_creation_allowed(
user_id="@gah:example.com",
room_id="!test",
alias="#goo:example.com",
))

self.assertFalse(rd_config.is_alias_creation_allowed(
user_id="@test:example.com",
room_id="!test",
alias="#test:example.com",
))

def test_room_publish_acl(self):
config = yaml.load("""
alias_creation_rules: []
room_list_publication_rules:
- user_id: "*bob*"
alias: "*"
action: "deny"
- user_id: "*"
alias: "#unofficial_*"
action: "allow"
- user_id: "@foo*:example.com"
alias: "*"
action: "allow"
- user_id: "@gah:example.com"
alias: "#goo:example.com"
action: "allow"
- room_id: "!test-deny"
action: "deny"
""")

rd_config = RoomDirectoryConfig()
rd_config.read_config(config)

self.assertFalse(rd_config.is_publishing_room_allowed(
user_id="@bob:example.com",
room_id="!test",
aliases=["#test:example.com"],
))

self.assertTrue(rd_config.is_publishing_room_allowed(
user_id="@test:example.com",
room_id="!test",
aliases=["#unofficial_st:example.com"],
))

self.assertTrue(rd_config.is_publishing_room_allowed(
user_id="@foobar:example.com",
room_id="!test",
aliases=[],
))

self.assertTrue(rd_config.is_publishing_room_allowed(
user_id="@gah:example.com",
room_id="!test",
aliases=["#goo:example.com"],
))

self.assertFalse(rd_config.is_publishing_room_allowed(
user_id="@test:example.com",
room_id="!test",
aliases=["#test:example.com"],
))

self.assertTrue(rd_config.is_publishing_room_allowed(
user_id="@foobar:example.com",
room_id="!test-deny",
aliases=[],
))

self.assertFalse(rd_config.is_publishing_room_allowed(
user_id="@gah:example.com",
room_id="!test-deny",
aliases=[],
))

self.assertTrue(rd_config.is_publishing_room_allowed(
user_id="@test:example.com",
room_id="!test",
aliases=["#unofficial_st:example.com", "#blah:example.com"],
))
@@ -121,6 +121,7 @@ def prepare(self, hs, reactor, clock):
"action": "allow",
}
]
config["room_list_publication_rules"] = []

rd_config = RoomDirectoryConfig()
rd_config.read_config(config)
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.