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

add registrations_require_3pid and allow_local_3pids #2813

Merged
merged 9 commits into from
Jan 22, 2018
Merged

add registrations_require_3pid and allow_local_3pids #2813

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
28a6ccb
add registrations_require_3pid
ara4n Jan 19, 2018
81d037d
mock registrations_require_3pid
ara4n Jan 19, 2018
0af58f1
fix pep8
ara4n Jan 19, 2018
9d332e0
fix up v1, and improve errors
ara4n Jan 19, 2018
447f4f0
rewrite based on PR feedback:
ara4n Jan 19, 2018
293380b
trailing commas
ara4n Jan 19, 2018
8fe253f
fix PR nitpicking
ara4n Jan 19, 2018
62d7d66
oops, check all login types
ara4n Jan 19, 2018
49fce04
fix typo (thanks sytest)
ara4n Jan 19, 2018
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
1 change: 1 addition & 0 deletions synapse/api/errors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ class Codes(object):
THREEPID_AUTH_FAILED = "M_THREEPID_AUTH_FAILED"
THREEPID_IN_USE = "M_THREEPID_IN_USE"
THREEPID_NOT_FOUND = "M_THREEPID_NOT_FOUND"
THREEPID_DENIED = "M_THREEPID_DENIED"
INVALID_USERNAME = "M_INVALID_USERNAME"
SERVER_NOT_TRUSTED = "M_SERVER_NOT_TRUSTED"

Expand Down
13 changes: 13 additions & 0 deletions synapse/config/registration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ def read_config(self, config):
strtobool(str(config["disable_registration"]))
)

self.registrations_require_3pid = config.get("registrations_require_3pid", [])
Copy link
Member

@turt2live turt2live Jan 19, 2018 •
edited
Loading

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 affect just registrations and could be misleading to some people.

Edit for clarity: I'm talking about the config key name

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, i've asked @dbkr to clarify the impact on the other 3PID manipulations

Copy link
Member Author

Choose a reason for hiding this comment

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

this convo ended up in IRL as dave presumably missed this comment: the impact is:

  • There are three request tokens atm for verifying 3PIDs:
    • registration
    • password reset
    • adding a 3pid to your account for use in login (which in turn tries to go bind it on the IS if asked)
  • this PR restricts the format of all three with allowed_local_3pids
  • it does not limit IS bindings (other than in a weak manner given in practice the account/3pid api is used by clients today to trigger bindings)

self.registration_shared_secret = config.get("registration_shared_secret")

self.bcrypt_rounds = config.get("bcrypt_rounds", 12)
Expand All @@ -52,6 +53,18 @@ def default_config(self, **kwargs):
# Enable registration for new users.
enable_registration: False

# Mandate that registrations require a 3PID which matches one or more
# of these 3PIDs. N.B. regexp escape backslashes are doubled (once for
# YAML and once for the regexp itself)
#
# registrations_require_3pid:
# - medium: email
# pattern: ".*@matrix\\.org"
# - medium: email
# pattern: ".*@vector\\.im"
# - medium: msisdn
# pattern: "\\+44"

# If set, allows registration by anyone who also has the shared
# secret, even if registration is otherwise disabled.
registration_shared_secret: "%(registration_shared_secret)s"
Expand Down
21 changes: 21 additions & 0 deletions synapse/rest/client/v2_alpha/_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,27 @@ def set_timeline_upper_limit(filter_json, filter_timeline_limit):
filter_timeline_limit)


def check_3pid_allowed(hs, medium, address):
# check whether the HS has whitelisted the given 3PID
Copy link
Member

Choose a reason for hiding this comment

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

Proper docstring please.


allow = False
if hs.config.registrations_require_3pid:
for constraint in hs.config.registrations_require_3pid:
logger.debug("Checking 3PID %s (%s) against %s (%s)" % (
address, medium, constraint['pattern'], constraint['medium']
))
Copy link
Member

Choose a reason for hiding this comment

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

