Merge pull request #102 from lambdaTotoro/master

Error on signup with taken username

nativeauthenticator/handlers.py

@@ -43,7 +43,7 @@ async def get(self):
         )
         self.finish(html)
 
-    def get_result_message(self, user):
+    def get_result_message(self, user, username):
         alert = 'alert-info'
         message = 'Your information have been sent to the admin'
 
@@ -54,11 +54,17 @@ def get_result_message(self, user):
         if not user:
             alert = 'alert-danger'
             pw_len = self.authenticator.minimum_password_length
+            taken  = self.authenticator.user_exists(username)
 
             if pw_len:
                 message = ("Something went wrong. Be sure your password has "
                            "at least {} characters, doesn't have spaces or "
                            "commas and is not too common.").format(pw_len)
+
+            elif taken:
+                message = ("Something went wrong. It appears that this "
+                           "username is already in use. Please try again "
+                           "with a different username.")
 
             else:
                 message = ("Something went wrong. Be sure your password "
@@ -77,9 +83,10 @@ async def post(self):
             'email': self.get_body_argument('email', '', strip=False),
             'has_2fa': bool(self.get_body_argument('2fa', '', strip=False)),
         }
-        user = self.authenticator.get_or_create_user(**user_info)
+        user = self.authenticator.create_user(**user_info)
+        name = self.authenticator.user_exists(user_info['username'])
 
-        alert, message = self.get_result_message(user)
+        alert, message = self.get_result_message(user, name)
 
         otp_secret, user_2fa = '', ''
         if user:

nativeauthenticator/nativeauthenticator.py

@@ -133,7 +133,7 @@ def authenticate(self, handler, data):
         username = self.normalize_username(data['username'])
         password = data['password']
 
-        user = UserInfo.find(self.db, username)
+        user = self.get_user(username)
         if not user:
             return
 
@@ -171,13 +171,19 @@ def is_password_strong(self, password):
             checks.append(not self.is_password_common(password))
 
         return all(checks)
-
-    def get_or_create_user(self, username, pw, **kwargs):
+
+    def get_user(self, username):
+        return UserInfo.find(self.db, self.normalize_username(username))
+
+    def user_exists(self, username):
+        return self.get_user(username) is not None
+
+    def create_user(self, username, pw, **kwargs):
         username = self.normalize_username(username)
-        user = UserInfo.find(self.db, username)
-        if user:
-            return user
-
+
+        if self.user_exists(username):
+            return
+        
         if not self.is_password_strong(pw) or \
            not self.validate_username(username):
             return
@@ -201,7 +207,7 @@ def get_or_create_user(self, username, pw, **kwargs):
         return user_info
 
     def change_password(self, username, new_password):
-        user = UserInfo.find(self.db, username)
+        user = self.get_user(username)
         user.password = bcrypt.hashpw(new_password.encode(), bcrypt.gensalt())
         self.db.commit()
 
@@ -222,7 +228,7 @@ def get_handlers(self, app):
         return native_handlers
 
     def delete_user(self, user):
-        user_info = UserInfo.find(self.db, user.name)
+        user_info = self.get_user(user.name)
         if user_info is not None:
             self.db.delete(user_info)
             self.db.commit()
@@ -244,7 +250,7 @@ def add_data_from_firstuse(self):
         with dbm.open(self.firstuse_db_path, 'c', 0o600) as db:
             for user in db.keys():
                 password = db[user].decode()
