5
\$\begingroup\$

I did the following Excercise from Automate the boring stuff with Python Chapter 7:

Write a function that uses regular exppressions to make sure the password string it is passed is strong. A strong password is defined as one that is at least eight characters long, contains both uppercase and lowercase characters, and has a least one digit. You may need to test the string against multiple regex patterns to validate its strengh.

My Solution:

I wrote four functions which check the individual aspects of the required password detection. Then these four functions are used to write the strong-password function which validates strings against all the requirements.

To test this i also checked out the unittest module.

Please let me know if theres sth to do better.

Is this a good way to test? Are these good test cases? Are there any bad practices/ little issues in the coding / testing?

Heres the code:

password.py

import re

def valid_length(string):
    """checks if length is > 8 to be a strong password"""
    lenght_regex = re.compile(r'.{8,}')
    if not lenght_regex.search(string):
        return False
    return True

def has_upper(string):
    """Check if string contains one upper letter or more"""
    upper_regex = re.compile(r'.*[A-Z]+.*')
    if not upper_regex.search(string):
        return False
    return True

def has_lower(string):
    """Check if string contains one lower letter or more"""
    lower_regex = re.compile(r'.*[a-z]+.*')
    if not lower_regex.search(string):
        return False
    return True

def has_digit(string):
    """Check if one or more signs is a digit"""
    digit_regex = re.compile(r'.*\d+.*')
    if not digit_regex.search(string):
        return False
    return True

def strong_password(password):
    """
    Validate if passed password is considered "strong",
    Password is considered strong if:
    - is eight characters or longer
    - contains uppercase and lowercase characters
    - has one digit or more
    """
    if not valid_length(password):
        return False
    if not has_upper(password):
        return False
    if not has_lower(password):
        return False
    if not has_digit(password):
        return False
    return True

password_unit_test.py

import unittest
import password as p

class TestIsStrongPassword(unittest.TestCase):
    """Test of strong password detection function."""

    def test_valid_length(self):
        """Test that only a string length of > 8 is accecpted"""
        self.assertEqual(p.valid_length('abcd'), False)
        self.assertEqual(p.valid_length('abcdefg'), False)
        self.assertEqual(p.valid_length('abcdefgh'), True)
        self.assertEqual(p.valid_length('abcdefghi'), True)

    def test_has_upper(self):
        """Test that only strings containing uppercase are accepted"""
        self.assertEqual(p.has_upper('abcd'), False)
        self.assertEqual(p.has_upper('aBcd'), True)
        self.assertEqual(p.has_upper('aBCd'), True)
        self.assertEqual(p.has_upper('Abcd'), True)
        self.assertEqual(p.has_upper('abcD'), True)
        self.assertEqual(p.has_upper('ABCD'), True)

    def test_has_lower(self):
        """Test that only strings containing lowercase are accepted"""
        self.assertEqual(p.has_lower('abcd'), True)
        self.assertEqual(p.has_lower('aBcd'), True)
        self.assertEqual(p.has_lower('aBCd'), True)
        self.assertEqual(p.has_lower('Abcd'), True)
        self.assertEqual(p.has_lower('abcD'), True)
        self.assertEqual(p.has_lower('ABCD'), False)

    def test_has_digit(self):
        """Test that only strings containing lowercase are accepted"""
        self.assertEqual(p.has_digit('abcd'), False)
        self.assertEqual(p.has_digit('a1cd'), True)
        self.assertEqual(p.has_digit('a12d'), True)
        self.assertEqual(p.has_digit('1bcd'), True)
        self.assertEqual(p.has_digit('abc1'), True)
        self.assertEqual(p.has_digit('1234'), True)

    def test_strong_password(self):
        """
        Test strong password function. Passed strings have to pass 
        all tests in valid_length, uppper, lower and digit functions.
        """

        # Test from single functions should all fail 
        # (not met all criteria)
        self.assertEqual(False, p.strong_password('abcd'))
        self.assertEqual(False, p.strong_password('abcdefg'))
        self.assertEqual(False, p.strong_password('abcdefgh'))
        self.assertEqual(False, p.strong_password('abcdefghi'))

        self.assertEqual(False, p.strong_password('abcd'))
        self.assertEqual(False, p.strong_password('aBcd'))
        self.assertEqual(False, p.strong_password('aBCd'))
        self.assertEqual(False, p.strong_password('Abcd'))
        self.assertEqual(False, p.strong_password('abcD'))
        self.assertEqual(False, p.strong_password('ABCD'))

        self.assertEqual(False, p.strong_password('abcd'))
        self.assertEqual(False, p.strong_password('a1cd'))
        self.assertEqual(False, p.strong_password('a12d'))
        self.assertEqual(False, p.strong_password('1bcd'))
        self.assertEqual(False, p.strong_password('abc1'))
        self.assertEqual(False, p.strong_password('1234'))

        # Combinations which met more than one cirteria
        self.assertEqual(False, p.strong_password('12345678'))
        self.assertEqual(False, p.strong_password('Abcdefgh'))
        self.assertEqual(False, p.strong_password('A12345678'))
        self.assertEqual(False, p.strong_password('Abcdfg1'))
        self.assertEqual(True, p.strong_password('A12345678b'))
        self.assertEqual(True, p.strong_password('Abcdefg1'))
        self.assertEqual(True, p.strong_password('123456aB'))
        self.assertEqual(True, p.strong_password('aB345678'))

