10
\$\begingroup\$

Can the following code be simplified? I think that the program that I have created is very excessive and quite slow. The program runs fine, but I would like to understand if this could actually be programmed better.

I have commented on every line of the code. Any kind of feedback on the program will be great, as I have only been programming for a couple of weeks now.

# Python 3.4.3. Using MacOS (Version 10.12.5)
# Username and Password...
# The programs purpose: A user must enter the correct username and password for a site called FaceSnap...
# The correct username is elmo and the correct password is blue.


userName = input("Hello! Welcome to FaceSnap! \n\nUsername: ") #Ask's the User for Username input
password = input("Password: ") # Ask's the user for their password


count = 0 # Create a variable, to ensure the user has limited attempts at entering their correct username and password
count += 1 # The user has already had one attempt above, therefore count has been incremented by 1 already.


while userName == userName and password == password: # The Input will always lead to this while loop, so we can see if their username and password is wrong or correct.


    if count == 3: # Counter, to make sure the user only gets a limited number (3)of attempts
        print("\nThree Username and Password Attempts used. Goodbye") # Lets the user know they have reached their limit
        break # Leave the Loop and the whole program


    elif userName == 'elmo' and password == 'blue': # The userName and password is equal to 'elmo' and 'blue', which is correct, they can enter FaceSnap!
        print("Welcome! ") # Welcomes the User, the username and password is correct
        break # Leave the loop and the whole program as the username and passowrd is correct


    elif userName != 'elmo' and password != 'blue': # The userName and password is NOT equal to 'elmo' and 'blue', the user cannot enter FaceSnap
        print("Your Username and Password is wrong!") # Lets the user know that the Username and password entered is wrong.
        userName = input("\n\nUsername: ") # Requests the user to have another attempt at entering their correct username
        password = input("Password: ") # Requests the user to have another attempt at entering their correct password
        count += 1 # Increments the count by 1
        continue # Continue, as the user hasn't managed to get their username and password correct yet


    elif userName == 'elmo' and password != 'blue': # The userName is equal to 'elmo', but password is NOT equal to 'blue', the user cannot enter FaceSnap
        print("Your Password is wrong!") # Lets the user know that their password is wrong
        userName = input("\n\nUsername: ") # Requests the user to have another attempt at entering their correct username
        password = input("Password: ") # Requests the user to have another attempt at entering their correct password
        count += 1 # increments the count by 1
        continue # Continue, as the user hasn't managed to get their username and password correct yet


    elif userName != 'elmo' and password == 'blue': # The userName is NOT equal to 'elmo', however password is equal to 'blue', the user cannot enter FaceSnap
        print("Your Username is wrong!") # Lets the user know that their username is wrong
        userName = input("\n\nUsername: ") # Requests the user to have another attempt at entering their correct username
        password = input("Password: ") # Requests the user to have another attempt at entering their correct password
        count += 1 # Increments the count by 1
        continue # Continue, as the user hasn't managed to get their username and password correct yet
\$\endgroup\$
11
\$\begingroup\$

A few points:

  • You can simplify the if loop by using the else condition.

  • Getting input should be at the beginning of the while loop, because it makes more logical sense to put it there. It's what happens every time you restart.

  • You can use the simple condition of True for your while loop.

  • You don't need the continue keyword, as it is at the end of your loop.


count = 0 
while True: 
    userName = input("Hello! Welcome to FaceSnap! \n\nUsername: ") 
    password = input("Password: ")
    count += 1
    if count == 3: 
        #tells user bye
        break #exit
    else:
        if userName == 'elmo' and password == 'blue':
            #let them in
            break #they are in, exit loop
        else:
            #tell them it is wrong and have them retry, stay in loop
\$\endgroup\$
  • \$\begingroup\$ Excellent, thank you for your time and points above. I will work on your above comments. Finally, I ran your code through python Idle and it was much more simple. Can you recommend a good IDE for Python please? \$\endgroup\$ – Greg May 28 '17 at 21:31
  • \$\begingroup\$ It is better to write while count < 3: and remove if from the loop body \$\endgroup\$ – belkka Jul 6 '18 at 15:16
4
\$\begingroup\$

Comments

