6
\$\begingroup\$

This is a translatable password validator for django, based on zxcvbn-python and available with pip. zxcvbn is a password strength estimator inspired by password crackers. It permits to prevent users to have to choose a password with one upper case, one special character and a number, but still check the password strengh and provide input to help the user choose a password.

The project is available here on Github, and the package is on Pypi here.

I check the code with the pre-commit framework, using black, isort, flake8 and pylint as git hooks. I'm testing it with tox and coverage locally, and I set up travis and codacy online. The translation is done with django-rosetta. I tried to make a clear readme with examples to explain what the package does in detail, there are also tests covering the code base entirely.

I would be really interested in remarks about the whole project, code, tests, documentation, tooling, set up, design, complexity to onboard, and basically everything around the project that could make it better.

But this makes a lot of things to review so the main part of the code is the following :

from django.conf import settings
from django.core.exceptions import ImproperlyConfigured, ValidationError
from django.utils.translation import ugettext_lazy as _
from zxcvbn import zxcvbn

from django_zxcvbn_password_validator.settings import DEFAULT_MINIMAL_STRENGTH
from django_zxcvbn_password_validator.translate_zxcvbn_text import (
    translate_zxcvbn_text,
    translate_zxcvbn_time_estimate,
)


class ZxcvbnPasswordValidator:
    def __init__(self, min_length=1, zxcvbn_implementation=zxcvbn):
        self.min_length = min_length
        self.zxcvbn_implementation = zxcvbn_implementation
        password_minimal_strength = getattr(settings, "PASSWORD_MINIMAL_STRENGTH", None)
        if password_minimal_strength is None:
            # Compatibility with a typo in previous version.
            password_minimal_strength = getattr(
                settings, "PASSWORD_MINIMAL_STRENTH", None
            )
        if password_minimal_strength is None:
            password_minimal_strength = DEFAULT_MINIMAL_STRENGTH
        self.password_minimal_strength = password_minimal_strength
        self.__check_password_minimal_strength()

    def __check_password_minimal_strength(self):
        error_msg = "ZxcvbnPasswordValidator need an integer between 0 and 4 "
        error_msg += "for PASSWORD_MINIMAL_STRENGTH in the settings."
        try:
            not_an_int = (
                int(self.password_minimal_strength) != self.password_minimal_strength
            )
        except ValueError:
            not_an_int = True
        if not_an_int:
            error_msg += f" (not '{self.password_minimal_strength}', "
            error_msg += f"a {self.password_minimal_strength.__class__.__name__})"
            raise ImproperlyConfigured(error_msg)
        if self.password_minimal_strength < 0 or self.password_minimal_strength > 4:
            error_msg += f" ({self.password_minimal_strength} is not in [0,4])"
            raise ImproperlyConfigured(error_msg)

    def validate(self, password, user=None):
        def append_translated_feedback(old_feedbacks, feedback_type, new_feedbacks):
            if new_feedbacks:
                if isinstance(new_feedbacks, str):
                    new_feedbacks = [new_feedbacks]
                for new_feedback in new_feedbacks:
                    old_feedbacks.append(
                        f"{feedback_type} : {translate_zxcvbn_text(new_feedback)}"
                    )

        user_inputs = []
        if user:
            for value in user.__dict__.values():
                user_inputs.append(value)
        results = self.zxcvbn_implementation(password, user_inputs=user_inputs)
        password_strength = results["score"]
        if password_strength < self.password_minimal_strength:
            crack_time = results["crack_times_display"]
            offline_time = crack_time["offline_slow_hashing_1e4_per_second"]
            feedbacks = [
                "{} {}".format(
                    _("Your password is too guessable :"),
                    _("It would take an offline attacker %(time)s to guess it.")
                    % {"time": translate_zxcvbn_time_estimate(offline_time)},
                )
            ]
            append_translated_feedback(
                feedbacks, _("Warning"), results["feedback"]["warning"]
            )
            append_translated_feedback(
                feedbacks, _("Advice"), results["feedback"]["suggestions"]
            )
            raise ValidationError(feedbacks)

    def get_help_text(self):
        expectations = _("We expect")
        if self.password_minimal_strength == 0:
            expectations += " {}".format(
                _("nothing: you can use any password you want.")
            )
            return expectations
        expectations += " {}".format(_("a password that cannot be guessed"))
        hardness = {
            1: _("by your familly or friends."),
            2: _("by attackers online."),
            3: _("without access to our database."),
            4: _("without a dedicated team and an access to our database."),
        }
        expectations += " {}".format(hardness.get(self.password_minimal_strength))
        return "{} {} {} {}".format(
            _("There is no specific rule for a great password,"),
            _("however if your password is too easy to guess,"),
            _("we will tell you how to make a better one."),
            expectations,
        )

