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

mudassir-hafeez · 2024-05-26T20:46:27Z

What are the relevant tickets?

Description (What does it do?)

This PR adds validation for the full name. It also introduces an API to validate registration on Open edX to ensure a valid full name is provided. This change affects both the registration details page and the edit profile page.

Screenshots (if appropriate):

Validation error on registration details page:

Validation error on edit details page:

How can this be tested?

Fetch, check out to this branch, install edx-api-client PR feat: validate user registration api edx-api-client#106 with editable mode, and make sure edx is configured.
Test the front-end validation (registration/edit details pages):
- The full name should not start with a special character (~!@&)(+:'.?/,-), and should not contain any of (/^$#*=[]%_;<>{}|").
- Test by including any of these characters, HTML, or URL in the full name.
Test the back-end validation (registration/edit details pages):
- Disable the front-end validation temporarily by commenting out this line.
- Check if the validation error appears by entering any URL or HTML.
Test rate limiting/non-field errors (registration/edit details pages):
- Disable the front-end validation as mentioned above.
- Set REGISTRATION_VALIDATION_RATELIMIT to a low number of requests per IP address (e.g., 10/7d). By default, the validation registration endpoint is rate-limited to 30 requests per IP address per week.
- Ensure the notification error appears when the rate limit is exceeded.

Note: The last two pointers are for testing validation/API with backend.

Additional Context

Further screenshots of the back-end validation:

arslanashraf7

This is the code review. However, I would like to mention a couple of things:

Long ago we implemented the same thing in MITx Online but with username. (734 - registration validate username against openedx mitxonline#757). This was implemented in MITx Online and not edx-api-client.
The future of this registration validation would be to serve as a common functionality for both MITx Online and xPRO. So, we should implement it that way. (This is related to edx-api-client but adding it here to keep things together)
Once we are done with above, We also need to refactor the code in MITx Online so that it uses the edx-api-client

arslanashraf7 · 2024-07-01T09:18:31Z

authentication/api_test.py

+    mock_client = mocker.MagicMock()
+    mock_client.user_info.validate_user_registration.return_value = {
+        "validation_decisions": {"name": ""}
+    }
+    mocker.patch(
+        "courseware.api.get_edx_api_registration_client", return_value=mock_client
+    )
+


One of the following should be done here since this is a repetitive code and we should avoid it and make it reusable:

Move this to TestCaseSetup if exists

Create a fixture for this mock e.g. mock_email_send in code (Recommended)

arslanashraf7 · 2024-07-01T09:40:31Z

courseware/api.py

+    if settings.MITXPRO_REGISTRATION_ACCESS_TOKEN is None:
+        raise ImproperlyConfigured("MITXPRO_REGISTRATION_ACCESS_TOKEN is not set")  # noqa: EM101


I don't think this is needed. Isn't the validation API public? Because if the ACCESS_TOKEN isn't set it will stop the operation.

Now if we remove this check, We might as well not need a method at all. The initialization can be done on demand wherever needed.

arslanashraf7 · 2024-07-01T09:41:35Z

courseware/api.py

@@ -534,6 +535,23 @@ def get_edx_api_service_client():
    )


+def get_edx_api_registration_client():


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

arslanashraf7 · 2024-07-01T09:44:31Z

courseware/api.py

@@ -816,3 +834,26 @@ def delete_oauth_application():
        name=settings.OPENEDX_OAUTH_APP_NAME
    ).delete()
    return _, deleted_applications_count
+
+
+def validate_name_with_edx(name):


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

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.

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.

arslanashraf7 · 2024-07-01T09:46:20Z

courseware/api.py

+        name (str): The full name
+
+    Raises:
+        EdxApiRegistrationValidationException: Raised if response status is not OK.


not OK is not a good representation of what we are trying to say. It should say 200 or HTTP_200_OK

arslanashraf7 · 2024-07-01T10:09:20Z

users/serializers.py

@@ -267,6 +277,28 @@ def get_unused_coupons(self, instance):
            return fetch_and_serialize_unused_coupons(instance)
        return []

+    def validate(self, data):
+        """Validate an existing user"""


Looks like an incorrect docstring

arslanashraf7 · 2024-07-01T10:09:55Z

users/serializers.py

@@ -267,6 +277,28 @@ def get_unused_coupons(self, instance):
            return fetch_and_serialize_unused_coupons(instance)
        return []

+    def validate(self, data):


Should it be validate_full_name?

I have used general validation outside of validate_full_name in order to make name validation required in case if openedx fails and returns an object-level error. We will not allow account creation to proceed and a proper error message will be displayed:

