Oauth 2.0 handler functions for Tornado

Question

With Tornado 3.2 they made some updates to auth module and have updated the code. Earlier I was using open id for Google login, since it will be deprecated in the future I am switching the code to Oauth 2.0 login. Also I was using tornado.web.asynchronous, now I have updated the code to use tornado.gen.coroutine

/products is only available for logged in users. Once the user is authenticated from Google or Facebook, a cookie called 'trakr' is set. In implementation of get_current_user() whether 'trakr' is set or not is checked.

class BaseHandler(tornado.web.RequestHandler):
 def get_current_user(self):
 user = self.get_secure_cookie('trakr')
 if not user: return None
 return True
class FAuthLoginHandler(BaseHandler, tornado.auth.FacebookGraphMixin):
 @tornado.gen.coroutine
 def get(self):
 if self.get_current_user():
 self.redirect('/products')
 return
 if self.get_argument('code', None):
 user = yield self.get_authenticated_user(
 redirect_uri=settings.fb_redirect_url,
 client_id=self.settings['facebook_api_key'],
 client_secret=self.settings['facebook_secret'],
 code=self.get_argument('code'))
 if not user:
 self.clear_all_cookies()
 raise tornado.web.HTTPError(500, 'Facebook authentication failed')
 user = yield self.facebook_request("/me", access_token=user["access_token"])
 if not user:
 raise tornado.web.HTTPError(500, "Facebook authentication failed.")
 self.set_secure_cookie('trakr', user.get('email'))
 self.redirect('/products')
 return
 elif self.get_secure_cookie('trakr'):
 self.redirect('/products')
 return
 else:
 yield self.authorize_redirect(
 redirect_uri=settings.fb_redirect_url,
 client_id=self.settings['facebook_api_key'], 
 extra_params={'scope': 'email'}
 )
class GAuthLoginHandler(BaseHandler, tornado.auth.GoogleOAuth2Mixin):
 @tornado.gen.coroutine
 def get(self):
 if self.get_current_user():
 self.redirect('/products')
 return
 if self.get_argument('code', False):
 user = yield self.get_authenticated_user(redirect_uri=settings.google_redirect_url,
 code=self.get_argument('code'))
 if not user:
 self.clear_all_cookies() 
 raise tornado.web.HTTPError(500, 'Google authentication failed')
 access_token = str(user['access_token'])
 http_client = self.get_auth_http_client()
 response = yield http_client.fetch('https://www.googleapis.com/oauth2/v1/userinfo?access_token='+access_token)
 user = json.loads(response.body)
 self.set_secure_cookie('trakr', user['email']) 
 self.redirect('/products')
 return
 elif self.get_secure_cookie('trakr'):
 self.redirect('/products')
 return
 else:
 yield self.authorize_redirect(
 redirect_uri=settings.google_redirect_url,
 client_id=self.settings['google_oauth']['key'],
 scope=['email'],
 response_type='code',
 extra_params={'approval_prompt': 'auto'})

Answer 1

There are a few small improvements I can see:

1. Your get_current_user() function is slightly misleading. Currently, you return None if there is no current user, or True if there is. This function isn't returning the current user, its returning if there is a current user.

   If I assume the return value of get_secure_cookie() is a basic string, the method can actually be reduced to this single line:

   # On a valid return, the return value will be a non-empty string == True.
   # Otherwise, it will either be None or '', both of which == False.
   return self.get_secure_cookie('trakr')
   

   Since, this function is just passes through of the return value of another function, it really isn't necessary. Unless you wanted to keep it to describe what the return value of self.get_secure_cookie() could mean, I would suggest removing it entirely.

2. In your get() functions you have this if structure:

   if get_current_user():
    self.redirect('/product')
    return
   

   followed by:

   if self.get_argument('code', None):
    ...
   elif self.get_secure_cookie('trackr'):
    self.redirect('/products')
    return
   

   First you check for a current user; if there is none you continue on. Then, if you get a False return value from self.get_argument('code', None), you immediately check again. From what I can tell from your snippet, unless you expect a user to get logged in during the time it takes to evaluate two if statements, the second user check is redundant.

3. I would change your naming conventions a bit. The G and F at the beginnings of your classes can be understood when looking at the source code. However, away from the source the F and G can be ambiguous.

   I would recommend using the names: FacebookLoginHandler and GoogleLoginHandler.

   Also, instead of trackr I would use the full work tracker. Saving 1 character isn't worth it.

4. I would use str.format() instead of string concatenation. You only do this once and with only two strings. However, string formatting is more Pythonic and it, personally, describes better what the final string will look like:

   'Hello' + ' ' + 'world' + '!' vs. '{} {}!'.format('Hello', 'world')
   

5. My final point is more personal preference than a sure-fire Python convention. Whenever you have to split up a single statement onto multiple lines, you indent the next line 4 spaces in. As alluded to earlier, this fine:

   yield self.authorize_redirect(
    redirect_uri=settings.fb_redirect_url,
    client_id=self.settings['facebook_api_key'], 
    extra_params={'scope': 'email'}
   )
   

   As a small aside, I don't like having the ending param on a line by itself; its too C/Java-like.

   However, my personal preference is to indent them to line up (or as close to in line) with the section of the statement they are related to. Take this 'simple' example:

   # Technically OK.
   some_long_variable_name = [foobar for foobar in map(ord, 'Hello World!')
    if foobar < 97]
   # My preference.
   some_long_variable_name = [foobar for foobar in map(ord, 'Hello World!')
    if foobar < 97]
   

   In the first statement above, its slightly difficult to tell at a glance the if statement is in the list comprehension. Even though they are related syntactically, they currently aren't related spatially.

   Now in the second statement, the if statement and the rest of the comprehension are related spatially which, personally, helps the code make more sense at a glance.

   Here is how I would structure your code:

   yield self.authorize_redirect(redirect_uri=settings.fb_redirect_url,
    client_id=self.settings['facebook_api_key'], 
    extra_params={'scope': 'email'})
   

---

Otherwise, your code looks pretty good. I really wanted to look more into a single Login class because your two classes are so similar. However, a solution did not come quickly to mind and creating one would go past the bounds of typical code review.

• avi: Amazing! Thank you very much. There is a typo in your answer, instead of get_source_cookie() it should be get_secure_cookie(). Regarding #1, you are right. Actually I use a boilerplate tornado code which I created when I first started learning python and tornado. However I still need to have get_current_user function since it will be used in @tornado.web.authenticated decorator. And I agree rest of your points. Cheers!