Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: prevent HTML/URLs in the Full Name field #2994

Merged
merged 7 commits into from
Aug 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be a part of the courseware. This should either be moved to users or opened directory

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have an openedx directory in xPro, but it exists in MITx Online in replace of courseware for handling all API interactions with the edX platform. This is beneficial as it centralizes interactions with Open edX.

In xPro, courseware serves a similar role e.g. both course-related and user-specific Open edX API interactions.
The name courseware can be misleading since it implies only course-specific API interactions, while it actually includes other APIs interactions. I would recommend to lets keep it here to have common directory to interact with openedx similar to what we have in mitxonline and should have some meaningful name in future that serves the purpose to interact with openedx.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I agree. Let's keep it here for now. But my recommendation would be to create and openedx app and move this there but we can do that later in a refactoring PR.

"""
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also check the error message generated with the exception.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not be a part of courseware app.

Copy link
Contributor Author

@mudassir-hafeez mudassir-hafeez Jul 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and all other related instances you mentioned will hold if we agree with this comment #2994 (comment).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agreed to #2994 (comment).

"""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
Loading