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

Restrict user to use accented characters in username #262

Closed
wants to merge 1 commit into from

Conversation

asadiqbal08
Copy link
Contributor

@asadiqbal08 asadiqbal08 commented Oct 20, 2021

Pre-Flight checklist

  • Testing
    • Code is tested
    • Changes have been manually tested

What are the relevant tickets?

fixes: #259

What's this PR do?

(Required)

How should this be manually tested?

Create a user with username that may have accented characters in it e.g. önlínê-user. The system should not allow it.

NOTE:

- I did not update the message Username can only contain letters, numbers, spaces, and the following characters: @_+- , seems of enough but if needs to modify then let me know.

@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2021

Codecov Report

Merging #262 (cc5e5cb) into main (878a9a0) will decrease coverage by 4.72%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #262      +/-   ##
==========================================
- Coverage   91.00%   86.27%   -4.73%     
==========================================
  Files         214       93     -121     
  Lines        6338     3542    -2796     
  Branches      314      314              
==========================================
- Hits         5768     3056    -2712     
+ Misses        483      399      -84     
  Partials       87       87              
Impacted Files Coverage Δ
users/serializers.py 94.83% <100.00%> (ø)
...ic/js/components/forms/RegisterDetailsForm_test.js
static/js/lib/validation.js
...ntainers/pages/register/RegisterDeniedPage_test.js
static/js/components/forms/RegisterEmailForm.js
static/js/contextDefinitions.js
static/js/constants.js
static/js/components/TopAppBar_test.js
static/js/lib/validation_test.js
static/js/factories/util.js
... and 112 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 878a9a0...cc5e5cb. Read the comment docs.

@arslanashraf7 arslanashraf7 self-assigned this Oct 20, 2021
@arslanashraf7
Copy link
Contributor

@asadiqbal08 You might need to re-run the tests since the CI is failing.

@@ -28,7 +28,7 @@
""",
flags=re.I | re.VERBOSE | re.MULTILINE,
)
USERNAME_RE_PARTIAL = r"[\w .@_+-]+"
USERNAME_RE_PARTIAL = r"[ A-Za-z0-9.@_+-]+"
Copy link
Contributor

Choose a reason for hiding this comment

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

Although functionality wise it works fine and doesn't allow accented characters, I have two minor things that you might consider here:

  1. I'm sure there could be a reference to using the space in the username which is unusual but if we do want to use it we should place that somewhere close to the allowed special characters list e.g. r"[A-Za-z0-9 .@_+-]+"
  2. Since we are matching the username text through a regex, shouldn't we add this regex validation on the frontend validator(https://github.com/mitodl/mitxonline/blob/main/static/js/lib/validation.js#L20) and we can save us an API call that is just going to tell the users about what characters they can use after hitting the submit button.

Copy link
Contributor

@arslanashraf7 arslanashraf7 Oct 20, 2021

Choose a reason for hiding this comment

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

I know point#2 might not be purely related to the PR but that has a chance to get fixed in this, @gsidebo could even give a better thought on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@arslanashraf7 regarding the point.2,

  • It makes sense to me that there should be frontend validation earlier before sending an api call.
  • space in username does not make sense to me as well. I keep it by considering it may be a requirement earlier but I have removed it and take input from @gsidebo if space really makes sense to be part of username.

kindly take a look over another commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

We currently have usernames with spaces. I don't think we need to remove that capability

@gsidebo gsidebo self-assigned this Oct 20, 2021
@gsidebo
Copy link
Contributor

gsidebo commented Oct 27, 2021

Just FYI I'm labeling this "on hold" for a few reasons:

  1. This doesn't help us for usernames that already have accents. If someone has already signed up as üser, that is their username in mitxonline and in Open edX. If another user comes along with the username user, that will trigger this same bug behavior. For this PR to avoid that issue as it stands now, we'd have to mass-update usernames to get rid of the accents. I want to see if we can avoid that.
  2. I don't know if there's a great UX for telling users that they can't use accented characters. The message as it is now is not clear enough (Username can only contain letters, numbers and the following characters: @_+-).
  3. If we can't figure out the true source of this bug, I think it probably makes more sense for us to add a new field to the mitxonline User model which will store the un-accented version of the username. That way a user can use accents in their username without an issue, but we can prevent 2 users from using the same username with different accents by checking against that new field

@gsidebo
Copy link
Contributor

gsidebo commented Nov 8, 2021

Closing this so we can re-implement according to the details in the updated issue (#259)

@gsidebo gsidebo closed this Nov 8, 2021
@rhysyngsun rhysyngsun deleted the asadiqbal08/fix-259 branch July 20, 2022 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix user creation for users with accented characters in their usernames
4 participants