Skip to content
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
3ec03d1
start skeleton for auth handler
squirrelo Dec 5, 2015
7547ef8
flake8
squirrelo Dec 5, 2015
1df6292
add table for oauth2 keys
squirrelo Dec 5, 2015
0107b15
add token storage in redis
squirrelo Dec 5, 2015
0f22b12
add tests
squirrelo Dec 5, 2015
2d279e8
add tests for password
squirrelo Dec 5, 2015
599cdb0
add oauth2 bad client tests
squirrelo Dec 5, 2015
ec40863
fix up tests
squirrelo Dec 5, 2015
2357500
add error messages to oauth2 errors
squirrelo Dec 5, 2015
8692357
more tests for oauth access codes
squirrelo Dec 5, 2015
964873f
remove print
squirrelo Dec 5, 2015
f721773
update tests for other handlers
squirrelo Dec 5, 2015
96a5be6
merge upstream/master
squirrelo Dec 5, 2015
476ecab
add missing auth check
squirrelo Dec 7, 2015
13b0896
move test inserts
squirrelo Dec 7, 2015
61df629
merge upstream/artifact
squirrelo Dec 8, 2015
edbbebb
add daily request limiting
squirrelo Dec 8, 2015
7c58a6b
docstrings
squirrelo Dec 8, 2015
19a227a
update error controlled vocab
squirrelo Dec 8, 2015
f49fa50
move authenticator to wrapper
squirrelo Dec 11, 2015
ab3cbed
update other handlers to reflect decorator
squirrelo Dec 11, 2015
8fe3c2c
Merge branch 'artifact' of https://github.com/biocore/qiita into oauth2
squirrelo Dec 11, 2015
11c680a
flake8
squirrelo Dec 11, 2015
dcd78a1
address comments
squirrelo Dec 14, 2015
e3ef9d9
break up tests
squirrelo Dec 14, 2015
4b79d66
address sugestions
squirrelo Dec 21, 2015
4d1d100
add returns
squirrelo Dec 21, 2015
df7a469
add raises
squirrelo Dec 21, 2015
0c7b152
expand docstring
squirrelo Dec 21, 2015
4e2d728
break up test imports
squirrelo Dec 21, 2015
f0c25b3
flake8 and docstrings
squirrelo Dec 21, 2015
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions qiita_db/handlers/artifact.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,8 @@
# The full license is in the file LICENSE, distributed with this software.
# -----------------------------------------------------------------------------

from tornado.web import RequestHandler

import qiita_db as qdb
from .oauth2 import OauthBaseHandler, authenticate_oauth


def _get_artifact(a_id):
Expand Down Expand Up @@ -36,7 +35,8 @@ def _get_artifact(a_id):
return artifact, True, ''


class ArtifactFilepathsHandler(RequestHandler):
class ArtifactFilepathsHandler(OauthBaseHandler):
@authenticate_oauth
def get(self, artifact_id):
"""Retrieves the filepath information of the given artifact

Expand Down Expand Up @@ -70,7 +70,8 @@ def get(self, artifact_id):
self.write(response)


class ArtifactMappingHandler(RequestHandler):
class ArtifactMappingHandler(OauthBaseHandler):
@authenticate_oauth
def get(self, artifact_id):
"""Retrieves the mapping file information of the given artifact

Expand Down Expand Up @@ -113,7 +114,8 @@ def get(self, artifact_id):
self.write(response)


class ArtifactTypeHandler(RequestHandler):
class ArtifactTypeHandler(OauthBaseHandler):
@authenticate_oauth
def get(self, artifact_id):
"""Retrieves the artifact type information of the given artifact

Expand Down
306 changes: 306 additions & 0 deletions qiita_db/handlers/oauth2.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,306 @@
# -----------------------------------------------------------------------------
# Copyright (c) 2014--, The Qiita Development Team.
#
# Distributed under the terms of the BSD 3-clause License.
#
# The full license is in the file LICENSE, distributed with this software.
# -----------------------------------------------------------------------------

from base64 import urlsafe_b64decode
from string import ascii_letters, digits
import datetime
from random import SystemRandom
import functools
from traceback import format_exception

from tornado.web import RequestHandler

from moi import r_client
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Group the import from moi with tornado rather than with qiita (3rd party lib group)

from qiita_core.exceptions import (IncorrectPasswordError, IncorrectEmailError,
UnverifiedEmailError)
import qiita_db as qdb


def login_client(client_id, client_secret=None):
"""Login client_id, optionally with client_secret

