From ce664da46ab7f86d29583ebc34f2ff776f0aa6c2 Mon Sep 17 00:00:00 2001 From: Andres Torres Date: Wed, 20 Nov 2024 09:47:47 -0600 Subject: [PATCH] Merge commit from fork * HJ-172 - Password Policy is now Enforced in Accept Invite API * Update changelog * HJ-172 - Password Policy is now Enforced in Accept Invite API * QA --- CHANGELOG.md | 3 + .../api/api/v1/endpoints/user_endpoints.py | 4 +- src/fides/api/schemas/user.py | 39 +++++++++--- .../api/v1/endpoints/test_user_endpoints.py | 62 +++++++++++++++---- 4 files changed, 88 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 35821c616b..02e738a806 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,9 @@ The types of changes are: ### Docs - Added docs for PrivacyNoticeRegion type [#5488](https://github.com/ethyca/fides/pull/5488) +### Security + - Password Policy is now Enforced in Accept Invite API [CVE-2024-52008](https://github.com/ethyca/fides/security/advisories/GHSA-v7vm-rhmg-8j2r) + ## [2.49.1](https://github.com/ethyca/fidesplus/compare/2.49.0...2.49.1) ### Added diff --git a/src/fides/api/api/v1/endpoints/user_endpoints.py b/src/fides/api/api/v1/endpoints/user_endpoints.py index 78dc45b527..ab6b491a42 100644 --- a/src/fides/api/api/v1/endpoints/user_endpoints.py +++ b/src/fides/api/api/v1/endpoints/user_endpoints.py @@ -173,7 +173,7 @@ def update_user_password( status_code=HTTP_401_UNAUTHORIZED, detail="Incorrect password." ) - current_user.update_password(db=db, new_password=b64_str_to_str(data.new_password)) + current_user.update_password(db=db, new_password=data.new_password) logger.info("Updated user with id: '{}'.", current_user.id) return current_user @@ -202,7 +202,7 @@ def force_update_password( detail=f"User with ID {user_id} does not exist.", ) - user.update_password(db=db, new_password=b64_str_to_str(data.new_password)) + user.update_password(db=db, new_password=data.new_password) logger.info("Updated user with id: '{}'.", user.id) return user diff --git a/src/fides/api/schemas/user.py b/src/fides/api/schemas/user.py index 1b1f1b738d..c1c538dfa5 100644 --- a/src/fides/api/schemas/user.py +++ b/src/fides/api/schemas/user.py @@ -10,6 +10,7 @@ from fides.api.schemas.oauth import AccessToken + class PrivacyRequestReviewer(FidesSchema): """Data we can expose via the PrivacyRequest.reviewer relation""" @@ -40,19 +41,29 @@ def validate_username(cls, username: str) -> str: def validate_password(cls, password: str) -> str: """Add some password requirements""" decoded_password = decode_password(password) - - if len(decoded_password) < 8: + return UserCreate._validate_password(decoded_password) + + @staticmethod + def _validate_password(password: str) -> str: + """ + Validate password requirements. + Raises: + ValueError: If password does not meet requirements + Returns: + str: password + """ + if len(password) < 8: raise ValueError("Password must have at least eight characters.") - if re.search("[0-9]", decoded_password) is None: + if re.search("[\d]", password) is None: raise ValueError("Password must have at least one number.") - if re.search("[A-Z]", decoded_password) is None: + if re.search("[A-Z]", password) is None: raise ValueError("Password must have at least one capital letter.") - if re.search("[a-z]", decoded_password) is None: + if re.search("[a-z]", password) is None: raise ValueError("Password must have at least one lowercase letter.") - if re.search(r"[\W_]", decoded_password) is None: + if re.search(r"[\W_]", password) is None: raise ValueError("Password must have at least one symbol.") - return decoded_password + return password class UserCreateResponse(FidesSchema): @@ -102,12 +113,26 @@ class UserPasswordReset(FidesSchema): old_password: str new_password: str + @field_validator("new_password") + @classmethod + def validate_new_password(cls, password: str) -> str: + """Add some password requirements""" + decoded_password = decode_password(password) + return UserCreate._validate_password(decoded_password) + class UserForcePasswordReset(FidesSchema): """Only a new password, for the case where the user does not remember their password""" new_password: str + @field_validator("new_password") + @classmethod + def validate_new_password(cls, password: str) -> str: + """Add some password requirements""" + decoded_password = decode_password(password) + return UserCreate._validate_password(decoded_password) + class UserUpdate(FidesSchema): """Data required to update a FidesUser""" diff --git a/tests/ops/api/v1/endpoints/test_user_endpoints.py b/tests/ops/api/v1/endpoints/test_user_endpoints.py index cc48e3146b..7d1f16078d 100644 --- a/tests/ops/api/v1/endpoints/test_user_endpoints.py +++ b/tests/ops/api/v1/endpoints/test_user_endpoints.py @@ -852,7 +852,7 @@ def test_update_different_user_password( application_user, ) -> None: OLD_PASSWORD = "oldpassword" - NEW_PASSWORD = "newpassword" + NEW_PASSWORD = "Newpassword1!" application_user.update_password(db=db, new_password=OLD_PASSWORD) auth_header = generate_auth_header_for_user(user=application_user, scopes=[]) @@ -874,7 +874,7 @@ def test_update_different_user_password( application_user = application_user.refresh_from_db(db=db) assert application_user.credentials_valid(password=OLD_PASSWORD) - def test_update_user_password_invalid( + def test_update_user_password_invalid_old_password( self, api_client, db, @@ -882,7 +882,7 @@ def test_update_user_password_invalid( application_user, ) -> None: OLD_PASSWORD = "oldpassword" - NEW_PASSWORD = "newpassword" + NEW_PASSWORD = "Newpassword1!" application_user.update_password(db=db, new_password=OLD_PASSWORD) auth_header = generate_auth_header_for_user(user=application_user, scopes=[]) @@ -909,7 +909,7 @@ def test_update_user_password( application_user, ) -> None: OLD_PASSWORD = "oldpassword" - NEW_PASSWORD = "newpassword" + NEW_PASSWORD = "Newpassword1!" application_user.update_password(db=db, new_password=OLD_PASSWORD) auth_header = generate_auth_header_for_user(user=application_user, scopes=[]) resp = api_client.post( @@ -934,7 +934,7 @@ def test_force_update_different_user_password_without_scope( application_user, ) -> None: """A user without the proper scope cannot change another user's password""" - NEW_PASSWORD = "newpassword" + NEW_PASSWORD = "Newpassword1!" old_hashed_password = user.hashed_password auth_header = generate_auth_header_for_user(user=application_user, scopes=[]) @@ -965,7 +965,7 @@ def test_force_update_different_user_password( A user with the right scope should be able to set a new password for another user. """ - NEW_PASSWORD = "newpassword" + NEW_PASSWORD = "Newpassword1!" auth_header = generate_auth_header_for_user( user=application_user, scopes=[USER_PASSWORD_RESET] ) @@ -982,6 +982,46 @@ def test_force_update_different_user_password( user = user.refresh_from_db(db=db) assert user.credentials_valid(password=NEW_PASSWORD) + @pytest.mark.parametrize( + "new_password, expected_error", + [ + ("short", "Value error, Password must have at least eight characters."), + ("longerpassword", "Value error, Password must have at least one number."), + ("longer55password", "Value error, Password must have at least one capital letter."), + ("LONGER55PASSWORD", "Value error, Password must have at least one lowercase letter."), + ("LoNgEr55paSSworD", "Value error, Password must have at least one symbol."), + ], + ) + def test_force_update_bad_password( + self, + api_client, + db, + url_no_id, + user, + application_user, + new_password, + expected_error, + ) -> None: + """ + A user with the right scope should be able to set a new password + for another user. + """ + auth_header = generate_auth_header_for_user( + user=application_user, scopes=[USER_PASSWORD_RESET] + ) + + resp = api_client.post( + f"{url_no_id}/{user.id}/force-reset-password", + headers=auth_header, + json={ + "new_password": str_to_b64_str(new_password), + }, + ) + + assert resp.status_code == HTTP_422_UNPROCESSABLE_ENTITY + assert expected_error in resp.json()["detail"][0]["msg"] + db.expunge(user) + def test_force_update_non_existent_user( self, api_client, @@ -991,7 +1031,7 @@ def test_force_update_non_existent_user( """ Resetting on a user that does not exist should 404 """ - NEW_PASSWORD = "newpassword" + NEW_PASSWORD = "Newpassword1!" auth_header = generate_auth_header_for_user( user=application_user, scopes=[USER_PASSWORD_RESET] ) @@ -1902,7 +1942,7 @@ def test_accept_invite_valid( response = api_client.post( url, params={"username": "valid_user", "invite_code": "valid_code"}, - json={"username": "valid_user", "new_password": "pass"}, + json={"username": "valid_user", "new_password": "Testpassword1!"}, ) assert response.status_code == HTTP_200_OK @@ -1925,7 +1965,7 @@ def test_accept_invite_invalid_code(self, db, api_client, url): response = api_client.post( url, params={"username": "valid_user", "invite_code": "invalid_code"}, - json={"username": "valid_user", "new_password": "pass"}, + json={"username": "valid_user", "new_password": "Testpassword1!"}, ) assert response.status_code == HTTP_400_BAD_REQUEST assert response.json()["detail"] == "Invite code is invalid." @@ -1943,7 +1983,7 @@ def test_accept_invite_expired_code(self, mock_get_by, api_client: TestClient, u response = api_client.post( url, params={"username": "valid_user", "invite_code": "expired_code"}, - json={"username": "valid_user", "new_password": "pass"}, + json={"username": "valid_user", "new_password": "Testpassword1!"}, ) assert response.status_code == HTTP_400_BAD_REQUEST assert response.json()["detail"] == "Invite code has expired." @@ -1954,7 +1994,7 @@ def test_accept_invite_nonexistent_user(self, api_client, url): params={"username": "nonexistent_user", "invite_code": "some_code"}, json={ "username": "nonexistent_user", - "new_password": "pass", + "new_password": "Testpassword1!", }, ) assert response.status_code == HTTP_404_NOT_FOUND