7
\$\begingroup\$

I would like a review of my code. It's a very simple code that checks the strength of a password.

def pass_input():

    while True:
        password = input("Enter a password of minimum 8 digits and maximum 16: ")
        if len(password) >= 8 and len(password) <= 16:
            return password


def pass_strenght(password):

    has_letter = False
    has_number = False
    has_special = False
    score = 0

    for digit in password:
        if digit.isalpha():
            has_letter = True
        elif digit.isdigit():
            has_number = True
        else:
            has_special = True

    score = has_letter + has_number + has_special

    
    if score == 1:
        return "weak"
    elif score == 2:
        return "medium"
    elif score == 3:
        return "strong"
    
            
def main():
    
    password = pass_input()
    print(pass_strenght(password))
    
if __name__ == "__main__":
    main()
\$\endgroup\$
1
  • 1
    \$\begingroup\$ I fully understand that this is a made-up strength checker, just want to make sure it is not used anywhere because it is basically against all password security principles. \$\endgroup\$ Commented Apr 15 at 11:35

6 Answers 6

8
\$\begingroup\$

In addition to what @toolic has said:

Unnecessary Initializtion

In function pass_strength, you initialize score with the value 0:

    score = 0

score will subsequently be recomputed without ever having used the value set above. Therefore, the above initialization can be removed.

Alternatives to Multiple if/elif Statements

In function pass_strength you have:

    if score == 1:
        return "weak"
    elif score == 2:
        return "medium"
    elif score == 3:
        return "strong"

Instead, we could also use a match statement, which in my opinion is a bit clearer:

    match score:
        case 1: return "weak"
        case 2: return "medium"
        case 3: return "strong"

We can do away with multiple if/elif and match/case tests altogether by using a dictionary:

    strength_rating = {
        1: "weak",
        2: "medium",
        3: "strong"
    }

    ...

    return strength_rating[score]

Or we can use a list:

    strength_rating = [
        "",  # unused
        "weak",
        "medium",
        "strong"
    ]

    ...

    return strength_rating[score]

Final Comments

A user could enter as a password '^%#)@(!+=-%'. This would receive a strength rating of "weak" since it only uses what you define to be "special characters". In reality, this would be a rather strong password. So, it's not a trivial matter to assign a strength to a given password. Most websites that require passwords don't even try. Instead, they require a minimum password length (like you do) and, for example, at least one character from each of the following classes: an upper case alpha character, a lower case alpha character, a digit and a special character (which is defined by a specific list of characters that are considered "special"). A proposed password either meets those requirements and is accepted or it doesn't. You might wish to code such a password validator and have it reviewed on this site.

\$\endgroup\$
4
  • \$\begingroup\$ Why do you consider a bunch of if to be better than a bunch of elif? \$\endgroup\$ Commented Apr 13 at 13:38
  • 1
    \$\begingroup\$ Good question. I have rethought what I wrote and I now think that is is not better because, although the elif can be changed to an if, it makes it clearer to the reader that we are dealing with alternate choices. I have updated the answer. \$\endgroup\$ Commented Apr 13 at 15:14
  • \$\begingroup\$ "We can do away with multiple if/elif and match/case tests altogether by using a dictionary...Or we can use a list" Couldn't we use enum.Enum to create an enumeration instead, like this? \$\endgroup\$ Commented Apr 14 at 16:59
  • \$\begingroup\$ @CrSb0001 Yes, if you are able to re-define the interface for function password_strength to return a PasswordStrength enum instead of a string without breaking any existing client code. Then you would return thus: return PasswordStrength(score). \$\endgroup\$ Commented Apr 14 at 17:23
7
\$\begingroup\$

DRY

The expression len(password) is repeated twice in this line:

if len(password) >= 8 and len(password) <= 16:

This is simpler:

if 8 <= len(password) <= 16:

For the score comparison, you could replace the if/elif with a match/case statement. That would eliminate repeating the score == code.

UX

