5
\$\begingroup\$

Lately, I was studying the concept of unit testing in python, so I have decided to write a imperative strong password validator ( checker ) and also attach a unit test to it.

I would like to know if there is any recommendation for improving this code, Thanks!

main.py

#!/usr/bin/env python3
# -*- coding: utf-8 -*-


"""
KhodeNima's unit test learning package. | 23/11/9
"""


import string


ascii_lowercase_letters = string.ascii_lowercase
ascii_uppercase_letters = string.ascii_uppercase
ascii_numbers = string.digits


def is_master_password(literal: str):
    """Chekcs if a string literal have the requirements of a masterpasssword.

    Args:
        string (str): The string literal.

    Raises:
        ValueError: If invalid argument is passed for any parameter.
    """

    if not isinstance(literal, str):
        argument_type = type(literal).__name__
        raise ValueError(f"Invalid argument type passed for the parameter: string | Expected: 'str' | Not: {argument_type}")
    

    allowed_characters = ''.join([ascii_lowercase_letters, ascii_uppercase_letters, ascii_numbers])
    min_password_length = 5
    max_password_length = 20
    string_length = len(literal)
    valid_characters_count = 0

    has_upper_case = False
    has_lower_case = False
    has_digit = False
    

    if string_length < min_password_length:
        return False

    if string_length > max_password_length:
       return False


    for character in literal:

        if character in allowed_characters:
            valid_characters_count += 1


    for character in literal:

        if all([has_digit, has_upper_case, has_lower_case]):
            break
        
        if character.isdigit():
            has_digit = True
        
        if character.isupper():
            has_upper_case = True

        if character.islower():
            has_lower_case = True

        

    if not valid_characters_count == string_length or not all([has_digit, has_lower_case, has_upper_case]):
        return False
    
    return True



print(is_master_password("Reedwolf13"))



unit_test.py

"""
KhodeNima's Unit test training package | 2023/11/9
"""


import unittest
from main import (
    is_master_password
)


class MasterPasswordExaminerTest(unittest.TestCase):
    """
    The main program test case
    """

    def test_character_validation_check(self):
        """Does the program character checking system work correctly?
        """

        self.assertEqual(is_master_password("!-*&%#@$()"), False)
        self.assertEqual(is_master_password('Redwolf1'), True)

    def test_length_validation_check(self):
        """Does the program validate the password length correctly?
        """

        self.assertEqual(is_master_password('Jsf3'), False)
        self.assertEqual(is_master_password('mnbRDSAQOPJK123986532nm'), False)

    def test_requirement_check(self):
        """Does the program validates the password requirements correcly? (1 Uppercase L, 1 Lowercase L, 1 Digit)
        """

        self.assertEqual(is_master_password("Redwolf1"), True)
        self.assertEqual(is_master_password("Redwolf"), False)
        self.assertEqual(is_master_password("redwolf1"), False)




if __name__ == '__main__':
    unittest.main()

\$\endgroup\$
4
  • 3
    \$\begingroup\$ I wouln't put your personal details in your profile. You don't have much to gain having the informaton in your profile. Anyway you're lucky Iran's not in the EU otherwise your account would be have to be removed from the platform. \$\endgroup\$
    – Peilonrayz
    Nov 10, 2023 at 7:24
  • 2
    \$\begingroup\$ why do you have a max length? why do you not allow special characters? why are all those rules hard-coded? where do they come from? \$\endgroup\$
    – njzk2
    Nov 10, 2023 at 22:43
  • \$\begingroup\$ (also, how does the caller know how to fix the "invalid" password they tested?) \$\endgroup\$
    – njzk2
    Nov 11, 2023 at 12:48
  • \$\begingroup\$ @njzk2 Greetings sir/madam. Fortunately these rules don't have a official guide or attendant and they are not genuinely certificated, these rules were set for training purpose only. \$\endgroup\$
    – KhodeNima
    Nov 11, 2023 at 16:47

