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

Add non-ASCII character check for username suggestions #36056

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
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: 0 additions & 1 deletion .github/workflows/add-remove-label-on-comment.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,3 @@ on:
jobs:
add_remove_labels:
uses: openedx/.github/.github/workflows/add-remove-label-on-comment.yml@master

9 changes: 4 additions & 5 deletions .github/workflows/check_python_dependencies.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,18 @@ jobs:
steps:
- name: Checkout Repository
uses: actions/checkout@v4

- name: Set up Python
uses: actions/setup-python@v5
with:
python-version: ${{ matrix.python-version }}

- name: Install repo-tools
run: pip install edx-repo-tools[find_dependencies]

- name: Install setuptool
run: pip install setuptools
run: pip install setuptools

- name: Run Python script
run: |
find_python_dependencies \
Expand All @@ -37,4 +37,3 @@ jobs:
--ignore https://github.com/mitodl/edx-sga \
--ignore https://github.com/edx/token-utils \
--ignore https://github.com/open-craft/xblock-poll

2 changes: 1 addition & 1 deletion lms/djangoapps/course_api/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ class LazyPageNumberPagination(NamespacedPageNumberPagination):

The paginator cache uses ``@cached_property`` to cache the property values for
count and num_pages. It assumes these won't change, but in the case of a
LazySquence, its count gets updated as we move through it. This class clears
LazySequence, its count gets updated as we move through it. This class clears
the cached property values before reporting results so they will be recalculated.

"""
Expand Down
76 changes: 75 additions & 1 deletion openedx/core/djangoapps/user_authn/tests/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,11 @@
from django.test.utils import override_settings

from openedx.core.djangoapps.oauth_dispatch.tests.factories import ApplicationFactory
from openedx.core.djangoapps.user_authn.utils import is_safe_login_or_logout_redirect
from openedx.core.djangoapps.user_authn.utils import (
is_safe_login_or_logout_redirect,
generate_username_suggestions,
remove_special_characters_from_name,
)


@ddt.ddt
Expand Down Expand Up @@ -68,3 +72,73 @@ def test_safe_redirect_oauth2(self, client_redirect_uri, redirect_url, host, exp
req = self.request.get(f'/logout?{urlencode(params)}', HTTP_HOST=host)
actual_is_safe = self._is_safe_redirect(req, redirect_url)
assert actual_is_safe == expected_is_safe


@ddt.ddt
class TestUsernameGeneration(TestCase):
"""Test username generation utility methods."""

def test_remove_special_characters(self):
"""Test the removal of special characters from a name."""
test_cases = [
('John Doe', 'JohnDoe'),
('John@Doe', 'JohnDoe'),
('John.Doe', 'JohnDoe'),
('John_Doe', 'John_Doe'), # Underscore is allowed
('John-Doe', 'John-Doe'), # Hyphen is allowed
('John$#@!Doe', 'JohnDoe'),
]
for input_name, expected in test_cases:
assert remove_special_characters_from_name(input_name) == expected

@ddt.data(
# Test normal ASCII name
('John Doe', True), # Should return suggestions
('Jane Smith', True), # Should return suggestions
# Test non-ASCII names
('José García', False), # Contains non-ASCII characters
('مریم میرزاخانی', False), # Persian name
('明美 田中', False), # Japanese name
('Σωκράτης', False), # Greek name
('Владимир', False), # Cyrillic characters
# Test edge cases
('A B', True), # Minimal valid name
('', True), # Empty string
(' ', True), # Just spaces
)
@ddt.unpack
def test_username_suggestions_ascii_check(self, name, should_generate):
"""Test username suggestion generation for ASCII and non-ASCII names."""
suggestions = generate_username_suggestions(name)

if should_generate:
if name.strip(): # If name is not empty or just spaces
# Should generate up to 3 suggestions for valid ASCII names
assert len(suggestions) <= 3
# Verify all suggestions are ASCII
for suggestion in suggestions:
assert suggestion.isascii()
assert suggestion.replace('_', '').replace('-', '').isalnum()
else:
# Empty or whitespace-only names should return empty list
assert not suggestions
else:
# Should return empty list for non-ASCII names
assert not suggestions

def test_unique_suggestions(self):
"""Test that generated suggestions are unique."""
name = "John Doe"
suggestions = generate_username_suggestions(name)
assert len(suggestions) == len(set(suggestions)), "All suggestions should be unique"

def test_suggestion_length(self):
"""Test that generated suggestions respect the maximum length."""
from openedx.core.djangoapps.user_api.accounts import USERNAME_MAX_LENGTH

# Test with a very long name
long_name = "John" * 50
suggestions = generate_username_suggestions(long_name)

for suggestion in suggestions:
assert len(suggestion) <= USERNAME_MAX_LENGTH
29 changes: 26 additions & 3 deletions openedx/core/djangoapps/user_authn/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,12 +72,35 @@ def is_registration_api_v1(request):
return 'v1' in request.get_full_path() and 'register' not in request.get_full_path()


def remove_special_characters_from_name(name):
def remove_special_characters_from_name(name: str) -> str:
return "".join(re.findall(r"[\w-]+", name))


def generate_username_suggestions(name):
""" Generate 3 available username suggestions """
def generate_username_suggestions(name: str) -> list[str] | list:
"""
Generate 3 available username suggestions based on the provided name.

Args:
name (str): The full name to generate username suggestions from.
Must contain only ASCII characters.

Returns:
list[str] | list: A list of up to 3 available username suggestions,
or an empty list if name contains non-ASCII characters or if no valid
suggestions could be generated.

Note:
Generated usernames will be combinations of:
- firstname + lastname
- first initial + lastname
- firstname + random number
"""
# Check if name contains non-ASCII characters
try:
name.encode('ascii')
except UnicodeEncodeError:
return []

username_suggestions = []
max_length = USERNAME_MAX_LENGTH
names = name.split(' ')
Expand Down
2 changes: 1 addition & 1 deletion openedx/core/djangoapps/user_authn/views/login.py
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ def _enforce_password_policy_compliance(request, user): # lint-amnesty, pylint:
# Allow login, but warn the user that they will be required to reset their password soon.
PageLevelMessages.register_warning_message(request, HTML(str(e)))
except password_policy_compliance.NonCompliantPasswordException as e:
# Increment the lockout counter to safguard from further brute force requests
# Increment the lockout counter to safeguard from further brute force requests
# if user's password has been compromised.
if LoginFailures.is_feature_enabled():
LoginFailures.increment_lockout_counter(user)
Expand Down
Loading