Translation is done here :

import logging

from django.utils.translation import ugettext_lazy as _

LOGGER = logging.getLogger(__file__)


def translate_zxcvbn_text(text):
    """ This PR would make it cleaner, but it will also be very slow
    to be integrated in python-zxcvbn and we want this to work now :
    https://github.com/dropbox/zxcvbn/pull/124 """
    i18n = {
        "Use a few words, avoid common phrases": _(
            "Use a few words, avoid common phrases"
        ),
        "No need for symbols, digits, or uppercase letters": _(
            "No need for symbols, digits, or uppercase letters"
        ),
        "Add another word or two. Uncommon words are better.": _(
            "Add another word or two. Uncommon words are better."
        ),
        "Straight rows of keys are easy to guess": _(
            "Straight rows of keys are easy to guess"
        ),
        "Short keyboard patterns are easy to guess": _(
            "Short keyboard patterns are easy to guess"
        ),
        "Use a longer keyboard pattern with more turns": _(
            "Use a longer keyboard pattern with more turns"
        ),
        'Repeats like "aaa" are easy to guess': _(
            'Repeats like "aaa" are easy to guess'
        ),
        'Repeats like "abcabcabc" are only slightly harder to guess than "abc"': _(
            'Repeats like "abcabcabc" are only slightly harder to guess than "abc"'
        ),
        "Avoid repeated words and characters": _("Avoid repeated words and characters"),
        'Sequences like "abc" or "6543" are easy to guess': _(
            'Sequences like "abc" or "6543" are easy to guess'
        ),
        "Avoid sequences": _("Avoid sequences"),
        "Recent years are easy to guess": _("Recent years are easy to guess"),
        "Avoid recent years": _("Avoid recent years"),
        "Avoid years that are associated with you": _(
            "Avoid years that are associated with you"
        ),
        "Dates are often easy to guess": _("Dates are often easy to guess"),
        "Avoid dates and years that are associated with you": _(
            "Avoid dates and years that are associated with you"
        ),
        "This is a top-10 common password": _("This is a top-10 common password"),
        "This is a top-100 common password": _("This is a top-100 common password"),
        "This is a very common password": _("This is a very common password"),
        "This is similar to a commonly used password": _(
            "This is similar to a commonly used password"
        ),
        "A word by itself is easy to guess": _("A word by itself is easy to guess"),
        "Names and surnames by themselves are easy to guess": _(
            "Names and surnames by themselves are easy to guess"
        ),
        "Common names and surnames are easy to guess": _(
            "Common names and surnames are easy to guess"
        ),
        "Capitalization doesn't help very much": _(
            "Capitalization doesn't help very much"
        ),
        "All-uppercase is almost as easy to guess as all-lowercase": _(
            "All-uppercase is almost as easy to guess as all-lowercase"
        ),
        "Reversed words aren't much harder to guess": _(
            "Reversed words aren't much harder to guess"
        ),
        "Predictable substitutions like '@' instead of 'a' don't help very much": _(
            "Predictable substitutions like '@' instead of 'a' don't help very much"
        ),
    }
    translated_text = i18n.get(text)
    if translated_text is None:
        # zxcvbn is inconsistent, sometime there is a dot, sometime not
        translated_text = i18n.get(text[:-1])
    if translated_text is None:
        LOGGER.warning(
            "No translation for '%s' or '%s', update the generatei18ndict command.",
            text,
            text[:-1],
        )
        return text
    return translated_text