We have something similar mitodl/mitxonline#1389 (review) suggestion for an object-level error message/handling done in mitxonline. The error message is subjective to what we want to use but I have used it what we were used https://github.com/mitodl/mitxpro/blob/master/authentication/serializers.py#L177 in such cases.

Stopping account creation can still be done even we do general validate or not. A general validate would be more useful when we want to throw multiple errors to the frontend at the same time.

arslanashraf7 · 2024-07-01T10:12:42Z

static/js/containers/pages/profile/EditProfilePage.js

-      if (errors && errors.length > 0) {
-        setErrors({
-          email: errors[0],
+      if (errors) {


Can we utilize the functionality of the Validation exception from the backend instead of making this code change in the frontend? But in reality are we sending multiple error messages from the backend?

Can we utilize the functionality of the Validation exception from the backend instead of making this code change in the frontend?

Is that change break anything with existing functionality?

I'm not sure if you have this question due to email: errors[0],, I tried to check email but did not get it's usage in on edit profile

I mean, when we return ValidationError from the backend would that become part of the field_errors?

Currently, We return a Validation Error in case legal address validation, But that doesn't get displayed on the frontend, That however, stops the registration process keeping the user on the same page. Take a look at this for reference. And I see that you are also throwing a validation error message. So, we don't need these frontend changes If that's still not going to be shown on the fields.

arslanashraf7 · 2024-07-01T10:14:03Z

static/js/containers/pages/register/RegisterDetailsPage.js

@@ -84,10 +91,31 @@ export class RegisterDetailsPage extends React.Component<Props, State> {
        partialToken,
      );

+      if (body.errors) {


Same as above message in EditProfile page

arslanashraf7 · 2024-07-01T10:15:26Z

users/serializers.py

+NAME_CONTAINS_HTML_URL_MSG = (
+    "Full name can not contain HTML or URL. Please try a different one."
+)


If we show the validation character message in this case as well, the would be fine. We don't need a new message for it.

arslanashraf7 · 2024-08-06T08:29:05Z

@mudassir-hafeez Could you rebase this PR ?

arslanashraf7 · 2024-08-06T23:30:00Z

courseware/api.py

+        return edx_client.user_validation.validate_user_registration(
+            registration_information=dict(  # noqa: C408
+                name=name,
+            )
+        ).name


It is always cleaner to get the API response in a variable i.e. response. Also, We unnecessarily ignored C408. We should try to resolve the issue instead of suppressing it here. e.g. You can pass a plain dictionary here.

arslanashraf7 · 2024-08-06T23:34:55Z

courseware/api.py

@@ -534,6 +535,26 @@ def get_edx_api_service_client():
    )


+def get_edx_api_registration_client():


Suggested change

def get_edx_api_registration_client():

def get_edx_api_registration_validation_client():

arslanashraf7 · 2024-08-06T23:36:04Z

courseware/api.py

@@ -534,6 +535,26 @@ def get_edx_api_service_client():
    )


+def get_edx_api_registration_client():
+    """
+    Gets an edx api client instance for the user registration


We should use Open edX in comments everywhere. I'm adding the suggestion here but you can check other cases like this.

Suggested change

Gets an edx api client instance for the user registration

Gets an Open edX api client instance for the user registration

arslanashraf7 · 2024-08-06T23:46:12Z

courseware/api.py

+    registration_access_token = (
+        settings.MITXPRO_REGISTRATION_ACCESS_TOKEN
+        if settings.MITXPRO_REGISTRATION_ACCESS_TOKEN
+        else ""
+    )
+
+    return EdxApi(


This API is not authenticated, We don't need to send a token maybe? you can directly send ""?

arslanashraf7 · 2024-08-06T23:58:28Z

users/serializers.py

+                HTTPError,
+                RequestsConnectionError,


I don't think these will ever be thrown. In validate_name_with_edx we catch a general exception and raise a EdxApiRegistrationValidationException every time.

mudassir-hafeez · 2024-08-13T14:11:29Z

@arslanashraf7 I've addressed all the request changes you mentioned in the current iteration along with mitodl/edx-api-client#106. They are ready for review now.

arslanashraf7 · 2024-08-16T06:47:53Z

While testing the PR I was having some weird issues, I don't know what's causing them but I wanted to check with you anyway.

I'm unable to load the dashboard page (Complaints about missing column)
I'm unable to load the profile page (Complaints about undefined value, likely related to ramada package)

Screenshots:

While loading the dashboard:

While loading profile:

mudassir-hafeez · 2024-08-16T12:22:30Z

While testing the PR I was having some weird issues, I don't know what's causing them but I wanted to check with you anyway.
I'm unable to load the dashboard page (Complaints about missing column)
I'm unable to load the profile page (Complaints about undefined value, likely related to ramada package)

Both issues seem to be unrelated to this PR. For the first issue, please check if the migrations have been applied correctly or if there have been any local changes related to the external course run. For the second issue, ensure that assets are compiling properly and that your local branch is up-to-date with mudassir/fix-html-urls--full-name-field, which includes the updated Ramda package.

I have also re-based this branch again in the meantime, and retested. I don't see any issues related to this. We can connect if those issues still persist.

arslanashraf7 · 2024-08-19T12:04:57Z

While testing the PR I was having some weird issues, I don't know what's causing them but I wanted to check with you anyway.
I'm unable to load the dashboard page (Complaints about missing column)
I'm unable to load the profile page (Complaints about undefined value, likely related to ramada package)
Both issues seem to be unrelated to this PR. For the first issue, please check if the migrations have been applied correctly or if there have been any local changes related to the external course run. For the second issue, ensure that assets are compiling properly and that your local branch is up-to-date with mudassir/fix-html-urls--full-name-field, which includes the updated Ramda package.

I have also re-based this branch again in the meantime, and retested. I don't see any issues related to this. We can connect if those issues still persist.

I'm not sure what was causing it but it only worked for me when I pulled your rebased branch.

arslanashraf7

👍 Looks good. Thanks for making all the changes and your patience on this.

Just on thing that we discussed over call about the error message in top bar. It stays there even if user fixes the issue and moved to the next page. Since this behaviour is pre-existing for Success case as well, we can work on that in a followup PR.

I would like to note that we should get the edx-api-client release out before merging this. We need to publish edx-api-client first and then pin the new version here for proper functioning.

…and registration code refactoring

mudassir-hafeez · 2024-08-19T14:46:05Z

@arslanashraf7 I've pinned the newly released edx-api-client version and tested it. Could you please review this final change?

odlbot had a problem deploying to xpro-ci-pr-2994 May 26, 2024 20:47 Failure

mudassir-hafeez force-pushed the mudassir/fix-html-urls--full-name-field branch from 9734ecc to c0e7912 Compare May 28, 2024 14:43

odlbot temporarily deployed to xpro-ci-pr-2994 May 28, 2024 14:43 Inactive

odlbot temporarily deployed to xpro-ci-pr-2994 May 28, 2024 14:44 Inactive

mudassir-hafeez force-pushed the mudassir/fix-html-urls--full-name-field branch from f132122 to 1ba7d9f Compare May 30, 2024 12:33

odlbot temporarily deployed to xpro-ci-pr-2994 May 30, 2024 12:33 Inactive

odlbot temporarily deployed to xpro-ci-pr-2994 May 30, 2024 12:36 Inactive

mudassir-hafeez force-pushed the mudassir/fix-html-urls--full-name-field branch from fcbea87 to 4144b19 Compare May 30, 2024 20:20

odlbot temporarily deployed to xpro-ci-pr-2994 May 30, 2024 20:21 Inactive

odlbot temporarily deployed to xpro-ci-pr-2994 May 30, 2024 20:22 Inactive

mudassir-hafeez force-pushed the mudassir/fix-html-urls--full-name-field branch from 49f2840 to d2272c0 Compare May 30, 2024 21:51

odlbot temporarily deployed to xpro-ci-pr-2994 May 30, 2024 21:52 Inactive

odlbot deployed to xpro-ci-pr-2994 May 30, 2024 21:52 View deployment

mudassir-hafeez mentioned this pull request May 30, 2024

feat: validate user registration api mitodl/edx-api-client#106

Merged

2 tasks

mudassir-hafeez marked this pull request as ready for review May 30, 2024 23:29

mudassir-hafeez added the Needs Review label May 30, 2024

arslanashraf7 self-assigned this Jun 26, 2024

arslanashraf7 requested changes Jul 1, 2024

View reviewed changes

arslanashraf7 added Waiting on Author and removed Needs Review labels Jul 1, 2024

mudassir-hafeez force-pushed the mudassir/fix-html-urls--full-name-field branch 2 times, most recently from e95c4d8 to 003b80f Compare July 9, 2024 17:54

mudassir-hafeez added Needs Review and removed Waiting on Author labels Jul 9, 2024

arslanashraf7 self-requested a review August 6, 2024 09:24

mudassir-hafeez force-pushed the mudassir/fix-html-urls--full-name-field branch 2 times, most recently from fe8a676 to 4f2ae1f Compare August 6, 2024 10:11

arslanashraf7 requested changes Aug 7, 2024

View reviewed changes

arslanashraf7 removed the Needs Review label Aug 7, 2024

arslanashraf7 added the Waiting on Author label Aug 7, 2024

mudassir-hafeez force-pushed the mudassir/fix-html-urls--full-name-field branch 2 times, most recently from 74eb93c to c9ba208 Compare August 13, 2024 13:50

mudassir-hafeez added Needs Review and removed Waiting on Author labels Aug 13, 2024

mudassir-hafeez force-pushed the mudassir/fix-html-urls--full-name-field branch from c9ba208 to 69d763e Compare August 16, 2024 12:18

arslanashraf7 approved these changes Aug 19, 2024

View reviewed changes

arslanashraf7 added Waiting on Author and removed Needs Review labels Aug 19, 2024

mudassir-hafeez and others added 7 commits August 19, 2024 18:23

fix: prevent HTML/URLs in the Full Name field

7ad8e2c

refactor: user notification/error handling on EditProfilePage, tests …

bf914cc

…and registration code refactoring

fix: tests

1ea0574

fix: ruff

b1a6f10

refactor: notification for full_name and some minor fixes

47bada0

refactor: few code refactoring

5769c1b

build(release): bump edx-api-client version to 1.10.0

6a6c048

mudassir-hafeez force-pushed the mudassir/fix-html-urls--full-name-field branch from 69d763e to 6a6c048 Compare August 19, 2024 14:26

mudassir-hafeez added Needs Review and removed Waiting on Author labels Aug 19, 2024

arslanashraf7 approved these changes Aug 19, 2024

View reviewed changes

arslanashraf7 added Waiting on Author and removed Needs Review labels Aug 19, 2024

mudassir-hafeez merged commit f386122 into master Aug 19, 2024
7 checks passed

mudassir-hafeez deleted the mudassir/fix-html-urls--full-name-field branch August 19, 2024 14:49

odlbot mentioned this pull request Aug 22, 2024

Release 0.157.1 #3108

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mudassir-hafeez commented May 26, 2024 •

edited

Loading

arslanashraf7 left a comment

arslanashraf7 Jul 1, 2024

arslanashraf7 Jul 1, 2024

arslanashraf7 Jul 1, 2024

arslanashraf7 Jul 1, 2024

mudassir-hafeez Jul 1, 2024

arslanashraf7 Jul 2, 2024

arslanashraf7 Jul 1, 2024

arslanashraf7 Jul 1, 2024

arslanashraf7 Jul 1, 2024

mudassir-hafeez Jul 1, 2024 •

edited

Loading

arslanashraf7 Jul 2, 2024

arslanashraf7 Jul 1, 2024

mudassir-hafeez Jul 1, 2024 •

edited

Loading

arslanashraf7 Jul 2, 2024

arslanashraf7 Jul 1, 2024

arslanashraf7 Jul 1, 2024

arslanashraf7 commented Aug 6, 2024

arslanashraf7 Aug 6, 2024

arslanashraf7 Aug 6, 2024

arslanashraf7 Aug 6, 2024

arslanashraf7 Aug 6, 2024

arslanashraf7 Aug 6, 2024

mudassir-hafeez commented Aug 13, 2024

arslanashraf7 commented Aug 16, 2024

mudassir-hafeez commented Aug 16, 2024

arslanashraf7 commented Aug 19, 2024

arslanashraf7 left a comment •

edited

Loading

mudassir-hafeez commented Aug 19, 2024

		if settings.MITXPRO_REGISTRATION_ACCESS_TOKEN is None:
		raise ImproperlyConfigured("MITXPRO_REGISTRATION_ACCESS_TOKEN is not set") # noqa: EM101

		@@ -534,6 +535,23 @@ def get_edx_api_service_client():
		)


		def get_edx_api_registration_client():

		@@ -534,6 +535,26 @@ def get_edx_api_service_client():
		)


		def get_edx_api_registration_client():

	def get_edx_api_registration_client():
	def get_edx_api_registration_validation_client():

	Gets an edx api client instance for the user registration
	Gets an Open edX api client instance for the user registration

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

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

Conversation

mudassir-hafeez commented May 26, 2024 • edited Loading

What are the relevant tickets?

Description (What does it do?)

Screenshots (if appropriate):

How can this be tested?

Additional Context

arslanashraf7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mudassir-hafeez Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mudassir-hafeez Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arslanashraf7 commented Aug 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mudassir-hafeez commented Aug 13, 2024

arslanashraf7 commented Aug 16, 2024

mudassir-hafeez commented Aug 16, 2024

arslanashraf7 commented Aug 19, 2024

arslanashraf7 left a comment • edited Loading

Choose a reason for hiding this comment

mudassir-hafeez commented Aug 19, 2024

mudassir-hafeez commented May 26, 2024 •

edited

Loading

mudassir-hafeez Jul 1, 2024 •

edited

Loading

mudassir-hafeez Jul 1, 2024 •

edited

Loading

arslanashraf7 left a comment •

edited

Loading