-
Notifications
You must be signed in to change notification settings - Fork 2
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
Changes from all commits
7ad8e2c
bf914cc
1ea0574
b1a6f10
47bada0
5769c1b
6a6c048
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
|
@@ -47,6 +48,7 @@ | |
from courseware.exceptions import ( | ||
CoursewareUserCreateError, | ||
EdxApiEnrollErrorException, | ||
EdxApiRegistrationValidationException, | ||
UnknownEdxApiEnrollException, | ||
UserNameUpdateFailedException, | ||
) | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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""" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should not be a part of courseware app. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 ofcourseware
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.
There was a problem hiding this comment.
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.