Parameters
----------
client_id : str
The client ID to validate
client_secret : str, optional
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is actually correct, neither if we want to support it. According to the RFC6749:

client_secret
         REQUIRED.  The client secret.  The client MAY omit the
         parameter if the client secret is an empty string.

According to this description, the only time that the parameter can be omitted is that if the client_secret of the client is an empty string. Here, it will validate the client correctly as long as the client_id is the same. That means, the only thing that I need to impersonate somebody else is using his client_id, I don't need this client_secret to gain access to the system. All our clients are going to have client_id and client_secret and client_secret will never be an empty string, given the way they're generated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

client_secret as optional is so it can also be used to check client_id for the Resource Owner Password Credentials login type, which only requires client ID and is in the database as a client ID with a NULL client secret.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to read the entire code to understand why this is correct. My take on this is that this is really dangerous and it is really easy to introduce bugs in the code.

Now, from the code structure correctness, this function is used in 2 places, the function validate_resource_owner and the function validate_client. In this function, you have an if statement, which the only thing that is doing is differentiating the 2 cases that is calling this function. This exposes that there is no functionality in common and this function should've not been decomposed in a single function - I'm not even sure that is saving lines of code...

Can you remove this function and just write the call in the given functions?

The client secret code. If not given, will only validate if client_id
exists

Returns
-------
bool
Whether client_id exists and, if client_secret given, the secret
matches the id
"""
with qdb.sql_connection.TRN:
sql = """SELECT EXISTS(
SELECT *
FROM qiita.oauth_identifiers
WHERE client_id = %s {0})"""
sql_info = [client_id]
if client_secret is not None:
sql = sql.format("AND client_secret = %s")
sql_info.append(client_secret)
else:
sql = sql.format("AND client_secret IS NULL")
qdb.sql_connection.TRN.add(sql, sql_info)
return qdb.sql_connection.TRN.execute_fetchlast()


def _oauth_error(self, error_msg, error):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change self here with something different? Like handler. Self doesn't make sense in a function like this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, thinking a bit more about this, shouldn't this just simply raise an HTTPError, so we don't need our own function? Based on the docs it looks like it will do everything that we need.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking a bit more around - if this is authentication, should this behave similarly as the authenticate from tornado, reference code here the error code should be 403 right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is built with ides from the tornado decorator, but pursuant to the Oauth2 framework linked:

 The authorization server responds with an HTTP 400 (Bad Request)
   status code (unless specified otherwise) and includes the following
   parameters with the response:

So this needs to return 400 error code and the controlled vocab error json in the body.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@squirrelo ping about the first comment in here - change self to handler. There is no self here.

self.set_status(400)
self.write({'error': error,
'error_description': error_msg})
self.finish()


def authenticate_oauth(f):
"""Decorate methods to require valid Oauth2 Authorization header[1]

If a valid header is given, the handoff is done and the page is rendered.
If an invalid header is given, a 400 error code is returned and the json
error message is automatically sent.

References
---------
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing 1 -.