It would be nice to give the user a chance to break out of the loop cleanly, without using Ctrl-c to terminate the program.

It would also be good to set a limit on the number of tries.

It is good that you inform the user of the expected password length, but you should also display the other criteria for a strong password:

  • Letter
  • Number
  • Special character

When the user chooses a weak or medium password, you could also display the reasons why it is so.

Input checking

It is great that you check that the input is within your required range. Consider using pyinputplus for more options on input checking.

Your checker allows spaces. For example, this was accepted:

123456 78

If that is not intentional, then you should add a check for that situation.

Typo

The function name pass_strenght has a typo. It should be "strength":

def pass_strength(password):

Documentation

The PEP 8 style guide recommends adding docstrings for functions. For example:

def pass_strenght(password):
    """
    Determine the strength of a password
    The input is the password string.
    The returned value is one of these strings: weak, medium, strong
    """

Also consider using type hints to describe input and return types of the functions to make the code even more self-documenting.

Naming

The function name main is too generic. You could give it a more meaningful name like get_and_print_password. Or, you could just eliminate it completely. It is great that you added a "main" guard, and instead of:

def main():
    
    password = pass_input()
    print(pass_strenght(password))
    
if __name__ == "__main__":
    main()

you could use:

if __name__ == "__main__":
    password = pass_input()
    print(pass_strenght(password))

This can be further simplified as:

if __name__ == "__main__":
    print(pass_strenght(pass_input()))

The variable name digit implies that it is a number. Perhaps the name character is more appropriate.

\$\endgroup\$
5
\$\begingroup\$

talk like a mathematician

        if len(password) >= 8 and len(password) <= 16:

I bet you've read theorems with expressions like: \$~ a \le N \le b\$. \$~\$ Python lets you write in the same way. Prefer:

        if 8 <= len(password) <= 16:

Or maybe even if len(password) in range(8, 17):

misleading identifier

    for digit in password:
        if digit.isalpha():

This is a character, which might be a letter. Prefer the usual name of for ch in ...

extra init

def pass_strenght(password):

nit: typo

Prefer to annotate, so we know what will come back to the caller:

def pass_strength(password: str) -> str:

There doesn't seem to be any motivation to init to \$0\$, since we unconditionally reassign a new score value.

    score = 0
    ...
    score = has_letter + has_number + has_special
\$\endgroup\$
4
\$\begingroup\$

Functionally, there is no reason to limit the password length to 16 characters. I use a password manager, so I casually use longer (and unique!) passwords. But some websites have dumb requirements, so I sometimes have to "downgrade" the password strength. Some people use passphrases too, so 16 characters is not that much.

And in fact, if the password is long enough, the complexity requirements make less sense. See NIST 2025 guidelines.

Hash

Remember that you are not going to store the password in clear of course. You are going to store a hash of it in the database, using a modern library like Argon2id. The resulting hash length will often be 32 bytes, but you can choose. So you just have to make sure that the password field as enough room to store the hash, regardless of how long the original password is.

CLI

For command line usage, consider using the getpass module.

Regex

You could use a regular expression. There are plenty of examples of patterns on the Internet.

To loop or not to loop

Note that in Python a string can be treated more or less like a list. So you could forfeit the loop and check in a different way. For example and for entertainment purposes:

password.isalpha()

Return True if all characters in the string are alphabetic and there is at least one character, False otherwise. (non-ASCII characters can be considered alphabetical too)

Likewise:

password.isdigit()

Return True if all characters in the string are digits and there is at least one character, False otherwise. Digits include decimal characters and digits that need special handling, such as the compatibility superscript digits...

Brute force

Unfortunately, there is a gap between the theory and the practical world.

The password should be robust enough to make brute force unlikely to succeed. But length alone cannot ensure that. Password complexity cannot guarantee that either.

Ideally, a password should be unique and not reused across sites.

For example, P@ssword qualifies as medium according to your program. It is weak, because it's a popular one. Test@123 is strong you say? It's another common one. Some websites use pwnlists for validation, so they will reject passwords that are known to have leaked in the past.

