Tornado quiz app

Question

I am writing a simple quiz app using Tornado and MongoDB. The user authentication is done using Facebook and each user is uniquely identified by their Facebook ID. When a user first creates an account, his score will be set to zero and he can access only questions whose ID is less than or equal to his current score. And question IF start from zero.

The questions are accessed with URL and answers also submitted using URL only. Following is the URL scheme for valid for accessing questions:

myapp.com/q_id 
myapp.com/1
myapp.com/1/

The following scheme is valid for submitting answers:

myapp.com/q_id/answer
myapp.com/1/hi
myapp.com/1/hi/

So, in summary:

• User logs in via Facebook, his Facebook ID will be stored in cookie

  self.set_secure_cookie('user_id', str(user['id']))
  

• He will presented with the homepage where his current score is displayed and link to next question by: myapp.com/score

  user_id = self.get_secure_cookie('user_id') 
  if user_id:
   users = self.application.db.users
   search_user = users.find_one({'user_id': user_id})
   if not search_user: #i.e. new user
   score = 0
   name = self._get_fb_name(user_id)
   users.save({'user_id': user_id, 
   'score': score,
   'name' : name
   }) 
   else:
   score = search_user.get('score')
   name = search_user.get('name')
  

• The user can access only questions whose ID equal to his current score (i.e. his next challenge) or less than current score (i.e. previously correctly answered questions)

  current_score = int(self.get_secure_cookie('score'))
  if int(q_id) > current_score:
   self.write('You cannot access this question as your score is less')
   return
  

• User submits answers as explained in above URL schemes. If answer is correct, then his score is updated in DB.

  question_data = questions.find_one({'q_id': q_id})
  if not question_data:
   self.write('Huh, this question no exist')
   return
  if not answer:
   # user is requesting for next question
   self.render("quest-template.html", qdata = question_data)
   return
  else:
   if answer in question_data['q_ans']:
   current_score += 1
   # update score in db and in cookie
   self.write('Correct answer, move to next question')
   return
   else:
   self.write('Wrong answer buddy')
  

Here is the complete Tornado app code:

#!/usr/bin/env python
import os.path
import os
import tornado.auth
import tornado.escape
import tornado.httpserver
import tornado.ioloop
import tornado.options
import tornado.web
from tornado.options import define, options
import pymongo
from pymongo import MongoClient
import requests
import views
from settings import *
define("port", default=8080, help="run on the given port", type=int)
class Application(tornado.web.Application):
 def __init__(self):
 handlers = [
 (r'/', MainHandler),
 (r'/auth/login', LoginHandler),
 (r'/auth/logout', LogoutHandler),
 (r'/(\d+)(/?\w*)', Q0Handler)
 ]
 settings = application_handler_setttings
 conn = MongoClient()
 self.db = conn["mydb"]
 tornado.web.Application.__init__(self, handlers, **settings)
class BaseHandler(tornado.web.RequestHandler):
 def get_current_user(self):
 if self.get_secure_cookie('user_name'):
 return True
 return None
class MainHandler(tornado.web.RequestHandler):
 def get(self):
 user_id = self.get_secure_cookie('user_id') 
 if user_id:
 users = self.application.db.users
 search_user = users.find_one({'user_id': user_id})
 if not search_user:
 score = 0
 name = self._get_fb_name(user_id)
 users.save({'user_id': user_id, 
 'score': score,
 'name' : name
 }) 
 else:
 score = search_user.get('score')
 name = search_user.get('name')
 self.set_secure_cookie('score', str(score))
 self.render("index.html", user_name = name, score = score)
 else:
 self.render("login.html")
 def _get_fb_name(self, user_id):
 fb_graph_api_url = 'https://graph.facebook.com/%s' % user_id
 r = requests.get(fb_graph_api_url)
 return dict(r.json()).get('name')
