Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error on signup with taken username #102

Merged
merged 5 commits into from
Mar 29, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
13 changes: 10 additions & 3 deletions nativeauthenticator/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,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'

Expand All @@ -51,11 +51,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 "
Expand All @@ -71,9 +77,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:
Expand Down
25 changes: 17 additions & 8 deletions nativeauthenticator/nativeauthenticator.py
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,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

Expand Down Expand Up @@ -166,13 +166,22 @@ 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):
username = self.normalize_username(username)
user = UserInfo.find(self.db, username)

def get_user(self, username):
user = UserInfo.find(self.db, self.normalize_username(username))
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I am not mistaken, if the user doesn't exist it already returns None, so we can remove the following if and just return the result of this search

if user:
return user
Copy link
Collaborator

Choose a reason for hiding this comment

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

then this won't be necessary


def user_exists(self, username):
return self.get_user(username) is not None

def create_user(self, username, pw, **kwargs):

Copy link
Collaborator

Choose a reason for hiding this comment

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

can you remove this line to follow patterns please?

username = self.normalize_username(username)

if self.user_exists(username):
return

if not self.is_password_strong(pw) or \
not self.validate_username(username):
return
Expand All @@ -193,7 +202,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()

Expand All @@ -214,7 +223,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()
Expand All @@ -236,7 +245,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
Expand Down
39 changes: 26 additions & 13 deletions nativeauthenticator/tests/test_authenticator.py
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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


Expand All @@ -81,7 +94,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,
Expand Down Expand Up @@ -113,7 +126,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
Expand All @@ -134,7 +147,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):
Expand All @@ -152,7 +165,7 @@ 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')
Expand All @@ -161,7 +174,7 @@ async def test_change_password(tmpcwd, app):

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)
Expand Down