Fix v2_alpha registration. Add unit tests.

V2 Registration forced everyone (including ASes) to create a password for a
user, when ASes should be able to omit passwords. Also unbreak AS registration
in general which checked too early if the given username was claimed by an AS;
it was checked before knowing if the AS was the one doing the registration! Add
unit tests for AS reg, user reg and disabled_registration flag.

synapse/handlers/register.py

@@ -73,7 +73,8 @@ class RegistrationHandler(BaseHandler):
             localpart : The local part of the user ID to register. If None,
               one will be randomly generated.
             password (str) : The password to assign to this user so they can
-            login again.
+            login again. This can be None which means they cannot login again
+            via a password (e.g. the user is an application service user).
         Returns:
             A tuple of (user_id, access_token).
         Raises:

synapse/rest/client/v2_alpha/register.py

@@ -19,7 +19,7 @@ from synapse.api.constants import LoginType
 from synapse.api.errors import SynapseError, Codes
 from synapse.http.servlet import RestServlet
 
-from ._base import client_v2_pattern, parse_request_allow_empty
+from ._base import client_v2_pattern, parse_json_dict_from_request
 
 import logging
 import hmac
@@ -55,30 +55,52 @@ class RegisterRestServlet(RestServlet):
     @defer.inlineCallbacks
     def on_POST(self, request):
         yield run_on_reactor()
+        body = parse_json_dict_from_request(request)
 
-        body = parse_request_allow_empty(request)
+        # we do basic sanity checks here because the auth layer will store these
-        # we do basic sanity checks here because the auth
+        # in sessions. Pull out the username/password provided to us.
-        # layer will store these in sessions
+        desired_password = None
         if 'password' in body:
-            if ((not isinstance(body['password'], str) and
+            if (not isinstance(body['password'], basestring) or
-                    not isinstance(body['password'], unicode)) or
                     len(body['password']) > 512):
                 raise SynapseError(400, "Invalid password")
+            desired_password = body["password"]
 
+        desired_username = None
         if 'username' in body:
-            if ((not isinstance(body['username'], str) and
+            if (not isinstance(body['username'], basestring) or
-                    not isinstance(body['username'], unicode)) or
                     len(body['username']) > 512):
                 raise SynapseError(400, "Invalid username")
             desired_username = body['username']
-            yield self.registration_handler.check_username(desired_username)
 
-        is_using_shared_secret = False
+        appservice = None
-        is_application_server = False
-
-        service = None
         if 'access_token' in request.args:
-            service = yield self.auth.get_appservice_by_req(request)
+            appservice = yield self.auth.get_appservice_by_req(request)
+
+        # fork off as soon as possible for ASes and shared secret auth which
+        # have completely different registration flows to normal users
+
+        # == Application Service Registration ==
+        if appservice:
+            result = yield self._do_appservice_registration(desired_username)
+            defer.returnValue((200, result))  # we throw for non 200 responses
+            return
+
+        # == Shared Secret Registration == (e.g. create new user scripts)
+        if 'mac' in body:
+            # FIXME: Should we really be determining if this is shared secret
+            # auth based purely on the 'mac' key?
+            result = yield self._do_shared_secret_registration(
+                desired_username, desired_password, body["mac"]
+            )
+            defer.returnValue((200, result))  # we throw for non 200 responses
+            return
+
+        # == Normal User Registration == (everyone else)
+        if self.hs.config.disable_registration:
+            raise SynapseError(403, "Registration has been disabled")
+
+        yield self.registration_handler.check_username(desired_username)
 
         if self.hs.config.enable_registration_captcha:
             flows = [
@@ -91,39 +113,20 @@ class RegisterRestServlet(RestServlet):
                 [LoginType.EMAIL_IDENTITY]
             ]
 
-        result = None
+        authed, result, params = yield self.auth_handler.check_auth(
-        if service:
+            flows, body, self.hs.get_ip_from_request(request)
-            is_application_server = True
-            params = body
-        elif 'mac' in body:
-            # Check registration-specific shared secret auth
-            if 'username' not in body:
-                raise SynapseError(400, "", Codes.MISSING_PARAM)
-            self._check_shared_secret_auth(
-                body['username'], body['mac']
-            )
-            is_using_shared_secret = True
-            params = body
-        else:
-            authed, result, params = yield self.auth_handler.check_auth(
-                flows, body, self.hs.get_ip_from_request(request)
-            )
-
-            if not authed:
-                defer.returnValue((401, result))
-
-        can_register = (
-            not self.hs.config.disable_registration
-            or is_application_server
-            or is_using_shared_secret
         )
-        if not can_register:
-            raise SynapseError(403, "Registration has been disabled")
 
+        if not authed:
+            defer.returnValue((401, result))
+            return
+
+        # NB: This may be from the auth handler and NOT from the POST
         if 'password' not in params:
-            raise SynapseError(400, "", Codes.MISSING_PARAM)
+            raise SynapseError(400, "Missing password.", Codes.MISSING_PARAM)
-        desired_username = params['username'] if 'username' in params else None
+
-        new_password = params['password']
+        desired_username = params.get("username", None)
+        new_password = params.get("password", None)
 
         (user_id, token) = yield self.registration_handler.register(
             localpart=desired_username,
@@ -156,18 +159,21 @@ class RegisterRestServlet(RestServlet):
             else:
                 logger.info("bind_email not specified: not binding email")
 
-        result = {
+        result = self._create_registration_details(user_id, token)
-            "user_id": user_id,
-            "access_token": token,
-            "home_server": self.hs.hostname,
-        }
-
         defer.returnValue((200, result))
 
     def on_OPTIONS(self, _):
         return 200, {}
 
-    def _check_shared_secret_auth(self, username, mac):
+    @defer.inlineCallbacks
+    def _do_appservice_registration(self, username):
+        (user_id, token) = yield self.registration_handler.register(
+            localpart=username
+        )
+        defer.returnValue(self._create_registration_details(user_id, token))
+
+    @defer.inlineCallbacks
+    def _do_shared_secret_registration(self, username, password, mac):
         if not self.hs.config.registration_shared_secret:
             raise SynapseError(400, "Shared secret registration is not enabled")
 
@@ -183,13 +189,23 @@ class RegisterRestServlet(RestServlet):
             digestmod=sha1,
         ).hexdigest()
 
-        if compare_digest(want_mac, got_mac):
+        if not compare_digest(want_mac, got_mac):
-            return True
-        else:
             raise SynapseError(
                 403, "HMAC incorrect",
             )
 
+        (user_id, token) = yield self.registration_handler.register(
+            localpart=username, password=password
+        )
+        defer.returnValue(self._create_registration_details(user_id, token))
+
+    def _create_registration_details(self, user_id, token):
+        return {
+            "user_id": user_id,
+            "access_token": token,
+            "home_server": self.hs.hostname,
+        }
+
 
 def register_servlets(hs, http_server):
     RegisterRestServlet(hs).register(http_server)

tests/rest/client/v2_alpha/test_register.py

@@ -0,0 +1,132 @@
+from synapse.rest.client.v2_alpha.register import RegisterRestServlet
+from synapse.api.errors import SynapseError
+from twisted.internet import defer
+from mock import Mock, MagicMock
+from tests import unittest
+import json
+
+
+class RegisterRestServletTestCase(unittest.TestCase):
+
+    def setUp(self):
+        # do the dance to hook up request data to self.request_data
+        self.request_data = ""
+        self.request = Mock(
+            content=Mock(read=Mock(side_effect=lambda: self.request_data)),
+        )
+        self.request.args = {}
+
+        self.appservice = None
+        self.auth = Mock(get_appservice_by_req=Mock(
+            side_effect=lambda x: defer.succeed(self.appservice))
+        )
+
+        self.auth_result = (False, None, None)
+        self.auth_handler = Mock(
+            check_auth=Mock(side_effect=lambda x,y,z: self.auth_result)
+        )
+        self.registration_handler = Mock()
+        self.identity_handler = Mock()
+        self.login_handler = Mock()
+
+        # do the dance to hook it up to the hs global
+        self.handlers = Mock(
+            auth_handler=self.auth_handler,
+            registration_handler=self.registration_handler,
+            identity_handler=self.identity_handler,
+            login_handler=self.login_handler
+        )
+        self.hs = Mock()
+        self.hs.hostname = "superbig~testing~thing.com"
+        self.hs.get_auth = Mock(return_value=self.auth)
+        self.hs.get_handlers = Mock(return_value=self.handlers)
+        self.hs.config.disable_registration = False
+
+        # init the thing we're testing
+        self.servlet = RegisterRestServlet(self.hs)
+
+    @defer.inlineCallbacks
+    def test_POST_appservice_registration_valid(self):
+        user_id = "@kermit:muppet"
+        token = "kermits_access_token"
+        self.request.args = {
+            "access_token": "i_am_an_app_service"
+        }
+        self.request_data = json.dumps({
+            "username": "kermit"
+        })
+        self.appservice = {
+            "id": "1234"
+        }
+        self.registration_handler.register = Mock(return_value=(user_id, token))
+        result = yield self.servlet.on_POST(self.request)
+        self.assertEquals(result, (200, {
+            "user_id": user_id,
+            "access_token": token,
+            "home_server": self.hs.hostname
+        }))
+
+    @defer.inlineCallbacks
+    def test_POST_appservice_registration_invalid(self):
+        self.request.args = {
+            "access_token": "i_am_an_app_service"
+        }
+        self.request_data = json.dumps({
+            "username": "kermit"
+        })
+        self.appservice = None  # no application service exists
+        result = yield self.servlet.on_POST(self.request)
+        self.assertEquals(result, (401, None))
+
+    def test_POST_bad_password(self):
+        self.request_data = json.dumps({
+            "username": "kermit",
+            "password": 666
+        })
+        d = self.servlet.on_POST(self.request)
+        return self.assertFailure(d, SynapseError)
+
+    def test_POST_bad_username(self):
+        self.request_data = json.dumps({
+            "username": 777,
+            "password": "monkey"
+        })
+        d = self.servlet.on_POST(self.request)
+        return self.assertFailure(d, SynapseError)
+
+    @defer.inlineCallbacks
+    def test_POST_user_valid(self):
+        user_id = "@kermit:muppet"
+        token = "kermits_access_token"
+        self.request_data = json.dumps({
+            "username": "kermit",
+            "password": "monkey"
+        })
+        self.registration_handler.check_username = Mock(return_value=True)
+        self.auth_result = (True, None, {
+            "username": "kermit",
+            "password": "monkey"
+        })
+        self.registration_handler.register = Mock(return_value=(user_id, token))
+
+        result = yield self.servlet.on_POST(self.request)
+        self.assertEquals(result, (200, {
+            "user_id": user_id,
+            "access_token": token,
+            "home_server": self.hs.hostname
+        }))
+
+    def test_POST_disabled_registration(self):
+        self.hs.config.disable_registration = True
+        self.request_data = json.dumps({
+            "username": "kermit",
+            "password": "monkey"
+        })
+        self.registration_handler.check_username = Mock(return_value=True)
+        self.auth_result = (True, None, {
+            "username": "kermit",
+            "password": "monkey"
+        })
+        self.registration_handler.register = Mock(return_value=("@user:id", "t"))
+        d = self.servlet.on_POST(self.request)
+        return self.assertFailure(d, SynapseError)