[1] The OAuth 2.0 Authorization Framework.
http://tools.ietf.org/html/rfc6749
"""
@functools.wraps(f)
def wrapper(self, *args, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this have a doc string?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the inner wrapper of the decorator, so no. The docstring of the containingauthenticate_oauth function is what's going to be seen.

header = self.request.headers.get('Authorization', None)
if header is None:
_oauth_error(self, 'Oauth2 error: invalid access token',
'invalid_request')
return
token_info = header.split()
if len(token_info) != 2 or token_info[0] != 'Bearer':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you looking for 'Bearer', could you clarify via documentation? Perhaps this is something obvious but someone without previous experience, me, it's not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Required by the OAuth2 authorization framework, which is explicitly linked in the docstring.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess these 2 comments show how is not fully clear how this works and expecting that everybody reads a full RFC seems unrealistic, what about adding the main assumptions/cases you are using here so is easier to fix/improve in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd argue you shouldn't be modifying code that must conform to a very specific set of rules laid out in the RFC unless you have read and understand the RFC. This is a web-wide standard we did not write, and it is very explicit on what it requires, which allows for things like the libraries used to do client verification in #1584.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like having the code well documented doesn't hurt any one ... do you feel otherwise?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good documentation is one thing. Overly documenting is another, and frowned upon by pep8. What do you gain from having the documentation lines like Make sure Auth header passed conforms to what is expected by Oauth2 standard?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree that your example doesn't help - not even sure why will you use it - but I was expecting something more like:
Based on RFC6750 if reply is not 2 elements in the format of: ['Bearer', token] we assume a wrong reply.
or something like this. Simple, easy, and no need to read lots of pages.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @antgonza on adding a bit more points of documentation. @squirrelo we are not asking to duplicate the entire RFC, but you can't expect that every single developer will read the RFC in order to understand the code that is written here.

As an example of documentation that I would expect having here can be seen here: https://github.com/idan/oauthlib/blob/master/oauthlib/oauth2/rfc6749/request_validator.py

Also, looking at that - the question that raises is why did you re-implement the RFC if the oauthlib already includes an implementation-independent framework for OAuth2? We shouldn't be reinventing the wheel, specially on topics that we are not experts.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @antgonza and @josenavas. Documentation is essential and it doesn't make sense to require every reader of the code to be an expert on topics they use infrequently. @squirrelo, please work with the rest of the group on this productively in this and similar instances -- part of the reason why we're so productive is precisely that the good documentation means people don't have to waste a lot of additional time looking up arcane topics.

_oauth_error(self, 'Oauth2 error: invalid access token',
'invalid_grant')
return

token = token_info[1]
db_token = r_client.hgetall(token)
if not db_token:
# token has timed out or never existed
_oauth_error(self, 'Oauth2 error: token has timed out',
'invalid_grant')
return
# Check daily rate limit for key if password style key
if db_token['grant_type'] == 'password':
limit_key = '%s_%s_daily_limit' % (db_token['client_id'],
db_token['user'])
limiter = r_client.get(limit_key)
if limiter is None:
# Set limit to 5,000 requests per day
r_client.setex(limit_key, 5000, 86400)
else:
r_client.decr(limit_key)
if int(r_client.get(limit_key)) <= 0:
_oauth_error(
self, 'Oauth2 error: daily request limit reached',
'invalid_grant')
return

return f(self, *args, **kwargs)
return wrapper


class OauthBaseHandler(RequestHandler):
def write_error(self, status_code, **kwargs):
"""Overriding the default write error in tornado RequestHandler

Instead of writing all errors to stderr, this writes them to the logger
tables.

Parameters
----------
status_code : int
HTML status code of the error
**kwargs : dict
Other parameters describing the error

Notes
-----
This function is automatically called by the tornado package on errors,
and should never be called directly.
"""
if status_code in {403, 404, 405}:
# We don't need to log these failues in the logging table
return
# log the error
exc_info = kwargs['exc_info']
trace_info = ''.join(['%s\n' % line for line in
format_exception(*exc_info)])
req_dict = self.request.__dict__
# must trim body to 1024 chars to prevent huge error messages
req_dict['body'] = req_dict.get('body', '')[:1024]
request_info = ''.join(['<strong>%s</strong>: %s\n' %
(k, req_dict[k]) for k in
req_dict.keys() if k != 'files'])
error = exc_info[1]
qdb.logger.LogEntry.create(
'Runtime',
'ERROR:\n%s\nTRACE:\n%s\nHTTP INFO:\n%s\n' %
(error, trace_info, request_info))

# Allow call to oauth_error function as part of the class
oauth_error = _oauth_error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?! Define the function here - you can then call the function in the decorator, as you are calling other methods/attributes of the class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As stated previously, the decorator function is for anything extending tornado's RequestHandler, not specifically the oauth handler, and the changes just pushed reflect that. Calling the function in the decorator breaks this functionality.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting really confused here. As you stated here, the authenticate_oauth is written so it works with any RequestHandler. On the other hand, you created the base class OauthBaseHandler that all the handlers that require oauth2 need to subclass. The question then is, is the OauthBaseHandler class needed or the authenticate_oauth really need to be as general for any of the Handlers type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The OauthBaseHandler is for anything that needs to actually set and manipulate the authorization keys in the system, or authenticate user/client information, so not every oauth2 using page needs to subclass it. I did it for convenience, as there are things like 404 and error handling that are better used in base classes, but it's not strictly needed. Leaving the decorator open like this allows extensibility in the future with no code change, as all you need to do is import the authenticate_oauth decorator and extend RequestHandler again, exactly the same as the tornado library already works.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any specific necessity of calling oauth_error as self.oauth_error? If not, remove this line and and just call the function _oauth_error as _oauth_error(self, params...). This is just making maintaining the code harder.


def head(self):
"""Adds proper response for head requests"""
self.finish()


class TokenAuthHandler(OauthBaseHandler):
def generate_access_token(self):
"""Creates the random alphanumeric token