3 Answers 3

13
\$\begingroup\$

standard libraries are, umm, standard

ascii_lowercase_letters = string.ascii_lowercase
ascii_uppercase_letters = string.ascii_uppercase
ascii_numbers = string.digits

It's hard to see how these renames improve readability or maintainability.

For each RHS expression I already know what it means, or it's self explanatory, or I can look it up. Since it's part of a standard library, I know "there's nothing up my sleeve", no tricks, no gotchas. That's not true for the three aliases, unless I refer back to view their definition.

Also, the -*- coding: utf-8 -*- is odd, given that UTF8 has been standard for a bunch of cPython interpreter versions in recent years. Recommend you delete it.


meaningful identifier

def is_master_password(literal: str):

I found this signature slightly confusing, as it made me think of this (which you clearly weren't saying):

from typing import Literal

def is_master_password(s: literal['foo', 'bar']):

Recommend you call a spade a spade:

def is_master_password(password: str):

Also, the function seems to have the wrong name. The docstring explains it should be called def is_valid_master_password.

    Args:
        string (str): The string literal.

I don't understand what you wrote. We don't see a parameter named string in the signature. Please delete this fluff, as it does not add any information and it is in danger of getting out of sync / has gotten out of sync with the code it describes.

    Raises:
        ValueError: If invalid argument is passed for any parameter.
    """

    if not isinstance(literal, str):

Recommend you delete all this fluff. The signature already has a very nice type annotation, so mypy will flag any attempted shenanigans. And duck typing will offer a pretty clear diagnostic stack trace for bad input when we attempt any one of these:

  • len( ... )
  • character in ...
  • .isdigit()

As written this just isn't a very pythonic predicate.

    max_password_length = 20

I don't understand why offering 21 characters would be insecure. This line needs a comment so we can trace it back to a requirements document.


premature optimization

        if all([has_digit, has_upper_case, has_lower_case]):
            break

Please delete this. It is distracting, and it does not appreciably speed things up.

May as well merge the boolean ops with the "valid" counting loop.


standard asserts

Expressions like this are slightly odd:

        self.assertEqual(is_master_password("Redwolf1"), True)

Using assertEqual is great in many contexts. But here, it would be more natural to self.assertTrue( ... ).

\$\endgroup\$
3
  • 1
    \$\begingroup\$ If you want to keep the ascii in there you might want to do import string as ascii \$\endgroup\$
    – Daviid
    Nov 10, 2023 at 14:14
  • 10
    \$\begingroup\$ @Daviid I'd never let code with standard library name aliasing pass my review. import string as ascii reads like "we all know what string module does, let's make reading this code a bit more challenging". \$\endgroup\$ Nov 11, 2023 at 2:22
  • 4
    \$\begingroup\$ Another minor suggestion is to use string.ascii_letters instead of concatenating string.ascii_lowercase and string.ascii_uppercase \$\endgroup\$
    – Joe
    Nov 12, 2023 at 17:50
5
\$\begingroup\$

Style

Your code is readable, well documented, and mostly follows conventions, nice.

My only nitpicks here is that you use snake_case for constants, where these should be ALL_CAPS in order to easily identify them as constants (MIN_PASSWORD_LENGTH, for example) and that you use a bit too much vertical whitespace for my taste (but too much is better than too little).

Constants should also be defined at the top level, right after imports.

Tests

I appreciate that you include unit tests, using a proper testing framework.

A couple of remarks here:

  • improve your code coverage: you don't test if missing lowercase characters triggers a rejection
  • Split your test cases so that each one tests one path: if you make changes to the function being tested and a test breaks, it will be much easier to figure out where you went wrong

Executed statement

The line print(is_master_password("Reedwolf13")) at the bottom of your script gets executed each time it is loaded, including after import. You can see its output when running tests. Not good. You should put it behind a if __name__ == '__main__' guard, or, since your code is tested anyway, remove it.

Return early

You already return early after length checks. You could go all the way and return early after checking for invalid characters.

Logic issues

When checking for invalid characters, your logic seems convoluted: you count valid characters, then compare with the total number of characters. I feel it's more straightforward to check if any character is invalid:

any(character not in ALLOWED_CHARACTERS for character in password)

Similarly, you can save explicitly looping and keeping track of flags by using any checks for character sets.

Naming

I find the name is_master_password confusing: I would expect this function to tell me if the password matches the master password. I find validate_password to be more descriptive.

You should name your modules to reflect what they do, something like password validator.py and password validator tests.py are much more descriptive.

Sample code

Here is my take on the problem, also addressing issues noted in @J_H's answer.

password_validator.py:

"""
Utility module to check password validity
"""


from string import ascii_lowercase, ascii_uppercase, digits


MIN_PASSWORD_LENGTH = 5
MAX_PASSWORD_LENGTH = 20
ALLOWED_CHARACTERS = ascii_lowercase + ascii_uppercase + digits


def is_valid_password(password: str) -> bool:
    """
    Checks if a password matches the following requirements:
        * is at least MIN_PASSWORD_LENGTH characters long
        * is at most MAX_PASSWORD_LENGTH characters long
        * contains only allowed characters
        * includes uppercase, lowercase, and digit characters

    Parameters
    ----------
    password : str
        the password to validate.

    Returns
    -------
    bool
        True if the password id valid, false otherwise.
    """

    if len(password) < MIN_PASSWORD_LENGTH:
        return False

    if len(password) > MAX_PASSWORD_LENGTH:
       return False

    if any(character not in ALLOWED_CHARACTERS for character in password):
        return False
    
    return (any(character in ascii_lowercase for character in password)
            and any(character in ascii_uppercase for character in password)
            and any(character in digits for character in password))

password_validator_tests.py:

"""
Tests for the password validator module
"""


import unittest

from password_validator import is_valid_password


class MasterPasswordExaminerTest(unittest.TestCase):

    def test_valid_passwords(self):
        """Should accepts a valid password"""
        self.assertTrue(is_valid_password("Redwolf1"))
        self.assertTrue(is_valid_password("abAB1"))
        self.assertTrue(is_valid_password("0123456789aAbBcCdDeE"))
        
    def test_password_too_short(self):
        """Should reject a too short password"""
        self.assertFalse(is_valid_password("aAB1"))
        self.assertFalse(is_valid_password(""))
        
    def test_password_too_long(self):
        """Should reject a too long password"""
        self.assertFalse(is_valid_password("0123456789aAbBcCdDeEf"))
        
    def test_character_validation_check(self):
        """Should reject a password with invalid characters"""
        self.assertFalse(is_valid_password("Redwolf1&"))
        self.assertFalse(is_valid_password("Redwolf1ù"))

    def test_includes_characters_from_each_set(self):
        """Should reject a password missing a character from each category"""
        self.assertFalse(is_valid_password("edwolf1"))
        self.assertFalse(is_valid_password("Redwolf"))
        self.assertFalse(is_valid_password("REDWOLF1"))


if __name__ == '__main__':
    unittest.main()

Going further

These password validation rules are very weak. It is good practice to use longer passwords, with a larger character set, and this password validator is not useful in real life.

If you want to go further, you might want to improve your logic to:

  • accept longer passwords
  • accept other character sets
  • ensure the password contains characters from X sets out of Y
  • base validation on entropy instead of a rigid set of rules, allowing long passphrases (correct horse battery staple) as well as shorter, random strings ({@C54UCiY#M:**fP)
\$\endgroup\$
1
  • \$\begingroup\$ I think too little vertical whitespace is far better than too much. Easier to see where the blocks are. \$\endgroup\$
    – minseong
    Nov 10, 2023 at 12:53
5
\$\begingroup\$

Please read this as an addition to two existing answers, I agree with almost all points made there.

Whitespace

You may like excessive vertical whitespace, it's your choice. But in this case it's a choice that violates formatting standard (PEP8, link to relevant section):

Use blank lines in functions, sparingly, to indicate logical sections.

Most existing formatters (black, ruff, autopep8) will deny more than one line inside a function. And it's generally good thing to do: when you split all the lines into separate "blocks" 1-2 lines each, it's becoming more difficult to read. And please, definitely no blank lines immediately after for loop - indentation makes the following line sufficiently distinct.

Docstring

A nitpick in addition to answer by @J_H

Prescribe, do not describe. The first sentence of a docstring should be prescriptive, like

Check if a string meets the requirements for a master password.

Booleans are good as booleans!

You have the following:

if not valid_characters_count == string_length or not all([has_digit, has_lower_case, has_upper_case]):
    return False
return True

While it could be just

return (
    valid_characters_count == string_length
    and all([has_digit, has_lower_case, has_upper_case])
)

(and definitely not if not x == y, there is a fancy syntax sugar for that called "not equal" and denoted as x != y)

Entrypoint

You have a shebang on top of your script. It is a good thing to have in scripts, but not at all in library parts. If you're going to allow using this as a script, that's OK - then make it actually usable as a script. Probably you want a script that can read a password and report its quality by exit code (0 for OK, 1 for insufficiently strong). You can use argparse for it to get help and validation for free.

def _read_args(source: list[str]):
    parser = argparse.ArgumentParser(
        prog="check_password.py",
        description="Utility for admin password validation",
    )
    parser.add_argument("password", help="The password to check")
    parser.add_argument(
        "-q",
        "--quiet",
        action="store_false",
        dest="verbose",
        help="Omit output",
    )
    return parser.parse_args(source)


if __name__ == "__main__":
    args = _read_args(sys.argv[1:])
    is_strong = is_strong_master_password(args.password)
    if args.verbose:
        if is_strong:
            print("Validated successfully")
        else:
            print("Validation failed", file=sys.stderr)
    sys.exit(1 - is_strong)

I'm omitting the return type above for brevity, you may use a Protocol to make this fully typed:

from typing import Protocol


class _CommandArguments(Protocol):
    password: str
    verbose: bool


def _read_args(source: list[str]) -> _CommandArguments:
    ...  # Implementation follows

Alternatively, you can remove the shebang if it's not going to be a script.

Reference code

Here's what (roughly) I would have done with these requirements (note that in real life 5-char ASCII password can not be secure):

#!/usr/bin/env python3
"""Utility script for admin password validation."""

import argparse
import string
import sys
from typing import Final, Protocol

MIN_PASSWORD_LENGTH: Final = 5
MAX_PASSWORD_LENGTH: Final = 20
ALLOWED_CHARACTERS: Final = set(
    "".join([string.ascii_lowercase, string.ascii_uppercase, string.digits])
)


def is_strong_master_password(password: str) -> bool:
    """Check if a string meets the requirements for a master password."""

    string_length = len(password)
    if string_length < MIN_PASSWORD_LENGTH:
        return False
    if string_length > MAX_PASSWORD_LENGTH:
        return False

    if set(password) - ALLOWED_CHARACTERS:
        # Contains characters outside of allowed range.
        return False

    has_upper_case = False
    has_lower_case = False
    has_digit = False
    for character in password:
        if character.isdigit():
            has_digit = True
        if character.isupper():
            has_upper_case = True
        if character.islower():
            has_lower_case = True

    return all([has_digit, has_lower_case, has_upper_case])

...  # Entrypoint code goes here

Yes, set building costs more than iteration, but less than you implementation: your original code will check up to (26+26+10) characters on every iteration, while set difference does not have to go this far. We build a set in O(n log n) with n being a password length, but it's still nothing with any real-life length constraints. This code is likely not something performance-critical, and so brevity and clean look should be more important.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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