From ddf01d1631f2d1235f4c3ba93bb5238a9447dbeb Mon Sep 17 00:00:00 2001 From: Johnny Date: Thu, 20 Sep 2018 00:06:09 +0000 Subject: [PATCH 1/4] Implements password validation via the native Django framework. --- evennia/contrib/security/__init__.py | 0 evennia/contrib/security/validators.py | 51 ++++++++++++++++++++++++++ evennia/settings_default.py | 23 ++++++++++++ 3 files changed, 74 insertions(+) create mode 100644 evennia/contrib/security/__init__.py create mode 100644 evennia/contrib/security/validators.py diff --git a/evennia/contrib/security/__init__.py b/evennia/contrib/security/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/evennia/contrib/security/validators.py b/evennia/contrib/security/validators.py new file mode 100644 index 0000000000..b10f990a8a --- /dev/null +++ b/evennia/contrib/security/validators.py @@ -0,0 +1,51 @@ +from django.core.exceptions import ValidationError +from django.utils.translation import gettext as _ +import re + +class EvenniaPasswordValidator: + + def __init__(self, regex=r"^[\w. @+\-',]+$", policy="Password should contain a mix of letters, spaces, digits and @/./+/-/_/'/, only."): + """ + Constructs a standard Django password validator. + + Args: + regex (str): Regex pattern of valid characters to allow. + policy (str): Brief explanation of what the defined regex permits. + + """ + self.regex = regex + self.policy = policy + + def validate(self, password, user=None): + """ + Validates a password string to make sure it meets predefined Evennia + acceptable character policy. + + Args: + password (str): Password to validate + user (None): Unused argument but required by Django + + Returns: + None (None): None if password successfully validated, + raises ValidationError otherwise. + + """ + # Check complexity + if not re.findall(self.regex, password): + raise ValidationError( + _(self.policy), + code='evennia_password_policy', + ) + + def get_help_text(self): + """ + Returns a user-facing explanation of the password policy defined + by this validator. + + Returns: + text (str): Explanation of password policy. + + """ + return _( + "%s From a terminal client, you can also use a phrase of multiple words if you enclose the password in double quotes." % self.policy + ) \ No newline at end of file diff --git a/evennia/settings_default.py b/evennia/settings_default.py index 9c34a6165a..d705d77d18 100644 --- a/evennia/settings_default.py +++ b/evennia/settings_default.py @@ -802,6 +802,29 @@ INSTALLED_APPS = ( # This should usually not be changed. AUTH_USER_MODEL = "accounts.AccountDB" +# Password validation +# https://docs.djangoproject.com/en/1.11/ref/settings/#auth-password-validators +AUTH_PASSWORD_VALIDATORS = [ + { + 'NAME': 'django.contrib.auth.password_validation.UserAttributeSimilarityValidator', + }, + { + 'NAME': 'django.contrib.auth.password_validation.MinimumLengthValidator', + 'OPTIONS': { + 'min_length': 8, + } + }, + { + 'NAME': 'django.contrib.auth.password_validation.CommonPasswordValidator', + }, + { + 'NAME': 'django.contrib.auth.password_validation.NumericPasswordValidator', + }, + { + 'NAME': 'evennia.contrib.security.validators.EvenniaPasswordValidator', + }, +] + # Use a custom test runner that just tests Evennia-specific apps. TEST_RUNNER = 'evennia.server.tests.EvenniaTestSuiteRunner' From c8c9e831eec906eac09a2f7164bb75dcc9d2177a Mon Sep 17 00:00:00 2001 From: Johnny Date: Thu, 20 Sep 2018 20:37:48 +0000 Subject: [PATCH 2/4] Forces validation on Account.set_password() and provides an Account.validate_password() method to validate passwords. --- evennia/accounts/accounts.py | 74 ++++++++++++++++++- evennia/accounts/tests.py | 25 ++++++- .../security => server}/validators.py | 0 evennia/settings_default.py | 2 +- 4 files changed, 98 insertions(+), 3 deletions(-) rename evennia/{contrib/security => server}/validators.py (100%) diff --git a/evennia/accounts/accounts.py b/evennia/accounts/accounts.py index c4c8c37df7..ba2616f003 100644 --- a/evennia/accounts/accounts.py +++ b/evennia/accounts/accounts.py @@ -13,6 +13,8 @@ instead for most things). import time from django.conf import settings +from django.contrib.auth import password_validation +from django.core.exceptions import ValidationError from django.utils import timezone from evennia.typeclasses.models import TypeclassBase from evennia.accounts.manager import AccountManager @@ -357,7 +359,66 @@ class DefaultAccount(with_metaclass(TypeclassBase, AccountDB)): puppet = property(__get_single_puppet) # utility methods - + @classmethod + def validate_password(cls, password, account=None): + """ + Checks the given password against the list of Django validators enabled + in the server.conf file. + + Args: + password (str): Password to validate + + Kwargs: + account (DefaultAccount, optional): Account object to validate the + password for. Optional, but Django includes some validators to + do things like making sure users aren't setting passwords to the + same value as their username. If left blank, these user-specific + checks are skipped. + + Returns: + valid (bool): Whether or not the password passed validation + error (ValidationError, None): Any validation error(s) raised. Multiple + errors can be nested within a single object. + + """ + valid = False + error = None + + # Validation returns None on success; invert it and return a more sensible bool + try: + valid = not password_validation.validate_password(password, user=account) + except ValidationError as e: + error = e + + return valid, error + + def set_password(self, password, force=False): + """ + Applies the given password to the account if it passes validation checks. + Can be overridden by using the 'force' flag. + + Args: + password (str): Password to set + + Kwargs: + force (bool): Sets password without running validation checks. + + Raises: + ValidationError + + Returns: + None (None): Does not return a value. + + """ + if not force: + # Run validation checks + valid, error = self.validate_password(password, account=self) + if error: raise error + + super(DefaultAccount, self).set_password(password) + logger.log_info("Password succesfully changed for %s." % self) + self.at_password_change() + def delete(self, *args, **kwargs): """ Deletes the account permanently. @@ -714,6 +775,17 @@ class DefaultAccount(with_metaclass(TypeclassBase, AccountDB)): """ pass + + def at_password_change(self, **kwargs): + """ + Called after a successful password set/modify. + + Args: + **kwargs (dict): Arbitrary, optional arguments for users + overriding the call (unused by default). + + """ + pass def at_pre_login(self, **kwargs): """ diff --git a/evennia/accounts/tests.py b/evennia/accounts/tests.py index 039a25601f..1eabd1e542 100644 --- a/evennia/accounts/tests.py +++ b/evennia/accounts/tests.py @@ -57,6 +57,29 @@ class TestDefaultAccount(TestCase): def setUp(self): self.s1 = Session() self.s1.sessid = 0 + + def test_password_validation(self): + "Check password validators deny bad passwords" + + self.account = create.create_account("TestAccount%s" % randint(0, 9), email="test@test.com", password="testpassword", typeclass=DefaultAccount) + for bad in ('', '123', 'password', 'TestAccount', '#', 'xyzzy'): + self.assertFalse(self.account.validate_password(bad, account=self.account)[0]) + + "Check validators allow sufficiently complex passwords" + for better in ('Mxyzptlk', "j0hn, i'M 0n1y d4nc1nG"): + self.assertTrue(self.account.validate_password(better, account=self.account)[0]) + + def test_password_change(self): + "Check password setting and validation is working as expected" + self.account = create.create_account("TestAccount%s" % randint(0, 9), email="test@test.com", password="testpassword", typeclass=DefaultAccount) + + from django.core.exceptions import ValidationError + # Try setting some bad passwords + for bad in ('', '#', 'TestAccount', 'password'): + self.assertRaises(ValidationError, self.account.set_password, bad) + + # Try setting a better password (test for False; returns None on success) + self.assertFalse(self.account.set_password('Mxyzptlk')) def test_puppet_object_no_object(self): "Check puppet_object method called with no object param" @@ -157,4 +180,4 @@ class TestDefaultAccount(TestCase): account.puppet_object(self.s1, obj) self.assertTrue(self.s1.data_out.call_args[1]['text'].endswith("is already puppeted by another Account.")) - self.assertIsNone(obj.at_post_puppet.call_args) + self.assertIsNone(obj.at_post_puppet.call_args) \ No newline at end of file diff --git a/evennia/contrib/security/validators.py b/evennia/server/validators.py similarity index 100% rename from evennia/contrib/security/validators.py rename to evennia/server/validators.py diff --git a/evennia/settings_default.py b/evennia/settings_default.py index d705d77d18..8c07244636 100644 --- a/evennia/settings_default.py +++ b/evennia/settings_default.py @@ -821,7 +821,7 @@ AUTH_PASSWORD_VALIDATORS = [ 'NAME': 'django.contrib.auth.password_validation.NumericPasswordValidator', }, { - 'NAME': 'evennia.contrib.security.validators.EvenniaPasswordValidator', + 'NAME': 'evennia.server.validators.EvenniaPasswordValidator', }, ] From e5828024e25f735d90a7e90e060cf60a0e1c143f Mon Sep 17 00:00:00 2001 From: Johnny Date: Thu, 20 Sep 2018 21:29:56 +0000 Subject: [PATCH 3/4] Modifies CmdUnconnectedCreate, CmdPassword and CmdNewPassword to use Django password validation before modification. --- evennia/commands/default/account.py | 10 ++++++++-- evennia/commands/default/admin.py | 17 ++++++++++++++--- evennia/commands/default/unloggedin.py | 12 ++++++++---- 3 files changed, 30 insertions(+), 9 deletions(-) diff --git a/evennia/commands/default/account.py b/evennia/commands/default/account.py index 40805f7465..d81ef8a8f0 100644 --- a/evennia/commands/default/account.py +++ b/evennia/commands/default/account.py @@ -627,10 +627,16 @@ class CmdPassword(COMMAND_DEFAULT_CLASS): return oldpass = self.lhslist[0] # Both of these are newpass = self.rhslist[0] # already stripped by parse() + + # Validate password + validated, error = account.validate_password(newpass) + if not account.check_password(oldpass): self.msg("The specified old password isn't correct.") - elif len(newpass) < 3: - self.msg("Passwords must be at least three characters long.") + elif not validated: + errors = [e for suberror in error.messages for e in error.messages] + string = "\n".join(errors) + self.msg(string) else: account.set_password(newpass) account.save() diff --git a/evennia/commands/default/admin.py b/evennia/commands/default/admin.py index 4e517b77f9..fc90277127 100644 --- a/evennia/commands/default/admin.py +++ b/evennia/commands/default/admin.py @@ -428,12 +428,23 @@ class CmdNewPassword(COMMAND_DEFAULT_CLASS): account = caller.search_account(self.lhs) if not account: return - account.set_password(self.rhs) + + newpass = self.rhs + + # Validate password + validated, error = account.validate_password(newpass) + if not validated: + errors = [e for suberror in error.messages for e in error.messages] + string = "\n".join(errors) + caller.msg(string) + return + + account.set_password(newpass) account.save() - self.msg("%s - new password set to '%s'." % (account.name, self.rhs)) + self.msg("%s - new password set to '%s'." % (account.name, newpass)) if account.character != caller: account.msg("%s has changed your password to '%s'." % (caller.name, - self.rhs)) + newpass)) class CmdPerm(COMMAND_DEFAULT_CLASS): diff --git a/evennia/commands/default/unloggedin.py b/evennia/commands/default/unloggedin.py index bc7e69934f..0b181538a1 100644 --- a/evennia/commands/default/unloggedin.py +++ b/evennia/commands/default/unloggedin.py @@ -294,10 +294,14 @@ class CmdUnconnectedCreate(COMMAND_DEFAULT_CLASS): string = "\n\r That name is reserved. Please choose another Accountname." session.msg(string) return - if not re.findall(r"^[\w. @+\-']+$", password) or not (3 < len(password)): - string = "\n\r Password should be longer than 3 characters. Letters, spaces, digits and @/./+/-/_/' only." \ - "\nFor best security, make it longer than 8 characters. You can also use a phrase of" \ - "\nmany words if you enclose the password in double quotes." + + # Validate password + Account = utils.class_from_module(settings.BASE_ACCOUNT_TYPECLASS) + # Have to create a dummy Account object to check username similarity + valid, error = Account.validate_password(password, account=Account(username=accountname)) + if error: + errors = [e for suberror in error.messages for e in error.messages] + string = "\n".join(errors) session.msg(string) return From 35efb57b5647842324f2c886bbd24d25b8963a67 Mon Sep 17 00:00:00 2001 From: Johnny Date: Thu, 20 Sep 2018 22:57:20 +0000 Subject: [PATCH 4/4] Adds test for Evennia validator. --- evennia/server/tests.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/evennia/server/tests.py b/evennia/server/tests.py index e821d58583..bb1fae4af5 100644 --- a/evennia/server/tests.py +++ b/evennia/server/tests.py @@ -23,6 +23,9 @@ try: from django.utils import unittest except ImportError: import unittest + +from evennia.server.validators import EvenniaPasswordValidator +from evennia.utils.test_resources import EvenniaTest from django.test.runner import DiscoverRunner @@ -77,3 +80,16 @@ class TestDeprecations(TestCase): self.assertRaises(DeprecationWarning, check_errors, MockSettings(setting)) # test check for WEBSERVER_PORTS having correct value self.assertRaises(DeprecationWarning, check_errors, MockSettings("WEBSERVER_PORTS", value=["not a tuple"])) + +class ValidatorTest(EvenniaTest): + + def test_validator(self): + # Validator returns None on success and ValidationError on failure. + validator = EvenniaPasswordValidator() + + # This password should meet Evennia standards. + self.assertFalse(validator.validate('testpassword', user=self.account)) + + # This password contains illegal characters and should raise an Exception. + from django.core.exceptions import ValidationError + self.assertRaises(ValidationError, validator.validate, '(#)[#]<>', user=self.account) \ No newline at end of file