Returns
-------
str
55 character random alphanumeric string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does the 55 come from? It is required by oauth2, please link the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, a cryptographically secure limit that needs to be hard coded so we can set the same varchar length for the postgres column storing the key.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, magic numbers are not useful w/o some context for other devs. You may know what they're for but not other devs, which makes this code hard to maintain. If that is something that you decided, at least add a comment, specially because 55 looks random...


Notes
-----
55 was chosen as a cryptographically secure limit that needs to be
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of understand this explanation. However I still don't understand why 55. Why we don't use 100? wouldn't it be more secure?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

62^55 = 3.815425e+98 which is probably secure enough, given that we also have request and rate limits.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you did not understand the point that I was trying to make. It looks like 55 is secure enough but 54 isn't? That is the point that I want to make, if you decided 55 based on some documentation that you read online, yo should link it, if it is something that you decided, you should have some other reason that 'looks like secure enough'. 55 is not a multiple of 2 and looks like strange number - this is too arbitrary and the same questions that I have are the same questions that another dev in 2 years can have, and we may or may not be around to answer those questions. So please, include the needed documentation so a dev that looks at the system for the first time understands what is going on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can update the database column to store 255 characters (max you can safely put in an HTTP variable), but as stated there is no documentation to cite for length. The number is not specified as part of the Oauth2 spec, so it's up to the implementer to decide.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't understand the decision of the 55 chars. The only answer that I received here is "I have the feeling that is secure enough". What are other systems using?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I just noticed that the description here is totally incorrect. As far as I know, the access_token is never stored in the postgres DB, cause you are storing it in redis. Please, update the documentation accordingly and do some research on what other systems are using, and link it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The explanation is still there is no set size for the key, and it must arbitrarily be decided by the implementer because there is no standard. Twitter seems to be using 21 characters although this is an example so it may be a different length, oauthlib seems to be using 30 characters by default, sometimes more, etc. It is completely arbitrary, and most systems don't even say how long the code is because it may change length at any time. The only fixture is that it can not be more than 255 characters for HTTP transfer reasons.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, can you actually add the comment that the exact length of the token can change, but it should never be longer than 255? That is a technology limit and it will be good to have it exposed here in the documentation.

I don't see any check in here but, what happens if the token that is generated already exists in the system? The RFC does not say anything specific, but I think we should double check that each new access_token is unique, or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

hard coded so we can set the same varchar length for the postgres
column storing the key.
"""
pool = ascii_letters + digits
return ''.join((SystemRandom().choice(pool) for _ in range(55)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why SystemRandom() instead of simply choice?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SystemRandom is cryptographically secure as it changes per system, choice is not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, thanks!


def set_token(self, client_id, grant_type, user=None, timeout=3600):
"""Create access token for the client on redis and send json response

Parameters
----------
client_id : str
Client that requested the token
grant_type : str
Type of key being requested
user : str, optional
If password grant type requested, the user requesting the key.
timeout : int, optional
The timeout, in seconds, for the token. Default 3600
"""
token = self.generate_access_token()

r_client.hset(token, 'timestamp', datetime.datetime.now())
r_client.hset(token, 'client_id', client_id)
r_client.hset(token, 'grant_type', grant_type)
r_client.expire(token, timeout)
if user:
r_client.hset(token, 'user', user)
if grant_type == 'password':
# Check if client has access limit key, and if not, create it
limit_key = '%s_%s_daily_limit' % (client_id, user)
limiter = r_client.get(limit_key)
if limiter is None:
# Set limit to 5,000 requests per day
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems redundant? Isn't it already set up above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically this is the first set up here. The one above is in case the access token's validity spans when the rate limit expires, and we need to reset the rate limit key during the lookup of token validity instead of the initial creation of the token.

r_client.setex(limit_key, 5000, 86400)

self.write({'access_token': token,
'token_type': 'Bearer',
'expires_in': '3600'})
self.finish()

def validate_client(self, client_id, client_secret):
"""Make sure client exists, then set the token and send it

Parameters
----------
client_id : str
The client making the request
client_secret : str
The secret key for the client
"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this doesn't have a return, we have been documenting the API returns in the returns sections of the numpy documentation so we know how the API looks like.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure what this means. Could you elaborate?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check the documentation of the other handlers. In the returns section we are including the explanation of the JSON that you're returning.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That does not fit with the standard numpy documentation methods. If a change like this happened, it should have been discussed at the meetings.