Your code is hard to read through, because of the excessive use of comments. Comments should add something to code, not repeat the code, or tell the reader what the code already strongly implies.

Use hashes

Merely hashing is very weak, but this is rather easy to implement. Also note that md5 is prone to hash collision attacks and should not be used anymore. If your platform supports it, use SHA(3)-256.

The problem with this application is that if anybody gets to access the code, they can read the username and password in plaintext. You would be better off first hashing the passwords, then using hashlib to check if the input matches. I've written an improved version of your code below, to show what I mean:

from hashlib import md5
from getpass import getpass
import sys

print("Hello! Welcome to FaceSnap!") 

attempts = 0
check_username = "5945261a168e06a5b763cc5f4908b6b2"
check_password = "48d6215903dff56238e52e8891380c8f"
# These hashes have been generated earlier on.
# This is not how you would go about storing usernames and passwords,
# but for the sake of simplicity, we'll do it like this.

while True: 
    username = input("Username: ")
    password = getpass("Password: ")
    # Getpass will not echo input to the screen, so your password remains 
    # invisible
    print()

    if attempts == 3:
        sys.exit("Too many failed attempts.")

    if md5(username.encode().hexdigest() == check_username:
        if md5(password.encode().hexdigest() == check_password:
            print("Username and password entered correctly.")
            # Username and password match - do something here
        else:
            print("Password entered incorrectly.")
            attempts += 1
    else:
        print("Username entered incorrectly.")
        attempts += 1

Use Scrypt

In reality, you wouldn't hash passwords to store them in a database, but use a dedicated key derivation function, such as Scrypt. The first cryptography library for Python that comes to mind is PyCrypto, but cryptography makes it significantly harder to shoot yourself in the foot.

\$\endgroup\$
  • 1
    \$\begingroup\$ Firstly, Thank you for your reply. I do have two questions regarding the hashes in the variables check_username and check_password. How did you create the hashes to represent elmo (username) and blue (password)? Finally, how does the program know that the hashes are equal to elmo and blue? \$\endgroup\$ – Greg May 28 '17 at 18:38
  • 1
    \$\begingroup\$ Hey Greg, I started a Python session and used from hashlib import md5 => md5(b"elmo").hexdigest() and md5(b"blue").hexdigest(). A hash is fixed-length digest of a message. In this case, a 'message' is a string of any length. This means you can hash one character, but also a whole book. Something to note is that hashing a string multiple times will always generate the same hash. You may want to read about hashing algorithms, it's quite interesting. Now on to your question, the program also imports md5, hashes the input and compares the hashes to the hashes it knows. Hope I helped! \$\endgroup\$ – Daniel May 28 '17 at 18:51
  • 2
    \$\begingroup\$ Brilliant. I understand this now, I will definitely do some further reading on hashing algorithms! By the way, I meant this section of the code* when I mentioned variable. Thank you again! *check_username = '5945261a168e06a5b763cc5f4908b6b2' \$\endgroup\$ – Greg May 28 '17 at 19:47
  • 2
    \$\begingroup\$ Hi @Coal_ I hope you are well. Apologies for an additional question. I have just noticed that the hashes are always producing the same encryption, for the same word. Therefore, couldn't a person who had gained access to the source easily find out that '5945261a168e06a5b763cc5f4908b6b2' is equal to 'elmo', for example? Thank you! \$\endgroup\$ – Greg May 29 '17 at 19:45
  • 1
    \$\begingroup\$ G'day to you again :P don't worry about asking another question, I'm happy to help. Regarding your question, the idea of hashing is not to be able to reverse it. This means it isn't technically encrypting anything, because you cannot decrypt a given hash. A common attack, however, can be a rainbow-tables attack (exploit-db.com/docs/104.pdf). Simplified; attackers try hashing common passwords (1234, passw0rd, etc.) until a hash matches the one they found in the database. \$\endgroup\$ – Daniel May 30 '17 at 5:14
3
\$\begingroup\$

This will be a short one, but when you start you write count = 0, and then count += 1, where you could just as easily written count = 1 to start off with.

\$\endgroup\$
1
\$\begingroup\$

Or you can do this:

attempts=0
while attempts<3:
    username=input('username?')
    password=input('password?')
    if username=='correctusername'and password=='correctpassword':
        print('you are in!')
    else:
        attempts+=1
        print('incorrect!')
        if attempts==3:
            print('too many attempts')

What this does is create a loop that asks you for a correct username and password, and checks if it is correct through an if-statement. Then, when the attempts have exceeded the maximum amount of allowed attempts, tells the user that.

\$\endgroup\$
  • 1
    \$\begingroup\$ I made it shorter. Shouldn't that be what the asker wanted? Simplified? \$\endgroup\$ – John Hao May 28 '17 at 10:42
  • 1
    \$\begingroup\$ Okay. Sorry. I understand what you are saying. \$\endgroup\$ – John Hao May 28 '17 at 12:13
  • \$\begingroup\$ Hi, thank you for the attempt at answering this, however I have decided which answer was most suitable for my question. Thank you. \$\endgroup\$ – Greg May 30 '17 at 23:23
-1
\$\begingroup\$

In terms of design, there are two main things you should take in consideration:

  1. It is not good to have lot of if statements, especially in your case. You need to refactor your code in the name of the Anti-If Compaign.
  2. You should accomplish the tasks through small units of code called functions.

In terms of security, there are also two main things to consider:

  1. When an attacker targets your applications and types the right username, you inform him that the user exists, but only the password is wrong. This is bad in the security perspective because it increases the attack surface of your application. You can overcome this by displaying a more generic and less informative error message instead.
  2. I do not know if your application is running on the Internet or offline. It is recommended that you hide the password which is being typed for the sake of reducing the attack surface too.

Given the above 4 observations, I want to provide you an improved version of your code through an MCVE where:

  • I use getpass module to hide the password when it is typed.
  • I use a predefined user credentials to simulate a more realistic situation (such as may be in your case you get the credentials from a database)
  • I use a separate function to read the user credentials
  • I use an other function which only purpose is to validate the user input.

These steps will lead us to using one if statement only:

#!/usr/bin/env python
# -*- coding: utf-8 -*-
import getpass


user_credentials = ['begueradj', 'password']

def get_user_credentials():
   username = input('Username: ')
   password = getpass.getpass('Password: ')   
   return username, password

def check_user_credentials(user_credentials):
   username, password = get_user_credentials()
   return user_credentials[0] == username and user_credentials[1] == password


if __name__ == "__main__":
    login_attempts = 1
    correct_credentials = check_user_credentials(user_credentials)
    while not correct_credentials and login_attempts < 3:  
       print('\nWrong credentials, try again:')  
       correct_credentials = check_user_credentials(user_credentials)
       login_attempts += 1
    if correct_credentials:
       print('Welcome! Do whatever work you need here ...')
    else:
       print('Game over!')
\$\endgroup\$
  • 2
    \$\begingroup\$ When I read this, I didn't think it implies that it's ok to store passwords in plain text, contrary to what Coal_ says. But, I find it painful to see such code, and I'm concerned that some inexperienced readers might be mislead by this. I'll remove the -1 if you rework that. \$\endgroup\$ – Stop ongoing harm to Monica May 28 '17 at 16:38
  • \$\begingroup\$ Hi, thank you for this. I have now made the answer clear at the top of the page. Thank you \$\endgroup\$ – Greg May 30 '17 at 23:24
  • \$\begingroup\$ Then be honest with yourself by downvoting my answer @Coal_ \$\endgroup\$ – Billal Begueradj May 31 '17 at 16:43
  • 2
    \$\begingroup\$ I have chosen not to downvote your answer because there was no need to do so at the time and there is even less reason to do so now. I explained why I didn't agree with your solution, instead of giving you a rather meaningless downvote. \$\endgroup\$ – Daniel May 31 '17 at 18:54
  • \$\begingroup\$ Unbelievable hypocrisy: you accused me twice of encouraging to store passwords in plain text but you say it is meaningless to downvote? I am speechless (Also you mentioned hashing password in your answer which was posted previous to mine, so why do you ask people to include your answer and repeat it in theirs?) I have no respect for intellectually dishonest people. End of discussion @Coal_ \$\endgroup\$ – Billal Begueradj Jun 8 '17 at 4:03

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