From 28c5b288c32d3e47d642bb63622123230a22ccee Mon Sep 17 00:00:00 2001 From: Jonas Betzendahl Date: Sun, 31 Oct 2021 21:59:55 +0100 Subject: [PATCH 1/5] Ask to confirm old/new passwords on signup and change --- nativeauthenticator/handlers.py | 37 +++++-- nativeauthenticator/nativeauthenticator.py | 24 +++-- .../templates/change-password-admin.html | 56 +++++++++++ .../templates/change-password.html | 28 +++++- nativeauthenticator/templates/signup.html | 9 ++ .../tests/test_authenticator.py | 97 ++++++++++++++----- 6 files changed, 206 insertions(+), 45 deletions(-) create mode 100644 nativeauthenticator/templates/change-password-admin.html diff --git a/nativeauthenticator/handlers.py b/nativeauthenticator/handlers.py index 4f584ce..09586e8 100644 --- a/nativeauthenticator/handlers.py +++ b/nativeauthenticator/handlers.py @@ -58,7 +58,7 @@ async def get(self): ) self.finish(html) - def get_result_message(self, user, taken, human=True): + def get_result_message(self, user, taken, match, human=True): alert = "alert-info" message = "Your information has been sent to the admin" if user and user.login_email_sent: @@ -72,6 +72,9 @@ def get_result_message(self, user, taken, human=True): "username is already in use. Please try again " "with a different username." ) + elif not match: + alert = "alert-danger" + message = "Your passwords did not match. Please try again." else: # Error if user creation was not successful. if not user: @@ -134,6 +137,7 @@ async def post(self): user_info = { "username": self.get_body_argument("username", strip=False), "pw": self.get_body_argument("pw", strip=False), + "pwc": self.get_body_argument("pwc", strip=False), "email": self.get_body_argument("email", "", strip=False), "has_2fa": bool(self.get_body_argument("2fa", "", strip=False)), } @@ -143,7 +147,10 @@ async def post(self): user = False taken = False - alert, message = self.get_result_message(user, taken, assume_human) + match = self.get_body_argument("pw", strip=False) == self.get_body_argument( + "pwc", strip=False + ) + alert, message = self.get_result_message(user, taken, match, assume_human) otp_secret, user_2fa = "", "" if user: @@ -266,8 +273,12 @@ async def get(self): @web.authenticated async def post(self): user = await self.get_current_user() - new_password = self.get_body_argument("password", strip=False) - success = self.authenticator.change_password(user.name, new_password) + old_password = self.get_body_argument("old_password", strip=False) + new_password = self.get_body_argument("new_password", strip=False) + confirmation = self.get_body_argument("con_password", strip=False) + success = self.authenticator.user_change_password( + user.name, old_password, new_password, confirmation + ) if success: alert = "alert-success" @@ -295,19 +306,26 @@ async def get(self, user_name): if not self.authenticator.user_exists(user_name): raise web.HTTPError(404) html = await self.render_template( - "change-password.html", + "change-password-admin.html", user_name=user_name, ) self.finish(html) @admin_users_scope async def post(self, user_name): - new_password = self.get_body_argument("password", strip=False) - success = self.authenticator.change_password(user_name, new_password) + new_password = self.get_body_argument("new_password", strip=False) + confirmation = self.get_body_argument("con_password", strip=False) + success = self.authenticator.change_password( + user_name, new_password, confirmation + ) if success: alert = "alert-success" msg = f"The password for {user_name} has been changed successfully" + elif not new_password == confirmation: + alert = "alert-danger" + pw_len = self.authenticator.minimum_password_length + msg = "The passwords didn't match. Please try again." else: alert = "alert-danger" pw_len = self.authenticator.minimum_password_length @@ -318,7 +336,10 @@ async def post(self, user_name): ) html = await self.render_template( - "change-password.html", user_name=user_name, result_message=msg, alert=alert + "change-password-admin.html", + user_name=user_name, + result_message=msg, + alert=alert, ) self.finish(html) diff --git a/nativeauthenticator/nativeauthenticator.py b/nativeauthenticator/nativeauthenticator.py index 3ba99f7..f463a92 100644 --- a/nativeauthenticator/nativeauthenticator.py +++ b/nativeauthenticator/nativeauthenticator.py @@ -297,13 +297,13 @@ def get_authed_users(self): def user_exists(self, username): return self.get_user(username) is not None - def create_user(self, username, pw, **kwargs): + def create_user(self, username, pw, pwc, **kwargs): username = self.normalize_username(username) - if self.user_exists(username): + if self.user_exists(username) or not self.validate_username(username): return - if not self.is_password_strong(pw) or not self.validate_username(username): + if not self.is_password_strong(pw) or not (pw == pwc): return if not self.enable_signup: @@ -374,10 +374,14 @@ def get_unauthed_amount(self): return unauthed - def change_password(self, username, new_password): + def change_password(self, username, new_password, confirmation): user = self.get_user(username) - criteria = [user is not None, self.is_password_strong(new_password)] + criteria = [ + user is not None, + self.is_password_strong(new_password), + new_password == confirmation, + ] if not all(criteria): return @@ -385,6 +389,14 @@ def change_password(self, username, new_password): self.db.commit() return True + def user_change_password(self, username, old_password, new_password, confirmation): + user = self.get_user(username) + + if (user is None) or (not user.is_valid_password(old_password)): + return + + return self.change_password(username, new_password, confirmation) + def validate_username(self, username): invalid_chars = [",", " "] if any((char in username) for char in invalid_chars): @@ -428,7 +440,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.create_user(user.decode(), password) + new_user = self.create_user(user.decode(), password, password) if not new_user: error = ( f"User {user} was not created. Check password " diff --git a/nativeauthenticator/templates/change-password-admin.html b/nativeauthenticator/templates/change-password-admin.html new file mode 100644 index 0000000..d88da27 --- /dev/null +++ b/nativeauthenticator/templates/change-password-admin.html @@ -0,0 +1,56 @@ +{% extends "page.html" %} + +{% block script %} +{{ super() }} + +{% endblock script %} + +{% block main %} +
+
+

