Adds better error handling and tests to user registration

onegov/user/collection.py

@@ -1,8 +1,18 @@
+from onegov.core.crypto import random_token
 from onegov.user.model import User
-from onegov.user.errors import UnknownUserError
+from onegov.user.errors import (
+    AlreadyActivatedError,
+    ExistingUserError,
+    InsecurePasswordError,
+    InvalidActivationTokenError,
+    UnknownUserError,
+)
 from sqlalchemy import sql
 
 
+MIN_PASSWORD_LENGTH = 8
+
+
 class UserCollection(object):
     """ Manages a list of users.
 
@@ -77,6 +87,36 @@ def by_username_and_password(self, username, password):
         else:
             return None
 
+    def register(self, username, password, role='member'):
+        """ Registers a new user.
+
+        The so created user needs to activated with a token before it becomes
+        active. Use the activation_token in the data dictionary together
+        with the :meth:`activate_with_token` function.
+
+        """
+
+        assert username
+
+        # we could implement a proper password policy, but a min-length of
+        # of eight characters is a good start. What we don't want is someone
+        # registering a user with a password of one character.
+        if len(password) < MIN_PASSWORD_LENGTH:
+            raise InsecurePasswordError()
+
+        if self.by_username(username):
+            raise ExistingUserError("{} already exists".format(username))
+
+        return self.add(
+            username=username,
+            password=password,
+            role=role,
+            data={
+                'activation_token': random_token()
+            },
+            active=False
+        )
+
     def activate_with_token(self, username, token):
         """ Activates the user if the given token matches the verification
         token stored in the data dictionary.
@@ -85,15 +125,17 @@ def activate_with_token(self, username, token):
         user = self.by_username(username)
 
         if not user:
-            return False
+            raise UnknownUserError("{} does not exist".format(username))
+
+        if user.active:
+            raise AlreadyActivatedError("{} already active".format(username))
 
         if user.data.get('activation_token', object()) != token:
-            return False
+            raise InvalidActivationTokenError("{} is invalid".format(token))
 
         del user.data['activation_token']
         user.active = True
-
-        return True
+        self.session.flush()
 
     def delete(self, username):
         """ Deletes the user if it exists.

onegov/user/errors.py

@@ -6,3 +6,22 @@ def __init__(self, message):
 
 class UnknownUserError(OnegovUserError):
     """ Raised when a user was not found. """
+
+
+class InvalidActivationTokenError(OnegovUserError):
+    """ Raised when the given activation token doesn't match. """
+
+
+class ExistingUserError(OnegovUserError):
+    """ Raised when a new user already exists. """
+
+
+class AlreadyActivatedError(OnegovUserError):
+    """ Raised when a user was already activated. """
+
+
+class InsecurePasswordError(OnegovUserError):
+    """ Raised when a user's password is not secure enough. """
+
+    def __init__(self):
+        pass

onegov/user/forms/registration.py

@@ -1,4 +1,3 @@
-from onegov.core.crypto import random_token
 from onegov.form import Form
 from onegov.user import _
 from onegov.user.collection import UserCollection
@@ -43,27 +42,13 @@ class RegistrationForm(Form):
         ))]
     )
 
-    def create_user(self, session, role='member'):
-        """ Creates the user using the information on the form.
+    def register_user(self, session, role='member'):
+        """ Registers the user using the information on the form.
 
-        The so created user needs to activated with a token before it becomes
-        active. Use the activation_token in the data dictionary together
-        with the :meth:`onegov.user.collections.activate_with_token`
-        function.
+        See :meth:`onegov.user.collections.UserCollection.register_user` for
+        more information.
 
         """
 
-        users = UserCollection(session)
-
-        if users.by_username(self.username.data):
-            return False
-
-        return users.add(
-            username=self.username.data,
-            password=self.password.data,
-            role=role,
-            data={
-                'activation_token': random_token()
-            },
-            active=False
-        )
+        return UserCollection(session).register(
+            self.username.data, self.password.data)

onegov/user/tests/test_collection.py

@@ -1,7 +1,14 @@
 import pytest
 
+from onegov.core.crypto import RANDOM_TOKEN_LENGTH
 from onegov.user import UserCollection
-from onegov.user.errors import UnknownUserError
+from onegov.user.errors import (
+    AlreadyActivatedError,
+    ExistingUserError,
+    InsecurePasswordError,
+    InvalidActivationTokenError,
+    UnknownUserError,
+)
 
 
 def test_user_add(session):
@@ -72,3 +79,37 @@ def may_login(username, password):
     assert user.password.startswith('$bcrypt-sha256$2a')
     assert user.password_hash.startswith('$bcrypt-sha256$2a')
     assert not may_login('AzureDiamon', '')
+
+
+def test_register_user(session):
+
+    users = UserCollection(session)
+
+    with pytest.raises(AssertionError):
+        users.register(None, None)
+
+    with pytest.raises(InsecurePasswordError):
+        users.register('user', 'short')
+
+    user = users.register('user', 'p@ssw0rd')
+    token = user.data['activation_token']
+
+    assert len(token) == RANDOM_TOKEN_LENGTH
+    assert not user.active
+    assert user.role == 'member'
+
+    with pytest.raises(UnknownUserError):
+        users.activate_with_token('waldo', 'asdf')
+
+    with pytest.raises(InvalidActivationTokenError):
+        users.activate_with_token('user', 'asdf')
+
+    users.activate_with_token('user', token)
+    assert user.active
+    assert 'activation_token' not in user.data
+
+    with pytest.raises(ExistingUserError):
+        user = users.register('user', 'p@ssw0rd')
+
+    with pytest.raises(AlreadyActivatedError):
+        users.activate_with_token('user', token)