From 9c57cd17700469f411b29b6f3851bc17b7a74a26 Mon Sep 17 00:00:00 2001 From: Cali Stenson Date: Wed, 3 Oct 2018 09:37:49 -0400 Subject: [PATCH] Add unicode normalization to passwords. LEARNER-4283 --- common/djangoapps/student/helpers.py | 9 +++- .../util/password_policy_validators.py | 12 ++++- .../tests/test_password_policy_validators.py | 20 ++++++-- .../core/djangoapps/user_api/accounts/api.py | 11 ++++- .../user_api/accounts/tests/test_api.py | 35 +++++++++++--- .../core/djangoapps/user_api/config/waffle.py | 4 +- .../core/djangoapps/user_authn/views/login.py | 10 +++- .../user_authn/views/tests/test_login.py | 47 ++++++++++++++++--- .../user_authn/views/tests/test_register.py | 24 +++++++++- 9 files changed, 149 insertions(+), 23 deletions(-) diff --git a/common/djangoapps/student/helpers.py b/common/djangoapps/student/helpers.py index 654cfef21b..fff1762ffd 100644 --- a/common/djangoapps/student/helpers.py +++ b/common/djangoapps/student/helpers.py @@ -36,6 +36,7 @@ from lms.djangoapps.verify_student.utils import is_verification_expiring_soon, v from openedx.core.djangoapps.certificates.api import certificates_viewable_for_course from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.theming import helpers as theming_helpers +from openedx.core.djangoapps.user_api.config.waffle import PASSWORD_UNICODE_NORMALIZE_FLAG from openedx.core.djangoapps.theming.helpers import get_themes from student.models import ( LinkedInAddToProfileConfiguration, @@ -47,6 +48,7 @@ from student.models import ( email_exists_or_retired, username_exists_or_retired ) +from util.password_policy_validators import normalize_password # Enumeration of per-course verification statuses @@ -425,6 +427,8 @@ def authenticate_new_user(request, username, password): logged in until they close the browser. They can't log in again until they click the activation link from the email. """ + if PASSWORD_UNICODE_NORMALIZE_FLAG.is_enabled(): + password = normalize_password(password) backend = load_backend(NEW_USER_AUTH_BACKEND) user = backend.authenticate(request=request, username=username, password=password) user.backend = NEW_USER_AUTH_BACKEND @@ -623,7 +627,10 @@ def do_create_account(form, custom_form=None): email=form.cleaned_data["email"], is_active=False ) - user.set_password(form.cleaned_data["password"]) + password = form.cleaned_data["password"] + if PASSWORD_UNICODE_NORMALIZE_FLAG.is_enabled(): + password = normalize_password(password) + user.set_password(password) registration = Registration() # TODO: Rearrange so that if part of the process fails, the whole process fails. diff --git a/common/djangoapps/util/password_policy_validators.py b/common/djangoapps/util/password_policy_validators.py index 61dc5b089d..b1ac096703 100644 --- a/common/djangoapps/util/password_policy_validators.py +++ b/common/djangoapps/util/password_policy_validators.py @@ -7,7 +7,6 @@ from __future__ import unicode_literals import logging import unicodedata -from django.conf import settings from django.contrib.auth.password_validation import ( get_default_password_validators, validate_password as django_validate_password, @@ -15,6 +14,7 @@ from django.contrib.auth.password_validation import ( ) from django.core.exceptions import ValidationError from django.utils.translation import ugettext as _, ungettext +from openedx.core.djangoapps.user_api.config.waffle import PASSWORD_UNICODE_NORMALIZE_FLAG from six import text_type log = logging.getLogger(__name__) @@ -89,6 +89,14 @@ def password_validators_restrictions(): return complexity_restrictions +def normalize_password(password): + """ + Normalize all passwords to 'NFKC' across the platform to prevent mismatched hash strings when comparing entered + passwords on login. See LEARNER-4283 for more context. + """ + return unicodedata.normalize('NFKC', password) + + def validate_password(password, user=None): """ EdX's custom password validator for passwords. This function performs the @@ -117,6 +125,8 @@ def validate_password(password, user=None): # no reason to get into weeds raise ValidationError([_('Invalid password.')]) + if PASSWORD_UNICODE_NORMALIZE_FLAG.is_enabled(): + password = normalize_password(password) django_validate_password(password, user) diff --git a/common/djangoapps/util/tests/test_password_policy_validators.py b/common/djangoapps/util/tests/test_password_policy_validators.py index d037d7557c..9f451ac169 100644 --- a/common/djangoapps/util/tests/test_password_policy_validators.py +++ b/common/djangoapps/util/tests/test_password_policy_validators.py @@ -1,6 +1,5 @@ # -*- coding: utf-8 -*- """Tests for util.password_policy_validators module.""" - import mock import unittest @@ -10,13 +9,15 @@ from django.contrib.auth.models import User from django.core.exceptions import ValidationError from django.test.utils import override_settings +from openedx.core.djangoapps.user_api.config.waffle import PASSWORD_UNICODE_NORMALIZE_FLAG +from openedx.core.djangolib.testing.utils import CacheIsolationTestCase from util.password_policy_validators import ( create_validator_config, validate_password, password_validators_instruction_texts, ) @ddt -class PasswordPolicyValidatorsTestCase(unittest.TestCase): +class PasswordPolicyValidatorsTestCase(CacheIsolationTestCase): """ Tests for password validator utility functions @@ -25,7 +26,6 @@ class PasswordPolicyValidatorsTestCase(unittest.TestCase): 2) requiring multiple instances of the check (also checks proper plural message) 3) successful check """ - def validation_errors_checker(self, password, msg, user=None): """ This helper function is used to check the proper error messages are @@ -61,6 +61,20 @@ class PasswordPolicyValidatorsTestCase(unittest.TestCase): # Test badly encoded password self.validation_errors_checker(b'\xff\xff', 'Invalid password.') + def test_password_unicode_normalization(self): + """ Tests that validate_password normalizes passwords """ + # s ̣ ̇ (s with combining dot below and combining dot above) + not_normalized_password = u'\u0073\u0323\u0307' + self.assertEqual(len(not_normalized_password), 3) + # When the flag is not set, the validation should succeed since len > 2 + self.validation_errors_checker(not_normalized_password, None) + + # When we normalize we expect the not_normalized password to fail + # because it should be normalized to u'\u1E69' -> ṩ + with PASSWORD_UNICODE_NORMALIZE_FLAG.override(active=True): + self.validation_errors_checker(not_normalized_password, + 'This password is too short. It must contain at least 2 characters.') + @data( ([create_validator_config('util.password_policy_validators.MinimumLengthValidator', {'min_length': 2})], 'at least 2 characters.'), diff --git a/openedx/core/djangoapps/user_api/accounts/api.py b/openedx/core/djangoapps/user_api/accounts/api.py index 3f08e68e1f..907be5f815 100644 --- a/openedx/core/djangoapps/user_api/accounts/api.py +++ b/openedx/core/djangoapps/user_api/accounts/api.py @@ -18,11 +18,16 @@ 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 validate_password, normalize_password from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.user_api import errors, accounts, forms, helpers -from openedx.core.djangoapps.user_api.config.waffle import PREVENT_AUTH_USER_WRITES, SYSTEM_MAINTENANCE_MSG, waffle +from openedx.core.djangoapps.user_api.config.waffle import ( + PASSWORD_UNICODE_NORMALIZE_FLAG, + PREVENT_AUTH_USER_WRITES, + SYSTEM_MAINTENANCE_MSG, + waffle, +) from openedx.core.djangoapps.user_api.errors import ( AccountUpdateError, AccountValidationError, @@ -331,6 +336,8 @@ def create_account(username, password, email): # Create the user account, setting them to "inactive" until they activate their account. user = User(username=username, email=email, is_active=False) + if PASSWORD_UNICODE_NORMALIZE_FLAG.is_enabled(): + password = normalize_password(password) user.set_password(password) try: 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 bd70aaebb4..fe7c8e2bfe 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_api.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_api.py @@ -5,11 +5,13 @@ Most of the functionality is covered in test_views.py. """ import re +import unicodedata import ddt import pytest from dateutil.parser import parse as parse_datetime from django.conf import settings +from django.contrib.auth.hashers import make_password from django.contrib.auth.models import User from django.core import mail from django.test import TestCase @@ -27,18 +29,23 @@ from openedx.core.djangoapps.user_api.accounts.api import ( request_password_change, update_account_settings ) +from openedx.core.djangoapps.user_api.accounts.tests.retirement_helpers import ( # pylint: disable=unused-import + RetirementTestCase, + fake_requested_retirement, + setup_retirement_states +) from openedx.core.djangoapps.user_api.accounts.tests.testutils import ( INVALID_EMAILS, INVALID_PASSWORDS, INVALID_USERNAMES, VALID_USERNAMES_UNICODE ) -from openedx.core.djangoapps.user_api.accounts.tests.retirement_helpers import ( # pylint: disable=unused-import - RetirementTestCase, - fake_requested_retirement, - setup_retirement_states +from openedx.core.djangoapps.user_api.config.waffle import ( + PASSWORD_UNICODE_NORMALIZE_FLAG, + PREVENT_AUTH_USER_WRITES, + SYSTEM_MAINTENANCE_MSG, + waffle ) -from openedx.core.djangoapps.user_api.config.waffle import PREVENT_AUTH_USER_WRITES, SYSTEM_MAINTENANCE_MSG, waffle from openedx.core.djangoapps.user_api.errors import ( AccountEmailInvalid, AccountPasswordInvalid, @@ -51,6 +58,7 @@ from openedx.core.djangoapps.user_api.errors import ( UserNotAuthorized, UserNotFound ) +from openedx.core.djangoapps.waffle_utils.testutils import override_waffle_flag from openedx.core.djangolib.testing.utils import skip_unless_lms from openedx.core.lib.tests import attr from student.models import PendingEmailChange @@ -317,7 +325,6 @@ class AccountSettingsOnCreationTest(TestCase): def test_create_account(self): # Create a new account, which should have empty account settings by default. create_account(self.USERNAME, self.PASSWORD, self.EMAIL) - # Retrieve the account settings user = User.objects.get(username=self.USERNAME) request = RequestFactory().get("/api/user/v1/accounts/") @@ -354,6 +361,22 @@ class AccountSettingsOnCreationTest(TestCase): 'extended_profile': [], }) + @override_waffle_flag(PASSWORD_UNICODE_NORMALIZE_FLAG, active=True) + def test_normalize_password(self): + """ + Test that unicode normalization on passwords is happening when a user is created. + """ + # Set user password to NFKD format so that we can test that it is normalized to + # NFKC format upon account creation. + create_account(self.USERNAME, unicodedata.normalize('NFKD', u'Ṗŕệṿïệẅ Ṯệẍt'), self.EMAIL) + + user = User.objects.get(username=self.USERNAME) + + salt_val = user.password.split('$')[1] + + expected_user_password = make_password(unicodedata.normalize('NFKC', u'Ṗŕệṿïệẅ Ṯệẍt'), salt_val) + self.assertEqual(expected_user_password, user.password) + @attr(shard=2) @pytest.mark.django_db diff --git a/openedx/core/djangoapps/user_api/config/waffle.py b/openedx/core/djangoapps/user_api/config/waffle.py index fc1166bd41..9292bb0143 100644 --- a/openedx/core/djangoapps/user_api/config/waffle.py +++ b/openedx/core/djangoapps/user_api/config/waffle.py @@ -5,10 +5,12 @@ from __future__ import absolute_import from django.utils.translation import ugettext_lazy as _ -from openedx.core.djangoapps.waffle_utils import WaffleSwitchNamespace +from openedx.core.djangoapps.waffle_utils import WaffleSwitchNamespace, WaffleFlagNamespace, WaffleFlag SYSTEM_MAINTENANCE_MSG = _(u'System maintenance in progress. Please try again later.') WAFFLE_NAMESPACE = u'user_api' +_WAFFLE_FLAG_NAMESPACE = WaffleFlagNamespace(WAFFLE_NAMESPACE) +PASSWORD_UNICODE_NORMALIZE_FLAG = WaffleFlag(_WAFFLE_FLAG_NAMESPACE, u'password_unicode_normalize') # Switches PREVENT_AUTH_USER_WRITES = u'prevent_auth_user_writes' diff --git a/openedx/core/djangoapps/user_authn/views/login.py b/openedx/core/djangoapps/user_authn/views/login.py index be7a339a2c..b2e32c4748 100644 --- a/openedx/core/djangoapps/user_authn/views/login.py +++ b/openedx/core/djangoapps/user_authn/views/login.py @@ -27,6 +27,7 @@ from openedx.core.djangoapps.external_auth.models import ExternalAuthMap from openedx.core.djangoapps.password_policy import compliance as password_policy_compliance from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.util.user_messages import PageLevelMessages +from openedx.core.djangoapps.user_api.config.waffle import PASSWORD_UNICODE_NORMALIZE_FLAG from student.models import ( LoginFailures, PasswordHistory, @@ -36,6 +37,7 @@ from student.forms import send_password_reset_email_for_user import third_party_auth from third_party_auth import pipeline, provider from util.json_request import JsonResponse +from util.password_policy_validators import normalize_password log = logging.getLogger("edx.student") AUDIT_LOG = logging.getLogger("audit") @@ -211,10 +213,14 @@ def _authenticate_first_party(request, unauthenticated_user): username = unauthenticated_user.username if unauthenticated_user else "" try: + password = request.POST['password'] + if PASSWORD_UNICODE_NORMALIZE_FLAG.is_enabled(): + password = normalize_password(password) return authenticate( username=username, - password=request.POST['password'], - request=request) + password=password, + request=request + ) # This occurs when there are too many attempts from the same IP address except RateLimitException: diff --git a/openedx/core/djangoapps/user_authn/views/tests/test_login.py b/openedx/core/djangoapps/user_authn/views/tests/test_login.py index 6f5eae5385..d9584cadb3 100644 --- a/openedx/core/djangoapps/user_authn/views/tests/test_login.py +++ b/openedx/core/djangoapps/user_authn/views/tests/test_login.py @@ -1,35 +1,43 @@ +#coding:utf-8 """ Tests for student activation and login """ import json +import unicodedata import unittest +import ddt from django.conf import settings from django.contrib.auth.models import User -from django.core.cache import cache from django.core import mail -from django.urls import NoReverseMatch, reverse +from django.core.cache import cache from django.http import HttpResponse, HttpResponseBadRequest from django.test.client import Client from django.test.utils import override_settings +from django.urls import NoReverseMatch, reverse from mock import patch from six import text_type from openedx.core.djangoapps.external_auth.models import ExternalAuthMap -from openedx.core.djangoapps.user_api.config.waffle import PREVENT_AUTH_USER_WRITES, waffle -from openedx.core.djangoapps.user_authn.cookies import jwt_cookies -from openedx.core.djangoapps.user_authn.tests.utils import setup_login_oauth_client -from openedx.core.djangoapps.user_authn.waffle import JWT_COOKIES_FLAG from openedx.core.djangoapps.password_policy.compliance import ( NonCompliantPasswordException, NonCompliantPasswordWarning ) +from openedx.core.djangoapps.user_api.config.waffle import ( + PASSWORD_UNICODE_NORMALIZE_FLAG, + PREVENT_AUTH_USER_WRITES, + waffle +) +from openedx.core.djangoapps.user_authn.cookies import jwt_cookies +from openedx.core.djangoapps.user_authn.tests.utils import setup_login_oauth_client +from openedx.core.djangoapps.user_authn.waffle import JWT_COOKIES_FLAG from openedx.core.djangolib.testing.utils import CacheIsolationTestCase from student.tests.factories import RegistrationFactory, UserFactory, UserProfileFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory +@ddt.ddt class LoginTest(CacheIsolationTestCase): """ Test login_user() view @@ -482,6 +490,33 @@ class LoginTest(CacheIsolationTestCase): self.assertIn('Test warning', self.client.session['_messages']) self.assertTrue(response_content.get('success')) + @ddt.data( + ('test_password', 'test_password', True, True), + (unicodedata.normalize('NFKD', u'Ṗŕệṿïệẅ Ṯệẍt'), + unicodedata.normalize('NFKC', u'Ṗŕệṿïệẅ Ṯệẍt'), False, True), + (unicodedata.normalize('NFKC', u'Ṗŕệṿïệẅ Ṯệẍt'), + unicodedata.normalize('NFKD', u'Ṗŕệṿïệẅ Ṯệẍt'), True, True), + (unicodedata.normalize('NFKD', u'Ṗŕệṿïệẅ Ṯệẍt'), + unicodedata.normalize('NFKD', u'Ṗŕệṿïệẅ Ṯệẍt'), False, True), + ('test_password', 'test_password', True, False), + (unicodedata.normalize('NFKD', u'Ṗŕệṿïệẅ Ṯệẍt'), + unicodedata.normalize('NFKC', u'Ṗŕệṿïệẅ Ṯệẍt'), False, False), + (unicodedata.normalize('NFKC', u'Ṗŕệṿïệẅ Ṯệẍt'), + unicodedata.normalize('NFKD', u'Ṗŕệṿïệẅ Ṯệẍt'), False, False), + (unicodedata.normalize('NFKD', u'Ṗŕệṿïệẅ Ṯệẍt'), + unicodedata.normalize('NFKD', u'Ṗŕệṿïệẅ Ṯệẍt'), True, False), + ) + @ddt.unpack + def test_password_unicode_normalization_login(self, password, password_entered, login_success, waffle_flag): + """ + Tests unicode normalization on user's passwords on login. + """ + with PASSWORD_UNICODE_NORMALIZE_FLAG.override(active=waffle_flag): + self.user.set_password(password) + self.user.save() + response, _ = self._login_response(self.user.email, password_entered) + self._assert_response(response, success=login_success) + def _login_response(self, email, password, patched_audit_log=None, extra_post_params=None): """ Post the login info 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 17b5ff16e9..1e7330ce75 100644 --- a/openedx/core/djangoapps/user_authn/views/tests/test_register.py +++ b/openedx/core/djangoapps/user_authn/views/tests/test_register.py @@ -4,6 +4,7 @@ import json import unittest from datetime import datetime from importlib import import_module +import unicodedata import ddt import mock @@ -14,6 +15,7 @@ from django.urls import reverse from django.test import TestCase, TransactionTestCase from django.test.client import RequestFactory from django.test.utils import override_settings +from django.contrib.auth.hashers import make_password from django_comment_common.models import ForumsConfig from notification_prefs import NOTIFICATION_PREF_KEY @@ -23,12 +25,15 @@ from openedx.core.djangoapps.user_authn.views.register import ( _skip_activation_email, ) from openedx.core.djangoapps.external_auth.models import ExternalAuthMap +from openedx.core.djangoapps.waffle_utils.testutils import override_waffle_flag from openedx.core.djangoapps.lang_pref import LANGUAGE_KEY from openedx.core.djangoapps.site_configuration.tests.mixins import SiteMixin from openedx.core.djangoapps.user_api.accounts import ( USERNAME_BAD_LENGTH_MSG, USERNAME_INVALID_CHARS_ASCII, USERNAME_INVALID_CHARS_UNICODE ) -from openedx.core.djangoapps.user_api.config.waffle import PREVENT_AUTH_USER_WRITES, waffle +from openedx.core.djangoapps.user_api.config.waffle import ( + PASSWORD_UNICODE_NORMALIZE_FLAG, PREVENT_AUTH_USER_WRITES, waffle +) from openedx.core.djangoapps.user_api.preferences.api import get_user_preference from student.models import UserAttribute from student.tests.factories import UserFactory @@ -130,6 +135,23 @@ class TestCreateAccount(SiteMixin, TestCase): user = User.objects.get(username=self.username) return user.profile + @override_waffle_flag(PASSWORD_UNICODE_NORMALIZE_FLAG, active=True) + def test_create_account_and_normalize_password(self): + """ + Test that unicode normalization on passwords is happening when a user registers. + """ + # Set user password to NFKD format so that we can test that it is normalized to + # NFKC format upon account creation. + self.params['password'] = unicodedata.normalize('NFKD', u'Ṗŕệṿïệẅ Ṯệẍt') + response = self.client.post(self.url, self.params) + self.assertEqual(response.status_code, 200) + + user = User.objects.get(username=self.username) + salt_val = user.password.split('$')[1] + + expected_user_password = make_password(unicodedata.normalize('NFKC', u'Ṗŕệṿïệẅ Ṯệẍt'), salt_val) + self.assertEqual(expected_user_password, user.password) + def test_marketing_cookie(self): response = self.client.post(self.url, self.params) self.assertEqual(response.status_code, 200)