Don't do string building, like most logging libraries it does it for you: logger.debug("foo: %s, bar: %s", foo, bar)

if (
medium == constraint['medium'] and
re.match(constraint['pattern'], address)
):
allow = True
break
else:
allow = True

return allow
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Normally I'd do the pattern of returning immediately rather than having an allow variable, e.g.:

def check_3pid_allowed(hs, medium, address):
    if not hs.config.registrations_require_3pid:
        return True
 
    for constraint in hs.config.registrations_require_3pid:
        if allowed:
            return True

    return False



def interactive_auth_handler(orig):
"""Wraps an on_POST method to handle InteractiveAuthIncompleteErrors

Expand Down
14 changes: 13 additions & 1 deletion synapse/rest/client/v2_alpha/account.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
)
from synapse.util.async import run_on_reactor
from synapse.util.msisdn import phone_number_to_msisdn
from ._base import client_v2_patterns, interactive_auth_handler
from ._base import client_v2_patterns, interactive_auth_handler, check_3pid_allowed

logger = logging.getLogger(__name__)

Expand All @@ -47,6 +47,9 @@ def on_POST(self, request):
'id_server', 'client_secret', 'email', 'send_attempt'
])

if not check_3pid_allowed(self.hs, "email", body['email']):
raise SynapseError(403, "3PID denied", Codes.THREEPID_DENIED)

existingUid = yield self.hs.get_datastore().get_user_id_by_threepid(
'email', body['email']
)
Expand Down Expand Up @@ -78,6 +81,9 @@ def on_POST(self, request):

msisdn = phone_number_to_msisdn(body['country'], body['phone_number'])

if not check_3pid_allowed(self.hs, "msisdn", msisdn):
raise SynapseError(403, "3PID denied", Codes.THREEPID_DENIED)

existingUid = yield self.datastore.get_user_id_by_threepid(
'msisdn', msisdn
)
Expand Down Expand Up @@ -217,6 +223,9 @@ def on_POST(self, request):
if absent:
raise SynapseError(400, "Missing params: %r" % absent, Codes.MISSING_PARAM)

if not check_3pid_allowed(self.hs, "email", body['email']):
raise SynapseError(403, "3PID denied", Codes.THREEPID_DENIED)

existingUid = yield self.datastore.get_user_id_by_threepid(
'email', body['email']
)
Expand Down Expand Up @@ -255,6 +264,9 @@ def on_POST(self, request):

msisdn = phone_number_to_msisdn(body['country'], body['phone_number'])

if not check_3pid_allowed(self.hs, "msisdn", msisdn):
raise SynapseError(403, "3PID denied", Codes.THREEPID_DENIED)

existingUid = yield self.datastore.get_user_id_by_threepid(
'msisdn', msisdn
)
Expand Down
73 changes: 61 additions & 12 deletions synapse/rest/client/v2_alpha/register.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,10 @@
)
from synapse.util.msisdn import phone_number_to_msisdn

from ._base import client_v2_patterns, interactive_auth_handler
from ._base import client_v2_patterns, interactive_auth_handler, check_3pid_allowed

import logging
import re
import hmac
from hashlib import sha1
from synapse.util.async import run_on_reactor
Expand Down Expand Up @@ -70,6 +71,9 @@ def on_POST(self, request):
'id_server', 'client_secret', 'email', 'send_attempt'
])

if not check_3pid_allowed(self.hs, "email", body['email']):
raise SynapseError(403, "3PID denied", Codes.THREEPID_DENIED)

existingUid = yield self.hs.get_datastore().get_user_id_by_threepid(
'email', body['email']
)
Expand Down Expand Up @@ -105,6 +109,9 @@ def on_POST(self, request):

msisdn = phone_number_to_msisdn(body['country'], body['phone_number'])

if not check_3pid_allowed(self.hs, "msisdn", msisdn):
raise SynapseError(403, "3PID denied", Codes.THREEPID_DENIED)

existingUid = yield self.hs.get_datastore().get_user_id_by_threepid(
'msisdn', msisdn
)
Expand Down Expand Up @@ -305,31 +312,73 @@ def on_POST(self, request):
if 'x_show_msisdn' in body and body['x_show_msisdn']:
show_msisdn = True

require_email = False
require_msisdn = False
for constraint in self.hs.config.registrations_require_3pid:
if constraint['medium'] == 'email':
require_email = True
elif constraint['medium'] == 'msisdn':
require_msisdn = True
else:
logger.warn(
"Unrecognised 3PID medium %s in registrations_require_3pid" %
constraint['medium']
Copy link
Member

Choose a reason for hiding this comment

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

Don't do string building.

)

flows = []
if self.hs.config.enable_registration_captcha:
flows = [
[LoginType.RECAPTCHA],
[LoginType.EMAIL_IDENTITY, LoginType.RECAPTCHA],
]
if not require_email and not require_msisdn:
flows.extend([[LoginType.RECAPTCHA]])
if require_email or not require_msisdn:
flows.extend([[LoginType.EMAIL_IDENTITY, LoginType.RECAPTCHA]])

if show_msisdn:
if not require_email or require_msisdn:
flows.extend([[LoginType.MSISDN, LoginType.RECAPTCHA]])
flows.extend([
[LoginType.MSISDN, LoginType.RECAPTCHA],
[LoginType.MSISDN, LoginType.EMAIL_IDENTITY, LoginType.RECAPTCHA],
])
else:
flows = [
[LoginType.DUMMY],
[LoginType.EMAIL_IDENTITY],
]
if not require_email and not require_msisdn:
flows.extend([[LoginType.DUMMY]])
if require_email or not require_msisdn:
flows.extend([[LoginType.EMAIL_IDENTITY]])

if show_msisdn:
if not require_email or require_msisdn:
flows.extend([[LoginType.MSISDN]])
flows.extend([
[LoginType.MSISDN],
[LoginType.MSISDN, LoginType.EMAIL_IDENTITY],
[LoginType.MSISDN, LoginType.EMAIL_IDENTITY]
Copy link
Member

Choose a reason for hiding this comment

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

(See comment where we do this in v1 register)

])

auth_result, params, session_id = yield self.auth_handler.check_auth(
flows, body, self.hs.get_ip_from_request(request)
)

# doublecheck that we're not trying to register an denied 3pid.
Copy link
Member

Choose a reason for hiding this comment

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

a

# the user-facing checks should already have happened when we requested
# a 3PID token to validate them in /register/email/requestToken etc
Copy link
Member

Choose a reason for hiding this comment

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

Not necessarily, since there's nothing to mandate that the user had to proxy their requestToken via the HS, so this check is important.


for constraint in self.hs.config.registrations_require_3pid:
if (
constraint['medium'] == 'email' and
auth_result and LoginType.EMAIL_IDENTITY in auth_result and
re.match(
constraint['pattern'],
auth_result[LoginType.EMAIL_IDENTITY].threepid.address
)
):
raise SynapseError(403, "3PID denied", Codes.THREEPID_DENIED)
elif (
constraint['medium'] == 'msisdn' and
auth_result and LoginType.MSISDN in auth_result and
re.match(
constraint['pattern'],
auth_result[LoginType.MSISDN].threepid.address
)
):
raise SynapseError(403, "3PID denied", Codes.THREEPID_DENIED)

if registered_user_id is not None:
logger.info(
"Already registered user ID %r for this session",
Expand Down
1 change: 1 addition & 0 deletions tests/rest/client/v2_alpha/test_register.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ def setUp(self):
self.hs.get_auth_handler = Mock(return_value=self.auth_handler)
self.hs.get_device_handler = Mock(return_value=self.device_handler)
self.hs.config.enable_registration = True
self.hs.config.registrations_require_3pid = []
self.hs.config.auto_join_rooms = []

# init the thing we're testing
Expand Down