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 6 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
19 changes: 19 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,8 @@ 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.allowed_local_3pids = config.get("allowed_local_3pids", [])
self.registration_shared_secret = config.get("registration_shared_secret")

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

# The user must provide all of the below types of 3PID when registering.
#
# registrations_require_3pid:
# - email
# - msisdn
Copy link
Member

Choose a reason for hiding this comment

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

Personally I would have the two config option entirely separate, as this doesn't support the use case of "require a work email but let people bind personal emails"

Copy link
Member Author

Choose a reason for hiding this comment

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

we're not trying to solve for that use case here, plus having talked to @dbkr it's hard to do anyway - as /account/3pid goes and adds the email to your account (letting you log in with it) as well as optionally binding it on the identity server... so you can't separate the two easily anyway.

Hence me splitting it into:

  • What 3PID media do we mandate at registration?
  • What format 3PIDs are allowed to be added to accounts on this HS?
    ...with no restrictions on formats valid on ISes.

Copy link
Member Author

Choose a reason for hiding this comment

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

btw, this is a good example of a review comment where I'm staring at it thinking "does he really want me to rewrite it again? or is he just voicing a random comment?". It might be worth clearly spelling out which comments actually block accepting the PR from your POV, otherwise the committer has no option than assume everything is a request for change...

Copy link
Member

Choose a reason for hiding this comment

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

Ok, in this case it was an observation that this feels odd and questioning whether this is the right thing to do. The synapse review process is not just about whether functional changes are right, but if we should even be doing it like that.


# Mandate that users are only allowed to associate certain formats of
# 3PIDs with accounts on this server.
#
# allowed_local_3pids:
# - 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
8 changes: 7 additions & 1 deletion synapse/handlers/register.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
from synapse import types
from synapse.types import UserID
from synapse.util.async import run_on_reactor
from synapse.util.threepids import check_3pid_allowed
from ._base import BaseHandler

logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -293,7 +294,7 @@ def register_email(self, threepidCreds):
"""

for c in threepidCreds:
logger.info("validating theeepidcred sid %s on id server %s",
logger.info("validating threepidcred sid %s on id server %s",
c['sid'], c['idServer'])
try:
identity_handler = self.hs.get_handlers().identity_handler
Expand All @@ -307,6 +308,11 @@ def register_email(self, threepidCreds):
logger.info("got threepid with medium '%s' and address '%s'",
threepid['medium'], threepid['address'])

if not check_3pid_allowed(self.hs, threepid['medium'], threepid['address']):
raise RegistrationError(
403, "Third party identifier is not allowed"
)

@defer.inlineCallbacks
def bind_emails(self, user_id, threepidCreds):
"""Links emails with a user ID and informs an identity server.
Expand Down
34 changes: 23 additions & 11 deletions synapse/rest/client/v1/register.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,15 @@ def __init__(self, hs):
self.handlers = hs.get_handlers()

def on_GET(self, request):

require_email = 'email' in self.hs.config.registrations_require_3pid
require_msisdn = 'msisdn' in self.hs.config.registrations_require_3pid

flows = []
if self.hs.config.enable_registration_captcha:
return (
200,
{"flows": [
# only support the email-only flow if we don't require MSISDN 3PIDs
if not require_msisdn:
flows.extend([
{
"type": LoginType.RECAPTCHA,
"stages": [
Expand All @@ -82,27 +87,34 @@ def on_GET(self, request):
LoginType.PASSWORD
]
},
])
# only support 3PIDless registration if no 3PIDs are required
if not require_email and not require_msisdn:
flows.extend([
{
"type": LoginType.RECAPTCHA,
"stages": [LoginType.RECAPTCHA, LoginType.PASSWORD]
}
]}
)
])
else:
return (
200,
{"flows": [
# only support the email-only flow if we don't require MSISDN 3PIDs
if require_email or not require_msisdn:
flows.extend([
{
"type": LoginType.EMAIL_IDENTITY,
"stages": [
LoginType.EMAIL_IDENTITY, LoginType.PASSWORD
]
},
}
])
# only support 3PIDless registration if no 3PIDs are required
if not require_email and not require_msisdn:
flows.extend([
{
"type": LoginType.PASSWORD
}
]}
)
])
return (200, {"flows": flows})
Copy link
Member

Choose a reason for hiding this comment

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

What does require_msisdn mean? We don't seem to add a stage for it.

Also I really can't follow whats going on here, there are two many if and nots and ands and ors. It seems that we're building up flows based on required stages, so something like the following might work better:

required_stages = []  # The stages that need to be in every flow

if self.hs.config.enable_registration_captcha:
    required_stages.append(LoginType.RECAPTCHA)

if require_email:
    required_stages.append(LoginType.EMAIL_IDENTITY)

# ... etc ...

# Now we work out what flows we need to offer. 
flows = []

# First we add the base flow with only the required stages:

flows.append({"stages": required_stages})

# If we don't require email, then we add a flow to let people optionally specify it
if not require_email:
    flows.append({"stages": required_stages + [LoginType.EMAIL_IDENTITY]})


@defer.inlineCallbacks
def on_POST(self, request):
Expand Down
21 changes: 21 additions & 0 deletions 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,6 +26,7 @@
)
from synapse.util.async import run_on_reactor
from synapse.util.msisdn import phone_number_to_msisdn
from synapse.util.threepids import check_3pid_allowed
from ._base import client_v2_patterns, interactive_auth_handler

