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.

Sign up for GitHub

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

Jump to bottom

Added Password Validators #358

Merged
merged 1 commit into from
May 5, 2020
Merged

Added Password Validators #358

merged 1 commit into from
May 5, 2020

Conversation

imhassantariq
Copy link
Member

@imhassantariq imhassantariq commented May 4, 2020 •
edited
Loading

Story Link

Review Password Requirements

PR Description

Added two of the password validators:

  • MasterPasswordValidator
  • RestrictedSymbolValidator

Type of change

Please select the options that are relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Change

How to test?

For testing this ticket you need to set the value of your AUTH_PASSWORD_VALIDATORS (located in lms.env.json) like this:

"AUTH_PASSWORD_VALIDATORS":[
    {
        "NAME":"util.password_policy_validators.MaximumLengthValidator",
        "OPTIONS":{
            "max_length":75
        }
    },
    {
        "NAME":"util.password_policy_validators.MinimumLengthValidator",
        "OPTIONS":{
            "min_length":8
        }
    },
    {
        "NAME":"openedx.features.ucsd_features.validators.RestrictedSymbolValidator",
        "OPTIONS":{
            "restricted_symbol_list":["$", "'", "!", ":", " "]
        }
    },
    {
        "NAME":"openedx.features.ucsd_features.validators.MasterPasswordValidator",
        "OPTIONS":{
            "min_validations":3,
            "validators":[
                {
                    "NAME":"util.password_policy_validators.LowercaseValidator",
                    "OPTIONS":{
                        "min_lower":1
                    }
                },
                {
                    "NAME":"util.password_policy_validators.UppercaseValidator",
                    "OPTIONS":{
                        "min_upper":1
                    }
                },
                {
                    "NAME":"util.password_policy_validators.NumericValidator",
                    "OPTIONS":{
                        "min_numeric":1
                    }
                },
                {
                    "NAME":"django.contrib.auth.password_validation.UserAttributeSimilarityValidator"
                },
                {
                    "NAME":"util.password_policy_validators.PunctuationValidator",
                    "OPTIONS":{
                        "min_punctuation":1
                    }
                }
            ]
        }
    }
]

Checklist before merging:

  • Squased
  • Reviewd

@imhassantariq imhassantariq self-assigned this May 4, 2020
@imhassantariq imhassantariq force-pushed the EDE-384/password_requirements_feature branch from e32deda to 48f3de6 Compare May 4, 2020 19:49
@edtecheco
Copy link

Your PR has finished running tests. There were no failures.

danialmalik
danialmalik suggested changes May 5, 2020
View reviewed changes
openedx/features/ucsd_features/validators.py Outdated

def get_help_text(self):
text = ungettext(
"your password must contain at least %(min_validations)d of the following:",
Copy link

Choose a reason for hiding this comment

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

Suggested change
"your password must contain at least %(min_validations)d of the following:",
"your password must conform to at least %(min_validations)d of the following:",

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

openedx/features/ucsd_features/validators.py Outdated
Comment on lines 73 to 74
if len(validators) >= min_validations >= 0:
return
elif len(validators) < min_validations:
raise Exception('Number of Validators in list is lower the the minimum number of required validations.')
Copy link

Choose a reason for hiding this comment

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

Suggested change
if len(validators) >= min_validations >= 0:
return
elif len(validators) < min_validations:
raise Exception('Number of Validators in list is lower the the minimum number of required validations.')
if len(validators) < min_validations:
raise Exception('Number of Validators in list is lower the the minimum number of required validations.')

From what I understand from the code, we only need a single if block.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

openedx/features/ucsd_features/validators.py Outdated

def get_help_text(self):
return _(
"Your password must not contain any of the following symbol: {}".format(self.restricted_symbol_text))
Copy link

Choose a reason for hiding this comment

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

Suggested change
"Your password must not contain any of the following symbol: {}".format(self.restricted_symbol_text))
"Your password must not contain any of the following symbols: {}".format(self.restricted_symbol_text))

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

openedx/features/ucsd_features/validators.py Outdated
Comment on lines 106 to 105
else:
return ""
Copy link

Choose a reason for hiding this comment

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

Suggested change
else:
return ""
return ""

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link

Choose a reason for hiding this comment

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

Don't think so 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@imhassantariq imhassantariq force-pushed the EDE-384/password_requirements_feature branch from 48f3de6 to 45ca464 Compare May 5, 2020 05:59
@imhassantariq imhassantariq requested a review from danialmalik May 5, 2020 06:00
HamzaIbnFarooq
HamzaIbnFarooq reviewed May 5, 2020
View reviewed changes
openedx/features/ucsd_features/validators.py Outdated
) % {"min_validations": self.min_validations}
for validator in self.validators:
if hasattr(validator, 'get_instruction_text'):
text += validator.get_instruction_text() + ","
Copy link