+ Change password for {{user_name}} +

+ +
+ +
+ + + + +
+

+ + +
+ +
+

+ + +
+
+ + {% if result_message %} + + {% endif %} +
+{% endblock %} diff --git a/nativeauthenticator/templates/change-password.html b/nativeauthenticator/templates/change-password.html index cedda58..8425033 100644 --- a/nativeauthenticator/templates/change-password.html +++ b/nativeauthenticator/templates/change-password.html @@ -6,12 +6,18 @@ document.addEventListener('DOMContentLoaded', function() { let button = document.getElementById('eye'); button.addEventListener("click", function(e) { - let pwd = document.getElementById("password_input"); + let opwd = document.getElementById("password_prove"); + let npwd = document.getElementById("password_input"); + let cpwd = document.getElementById("password_confirm"); if (pwd.getAttribute("type") === "password") { - pwd.setAttribute("type", "text"); + opwd.setAttribute("type", "text"); + npwd.setAttribute("type", "text"); + cpwd.setAttribute("type", "text"); button.textContent = "🔑"; } else { - pwd.setAttribute("type", "password"); + opwd.setAttribute("type", "password"); + npwd.setAttribute("type", "password"); + cpwd.setAttribute("type", "password"); button.textContent = "👁"; } }); @@ -27,14 +33,26 @@

- +
- +

+ + +
+ +
+

+ + +
+ +
+

diff --git a/nativeauthenticator/templates/signup.html b/nativeauthenticator/templates/signup.html index 8435493..de288f9 100644 --- a/nativeauthenticator/templates/signup.html +++ b/nativeauthenticator/templates/signup.html @@ -7,11 +7,14 @@ let button = document.getElementById('eye'); button.addEventListener("click", function(e) { let pwd = document.getElementById("password_input"); + let pwdc = document.getElementById("password_confirm"); if (pwd.getAttribute("type") === "password") { pwd.setAttribute("type", "text"); + pwdc.setAttribute("type", "text"); button.textContent = "🔑"; } else { pwd.setAttribute("type", "password"); + pwdc.setAttribute("type", "password"); button.textContent = "👁"; } }); @@ -86,6 +89,12 @@

+ + +
+ +
+

{% if two_factor_auth %} diff --git a/nativeauthenticator/tests/test_authenticator.py b/nativeauthenticator/tests/test_authenticator.py index 87de477..3fc2ea3 100644 --- a/nativeauthenticator/tests/test_authenticator.py +++ b/nativeauthenticator/tests/test_authenticator.py @@ -48,7 +48,7 @@ async def test_create_user(is_admin, open_signup, expected_authorization, tmpcwd if open_signup: auth.open_signup = True - auth.create_user("johnsnow", "password") + auth.create_user("johnsnow", "password", "password") user_info = UserInfo.find(app.db, "johnsnow") assert user_info.username == "johnsnow" assert user_info.is_authorized == expected_authorization @@ -57,8 +57,8 @@ async def test_create_user(is_admin, open_signup, expected_authorization, tmpcwd 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.create_user("john snow", "password") - assert not auth.create_user("john,snow", "password") + assert not auth.create_user("john snow", "password", "password") + assert not auth.create_user("john,snow", "password", "password") async def test_create_user_twice(tmpcwd, app): @@ -66,13 +66,13 @@ async def test_create_user_twice(tmpcwd, app): auth = NativeAuthenticator(db=app.db) # First creation should succeed. - assert auth.create_user("johnsnow", "password") + assert auth.create_user("johnsnow", "password", "password") # Creating the same account again should fail. - assert not auth.create_user("johnsnow", "password") + assert not auth.create_user("johnsnow", "password", "password") # Creating a user with same handle but different pw should also fail. - assert not auth.create_user("johnsnow", "adifferentpassword") + assert not auth.create_user("johnsnow", "adifferentpassword", "adifferentpassword") async def test_get_authed_users(tmpcwd, app): @@ -82,13 +82,13 @@ async def test_get_authed_users(tmpcwd, app): auth.admin_users = set() assert auth.get_authed_users() == set() - auth.create_user("johnsnow", "password") + auth.create_user("johnsnow", "password", "password") assert auth.get_authed_users() == set() UserInfo.change_authorization(app.db, "johnsnow") assert auth.get_authed_users() == set({"johnsnow"}) - auth.create_user("daenerystargaryen", "anotherpassword") + auth.create_user("daenerystargaryen", "anotherpassword", "anotherpassword") assert auth.get_authed_users() == set({"johnsnow"}) auth.admin_users = set({"daenerystargaryen"}) @@ -103,16 +103,16 @@ async def test_get_unauthed_amount(tmpcwd, app): auth.admin_users = set() assert auth.get_unauthed_amount() == 0 - auth.create_user("johnsnow", "password") + auth.create_user("johnsnow", "password", "password") assert auth.get_unauthed_amount() == 1 UserInfo.change_authorization(app.db, "johnsnow") assert auth.get_unauthed_amount() == 0 - auth.create_user("daenerystargaryen", "anotherpassword") + auth.create_user("daenerystargaryen", "anotherpassword", "anotherpassword") assert auth.get_unauthed_amount() == 1 - auth.create_user("tyrionlannister", "yetanotherpassword") + auth.create_user("tyrionlannister", "yetanotherpassword", "yetanotherpassword") assert auth.get_unauthed_amount() == 2 auth.admin_users = set({"daenerystargaryen"}) @@ -135,15 +135,15 @@ async def test_create_user_with_strong_passwords( auth = NativeAuthenticator(db=app.db) auth.check_common_password = True auth.minimum_password_length = min_len - user = auth.create_user("johnsnow", password) + user = auth.create_user("johnsnow", password, password) assert bool(user) == expected async def test_change_password(tmpcwd, app): auth = NativeAuthenticator(db=app.db) - user = auth.create_user("johnsnow", "password") + user = auth.create_user("johnsnow", "password", "password") assert user.is_valid_password("password") - auth.change_password("johnsnow", "newpassword") + auth.change_password("johnsnow", "newpassword", "newpassword") assert not user.is_valid_password("password") assert user.is_valid_password("newpassword") @@ -154,24 +154,69 @@ async def test_no_change_to_bad_password(tmpcwd, app): auth.check_common_password = True auth.minimum_password_length = 8 - auth.create_user("johnsnow", "ironwood") + auth.create_user("johnsnow", "ironwood", "ironwood") # Can't change password of nonexistent users. - assert auth.change_password("samwelltarly", "palanquin") is None + assert auth.change_password("samwelltarly", "palanquin", "palanquin") is None assert auth.get_user("johnsnow").is_valid_password("ironwood") # Can't change password to something too short. - assert auth.change_password("johnsnow", "mummer") is None + assert auth.change_password("johnsnow", "mummer", "mummer") is None assert auth.get_user("johnsnow").is_valid_password("ironwood") # Can't change password to something too common. - assert auth.change_password("johnsnow", "dragon") is None + assert auth.change_password("johnsnow", "dragon", "dragon") is None assert auth.get_user("johnsnow").is_valid_password("ironwood") # CAN change password to something fulfilling criteria. - assert auth.change_password("johnsnow", "DaenerysTargaryen") is not None + assert auth.change_password("johnsnow", "Daenerys", "Daenerys") is not None assert not auth.get_user("johnsnow").is_valid_password("ironwood") - assert auth.get_user("johnsnow").is_valid_password("DaenerysTargaryen") + assert auth.get_user("johnsnow").is_valid_password("Daenerys") + + +async def test_password_confirmation(tmpcwd, app): + """Test password confirmation mechanisms.""" + auth = NativeAuthenticator(db=app.db) + + # Can create with matching confirmation entry. + assert auth.create_user("johnsnow", "ironwood", "ironwood") + # Cannot create with empty confirmation entry. + assert not auth.create_user("johnsnow", "ironwood", "") + # Cannot create with incorrect confirmation entry. + assert not auth.create_user("johnsnow", "ironwood", "throne") + + assert auth.get_user("johnsnow").is_valid_password("ironwood") + + # Can change with matching confirmation entry. + assert auth.change_password("johnsnow", "westeros", "westeros") is not None + assert auth.get_user("johnsnow").is_valid_password("westeros") + # Cannot change with empty confirmation entry. + assert auth.change_password("johnsnow", "Daenerys", "") is None + assert auth.get_user("johnsnow").is_valid_password("westeros") + # Cannot change with incorrect confirmation entry. + assert auth.change_password("johnsnow", "Daenerys", "throne") is None + assert auth.get_user("johnsnow").is_valid_password("westeros") + + +async def test_user_change_password(tmpcwd, app): + """Test user changing password needing current password.""" + auth = NativeAuthenticator(db=app.db) + assert auth.create_user("johnsnow", "westeros", "westeros") + + # Wrong previous password. + assert ( + auth.user_change_password("johnsnow", "dragon", "Daenerys", "Daenerys") is None + ) + assert auth.get_user("johnsnow").is_valid_password("westeros") + # Empty previous password. + assert auth.user_change_password("johnsnow", "", "Daenerys", "Daenerys") is None + assert auth.get_user("johnsnow").is_valid_password("westeros") + # Correct previous password. + assert ( + auth.user_change_password("johnsnow", "westeros", "Daenerys", "Daenerys") + is not None + ) + assert auth.get_user("johnsnow").is_valid_password("Daenerys") @pytest.mark.parametrize( @@ -186,7 +231,7 @@ async def test_create_user_disable(enable_signup, expected_success, tmpcwd, app) auth = NativeAuthenticator(db=app.db) auth.enable_signup = enable_signup - user = auth.create_user("johnsnow", "password") + user = auth.create_user("johnsnow", "password", "password") if expected_success: assert user.username == "johnsnow" @@ -207,7 +252,7 @@ async def test_create_user_disable(enable_signup, expected_success, tmpcwd, app) async def test_authentication(username, password, authorized, expected, tmpcwd, app): """Test if authentication fails with a unexistent user""" auth = NativeAuthenticator(db=app.db) - auth.create_user("johnsnow", "password") + auth.create_user("johnsnow", "password", "password") if authorized: UserInfo.change_authorization(app.db, "johnsnow") response = await auth.authenticate( @@ -244,7 +289,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.create_user(infos["username"], infos["password"]) + auth.create_user(infos["username"], infos["password"], infos["password"]) UserInfo.change_authorization(app.db, "johnsnow") assert not auth.login_attempts @@ -265,7 +310,7 @@ async def test_authentication_with_exceed_atempts_of_login(tmpcwd, app): auth.secs_before_next_try = 10 infos = {"username": "johnsnow", "password": "wrongpassword"} - auth.create_user(infos["username"], "password") + auth.create_user(infos["username"], "password", "password") UserInfo.change_authorization(app.db, "johnsnow") for i in range(3): @@ -283,7 +328,7 @@ async def test_authentication_with_exceed_atempts_of_login(tmpcwd, app): async def test_get_user(tmpcwd, app): auth = NativeAuthenticator(db=app.db) - auth.create_user("johnsnow", "password") + auth.create_user("johnsnow", "password", "password") # Getting existing user is successful. assert auth.get_user("johnsnow") is not None @@ -294,7 +339,7 @@ async def test_get_user(tmpcwd, app): async def test_delete_user(tmpcwd, app): auth = NativeAuthenticator(db=app.db) - auth.create_user("johnsnow", "password") + auth.create_user("johnsnow", "password", "password") user = type("User", (), {"name": "johnsnow"}) auth.delete_user(user) From fa460ad96f14275d352bc4f1dc065964aa6722b6 Mon Sep 17 00:00:00 2001 From: Jonas Betzendahl Date: Sun, 31 Oct 2021 23:05:03 +0100 Subject: [PATCH 2/5] fix visibility toggle, add error message on incorrect current password, improve page layout --- nativeauthenticator/handlers.py | 8 ++++++++ .../templates/change-password-admin.html | 10 ++++++---- nativeauthenticator/templates/change-password.html | 13 ++++++++----- nativeauthenticator/templates/signup.html | 2 +- 4 files changed, 23 insertions(+), 10 deletions(-) diff --git a/nativeauthenticator/handlers.py b/nativeauthenticator/handlers.py index 09586e8..7bae921 100644 --- a/nativeauthenticator/handlers.py +++ b/nativeauthenticator/handlers.py @@ -276,6 +276,11 @@ async def post(self): old_password = self.get_body_argument("old_password", strip=False) new_password = self.get_body_argument("new_password", strip=False) confirmation = self.get_body_argument("con_password", strip=False) + + correctpw = self.authenticator.get_user(user.name).is_valid_password( + old_password + ) + success = self.authenticator.user_change_password( user.name, old_password, new_password, confirmation ) @@ -283,6 +288,9 @@ async def post(self): if success: alert = "alert-success" msg = "Your password has been changed successfully!" + elif not correctpw: + alert = "alert-danger" + msg = "Your current password was incorrect. Please try again." else: alert = "alert-danger" pw_len = self.authenticator.minimum_password_length diff --git a/nativeauthenticator/templates/change-password-admin.html b/nativeauthenticator/templates/change-password-admin.html index d88da27..2f79771 100644 --- a/nativeauthenticator/templates/change-password-admin.html +++ b/nativeauthenticator/templates/change-password-admin.html @@ -8,7 +8,7 @@ button.addEventListener("click", function(e) { let npwd = document.getElementById("password_input"); let cpwd = document.getElementById("password_confirm"); - if (pwd.getAttribute("type") === "password") { + if (npwd.getAttribute("type") === "password") { npwd.setAttribute("type", "text"); cpwd.setAttribute("type", "text"); button.textContent = "🔑"; @@ -28,10 +28,12 @@

Change password for {{user_name}}

+ +

Please enter the new password you want to set for {{user_name}}.

-
+
@@ -40,7 +42,7 @@

-
+

@@ -50,7 +52,7 @@

{% if result_message %} - + {% endif %}

{% endblock %} diff --git a/nativeauthenticator/templates/change-password.html b/nativeauthenticator/templates/change-password.html index 8425033..3fce03e 100644 --- a/nativeauthenticator/templates/change-password.html +++ b/nativeauthenticator/templates/change-password.html @@ -9,7 +9,7 @@ let opwd = document.getElementById("password_prove"); let npwd = document.getElementById("password_input"); let cpwd = document.getElementById("password_confirm"); - if (pwd.getAttribute("type") === "password") { + if (opwd.getAttribute("type") === "password") { opwd.setAttribute("type", "text"); npwd.setAttribute("type", "text"); cpwd.setAttribute("type", "text"); @@ -31,10 +31,12 @@

Change password for {{user_name}}

+ +

Please enter your current password and the new password you want to set it to. If you have forgotten your password, an admin can reset it for you.

-
+
@@ -43,14 +45,15 @@

-
+

-
+
+

@@ -59,7 +62,7 @@

{% if result_message %} - + {% endif %}

{% endblock %} diff --git a/nativeauthenticator/templates/signup.html b/nativeauthenticator/templates/signup.html index de288f9..06a76a8 100644 --- a/nativeauthenticator/templates/signup.html +++ b/nativeauthenticator/templates/signup.html @@ -90,7 +90,7 @@

- +
From 9c1a965bb24aa509e41a8574ebef397216e255de Mon Sep 17 00:00:00 2001 From: Jonas Betzendahl Date: Mon, 1 Nov 2021 11:58:24 +0100 Subject: [PATCH 3/5] Changes and refactorings requested --- nativeauthenticator/handlers.py | 112 ++++++++++-------- nativeauthenticator/nativeauthenticator.py | 17 +-- .../templates/change-password-admin.html | 12 +- .../templates/change-password.html | 18 +-- nativeauthenticator/templates/signup.html | 7 +- .../tests/test_authenticator.py | 97 ++++----------- 6 files changed, 112 insertions(+), 151 deletions(-) diff --git a/nativeauthenticator/handlers.py b/nativeauthenticator/handlers.py index 7bae921..3ed2461 100644 --- a/nativeauthenticator/handlers.py +++ b/nativeauthenticator/handlers.py @@ -58,7 +58,7 @@ async def get(self): ) self.finish(html) - def get_result_message(self, user, taken, match, human=True): + def get_result_message(self, user, taken, confirmation_matches, human=True): alert = "alert-info" message = "Your information has been sent to the admin" if user and user.login_email_sent: @@ -72,9 +72,9 @@ def get_result_message(self, user, taken, match, human=True): "username is already in use. Please try again " "with a different username." ) - elif not match: + elif not confirmation_matches: alert = "alert-danger" - message = "Your passwords did not match. Please try again." + message = "Your password did not match the confirmation. Please try again." else: # Error if user creation was not successful. if not user: @@ -136,8 +136,7 @@ async def post(self): if assume_human: user_info = { "username": self.get_body_argument("username", strip=False), - "pw": self.get_body_argument("pw", strip=False), - "pwc": self.get_body_argument("pwc", strip=False), + "signup_password": self.get_body_argument("pw", strip=False), "email": self.get_body_argument("email", "", strip=False), "has_2fa": bool(self.get_body_argument("2fa", "", strip=False)), } @@ -147,10 +146,15 @@ async def post(self): user = False taken = False - match = self.get_body_argument("pw", strip=False) == self.get_body_argument( - "pwc", strip=False + password = self.get_body_argument("signup_password", strip=False) + confirmation = self.get_body_argument( + "signup_password_confirmation", strip=False + ) + confirmation_matches = password == confirmation + + alert, message = self.get_result_message( + user, taken, confirmation_matches, assume_human ) - alert, message = self.get_result_message(user, taken, match, assume_human) otp_secret, user_2fa = "", "" if user: @@ -194,7 +198,7 @@ async def get(self, slug): class AuthorizeHandler(LocalBase): async def get(self, slug): must_stop = True - msg = "Invalid URL" + message = "Invalid URL" if self.authenticator.allow_self_approval_for: try: data = AuthorizeHandler.validate_slug( @@ -206,17 +210,17 @@ async def get(self, slug): if not must_stop: username = data["username"] - msg = f"{username} was already authorized" + message = f"{username} was already authorized" usr = UserInfo.find(self.db, username) if not usr.is_authorized: UserInfo.change_authorization(self.db, username) - msg = f"{username} has been authorized" + message = f"{username} has been authorized" # add POSIX user!! html = await self.render_template( "my_message.html", - message=msg, + message=message, ) self.finish(html) @@ -275,33 +279,41 @@ async def post(self): user = await self.get_current_user() old_password = self.get_body_argument("old_password", strip=False) new_password = self.get_body_argument("new_password", strip=False) - confirmation = self.get_body_argument("con_password", strip=False) + confirmation = self.get_body_argument("new_password_confirmation", strip=False) - correctpw = self.authenticator.get_user(user.name).is_valid_password( - old_password - ) + correct_password_provided = self.authenticator.get_user( + user.name + ).is_valid_password(old_password) - success = self.authenticator.user_change_password( - user.name, old_password, new_password, confirmation - ) + new_password_matches_confirmation = new_password == confirmation - if success: - alert = "alert-success" - msg = "Your password has been changed successfully!" - elif not correctpw: + if not correct_password_provided: alert = "alert-danger" - msg = "Your current password was incorrect. Please try again." - else: + message = "Your current password was incorrect. Please try again." + elif not new_password_matches_confirmation: alert = "alert-danger" - pw_len = self.authenticator.minimum_password_length - msg = ( - "Something went wrong! Be sure your new " - f"password has at least {pw_len} characters and is " - "not too common." + message = ( + "Your new password didn't match the confirmation. Please try again." ) + else: + success = self.authenticator.change_password(user.name, new_password) + if success: + alert = "alert-success" + message = "Your password has been changed successfully!" + else: + alert = "alert-danger" + pw_len = self.authenticator.minimum_password_length + message = ( + "Something went wrong! Be sure your new " + f"password has at least {pw_len} characters and is " + "not too common." + ) html = await self.render_template( - "change-password.html", user_name=user.name, result_message=msg, alert=alert + "change-password.html", + user_name=user.name, + result_message=message, + alert=alert, ) self.finish(html) @@ -322,31 +334,33 @@ async def get(self, user_name): @admin_users_scope async def post(self, user_name): new_password = self.get_body_argument("new_password", strip=False) - confirmation = self.get_body_argument("con_password", strip=False) - success = self.authenticator.change_password( - user_name, new_password, confirmation - ) + confirmation = self.get_body_argument("new_password_confirmation", strip=False) - if success: - alert = "alert-success" - msg = f"The password for {user_name} has been changed successfully" - elif not new_password == confirmation: - alert = "alert-danger" - pw_len = self.authenticator.minimum_password_length - msg = "The passwords didn't match. Please try again." - else: + new_password_matches_confirmation = new_password == confirmation + + if not new_password_matches_confirmation: alert = "alert-danger" - pw_len = self.authenticator.minimum_password_length - msg = ( - "Something went wrong! Be sure the new password " - f"for {user_name} has at least {pw_len} characters and is " - "not too common." + message = ( + "The new password didn't match the confirmation. Please try again." ) + else: + success = self.authenticator.change_password(user_name, new_password) + if success: + alert = "alert-success" + message = f"The password for {user_name} has been changed successfully" + else: + alert = "alert-danger" + pw_len = self.authenticator.minimum_password_length + message = ( + "Something went wrong! Be sure the new password " + f"for {user_name} has at least {pw_len} characters and is " + "not too common." + ) html = await self.render_template( "change-password-admin.html", user_name=user_name, - result_message=msg, + result_message=message, alert=alert, ) self.finish(html) diff --git a/nativeauthenticator/nativeauthenticator.py b/nativeauthenticator/nativeauthenticator.py index cb0058e..9d28352 100644 --- a/nativeauthenticator/nativeauthenticator.py +++ b/nativeauthenticator/nativeauthenticator.py @@ -295,13 +295,13 @@ def get_authed_users(self): def user_exists(self, username): return self.get_user(username) is not None - def create_user(self, username, pw, pwc, **kwargs): + def create_user(self, username, pw, **kwargs): username = self.normalize_username(username) if self.user_exists(username) or not self.validate_username(username): return - if not self.is_password_strong(pw) or not (pw == pwc): + if not self.is_password_strong(pw): return if not self.enable_signup: @@ -358,7 +358,7 @@ def send_approval_email(self, dest, url): raise web.HTTPError( 503, reason="Self-authorization email could not " - + "be sent. Please contact the jupyterhub " + + "be sent. Please contact the JupyterHub " + "admin about this.", ) @@ -372,13 +372,12 @@ def get_unauthed_amount(self): return unauthed - def change_password(self, username, new_password, confirmation): + def change_password(self, username, new_password): user = self.get_user(username) criteria = [ user is not None, self.is_password_strong(new_password), - new_password == confirmation, ] if not all(criteria): return @@ -387,14 +386,6 @@ def change_password(self, username, new_password, confirmation): self.db.commit() return True - def user_change_password(self, username, old_password, new_password, confirmation): - user = self.get_user(username) - - if (user is None) or (not user.is_valid_password(old_password)): - return - - return self.change_password(username, new_password, confirmation) - def validate_username(self, username): invalid_chars = [",", " "] if any((char in username) for char in invalid_chars): diff --git a/nativeauthenticator/templates/change-password-admin.html b/nativeauthenticator/templates/change-password-admin.html index 2f79771..977f4ba 100644 --- a/nativeauthenticator/templates/change-password-admin.html +++ b/nativeauthenticator/templates/change-password-admin.html @@ -6,8 +6,8 @@ document.addEventListener('DOMContentLoaded', function() { let button = document.getElementById('eye'); button.addEventListener("click", function(e) { - let npwd = document.getElementById("password_input"); - let cpwd = document.getElementById("password_confirm"); + let npwd = document.getElementById("new_password_input"); + let cpwd = document.getElementById("new_password_confirmation_input"); if (npwd.getAttribute("type") === "password") { npwd.setAttribute("type", "text"); cpwd.setAttribute("type", "text"); @@ -32,18 +32,18 @@

Please enter the new password you want to set for {{user_name}}.

- +
- +

- +
- +

diff --git a/nativeauthenticator/templates/change-password.html b/nativeauthenticator/templates/change-password.html index 3fce03e..dcd02e1 100644 --- a/nativeauthenticator/templates/change-password.html +++ b/nativeauthenticator/templates/change-password.html @@ -6,9 +6,9 @@ document.addEventListener('DOMContentLoaded', function() { let button = document.getElementById('eye'); button.addEventListener("click", function(e) { - let opwd = document.getElementById("password_prove"); - let npwd = document.getElementById("password_input"); - let cpwd = document.getElementById("password_confirm"); + let opwd = document.getElementById("old_password_input"); + let npwd = document.getElementById("new_password_input"); + let cpwd = document.getElementById("new_password_confirmation_input"); if (opwd.getAttribute("type") === "password") { opwd.setAttribute("type", "text"); npwd.setAttribute("type", "text"); @@ -35,24 +35,24 @@

Please enter your current password and the new password you want to set it to. If you have forgotten your password, an admin can reset it for you.

- +
- +

- +
- +

- +
- +

diff --git a/nativeauthenticator/templates/signup.html b/nativeauthenticator/templates/signup.html index 06a76a8..2db3b7a 100644 --- a/nativeauthenticator/templates/signup.html +++ b/nativeauthenticator/templates/signup.html @@ -7,7 +7,8 @@ let button = document.getElementById('eye'); button.addEventListener("click", function(e) { let pwd = document.getElementById("password_input"); - let pwdc = document.getElementById("password_confirm"); + let pwdc = document.getElementById("password_confirmation_input"); + if (pwd.getAttribute("type") === "password") { pwd.setAttribute("type", "text"); pwdc.setAttribute("type", "text"); @@ -83,7 +84,7 @@
- + @@ -92,7 +93,7 @@
- +

diff --git a/nativeauthenticator/tests/test_authenticator.py b/nativeauthenticator/tests/test_authenticator.py index 3fc2ea3..710018c 100644 --- a/nativeauthenticator/tests/test_authenticator.py +++ b/nativeauthenticator/tests/test_authenticator.py @@ -48,7 +48,7 @@ async def test_create_user(is_admin, open_signup, expected_authorization, tmpcwd if open_signup: auth.open_signup = True - auth.create_user("johnsnow", "password", "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 @@ -57,8 +57,8 @@ async def test_create_user(is_admin, open_signup, expected_authorization, tmpcwd 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.create_user("john snow", "password", "password") - assert not auth.create_user("john,snow", "password", "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): @@ -66,13 +66,13 @@ async def test_create_user_twice(tmpcwd, app): auth = NativeAuthenticator(db=app.db) # First creation should succeed. - assert auth.create_user("johnsnow", "password", "password") + assert auth.create_user("johnsnow", "password") # Creating the same account again should fail. - assert not auth.create_user("johnsnow", "password", "password") + 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", "adifferentpassword") + assert not auth.create_user("johnsnow", "adifferentpassword") async def test_get_authed_users(tmpcwd, app): @@ -82,13 +82,13 @@ async def test_get_authed_users(tmpcwd, app): auth.admin_users = set() assert auth.get_authed_users() == set() - auth.create_user("johnsnow", "password", "password") + auth.create_user("johnsnow", "password") assert auth.get_authed_users() == set() UserInfo.change_authorization(app.db, "johnsnow") assert auth.get_authed_users() == set({"johnsnow"}) - auth.create_user("daenerystargaryen", "anotherpassword", "anotherpassword") + auth.create_user("daenerystargaryen", "anotherpassword") assert auth.get_authed_users() == set({"johnsnow"}) auth.admin_users = set({"daenerystargaryen"}) @@ -103,16 +103,16 @@ async def test_get_unauthed_amount(tmpcwd, app): auth.admin_users = set() assert auth.get_unauthed_amount() == 0 - auth.create_user("johnsnow", "password", "password") + auth.create_user("johnsnow", "password") assert auth.get_unauthed_amount() == 1 UserInfo.change_authorization(app.db, "johnsnow") assert auth.get_unauthed_amount() == 0 - auth.create_user("daenerystargaryen", "anotherpassword", "anotherpassword") + auth.create_user("daenerystargaryen", "anotherpassword") assert auth.get_unauthed_amount() == 1 - auth.create_user("tyrionlannister", "yetanotherpassword", "yetanotherpassword") + auth.create_user("tyrionlannister", "yetanotherpassword") assert auth.get_unauthed_amount() == 2 auth.admin_users = set({"daenerystargaryen"}) @@ -135,15 +135,15 @@ async def test_create_user_with_strong_passwords( auth = NativeAuthenticator(db=app.db) auth.check_common_password = True auth.minimum_password_length = min_len - user = auth.create_user("johnsnow", password, password) + user = auth.create_user("johnsnow", password) assert bool(user) == expected async def test_change_password(tmpcwd, app): auth = NativeAuthenticator(db=app.db) - user = auth.create_user("johnsnow", "password", "password") + user = auth.create_user("johnsnow", "password") assert user.is_valid_password("password") - auth.change_password("johnsnow", "newpassword", "newpassword") + auth.change_password("johnsnow", "newpassword") assert not user.is_valid_password("password") assert user.is_valid_password("newpassword") @@ -157,68 +157,23 @@ async def test_no_change_to_bad_password(tmpcwd, app): auth.create_user("johnsnow", "ironwood", "ironwood") # Can't change password of nonexistent users. - assert auth.change_password("samwelltarly", "palanquin", "palanquin") is None + assert auth.change_password("samwelltarly", "palanquin") is None assert auth.get_user("johnsnow").is_valid_password("ironwood") # Can't change password to something too short. - assert auth.change_password("johnsnow", "mummer", "mummer") is None + assert auth.change_password("johnsnow", "mummer") is None assert auth.get_user("johnsnow").is_valid_password("ironwood") # Can't change password to something too common. - assert auth.change_password("johnsnow", "dragon", "dragon") is None + assert auth.change_password("johnsnow", "dragon") is None assert auth.get_user("johnsnow").is_valid_password("ironwood") # CAN change password to something fulfilling criteria. - assert auth.change_password("johnsnow", "Daenerys", "Daenerys") is not None + assert auth.change_password("johnsnow", "Daenerys") is not None assert not auth.get_user("johnsnow").is_valid_password("ironwood") assert auth.get_user("johnsnow").is_valid_password("Daenerys") -async def test_password_confirmation(tmpcwd, app): - """Test password confirmation mechanisms.""" - auth = NativeAuthenticator(db=app.db) - - # Can create with matching confirmation entry. - assert auth.create_user("johnsnow", "ironwood", "ironwood") - # Cannot create with empty confirmation entry. - assert not auth.create_user("johnsnow", "ironwood", "") - # Cannot create with incorrect confirmation entry. - assert not auth.create_user("johnsnow", "ironwood", "throne") - - assert auth.get_user("johnsnow").is_valid_password("ironwood") - - # Can change with matching confirmation entry. - assert auth.change_password("johnsnow", "westeros", "westeros") is not None - assert auth.get_user("johnsnow").is_valid_password("westeros") - # Cannot change with empty confirmation entry. - assert auth.change_password("johnsnow", "Daenerys", "") is None - assert auth.get_user("johnsnow").is_valid_password("westeros") - # Cannot change with incorrect confirmation entry. - assert auth.change_password("johnsnow", "Daenerys", "throne") is None - assert auth.get_user("johnsnow").is_valid_password("westeros") - - -async def test_user_change_password(tmpcwd, app): - """Test user changing password needing current password.""" - auth = NativeAuthenticator(db=app.db) - assert auth.create_user("johnsnow", "westeros", "westeros") - - # Wrong previous password. - assert ( - auth.user_change_password("johnsnow", "dragon", "Daenerys", "Daenerys") is None - ) - assert auth.get_user("johnsnow").is_valid_password("westeros") - # Empty previous password. - assert auth.user_change_password("johnsnow", "", "Daenerys", "Daenerys") is None - assert auth.get_user("johnsnow").is_valid_password("westeros") - # Correct previous password. - assert ( - auth.user_change_password("johnsnow", "westeros", "Daenerys", "Daenerys") - is not None - ) - assert auth.get_user("johnsnow").is_valid_password("Daenerys") - - @pytest.mark.parametrize( "enable_signup,expected_success", [ @@ -231,7 +186,7 @@ async def test_create_user_disable(enable_signup, expected_success, tmpcwd, app) auth = NativeAuthenticator(db=app.db) auth.enable_signup = enable_signup - user = auth.create_user("johnsnow", "password", "password") + user = auth.create_user("johnsnow", "password") if expected_success: assert user.username == "johnsnow" @@ -252,7 +207,7 @@ async def test_create_user_disable(enable_signup, expected_success, tmpcwd, app) async def test_authentication(username, password, authorized, expected, tmpcwd, app): """Test if authentication fails with a unexistent user""" auth = NativeAuthenticator(db=app.db) - auth.create_user("johnsnow", "password", "password") + auth.create_user("johnsnow", "password") if authorized: UserInfo.change_authorization(app.db, "johnsnow") response = await auth.authenticate( @@ -289,7 +244,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.create_user(infos["username"], infos["password"], infos["password"]) + auth.create_user(infos["username"], infos["password"]) UserInfo.change_authorization(app.db, "johnsnow") assert not auth.login_attempts @@ -310,7 +265,7 @@ async def test_authentication_with_exceed_atempts_of_login(tmpcwd, app): auth.secs_before_next_try = 10 infos = {"username": "johnsnow", "password": "wrongpassword"} - auth.create_user(infos["username"], "password", "password") + auth.create_user(infos["username"], "password") UserInfo.change_authorization(app.db, "johnsnow") for i in range(3): @@ -328,7 +283,7 @@ async def test_authentication_with_exceed_atempts_of_login(tmpcwd, app): async def test_get_user(tmpcwd, app): auth = NativeAuthenticator(db=app.db) - auth.create_user("johnsnow", "password", "password") + auth.create_user("johnsnow", "password") # Getting existing user is successful. assert auth.get_user("johnsnow") is not None @@ -339,7 +294,7 @@ async def test_get_user(tmpcwd, app): async def test_delete_user(tmpcwd, app): auth = NativeAuthenticator(db=app.db) - auth.create_user("johnsnow", "password", "password") + auth.create_user("johnsnow", "password") user = type("User", (), {"name": "johnsnow"}) auth.delete_user(user) @@ -393,7 +348,7 @@ async def test_import_from_firstuse_invalid_password(user, pwd, tmpcwd, app): async def test_secret_key(app): auth = NativeAuthenticator(db=app.db) auth.ask_email_on_signup = False - auth.allow_self_approval_for = ".*@some-domain.com$" + auth.allow_self_approval_for = ".*@example.com$" auth.secret_key = "short" with pytest.raises(ValueError): @@ -407,7 +362,7 @@ async def test_secret_key(app): async def test_approval_url(app): auth = NativeAuthenticator(db=app.db) - auth.allow_self_approval_for = ".*@some-domain.com$" + auth.allow_self_approval_for = ".*@example.com$" auth.secret_key = "very long and kind-of random asdgaisgfjbafksdgasg" auth.setup_self_approval() From 882ab0af80f1eb369a6457534030f55899e6fdb7 Mon Sep 17 00:00:00 2001 From: Jonas Betzendahl Date: Mon, 1 Nov 2021 12:05:10 +0100 Subject: [PATCH 4/5] fix remnant errors in tests and code --- nativeauthenticator/nativeauthenticator.py | 2 +- nativeauthenticator/tests/test_authenticator.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/nativeauthenticator/nativeauthenticator.py b/nativeauthenticator/nativeauthenticator.py index 9d28352..5c30e01 100644 --- a/nativeauthenticator/nativeauthenticator.py +++ b/nativeauthenticator/nativeauthenticator.py @@ -429,7 +429,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.create_user(user.decode(), password, password) + new_user = self.create_user(user.decode(), password) if not new_user: error = ( f"User {user} was not created. Check password " diff --git a/nativeauthenticator/tests/test_authenticator.py b/nativeauthenticator/tests/test_authenticator.py index 710018c..337057f 100644 --- a/nativeauthenticator/tests/test_authenticator.py +++ b/nativeauthenticator/tests/test_authenticator.py @@ -154,7 +154,7 @@ async def test_no_change_to_bad_password(tmpcwd, app): auth.check_common_password = True auth.minimum_password_length = 8 - auth.create_user("johnsnow", "ironwood", "ironwood") + auth.create_user("johnsnow", "ironwood") # Can't change password of nonexistent users. assert auth.change_password("samwelltarly", "palanquin") is None From 7b56ac61210ad7570cfe5dfc4dc54d401b2de2e3 Mon Sep 17 00:00:00 2001 From: Jonas Betzendahl Date: Tue, 2 Nov 2021 11:12:04 +0100 Subject: [PATCH 5/5] fix name mixup, small refactor --- nativeauthenticator/handlers.py | 2 +- nativeauthenticator/nativeauthenticator.py | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/nativeauthenticator/handlers.py b/nativeauthenticator/handlers.py index 3ed2461..b799ef5 100644 --- a/nativeauthenticator/handlers.py +++ b/nativeauthenticator/handlers.py @@ -136,7 +136,7 @@ async def post(self): if assume_human: user_info = { "username": self.get_body_argument("username", strip=False), - "signup_password": self.get_body_argument("pw", strip=False), + "password": self.get_body_argument("signup_password", strip=False), "email": self.get_body_argument("email", "", strip=False), "has_2fa": bool(self.get_body_argument("2fa", "", strip=False)), } diff --git a/nativeauthenticator/nativeauthenticator.py b/nativeauthenticator/nativeauthenticator.py index 5c30e01..93ad901 100644 --- a/nativeauthenticator/nativeauthenticator.py +++ b/nativeauthenticator/nativeauthenticator.py @@ -295,20 +295,20 @@ def get_authed_users(self): def user_exists(self, username): return self.get_user(username) is not None - def create_user(self, username, pw, **kwargs): + def create_user(self, username, password, **kwargs): username = self.normalize_username(username) if self.user_exists(username) or not self.validate_username(username): return - if not self.is_password_strong(pw): + if not self.is_password_strong(password): return if not self.enable_signup: return - encoded_pw = bcrypt.hashpw(pw.encode(), bcrypt.gensalt()) - infos = {"username": username, "password": encoded_pw} + encoded_password = bcrypt.hashpw(password.encode(), bcrypt.gensalt()) + infos = {"username": username, "password": encoded_password} infos.update(kwargs) if self.open_signup or username in self.get_authed_users():