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 config option to control alias creation #4051

Merged
merged 8 commits into from Oct 25, 2018

Conversation

4 participants
@erikjohnston
Member

erikjohnston commented Oct 17, 2018

No description provided.

@erikjohnston erikjohnston added this to To Do in Backend Core Team via automation Oct 17, 2018

@erikjohnston erikjohnston requested a review from matrix-org/synapse-core Oct 17, 2018

@richvdh

y u no pep8

@richvdh

the lack of UTs on this makes me unhappy - I think this sort of stuff is easy to screw up (cf the anchoring bug, for example), but very amenable to testing.

What do we expect to happen when somebody calls /createRoom with a room_alias_name, and the alias fails? I suspect the whole thing is going to fail with a 403, despite the room getting successfully created, which is less than ideal.

def glob_to_regex(glob):
"""Converts a glob to a compiled regex object

This comment has been minimized.

@richvdh

richvdh Oct 17, 2018

Member

we should probably spell out that the regex is anchored at the end but not the start of the string.

This comment has been minimized.

@erikjohnston

erikjohnston Oct 17, 2018

Member

Its not anchored at all, is it?

boolean
"""
if not self._user_id_regex.search(user_id):

This comment has been minimized.

@richvdh

richvdh Oct 17, 2018

Member

this needs to be match rather than search, because the regexes generated by glob_to_regex aren't anchored.

This comment has been minimized.

@erikjohnston

erikjohnston Oct 17, 2018

Member

I suppose it'd be better to correctly anchor the regex both sides

class RoomDirectoryConfig(Config):
def read_config(self, config):
alias_creation_rules = config.get("alias_creation_rules", [])

This comment has been minimized.

@richvdh

richvdh Oct 17, 2018

Member

should the default be a single allow-all rule, to avoid breaking existing configs?

This comment has been minimized.

@erikjohnston

erikjohnston Oct 17, 2018

Member

Actually, its entirely unnecessary to have the default, as it'll be pulled in by the default_config. Will change.

# 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(

This comment has been minimized.

@richvdh

richvdh Oct 17, 2018

Member

can we raise an AuthError, or indeed anything that doesn't result in an M_UNKNOWN error code?

This comment has been minimized.

@erikjohnston

erikjohnston Oct 17, 2018

Member

Oh, true. I'll clean up the existing exceptions

@erikjohnston

This comment has been minimized.

Member

erikjohnston commented Oct 17, 2018

the lack of UTs on this makes me unhappy - I think this sort of stuff is easy to screw up (cf the anchoring bug, for example), but very amenable to testing.

Ugh yes, I was going to ask how to add UTs as we don't currently test any of the config stuff. I could just manually insert the list of parsed _AuthRules, but that feels like its bypassing all the interesting stuff

@erikjohnston

This comment has been minimized.

Member

erikjohnston commented Oct 17, 2018

What do we expect to happen when somebody calls /createRoom with a room_alias_name, and the alias fails? I suspect the whole thing is going to fail with a 403, despite the room getting successfully created, which is less than ideal.

True, I haven't looked as was just expanding the rules already there. Will investigate, though I'm not sure what the right thing to do is (in terms of failing the request vs succeeding without the alias)

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

@erikjohnston

This comment has been minimized.

Member

erikjohnston commented Oct 18, 2018

What do we expect to happen when somebody calls /createRoom with a room_alias_name, and the alias fails? I suspect the whole thing is going to fail with a 403, despite the room getting successfully created, which is less than ideal.

Actually, we register the alias before creating the room, so it'll fail without creating the room if the alias is taken.

@erikjohnston

This comment has been minimized.

Member

erikjohnston commented Oct 18, 2018

I had to refactor the alias creation rest servlet to make it not be insane.

I guess I should probably split it out into a separate PR

@erikjohnston

This comment has been minimized.

Member

erikjohnston commented Oct 19, 2018

I've rebased this on top of #4063 as its just easier

@erikjohnston erikjohnston requested a review from matrix-org/synapse-core Oct 24, 2018

@erikjohnston erikjohnston removed their assignment Oct 25, 2018

@richvdh

looks really nice modulo the below.

class RoomDirectoryConfig(Config):
def read_config(self, config):
alias_creation_rules = config["alias_creation_rules"]

This comment has been minimized.

@richvdh

richvdh Oct 25, 2018

Member

I'm still pretty unclear how this is going to for an existing deployment. Adding it to default_config doesn't help that.

This comment has been minimized.

@erikjohnston

erikjohnston Oct 25, 2018

Member

I'm pretty sure we load the config on top of the default_config?

This comment has been minimized.

@richvdh

richvdh Oct 25, 2018

Member

that would be news to me

# 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 "allowed" or "denied".

This comment has been minimized.

@richvdh

richvdh Oct 25, 2018

Member

I feel like an action should be allow or deny rather than allowed or denied.

@erikjohnston erikjohnston requested a review from richvdh Oct 25, 2018

@erikjohnston erikjohnston merged commit c85e063 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

@rubo77

This comment has been minimized.

Contributor

rubo77 commented Nov 14, 2018

What does this pull request actually do?

If this is linked from the Release page, it should at least explain what is new here

@neilisfragile

This comment has been minimized.

Contributor

neilisfragile commented Nov 14, 2018

It's an admin management feature in the case where the admin wants to restrict who can create room aliases.

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