@HamzaIbnFarooq HamzaIbnFarooq May 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

nit: for multiple lines

Suggested change
text += validator.get_instruction_text() + ","
text += validator.get_instruction_text() + ", "

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

tehreem-sadat
tehreem-sadat reviewed May 5, 2020
View reviewed changes
openedx/features/ucsd_features/validators.py

def get_instruction_text(self):
text = ungettext(
"your password must confirm to at least %(min_validations)d of the following:",
Copy link

Choose a reason for hiding this comment

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

Repeated lines (62 and 63).

Copy link
Member Author

Choose a reason for hiding this comment

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

Singular and Plural at the end following and followings. Reference: https://github.com/ucsd-ets/edx-platform/blob/develop/common/djangoapps/util/password_policy_validators.py#L177

HamzaIbnFarooq
HamzaIbnFarooq reviewed May 5, 2020
View reviewed changes
openedx/features/ucsd_features/validators.py Outdated
Comment on lines 114 to 122
if len(self.restricted_symbol_list) > 0:
text = ""
for c in self.restricted_symbol_list:
if c.isspace():
text += " Space"
else:
text += " " + c
return text
return ""
Copy link

Choose a reason for hiding this comment

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

Suggested change
if len(self.restricted_symbol_list) > 0:
text = ""
for c in self.restricted_symbol_list:
if c.isspace():
text += " Space"
else:
text += " " + c
return text
return ""
text = ""
for c in self.restricted_symbol_list:
if c.isspace():
text += " Space"
else:
text += " " + c
return text

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@imhassantariq imhassantariq force-pushed the EDE-384/password_requirements_feature branch from 45ca464 to 36f833b Compare May 5, 2020 06:10
@imhassantariq imhassantariq force-pushed the EDE-384/password_requirements_feature branch from 36f833b to 8fcdce9 Compare May 5, 2020 06:14
danialmalik
danialmalik reviewed May 5, 2020
View reviewed changes
Copy link

@danialmalik danialmalik left a comment

Choose a reason for hiding this comment

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

Just a few minor things.

openedx/features/ucsd_features/validators.py Outdated
"""
def __init__(self, validators=[], min_validations=0):
self.check_validators_list(validators, min_validations)
self.validators_list = validators
Copy link

Choose a reason for hiding this comment

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

If we are not going to use this list anywhere, I don't think we need to save this list as self attribute?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

openedx/features/ucsd_features/validators.py Outdated
try:
validator_name = validator["NAME"]
except KeyError:
raise KeyError("Validator doesn't contains 'NAME'. Please enter valid Validator")
Copy link

Choose a reason for hiding this comment

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

Suggested change
raise KeyError("Validator doesn't contains 'NAME'. Please enter valid Validator")
raise KeyError("Validator doesn't contain 'NAME'. Please enter valid Validator")

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

openedx/features/ucsd_features/validators.py Outdated
Comment on lines 106 to 105
else:
return ""
Copy link

Choose a reason for hiding this comment

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

Don't think so 😅

@imhassantariq imhassantariq force-pushed the EDE-384/password_requirements_feature branch 2 times, most recently from 75bae45 to f19d271 Compare May 5, 2020 06:29
@imhassantariq imhassantariq force-pushed the EDE-384/password_requirements_feature branch from f19d271 to 354edd4 Compare May 5, 2020 06:32
@edtecheco
Copy link

Your PR has finished running tests. The following contexts failed:

  • jenkins/python

@imhassantariq
Copy link
Member Author

jenkins run all

@edtecheco
Copy link

Your PR has finished running tests. The following contexts failed:

  • jenkins/python

@imhassantariq
Copy link
Member Author

jenkins run all

@edtecheco
Copy link

Your PR has finished running tests. The following contexts failed:

  • jenkins/python

@imhassantariq
Copy link
Member Author

jenkins run all

@edtecheco
Copy link

Your PR has finished running tests. The following contexts failed:

  • jenkins/python

@imhassantariq imhassantariq merged commit 2da1016 into develop May 5, 2020
This was referenced May 5, 2020
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HamzaIbnFarooq HamzaIbnFarooq HamzaIbnFarooq approved these changes

@tehreem-sadat tehreem-sadat tehreem-sadat approved these changes

@danialmalik danialmalik danialmalik approved these changes

@mumarkhan999 mumarkhan999 Awaiting requested review from mumarkhan999

@tasawernawaz tasawernawaz Awaiting requested review from tasawernawaz

Assignees

@imhassantariq imhassantariq

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@imhassantariq @edtecheco @danialmalik @tehreem-sadat @HamzaIbnFarooq