def translate_zxcvbn_time_estimate(text):
    def replace_dict(text, times):
        for original, translated in times.items():
            text = text.replace(original, str(translated))
        return text

    if text == "less than a second":
        return _("less than a second")
    text = text.replace("centuries", str(_("centuries")))
    plural_times = {
        "seconds": _("seconds"),
        "minutes": _("minutes"),
        "hours": _("hours"),
        "days": _("days"),
        "months": _("months"),
        "years": _("years"),
    }
    times = {
        "second": _("second"),
        "minute": _("minute"),
        "hour": _("hour"),
        "day": _("day"),
        "month": _("month"),
        "year": _("year"),
    }
    # Plural first to avoid replacing "hours" by _("hour") + s
    # Adding an 's' does not mean plural in every language
    text = replace_dict(text, plural_times)
    text = replace_dict(text, times)
return text

This part of the code is mostly generated by the following management command (done in case zxcvbn add string or remove string so it's easier to make new a new translation) :

# -*- coding: utf-8 -*-

from django.core.management.base import BaseCommand


class Command(BaseCommand):

    help = "Will generate what the i18n dict for the translate_zxcvbn_text function"

    def handle(self, *args, **options):
        existings_messages = [
            "Use a few words, avoid common phrases",
            "No need for symbols, digits, or uppercase letters",
            "Add another word or two. Uncommon words are better.",
            "Straight rows of keys are easy to guess",
            "Short keyboard patterns are easy to guess",
            "Use a longer keyboard pattern with more turns",
            'Repeats like "aaa" are easy to guess',
            'Repeats like "abcabcabc" are only slightly harder to guess than "abc"',
            "Avoid repeated words and characters",
            'Sequences like "abc" or "6543" are easy to guess',
            "Avoid sequences",
            "Recent years are easy to guess",
            "Avoid recent years",
            "Avoid years that are associated with you",
            "Dates are often easy to guess",
            "Avoid dates and years that are associated with you",
            "This is a top-10 common password",
            "This is a top-100 common password",
            "This is a very common password",
            "This is similar to a commonly used password",
            "A word by itself is easy to guess",
            "Names and surnames by themselves are easy to guess",
            "Common names and surnames are easy to guess",
            "Capitalization doesn't help very much",
            "All-uppercase is almost as easy to guess as all-lowercase",
            "Reversed words aren't much harder to guess",
            "Predictable substitutions like '@' instead of 'a' don't help very much",
        ]
        msg = "    i18n = {"
        for message in existings_messages:
            message = message.replace("'", "\\'")
            msg += f"        '{message}': _('{message}'),"
        msg += "    }"
        msg += "Please copy paste the following in the translate_zxcvbn_text function,"
        msg += " then use 'python manage.py makemessages'."
print(msg)

This is the whole code, everything else is either tests, documentation or packaging.

Thank you in advance for any remarks or advices !

\$\endgroup\$
3
\$\begingroup\$

Some suggestions:

  • The translations strings are duplicated, and are present in code rather than as a separate text file. I've not done i18n yet, but that seems strange. I would expect a .po file containing text and translations.
  • user_imputs should be user_inputs
  • You mix four types of string formatting: +/+=, %, .format() and f-strings. The first two I believe are generally discouraged in favour of the last two.
  • add_list_of_advices is confusing. The two calls to it each use a different branch of the method, and there is no common code between the branches. it looks like inlining it would actually make the code more readable.
  • This is just personal style, but on the last major project my colleagues and I have found that 80 character width is too limiting - you end up having to artificially abbreviate names (which reduces readability) or use some very clunky line splitting (also reducing readability) far too often. We found that we can fit two files side by side with 120 character line widths on a modern screen; your experience may vary.

In any case, this is some of the best code I've seen on this site!

\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you very much! As you found a typo, it prompted me to check other typos as well. Well, it looks like I'll have to handle compatibility with old typos, because there is a typo in "strength" everywhere, including the variable defined by users in the setting file "PASSWORD_MINIMAL_STRENGTH" :) \$\endgroup\$ – Pierre.Sassoulas Jun 16 at 13:31
  • \$\begingroup\$ @Pierre.Sassoulas Also "__check_password_minimal_strength", in case you haven't caught it yet. \$\endgroup\$ – Graipher Jun 16 at 14:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.