Skip to content

Commit

Permalink
fix: prevent HTML/URLs in the Full Name field (#2994)
Browse files Browse the repository at this point in the history
  • Loading branch information
mudassir-hafeez authored Aug 19, 2024
1 parent 1e40d39 commit f386122
Show file tree
Hide file tree
Showing 19 changed files with 226 additions and 16 deletions.
1 change: 1 addition & 0 deletions authentication/api_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ def test_create_user_session(user):
assert session.session_key is not None


@pytest.mark.usefixtures("mock_validate_user_registration")
def test_create_user_with_generated_username(mocker, valid_address_dict):
"""
Integration test to assert that create_user_with_generated_username tries to find an available
Expand Down
3 changes: 2 additions & 1 deletion authentication/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,10 @@ def __init__(self, backend, social_auth):
class PartialException(AuthException):
"""Partial pipeline exception"""

def __init__(self, backend, partial, errors=None):
def __init__(self, backend, partial, errors=None, field_errors=None):
self.partial = partial
self.errors = errors
self.field_errors = field_errors
super().__init__(backend)


Expand Down
6 changes: 5 additions & 1 deletion authentication/pipeline/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from django.conf import settings
from django.contrib.auth import get_user_model
from django.db import IntegrityError
from mitol.common.utils import dict_without_keys
from social_core.backends.email import EmailAuth
from social_core.exceptions import AuthAlreadyAssociated, AuthException
from social_core.pipeline.partial import partial
Expand Down Expand Up @@ -138,7 +139,10 @@ def create_user_via_email(

if not serializer.is_valid():
raise RequirePasswordAndPersonalInfoException(
backend, current_partial, errors=serializer.errors
backend,
current_partial,
errors=serializer.errors.get("non_field_errors"),
field_errors=dict_without_keys(serializer.errors, "non_field_errors"),
)

try:
Expand Down
3 changes: 3 additions & 0 deletions authentication/pipeline/user_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@ def test_create_user_via_email_exit(mocker, backend_name, flow):


@pytest.mark.django_db
@pytest.mark.usefixtures("mock_validate_user_registration")
def test_create_user_via_email(mocker, mock_email_backend, mock_create_user_strategy):
"""
Tests that create_user_via_email creates a user via social_core.pipeline.user.create_user_via_email,
Expand Down Expand Up @@ -381,6 +382,7 @@ def test_create_user_via_email_with_email_case_insensitive_existing_user(
"create_user_return_val,create_user_exception", # noqa: PT006
[[None, None], [UserFactory.build(), ValueError("bad value")]], # noqa: PT007
)
@pytest.mark.usefixtures("mock_validate_user_registration")
def test_create_user_via_email_create_fail(
mocker,
mock_email_backend,
Expand All @@ -406,6 +408,7 @@ def test_create_user_via_email_create_fail(


@pytest.mark.django_db
@pytest.mark.usefixtures("mock_validate_user_registration")
def test_create_user_via_email_affiliate(
mocker, mock_create_user_strategy, mock_email_backend
):
Expand Down
8 changes: 6 additions & 2 deletions authentication/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
UserTryAgainLaterException,
)
from authentication.utils import SocialAuthState
from users.constants import USER_REGISTRATION_FAILED_MSG

PARTIAL_PIPELINE_TOKEN_KEY = "partial_pipeline_token" # noqa: S105

Expand Down Expand Up @@ -169,12 +170,15 @@ def save(self, **kwargs): # noqa: C901
)
except RequirePasswordAndPersonalInfoException as exc:
result = SocialAuthState(
SocialAuthState.STATE_REGISTER_DETAILS, partial=exc.partial
SocialAuthState.STATE_REGISTER_DETAILS,
partial=exc.partial,
errors=exc.errors or [],
field_errors=exc.field_errors or {},
)
except UserTryAgainLaterException:
result = SocialAuthState(
SocialAuthState.STATE_ERROR_TEMPORARY,
errors=["Unable to register at this time, please try again later"],
errors=[USER_REGISTRATION_FAILED_MSG],
)
except AuthException as exc:
log.exception("Received unexpected AuthException")
Expand Down
6 changes: 6 additions & 0 deletions authentication/views_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,10 @@ class AuthStateMachine(RuleBasedStateMachine):
email_send_patcher = patch(
"mail.verification_api.send_verification_email", autospec=True
)
mock_edx_name_patcher = patch(
"users.serializers.validate_name_with_edx",
return_value="",
)
courseware_api_patcher = patch("authentication.pipeline.user.courseware_api")
courseware_tasks_patcher = patch("authentication.pipeline.user.courseware_tasks")

Expand All @@ -156,6 +160,7 @@ def __init__(self):

# wrap the execution in a patch()
self.mock_email_send = self.email_send_patcher.start()
self.mock_edx_name_api = self.mock_edx_name_patcher.start()
self.mock_courseware_api = self.courseware_api_patcher.start()
self.mock_courseware_tasks = self.courseware_tasks_patcher.start()

Expand All @@ -179,6 +184,7 @@ def teardown(self):
self.email_send_patcher.stop()
self.courseware_api_patcher.stop()
self.courseware_tasks_patcher.stop()
self.mock_edx_name_patcher.stop()

# end the transaction with a rollback to cleanup any state
transaction.set_rollback(True) # noqa: FBT003, RUF100
Expand Down
36 changes: 36 additions & 0 deletions courseware/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
from courseware.exceptions import (
CoursewareUserCreateError,
EdxApiEnrollErrorException,
EdxApiRegistrationValidationException,
NoEdxApiAuthError,
OpenEdXOAuth2Error,
UnknownEdxApiEnrollException,
Expand Down Expand Up @@ -534,6 +535,20 @@ def get_edx_api_service_client():
)


def get_edx_api_registration_validation_client():
"""
Gets an Open edX api client instance for the user registration
Returns:
EdxApi: Open edX api registration client instance
"""
return EdxApi(
{"access_token": ""},
settings.OPENEDX_API_BASE_URL,
timeout=settings.EDX_API_CLIENT_TIMEOUT,
)


def get_edx_api_course_detail_client():
"""
Gets an edx api client instance for use with the grades api
Expand Down Expand Up @@ -816,3 +831,24 @@ def delete_oauth_application():
name=settings.OPENEDX_OAUTH_APP_NAME
).delete()
return _, deleted_applications_count


def validate_name_with_edx(name):
"""
Returns validation message after validating it with Open edX.
Args:
name (str): The full name
Raises:
EdxApiRegistrationValidationException: Raised if response status is not 200.
"""
edx_client = get_edx_api_registration_validation_client()
try:
response = edx_client.user_validation.validate_user_registration_info(
registration_information={"name": name},
)
except Exception as exc:
raise EdxApiRegistrationValidationException(name, exc.response) from exc
else:
return response.name
56 changes: 56 additions & 0 deletions courseware/api_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
update_edx_user_email,
update_edx_user_name,
update_xpro_user_username,
validate_name_with_edx,
)
from courseware.constants import (
COURSEWARE_REPAIR_GRACE_PERIOD_MINS,
Expand All @@ -47,6 +48,7 @@
from courseware.exceptions import (
CoursewareUserCreateError,
EdxApiEnrollErrorException,
EdxApiRegistrationValidationException,
UnknownEdxApiEnrollException,
UserNameUpdateFailedException,
)
Expand Down Expand Up @@ -110,6 +112,60 @@ def update_token_response_error(settings):
return SimpleNamespace(refresh_token=refresh_token, access_token=access_token)


def test_validate_name_with_edx_success(mock_validate_user_registration):
"""
Test that validate_name_with_edx successfully returns the validation message
from Open edX API when the name is valid.
"""
name = "Test User"

result = validate_name_with_edx(name)
assert result == ""
mock_validate_user_registration.user_validation.validate_user_registration_info.assert_called_once_with(
registration_information={"name": name}
)


def test_validate_name_with_edx_failure(mocker):
"""
Test that validate_name_with_edx raises EdxApiRegistrationValidationException
when the Open edX API call fails.
"""
name = "Test User"

class MockApiException(Exception): # noqa: N818
"""Mock exception for API errors with a response attribute."""

def __init__(self, message, response):
super().__init__(message)
self.response = response

mock_response = mocker.MagicMock()
mock_response.status_code = 403
mock_response.headers = {"Content-Type": "application/json"}
mock_response.text = "Some error details"

mock_client = mocker.MagicMock()
mock_client.user_validation.validate_user_registration_info.side_effect = (
MockApiException("API error", response=mock_response)
)
mocker.patch(
"courseware.api.get_edx_api_registration_validation_client",
return_value=mock_client,
)

with pytest.raises(EdxApiRegistrationValidationException) as exc_info:
validate_name_with_edx(name)
mock_client.user_validation.validate_user_registration_info.assert_called_once_with(
registration_information={"name": name}
)
assert (
str(exc_info.value)
== f"EdX API error validating registration name {name}.\nResponse - code: {mock_response.status_code}, "
f"content: {mock_response.text}"
)


@pytest.fixture
def create_token_responses(settings):
"""Mock responses for creating an auth token"""
Expand Down
19 changes: 19 additions & 0 deletions courseware/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,3 +63,22 @@ def __init__(self, user, course_run, base_exc, msg=None):

class UserNameUpdateFailedException(Exception): # noqa: N818
"""Raised if a user's profile name(Full Name) update call is failed"""


class EdxApiRegistrationValidationException(Exception): # noqa: N818
"""An Open edX Registration Validation API call resulted in an error response"""

def __init__(self, name, error_response, msg=None):
"""
Sets exception properties and adds a default message
Args:
name (str): The name being validated
response (requests.Response): Open edX response for name validation
"""
self.name = name
self.response = error_response
if msg is None:
# Set some default useful error message
msg = f"EdX API error validating registration name {self.name}.\n{get_error_response_summary(self.response)}"
super().__init__(msg)
18 changes: 18 additions & 0 deletions fixtures/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -110,3 +110,21 @@ def valid_address_dict():
def nplusone_fail(settings): # noqa: PT004
"""Configures the nplusone app to raise errors"""
settings.NPLUSONE_RAISE = True


@pytest.fixture
def mock_validate_user_registration(mocker):
"""Fixture to mock validate_user_registration_info method."""
mock_response = mocker.MagicMock()
mock_response.name = ""

mock_client = mocker.MagicMock()
mock_client.user_validation.validate_user_registration_info.return_value = (
mock_response
)
mocker.patch(
"courseware.api.get_edx_api_registration_validation_client",
return_value=mock_client,
)

return mock_client
8 changes: 4 additions & 4 deletions poetry.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ django-user-agents = "0.4.0"
django-webpack-loader = "0.7.0"
djangorestframework = "3.15.2"
drf-flex-fields = "0.9.9"
edx-api-client = "1.9.0"
edx-api-client = "1.10.0"
flaky = "3.8.1"
google-api-python-client = "1.12.11"
google-auth = "1.35.0"
Expand Down
1 change: 1 addition & 0 deletions static/js/components/forms/ProfileFormFields.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ export const legalAddressValidation = yup.object().shape({
.label("Full Name")
.trim()
.required()
.matches(NAME_REGEX, NAME_REGEX_FAIL_MESSAGE)
.max(255)
.min(2),
legal_address: yup.object().shape({
Expand Down
Loading

0 comments on commit f386122

Please sign in to comment.