if login_client(client_id, client_secret):
self.set_token(client_id, 'client')
else:
self.oauth_error('Oauth2 error: invalid client information',
'invalid_client')

def validate_resource_owner(self, username, password, client_id):
"""Make sure user and client exist, then set the token and send it

Parameters
----------
username : str
The username to validate
password : str
The password for the username
client_id : str
The client making the request
"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here re documenting returns

try:
qdb.user.User.login(username, password)
except (IncorrectEmailError, IncorrectPasswordError,
UnverifiedEmailError):
self.oauth_error('Oauth2 error: invalid user information',
'invalid_client')
return

if login_client(client_id):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code and the one in validate_client are the same - can you decompose in another function so we avoid code duplication?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

self.set_token(client_id, 'password', user=username)
else:
self.oauth_error('Oauth2 error: invalid client information',
'invalid_client')

def post(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to double check on how oauth2 works. You get a new token by posting? It looks counter-intuitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is how it works.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add numpy documentation in the post. Also, as it was agreed over email on documenting the RESTapi handlers (@ElDeveloper, @antgonza and I agreed, and no body complain), add the Returns section including the json response of the query. If you don't agree with this solution, please propose an alternative one, but we are not going to merge undocumented code.

# first check for header version of sending auth, meaning client ID
header = self.request.headers.get('Authorization', None)
if header is not None:
header_info = header.split()
if header_info[0] != 'Basic':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's Basic and why is invalid ?

# Invalid Authorization header type for this page
self.oauth_error('Oauth2 error: invalid token type',
'invalid_request')
return

# Get client information from the header and validate it
grant_type = self.get_argument('grant_type', None)
if grant_type != 'client':
self.oauth_error('Oauth2 error: invalid grant_type',
'invalid_request')
return
try:
client_id, client_secret = urlsafe_b64decode(
header_info[1]).split(':')
except ValueError:
# Split didn't work, so invalid information sent
self.oauth_error('Oauth2 error: invalid base64 encoded info',
'invalid_request')
return
self.validate_client(client_id, client_secret)
return

# Otherwise, do either password or client based authentication
client_id = self.get_argument('client_id', None)
grant_type = self.get_argument('grant_type', None)
if grant_type == 'password':
username = self.get_argument('username', None)
password = self.get_argument('password', None)
if not all([username, password, client_id]):
self.oauth_error('Oauth2 error: missing user information',
'invalid_request')
else:
self.validate_resource_owner(username, password, client_id)

elif grant_type == 'client':
client_secret = self.get_argument('client_secret', None)
if not all([client_id, client_secret]):
self.oauth_error('Oauth2 error: missing client information',
'invalid_request')
return
self.validate_client(client_id, client_secret)
else:
self.oauth_error('Oauth2 error: invalid grant_type',
'unsupported_grant_type')
return
15 changes: 9 additions & 6 deletions qiita_db/handlers/processing_job.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,8 @@
from datetime import datetime
from json import loads

from tornado.web import RequestHandler

import qiita_db as qdb
from .oauth2 import OauthBaseHandler, authenticate_oauth


def _get_job(job_id):
Expand Down Expand Up @@ -40,7 +39,8 @@ def _get_job(job_id):
return job, True, ''


class JobHandler(RequestHandler):
class JobHandler(OauthBaseHandler):
@authenticate_oauth
def get(self, job_id):
"""Get the job information

Expand Down Expand Up @@ -82,7 +82,8 @@ def get(self, job_id):
self.write(response)


class HeartbeatHandler(RequestHandler):
class HeartbeatHandler(OauthBaseHandler):
@authenticate_oauth
def post(self, job_id):
"""Update the heartbeat timestamp of the job

Expand Down Expand Up @@ -117,7 +118,8 @@ def post(self, job_id):
self.write(response)


class ActiveStepHandler(RequestHandler):
class ActiveStepHandler(OauthBaseHandler):
@authenticate_oauth
def post(self, job_id):
"""Changes the current exectuion step of the given job

Expand Down Expand Up @@ -151,7 +153,8 @@ def post(self, job_id):
self.write(response)


class CompleteHandler(RequestHandler):
class CompleteHandler(OauthBaseHandler):
@authenticate_oauth
def post(self, job_id):
"""Updates the job to one of the completed statuses: 'success', 'error'

Expand Down
Loading