Relevant questions on Code Review:

\$\endgroup\$
1
  • \$\begingroup\$ I agree that 16 is a low limit. Esp. when "passphrases+with+3+or+4+words" are used, it gets much longer. \$\endgroup\$ Commented Apr 15 at 10:48
2
\$\begingroup\$

Without making any of the suggested refinements in terms of categorizing strength of a password, I would probably start by making a Password class that can then have a lot of this logic structured as methods on a Password object.

from functools import cache

class Password:
    def __init__(self, password):
        self._password = password
    
    @property
    def password(self):
        return self._password
    
    @property
    @cache
    def has_alpha(self):
        return any(ch.isalpha() for ch in self._password)
    
    @property
    @cache
    def has_numeric(self):
        return any(ch.isdigit() for ch in self._password)
    
    @property
    @cache
    def has_special(self):
        return any(not ch.isdigit() and not ch.isalpha() for ch in self._password)
    
    @property
    @cache
    def strength(self):
        return self.has_alpha + self.has_numeric + self.has_special
    
    def __len__(self):
        return len(self._password)

    def __str__(self):
        return self.password

I think this would set you up well to implement the types of improvements others have suggested here.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ The next improvement to this would be to add a method that returns a hash of the password ;) \$\endgroup\$ Commented Apr 14 at 18:54
  • \$\begingroup\$ @Kate agreed. There are a lot of things that could be added to this basic template. \$\endgroup\$ Commented Apr 14 at 18:55
2
\$\begingroup\$

The NIST report.

I think the biggest error here is believing an outdated NIST report. When it was written the major password system was the original Unix password system. This encrypts a constant with 56 bits from an 8 (or less) character password. As such, not having 16 of the bits known because you used only lowercase letters was important. The fact that many many web sites don't know any better is no excuse.

Modern password systems are more likely to be using a cryptographic hash on your password. This means the length of your password should be effectively unlimited.

Thus, how to choose a strong password has changed.

Entropy

A strong password turns out to be one with a fair bit of entropy in it. So the problem is to measure the entropy. It turns out that compression algorithms are about producing an alternate representation of your data while maximizing the entropy density.

So one way to check the password strength is to compress it.

    strength = len(zlib.compress(password.encode(),9))

At this point, some thresholding is needed, especially since the compression adds some fixed overhead. But this is a possible starting point.


Some samples to consider (in no particular order):

password strength pass_strenght() [sic]
a1@ 11 strong
a1@a1@a1@a1@a1@ 13 strong
this is a sample password 31 medium
password 16 weak
aaaaaaaaaaaaaaaa 11 weak
aaaaaaaa 11 weak
hliuaheiurhfliau 24 weak
lauleuirghliuaheliuaulierhgli 34 weak
Tr0ub4dor&3 19 strong
correct horse battery staple 36 medium
\$\endgroup\$
3
  • \$\begingroup\$ I like the idea, but... I would take the minimum of compressed length of username || password and email || password (possibly stripping the @... part of the e-mail), to also account for users using a password very similar to their username or email. Also... shouldn't longer length mean stronger? I don't understand your final table... \$\endgroup\$ Commented Apr 14 at 14:59
  • 1
    \$\begingroup\$ @MatthieuM. Longer length USUALLY means stronger, but not if you have too much pattern. I Agree that combining the password with username and separately with email, (and any other personal information on the user you have) makes a lot of sense. I will comment that in those cases, it is important to consider the incremental compressed length over the other field alone. (And I did describe it as a "starting point".) The table is taking the password in the first column and using my computation and OP's computation on it. And the "typo" you "fixed" was to match OP's function name. \$\endgroup\$ Commented Apr 14 at 20:42
  • \$\begingroup\$ Ah! I hadn't noticed there was a typo in the OP's. And I agree that incremental compressed length is an even better idea of strength. \$\endgroup\$ Commented Apr 15 at 7:05

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.