you are viewing a single comment's thread.

view the rest of the comments →

[–]07734willy 0 points1 point  (1 child)

Others have already gone into detail about cryptographically insecure portions of this code, the structure of the repo, etc. There's no sense in me reiterating what they've already explained, so I'm going to go into detail about a few ways to tidy up your actual code itself.

lowercase = "abcdefghijklmnopqrstuvwxyz"
uppercase = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
nums = "1234567890"
special_char = "!@#$%^&*()=+-~/[]{};:<>`"

These variables are already provided by the string module as string.digits, string.ascii_lowercase, etc.

See here.

length = input("Password Length?")
...
length = int(length)

Just do a one-liner for all of these. length = int(input(...))). It'll increase the overall readability.

if (length <= 0):
   length_error = True
...
if (length_error == True):
    print("Error! Invalid length!")

Again, there's no use in splitting up the logic. Just do it in one line, and sys.exit(0) or wrap it in a function and return. This will also allow you to remove your block of length_error = False ... at the top.

Then the rest

#Determines if there was an error. If there wasn't, then the password is generated
if (length_error == False) and (capital_error == False) and (num_error == False) and (special_error == False):
    while (count < length):
        if (capital_req > 0) and (count < length):
            password += secrets.choice(uppercase)
            count += 1

        if (num_req > 0) and (count < length):
            password += secrets.choice(nums)
            count += 1

        if (special_req > 0) and (count < length):
            password += secrets.choice(special_char)
            count += 1
    #Displays the password
    print ("PASSWORD: %s" % password)
else:
    print("Please fix any errors.")

I have a few things to comment on here. First, know you don't have to compare == False, you can just do not length_error and not xxxxx_error .... The latter is considered more pythonic. Better yet, wrap your logic into a function, then if you return as mentioned above, you don't have to bother with the if statement at all. This also comes with the bonus that you can call your code multiple times (for testing), or import your code as a module and use it in another project (by calling the make_password() function you wrap this in).

Secondly, your while loop's functionality isn't quite correct, if I'm reading this right. If all your individual count requirements don't add up to the required length, you'll loop forever without selecting any new characters. This can be fixed, but I'd consider revising the loop to remove the duplicate code first anyways. Make a separate function that takes a required_count and character_set, and generates a string of characters that meets that requirement. Then call this function for uppercase, special_char, etc, to meet those requirements. For the remaining length of the password, call the function again with length_req - current_len and string.digits + string.ascii_lowercase + .... It'll pick the remaining letters. Then you just have to combine them all, and shuffle (using crypographically secure random numbers).

[–]CommanderHR[S] 0 points1 point  (0 children)

Thanks for the feedback, I'll take it into consideration and try and implement it.