6
\$\begingroup\$

I've written this script as a basic authentication system for use on Google App Engine. I would appreciate it if anyone would mind having a look and seeing if there are any obvious security vulnerabilities.

Overview

The whole thing is an API so obviously there will be a client (probably written in JavaScript) which will interface with it.

User creates an account using /user/register and a 'random' code is generated using the time, their email address and the key for 'their' entity. This is then emailed to them with a link to the verification page.

This then checks the verification code against one which is generated again using the same values as before (the rationale being that an attacker would need to have access to the code as well as the datastore to be able to generate a code to use in the verification).

Authentication is then done using either and auth key or the users password.

The auth key is generated using the entity key, their IP address and their email address and is stored. This key is returned if the correct password had been given and therefore 'proves' that the client knows/knew the password. This would then be stored by the client (perhaps using the JavaScript localstorage API) so that actions can be done on the account without needing to re-enter the password.

An example of the auth code being used can be seen in the protectedthing class. This would allow some action to be take which required access to the account but which wasn't too critical.

There is also the option to authenticate using a password as seen in the realyprotectedthing class. This would be used for something which was more critical and where we needed to ensure that it really was the user.

There are some obvious areas of improvement such as password validation but I would like to check that the basic idea seems safe.

protectedthing and reallyprotectedthing are examples of how the other functions would be used. protectedthing is some sort of action which only needs the auth code for authentication where as reallyprotectedthing is more secure and requires the user to reenter the password.

pepper = "xxxxxxxxxxxxxxxxxxxx"
uniquepepper = "xxxxxxxxxxxxxxxxxxxx"
import webapp2
import datetime
from google.appengine.ext import db
from google.appengine.api import mail
from webapp2_extras import security


class useraccount(db.Model):
  email = db.StringProperty(indexed=True)
  password = db.StringProperty()
  verified = db.BooleanProperty(default=False)
  verificationcoderequested = db.DateTimeProperty(default=None)
  created = db.DateTimeProperty(auto_now_add=True)
  authcodes = db.StringListProperty()
  def getverificationcode(self):
      time = datetime.datetime.now()
      self.verificationcoderequested = time
      self.put()
      return security.hash_password((str(self.key)[8:10]+self.email+str(time)), method='sha1',pepper=uniquepepper).split('$')[0]

  def checkverificationcode(self,code):
      if self.verificationcoderequested != None:
          actual = security.hash_password((str(self.key)[8:10]+self.email+str(self.verificationcoderequested)), method='sha1',pepper=uniquepepper).split('$')[0]
          if code == actual:
              self.verified = True
              self.verificationcoderequested = None
              self.put()
              return "success"
          else:
              return "codes do not match"
      else:
          return "no code requested, code expired or already verified"
      return "failure"

  def checklogin(self,password):
      if security.check_password_hash(password, self.password, pepper=pepper) == True:
          if self.verified == True:
              return "correct"
          else:
              return "confirm email"
      else:
          return "incorrect"

  def checkauth(self,auth):
        if auth in self.authcodes:
            return "correct"
        else:
            return "incorrect"

  def createauth(self,ip):
      authcode = security.generate_password_hash((str(self.key)[8:10]+self.email + ip), method='sha1', length=12, pepper=uniquepepper).upper().split('$')[0][0:5]
      self.authcodes.append(authcode)
      self.put()
      return authcode
class MainHandler(webapp2.RequestHandler):
    def get(self):
        self.response.write("hello")

class register(webapp2.RequestHandler):
    def post(self):
        email = self.request.get("email")
        u = useraccount.all().filter('email =', email)
        if not u.get():
            user = useraccount()
            user.email = email
            user.password = security.generate_password_hash(self.request.get("password"), method='sha1', length=12, pepper=pepper)
            user.put()
            mail.send_mail("email@example.com",user.email,"Account Registration Email Confirmation",'You have succesfully registered for an account, please confirm you email address using the link bellow to get started </br> <a href="http://localhost:9080/user/confirmregistration?code=' + user.getverificationcode() + '&email='+user.email+'"></a>')
            self.response.write(email+" account created " )
        else:
            self.response.write("Email already exists")