if __name__ == '__main__':
    unittest.main()
\$\endgroup\$
  • \$\begingroup\$ Just for the record, these are really bad criteria for strong passwords \$\endgroup\$ – Oscar Smith Nov 28 '18 at 10:50
  • \$\begingroup\$ @OscarSmith Yes in my answer I've added the obligatory xkcd as a note ;) \$\endgroup\$ – Ludisposed Nov 28 '18 at 10:56
  • \$\begingroup\$ well it is just an excercise not a real time application \$\endgroup\$ – Sandro4912 Nov 28 '18 at 11:12
8
\$\begingroup\$

Good job on the easily understandable code.

Good

  • Good functions, with clear names!
  • Modular approach
  • Unittests
  • Docstrings

Improvements

  • Regex with lots of backtracking can produce some major performance loss

    Consider that this re.search(r'.*[A-Z]+.*', string)

    is equal to re.search(r'[A-Z]+', string)

    or even re.search(r'[A-Z]', string) as Toby correctly suggested.

    Since we only care if one character is in the given string.

  • Return directly

    Instead of doing

    if exdpression:
        return True
    return False
    

    Return directly with return expression

  • Your compile has no performance gain, because with every new string it will compile again. Instead you could compile only onc,e and store it as a constant.

  • Use the all keyword to check if all expressions evaluates to truthy.

  • Instead of assertEqual(expression, function)

    Do the more direct assertFalse or assertTrue

Revised code

import re
import unittest

PASSWORD_CHECKS = [
    re.compile(r'[A-Z]'),
    re.compile(r'.{8,}'),
    re.compile(r'[a-z]'),
    re.compile(r'[0-9]'),
]

def strong_password(password):
    """
    Validate if passed password is considered "strong",
    Password is considered strong if:
      - is eight characters or longer
      - contains uppercase and lowercase characters
      - has one digit or more
    """
    return all(check.search(password) for check in PASSWORD_CHECKS)

class TestIsStrongPassword(unittest.TestCase):
    """Test of strong password detection function."""
    def test_strong_password(self):
        """
        Test strong password function. Passed strings have to pass 
        all tests in valid_length, uppper, lower and digit functions.
        """

        # Test from single functions should all fail 
        # (not met all criteria)
        self.assertFalse(strong_password('abcd'))
        self.assertFalse(strong_password('abcdefg'))
        self.assertFalse(strong_password('abcdefgh'))
        self.assertFalse(strong_password('abcdefghi'))

        self.assertFalse(strong_password('abcd'))
        self.assertFalse(strong_password('aBcd'))
        self.assertFalse(strong_password('aBCd'))
        self.assertFalse(strong_password('Abcd'))
        self.assertFalse(strong_password('abcD'))
        self.assertFalse(strong_password('ABCD'))

        self.assertFalse(strong_password('abcd'))
        self.assertFalse(strong_password('a1cd'))
        self.assertFalse(strong_password('a12d'))
        self.assertFalse(strong_password('1bcd'))
        self.assertFalse(strong_password('abc1'))
        self.assertFalse(strong_password('1234'))

        # Combinations which met more than one cirteria
        self.assertFalse(strong_password('12345678'))
        self.assertFalse(strong_password('Abcdefgh'))
        self.assertFalse(strong_password('A12345678'))
        self.assertFalse(strong_password('Abcdfg1'))
        self.assertTrue(strong_password('A12345678b'))
        self.assertTrue(strong_password('Abcdefg1'))
        self.assertTrue(strong_password('123456aB'))
        self.assertTrue(strong_password('aB345678'))

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

Notes

What is a strong password? Obligatory xkcd

\$\endgroup\$
  • 1
    \$\begingroup\$ I don't think the + in any of those regexes buys anything valuable - "contains at least one" means that you can be content after matching exactly one. \$\endgroup\$ – Toby Speight Dec 4 '18 at 17:12

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.