class Q0Handler(BaseHandler):
 @tornado.web.authenticated
 def get(self, q_id, answer=None):
 answer = answer[1:]
 questions = self.application.db.questions
 users = self.application.db.users
 user_id = self.get_secure_cookie('user_id')
 current_score = int(self.get_secure_cookie('score'))
 if int(q_id) > current_score:
 self.write('You cannot access this question as your score is less')
 return
 question_data = questions.find_one({'q_id': q_id})
 if not question_data:
 self.write('Huh, this question no exist')
 return
 if not answer:
 self.render("quest-template.html", qdata = question_data)
 return
 else:
 if answer in question_data['q_ans']:
 current_score += 1
 self.set_secure_cookie('score', str(current_score))
 users.update({'user_id': user_id}, {'$set': {'score': current_score}})
 self.write('Correct answer, move to next question')
 return
 else:
 self.write('Wrong answer buddy')
class LoginHandler(tornado.web.RequestHandler, tornado.auth.FacebookGraphMixin):
 @tornado.web.asynchronous
 def get(self):
 user_id = self.get_secure_cookie('user_id')
 if self.get_argument('code', None):
 self.get_authenticated_user(
 redirect_uri=redirect_url,
 client_id=self.settings['facebook_api_key'],
 client_secret=self.settings['facebook_secret'],
 code=self.get_argument('code'),
 callback=self.async_callback(self._on_facebook_login))
 return
 elif self.get_secure_cookie('access_token'):
 self.redirect('/')
 self.authorize_redirect(
 redirect_uri=redirect_url,
 client_id=self.settings['facebook_api_key'],
 extra_params={'scope': 'user_photos, publish_stream'}
 )
 def _on_facebook_login(self, user):
 if not user:
 self.clear_all_cookies()
 raise tornado.web.HTTPError(500, 'Facebook authentication failed')
 self.set_secure_cookie('user_id', str(user['id']))
 self.set_secure_cookie('user_name', str(user['name']))
 self.set_secure_cookie('access_token', str(user['access_token']))
 self.redirect('/')
class LogoutHandler(tornado.web.RequestHandler):
 def get(self):
 self.clear_all_cookies()
 self.render('logout.html')
def main():
 tornado.options.parse_command_line()
 http_server = tornado.httpserver.HTTPServer(Application())
 http_server.listen(options.port)
 tornado.ioloop.IOLoop.instance().start()
if __name__ == "__main__":
 main()

1. How do I improve my regex and is there any to avoid answer = answer[1:]? Currently my regex is so that the second parameter answer also contains leading slash. To remove it I am using slicing.
2. At every correct answer, I am updating the database and also the cookie. However, when I need to check his score, I am only seeing the cookie. Is that bad?
3. Earlier, I was not updating the database at every current answer, instead I was saving it in just cookie. But when user logs out, I save the scores to the database. Is this the right/suggested way?

Answer 1

I'm not a Tornado expert, so these comments will be general to Python web development.

I'll first answer your specific questions

1. I don't know about your regexp, but you have a serious problem with Q0Handler.get; when answer = None (as is the default parameter), the line answer = answer[1:] will fail. I don't know if the code never gets called with the default parameter, in which case you don't need the default, but otherwise, you should probabaly change this to something like answer = answer[1:] if answer is not None or None.