class confirmregistration(webapp2.RequestHandler):
    def get(self):
        email = self.request.get("email")
        code = self.request.get("code")
        user = useraccount.all().filter('email =', email).get()        
        self.response.write(user.checkverificationcode(code))

class login(webapp2.RequestHandler):
    def post(self):
        email = self.request.get("email")
        password = self.request.get("password")
        user = useraccount.all().filter('email =', email).get()
        loginstatus = user.checklogin(password)
        if loginstatus == "correct":
            self.response.write("authorised " + user.createauth(self.request.remote_addr))
        else:
            self.response.write("invalid login " + loginstatus)

class protectedthing(webapp2.RequestHandler):
    def post(self):
        email = self.request.get("email")
        auth = self.request.get("auth")
        user = useraccount.all().filter('email =', email).get()
        loginstatus = user.checkauth(auth)
        if loginstatus == "correct":
            #do thing
            self.response.write("thing done")
        else:
            self.response.write("invalid auth")

class reallyprotectedthing(webapp2.RequestHandler):
    def post(self):
        email = self.request.get("email")
        password = self.request.get("password")
        user = useraccount.all().filter('email =', email).get()
        loginstatus = user.checklogin(password)
        if loginstatus == "correct":
        #do really protected thing
            self.response.write("thing done")
        else:
            self.response.write("invalid login " + loginstatus)

app = webapp2.WSGIApplication([
    ('/user', MainHandler),
    ('/user/login', login),
    ('/user/protectedthing', protectedthing),
    ('/user/reallyprotectedthing', reallyprotectedthing),
    ('/user/register', register),
    ('/user/confirmregistration', confirmregistration),
], debug=True)
\$\endgroup\$
  • \$\begingroup\$ protectedthing and reallyprotectedthing aren't very distinct names from each other and don't communicate what they are supposed to do very well \$\endgroup\$ – syb0rg Dec 20 '14 at 18:54
  • \$\begingroup\$ Sorry, I suppose they are but they are only examples of how the other functions would be used. \$\endgroup\$ – user1492867 Dec 20 '14 at 19:01
3
\$\begingroup\$

I've never used Google App Engine so I can't comment on your approach and the biggest questions, but there are a couple of coding practice issues worth noting.

PEP8

You are clearly not following PEP8, the official coding style guide of Python. Give it a good read and start following it to improve the readability of your code. The most notable violations that jump in the eye:

  • Need more line breaks:
    • There should be 2 empty lines before top-level class or function definitions
    • There should be 1 empty line before function definitions within a class
  • Put spaces around operators, for example:
    • Instead of: str(self.key)[8:10]+self.email
    • Write like this: str(self.key)[8:10] + self.email
  • Some lines are way too long

    • For example this is a bit excessive, don't you think?

      mail.send_mail("email@example.com",user.email,"Account Registration Email Confirmation",'You have succesfully registered for an account, please confirm you email address using the link bellow to get started </br> <a href="http://localhost:9080/user/confirmregistration?code=' + user.getverificationcode() + '&email='+user.email+'"></a>')
      
  • Use snake_case instead manywordsstucktogether for function and variable names

Overall, this code looks very messy. For an API, especially security related, it's really better to make the code as clear and readable as possible, and following the PEP8 rules will help with that.

Duplicated logic

This kind of call appears at multiple places in the code:

((str(self.key)[8:10]+self.email+str(self.verificationcoderequested))

(str(self.key)[8:10]+self.email + ip)

The problem with duplicated logic is that later if you need to change something in that expression, you have to remember to make the same change everywhere. To avoid making a mistake, it's better to put this logic into a dedicated helper function, for example:

def create_key(self, extra):
    return str(self.key)[8:10] + self.email + extra

Pythonic conditions

These conditions are not Pythonic:

      if self.verificationcoderequested != None:
          # ...

      if self.verified == True:
          # ...

The Pythonic way is simply:

      if self.verificationcoderequested:
          # ...

      if self.verified:
          # ...

Also, when you really need to compare with None, you should use is None and is not None instead of == None and != None, respectively.

Other issues

At the end of checkverificationcode, the last return statement (return "failure") is unreachable code, you should remove it.

\$\endgroup\$

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.