OAuth2 Implementation

I am trying to implement an OAuth2 service. I am not planning to use it anytime soon (like will write unit tests). This is my fun project I do to practice. I have used the python_jwt library to create and validate signatures.

OAuth Request handler

```
"""
Copyright (C) 2016 Maruf Maniruzzaman
Website: https://github.com/kuasha/cosmos
Author: Maruf Maniruzzaman
License :: OSI Approved :: MIT License

This service handler follows the style found in Azure (https://msdn.microsoft.com/en-us/library/azure/dn645542.aspx)
Also for more information look at The OAuth 2.0 Authorization Framework (http://tools.ietf.org/html/rfc6749)
"""

import ast
import base64
import json

import datetime
import uuid

import tornado.web
from tornado import gen
import logging

from cosmos.rbac.object import COSMOS_USERS_OBJECT_NAME
from cosmos.auth.oauth2 import get_token
from cosmos.rbac.object import SYSTEM_USER

try:
import urlparse # py2
from urllib import urlencode
import urllib as urllib_parse
except ImportError:
import urllib.parse as urlparse # py3
from urllib.parse import urlencode
import urllib.parse as urllib_parse

from cosmos.service.requesthandler import RequestHandler

OAUTH2_REQUESTS_OBJECT_NAME = "cosmos.auth.oauth2.requests"
OAUTH2_TOKENS_OBJECT_NAME = "cosmos.auth.oauth2.tokens"
OAUTH2_CODE_STATUS_OBJECT_NAME = "cosmos.auth.oauth2.codestatus"
OAUTH2_MAX_CODE_RESPONSE_LENGTH = 200

class OAuth2RequestException(Exception):
pass

#TODO: security check for entire class

class OAuth2ServiceHandler(RequestHandler):

@gen.coroutine
def get(self, tenant_id, function):
try:
req_id = None
auth_request = None
serve_request = self.get_argument("serve_request", None)
if not (serve_request):
auth_request = self._collect_auth_request_parameters(tenant_id, function)

req_id = yield self.insert_auth_request(auth_request)

if not req_id:

Second part first:

• In verify_token the return values are assigned - only to be

immediately returned. I get that it might be nice in a debugger(?)
but there's really no reason for this - just return the values. I'd
possibly create a function that wraps other functions in such an
exception handler, but that's probably overkill here.

• dict.get already defaults to returning None - I don't see a

particular reason why you'd specify that everywhere. Defaults are
really useful to remove that clutter from your code.

• I'd also inline a lot of those variables to make the methods shorter

(i.e. aud, exp and so on), or create a similar loop as described
in the next point.

-
The repeated pattern with x = kwargs.get("x"); if x: ...["x"] = x
should also be refactored to remove repetition, e.g. by using a loop
instead:

for name in ["iss", "nbf", "oid", "sub", "tid", "unique_name", "upn"]:
x = kwargs.get(name)
if x:
token_payload[name] = x


-
I'd also reorder code fragments that belong together. At the moment
the reader is jumping from service_private_pem to the initialisation
of token_payload to only go back to the first variable in the second
to last line, while token_payload is used in the last line. I'd
move the RSA.importKey after the first block, then handle the
token_payload and all associated things after, that way the order of
operation is clearly distinguished.

That leaves the following (could probably still be improved with more
factoring of code, but hey, looks compact enough to me):

def get_token(**kwargs):
service_private_pem = kwargs.get("service_private_pem")
if not service_private_pem:
raise ValueError("service_private_pem is not defined")
priv_key = RSA.importKey(service_private_pem)

token_payload = {"ver": "1.0"}

for name in ["aud", "exp", "family_name", "given_name"]:
token_payload[name] = kwargs.get(name)

for name in ["iss", "nbf", "oid", "sub", "tid", "unique_name", "upn"]:
x = kwargs.get(name)
if x:
token_payload[name] = x

return jwt.generate_jwt(token_payload, priv_key, 'RS256', exp)


Now for the first part. Well that's a lot of code. Okay, so small
stuff:

• Empty returns with no other afterwards don't do anything, so I'd leave

them out.

• logging already supports arguments using %s and friends - the

calls can thus be written like logging.debug("... %s", req_id) etc.

-
the six coroutine methods look awfully similar. I'm not too familiar
with generators or the framework, so this could be wrong:

def _request_helper(self, *args):
obj_serv = self.settings["object_service"]
promise = obj_serv.insert(SYSTEM_USER, OAUTH2_REQUESTS_OBJECT_NAME, *args)
req_id = yield promise
return req_id


This helper would then reused from the individual methods (though at
that point you could as well use that method directly?):

@gen.coroutine
def insert_auth_request(self, params):
return self._request_helper(params)


-
In _collect_auth_request_parameters the update of the dictionary
could be a bit more compact:

def _collect_auth_request_parameters(self, tenant_id, function):
params = {k: self.get_argument(k) for k in self.request.arguments}
params.update({
"request_protocol": self.request.protocol,
"request_host": self.request.host,
"request_uri": self.request.uri,
"tenant_id": tenant_id,
"function": function
})
return params


-
Having these giant try/except blocks is just increasing the
indentation. I'd suggest that the inner part is either compacted so
much as to fit into one screen, or moved into its own method.

• Similarly having the return cases early can remove the indentation

from the else branch, which makes the flow also a bit more clear.
Initialising variables to None is also mostly not that useful.

• _do_authorize is just convoluted and can be cleaned up a lot by

removing all the code moving values around between the dictionaries.
And again, sprinkling assignments out over the method like this makes
for a great treasure hunt, but doesn't help understanding the code too
much. Reassigning params also tripped me up during rewriting.

• _do_token is similarly just too much - perhaps no wonder considering

it looks mostly like copy & paste from the method before. IMO that's
the biggest mistake as it requires a lot of attention when trying to
deduplicate it again. It's better to spend the effort creating a
"DRY" implementation from the start.

• The exceptions are mostly similar and can be moved into methods.

Actually I got confused when trying to clean up _do_authorize and
_do_token, so I'll give up on that for now. I can imagine that other
readers might have a similar problem. I strongly suggest to
aggressively factor out common functions and creating more methods,
helper functions, whatever to help keep the line count and number of
variables to read down.

StackExchange Code Review Q#124966, answer score: 5