From a38140d547358f11b8963338569b653691af11e1 Mon Sep 17 00:00:00 2001 From: CP Date: Mon, 18 Aug 2025 16:06:06 -0400 Subject: [PATCH 1/3] hold --- authentication/api_gateway/serializers.py | 25 ++++++ .../api_gateway/serializers_test.py | 76 +++++++++++++++++++ 2 files changed, 101 insertions(+) diff --git a/authentication/api_gateway/serializers.py b/authentication/api_gateway/serializers.py index 6d3778a9b2..7f6e7da02a 100644 --- a/authentication/api_gateway/serializers.py +++ b/authentication/api_gateway/serializers.py @@ -1,6 +1,7 @@ """Authentication serializers""" import logging +import re from django.contrib.auth import get_user_model from django.db import transaction @@ -8,9 +9,12 @@ from hubspot_sync.task_helpers import sync_hubspot_user from openedx.api import create_user +from openedx.constants import OPENEDX_USERNAME_MAX_LEN from users.serializers import ( LegalAddressSerializer, UserProfileSerializer, + USERNAME_RE_PARTIAL, + USERNAME_ERROR_MSG, ) log = logging.getLogger() @@ -25,6 +29,27 @@ class RegisterDetailsSerializer(serializers.Serializer): legal_address = LegalAddressSerializer(write_only=True) user_profile = UserProfileSerializer(write_only=True) + def validate_username(self, value): + """Validate username format and length""" + trimmed_value = value.strip() + + # Check length constraints + if len(trimmed_value) > OPENEDX_USERNAME_MAX_LEN: + msg = f"Username must be no more than {OPENEDX_USERNAME_MAX_LEN} characters." + raise serializers.ValidationError(msg) + + min_username_length = 3 + if len(trimmed_value) < min_username_length: + msg = "Username must be at least 3 characters." + raise serializers.ValidationError(msg) + + # Check character constraints using the same pattern as users.serializers + username_pattern = re.compile(rf"^{USERNAME_RE_PARTIAL}$") + if not username_pattern.match(trimmed_value): + raise serializers.ValidationError(USERNAME_ERROR_MSG) + + return trimmed_value + def create(self, validated_data): """Save user legal address and user profile""" request = self.context["request"] diff --git a/authentication/api_gateway/serializers_test.py b/authentication/api_gateway/serializers_test.py index 9b3c4fee69..ec211bff5e 100644 --- a/authentication/api_gateway/serializers_test.py +++ b/authentication/api_gateway/serializers_test.py @@ -6,6 +6,7 @@ RegisterDetailsSerializer, RegisterExtraDetailsSerializer, ) +from openedx.constants import OPENEDX_USERNAME_MAX_LEN from openedx.models import OpenEdxUser from users.factories import UserFactory @@ -128,3 +129,78 @@ def test_register_extra_details_serializer_valid_data(user): assert validated_data["gender"] == "Male" assert validated_data["birth_year"] == "1990" assert validated_data["company"] == "TechCorp" + + +@pytest.mark.django_db +@pytest.mark.parametrize( + ("username", "expected_valid"), + [ + ("validuser", True), # Valid username + ("valid_user-123", True), # Valid with allowed special chars + ("ab", False), # Too short (less than 3 chars) + ("a" * (OPENEDX_USERNAME_MAX_LEN + 1), False), # Too long (more than 30 chars) + ("user@domain", False), # Invalid character (@) + ("user#name", False), # Invalid character (#) + ("user$name", False), # Invalid character ($) + (" validuser ", True), # Valid with leading/trailing spaces (should be trimmed) + ("user name", True), # Valid with space + ("user.name", True), # Valid with period + ("user+name", True), # Valid with plus + ("user-name", True), # Valid with hyphen + ("user_name", True), # Valid with underscore + ], +) +def test_register_details_serializer_username_validation( + user, valid_address_dict, user_profile_dict, rf, username, expected_valid +): + """Test RegisterDetailsSerializer username validation""" + request = rf.post("/api/profile/details/") + request.user = user + + data = { + "name": "John Doe", + "username": username, + "legal_address": valid_address_dict, + "user_profile": user_profile_dict, + } + + serializer = RegisterDetailsSerializer(data=data, context={"request": request}) + + if expected_valid: + assert serializer.is_valid(), f"Username '{username}' should be valid but got errors: {serializer.errors}" + # Check that spaces are trimmed for valid usernames with leading/trailing spaces + if username.strip() != username: + assert serializer.validated_data["username"] == username.strip() + else: + assert not serializer.is_valid(), f"Username '{username}' should be invalid but passed validation" + assert "username" in serializer.errors, f"Username error not found in {serializer.errors}" + + +@pytest.mark.django_db +def test_register_details_serializer_username_length_error_message( + user, valid_address_dict, user_profile_dict, rf +): + """Test that username length error messages are descriptive""" + request = rf.post("/api/profile/details/") + request.user = user + + # Test too long username + long_username = "a" * (OPENEDX_USERNAME_MAX_LEN + 1) + data = { + "name": "John Doe", + "username": long_username, + "legal_address": valid_address_dict, + "user_profile": user_profile_dict, + } + + serializer = RegisterDetailsSerializer(data=data, context={"request": request}) + assert not serializer.is_valid() + assert "username" in serializer.errors + assert f"must be no more than {OPENEDX_USERNAME_MAX_LEN} characters" in str(serializer.errors["username"][0]) + + # Test too short username + data["username"] = "ab" + serializer = RegisterDetailsSerializer(data=data, context={"request": request}) + assert not serializer.is_valid() + assert "username" in serializer.errors + assert "must be at least 3 characters" in str(serializer.errors["username"][0]) From 5fbdc922472852f6577e640b58873aba6cd683ec Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Tue, 19 Aug 2025 16:04:03 +0000 Subject: [PATCH 2/3] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- authentication/api_gateway/serializers.py | 8 ++++--- .../api_gateway/serializers_test.py | 21 ++++++++++++++----- 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/authentication/api_gateway/serializers.py b/authentication/api_gateway/serializers.py index 7f6e7da02a..3732ffec67 100644 --- a/authentication/api_gateway/serializers.py +++ b/authentication/api_gateway/serializers.py @@ -11,10 +11,10 @@ from openedx.api import create_user from openedx.constants import OPENEDX_USERNAME_MAX_LEN from users.serializers import ( + USERNAME_ERROR_MSG, + USERNAME_RE_PARTIAL, LegalAddressSerializer, UserProfileSerializer, - USERNAME_RE_PARTIAL, - USERNAME_ERROR_MSG, ) log = logging.getLogger() @@ -35,7 +35,9 @@ def validate_username(self, value): # Check length constraints if len(trimmed_value) > OPENEDX_USERNAME_MAX_LEN: - msg = f"Username must be no more than {OPENEDX_USERNAME_MAX_LEN} characters." + msg = ( + f"Username must be no more than {OPENEDX_USERNAME_MAX_LEN} characters." + ) raise serializers.ValidationError(msg) min_username_length = 3 diff --git a/authentication/api_gateway/serializers_test.py b/authentication/api_gateway/serializers_test.py index ec211bff5e..9494ff4818 100644 --- a/authentication/api_gateway/serializers_test.py +++ b/authentication/api_gateway/serializers_test.py @@ -142,7 +142,10 @@ def test_register_extra_details_serializer_valid_data(user): ("user@domain", False), # Invalid character (@) ("user#name", False), # Invalid character (#) ("user$name", False), # Invalid character ($) - (" validuser ", True), # Valid with leading/trailing spaces (should be trimmed) + ( + " validuser ", + True, + ), # Valid with leading/trailing spaces (should be trimmed) ("user name", True), # Valid with space ("user.name", True), # Valid with period ("user+name", True), # Valid with plus @@ -167,13 +170,19 @@ def test_register_details_serializer_username_validation( serializer = RegisterDetailsSerializer(data=data, context={"request": request}) if expected_valid: - assert serializer.is_valid(), f"Username '{username}' should be valid but got errors: {serializer.errors}" + assert serializer.is_valid(), ( + f"Username '{username}' should be valid but got errors: {serializer.errors}" + ) # Check that spaces are trimmed for valid usernames with leading/trailing spaces if username.strip() != username: assert serializer.validated_data["username"] == username.strip() else: - assert not serializer.is_valid(), f"Username '{username}' should be invalid but passed validation" - assert "username" in serializer.errors, f"Username error not found in {serializer.errors}" + assert not serializer.is_valid(), ( + f"Username '{username}' should be invalid but passed validation" + ) + assert "username" in serializer.errors, ( + f"Username error not found in {serializer.errors}" + ) @pytest.mark.django_db @@ -196,7 +205,9 @@ def test_register_details_serializer_username_length_error_message( serializer = RegisterDetailsSerializer(data=data, context={"request": request}) assert not serializer.is_valid() assert "username" in serializer.errors - assert f"must be no more than {OPENEDX_USERNAME_MAX_LEN} characters" in str(serializer.errors["username"][0]) + assert f"must be no more than {OPENEDX_USERNAME_MAX_LEN} characters" in str( + serializer.errors["username"][0] + ) # Test too short username data["username"] = "ab" From 0cc6e37ea5481f3bfa738670b474fc5ab6952649 Mon Sep 17 00:00:00 2001 From: CP Date: Fri, 22 Aug 2025 12:43:27 -0400 Subject: [PATCH 3/3] ruff --- .../api_gateway/serializers_test.py | 50 +++++++++++-------- 1 file changed, 28 insertions(+), 22 deletions(-) diff --git a/authentication/api_gateway/serializers_test.py b/authentication/api_gateway/serializers_test.py index 9494ff4818..79a0884caf 100644 --- a/authentication/api_gateway/serializers_test.py +++ b/authentication/api_gateway/serializers_test.py @@ -45,14 +45,13 @@ def test_register_details_serializer_create( @pytest.mark.django_db @responses.activate -def test_register_no_edx_user( # noqa: PLR0913 - mocker, settings, user, valid_address_dict, user_profile_dict, rf -): +def test_register_no_edx_user(mocker, settings, register_details_test_data): """Test the create method of RegisterDetailsSerializer""" - request = rf.post("/api/profile/details/") - user = UserFactory.create(no_openedx_user=True, no_openedx_api_auth=True) + + # Create a new request with the different user + request = register_details_test_data["request"] request.user = user responses.add( @@ -64,12 +63,8 @@ def test_register_no_edx_user( # noqa: PLR0913 patched_create_edx_auth_token = mocker.patch("openedx.api.create_edx_auth_token") - data = { - "name": "John Doe", - "username": "johndoe", - "legal_address": valid_address_dict, - "user_profile": user_profile_dict, - } + data = register_details_test_data["base_data"].copy() + data["username"] = "johndoe" assert user.openedx_user is None serializer = RegisterDetailsSerializer(data=data, context={"request": request}) @@ -131,6 +126,22 @@ def test_register_extra_details_serializer_valid_data(user): assert validated_data["company"] == "TechCorp" +@pytest.fixture +def register_details_test_data(user, valid_address_dict, user_profile_dict, rf): + """Fixture providing test data for RegisterDetailsSerializer tests""" + request = rf.post("/api/profile/details/") + request.user = user + + return { + "base_data": { + "name": "John Doe", + "legal_address": valid_address_dict, + "user_profile": user_profile_dict, + }, + "request": request, + } + + @pytest.mark.django_db @pytest.mark.parametrize( ("username", "expected_valid"), @@ -154,20 +165,15 @@ def test_register_extra_details_serializer_valid_data(user): ], ) def test_register_details_serializer_username_validation( - user, valid_address_dict, user_profile_dict, rf, username, expected_valid + register_details_test_data, username, expected_valid ): """Test RegisterDetailsSerializer username validation""" - request = rf.post("/api/profile/details/") - request.user = user + data = register_details_test_data["base_data"].copy() + data["username"] = username - data = { - "name": "John Doe", - "username": username, - "legal_address": valid_address_dict, - "user_profile": user_profile_dict, - } - - serializer = RegisterDetailsSerializer(data=data, context={"request": request}) + serializer = RegisterDetailsSerializer( + data=data, context={"request": register_details_test_data["request"]} + ) if expected_valid: assert serializer.is_valid(), (