From bbdb7f8ce5a85aa58543185306c80eca7c5166cc Mon Sep 17 00:00:00 2001 From: Tessa Walsh Date: Tue, 3 Oct 2023 21:57:46 -0400 Subject: [PATCH] Require that all passwords are between 8 and 64 characters (#1239) - Require that all passwords are between 8 and 64 characters - Fixes account settings password reset form to only trigger logged-in event after successful password change. - Password validation can be extended within the UserManager's validate_password method to add or modify requirements. - Add tests for password validation --- backend/btrixcloud/users.py | 39 +++++- backend/test/test_org.py | 2 +- backend/test/test_users.py | 142 +++++++++++++++++++- frontend/src/components/account-settings.ts | 11 +- frontend/src/components/sign-up-form.ts | 11 +- frontend/src/pages/reset-password.ts | 14 +- 6 files changed, 200 insertions(+), 19 deletions(-) diff --git a/backend/btrixcloud/users.py b/backend/btrixcloud/users.py index 661aeaf73f..047f29ee0f 100644 --- a/backend/btrixcloud/users.py +++ b/backend/btrixcloud/users.py @@ -6,7 +6,7 @@ import uuid import asyncio -from typing import Optional +from typing import Optional, Union from pydantic import UUID4 import passlib.pwd @@ -17,7 +17,7 @@ from pymongo.errors import DuplicateKeyError from fastapi_users import FastAPIUsers, BaseUserManager -from fastapi_users.manager import UserAlreadyExists +from fastapi_users.manager import UserAlreadyExists, InvalidPasswordException from fastapi_users.authentication import ( AuthenticationBackend, BearerTransport, @@ -96,6 +96,23 @@ async def create( await self.on_after_register_custom(created_user, user, request) return created_user + async def validate_password( + self, password: str, user: Union[UserCreate, UserDB] + ) -> None: + """ + Validate a password. + + Overloaded to set password requirements. + + :param password: The password to validate. + :param user: The user associated to this password. + :raises InvalidPasswordException: The password is invalid. + :return: None if the password is valid. + """ + pw_length = len(password) + if not 8 <= pw_length <= 64: + raise InvalidPasswordException(reason="invalid_password_length") + async def get_user_names_by_ids(self, user_ids): """return list of user names for given ids""" user_ids = [UUID4(id_) for id_ in user_ids] @@ -147,9 +164,11 @@ async def create_super_user(self): ) print(f"Super user {email} created", flush=True) print(res, flush=True) - except (DuplicateKeyError, UserAlreadyExists): print(f"User {email} already exists", flush=True) + # pylint: disable=raise-missing-from + except InvalidPasswordException: + raise HTTPException(status_code=422, detail="invalid_password") async def create_non_super_user( self, @@ -174,9 +193,17 @@ async def create_non_super_user( newOrg=False, is_verified=True, ) - created_user = await super().create(user_create, safe=False, request=None) - await self.on_after_register_custom(created_user, user_create, request=None) - return created_user + try: + created_user = await super().create( + user_create, safe=False, request=None + ) + await self.on_after_register_custom( + created_user, user_create, request=None + ) + return created_user + # pylint: disable=raise-missing-from + except InvalidPasswordException: + raise HTTPException(status_code=422, detail="invalid_password") except (DuplicateKeyError, UserAlreadyExists): print(f"User {email} already exists", flush=True) diff --git a/backend/test/test_org.py b/backend/test/test_org.py index 68f5c3e2f4..09a3fb1bcf 100644 --- a/backend/test/test_org.py +++ b/backend/test/test_org.py @@ -226,7 +226,7 @@ def test_send_and_accept_org_invite( json={ "name": "accepted", "email": expected_stored_email, - "password": "testpw", + "password": "testingpassword", "inviteToken": token, "newOrg": False, }, diff --git a/backend/test/test_users.py b/backend/test/test_users.py index 2e7e11f299..836cd46e2c 100644 --- a/backend/test/test_users.py +++ b/backend/test/test_users.py @@ -1,6 +1,10 @@ import requests +import time -from .conftest import API_PREFIX, CRAWLER_USERNAME +from .conftest import API_PREFIX, CRAWLER_USERNAME, ADMIN_PW, ADMIN_USERNAME + +VALID_USER_EMAIL = "validpassword@example.com" +VALID_USER_PW = "validpassw0rd!" def test_create_super_user(admin_auth_headers): @@ -42,3 +46,139 @@ def test_me_with_orgs(crawler_auth_headers, default_org_id): assert default_org["name"] assert default_org["default"] assert default_org["role"] == 20 + + +def test_add_user_to_org_invalid_password(admin_auth_headers, default_org_id): + r = requests.post( + f"{API_PREFIX}/orgs/{default_org_id}/add-user", + json={ + "email": "invalidpassword@example.com", + "password": "pw", + "name": "invalid pw user", + "description": "test invalid password", + "role": 20, + }, + headers=admin_auth_headers, + ) + assert r.status_code == 422 + assert r.json()["detail"] == "invalid_password" + + +def test_register_user_invalid_password(admin_auth_headers, default_org_id): + email = "invalidpassword@example.com" + # Send invite + r = requests.post( + f"{API_PREFIX}/orgs/{default_org_id}/invite", + headers=admin_auth_headers, + json={"email": email, "role": 20}, + ) + assert r.status_code == 200 + data = r.json() + assert data["invited"] == "new_user" + + # Look up token + r = requests.get( + f"{API_PREFIX}/orgs/{default_org_id}/invites", + headers=admin_auth_headers, + ) + assert r.status_code == 200 + data = r.json() + invites_matching_email = [ + invite for invite in data["items"] if invite["email"] == email + ] + token = invites_matching_email[0]["id"] + + # Create user with invite + r = requests.post( + f"{API_PREFIX}/auth/register", + headers=admin_auth_headers, + json={ + "name": "invalid", + "email": email, + "password": "passwd", + "inviteToken": token, + "newOrg": False, + }, + ) + assert r.status_code == 400 + detail = r.json()["detail"] + assert detail["code"] == "REGISTER_INVALID_PASSWORD" + assert detail["reason"] == "invalid_password_length" + + +def test_register_user_valid_password(admin_auth_headers, default_org_id): + # Send invite + r = requests.post( + f"{API_PREFIX}/orgs/{default_org_id}/invite", + headers=admin_auth_headers, + json={"email": VALID_USER_EMAIL, "role": 20}, + ) + assert r.status_code == 200 + data = r.json() + assert data["invited"] == "new_user" + + # Look up token + r = requests.get( + f"{API_PREFIX}/orgs/{default_org_id}/invites", + headers=admin_auth_headers, + ) + assert r.status_code == 200 + data = r.json() + invites_matching_email = [ + invite for invite in data["items"] if invite["email"] == VALID_USER_EMAIL + ] + token = invites_matching_email[0]["id"] + + # Create user with invite + r = requests.post( + f"{API_PREFIX}/auth/register", + headers=admin_auth_headers, + json={ + "name": "valid", + "email": VALID_USER_EMAIL, + "password": VALID_USER_PW, + "inviteToken": token, + "newOrg": False, + }, + ) + assert r.status_code == 201 + + +def test_reset_invalid_password(admin_auth_headers): + r = requests.patch( + f"{API_PREFIX}/users/me", + headers=admin_auth_headers, + json={"email": ADMIN_USERNAME, "password": "12345"}, + ) + assert r.status_code == 400 + detail = r.json()["detail"] + assert detail["code"] == "UPDATE_USER_INVALID_PASSWORD" + assert detail["reason"] == "invalid_password_length" + + +def test_reset_valid_password(admin_auth_headers, default_org_id): + valid_user_headers = {} + while True: + r = requests.post( + f"{API_PREFIX}/auth/jwt/login", + data={ + "username": VALID_USER_EMAIL, + "password": VALID_USER_PW, + "grant_type": "password", + }, + ) + data = r.json() + try: + valid_user_headers = {"Authorization": f"Bearer {data['access_token']}"} + break + except: + print("Waiting for valid user auth headers") + time.sleep(5) + + r = requests.patch( + f"{API_PREFIX}/users/me", + headers=valid_user_headers, + json={"email": VALID_USER_EMAIL, "password": "new!password"}, + ) + assert r.status_code == 200 + assert r.json()["email"] == VALID_USER_EMAIL diff --git a/frontend/src/components/account-settings.ts b/frontend/src/components/account-settings.ts index 35316258ad..62d8b8dc0a 100644 --- a/frontend/src/components/account-settings.ts +++ b/frontend/src/components/account-settings.ts @@ -311,6 +311,8 @@ export class AccountSettings extends LiteElement { name="newPassword" type="password" label="${msg("New password")}" + help-text=${msg("Must be between 8-64 characters")} + minlength="8" autocomplete="new-password" password-toggle required @@ -351,8 +353,6 @@ export class AccountSettings extends LiteElement { email: this.authState.username, password: formData.get("password") as string, }); - - this.dispatchEvent(AuthService.createLoggedInEvent(nextAuthState)); } catch (e: any) { console.debug(e); } @@ -371,6 +371,7 @@ export class AccountSettings extends LiteElement { const params = { password: formData.get("newPassword"), + email: this.userInfo?.email, }; try { @@ -385,13 +386,17 @@ export class AccountSettings extends LiteElement { successMessage: "Successfully updated password", }, }); + + this.dispatchEvent(AuthService.createLoggedInEvent(nextAuthState)); } catch (e) { console.error(e); this.formStateService.send({ type: "ERROR", detail: { - serverError: msg("Something went wrong changing password"), + serverError: msg( + "Something went wrong changing password. Verify that new password meets requirements (8-64 characters)." + ), }, }); } diff --git a/frontend/src/components/sign-up-form.ts b/frontend/src/components/sign-up-form.ts index 4a4c795502..815dcc48b3 100644 --- a/frontend/src/components/sign-up-form.ts +++ b/frontend/src/components/sign-up-form.ts @@ -79,7 +79,9 @@ export class SignUpForm extends LiteElement { id="password" name="password" type="password" - label=${msg("Create a password")} + label="${msg("New password")}" + help-text=${msg("Must be between 8-64 characters")} + minlength="8" autocomplete="new-password" passwordToggle required @@ -172,9 +174,12 @@ export class SignUpForm extends LiteElement { const { detail } = await resp.json(); if (detail === "REGISTER_USER_ALREADY_EXISTS") { shouldLogIn = true; + } else if (detail.code && detail.code === "REGISTER_INVALID_PASSWORD") { + this.serverError = msg( + "Invalid password. Must be between 8 and 64 characters" + ); } else { - // TODO show validation details - this.serverError = msg("Invalid email address or password"); + this.serverError = msg("Invalid email or password"); } break; default: diff --git a/frontend/src/pages/reset-password.ts b/frontend/src/pages/reset-password.ts index e819c26a54..875862ce55 100644 --- a/frontend/src/pages/reset-password.ts +++ b/frontend/src/pages/reset-password.ts @@ -38,6 +38,8 @@ export class ResetPassword extends LiteElement { name="password" type="password" label="${msg("New password")}" + help-text=${msg("Must be between 8-64 characters")} + minlength="8" autocomplete="new-password" passwordToggle required @@ -95,17 +97,19 @@ export class ResetPassword extends LiteElement { case 400: case 422: const { detail } = await resp.json(); - if (detail === "RESET_PASSWORD_BAD_TOKEN") { // TODO password validation details this.serverError = msg( "Password reset email is not valid. Request a new password reset email" ); - } else { - // TODO password validation details - this.serverError = msg("Invalid password"); + } else if ( + detail.code && + detail.code === "RESET_PASSWORD_INVALID_PASSWORD" + ) { + this.serverError = msg( + "Invalid password. Must be between 8 and 64 characters" + ); } - break; default: this.serverError = msg("Something unexpected went wrong");