logger = logging.getLogger(__name__)
Expand All @@ -47,6 +48,11 @@ 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, "Third party identifier is not allowed", Codes.THREEPID_DENIED,
)

existingUid = yield self.hs.get_datastore().get_user_id_by_threepid(
'email', body['email']
)
Expand Down Expand Up @@ -78,6 +84,11 @@ 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, "Third party identifier is not allowed", Codes.THREEPID_DENIED,
)

existingUid = yield self.datastore.get_user_id_by_threepid(
'msisdn', msisdn
)
Expand Down Expand Up @@ -217,6 +228,11 @@ 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, "Third party identifier is not allowed", Codes.THREEPID_DENIED,
)

existingUid = yield self.datastore.get_user_id_by_threepid(
'email', body['email']
)
Expand Down Expand Up @@ -255,6 +271,11 @@ 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, "Third party identifier is not allowed", Codes.THREEPID_DENIED,
)

existingUid = yield self.datastore.get_user_id_by_threepid(
'msisdn', msisdn
)
Expand Down
72 changes: 61 additions & 11 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 @@ -26,6 +26,7 @@
RestServlet, parse_json_object_from_request, assert_params_in_request, parse_string
)
from synapse.util.msisdn import phone_number_to_msisdn
from synapse.util.threepids import check_3pid_allowed

from ._base import client_v2_patterns, interactive_auth_handler

Expand Down Expand Up @@ -70,6 +71,11 @@ 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, "Third party identifier is not allowed", Codes.THREEPID_DENIED,
)

existingUid = yield self.hs.get_datastore().get_user_id_by_threepid(
'email', body['email']
)
Expand Down Expand Up @@ -105,6 +111,11 @@ 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, "Third party identifier is not allowed", Codes.THREEPID_DENIED,
)

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

# FIXME: need a better error than "no auth flow found" for scenarios
# where we required 3PID for registration but the user didn't give one
require_email = 'email' in self.hs.config.registrations_require_3pid
require_msisdn = 'msisdn' in self.hs.config.registrations_require_3pid

flows = []
if self.hs.config.enable_registration_captcha:
flows = [
[LoginType.RECAPTCHA],
[LoginType.EMAIL_IDENTITY, LoginType.RECAPTCHA],
]
# only support 3PIDless registration if no 3PIDs are required
if not require_email and not require_msisdn:
flows.extend([[LoginType.RECAPTCHA]])
# only support the email-only flow if we don't require MSISDN 3PIDs
if not require_msisdn:
flows.extend([[LoginType.EMAIL_IDENTITY, LoginType.RECAPTCHA]])

if show_msisdn:
# only support the MSISDN-only flow if we don't require email 3PIDs
if not require_email:
flows.extend([[LoginType.MSISDN, LoginType.RECAPTCHA]])
# always let users provide both MSISDN & email
flows.extend([
[LoginType.MSISDN, LoginType.RECAPTCHA],
[LoginType.MSISDN, LoginType.EMAIL_IDENTITY, LoginType.RECAPTCHA],
])
else:
flows = [
[LoginType.DUMMY],
[LoginType.EMAIL_IDENTITY],
]
# only support 3PIDless registration if no 3PIDs are required
if not require_email and not require_msisdn:
flows.extend([[LoginType.DUMMY]])
# only support the email-only flow if we don't require MSISDN 3PIDs
if not require_msisdn:
flows.extend([[LoginType.EMAIL_IDENTITY]])

if show_msisdn:
# only support the MSISDN-only flow if we don't require email 3PIDs
if not require_email or require_msisdn:
flows.extend([[LoginType.MSISDN]])
# always let users provide both MSISDN & email
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)
)

# Check that we're not trying to register a denied 3pid.
#
# the user-facing checks will probably already have happened in
# /register/email/requestToken when we requested a 3pid, but that's not
# guaranteed.

if (
auth_result and
(
LoginType.EMAIL_IDENTITY in auth_result or
LoginType.EMAIL_MSISDN in auth_result
)
):
medium = auth_result[LoginType.EMAIL_IDENTITY].threepid['medium']
address = auth_result[LoginType.EMAIL_IDENTITY].threepid['address']
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this need to check both msisdn and email?

Copy link
Member Author

Choose a reason for hiding this comment

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

oops, thanks


if not check_3pid_allowed(self.hs, medium, address):
raise SynapseError(
403, "Third party identifier is not allowed", Codes.THREEPID_DENIED,
)
Copy link
Member

Choose a reason for hiding this comment

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

Can we not use check_3pid_allowed here?


if registered_user_id is not None:
logger.info(
"Already registered user ID %r for this session",
Expand Down
45 changes: 45 additions & 0 deletions synapse/util/threepids.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
# -*- coding: utf-8 -*-
# Copyright 2018 New Vector Ltd
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

import logging
import re

logger = logging.getLogger(__name__)


def check_3pid_allowed(hs, medium, address):
"""Checks whether a given format of 3PID is allowed to be used on this HS

Args:
hs (synapse.server.HomeServer): server
medium (str): 3pid medium - e.g. email, msisdn
address (str): address within that medium (e.g. "[email protected]")
msisdns need to first have been canonicalised
"""
Copy link
Member

Choose a reason for hiding this comment

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

Should have a:

Returns:
    bool: ...

especially since I think there are other check_ functions raise rather than return a bool


if hs.config.allowed_local_3pids:
for constraint in hs.config.allowed_local_3pids:
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 need explicit string formatting, logger.debug does it for you.

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

return False
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