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

Change isnumeric to isdigit in NumericValidator #376

Merged
merged 1 commit into from
Jun 11, 2020

Conversation

imhassantariq
Copy link
Member

Issue:

isnumeric() only works on Unicode characters. If we run the command changepassword:

/edx/app/edxapp/edx-platform/manage.py lms changepassword

It gives us error:

Changing password for user 'test'
Password: 
Password (again): 
Traceback (most recent call last):
  File "manage.py", line 123, in <module>
    execute_from_command_line([sys.argv[0]] + django_args)
  File "/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/django/core/management/__init__.py", line 364, in execute_from_command_line
    utility.execute()
  File "/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/django/core/management/__init__.py", line 356, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/django/core/management/base.py", line 283, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/django/core/management/base.py", line 330, in execute
    output = self.handle(*args, **options)
  File "/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/django/contrib/auth/management/commands/changepassword.py", line 65, in handle
    validate_password(p2, u)
  File "/edx/app/edxapp/venvs/edxapp/local/lib/python2.7/site-packages/django/contrib/auth/password_validation.py", line 52, in validate_password
    validator.validate(password, user)
  File "/edx/app/edxapp/edx-platform/common/djangoapps/util/password_policy_validators.py", line 260, in validate
    if _validate_condition(password, lambda c: c.isnumeric(), self.min_numeric):
  File "/edx/app/edxapp/edx-platform/common/djangoapps/util/password_policy_validators.py", line 145, in _validate_condition
    valid_count = len([c for c in password if fn(c)])
  File "/edx/app/edxapp/edx-platform/common/djangoapps/util/password_policy_validators.py", line 260, in <lambda>
    if _validate_condition(password, lambda c: c.isnumeric(), self.min_numeric):
AttributeError: 'str' object has no attribute 'isnumeric'

We can use isdigit() it works on both str and Unicode. Django itself use isdigit() in it's NumericValidator. Reference Link

@@ -256,7 +256,7 @@ def __init__(self, min_numeric=0):
self.min_numeric = min_numeric

def validate(self, password, user=None):
if _validate_condition(password, lambda c: c.isnumeric(), self.min_numeric):
if _validate_condition(password, lambda c: c.isdigit(), self.min_numeric):

Choose a reason for hiding this comment

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

but are both of these actually the same things ?

Choose a reason for hiding this comment

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

Since we are changing the edx's code, we should make sure that we are doing exactly what we want.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here we are only referring to the NumericValidator that checks if the digit is number or not. So, yes isdigit() and isnumeric() both serve the same purpose here. I have added a Django code Reference link in PR description Django itself uses isdigit().

If you have anything else in mind, let me know I will try that use case.

Copy link
Member Author

Choose a reason for hiding this comment

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

According to this documentation: Link

They can work differently in some scenarios like different languages or Unicode numeric characters.

Choose a reason for hiding this comment

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

Then i think everything's cool.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are no test failures:

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Booooyaah 😃

@imhassantariq imhassantariq merged commit 8c76fa1 into develop Jun 11, 2020
@pjamason
Copy link

thanks for your quick work on this! one other error i ran into was the following in PunctuationValidator class (original and new code below):

#if _validate_condition(password, lambda c: ‘P’ in unicodedata.category(c), self.min_punctuation):
if _validate_condition(unicode(password), lambda c: ‘P’ in unicodedata.category(c), self.min_punctuation):

@imhassantariq imhassantariq deleted the EDE-586/fix_changepassword_command branch June 12, 2020 10:02
This was referenced Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants