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
@@ -0,0 +1 @@
Add configurable room list publishing rules
@@ -23,70 +23,121 @@ 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: "*"
alias: "*" # This matches alias being created
This conversation was marked as resolved by erikjohnston

This comment has been minimized.

Copy link
@richvdh

richvdh Feb 14, 2019

Member

I don't understand this comment

room_id: "*"
action: allow
# The `room_list_publication_rules` option control who and what can be
This conversation was marked as resolved by erikjohnston

This comment has been minimized.

Copy link
@richvdh

richvdh Feb 14, 2019

Member

controls

This comment has been minimized.

Copy link
@richvdh

richvdh Feb 14, 2019

Member

who can be published? makes no sense

# published in the public room list.
#
# The format of this option is the same as that for
# `alias_creation_rules`
This conversation was marked as resolved by erikjohnston

This comment has been minimized.

Copy link
@richvdh

richvdh Feb 14, 2019

Member

what is the behaviour if a room has more than one alias?

This conversation was marked as resolved by erikjohnston

This comment has been minimized.

Copy link
@richvdh

richvdh Feb 14, 2019

Member

what does the user_id match against?

This conversation was marked as resolved by erikjohnston

This comment has been minimized.

Copy link
@richvdh

richvdh Feb 14, 2019

Member

does this match on all aliases (using the m.room.aliases event ?) or just local aliases?

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: "*"
alias: "*" # This matches any local or canonical alias
This conversation was marked as resolved by erikjohnston

This comment has been minimized.

Copy link
@richvdh

richvdh Feb 14, 2019

Member

I don't really understand this comment either. Is it just saying "wildcard is wildcard"?

# 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

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 +147,16 @@ 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
if not aliases and not self._alias_matches_all:
return False

for alias in aliases:
if not self._alias_regex.match(alias):
return False

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

return True
@@ -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")
room_aliases = yield self.store.get_aliases_for_room(room_id)
This conversation was marked as resolved by erikjohnston

This comment has been minimized.

Copy link
@richvdh

richvdh Feb 14, 2019

Member

it'd be nice to only do this if making_public is true

canonical_alias = yield self.store.get_canonical_alias_for_room(room_id)
if canonical_alias:
room_aliases.append(canonical_alias)

making_public = visibility == "public"

if making_public and 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,
@@ -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,105 @@ 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=[],
))
@@ -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.