-                new_user = self.get_or_create_user(user.decode(), password)
+                new_user = self.create_user(user.decode(), password)
                 if not new_user:
                     error = '''User {} was not created. Check password
                                restrictions or username problems before trying

nativeauthenticator/tests/test_authenticator.py

@@ -33,27 +33,40 @@ def app():
 ])
 async def test_create_user(is_admin, open_signup, expected_authorization,
                            tmpcwd, app):
-    '''Test method get_or_create_user for new user and authorization '''
+    '''Test method create_user for new user and authorization '''
     auth = NativeAuthenticator(db=app.db)
 
     if is_admin:
         auth.admin_users = ({'johnsnow'})
     if open_signup:
         auth.open_signup = True
 
-    auth.get_or_create_user('johnsnow', 'password')
+    auth.create_user('johnsnow', 'password')
     user_info = UserInfo.find(app.db, 'johnsnow')
     assert user_info.username == 'johnsnow'
     assert user_info.is_authorized == expected_authorization
 
 
-async def test_create_user_bas_characters(tmpcwd, app):
-    '''Test method get_or_create_user with bad characters on username'''
+async def test_create_user_bad_characters(tmpcwd, app):
+    '''Test method create_user with bad characters on username'''
     auth = NativeAuthenticator(db=app.db)
-    assert not auth.get_or_create_user('john snow', 'password')
-    assert not auth.get_or_create_user('john,snow', 'password')
+    assert not auth.create_user('john snow', 'password')
+    assert not auth.create_user('john,snow', 'password')
 
 
+async def test_create_user_twice(tmpcwd, app):
+    '''Test if creating users with an existing handle errors.'''
+    auth = NativeAuthenticator(db=app.db)
+
+    # First creation should succeed.
+    assert auth.create_user('johnsnow', 'password')
+
+    # Creating the same account again should fail.
+    assert not auth.create_user('johnsnow', 'password')
+
+    # Creating a user with same handle but different pw should also fail.
+    assert not auth.create_user('johnsnow', 'adifferentpassword')
+
 @pytest.mark.parametrize("password,min_len,expected", [
     ("qwerty", 1, False),
     ("agameofthrones", 1, True),
@@ -62,11 +75,11 @@ async def test_create_user_bas_characters(tmpcwd, app):
 ])
 async def test_create_user_with_strong_passwords(password, min_len, expected,
                                                  tmpcwd, app):
-    '''Test if method get_or_create_user and strong passwords'''
+    '''Test if method create_user and strong passwords mesh'''
     auth = NativeAuthenticator(db=app.db)
     auth.check_common_password = True
     auth.minimum_password_length = min_len
-    user = auth.get_or_create_user('johnsnow', password)
+    user = auth.create_user('johnsnow', password)
     assert bool(user) == expected
 
 
@@ -99,7 +112,7 @@ async def test_authentication(username, password, authorized, expected,
                               tmpcwd, app):
     '''Test if authentication fails with a unexistent user'''
     auth = NativeAuthenticator(db=app.db)
-    auth.get_or_create_user('johnsnow', 'password')
+    auth.create_user('johnsnow', 'password')
     if authorized:
         UserInfo.change_authorization(app.db, 'johnsnow')
     response = await auth.authenticate(app, {'username': username,
@@ -131,7 +144,7 @@ async def test_authentication_login_count(tmpcwd, app):
     auth = NativeAuthenticator(db=app.db)
     infos = {'username': 'johnsnow', 'password': 'password'}
     wrong_infos = {'username': 'johnsnow', 'password': 'wrong_password'}
-    auth.get_or_create_user(infos['username'], infos['password'])
+    auth.create_user(infos['username'], infos['password'])
     UserInfo.change_authorization(app.db, 'johnsnow')
 
     assert not auth.login_attempts
@@ -152,7 +165,7 @@ async def test_authentication_with_exceed_atempts_of_login(tmpcwd, app):
     auth.secs_before_next_try = 10
 
     infos = {'username': 'johnsnow', 'password': 'wrongpassword'}
-    auth.get_or_create_user(infos['username'], 'password')
+    auth.create_user(infos['username'], 'password')
     UserInfo.change_authorization(app.db, 'johnsnow')
 
     for i in range(3):
@@ -170,16 +183,26 @@ async def test_authentication_with_exceed_atempts_of_login(tmpcwd, app):
 
 async def test_change_password(tmpcwd, app):
     auth = NativeAuthenticator(db=app.db)
-    user = auth.get_or_create_user('johnsnow', 'password')
+    user = auth.create_user('johnsnow', 'password')
     assert user.is_valid_password('password')
     auth.change_password('johnsnow', 'newpassword')
     assert not user.is_valid_password('password')
     assert user.is_valid_password('newpassword')
 
 
+async def test_get_user(tmpcwd, app):
+    auth = NativeAuthenticator(db=app.db)
+    auth.create_user('johnsnow', 'password')
+
+    # Getting existing user is successful.
+    assert auth.get_user('johnsnow') != None
+
+    # Getting non-existing user fails.
+    assert auth.get_user('samwelltarly') == None
+
 async def test_delete_user(tmpcwd, app):
     auth = NativeAuthenticator(db=app.db)
-    auth.get_or_create_user('johnsnow', 'password')
+    auth.create_user('johnsnow', 'password')
 
     user = type('User', (), {'name': 'johnsnow'})
     auth.delete_user(user)