From 4fa27f98dcc22939fce96ed34477dbcf6037e20b Mon Sep 17 00:00:00 2001 From: Dillon Dumesnil Date: Thu, 13 Sep 2018 15:28:21 -0400 Subject: [PATCH 1/2] Implementing django password validators for edX. This involves removing the old validate password method and configuration values in favor of AUTH_PASSWORD_VALIDATORS, a list of validators to use to check a password. These include some that come straight from Django and some that were written according to Django's specifications. This work also included maintaining the current messaging as instruction text and passing along restrictions for the password field. --- cms/envs/aws.py | 6 +- common/djangoapps/student/forms.py | 22 +- common/djangoapps/student/views/management.py | 4 +- .../util/password_policy_validators.py | 672 +++++++++++------- lms/envs/aws.py | 6 +- lms/envs/devstack.py | 3 - .../djangoapps/password_policy/compliance.py | 4 +- .../core/djangoapps/user_api/accounts/api.py | 22 +- openedx/core/djangoapps/user_api/api.py | 23 +- openedx/core/djangoapps/user_api/helpers.py | 4 +- .../djangoapps/user_api/validation/views.py | 7 +- 11 files changed, 441 insertions(+), 332 deletions(-) diff --git a/cms/envs/aws.py b/cms/envs/aws.py index 72f8d0ca7c..25d646ed1e 100644 --- a/cms/envs/aws.py +++ b/cms/envs/aws.py @@ -447,11 +447,7 @@ MAX_FAILED_LOGIN_ATTEMPTS_ALLOWED = ENV_TOKENS.get("MAX_FAILED_LOGIN_ATTEMPTS_AL MAX_FAILED_LOGIN_ATTEMPTS_LOCKOUT_PERIOD_SECS = ENV_TOKENS.get("MAX_FAILED_LOGIN_ATTEMPTS_LOCKOUT_PERIOD_SECS", 15 * 60) #### PASSWORD POLICY SETTINGS ##### -PASSWORD_MIN_LENGTH = ENV_TOKENS.get("PASSWORD_MIN_LENGTH") -PASSWORD_MAX_LENGTH = ENV_TOKENS.get("PASSWORD_MAX_LENGTH") -PASSWORD_COMPLEXITY = ENV_TOKENS.get("PASSWORD_COMPLEXITY", {}) -PASSWORD_DICTIONARY_EDIT_DISTANCE_THRESHOLD = ENV_TOKENS.get("PASSWORD_DICTIONARY_EDIT_DISTANCE_THRESHOLD") -PASSWORD_DICTIONARY = ENV_TOKENS.get("PASSWORD_DICTIONARY", []) +AUTH_PASSWORD_VALIDATORS = ENV_TOKENS.get("AUTH_PASSWORD_VALIDATORS", []) ### INACTIVITY SETTINGS #### SESSION_INACTIVITY_TIMEOUT_IN_SECONDS = AUTH_TOKENS.get("SESSION_INACTIVITY_TIMEOUT_IN_SECONDS") diff --git a/common/djangoapps/student/forms.py b/common/djangoapps/student/forms.py index 8ca0a6f4c9..c43b10ca49 100644 --- a/common/djangoapps/student/forms.py +++ b/common/djangoapps/student/forms.py @@ -27,7 +27,7 @@ from openedx.core.djangoapps.user_api import accounts as accounts_settings from openedx.core.djangoapps.user_api.preferences.api import get_user_preference from student.message_types import PasswordReset from student.models import CourseEnrollmentAllowed, email_exists_or_retired -from util.password_policy_validators import password_max_length, password_min_length, validate_password +from util.password_policy_validators import edX_validate_password def send_password_reset_email_for_user(user, request): @@ -193,7 +193,6 @@ class AccountCreationForm(forms.Form): """ _EMAIL_INVALID_MSG = _("A properly formatted e-mail is required") - _PASSWORD_INVALID_MSG = _("A valid password is required") _NAME_TOO_SHORT_MSG = _("Your legal name must be a minimum of two characters long") # TODO: Resolve repetition @@ -209,15 +208,9 @@ class AccountCreationForm(forms.Form): "max_length": _("Email cannot be more than %(limit_value)s characters long"), } ) - password = forms.CharField( - min_length=password_min_length(), - max_length=password_max_length(), - error_messages={ - "required": _PASSWORD_INVALID_MSG, - "min_length": _PASSWORD_INVALID_MSG, - "max_length": _PASSWORD_INVALID_MSG, - } - ) + + password = forms.CharField() + name = forms.CharField( min_length=accounts_settings.NAME_MIN_LENGTH, error_messages={ @@ -288,7 +281,12 @@ class AccountCreationForm(forms.Form): """Enforce password policies (if applicable)""" password = self.cleaned_data["password"] if self.enforce_password_policy: - validate_password(password, username=self.cleaned_data.get('username')) + # Creating a temporary user object to test password against username + # This user should NOT be saved + username = self.cleaned_data.get('username') + email = self.cleaned_data.get('email') + temp_user = User(username=username, email=email) if username else None + edX_validate_password(password, temp_user) return password def clean_email(self): diff --git a/common/djangoapps/student/views/management.py b/common/djangoapps/student/views/management.py index e01f0fb2a2..47ce54bf30 100644 --- a/common/djangoapps/student/views/management.py +++ b/common/djangoapps/student/views/management.py @@ -94,7 +94,7 @@ from student.text_me_the_app import TextMeTheAppFragmentView from util.bad_request_rate_limiter import BadRequestRateLimiter from util.db import outer_atomic from util.json_request import JsonResponse -from util.password_policy_validators import SecurityPolicyError, validate_password +from util.password_policy_validators import edX_validate_password log = logging.getLogger("edx.student") @@ -830,7 +830,7 @@ def password_reset_confirm_wrapper(request, uidb36=None, token=None): password = request.POST['new_password1'] try: - validate_password(password, user=user) + edX_validate_password(password, user=user) except ValidationError as err: # We have a password reset attempt which violates some security # policy, or any other validation. Use the existing Django template to communicate that diff --git a/common/djangoapps/util/password_policy_validators.py b/common/djangoapps/util/password_policy_validators.py index d7c0138fb3..ded8d0040d 100644 --- a/common/djangoapps/util/password_policy_validators.py +++ b/common/djangoapps/util/password_policy_validators.py @@ -1,322 +1,450 @@ """ -This file exposes a number of password complexity validators which can be optionally added to +This file exposes a number of password validators which can be optionally added to account creation - -This file was inspired by the django-passwords project at https://github.com/dstufft/django-passwords -authored by dstufft (https://github.com/dstufft) """ -from __future__ import division +from __future__ import unicode_literals import logging -import string import unicodedata from django.conf import settings +from django.contrib.auth.password_validation import ( + get_default_password_validators, + validate_password, + MinimumLengthValidator as DjangoMinimumLengthValidator, +) from django.core.exceptions import ValidationError -from django.utils.translation import ugettext_lazy as _ -from django.utils.translation import ungettext_lazy as ungettext -from Levenshtein import distance +from django.utils.translation import ugettext as _, ungettext from six import text_type from student.models import PasswordHistory - log = logging.getLogger(__name__) -# In description order -_allowed_password_complexity = [ - 'ALPHABETIC', - 'UPPER', - 'LOWER', - 'NUMERIC', - 'DIGITS', - 'PUNCTUATION', - 'NON ASCII', - 'WORDS', -] - -class SecurityPolicyError(ValidationError): - pass - - -def password_min_length(): +def password_validators_instruction_texts(): """ - Returns minimum required length of a password. - Can be overridden by site configuration of PASSWORD_MIN_LENGTH. + Return a string of instruction texts of all configured validators. + Expects at least the MinimumLengthValidator to be defined. """ - min_length = getattr(settings, 'PASSWORD_MIN_LENGTH', None) - if min_length is None: - return 2 # Note: This default is simply historical - return min_length - - -def password_max_length(): - """ - Returns maximum allowed length of a password. If zero, no maximum. - Can be overridden by site configuration of PASSWORD_MAX_LENGTH. - """ - # Note: The default value here is simply historical - max_length = getattr(settings, 'PASSWORD_MAX_LENGTH', None) - if max_length is None: - return 75 # Note: This default is simply historical - return max_length - - -def password_complexity(): - """ - :return: A dict of complexity requirements from settings - """ - complexity = {} - if settings.FEATURES.get('ENFORCE_PASSWORD_POLICY', False): - complexity = getattr(settings, 'PASSWORD_COMPLEXITY', {}) - - valid_complexity = {x: y for x, y in complexity.iteritems() if x in _allowed_password_complexity} - - if not password_complexity.logged: - invalid = frozenset(complexity.keys()) - frozenset(valid_complexity.keys()) - for key in invalid: - log.warning('Unrecognized %s value in PASSWORD_COMPLEXITY setting.', key) - password_complexity.logged = True - - return valid_complexity - - -# Declare static variable for the function above, which helps avoid issuing multiple log warnings. -# We don't instead keep a cached version of the complexity rules, because that might trip up unit tests. -password_complexity.logged = False - - -def _password_complexity_descriptions(which=None): - """ - which: A list of which complexities to describe, None if you want the configured ones - :return: A list of complexity descriptions - """ - descs = [] - complexity = password_complexity() - if which is None: - which = complexity.keys() - - for key in _allowed_password_complexity: # we iterate over allowed keys so that we get the order right - value = complexity.get(key, 0) if key in which else 0 - if not value: - continue - - if key == 'ALPHABETIC': - # Translators: This appears in a list of password requirements - descs.append(ungettext('{num} letter', '{num} letters', value).format(num=value)) - elif key == 'UPPER': - # Translators: This appears in a list of password requirements - descs.append(ungettext('{num} uppercase letter', '{num} uppercase letters', value).format(num=value)) - elif key == 'LOWER': - # Translators: This appears in a list of password requirements - descs.append(ungettext('{num} lowercase letter', '{num} lowercase letters', value).format(num=value)) - elif key == 'DIGITS': - # Translators: This appears in a list of password requirements - descs.append(ungettext('{num} digit', '{num} digits', value).format(num=value)) - elif key == 'NUMERIC': - # Translators: This appears in a list of password requirements - descs.append(ungettext('{num} number', '{num} numbers', value).format(num=value)) - elif key == 'PUNCTUATION': - # Translators: This appears in a list of password requirements - descs.append(ungettext('{num} punctuation mark', '{num} punctuation marks', value).format(num=value)) - elif key == 'NON ASCII': # note that our definition of non-ascii is non-letter, non-digit, non-punctuation - # Translators: This appears in a list of password requirements - descs.append(ungettext('{num} symbol', '{num} symbols', value).format(num=value)) - elif key == 'WORDS': - # Translators: This appears in a list of password requirements - descs.append(ungettext('{num} word', '{num} words', value).format(num=value)) - else: - raise Exception('Unexpected complexity value {}'.format(key)) - - return descs - - -def password_instructions(): - """ - :return: A string suitable for display to the user to tell them what password to enter - """ - min_length = password_min_length() - reqs = _password_complexity_descriptions() - - if not reqs: - return ungettext('Your password must contain at least {num} character.', - 'Your password must contain at least {num} characters.', - min_length).format(num=min_length) + complexity_instructions = [] + # For clarity in the printed instructions, the minimum length instruction + # is separated from the complexity instructions. + length_instruction = '' + password_validators = get_default_password_validators() + for validator in password_validators: + if hasattr(validator, 'get_instruction_text'): + text = validator.get_instruction_text() + if isinstance(validator, MinimumLengthValidator): + length_instruction = text + else: + complexity_instructions.append(text) + if complexity_instructions: + return _('Your password must contain {length_instruction}, including {complexity_instructions}.').format( + length_instruction=length_instruction, + complexity_instructions=' & '.join(complexity_instructions) + ) else: - return ungettext('Your password must contain at least {num} character, including {requirements}.', - 'Your password must contain at least {num} characters, including {requirements}.', - min_length).format(num=min_length, requirements=' & '.join(reqs)) + return _('Your password must contain {length_instruction}.'.format(length_instruction=length_instruction)) -def validate_password(password, user=None, username=None, password_reset=True): +def password_validators_restrictions(): """ - Checks user-provided password against our current site policy. + Return a dictionary of complexity restrictions to be used by mobile users on + the registration form + """ + password_validators = get_default_password_validators() + complexity_restrictions = dict(validator.get_restriction() + for validator in password_validators + if hasattr(validator, 'get_restriction') + ) + return complexity_restrictions - Raises a ValidationError or SecurityPolicyError depending on the type of error. - Arguments: - password: The user-provided password as a string - user: A User model object, if available. Required to check against security policy. - username: The user-provided username, if available. Taken from 'user' if not provided. - password_reset: Whether to run validators that only make sense in a password reset - context (like PasswordHistory). +def edX_validate_password(password, user=None): + """ + EdX's custom password validator for passwords. This function performs the + following functions: + 1) Converts the password to unicode if it is not already + 2) Calls Django's validate_password method. This calls the validate function + in all validators specified in AUTH_PASSWORD_VALIDATORS configuration. + + Parameters: + password (str or unicode): the user's password to be validated + user (django.contrib.auth.models.User): The user object to use for validating + the given password against the username and/or email. + + Returns: + None + + Raises: + ValidationError if unable to convert password to utf8 or if any of the + password validators fail. """ if not isinstance(password, text_type): try: - password = text_type(password, encoding='utf8') # some checks rely on unicode semantics (e.g. length) + # some checks rely on unicode semantics (e.g. length) + password = text_type(password, encoding='utf8') except UnicodeDecodeError: - raise ValidationError(_('Invalid password.')) # no reason to get into weeds + # no reason to get into weeds + raise ValidationError([_('Invalid password.')]) - username = username or (user and user.username) - - if user and password_reset: - _validate_password_security(password, user) - - _validate_password_dictionary(password) - _validate_password_against_username(password, username) - - # Some messages are composable, so we'll add them together here - errors = [_validate_password_length(password)] - errors += _validate_password_complexity(password) - errors = filter(None, errors) - - if errors: - msg = _('Enter a password with at least {requirements}.').format(requirements=' & '.join(errors)) - raise ValidationError(msg) + validate_password(password, user) -def _validate_password_security(password, user): +def _validate_condition(password, fn, min_count): """ - Check password reuse and similar operational security policy considerations. + Validates the password using the given function. This is performed by + iterating through each character in the password and counting up the number + of characters that satisfy the function. + + Parameters: + password (str): the password + fn: the function to be tested against the string. + min_count (int): the minimum number of characters that must satisfy the function + + Return: + True if valid_count >= min_count, else False """ - # Check reuse - if not PasswordHistory.is_allowable_password_reuse(user, password): - if user.is_staff: - num_distinct = settings.ADVANCED_SECURITY_CONFIG['MIN_DIFFERENT_STAFF_PASSWORDS_BEFORE_REUSE'] - else: - num_distinct = settings.ADVANCED_SECURITY_CONFIG['MIN_DIFFERENT_STUDENT_PASSWORDS_BEFORE_REUSE'] - raise SecurityPolicyError(ungettext( - "You are re-using a password that you have used recently. " - "You must have {num} distinct password before reusing a previous password.", - "You are re-using a password that you have used recently. " - "You must have {num} distinct passwords before reusing a previous password.", - num_distinct - ).format(num=num_distinct)) - - # Check reset frequency - if PasswordHistory.is_password_reset_too_soon(user): - num_days = settings.ADVANCED_SECURITY_CONFIG['MIN_TIME_IN_DAYS_BETWEEN_ALLOWED_RESETS'] - raise SecurityPolicyError(ungettext( - "You are resetting passwords too frequently. Due to security policies, " - "{num} day must elapse between password resets.", - "You are resetting passwords too frequently. Due to security policies, " - "{num} days must elapse between password resets.", - num_days - ).format(num=num_days)) + valid_count = len([c for c in password if fn(c)]) + return valid_count >= min_count -def _validate_password_length(value): - """ - Validator that enforces minimum length of a password - """ - min_length = password_min_length() - max_length = password_max_length() - - if min_length and len(value) < min_length: - # This is an error that can be composed with other requirements, so just return a fragment - # Translators: This appears in a list of password requirements +class MinimumLengthValidator(DjangoMinimumLengthValidator): + def get_instruction_text(self): return ungettext( - "{num} character", - "{num} characters", - min_length - ).format(num=min_length) - elif max_length and len(value) > max_length: - raise ValidationError(ungettext( - "Enter a password with at most {num} character.", - "Enter a password with at most {num} characters.", - max_length - ).format(num=max_length)) + 'at least %(min_length)d character', + 'at least %(min_length)d characters', + self.min_length + ) % {'min_length': self.min_length} + + def get_restriction(self): + """ + Returns a key, value pair for the restrictions related to the Validator + """ + return 'min_length', self.min_length -def _validate_password_complexity(value): +class MaximumLengthValidator(object): """ - Validator that enforces minimum complexity + Validate whether the password is shorter than a maximum length. + + Parameters: + max_length (int): the maximum number of characters to require in the password. """ - complexities = password_complexity() - if not complexities: - return [] + def __init__(self, max_length=75): + self.max_length = max_length - # Sets are here intentionally - uppercase, lowercase, digits, non_ascii, punctuation = set(), set(), set(), set(), set() - alphabetic, numeric = [], [] + def validate(self, password, user=None): + if len(password) > self.max_length: + raise ValidationError( + ungettext( + 'This password is too long. It must contain no more than %(max_length)d character.', + 'This password is too long. It must contain no more than %(max_length)d characters.', + self.max_length + ), + code='password_too_long', + params={'max_length': self.max_length}, + ) - for character in value: - if character.isupper(): - uppercase.add(character) - elif character.islower(): - lowercase.add(character) - elif character.isdigit(): - digits.add(character) - elif character in string.punctuation: - punctuation.add(character) + def get_help_text(self): + return ungettext( + 'Your password must contain no more than %(max_length)d character.', + 'Your password must contain no more than %(max_length)d characters.', + self.max_length + ) % {'max_length': self.max_length} + + def get_restriction(self): + """ + Returns a key, value pair for the restrictions related to the Validator + """ + return 'max_length', self.max_length + + +class AlphabeticValidator(object): + """ + Validate whether the password contains at least min_alphabetic letters. + + Parameters: + min_alphabetic (int): the minimum number of alphabetic characters to require + in the password. Must be >= 0. + """ + def __init__(self, min_alphabetic=0): + self.min_alphabetic = min_alphabetic + + def validate(self, password, user=None): + if _validate_condition(password, lambda c: c.isalpha(), self.min_alphabetic): + return + raise ValidationError( + ungettext( + 'Your password must contain at least %(min_alphabetic)d letter.', + 'Your password must contain at least %(min_alphabetic)d letters.', + self.min_alphabetic + ), + code='too_few_alphabetic_char', + params={'min_alphabetic': self.min_alphabetic}, + ) + + def get_help_text(self): + return ungettext( + 'Your password must contain at least %(min_alphabetic)d letter.', + 'Your password must contain at least %(min_alphabetic)d letters.', + self.min_alphabetic + ) % {'min_alphabetic': self.min_alphabetic} + + def get_instruction_text(self): + if self.min_alphabetic > 0: + return ungettext( + '%(num)d letter', + '%(num)d letters', + self.min_alphabetic + ) % {'num': self.min_alphabetic} else: - non_ascii.add(character) + return '' - if character.isalpha(): - alphabetic.append(character) - if 'N' in unicodedata.category(character): # Check to see if the unicode category contains a 'N'umber - numeric.append(character) - - words = set(value.split()) - - errors = [] - if len(uppercase) < complexities.get("UPPER", 0): - errors.append('UPPER') - if len(lowercase) < complexities.get("LOWER", 0): - errors.append('LOWER') - if len(digits) < complexities.get("DIGITS", 0): - errors.append('DIGITS') - if len(punctuation) < complexities.get("PUNCTUATION", 0): - errors.append('PUNCTUATION') - if len(non_ascii) < complexities.get("NON ASCII", 0): - errors.append('NON ASCII') - if len(words) < complexities.get("WORDS", 0): - errors.append('WORDS') - if len(numeric) < complexities.get("NUMERIC", 0): - errors.append('NUMERIC') - if len(alphabetic) < complexities.get("ALPHABETIC", 0): - errors.append('ALPHABETIC') - - if errors: - return _password_complexity_descriptions(errors) - else: - return [] + def get_restriction(self): + """ + Returns a key, value pair for the restrictions related to the Validator + """ + return 'min_alphabetic', self.min_alphabetic -def _validate_password_against_username(password, username): - if not username: - return - - if password == username: - # Translators: This message is shown to users who enter a password matching - # the username they enter(ed). - raise ValidationError(_(u"Password cannot be the same as the username.")) - - -def _validate_password_dictionary(value): +class NumericValidator(object): """ - Insures that the password is not too similar to a defined set of dictionary words + Validate whether the password contains at least min_numeric numbers. + + Parameters: + min_numeric (int): the minimum number of numeric characters to require + in the password. Must be >= 0. """ - if not settings.FEATURES.get('ENFORCE_PASSWORD_POLICY', False): - return + def __init__(self, min_numeric=0): + self.min_numeric = min_numeric - password_max_edit_distance = getattr(settings, "PASSWORD_DICTIONARY_EDIT_DISTANCE_THRESHOLD", None) - password_dictionary = getattr(settings, "PASSWORD_DICTIONARY", None) + def validate(self, password, user=None): + if _validate_condition(password, lambda c: c.isnumeric(), self.min_numeric): + return + raise ValidationError( + ungettext( + 'Your password must contain at least %(min_numeric)d number.', + 'Your password must contain at least %(min_numeric)d numbers.', + self.min_numeric + ), + code='too_few_numeric_char', + params={'min_numeric': self.min_numeric}, + ) - if password_max_edit_distance and password_dictionary: - for word in password_dictionary: - edit_distance = distance(value, text_type(word)) - if edit_distance <= password_max_edit_distance: - raise ValidationError(_("Password is too similar to a dictionary word."), - code="dictionary_word") + def get_help_text(self): + return ungettext( + "Your password must contain at least %(min_numeric)d number.", + "Your password must contain at least %(min_numeric)d numbers.", + self.min_numeric + ) % {'min_numeric': self.min_numeric} + + def get_instruction_text(self): + if self.min_numeric > 0: + return ungettext( + '%(num)d number', + '%(num)d numbers', + self.min_numeric + ) % {'num': self.min_numeric} + else: + return '' + + def get_restriction(self): + """ + Returns a key, value pair for the restrictions related to the Validator + """ + return 'min_numeric', self.min_numeric + + +class UppercaseValidator(object): + """ + Validate whether the password contains at least min_upper uppercase letters. + + Parameters: + min_upper (int): the minimum number of uppercase characters to require + in the password. Must be >= 0. + """ + def __init__(self, min_upper=0): + self.min_upper = min_upper + + def validate(self, password, user=None): + if _validate_condition(password, lambda c: c.isupper(), self.min_upper): + return + raise ValidationError( + ungettext( + 'Your password must contain at least %(min_upper)d uppercase letter.', + 'Your password must contain at least %(min_upper)d uppercase letters.', + self.min_upper + ), + code='too_few_uppercase_char', + params={'min_upper': self.min_upper}, + ) + + def get_help_text(self): + return ungettext( + "Your password must contain at least %(min_upper)d uppercase letter.", + "Your password must contain at least %(min_upper)d uppercase letters.", + self.min_upper + ) % {'min_upper': self.min_upper} + + def get_instruction_text(self): + if self.min_upper > 0: + return ungettext( + '%(num)d uppercase letter', + '%(num)d uppercase letters', + self.min_upper + ) % {'num': self.min_upper} + else: + return '' + + def get_restriction(self): + """ + Returns a key, value pair for the restrictions related to the Validator + """ + return 'min_upper', self.min_upper + + +class LowercaseValidator(object): + """ + Validate whether the password contains at least min_lower lowercase letters. + + Parameters: + min_lower (int): the minimum number of lowercase characters to require + in the password. Must be >= 0. + """ + def __init__(self, min_lower=0): + self.min_lower = min_lower + + def validate(self, password, user=None): + if _validate_condition(password, lambda c: c.islower(), self.min_lower): + return + raise ValidationError( + ungettext( + 'Your password must contain at least %(min_lower)d lowercase letter.', + 'Your password must contain at least %(min_lower)d lowercase letters.', + self.min_lower + ), + code='too_few_lowercase_char', + params={'min_lower': self.min_lower}, + ) + + def get_help_text(self): + return ungettext( + "Your password must contain at least %(min_lower)d lowercase letter.", + "Your password must contain at least %(min_lower)d lowercase letters.", + self.min_lower + ) % {'min_lower': self.min_lower} + + def get_instruction_text(self): + if self.min_lower > 0: + return ungettext( + '%(num)d lowercase letter', + '%(num)d lowercase letters', + self.min_lower + ) % {'num': self.min_lower} + else: + return '' + + def get_restriction(self): + """ + Returns a key, value pair for the restrictions related to the Validator + """ + return 'min_lower', self.min_lower + + +class PunctuationValidator(object): + """ + Validate whether the password contains at least min_punctuation punctuation characters + as defined by unicode categories. + + Parameters: + min_punctuation (int): the minimum number of punctuation characters to require + in the password. Must be >= 0. + """ + def __init__(self, min_punctuation=0): + self.min_punctuation = min_punctuation + + def validate(self, password, user=None): + if _validate_condition(password, lambda c: 'P' in unicodedata.category(c), self.min_punctuation): + return + raise ValidationError( + ungettext( + 'Your password must contain at least %(min_punctuation)d punctuation character.', + 'Your password must contain at least %(min_punctuation)d punctuation characters.', + self.min_punctuation + ), + code='too_few_punctuation_characters', + params={'min_punctuation': self.min_punctuation}, + ) + + def get_help_text(self): + return ungettext( + "Your password must contain at least %(min_punctuation)d punctuation character.", + "Your password must contain at least %(min_punctuation)d punctuation characters.", + self.min_punctuation + ) % {'min_punctuation': self.min_punctuation} + + def get_instruction_text(self): + if self.min_punctuation > 0: + return ungettext( + '%(num)d punctuation character', + '%(num)d punctuation characters', + self.min_punctuation + ) % {'num': self.min_punctuation} + else: + return '' + + def get_restriction(self): + """ + Returns a key, value pair for the restrictions related to the Validator + """ + return 'min_punctuation', self.min_punctuation + + +class SymbolValidator(object): + """ + Validate whether the password contains at least min_symbol symbols as defined by unicode categories. + + Parameters: + min_symbol (int): the minimum number of symbols to require + in the password. Must be >= 0. + """ + def __init__(self, min_symbol=0): + self.min_symbol = min_symbol + + def validate(self, password, user=None): + if _validate_condition(password, lambda c: 'S' in unicodedata.category(c), self.min_symbol): + return + raise ValidationError( + ungettext( + 'Your password must contain at least %(min_symbol)d symbol.', + 'Your password must contain at least %(min_symbol)d symbols.', + self.min_symbol + ), + code='too_few_symbols', + params={'min_symbol': self.min_symbol}, + ) + + def get_help_text(self): + return ungettext( + "Your password must contain at least %(min_symbol)d symbol.", + "Your password must contain at least %(min_symbol)d symbols.", + self.min_symbol + ) % {'min_symbol': self.min_symbol} + + def get_instruction_text(self): + if self.min_symbol > 0: + return ungettext( + '%(num)d symbol', + '%(num)d symbols', + self.min_symbol + ) % {'num': self.min_symbol} + else: + return '' + + def get_restriction(self): + """ + Returns a key, value pair for the restrictions related to the Validator + """ + return 'min_symbol', self.min_symbol diff --git a/lms/envs/aws.py b/lms/envs/aws.py index d6ae2d06af..4f56c9930d 100644 --- a/lms/envs/aws.py +++ b/lms/envs/aws.py @@ -652,11 +652,7 @@ MAX_FAILED_LOGIN_ATTEMPTS_ALLOWED = ENV_TOKENS.get("MAX_FAILED_LOGIN_ATTEMPTS_AL MAX_FAILED_LOGIN_ATTEMPTS_LOCKOUT_PERIOD_SECS = ENV_TOKENS.get("MAX_FAILED_LOGIN_ATTEMPTS_LOCKOUT_PERIOD_SECS", 15 * 60) #### PASSWORD POLICY SETTINGS ##### -PASSWORD_MIN_LENGTH = ENV_TOKENS.get("PASSWORD_MIN_LENGTH") -PASSWORD_MAX_LENGTH = ENV_TOKENS.get("PASSWORD_MAX_LENGTH") -PASSWORD_COMPLEXITY = ENV_TOKENS.get("PASSWORD_COMPLEXITY", {}) -PASSWORD_DICTIONARY_EDIT_DISTANCE_THRESHOLD = ENV_TOKENS.get("PASSWORD_DICTIONARY_EDIT_DISTANCE_THRESHOLD") -PASSWORD_DICTIONARY = ENV_TOKENS.get("PASSWORD_DICTIONARY", []) +AUTH_PASSWORD_VALIDATORS = ENV_TOKENS.get("AUTH_PASSWORD_VALIDATORS", []) ### INACTIVITY SETTINGS #### SESSION_INACTIVITY_TIMEOUT_IN_SECONDS = AUTH_TOKENS.get("SESSION_INACTIVITY_TIMEOUT_IN_SECONDS") diff --git a/lms/envs/devstack.py b/lms/envs/devstack.py index 70594981c3..1f811dec70 100644 --- a/lms/envs/devstack.py +++ b/lms/envs/devstack.py @@ -142,9 +142,6 @@ FEATURES['ENABLE_MAX_FAILED_LOGIN_ATTEMPTS'] = False FEATURES['SQUELCH_PII_IN_LOGS'] = False FEATURES['PREVENT_CONCURRENT_LOGINS'] = False FEATURES['ADVANCED_SECURITY'] = False -PASSWORD_MIN_LENGTH = None -PASSWORD_COMPLEXITY = {} - ########################### Milestones ################################# FEATURES['MILESTONES_APP'] = True diff --git a/openedx/core/djangoapps/password_policy/compliance.py b/openedx/core/djangoapps/password_policy/compliance.py index 2c20364f11..53326b9957 100644 --- a/openedx/core/djangoapps/password_policy/compliance.py +++ b/openedx/core/djangoapps/password_policy/compliance.py @@ -8,7 +8,7 @@ from django.conf import settings from django.utils.translation import ugettext as _ from util.date_utils import DEFAULT_SHORT_DATE_FORMAT, strftime_localized -from util.password_policy_validators import validate_password +from util.password_policy_validators import edX_validate_password class NonCompliantPasswordException(Exception): @@ -104,7 +104,7 @@ def _check_user_compliance(user, password): Returns a boolean indicating whether or not the user is compliant with password policy rules. """ try: - validate_password(password, user=user, password_reset=False) + edX_validate_password(password, user=user) return True except Exception: # pylint: disable=broad-except # If anything goes wrong, we should assume the password is not compliant but we don't necessarily diff --git a/openedx/core/djangoapps/user_api/accounts/api.py b/openedx/core/djangoapps/user_api/accounts/api.py index 50ec3920ff..25b62aabb3 100644 --- a/openedx/core/djangoapps/user_api/accounts/api.py +++ b/openedx/core/djangoapps/user_api/accounts/api.py @@ -18,7 +18,7 @@ from student.models import User, UserProfile, Registration, email_exists_or_reti from student import forms as student_forms from student import views as student_views from util.model_utils import emit_setting_changed_event -from util.password_policy_validators import validate_password +from util.password_policy_validators import edX_validate_password from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.user_api import errors, accounts, forms, helpers @@ -327,7 +327,7 @@ def create_account(username, password, email): # Validate the username, password, and email # This will raise an exception if any of these are not in a valid format. _validate_username(username) - _validate_password(password, username) + _validate_password(password, username, email) _validate_email(email) # Create the user account, setting them to "inactive" until they activate their account. @@ -494,17 +494,17 @@ def get_confirm_email_validation_error(confirm_email, email): return _validate(_validate_confirm_email, errors.AccountEmailInvalid, confirm_email, email) -def get_password_validation_error(password, username=None): +def get_password_validation_error(password, username=None, email=None): """Get the built-in validation error message for when the password is invalid in some way. :param password: The proposed password (unicode). :param username: The username associated with the user's account (unicode). - :param default: The message to default to in case of no error. + :param email: The email associated with the user's account (unicode). :return: Validation error message. """ - return _validate(_validate_password, errors.AccountPasswordInvalid, password, username) + return _validate(_validate_password, errors.AccountPasswordInvalid, password, username, email) def get_country_validation_error(country): @@ -643,15 +643,17 @@ def _validate_confirm_email(confirm_email, email): raise errors.AccountEmailInvalid(accounts.REQUIRED_FIELD_CONFIRM_EMAIL_MSG) -def _validate_password(password, username=None): +def _validate_password(password, username=None, email=None): """Validate the format of the user's password. Passwords cannot be the same as the username of the account, - so we take `username` as an argument. + so we create a temp_user using the username and email to test the password against. + This user is never saved. Arguments: password (unicode): The proposed password. username (unicode): The username associated with the user's account. + email (unicode): The email associated with the user's account. Returns: None @@ -662,12 +664,12 @@ def _validate_password(password, username=None): """ try: _validate_type(password, basestring, accounts.PASSWORD_BAD_TYPE_MSG) - - validate_password(password, username=username) + temp_user = User(username=username, email=email) if username else None + edX_validate_password(password, user=temp_user) except errors.AccountDataBadType as invalid_password_err: raise errors.AccountPasswordInvalid(text_type(invalid_password_err)) except ValidationError as validation_err: - raise errors.AccountPasswordInvalid(validation_err.message) + raise errors.AccountPasswordInvalid(' '.join(validation_err.messages)) def _validate_country(country): diff --git a/openedx/core/djangoapps/user_api/api.py b/openedx/core/djangoapps/user_api/api.py index 4a321347b0..3a349409d5 100644 --- a/openedx/core/djangoapps/user_api/api.py +++ b/openedx/core/djangoapps/user_api/api.py @@ -18,7 +18,7 @@ from openedx.features.enterprise_support.api import enterprise_customer_for_requ from student.forms import get_registration_extension_form from student.models import UserProfile from util.password_policy_validators import ( - password_complexity, password_instructions, password_max_length, password_min_length + password_validators_instruction_texts, password_validators_restrictions ) @@ -118,9 +118,10 @@ def get_login_session_form(request): "password", label=password_label, field_type="password", - restrictions={ - "max_length": password_max_length(), - } + # The following restriction contains the assumption that the max password length will never exceed 5000 + # characters. The point of this restriction on the login page is to prevent any sort of attacks + # involving sending massive passwords. + restrictions={'max_length': 5000} ) form_desc.add_field( @@ -419,22 +420,12 @@ class RegistrationFormFactory(object): # meant to hold the user's password. password_label = _(u"Password") - restrictions = { - "min_length": password_min_length(), - "max_length": password_max_length(), - } - - complexities = password_complexity() - for key, value in complexities.iteritems(): - api_key = key.lower().replace(' ', '_') - restrictions[api_key] = value - form_desc.add_field( "password", label=password_label, field_type="password", - instructions=password_instructions(), - restrictions=restrictions, + instructions=password_validators_instruction_texts(), + restrictions=password_validators_restrictions(), required=required ) diff --git a/openedx/core/djangoapps/user_api/helpers.py b/openedx/core/djangoapps/user_api/helpers.py index 66f0a9dfc9..60784e66db 100644 --- a/openedx/core/djangoapps/user_api/helpers.py +++ b/openedx/core/djangoapps/user_api/helpers.py @@ -125,8 +125,8 @@ class FormDescription(object): ALLOWED_RESTRICTIONS = { "text": ["min_length", "max_length"], - "password": ["min_length", "max_length", "upper", "lower", "digits", "punctuation", "non_ascii", "words", - "numeric", "alphabetic"], + "password": ["min_length", "max_length", "min_upper", "min_lower", + "min_punctuation", "min_symbol", "min_numeric", "min_alphabetic"], "email": ["min_length", "max_length", "readonly"], } diff --git a/openedx/core/djangoapps/user_api/validation/views.py b/openedx/core/djangoapps/user_api/validation/views.py index e5a5a41fed..16812159a1 100644 --- a/openedx/core/djangoapps/user_api/validation/views.py +++ b/openedx/core/djangoapps/user_api/validation/views.py @@ -144,14 +144,15 @@ class RegistrationValidationView(APIView): return invalid_email_error or email_exists_error def confirm_email_handler(self, request): - email = request.data.get('email', None) + email = request.data.get('email') confirm_email = request.data.get('confirm_email') return get_confirm_email_validation_error(confirm_email, email) def password_handler(self, request): - username = request.data.get('username', None) + username = request.data.get('username') + email = request.data.get('email') password = request.data.get('password') - return get_password_validation_error(password, username) + return get_password_validation_error(password, username, email) def country_handler(self, request): country = request.data.get('country') From deb758c808aa81a6d370ab94a97ffd90ae1b7dde Mon Sep 17 00:00:00 2001 From: Dillon Dumesnil Date: Tue, 2 Oct 2018 14:21:07 -0400 Subject: [PATCH 2/2] Added in tests for the new password validation. Fixed old tests that relied on the old configuration values and old way of validating passwords. Also improved registration page by always showing error messages rather than hiding them on leaving the field. --- cms/envs/aws.py | 2 +- cms/envs/common.py | 26 +- cms/envs/production.py | 6 +- common/djangoapps/student/forms.py | 10 +- .../management/tests/test_create_user.py | 2 +- .../student/tests/test_password_policy.py | 204 +++++++------- .../student/tests/test_reset_password.py | 11 +- common/djangoapps/student/views/management.py | 6 +- .../util/password_policy_validators.py | 74 +++-- .../tests/test_password_policy_validators.py | 253 ++++++++++++++---- .../tests/studio/test_studio_course_team.py | 2 +- .../courseware/tests/test_password_history.py | 148 +--------- lms/envs/aws.py | 2 +- lms/envs/bok_choy.py | 4 - lms/envs/common.py | 25 +- lms/envs/devstack.py | 1 - lms/envs/production.py | 6 +- lms/envs/test.py | 2 - .../js/student_account/views/RegisterView.js | 4 +- .../djangoapps/password_policy/compliance.py | 4 +- .../password_policy/tests/test_compliance.py | 25 +- .../core/djangoapps/user_api/accounts/api.py | 5 +- .../user_api/accounts/tests/test_api.py | 4 +- .../user_api/accounts/tests/testutils.py | 8 +- openedx/core/djangoapps/user_api/api.py | 7 +- .../djangoapps/user_api/tests/test_views.py | 58 ++-- .../user_api/validation/tests/test_views.py | 20 +- .../djangoapps/user_authn/views/deprecated.py | 2 +- .../djangoapps/user_authn/views/register.py | 3 +- .../user_authn/views/tests/test_register.py | 27 +- 30 files changed, 482 insertions(+), 469 deletions(-) diff --git a/cms/envs/aws.py b/cms/envs/aws.py index 25d646ed1e..a7918cfd2e 100644 --- a/cms/envs/aws.py +++ b/cms/envs/aws.py @@ -447,7 +447,7 @@ MAX_FAILED_LOGIN_ATTEMPTS_ALLOWED = ENV_TOKENS.get("MAX_FAILED_LOGIN_ATTEMPTS_AL MAX_FAILED_LOGIN_ATTEMPTS_LOCKOUT_PERIOD_SECS = ENV_TOKENS.get("MAX_FAILED_LOGIN_ATTEMPTS_LOCKOUT_PERIOD_SECS", 15 * 60) #### PASSWORD POLICY SETTINGS ##### -AUTH_PASSWORD_VALIDATORS = ENV_TOKENS.get("AUTH_PASSWORD_VALIDATORS", []) +AUTH_PASSWORD_VALIDATORS = ENV_TOKENS.get("AUTH_PASSWORD_VALIDATORS", AUTH_PASSWORD_VALIDATORS) ### INACTIVITY SETTINGS #### SESSION_INACTIVITY_TIMEOUT_IN_SECONDS = AUTH_TOKENS.get("SESSION_INACTIVITY_TIMEOUT_IN_SECONDS") diff --git a/cms/envs/common.py b/cms/envs/common.py index 3c185494e1..d6b9f33d8e 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -200,9 +200,6 @@ FEATURES = { # an Open edX admin has added them to the course creator group. 'ENABLE_CREATOR_GROUP': True, - # whether to use password policy enforcement or not - 'ENFORCE_PASSWORD_POLICY': False, - # Turn off account locking if failed login attempts exceeds a limit 'ENABLE_MAX_FAILED_LOGIN_ATTEMPTS': False, @@ -1240,12 +1237,23 @@ EVENT_TRACKING_BACKENDS = { EVENT_TRACKING_PROCESSORS = [] #### PASSWORD POLICY SETTINGS ##### - -PASSWORD_MIN_LENGTH = None -PASSWORD_MAX_LENGTH = None -PASSWORD_COMPLEXITY = {} -PASSWORD_DICTIONARY_EDIT_DISTANCE_THRESHOLD = None -PASSWORD_DICTIONARY = [] +AUTH_PASSWORD_VALIDATORS = [ + { + "NAME": "django.contrib.auth.password_validation.UserAttributeSimilarityValidator", + }, + { + "NAME": "util.password_policy_validators.MinimumLengthValidator", + "OPTIONS": { + "min_length": 2 + } + }, + { + "NAME": "util.password_policy_validators.MaximumLengthValidator", + "OPTIONS": { + "max_length": 75 + } + }, +] ##### ACCOUNT LOCKOUT DEFAULT PARAMETERS ##### MAX_FAILED_LOGIN_ATTEMPTS_ALLOWED = 5 diff --git a/cms/envs/production.py b/cms/envs/production.py index 5a8842f9d2..65f6f8813a 100644 --- a/cms/envs/production.py +++ b/cms/envs/production.py @@ -446,11 +446,7 @@ MAX_FAILED_LOGIN_ATTEMPTS_ALLOWED = ENV_TOKENS.get("MAX_FAILED_LOGIN_ATTEMPTS_AL MAX_FAILED_LOGIN_ATTEMPTS_LOCKOUT_PERIOD_SECS = ENV_TOKENS.get("MAX_FAILED_LOGIN_ATTEMPTS_LOCKOUT_PERIOD_SECS", 15 * 60) #### PASSWORD POLICY SETTINGS ##### -PASSWORD_MIN_LENGTH = ENV_TOKENS.get("PASSWORD_MIN_LENGTH") -PASSWORD_MAX_LENGTH = ENV_TOKENS.get("PASSWORD_MAX_LENGTH") -PASSWORD_COMPLEXITY = ENV_TOKENS.get("PASSWORD_COMPLEXITY", {}) -PASSWORD_DICTIONARY_EDIT_DISTANCE_THRESHOLD = ENV_TOKENS.get("PASSWORD_DICTIONARY_EDIT_DISTANCE_THRESHOLD") -PASSWORD_DICTIONARY = ENV_TOKENS.get("PASSWORD_DICTIONARY", []) +AUTH_PASSWORD_VALIDATORS = ENV_TOKENS.get("AUTH_PASSWORD_VALIDATORS", AUTH_PASSWORD_VALIDATORS) ### INACTIVITY SETTINGS #### SESSION_INACTIVITY_TIMEOUT_IN_SECONDS = AUTH_TOKENS.get("SESSION_INACTIVITY_TIMEOUT_IN_SECONDS") diff --git a/common/djangoapps/student/forms.py b/common/djangoapps/student/forms.py index c43b10ca49..d3b3cdb5a8 100644 --- a/common/djangoapps/student/forms.py +++ b/common/djangoapps/student/forms.py @@ -27,7 +27,7 @@ from openedx.core.djangoapps.user_api import accounts as accounts_settings from openedx.core.djangoapps.user_api.preferences.api import get_user_preference from student.message_types import PasswordReset from student.models import CourseEnrollmentAllowed, email_exists_or_retired -from util.password_policy_validators import edX_validate_password +from util.password_policy_validators import validate_password def send_password_reset_email_for_user(user, request): @@ -225,14 +225,14 @@ class AccountCreationForm(forms.Form): data=None, extra_fields=None, extended_profile_fields=None, - enforce_password_policy=False, + do_third_party_auth=True, tos_required=True ): super(AccountCreationForm, self).__init__(data) extra_fields = extra_fields or {} self.extended_profile_fields = extended_profile_fields or {} - self.enforce_password_policy = enforce_password_policy + self.do_third_party_auth = do_third_party_auth if tos_required: self.fields["terms_of_service"] = TrueField( error_messages={"required": _("You must accept the terms of service.")} @@ -280,13 +280,13 @@ class AccountCreationForm(forms.Form): def clean_password(self): """Enforce password policies (if applicable)""" password = self.cleaned_data["password"] - if self.enforce_password_policy: + if not self.do_third_party_auth: # Creating a temporary user object to test password against username # This user should NOT be saved username = self.cleaned_data.get('username') email = self.cleaned_data.get('email') temp_user = User(username=username, email=email) if username else None - edX_validate_password(password, temp_user) + validate_password(password, temp_user) return password def clean_email(self): diff --git a/common/djangoapps/student/management/tests/test_create_user.py b/common/djangoapps/student/management/tests/test_create_user.py index e0a638b9a7..903d43faa7 100644 --- a/common/djangoapps/student/management/tests/test_create_user.py +++ b/common/djangoapps/student/management/tests/test_create_user.py @@ -22,7 +22,7 @@ class CreateUserMgmtTests(SharedModuleStoreTestCase): self.course = CourseFactory.create() self.user_model = get_user_model() self.default_email = 'testuser555@test.edx.org' - self.default_password = 'testuser@555@password' + self.default_password = 'b3TT3rPa$$w0rd!' # This is the default mode that the create_user commands gives a user enrollment self.default_course_mode = CourseMode.HONOR diff --git a/common/djangoapps/student/tests/test_password_policy.py b/common/djangoapps/student/tests/test_password_policy.py index 1b8dc1ce04..366fdf11ae 100644 --- a/common/djangoapps/student/tests/test_password_policy.py +++ b/common/djangoapps/student/tests/test_password_policy.py @@ -16,9 +16,9 @@ from mock import patch from openedx.core.djangoapps.external_auth.models import ExternalAuthMap from openedx.core.djangoapps.site_configuration.tests.factories import SiteFactory from openedx.core.djangoapps.user_authn.views.deprecated import create_account +from util.password_policy_validators import create_validator_config -@patch.dict("django.conf.settings.FEATURES", {'ENFORCE_PASSWORD_POLICY': True}) class TestPasswordPolicy(TestCase): """ Go through some password policy tests to make sure things are properly working @@ -35,7 +35,9 @@ class TestPasswordPolicy(TestCase): 'honor_code': 'true', } - @override_settings(PASSWORD_MIN_LENGTH=6) + @override_settings(AUTH_PASSWORD_VALIDATORS=[ + create_validator_config('util.password_policy_validators.MinimumLengthValidator', {'min_length': 6}) + ]) def test_password_length_too_short(self): self.url_params['password'] = 'aaa' response = self.client.post(self.url, self.url_params) @@ -43,10 +45,12 @@ class TestPasswordPolicy(TestCase): obj = json.loads(response.content) self.assertEqual( obj['value'], - "Enter a password with at least 6 characters.", + "This password is too short. It must contain at least 6 characters.", ) - @override_settings(PASSWORD_MIN_LENGTH=6) + @override_settings(AUTH_PASSWORD_VALIDATORS=[ + create_validator_config('util.password_policy_validators.MinimumLengthValidator', {'min_length': 6}) + ]) def test_password_length_long_enough(self): self.url_params['password'] = 'ThisIsALongerPassword' response = self.client.post(self.url, self.url_params) @@ -54,7 +58,9 @@ class TestPasswordPolicy(TestCase): obj = json.loads(response.content) self.assertTrue(obj['success']) - @override_settings(PASSWORD_MAX_LENGTH=12) + @override_settings(AUTH_PASSWORD_VALIDATORS=[ + create_validator_config('util.password_policy_validators.MaximumLengthValidator', {'max_length': 12}) + ]) def test_password_length_too_long(self): self.url_params['password'] = 'ThisPasswordIsWayTooLong' response = self.client.post(self.url, self.url_params) @@ -62,10 +68,12 @@ class TestPasswordPolicy(TestCase): obj = json.loads(response.content) self.assertEqual( obj['value'], - "Enter a password with at most 12 characters.", + "This password is too long. It must contain no more than 12 characters.", ) - @patch.dict("django.conf.settings.PASSWORD_COMPLEXITY", {'UPPER': 3}) + @override_settings(AUTH_PASSWORD_VALIDATORS=[ + create_validator_config('util.password_policy_validators.UppercaseValidator', {'min_upper': 3}) + ]) def test_password_not_enough_uppercase(self): self.url_params['password'] = 'thisshouldfail' response = self.client.post(self.url, self.url_params) @@ -73,10 +81,12 @@ class TestPasswordPolicy(TestCase): obj = json.loads(response.content) self.assertEqual( obj['value'], - "Enter a password with at least 3 uppercase letters.", + "This password must contain at least 3 uppercase letters.", ) - @patch.dict("django.conf.settings.PASSWORD_COMPLEXITY", {'UPPER': 3}) + @override_settings(AUTH_PASSWORD_VALIDATORS=[ + create_validator_config('util.password_policy_validators.UppercaseValidator', {'min_upper': 3}) + ]) def test_password_enough_uppercase(self): self.url_params['password'] = 'ThisShouldPass' response = self.client.post(self.url, self.url_params) @@ -84,7 +94,9 @@ class TestPasswordPolicy(TestCase): obj = json.loads(response.content) self.assertTrue(obj['success']) - @patch.dict("django.conf.settings.PASSWORD_COMPLEXITY", {'LOWER': 3}) + @override_settings(AUTH_PASSWORD_VALIDATORS=[ + create_validator_config('util.password_policy_validators.LowercaseValidator', {'min_lower': 3}) + ]) def test_password_not_enough_lowercase(self): self.url_params['password'] = 'THISSHOULDFAIL' response = self.client.post(self.url, self.url_params) @@ -92,10 +104,12 @@ class TestPasswordPolicy(TestCase): obj = json.loads(response.content) self.assertEqual( obj['value'], - "Enter a password with at least 3 lowercase letters.", + "This password must contain at least 3 lowercase letters.", ) - @patch.dict("django.conf.settings.PASSWORD_COMPLEXITY", {'LOWER': 3}) + @override_settings(AUTH_PASSWORD_VALIDATORS=[ + create_validator_config('util.password_policy_validators.LowercaseValidator', {'min_lower': 3}) + ]) def test_password_enough_lowercase(self): self.url_params['password'] = 'ThisShouldPass' response = self.client.post(self.url, self.url_params) @@ -103,26 +117,9 @@ class TestPasswordPolicy(TestCase): obj = json.loads(response.content) self.assertTrue(obj['success']) - @patch.dict("django.conf.settings.PASSWORD_COMPLEXITY", {'DIGITS': 3}) - def test_not_enough_digits(self): - self.url_params['password'] = 'thishasnodigits' - response = self.client.post(self.url, self.url_params) - self.assertEqual(response.status_code, 400) - obj = json.loads(response.content) - self.assertEqual( - obj['value'], - "Enter a password with at least 3 digits.", - ) - - @patch.dict("django.conf.settings.PASSWORD_COMPLEXITY", {'DIGITS': 3}) - def test_enough_digits(self): - self.url_params['password'] = 'Th1sSh0uldPa88' - response = self.client.post(self.url, self.url_params) - self.assertEqual(response.status_code, 200) - obj = json.loads(response.content) - self.assertTrue(obj['success']) - - @patch.dict("django.conf.settings.PASSWORD_COMPLEXITY", {'PUNCTUATION': 3}) + @override_settings(AUTH_PASSWORD_VALIDATORS=[ + create_validator_config('util.password_policy_validators.PunctuationValidator', {'min_punctuation': 3}) + ]) def test_not_enough_punctuations(self): self.url_params['password'] = 'thisshouldfail' response = self.client.post(self.url, self.url_params) @@ -130,10 +127,12 @@ class TestPasswordPolicy(TestCase): obj = json.loads(response.content) self.assertEqual( obj['value'], - "Enter a password with at least 3 punctuation marks.", + "This password must contain at least 3 punctuation marks.", ) - @patch.dict("django.conf.settings.PASSWORD_COMPLEXITY", {'PUNCTUATION': 3}) + @override_settings(AUTH_PASSWORD_VALIDATORS=[ + create_validator_config('util.password_policy_validators.PunctuationValidator', {'min_punctuation': 3}) + ]) def test_enough_punctuations(self): self.url_params['password'] = 'Th!sSh.uldPa$*' response = self.client.post(self.url, self.url_params) @@ -141,26 +140,9 @@ class TestPasswordPolicy(TestCase): obj = json.loads(response.content) self.assertTrue(obj['success']) - @patch.dict("django.conf.settings.PASSWORD_COMPLEXITY", {'WORDS': 3}) - def test_not_enough_words(self): - self.url_params['password'] = 'thisshouldfail' - response = self.client.post(self.url, self.url_params) - self.assertEqual(response.status_code, 400) - obj = json.loads(response.content) - self.assertEqual( - obj['value'], - "Enter a password with at least 3 words.", - ) - - @patch.dict("django.conf.settings.PASSWORD_COMPLEXITY", {'WORDS': 3}) - def test_enough_wordss(self): - self.url_params['password'] = u'this should pass' - response = self.client.post(self.url, self.url_params) - self.assertEqual(response.status_code, 200) - obj = json.loads(response.content) - self.assertTrue(obj['success']) - - @patch.dict("django.conf.settings.PASSWORD_COMPLEXITY", {'NUMERIC': 3}) + @override_settings(AUTH_PASSWORD_VALIDATORS=[ + create_validator_config('util.password_policy_validators.NumericValidator', {'min_numeric': 3}) + ]) def test_not_enough_numeric_characters(self): self.url_params['password'] = u'thishouldfail½2' response = self.client.post(self.url, self.url_params) @@ -168,18 +150,23 @@ class TestPasswordPolicy(TestCase): obj = json.loads(response.content) self.assertEqual( obj['value'], - "Enter a password with at least 3 numbers.", + "This password must contain at least 3 numbers.", ) - @patch.dict("django.conf.settings.PASSWORD_COMPLEXITY", {'NUMERIC': 3}) + @override_settings(AUTH_PASSWORD_VALIDATORS=[ + create_validator_config('util.password_policy_validators.NumericValidator', {'min_numeric': 3}) + ]) def test_enough_numeric_characters(self): - self.url_params['password'] = u'thisShouldPass½33' # This unicode 1/2 should count as a numeric value here + # This unicode 1/2 should count as a numeric value here + self.url_params['password'] = u'thisShouldPass½33' response = self.client.post(self.url, self.url_params) self.assertEqual(response.status_code, 200) obj = json.loads(response.content) self.assertTrue(obj['success']) - @patch.dict("django.conf.settings.PASSWORD_COMPLEXITY", {'ALPHABETIC': 3}) + @override_settings(AUTH_PASSWORD_VALIDATORS=[ + create_validator_config('util.password_policy_validators.AlphabeticValidator', {'min_alphabetic': 3}) + ]) def test_not_enough_alphabetic_characters(self): self.url_params['password'] = '123456ab' response = self.client.post(self.url, self.url_params) @@ -187,10 +174,12 @@ class TestPasswordPolicy(TestCase): obj = json.loads(response.content) self.assertEqual( obj['value'], - "Enter a password with at least 3 letters.", + "This password must contain at least 3 letters.", ) - @patch.dict("django.conf.settings.PASSWORD_COMPLEXITY", {'ALPHABETIC': 3}) + @override_settings(AUTH_PASSWORD_VALIDATORS=[ + create_validator_config('util.password_policy_validators.AlphabeticValidator', {'min_alphabetic': 3}) + ]) def test_enough_alphabetic_characters(self): self.url_params['password'] = u'𝒯𝓗Ï𝓼𝒫å𝓼𝓼𝔼𝓼' response = self.client.post(self.url, self.url_params) @@ -198,86 +187,65 @@ class TestPasswordPolicy(TestCase): obj = json.loads(response.content) self.assertTrue(obj['success']) - @patch.dict("django.conf.settings.PASSWORD_COMPLEXITY", { - 'PUNCTUATION': 3, - 'WORDS': 3, - 'DIGITS': 3, - 'LOWER': 3, - 'UPPER': 3, - }) + @override_settings(AUTH_PASSWORD_VALIDATORS=[ + create_validator_config('util.password_policy_validators.MinimumLengthValidator', {'min_length': 3}), + create_validator_config('util.password_policy_validators.UppercaseValidator', {'min_upper': 3}), + create_validator_config('util.password_policy_validators.NumericValidator', {'min_numeric': 3}), + create_validator_config('util.password_policy_validators.PunctuationValidator', {'min_punctuation': 3}), + ]) def test_multiple_errors_fail(self): self.url_params['password'] = 'thisshouldfail' response = self.client.post(self.url, self.url_params) self.assertEqual(response.status_code, 400) obj = json.loads(response.content) errstring = ( - "Enter a password with at least " - "3 uppercase letters & " - "3 digits & " - "3 punctuation marks & " - "3 words." + "This password must contain at least 3 uppercase letters. " + "This password must contain at least 3 numbers. " + "This password must contain at least 3 punctuation marks." ) self.assertEqual(obj['value'], errstring) - @patch.dict("django.conf.settings.PASSWORD_COMPLEXITY", { - 'PUNCTUATION': 3, - 'WORDS': 3, - 'DIGITS': 3, - 'LOWER': 3, - 'UPPER': 3, - }) + @override_settings(AUTH_PASSWORD_VALIDATORS=[ + create_validator_config('util.password_policy_validators.MinimumLengthValidator', {'min_length': 3}), + create_validator_config('util.password_policy_validators.UppercaseValidator', {'min_upper': 3}), + create_validator_config('util.password_policy_validators.LowercaseValidator', {'min_lower': 3}), + create_validator_config('util.password_policy_validators.NumericValidator', {'min_numeric': 3}), + create_validator_config('util.password_policy_validators.PunctuationValidator', {'min_punctuation': 3}), + ]) def test_multiple_errors_pass(self): - self.url_params['password'] = u'tH1s Sh0u!d P3#$' + self.url_params['password'] = u'tH1s Sh0u!d P3#$!' response = self.client.post(self.url, self.url_params) self.assertEqual(response.status_code, 200) obj = json.loads(response.content) self.assertTrue(obj['success']) - @override_settings(PASSWORD_DICTIONARY=['foo', 'bar']) - @override_settings(PASSWORD_DICTIONARY_EDIT_DISTANCE_THRESHOLD=1) - def test_dictionary_similarity_fail1(self): - self.url_params['password'] = 'foo' + @override_settings(AUTH_PASSWORD_VALIDATORS=[ + create_validator_config('django.contrib.auth.password_validation.CommonPasswordValidator') + ]) + def test_common_password_fail(self): + self.url_params['password'] = 'password' response = self.client.post(self.url, self.url_params) self.assertEqual(response.status_code, 400) obj = json.loads(response.content) self.assertEqual( obj['value'], - "Password is too similar to a dictionary word.", + "This password is too common.", ) - @override_settings(PASSWORD_DICTIONARY=['foo', 'bar']) - @override_settings(PASSWORD_DICTIONARY_EDIT_DISTANCE_THRESHOLD=1) - def test_dictionary_similarity_fail2(self): - self.url_params['password'] = 'bar' - response = self.client.post(self.url, self.url_params) - self.assertEqual(response.status_code, 400) - obj = json.loads(response.content) - self.assertEqual( - obj['value'], - "Password is too similar to a dictionary word.", - ) - - @override_settings(PASSWORD_DICTIONARY=['foo', 'bar']) - @override_settings(PASSWORD_DICTIONARY_EDIT_DISTANCE_THRESHOLD=1) - def test_dictionary_similarity_fail3(self): - self.url_params['password'] = 'fo0' - response = self.client.post(self.url, self.url_params) - self.assertEqual(response.status_code, 400) - obj = json.loads(response.content) - self.assertEqual( - obj['value'], - "Password is too similar to a dictionary word.", - ) - - @override_settings(PASSWORD_DICTIONARY=['foo', 'bar']) - @override_settings(PASSWORD_DICTIONARY_EDIT_DISTANCE_THRESHOLD=1) - def test_dictionary_similarity_pass(self): + @override_settings(AUTH_PASSWORD_VALIDATORS=[ + create_validator_config('django.contrib.auth.password_validation.CommonPasswordValidator') + ]) + def test_common_password_pass(self): self.url_params['password'] = 'this_is_ok' response = self.client.post(self.url, self.url_params) self.assertEqual(response.status_code, 200) obj = json.loads(response.content) self.assertTrue(obj['success']) + @override_settings(AUTH_PASSWORD_VALIDATORS=[ + create_validator_config('util.password_policy_validators.MinimumLengthValidator', {'min_length': 6}), + create_validator_config('util.password_policy_validators.MaximumLengthValidator', {'max_length': 75}), + ]) def test_with_unicode(self): self.url_params['password'] = u'四節比分和七年前' response = self.client.post(self.url, self.url_params) @@ -285,7 +253,9 @@ class TestPasswordPolicy(TestCase): obj = json.loads(response.content) self.assertTrue(obj['success']) - @override_settings(PASSWORD_MIN_LENGTH=6, SESSION_ENGINE='django.contrib.sessions.backends.cache') + @override_settings(AUTH_PASSWORD_VALIDATORS=[ + create_validator_config('util.password_policy_validators.MinimumLengthValidator', {'min_length': 6}) + ], SESSION_ENGINE='django.contrib.sessions.backends.cache') def test_ext_auth_password_length_too_short(self): """ Tests that even if password policy is enforced, ext_auth registrations aren't subject to it @@ -325,6 +295,9 @@ class TestUsernamePasswordNonmatch(TestCase): 'honor_code': 'true', } + @override_settings(AUTH_PASSWORD_VALIDATORS=[ + create_validator_config('django.contrib.auth.password_validation.UserAttributeSimilarityValidator') + ]) def test_with_username_password_match(self): self.url_params['username'] = "foobar" self.url_params['password'] = "foobar" @@ -333,9 +306,12 @@ class TestUsernamePasswordNonmatch(TestCase): obj = json.loads(response.content) self.assertEqual( obj['value'], - "Password cannot be the same as the username.", + "The password is too similar to the username.", ) + @override_settings(AUTH_PASSWORD_VALIDATORS=[ + create_validator_config('django.contrib.auth.password_validation.UserAttributeSimilarityValidator') + ]) def test_with_username_password_nonmatch(self): self.url_params['username'] = "foobar" self.url_params['password'] = "nonmatch" diff --git a/common/djangoapps/student/tests/test_reset_password.py b/common/djangoapps/student/tests/test_reset_password.py index 77ad5c0075..cb9c0ee075 100644 --- a/common/djangoapps/student/tests/test_reset_password.py +++ b/common/djangoapps/student/tests/test_reset_password.py @@ -29,6 +29,7 @@ from openedx.core.djangolib.testing.utils import CacheIsolationTestCase from student.tests.factories import UserFactory from student.tests.test_email import mock_render_to_string from student.views import SETTING_CHANGE_INITIATED, password_reset, password_reset_confirm_wrapper +from util.password_policy_validators import create_validator_config from util.testing import EventTestMixin from .test_configuration_overrides import fake_get_value @@ -350,16 +351,18 @@ class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase): self.user.refresh_from_db() assert not self.user.is_active - @override_settings(PASSWORD_MIN_LENGTH=2) - @override_settings(PASSWORD_MAX_LENGTH=10) + @override_settings(AUTH_PASSWORD_VALIDATORS=[ + create_validator_config('util.password_policy_validators.MinimumLengthValidator', {'min_length': 2}), + create_validator_config('util.password_policy_validators.MaximumLengthValidator', {'max_length': 10}) + ]) @ddt.data( { 'password': '1', - 'error_message': 'Enter a password with at least 2 characters.', + 'error_message': 'This password is too short. It must contain at least 2 characters.', }, { 'password': '01234567891', - 'error_message': 'Enter a password with at most 10 characters.', + 'error_message': 'This password is too long. It must contain no more than 10 characters.', } ) def test_password_reset_with_invalid_length(self, password_dict): diff --git a/common/djangoapps/student/views/management.py b/common/djangoapps/student/views/management.py index 47ce54bf30..8652adceda 100644 --- a/common/djangoapps/student/views/management.py +++ b/common/djangoapps/student/views/management.py @@ -94,7 +94,7 @@ from student.text_me_the_app import TextMeTheAppFragmentView from util.bad_request_rate_limiter import BadRequestRateLimiter from util.db import outer_atomic from util.json_request import JsonResponse -from util.password_policy_validators import edX_validate_password +from util.password_policy_validators import validate_password log = logging.getLogger("edx.student") @@ -830,7 +830,7 @@ def password_reset_confirm_wrapper(request, uidb36=None, token=None): password = request.POST['new_password1'] try: - edX_validate_password(password, user=user) + validate_password(password, user=user) except ValidationError as err: # We have a password reset attempt which violates some security # policy, or any other validation. Use the existing Django template to communicate that @@ -839,7 +839,7 @@ def password_reset_confirm_wrapper(request, uidb36=None, token=None): 'validlink': True, 'form': None, 'title': _('Password reset unsuccessful'), - 'err_msg': err.message, + 'err_msg': ' '.join(err.messages), } context.update(platform_name) return TemplateResponse( diff --git a/common/djangoapps/util/password_policy_validators.py b/common/djangoapps/util/password_policy_validators.py index ded8d0040d..61dc5b089d 100644 --- a/common/djangoapps/util/password_policy_validators.py +++ b/common/djangoapps/util/password_policy_validators.py @@ -10,17 +10,45 @@ import unicodedata from django.conf import settings from django.contrib.auth.password_validation import ( get_default_password_validators, - validate_password, + validate_password as django_validate_password, MinimumLengthValidator as DjangoMinimumLengthValidator, ) from django.core.exceptions import ValidationError from django.utils.translation import ugettext as _, ungettext from six import text_type -from student.models import PasswordHistory - log = logging.getLogger(__name__) +# The following constant contains the assumption that the max password length will never exceed 5000 +# characters. The point of this restriction is to restrict the login page password field to prevent +# any sort of attacks involving sending massive passwords. +DEFAULT_MAX_PASSWORD_LENGTH = 5000 + + +def create_validator_config(name, options={}): + """ + This function is meant to be used for testing purposes to create validators + easily. It returns a validator config of the form: + { + "NAME": "util.password_policy_validators.SymbolValidator", + "OPTIONS": {"min_symbol": 1} + } + + Parameters: + name (str): the path name to the validator class to instantiate + options (dict): The dictionary of options to pass in to the validator. + These are used to initialize the validator with parameters. + If undefined, the default parameters will be used. + + Returns: + Dictionary containing the NAME and OPTIONS for the validator. These will + be used to instantiate an instance of the validator using Django. + """ + if options: + return {'NAME': name, 'OPTIONS': options} + + return {'NAME': name} + def password_validators_instruction_texts(): """ @@ -61,7 +89,7 @@ def password_validators_restrictions(): return complexity_restrictions -def edX_validate_password(password, user=None): +def validate_password(password, user=None): """ EdX's custom password validator for passwords. This function performs the following functions: @@ -89,7 +117,7 @@ def edX_validate_password(password, user=None): # no reason to get into weeds raise ValidationError([_('Invalid password.')]) - validate_password(password, user) + django_validate_password(password, user) def _validate_condition(password, fn, min_count): @@ -177,8 +205,8 @@ class AlphabeticValidator(object): return raise ValidationError( ungettext( - 'Your password must contain at least %(min_alphabetic)d letter.', - 'Your password must contain at least %(min_alphabetic)d letters.', + 'This password must contain at least %(min_alphabetic)d letter.', + 'This password must contain at least %(min_alphabetic)d letters.', self.min_alphabetic ), code='too_few_alphabetic_char', @@ -225,8 +253,8 @@ class NumericValidator(object): return raise ValidationError( ungettext( - 'Your password must contain at least %(min_numeric)d number.', - 'Your password must contain at least %(min_numeric)d numbers.', + 'This password must contain at least %(min_numeric)d number.', + 'This password must contain at least %(min_numeric)d numbers.', self.min_numeric ), code='too_few_numeric_char', @@ -273,8 +301,8 @@ class UppercaseValidator(object): return raise ValidationError( ungettext( - 'Your password must contain at least %(min_upper)d uppercase letter.', - 'Your password must contain at least %(min_upper)d uppercase letters.', + 'This password must contain at least %(min_upper)d uppercase letter.', + 'This password must contain at least %(min_upper)d uppercase letters.', self.min_upper ), code='too_few_uppercase_char', @@ -321,8 +349,8 @@ class LowercaseValidator(object): return raise ValidationError( ungettext( - 'Your password must contain at least %(min_lower)d lowercase letter.', - 'Your password must contain at least %(min_lower)d lowercase letters.', + 'This password must contain at least %(min_lower)d lowercase letter.', + 'This password must contain at least %(min_lower)d lowercase letters.', self.min_lower ), code='too_few_lowercase_char', @@ -355,11 +383,11 @@ class LowercaseValidator(object): class PunctuationValidator(object): """ - Validate whether the password contains at least min_punctuation punctuation characters + Validate whether the password contains at least min_punctuation punctuation marks as defined by unicode categories. Parameters: - min_punctuation (int): the minimum number of punctuation characters to require + min_punctuation (int): the minimum number of punctuation marks to require in the password. Must be >= 0. """ def __init__(self, min_punctuation=0): @@ -370,8 +398,8 @@ class PunctuationValidator(object): return raise ValidationError( ungettext( - 'Your password must contain at least %(min_punctuation)d punctuation character.', - 'Your password must contain at least %(min_punctuation)d punctuation characters.', + 'This password must contain at least %(min_punctuation)d punctuation mark.', + 'This password must contain at least %(min_punctuation)d punctuation marks.', self.min_punctuation ), code='too_few_punctuation_characters', @@ -380,16 +408,16 @@ class PunctuationValidator(object): def get_help_text(self): return ungettext( - "Your password must contain at least %(min_punctuation)d punctuation character.", - "Your password must contain at least %(min_punctuation)d punctuation characters.", + "Your password must contain at least %(min_punctuation)d punctuation mark.", + "Your password must contain at least %(min_punctuation)d punctuation marks.", self.min_punctuation ) % {'min_punctuation': self.min_punctuation} def get_instruction_text(self): if self.min_punctuation > 0: return ungettext( - '%(num)d punctuation character', - '%(num)d punctuation characters', + '%(num)d punctuation mark', + '%(num)d punctuation marks', self.min_punctuation ) % {'num': self.min_punctuation} else: @@ -418,8 +446,8 @@ class SymbolValidator(object): return raise ValidationError( ungettext( - 'Your password must contain at least %(min_symbol)d symbol.', - 'Your password must contain at least %(min_symbol)d symbols.', + 'This password must contain at least %(min_symbol)d symbol.', + 'This password must contain at least %(min_symbol)d symbols.', self.min_symbol ), code='too_few_symbols', diff --git a/common/djangoapps/util/tests/test_password_policy_validators.py b/common/djangoapps/util/tests/test_password_policy_validators.py index 97b636a358..d037d7557c 100644 --- a/common/djangoapps/util/tests/test_password_policy_validators.py +++ b/common/djangoapps/util/tests/test_password_policy_validators.py @@ -6,37 +6,44 @@ import unittest from ddt import data, ddt, unpack from django.conf import settings +from django.contrib.auth.models import User from django.core.exceptions import ValidationError from django.test.utils import override_settings from util.password_policy_validators import ( - password_instructions, password_min_length, validate_password, _validate_password_dictionary + create_validator_config, validate_password, password_validators_instruction_texts, ) @ddt class PasswordPolicyValidatorsTestCase(unittest.TestCase): - """ Tests for password validator utility functions """ + """ + Tests for password validator utility functions - @override_settings(PASSWORD_DICTIONARY_EDIT_DISTANCE_THRESHOLD=2) - @override_settings(PASSWORD_DICTIONARY=['testme']) - @mock.patch.dict(settings.FEATURES, {'ENFORCE_PASSWORD_POLICY': True}) - def test_validate_password_dictionary(self): - """ Tests dictionary checks """ - # Direct match - with self.assertRaises(ValidationError): - _validate_password_dictionary(u'testme') + The general framework I went with for testing the validators was to test: + 1) requiring a single check (also checks proper singular message) + 2) requiring multiple instances of the check (also checks proper plural message) + 3) successful check + """ - # Off by one - with self.assertRaises(ValidationError): - _validate_password_dictionary(u'estme') + def validation_errors_checker(self, password, msg, user=None): + """ + This helper function is used to check the proper error messages are + being displayed based on the password and validator. - # Off by two - with self.assertRaises(ValidationError): - _validate_password_dictionary(u'bestmet') - - # Off by three (should pass) - _validate_password_dictionary(u'bestem') + Parameters: + password (unicode): the password to validate on + user (django.contrib.auth.models.User): user object to use in validation. + This is an optional parameter unless the validator requires a + user object. + msg (str): The expected ValidationError message + """ + if msg is None: + validate_password(password, user) + else: + with self.assertRaises(ValidationError) as cm: + validate_password(password, user) + self.assertIn(msg, ' '.join(cm.exception.messages)) def test_unicode_password(self): """ Tests that validate_password enforces unicode """ @@ -46,45 +53,193 @@ class PasswordPolicyValidatorsTestCase(unittest.TestCase): # Sanity checks and demonstration of why this test is useful self.assertEqual(len(byte_str), 4) self.assertEqual(len(unicode_str), 1) - self.assertEqual(password_min_length(), 2) # Test length check - with self.assertRaises(ValidationError): - validate_password(byte_str) - validate_password(byte_str + byte_str) + self.validation_errors_checker(byte_str, 'This password is too short. It must contain at least 2 characters.') + self.validation_errors_checker(byte_str + byte_str, None) # Test badly encoded password - with self.assertRaises(ValidationError) as cm: - validate_password(b'\xff\xff') - self.assertEquals('Invalid password.', cm.exception.message) + self.validation_errors_checker(b'\xff\xff', 'Invalid password.') @data( - (u'', 'at least 2 characters & 2 letters & 1 number.'), - (u'a.', 'at least 2 letters & 1 number.'), - (u'a1', 'at least 2 letters.'), - (u'aa1', None), + ([create_validator_config('util.password_policy_validators.MinimumLengthValidator', {'min_length': 2})], + 'at least 2 characters.'), + + ([ + create_validator_config('util.password_policy_validators.MinimumLengthValidator', {'min_length': 2}), + create_validator_config('util.password_policy_validators.AlphabeticValidator', {'min_alphabetic': 2}), + ], 'characters, including 2 letters.'), + + ([ + create_validator_config('util.password_policy_validators.MinimumLengthValidator', {'min_length': 2}), + create_validator_config('util.password_policy_validators.AlphabeticValidator', {'min_alphabetic': 2}), + create_validator_config('util.password_policy_validators.NumericValidator', {'min_numeric': 1}), + ], 'characters, including 2 letters & 1 number.'), + + ([ + create_validator_config('util.password_policy_validators.MinimumLengthValidator', {'min_length': 2}), + create_validator_config('util.password_policy_validators.UppercaseValidator', {'min_upper': 3}), + create_validator_config('util.password_policy_validators.NumericValidator', {'min_numeric': 1}), + create_validator_config('util.password_policy_validators.SymbolValidator', {'min_symbol': 2}), + ], 'including 3 uppercase letters & 1 number & 2 symbols.'), ) @unpack - @override_settings(PASSWORD_COMPLEXITY={'ALPHABETIC': 2, 'NUMERIC': 1}) - @mock.patch.dict(settings.FEATURES, {'ENFORCE_PASSWORD_POLICY': True}) - def test_validation_errors(self, password, msg): - """ Tests validate_password error messages """ - if msg is None: - validate_password(password) - else: - with self.assertRaises(ValidationError) as cm: - validate_password(password) - self.assertIn(msg, cm.exception.message) + def test_password_instructions(self, config, msg): + """ Tests password instructions """ + with override_settings(AUTH_PASSWORD_VALIDATORS=config): + self.assertIn(msg, password_validators_instruction_texts()) @data( - ({}, 'at least 2 characters.'), - ({'ALPHABETIC': 2}, 'characters, including 2 letters.'), - ({'ALPHABETIC': 2, 'NUMERIC': 1}, 'characters, including 2 letters & 1 number.'), - ({'NON ASCII': 2, 'NUMERIC': 1, 'UPPER': 3}, 'including 3 uppercase letters & 1 number & 2 symbols.'), + (u'userna', u'username', 'test@example.com', 'The password is too similar to the username.'), + (u'password', u'username', 'password@example.com', 'The password is too similar to the email address.'), + (u'password', u'username', 'test@password.com', 'The password is too similar to the email address.'), + (u'password', u'username', 'test@example.com', None), ) @unpack - @mock.patch.dict(settings.FEATURES, {'ENFORCE_PASSWORD_POLICY': True}) - def test_password_instruction(self, config, msg): - """ Tests password_instruction """ - with override_settings(PASSWORD_COMPLEXITY=config): - self.assertIn(msg, password_instructions()) + @override_settings(AUTH_PASSWORD_VALIDATORS=[ + create_validator_config('django.contrib.auth.password_validation.UserAttributeSimilarityValidator') + ]) + def test_user_attribute_similarity_validation_errors(self, password, username, email, msg): + """ Tests validate_password error messages for the UserAttributeSimilarityValidator """ + user = User(username=username, email=email) + self.validation_errors_checker(password, msg, user) + + @data( + ([create_validator_config('util.password_policy_validators.MinimumLengthValidator', {'min_length': 1})], + u'', 'This password is too short. It must contain at least 1 character.'), + + ([create_validator_config('util.password_policy_validators.MinimumLengthValidator', {'min_length': 8})], + u'd', 'This password is too short. It must contain at least 8 characters.'), + + ([create_validator_config('util.password_policy_validators.MinimumLengthValidator', {'min_length': 8})], + u'longpassword', None), + ) + @unpack + def test_minimum_length_validation_errors(self, config, password, msg): + """ Tests validate_password error messages for the MinimumLengthValidator """ + with override_settings(AUTH_PASSWORD_VALIDATORS=config): + self.validation_errors_checker(password, msg) + + @data( + ([create_validator_config('util.password_policy_validators.MaximumLengthValidator', {'max_length': 1})], + u'longpassword', 'This password is too long. It must contain no more than 1 character.'), + + ([create_validator_config('util.password_policy_validators.MaximumLengthValidator', {'max_length': 10})], + u'longpassword', 'This password is too long. It must contain no more than 10 characters.'), + + ([create_validator_config('util.password_policy_validators.MaximumLengthValidator', {'max_length': 20})], + u'shortpassword', None), + ) + @unpack + def test_maximum_length_validation_errors(self, config, password, msg): + """ Tests validate_password error messages for the MaximumLengthValidator """ + with override_settings(AUTH_PASSWORD_VALIDATORS=config): + self.validation_errors_checker(password, msg) + + @data( + (u'password', 'This password is too common.'), + (u'good_password', None), + ) + @unpack + @override_settings(AUTH_PASSWORD_VALIDATORS=[ + create_validator_config('django.contrib.auth.password_validation.CommonPasswordValidator') + ]) + def test_common_password_validation_errors(self, password, msg): + """ Tests validate_password error messages for the CommonPasswordValidator """ + self.validation_errors_checker(password, msg) + + @data( + ([create_validator_config('util.password_policy_validators.AlphabeticValidator', {'min_alphabetic': 1})], + u'12345', 'This password must contain at least 1 letter.'), + + ([create_validator_config('util.password_policy_validators.AlphabeticValidator', {'min_alphabetic': 5})], + u'test123', 'This password must contain at least 5 letters.'), + + ([create_validator_config('util.password_policy_validators.AlphabeticValidator', {'min_alphabetic': 2})], + u'password', None), + ) + @unpack + def test_alphabetic_validation_errors(self, config, password, msg): + """ Tests validate_password error messages for the AlphabeticValidator """ + with override_settings(AUTH_PASSWORD_VALIDATORS=config): + self.validation_errors_checker(password, msg) + + @data( + ([create_validator_config('util.password_policy_validators.NumericValidator', {'min_numeric': 1})], + u'test', 'This password must contain at least 1 number.'), + + ([create_validator_config('util.password_policy_validators.NumericValidator', {'min_numeric': 4})], + u'test123', 'This password must contain at least 4 numbers.'), + + ([create_validator_config('util.password_policy_validators.NumericValidator', {'min_numeric': 2})], + u'password123', None), + ) + @unpack + def test_numeric_validation_errors(self, config, password, msg): + """ Tests validate_password error messages for the NumericValidator """ + with override_settings(AUTH_PASSWORD_VALIDATORS=config): + self.validation_errors_checker(password, msg) + + @data( + ([create_validator_config('util.password_policy_validators.UppercaseValidator', {'min_upper': 1})], + u'lowercase', 'This password must contain at least 1 uppercase letter.'), + + ([create_validator_config('util.password_policy_validators.UppercaseValidator', {'min_upper': 6})], + u'NOTenough', 'This password must contain at least 6 uppercase letters.'), + + ([create_validator_config('util.password_policy_validators.UppercaseValidator', {'min_upper': 1})], + u'camelCase', None), + ) + @unpack + def test_upper_case_validation_errors(self, config, password, msg): + """ Tests validate_password error messages for the UppercaseValidator """ + with override_settings(AUTH_PASSWORD_VALIDATORS=config): + self.validation_errors_checker(password, msg) + + @data( + ([create_validator_config('util.password_policy_validators.LowercaseValidator', {'min_lower': 1})], + u'UPPERCASE', 'This password must contain at least 1 lowercase letter.'), + + ([create_validator_config('util.password_policy_validators.LowercaseValidator', {'min_lower': 4})], + u'notENOUGH', 'This password must contain at least 4 lowercase letters.'), + + ([create_validator_config('util.password_policy_validators.LowercaseValidator', {'min_lower': 1})], + u'goodPassword', None), + ) + @unpack + def test_lower_case_validation_errors(self, config, password, msg): + """ Tests validate_password error messages for the LowercaseValidator """ + with override_settings(AUTH_PASSWORD_VALIDATORS=config): + self.validation_errors_checker(password, msg) + + @data( + ([create_validator_config('util.password_policy_validators.PunctuationValidator', {'min_punctuation': 1})], + u'no punctuation', 'This password must contain at least 1 punctuation mark.'), + + ([create_validator_config('util.password_policy_validators.PunctuationValidator', {'min_punctuation': 7})], + u'p@$$w0rd$!', 'This password must contain at least 7 punctuation marks.'), + + ([create_validator_config('util.password_policy_validators.PunctuationValidator', {'min_punctuation': 3})], + u'excl@m@t!on', None), + ) + @unpack + def test_punctuation_validation_errors(self, config, password, msg): + """ Tests validate_password error messages for the PunctuationValidator """ + with override_settings(AUTH_PASSWORD_VALIDATORS=config): + self.validation_errors_checker(password, msg) + + @data( + ([create_validator_config('util.password_policy_validators.SymbolValidator', {'min_symbol': 1})], + u'no symbol', 'This password must contain at least 1 symbol.'), + + ([create_validator_config('util.password_policy_validators.SymbolValidator', {'min_symbol': 3})], + u'☹️boo☹️', 'This password must contain at least 3 symbols.'), + + ([create_validator_config('util.password_policy_validators.SymbolValidator', {'min_symbol': 2})], + u'☪symbols!☹️', None), + ) + @unpack + def test_symbol_validation_errors(self, config, password, msg): + """ Tests validate_password error messages for the SymbolValidator """ + with override_settings(AUTH_PASSWORD_VALIDATORS=config): + self.validation_errors_checker(password, msg) diff --git a/common/test/acceptance/tests/studio/test_studio_course_team.py b/common/test/acceptance/tests/studio/test_studio_course_team.py index 09d02065b0..e4b1a56d22 100644 --- a/common/test/acceptance/tests/studio/test_studio_course_team.py +++ b/common/test/acceptance/tests/studio/test_studio_course_team.py @@ -16,7 +16,7 @@ class CourseTeamPageTest(StudioCourseTest): user = { 'username': username, 'email': username + "@example.com", - 'password': username + '123' + 'password': username + '123$%^' } AutoAuthPage( self.browser, no_login=True, diff --git a/lms/djangoapps/courseware/tests/test_password_history.py b/lms/djangoapps/courseware/tests/test_password_history.py index 5bf49b2c4b..bad1f5dd8b 100644 --- a/lms/djangoapps/courseware/tests/test_password_history.py +++ b/lms/djangoapps/courseware/tests/test_password_history.py @@ -17,6 +17,7 @@ from mock import patch from courseware.tests.helpers import LoginEnrollmentTestCase from student.models import PasswordHistory +from util.password_policy_validators import create_validator_config @patch.dict("django.conf.settings.FEATURES", {'ADVANCED_SECURITY': True}) @@ -161,148 +162,9 @@ class TestPasswordHistory(LoginEnrollmentTestCase): resp.content ) - @patch.dict("django.conf.settings.ADVANCED_SECURITY_CONFIG", {'MIN_DIFFERENT_STUDENT_PASSWORDS_BEFORE_REUSE': 1}) - def test_student_password_reset_reuse(self): - """ - Goes through the password reset flows to make sure the various password reuse policies are enforced - """ - student_email, _ = self._setup_user() - user = User.objects.get(email=student_email) - - err_msg = 'You are re\\\\u002Dusing a password that you have used recently. You must have 1 distinct password' - success_msg = 'Your Password Reset is Complete' - - token = default_token_generator.make_token(user) - uidb36 = int_to_base36(user.id) - - # try to do a password reset with the same password as before - resp = self.client.post('/password_reset_confirm/{0}-{1}/'.format(uidb36, token), { - 'new_password1': 'foo', - 'new_password2': 'foo' - }, follow=True) - - self.assertPasswordResetError(resp, err_msg) - - # now retry with a different password - resp = self.client.post('/password_reset_confirm/{0}-{1}/'.format(uidb36, token), { - 'new_password1': 'bar', - 'new_password2': 'bar' - }, follow=True) - - self.assertIn(success_msg, resp.content) - - @patch.dict("django.conf.settings.ADVANCED_SECURITY_CONFIG", {'MIN_DIFFERENT_STAFF_PASSWORDS_BEFORE_REUSE': 2}) - def test_staff_password_reset_reuse(self): - """ - Goes through the password reset flows to make sure the various password reuse policies are enforced - """ - staff_email, _ = self._setup_user(is_staff=True) - user = User.objects.get(email=staff_email) - - err_msg = 'You are re\\\\u002Dusing a password that you have used recently. You must have 2 distinct passwords' - success_msg = 'Your Password Reset is Complete' - - token = default_token_generator.make_token(user) - uidb36 = int_to_base36(user.id) - - # try to do a password reset with the same password as before - resp = self.client.post('/password_reset_confirm/{0}-{1}/'.format(uidb36, token), { - 'new_password1': 'foo', - 'new_password2': 'foo', - }, follow=True) - - self.assertPasswordResetError(resp, err_msg) - - # now use different one - user = User.objects.get(email=staff_email) - token = default_token_generator.make_token(user) - uidb36 = int_to_base36(user.id) - - resp = self.client.post('/password_reset_confirm/{0}-{1}/'.format(uidb36, token), { - 'new_password1': 'bar', - 'new_password2': 'bar', - }, follow=True) - - self.assertIn(success_msg, resp.content) - - # now try again with the first one - user = User.objects.get(email=staff_email) - token = default_token_generator.make_token(user) - uidb36 = int_to_base36(user.id) - - resp = self.client.post('/password_reset_confirm/{0}-{1}/'.format(uidb36, token), { - 'new_password1': 'foo', - 'new_password2': 'foo', - }, follow=True) - - self.assertPasswordResetError(resp, err_msg) - - # now use different one - user = User.objects.get(email=staff_email) - token = default_token_generator.make_token(user) - uidb36 = int_to_base36(user.id) - - resp = self.client.post('/password_reset_confirm/{0}-{1}/'.format(uidb36, token), { - 'new_password1': 'baz', - 'new_password2': 'baz', - }, follow=True) - - self.assertIn(success_msg, resp.content) - - # now we should be able to reuse the first one - user = User.objects.get(email=staff_email) - token = default_token_generator.make_token(user) - uidb36 = int_to_base36(user.id) - - resp = self.client.post('/password_reset_confirm/{0}-{1}/'.format(uidb36, token), { - 'new_password1': 'foo', - 'new_password2': 'foo', - }, follow=True) - - self.assertIn(success_msg, resp.content) - - @patch.dict("django.conf.settings.ADVANCED_SECURITY_CONFIG", {'MIN_TIME_IN_DAYS_BETWEEN_ALLOWED_RESETS': 1}) - def test_password_reset_frequency_limit(self): - """ - Asserts the frequency limit on how often we can change passwords - """ - staff_email, _ = self._setup_user(is_staff=True) - - success_msg = 'Your Password Reset is Complete' - - # try to reset password, it should fail - user = User.objects.get(email=staff_email) - token = default_token_generator.make_token(user) - uidb36 = int_to_base36(user.id) - - # try to do a password reset with the same password as before - resp = self.client.post('/password_reset_confirm/{0}-{1}/'.format(uidb36, token), { - 'new_password1': 'foo', - 'new_password2': 'foo', - }, follow=True) - - self.assertNotIn( - success_msg, - resp.content - ) - - # pretend we're in the future - staff_reset_time = timezone.now() + timedelta(days=1) - with freeze_time(staff_reset_time): - user = User.objects.get(email=staff_email) - token = default_token_generator.make_token(user) - uidb36 = int_to_base36(user.id) - - # try to do a password reset with the same password as before - resp = self.client.post('/password_reset_confirm/{0}-{1}/'.format(uidb36, token), { - 'new_password1': 'foo', - 'new_password2': 'foo', - }, follow=True) - - self.assertIn(success_msg, resp.content) - - @patch.dict("django.conf.settings.FEATURES", {'ENFORCE_PASSWORD_POLICY': True}) - @override_settings(PASSWORD_MIN_LENGTH=6) + @override_settings(AUTH_PASSWORD_VALIDATORS=[ + create_validator_config('util.password_policy_validators.MinimumLengthValidator', {'min_length': 6}) + ]) def test_password_policy_on_password_reset(self): """ This makes sure the proper asserts on password policy also works on password reset @@ -342,7 +204,7 @@ class TestPasswordHistory(LoginEnrollmentTestCase): @ddt.data( ('foo', 'foobar', 'Error in resetting your password. Please try again.'), - ('', '', 'Enter a password with at least'), + ('', '', 'This password is too short. It must contain at least'), ) @ddt.unpack def test_password_reset_form_invalid(self, password1, password2, err_msg): diff --git a/lms/envs/aws.py b/lms/envs/aws.py index 4f56c9930d..37f68af3e9 100644 --- a/lms/envs/aws.py +++ b/lms/envs/aws.py @@ -652,7 +652,7 @@ MAX_FAILED_LOGIN_ATTEMPTS_ALLOWED = ENV_TOKENS.get("MAX_FAILED_LOGIN_ATTEMPTS_AL MAX_FAILED_LOGIN_ATTEMPTS_LOCKOUT_PERIOD_SECS = ENV_TOKENS.get("MAX_FAILED_LOGIN_ATTEMPTS_LOCKOUT_PERIOD_SECS", 15 * 60) #### PASSWORD POLICY SETTINGS ##### -AUTH_PASSWORD_VALIDATORS = ENV_TOKENS.get("AUTH_PASSWORD_VALIDATORS", []) +AUTH_PASSWORD_VALIDATORS = ENV_TOKENS.get("AUTH_PASSWORD_VALIDATORS", AUTH_PASSWORD_VALIDATORS) ### INACTIVITY SETTINGS #### SESSION_INACTIVITY_TIMEOUT_IN_SECONDS = AUTH_TOKENS.get("SESSION_INACTIVITY_TIMEOUT_IN_SECONDS") diff --git a/lms/envs/bok_choy.py b/lms/envs/bok_choy.py index b5882fdffe..74381db39f 100644 --- a/lms/envs/bok_choy.py +++ b/lms/envs/bok_choy.py @@ -173,7 +173,6 @@ YOUTUBE['TEXT_API']['url'] = "{0}:{1}/test_transcripts_youtube/".format(YOUTUBE_ ############################# SECURITY SETTINGS ################################ # Default to advanced security in common.py, so tests can reset here to use # a simpler security model -FEATURES['ENFORCE_PASSWORD_POLICY'] = False FEATURES['ENABLE_MAX_FAILED_LOGIN_ATTEMPTS'] = False FEATURES['SQUELCH_PII_IN_LOGS'] = False FEATURES['PREVENT_CONCURRENT_LOGINS'] = False @@ -183,9 +182,6 @@ FEATURES['ENABLE_MOBILE_REST_API'] = True # Show video bumper in LMS FEATURES['ENABLE_VIDEO_BUMPER'] = True # Show video bumper in LMS FEATURES['SHOW_BUMPER_PERIODICITY'] = 1 -PASSWORD_MIN_LENGTH = None -PASSWORD_COMPLEXITY = {} - # Enable courseware search for tests FEATURES['ENABLE_COURSEWARE_SEARCH'] = True diff --git a/lms/envs/common.py b/lms/envs/common.py index 9d44e68556..6c441558fb 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -202,9 +202,6 @@ FEATURES = { # Maximum number of rows to include in the csv file for downloading problem responses. 'MAX_PROBLEM_RESPONSES_COUNT': 5000, - # whether to use password policy enforcement or not - 'ENFORCE_PASSWORD_POLICY': True, - 'ENABLED_PAYMENT_REPORTS': [ "refund_report", "itemized_purchase_report", @@ -2594,11 +2591,23 @@ FINANCIAL_REPORTS = { POLICY_CHANGE_TASK_RATE_LIMIT = '300/h' #### PASSWORD POLICY SETTINGS ##### -PASSWORD_MIN_LENGTH = 8 -PASSWORD_MAX_LENGTH = None -PASSWORD_COMPLEXITY = {"UPPER": 1, "LOWER": 1, "DIGITS": 1} -PASSWORD_DICTIONARY_EDIT_DISTANCE_THRESHOLD = None -PASSWORD_DICTIONARY = [] +AUTH_PASSWORD_VALIDATORS = [ + { + "NAME": "django.contrib.auth.password_validation.UserAttributeSimilarityValidator", + }, + { + "NAME": "util.password_policy_validators.MinimumLengthValidator", + "OPTIONS": { + "min_length": 2 + } + }, + { + "NAME": "util.password_policy_validators.MaximumLengthValidator", + "OPTIONS": { + "max_length": 75 + } + }, +] ############################ ORA 2 ############################################ diff --git a/lms/envs/devstack.py b/lms/envs/devstack.py index 1f811dec70..c625c88c51 100644 --- a/lms/envs/devstack.py +++ b/lms/envs/devstack.py @@ -137,7 +137,6 @@ FEATURES['ENABLE_MOBILE_REST_API'] = True FEATURES['ENABLE_VIDEO_ABSTRACTION_LAYER_API'] = True ########################## SECURITY ####################### -FEATURES['ENFORCE_PASSWORD_POLICY'] = False FEATURES['ENABLE_MAX_FAILED_LOGIN_ATTEMPTS'] = False FEATURES['SQUELCH_PII_IN_LOGS'] = False FEATURES['PREVENT_CONCURRENT_LOGINS'] = False diff --git a/lms/envs/production.py b/lms/envs/production.py index 5929dacf88..6f229d23bd 100644 --- a/lms/envs/production.py +++ b/lms/envs/production.py @@ -648,11 +648,7 @@ MAX_FAILED_LOGIN_ATTEMPTS_ALLOWED = ENV_TOKENS.get("MAX_FAILED_LOGIN_ATTEMPTS_AL MAX_FAILED_LOGIN_ATTEMPTS_LOCKOUT_PERIOD_SECS = ENV_TOKENS.get("MAX_FAILED_LOGIN_ATTEMPTS_LOCKOUT_PERIOD_SECS", 15 * 60) #### PASSWORD POLICY SETTINGS ##### -PASSWORD_MIN_LENGTH = ENV_TOKENS.get("PASSWORD_MIN_LENGTH") -PASSWORD_MAX_LENGTH = ENV_TOKENS.get("PASSWORD_MAX_LENGTH") -PASSWORD_COMPLEXITY = ENV_TOKENS.get("PASSWORD_COMPLEXITY", {}) -PASSWORD_DICTIONARY_EDIT_DISTANCE_THRESHOLD = ENV_TOKENS.get("PASSWORD_DICTIONARY_EDIT_DISTANCE_THRESHOLD") -PASSWORD_DICTIONARY = ENV_TOKENS.get("PASSWORD_DICTIONARY", []) +AUTH_PASSWORD_VALIDATORS = ENV_TOKENS.get("AUTH_PASSWORD_VALIDATORS", AUTH_PASSWORD_VALIDATORS) ### INACTIVITY SETTINGS #### SESSION_INACTIVITY_TIMEOUT_IN_SECONDS = AUTH_TOKENS.get("SESSION_INACTIVITY_TIMEOUT_IN_SECONDS") diff --git a/lms/envs/test.py b/lms/envs/test.py index d2afa71876..08cfdd4e7e 100644 --- a/lms/envs/test.py +++ b/lms/envs/test.py @@ -223,8 +223,6 @@ FEATURES['ENABLE_MAX_FAILED_LOGIN_ATTEMPTS'] = False FEATURES['SQUELCH_PII_IN_LOGS'] = False FEATURES['PREVENT_CONCURRENT_LOGINS'] = False FEATURES['ADVANCED_SECURITY'] = False -PASSWORD_MIN_LENGTH = None -PASSWORD_COMPLEXITY = {} ######### Third-party auth ########## FEATURES['ENABLE_THIRD_PARTY_AUTH'] = True diff --git a/lms/static/js/student_account/views/RegisterView.js b/lms/static/js/student_account/views/RegisterView.js index 034f113e55..9daa4720c7 100644 --- a/lms/static/js/student_account/views/RegisterView.js +++ b/lms/static/js/student_account/views/RegisterView.js @@ -195,7 +195,9 @@ // Hide each input tip $(this).children().each(function() { - if (inputTipSelectors.indexOf($(this).attr('class')) >= 0) { + // This is a 1 instead of 0 so the error message for a field is not + // hidden on blur and only the help tip is hidden. + if (inputTipSelectors.indexOf($(this).attr('class')) >= 1) { $(this).addClass('hidden'); } }); diff --git a/openedx/core/djangoapps/password_policy/compliance.py b/openedx/core/djangoapps/password_policy/compliance.py index 53326b9957..0447060e1d 100644 --- a/openedx/core/djangoapps/password_policy/compliance.py +++ b/openedx/core/djangoapps/password_policy/compliance.py @@ -8,7 +8,7 @@ from django.conf import settings from django.utils.translation import ugettext as _ from util.date_utils import DEFAULT_SHORT_DATE_FORMAT, strftime_localized -from util.password_policy_validators import edX_validate_password +from util.password_policy_validators import validate_password class NonCompliantPasswordException(Exception): @@ -104,7 +104,7 @@ def _check_user_compliance(user, password): Returns a boolean indicating whether or not the user is compliant with password policy rules. """ try: - edX_validate_password(password, user=user) + validate_password(password, user=user) return True except Exception: # pylint: disable=broad-except # If anything goes wrong, we should assume the password is not compliant but we don't necessarily diff --git a/openedx/core/djangoapps/password_policy/tests/test_compliance.py b/openedx/core/djangoapps/password_policy/tests/test_compliance.py index 3a68c7c185..e05b371290 100644 --- a/openedx/core/djangoapps/password_policy/tests/test_compliance.py +++ b/openedx/core/djangoapps/password_policy/tests/test_compliance.py @@ -16,7 +16,7 @@ from openedx.core.djangoapps.password_policy.compliance import (NonCompliantPass should_enforce_compliance_on_login) from student.tests.factories import (CourseAccessRoleFactory, UserFactory) -from util.password_policy_validators import SecurityPolicyError, ValidationError, validate_password +from util.password_policy_validators import ValidationError date1 = parse_date('2018-01-01 00:00:00+00:00') @@ -93,36 +93,21 @@ class TestCompliance(TestCase): """ # Test that a user that passes validate_password returns True - with patch('openedx.core.djangoapps.password_policy.compliance.validate_password') as mock_validate_password: + with patch('openedx.core.djangoapps.password_policy.compliance.validate_password') as \ + mock_validate_password: user = UserFactory() # Mock validate_password to return True without checking the password mock_validate_password.return_value = True self.assertTrue(_check_user_compliance(user, None)) # Don't need a password here # Test that a user that does not pass validate_password returns False - with patch('openedx.core.djangoapps.password_policy.compliance.validate_password') as mock_validate_password: + with patch('openedx.core.djangoapps.password_policy.compliance.validate_password') as \ + mock_validate_password: user = UserFactory() # Mock validate_password to throw a ValidationError without checking the password mock_validate_password.side_effect = ValidationError('Some validation error') self.assertFalse(_check_user_compliance(user, None)) # Don't need a password here - @patch('student.models.PasswordHistory.is_allowable_password_reuse') - @override_settings(ADVANCED_SECURITY_CONFIG={'MIN_DIFFERENT_STUDENT_PASSWORDS_BEFORE_REUSE': 1}) - def test_ignore_reset_checks(self, mock_reuse): - """ - Test that we don't annoy user about compliance failures that only affect password resets - """ - user = UserFactory() - password = 'nope1234' - mock_reuse.return_value = False - - # Sanity check that normal validation would trip us up - with self.assertRaises(SecurityPolicyError): - validate_password(password, user=user) - - # Confirm that we don't trip on it - self.assertTrue(_check_user_compliance(user, password)) - @override_settings(PASSWORD_POLICY_COMPLIANCE_ROLLOUT_CONFIG={ 'STAFF_USER_COMPLIANCE_DEADLINE': date1, 'ELEVATED_PRIVILEGE_USER_COMPLIANCE_DEADLINE': date2, diff --git a/openedx/core/djangoapps/user_api/accounts/api.py b/openedx/core/djangoapps/user_api/accounts/api.py index 25b62aabb3..3f08e68e1f 100644 --- a/openedx/core/djangoapps/user_api/accounts/api.py +++ b/openedx/core/djangoapps/user_api/accounts/api.py @@ -18,7 +18,7 @@ from student.models import User, UserProfile, Registration, email_exists_or_reti from student import forms as student_forms from student import views as student_views from util.model_utils import emit_setting_changed_event -from util.password_policy_validators import edX_validate_password +from util.password_policy_validators import validate_password from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.user_api import errors, accounts, forms, helpers @@ -287,7 +287,6 @@ def create_account(username, password, email): * 3rd party auth * External auth (shibboleth) - * Complex password policies (ENFORCE_PASSWORD_POLICY) In addition, we assume that some functionality is handled at higher layers: @@ -665,7 +664,7 @@ def _validate_password(password, username=None, email=None): try: _validate_type(password, basestring, accounts.PASSWORD_BAD_TYPE_MSG) temp_user = User(username=username, email=email) if username else None - edX_validate_password(password, user=temp_user) + validate_password(password, user=temp_user) except errors.AccountDataBadType as invalid_password_err: raise errors.AccountPasswordInvalid(text_type(invalid_password_err)) except ValidationError as validation_err: diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_api.py b/openedx/core/djangoapps/user_api/accounts/tests/test_api.py index 5c81225196..bd70aaebb4 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_api.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_api.py @@ -387,9 +387,9 @@ class AccountCreationActivationAndPasswordChangeTest(TestCase): """ Test cases to cover the account initialization workflow """ - USERNAME = u'frank-underwood' + USERNAME = u'claire-underwood' PASSWORD = u'ṕáśśẃőŕd' - EMAIL = u'frank+underwood@example.com' + EMAIL = u'claire+underwood@example.com' IS_SECURE = False diff --git a/openedx/core/djangoapps/user_api/accounts/tests/testutils.py b/openedx/core/djangoapps/user_api/accounts/tests/testutils.py index 97e565719c..37d1c47efc 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/testutils.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/testutils.py @@ -7,7 +7,7 @@ from openedx.core.djangoapps.user_api.accounts import ( USERNAME_MIN_LENGTH, USERNAME_MAX_LENGTH, EMAIL_MAX_LENGTH, ) -from util.password_policy_validators import password_max_length, password_min_length +from util.password_policy_validators import DEFAULT_MAX_PASSWORD_LENGTH INVALID_NAMES = [ @@ -55,7 +55,7 @@ INVALID_PASSWORDS = [ None, u'', u'a', - u'a' * (password_max_length() + 1) + u'a' * (DEFAULT_MAX_PASSWORD_LENGTH + 1), ] INVALID_COUNTRIES = [ @@ -92,9 +92,7 @@ VALID_EMAILS = [ ] VALID_PASSWORDS = [ - u'password', # :) - u'a' * password_min_length(), - u'a' * password_max_length() + u'good_password_339', ] VALID_COUNTRIES = [ diff --git a/openedx/core/djangoapps/user_api/api.py b/openedx/core/djangoapps/user_api/api.py index 3a349409d5..dd7c6d8194 100644 --- a/openedx/core/djangoapps/user_api/api.py +++ b/openedx/core/djangoapps/user_api/api.py @@ -18,7 +18,7 @@ from openedx.features.enterprise_support.api import enterprise_customer_for_requ from student.forms import get_registration_extension_form from student.models import UserProfile from util.password_policy_validators import ( - password_validators_instruction_texts, password_validators_restrictions + password_validators_instruction_texts, password_validators_restrictions, DEFAULT_MAX_PASSWORD_LENGTH, ) @@ -118,10 +118,7 @@ def get_login_session_form(request): "password", label=password_label, field_type="password", - # The following restriction contains the assumption that the max password length will never exceed 5000 - # characters. The point of this restriction on the login page is to prevent any sort of attacks - # involving sending massive passwords. - restrictions={'max_length': 5000} + restrictions={'max_length': DEFAULT_MAX_PASSWORD_LENGTH} ) form_desc.add_field( diff --git a/openedx/core/djangoapps/user_api/tests/test_views.py b/openedx/core/djangoapps/user_api/tests/test_views.py index 37eda24941..56a1d2a571 100644 --- a/openedx/core/djangoapps/user_api/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/tests/test_views.py @@ -32,7 +32,10 @@ from third_party_auth.tests.testutil import simulate_running_pipeline, ThirdPart from third_party_auth.tests.utils import ( ThirdPartyOAuthTestMixin, ThirdPartyOAuthTestMixinFacebook, ThirdPartyOAuthTestMixinGoogle ) -from util.password_policy_validators import password_max_length, password_min_length +from util.password_policy_validators import ( + create_validator_config, password_validators_instruction_texts, password_validators_restrictions, + DEFAULT_MAX_PASSWORD_LENGTH, +) from .test_helpers import TestCaseForm from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory @@ -620,7 +623,7 @@ class LoginSessionViewTest(UserAPITestCase): "placeholder": "", "instructions": "", "restrictions": { - "max_length": password_max_length(), + "max_length": DEFAULT_MAX_PASSWORD_LENGTH, }, "errorMessages": {}, "supplementalText": "", @@ -1186,15 +1189,16 @@ class RegistrationViewTest(ThirdPartyAuthTestMixin, UserAPITestCase): u"type": u"password", u"required": True, u"label": u"Password", - u"instructions": u'Your password must contain at least {} characters.'.format(password_min_length()), - u"restrictions": { - 'min_length': password_min_length(), - 'max_length': password_max_length(), - }, + u"instructions": password_validators_instruction_texts(), + u"restrictions": password_validators_restrictions(), } ) - @override_settings(PASSWORD_COMPLEXITY={'NON ASCII': 1, 'UPPER': 3}) + @override_settings(AUTH_PASSWORD_VALIDATORS=[ + create_validator_config('util.password_policy_validators.MinimumLengthValidator', {'min_length': 2}), + create_validator_config('util.password_policy_validators.UppercaseValidator', {'min_upper': 3}), + create_validator_config('util.password_policy_validators.SymbolValidator', {'min_symbol': 1}), + ]) def test_register_form_password_complexity(self): no_extra_fields_setting = {} @@ -1204,32 +1208,22 @@ class RegistrationViewTest(ThirdPartyAuthTestMixin, UserAPITestCase): { u'name': u'password', u'label': u'Password', - u'instructions': u'Your password must contain at least {} characters.'.format(password_min_length()), - u'restrictions': { - 'min_length': password_min_length(), - 'max_length': password_max_length(), - }, + u"instructions": password_validators_instruction_texts(), + u"restrictions": password_validators_restrictions(), } ) - # Now with an enabled password policy - with mock.patch.dict(settings.FEATURES, {'ENFORCE_PASSWORD_POLICY': True}): - msg = u'Your password must contain at least {} characters, including '\ - u'3 uppercase letters & 1 symbol.'.format(password_min_length()) - self._assert_reg_field( - no_extra_fields_setting, - { - u'name': u'password', - u'label': u'Password', - u'instructions': msg, - u'restrictions': { - 'min_length': password_min_length(), - 'max_length': password_max_length(), - 'non_ascii': 1, - 'upper': 3, - }, - } - ) + msg = u'Your password must contain at least 2 characters, including '\ + u'3 uppercase letters & 1 symbol.' + self._assert_reg_field( + no_extra_fields_setting, + { + u'name': u'password', + u'label': u'Password', + u'instructions': msg, + u"restrictions": password_validators_restrictions(), + } + ) @override_settings(REGISTRATION_EXTENSION_FORM='openedx.core.djangoapps.user_api.tests.test_helpers.TestCaseForm') def test_extension_form_fields(self): @@ -2314,7 +2308,7 @@ class RegistrationViewTest(ThirdPartyAuthTestMixin, UserAPITestCase): response_json, { u"username": [{u"user_message": USERNAME_BAD_LENGTH_MSG}], - u"password": [{u"user_message": u"A valid password is required"}], + u"password": [{u"user_message": u"This field is required."}], } ) diff --git a/openedx/core/djangoapps/user_api/validation/tests/test_views.py b/openedx/core/djangoapps/user_api/validation/tests/test_views.py index c9e02b8162..8046fa57a3 100644 --- a/openedx/core/djangoapps/user_api/validation/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/validation/tests/test_views.py @@ -16,7 +16,7 @@ from openedx.core.djangoapps.user_api import accounts from openedx.core.djangoapps.user_api.accounts.tests import testutils from openedx.core.lib.api import test_utils from openedx.core.djangoapps.user_api.validation.views import RegistrationValidationThrottle -from util.password_policy_validators import password_max_length, password_min_length +from util.password_policy_validators import DEFAULT_MAX_PASSWORD_LENGTH @ddt.ddt @@ -174,23 +174,29 @@ class RegistrationValidationViewTests(test_utils.ApiTestCase): ) def test_password_empty_validation_decision(self): - msg = u'Enter a password with at least {0} characters.'.format(password_min_length()) + # 2 is the default setting for minimum length found in lms/envs/common.py + # under AUTH_PASSWORD_VALIDATORS.MinimumLengthValidator + msg = u'This password is too short. It must contain at least 2 characters.' self.assertValidationDecision( {'password': ''}, {"password": msg} ) def test_password_bad_min_length_validation_decision(self): - password = 'p' * (password_min_length() - 1) - msg = u'Enter a password with at least {0} characters.'.format(password_min_length()) + password = 'p' + # 2 is the default setting for minimum length found in lms/envs/common.py + # under AUTH_PASSWORD_VALIDATORS.MinimumLengthValidator + msg = u'This password is too short. It must contain at least 2 characters.' self.assertValidationDecision( {'password': password}, {"password": msg} ) def test_password_bad_max_length_validation_decision(self): - password = 'p' * (password_max_length() + 1) - msg = u'Enter a password with at most {0} characters.'.format(password_max_length()) + password = 'p' * DEFAULT_MAX_PASSWORD_LENGTH + # 75 is the default setting for maximum length found in lms/envs/common.py + # under AUTH_PASSWORD_VALIDATORS.MaximumLengthValidator + msg = u'This password is too long. It must contain no more than 75 characters.' self.assertValidationDecision( {'password': password}, {"password": msg} @@ -199,7 +205,7 @@ class RegistrationValidationViewTests(test_utils.ApiTestCase): def test_password_equals_username_validation_decision(self): self.assertValidationDecision( {"username": "somephrase", "password": "somephrase"}, - {"username": "", "password": u"Password cannot be the same as the username."} + {"username": "", "password": u"The password is too similar to the username."} ) @override_settings( diff --git a/openedx/core/djangoapps/user_authn/views/deprecated.py b/openedx/core/djangoapps/user_authn/views/deprecated.py index 79bc813a78..4d2f496384 100644 --- a/openedx/core/djangoapps/user_authn/views/deprecated.py +++ b/openedx/core/djangoapps/user_authn/views/deprecated.py @@ -142,7 +142,7 @@ def create_account(request, post_override=None): { "success": False, "field": field, - "value": error_list[0], + "value": ' '.join(error_list), }, status=400 ) diff --git a/openedx/core/djangoapps/user_authn/views/register.py b/openedx/core/djangoapps/user_authn/views/register.py index 951cf9120b..66009b957b 100644 --- a/openedx/core/djangoapps/user_authn/views/register.py +++ b/openedx/core/djangoapps/user_authn/views/register.py @@ -137,7 +137,6 @@ def create_account_with_params(request, params): do_external_auth, eamap = pre_account_creation_external_auth(request, params) extended_profile_fields = configuration_helpers.get_value('extended_profile_fields', []) - enforce_password_policy = not do_external_auth # Can't have terms of service for certain SHIB users, like at Stanford registration_fields = getattr(settings, 'REGISTRATION_EXTRA_FIELDS', {}) tos_required = ( @@ -154,7 +153,7 @@ def create_account_with_params(request, params): data=params, extra_fields=extra_fields, extended_profile_fields=extended_profile_fields, - enforce_password_policy=enforce_password_policy, + do_third_party_auth=do_external_auth, tos_required=tos_required, ) custom_form = get_registration_extension_form(data=params) diff --git a/openedx/core/djangoapps/user_authn/views/tests/test_register.py b/openedx/core/djangoapps/user_authn/views/tests/test_register.py index 7e4e166ad1..17b5ff16e9 100644 --- a/openedx/core/djangoapps/user_authn/views/tests/test_register.py +++ b/openedx/core/djangoapps/user_authn/views/tests/test_register.py @@ -637,10 +637,14 @@ class TestCreateAccountValidation(TestCase): del params["email"] assert_email_error("A properly formatted e-mail is required") - # Empty, too short - for email in ["", "a"]: - params["email"] = email - assert_email_error("A properly formatted e-mail is required") + # Empty + params["email"] = "" + assert_email_error("A properly formatted e-mail is required") + + #too short + params["email"] = "a" + assert_email_error("A properly formatted e-mail is required " + "Ensure this value has at least 3 characters (it has 1).") # Too long params["email"] = '{email}@example.com'.format( @@ -703,18 +707,21 @@ class TestCreateAccountValidation(TestCase): # Missing del params["password"] - assert_password_error("A valid password is required") + assert_password_error("This field is required.") - # Empty, too short - for password in ["", "a"]: - params["password"] = password - assert_password_error("A valid password is required") + # Empty + params["password"] = "" + assert_password_error("This field is required.") + + # Too short + params["password"] = "a" + assert_password_error("This password is too short. It must contain at least 2 characters.") # Password policy is tested elsewhere # Matching username params["username"] = params["password"] = "test_username_and_password" - assert_password_error("Password cannot be the same as the username.") + assert_password_error("The password is too similar to the username.") def test_name(self): params = dict(self.minimal_params)