2. I would recommend checking the DB. In general, I like to think of all user data coming under 3 categories: Untrusted, Trusted but not reliable, Trusted and reliable. In web development, we can think of cookies as Untrusted (i.e. the user may manipulate they at will), secure/encryped cookies as Trusted but not reliable (i.e. the user may not manipulate the contents without it being obvious to you, but it's not guaranteed to {be,stay} {there,well formed}), and the database can be trusted (i.e. if I put data in there, it should stay there, though I note you are using MongoDB, so that invariant may not be applicable here).

When it comes to implementing those properties, I would say this: you need to handle the case where someone deletes a cookie (i.e. wrap self.get_secure_cookie('score') in a try catch or test for 'score''s existence before hand). You also need to deal with cases where people are logged in on two browsers; if you just get data from cookies, then these will have different scores, which could be frustrating for the user. So all in all, It would just be easier to use the database in most places.

---

I'll give some general comments now

Application

You currently hard code your urls, which could lead to a very brittle structure, and make extensibility a pain. Have you considered using class decorators to register route handlers? They'd look something like this:

class Application(tornado.web.Application):
 handlers = []
 @staticmethod
 def register_route(route):
 def inner(handler_class):
 Application.handlers.append((route, handler_class))
 return handler_class
 return inner
 ... # The rest of your Application class
 @Application.register_route(r'/foobar/(\d+)')
 class MyFoobarHandler(tornado.web.RequestHandler):
 .... # Standard handler junk

However, that's probably just because I'm a Flask user, so I'm in love with decorators.

Conciser using super(Application, self).__init__(...) over explicitly calling the constructor of the super class (this may not be possible, due to complications with old style classes, though don't quote me on that).

"mydb" wins the word for the worst name for a database I've seen all year ;)

BaseHandler

Having a function that returns True or None is usually a code smell. If you want a function that returns True and some value indicating the converse of True, usually people use False. In that case, you can rewrite the function as

def get_current_user(self):
 return bool(self.get_secure_cookie("user_name"))

MainHandler

The name MainHandler tells me nothing. Due to your routes being defined in a different place, maybe naming it something like IndexHandler would be better (though I still don't like that).

An oft advised idiom on this site is the use of guard clauses. I'd recommend using that here; instead of

if user_id:
 ...
 10 lines of code and 3 indentation levels ommited
 ...
 self.render("index.html")
else:
 self.render("login.html")

Something like this will read easier

if not user_id:
 self.render("login.html")
 return
 # Insert your 10 lines of code and many indentation levels here
 ...
 self.render("index.html")

See something like this for a better justification.

In _get_fb_name, some things to note:

• If you were feeling REALLY paranoid (it's the best way to be), you should call urlencode on the user_id variable before constructing a URL with it.
• You should check r.status_code to make sure the request succeeded.
• If the request is well formed, r.json() will return a python dict (assuming the json has an object on the top level); no need to wrap it in a dict call. I would however check that it is indeed a dict that you are getting.
• My, you seem to love returning None in place of raising an exception or something. I'd recommend against it; returning a None is easy to overlook, and exceptions give you a glorious traceback.

Q0Handler

Numbers in class names make me feel slightly sick, and I have no idea what Q0 means; question 0? Because this view seems to handle all questions, not just question 0.

We've already discussed the answer = answer[1:] bug. There are a couple of other bugs; if I delete my score cookie, I'll end up causing a bug when you try to do int((self.get_secure_cookie('score')); int(None) -> TypeError (same problem exists for user_id).

When you do users.update to increment the score, I think you could be vulnerable to a race condition, but that's not too big a problem (I think Mongo supports an atomic increment, use that instead).

Python convention when passing named parameters is to use foo(bar=foz) spacing not foo(bar = foz), but that's just stylistic.

---

I think that covers it from me. There will probably be someone out there who will turn up shortly to shout at me for talking about security without a PhD, but I think you should be OK (Hash your passwords, kids!).

• avi: Thank you very much for great feedback ;-) I agree mydb is the worst name! Earlier I was using different handlers for questions and answers, in the process of renaming, I forgot about Q0Handler. I agree again, another worse name. In Q0Handler, I think it never gets called with answer=None, thats why I am not getting any error when I am trying to do answer=answer[1:]. If the answer is not present in URL, then it will be called with empty string and that's why slicing doesn't throw any error.
• avi: I am not hardcoding URLs and I am following standard examples from Tornado itself. But I will look into it. I don't know Flask, so I cannot really comment. Rest all your advice makes sense to me and I will correct my mistakes :)