Added JWT authentication. #537

dsmurrell · 2017-07-28T20:16:36Z

I've included examples of use in the examples folder.

A possible enhancement is that the jwt token is considered for renewal while it is being verified.

Remove trailing white space that linter is complaining about

coveralls · 2017-08-24T17:15:41Z

Coverage decreased (-1.7%) to 98.299% when pulling 5d16cfb on dsmurrell:develop into 78a6be2 on timothycrosley:develop.

OGKevin

A few comments and question👀

Sentences are not beginning with a Capital letter and don't end with a ., some print statements and redundant else's

OGKevin · 2017-08-24T18:36:21Z

hug/authentication.py

+            return jwt.encode({'user_id': decoding['user_id'],
+                               'exp': datetime.utcnow() + timedelta(seconds=token_expiration_seconds)},
+                               jwt_secret, algorithm='HS256').decode("utf-8")
+        else:


Isn't this else statement redundant ? 🤔

Does a Python function return None if it reaches its end? Even so, isn't it better to be explicit about where you return None and what it means?

Wel, yes python automatically returns None but there is no harm in explicitly returning None yourself. My point here was the following:

if x: return x else: return y

Can be rewritten to:

if x: return x return y

As of there is no need to say else because the if statement exists the function if its true.

Yes, I guess it is strictly redundant. It also takes the same space. I prefer the readability of the former though as it links the code in the else to if statement. Is there a general standard on the best way in Python?

Well it is opinion based really as you can see here.

So i guess we should see what @timothycrosley thinks about this one. Or we can search in the project's code for an if else return block code and use that as reference to be consistent with the already written code.

Sounds good.

Thanks for the feedback btw. I'm new to this open source collaboration stuff.

@dsmurrell, I want to stress that for being new to open source collaboration, this is a very good high quality first pull request, great work!

I have a slight preference for removing the return None in both places, and then remove the else and let it fall through

OGKevin · 2017-08-24T18:36:25Z

hug/authentication.py

+    try:
+        token = authorization.split(' ')[1]
+        decoding = jwt.decode(token, jwt_secret, algorithm='HS256')
+        print(decoding)


Is this print statement suppose to be there ?

You are right. It's not.

OGKevin · 2017-08-24T18:36:53Z

hug/authentication.py

+        return decoding['user_id']
+    except Exception as ex:
+        template = "An exception of type {0} occurred. Arguments:\n{1!r}"
+        print(template.format(type(ex).__name__, ex.args))


Same as above ☝️

What is the standard way to report on something unexpected? I would normally use a logger, but it's not in this scope. @timothycrosley how do you normally handle reporting unexpected exceptions in hug?

OGKevin · 2017-08-24T18:37:50Z

hug/authentication.py

+            return None
+    except Exception as ex:
+        template = "An exception of type {0} occurred. Arguments:\n{1!r}"
+        print(template.format(type(ex).__name__, ex.args))


Suppose to print ?

Same as above. Looking for the way hug reports or outputs unexpected exceptions.

Hi @dsmurrell,

The answer is that you should catch any errors you expect and want to use to change output, and let any errors you don't expect bubble up and be handled by code higher up.

Thanks!

~Timothy

OGKevin · 2017-08-24T18:40:05Z

examples/authentication.py

+replace_this = False
+config = {
+    'jwt_secret': 'super-secret-key-please-change',
+    # token will expire in 3600 seconds if it is not refreshed and the user will be required to log in again


Your sentences do not begin with a Capital letter and don't end with a dot(.), is this intended ?

I'm pretty new to collaboration. Is that the standard for comments? Begin with a capital letter and end with a dot?

Well, I mean normally sentences have a Capital letter at the start and ends with a dot 😝. But I can see that this projects sentences don't end with a dot xD

Ex: https://github.com/timothycrosley/hug/blob/a2dfdd71dc5cb809107ca710fd6c0629c6802cfc/hug/api.py#L77

So lets stick with the project's standard and use a Capital letter for the begging of the sentence. 😉

PEP seems to indicate that "If a comment is short, the period at the end can be omitted."

I usually start comments with lower case, but yeah, that doesn't seem compliant. I'll change this then.

Ah yea that is true, guess we have different standards for short comments 😁 .

timothycrosley

With the few small changes here that you and @OGKevin
had talked about implemented, I would be happy to pull this in, thanks for the great work!

~Timothy

OGKevin

Exception questions 🤔

OGKevin · 2017-09-01T11:51:20Z

hug/authentication.py

-    except Exception as ex:
-        template = "An exception of type {0} occurred. Arguments:\n{1!r}"
-        print(template.format(type(ex).__name__, ex.args))
+    except:


Are you catching all exceptions ? 😮 Wouldn't this make things hard to debug ?

Isn't there a specific exception that can be caught ?

Yes it would, I'll go figure out which one/s I'm expecting to catch when the token is no longer valid. Thanks.

@OGKevin If an exception is thrown that isn't caught here what happens to the server? Does it continue running and print and error message, or does it break? Where is the code that handles the exceptions that get bubbled up?

@dsmurrell The individual request will die, but the server itself will keep running, if you have an @hug.exception handler you can manage how that exception is handled

OGKevin · 2017-09-01T11:52:27Z

hug/authentication.py

-    except Exception as ex:
-        template = "An exception of type {0} occurred. Arguments:\n{1!r}"
-        print(template.format(type(ex).__name__, ex.args))
+    except:


same as above ☝️

OGKevin

2 new lines between functions that are not nested.

OGKevin · 2017-09-01T11:55:32Z

examples/authentication.py

+# Enable authenticated endpoints, example @authenticated.get('/users/me').
+authenticated = hug.http(requires=hug.authentication.json_web_token(hug.authentication.verify_jwt, config['jwt_secret']))
+
+# Check the token and issue a new one if it is about to expire (within token_refresh_seconds from expiry).


Normally there will be 2 new lines between function and classes and one new line between class methods. Please add a new line.

OGKevin · 2017-09-01T11:55:40Z

examples/authentication.py

+        if token:
+            response.set_header('token', token)
+
+@hug.post('/login')


OGKevin · 2017-09-01T11:56:39Z

hug/authentication.py

+
+    return wrapper
+
+@jwt_authenticator


☝️ 2 new lines between functions

OGKevin · 2017-09-01T11:56:46Z

hug/authentication.py

+        else:
+            return False
+    return None
+


OGKevin · 2017-09-01T11:56:56Z

hug/authentication.py

+                       'exp': datetime.utcnow() + timedelta(seconds=token_expiration_seconds)},
+                       jwt_secret, algorithm='HS256').decode("utf-8")
+
+def refresh_jwt(authorization, token_refresh_seconds, token_expiration_seconds, jwt_secret):


coveralls · 2017-09-01T12:38:38Z

Coverage decreased (-1.5%) to 98.48% when pulling a5cd7f5 on dsmurrell:develop into 78a6be2 on timothycrosley:develop.

dsmurrell · 2017-09-03T15:20:49Z

I've modified the code to catch jwt.InvalidTokenError exceptions.

This is the base class which includes exceptions such as jwt.ExpiredSignatureError and jwt.DecodeError. jwt.ExpiredSignatureError will be far more common so it should definitely catch these silently. I'm less convinced about catching jwt.DecodeErrors silently as this is relatively unexpected.

It's possible I should only be catching jwt.ExpiredSignatureError exceptions?

For someone trying to get this working from the client side, it would be quite useful to know which exception is being caught. Another question is, how would someone who is trying to get this working for the first time get feedback about what exception is being thrown?

timothycrosley · 2017-09-06T22:59:48Z

Hi @dsmurrell,

In general, the design currently is such that if there is an error you want to customize output for the correct way to do that is simply to allow the exception to bubble up, so incorrect credentials should be handled as False, and for others simply raise. In the case that you want to provide additional information beyond this for handling the exception, you could add contextual information to the request.context or in the exception that you raise for things higher up to introspect.

Thanks!

~Timothy

danigosa · 2017-11-05T21:01:57Z

Hey guys, how is this going? JWT is a must-have auth backend for every serious backend microservice so what's left? can we help?

Cheers,
Dgz

danigosa · 2017-12-13T02:24:32Z

We have already implemented the JWT authentication using the provided hug.hug.authentication.token that supports treating Authorization header in a raw way, I can't see the point to implement full fledge JWT authenticator, microframeworks are meant to not add unnecessary stuff, if you @timothycrosley wants us to do a PR with a fully functional example in the examples folder we can do it, I paste here a working summary of it, it raises a falcon.HTTPUnauthorized producing a proper 401 response with the corresponding error as message:

import hug
import jwt  # pip install pyjwt
from datetime import datetime, timedelta
from falcon import HTTPUnauthorized

# These can be in a constants/settings file
JWT_OPTIONS = {
        'verify_signature': True,
        'verify_exp': True,
        'verify_nbf': False,
        'verify_iat': False,
        'verify_aud': True,
        'verify_iss': True,
        'require_exp': True,
        'require_iat': False,
        'require_nbf': False
    }
SECRET_KEY = 'some-super-secret'
JWT_ISSUER = 'some-issuer'
JWT_AUDIENCE = 'some-audience'
JWT_OPTIONS_ALGORITHM = 'HS256'

def decode_token(token, options=JWT_OPTIONS):
    """
    Decodes a JWT token and returns payload info
    :return:
    """
    return jwt.decode(
        token,
        SECRET_KEY,
        issuer=JWT_ISSUER,
        audience=JWT_AUDIENCE,
        options=options,
        algorithms=(JWT_OPTIONS_ALGORITHM,)
    )

def create_token(payload, exp=None) -> object:
    """
    Creates JWT Token
    :return:
    """
    if exp is None:
        exp = datetime.utcnow() + timedelta(seconds=3600)
    _token = {
        'aud': JWT_AUDIENCE,
        'exp': exp,
        'iss': JWT_ISSUER,
    }
    _token.update(payload)
    return jwt.encode(
        _token,
        SECRET_KEY,
        algorithm=JWT_OPTIONS_ALGORITHM
    ).decode('utf-8')

def parse_header(authorization_header, exception_class=HTTPUnauthorized):
    """
    Parses an authorization header. Accepts a custom Exception if any failure
    :param authorization_header:
    :param exception_class:
    :return:
    """
    if authorization_header:
        parts = authorization_header.split()

        if parts[0].lower() != 'bearer' or len(parts) == 1 or len(
                parts) > 2:
            raise exception_class("invalid_header")

        jwt_token = parts[1]
        try:
            # Decode token
            payload = decode_token(jwt_token)
            return jwt_token, payload
        except jwt.ExpiredSignature:  # pragma: no cover
            raise exception_class("token is expired")

        except jwt.InvalidAudienceError:  # pragma: no cover
            raise exception_class("incorrect audience")

        except jwt.DecodeError:  # pragma: no cover
            raise exception_class("token signature is invalid")

        except jwt.InvalidIssuerError:  # pragma: no cover
            raise exception_class("token issuer is invalid")

        except Exception as exc:  # pragma: no cover
            raise exception_class(exc)
    else:  # pragma: no cover
        raise exception_class("Authorization header not present")

def jwt_token_verify(auth_header):
    """
    Verifier function for hug token authenticator
    :param auth_header:
    :return:
    """
    # Hug do not extract Bearer prefix
    auth_token, payload = parse_header(auth_header)
    return payload

token_key_authentication = hug.authentication.token(jwt_token_verify)

# Then the protected API
@hug.get('/jwt', requires=token_key_authentication)
def some_jwt_endpoint():
      return "I'm JWT!"

# Then the JWT Token Generator
@hug.get('/token')
def some_jwt_endpoint():
      return create_token({'some': 'payload'})

briceparent · 2018-04-10T08:06:51Z

Hello @timothycrosley, @dsmurrell and @OGKevin.
What's the status of this PR? It seemed to be almost mergeable some months ago, but nothing new since. Is there something we can do to help?
I've seen with interest @danigosa 's solution, but I'm not sure whether it's unit-tested somewhere, respecting the standard, and it misses the token refreshing capabilities that I rely on.

OGKevin · 2018-04-10T08:09:52Z

@briceparent im not an official maintainer. It is up to @timothycrosley to merge.

briceparent · 2018-04-10T08:20:53Z

I included everyone that seemed to be involved in this thread, as you all might have direct or off-the-record information about that too. But thanks for your very quick answer!

tadeoiiit · 2019-02-22T12:55:05Z

@dsmurrell are you planning on finishing this?

dsmurrell · 2019-02-22T15:36:06Z

@tadeoiiit, I did want to finish this at some point. I ended up moving to work on another project that had flask serving its backend so I never got around to finishing this.

@timothycrosley, has much changed on the authentication side? Is this PR still something that could benefit people if completed? Is so, I can put in some work this weekend to finish it.

timothycrosley · 2019-02-23T20:13:11Z

@dsmurrell authentication is mostly the same, I think finishing it would be useful! We may end up moving it into a plugin - but either way it's important to support :) Thank you for your work on it!

dsmurrell and others added 2 commits July 28, 2017 21:08

Added JWT authentication.

7f199a2

Update authentication.py

5d16cfb

Remove trailing white space that linter is complaining about

OGKevin reviewed Aug 24, 2017

View reviewed changes

timothycrosley requested changes Aug 26, 2017

View reviewed changes

Updates based on comments from @OGKevin and @timothycrosley.

a5cd7f5

OGKevin reviewed Sep 1, 2017

View reviewed changes

OGKevin suggested changes Sep 1, 2017

View reviewed changes

Added spaces and caught only the exceptions I expect.

96f61cc

Added JWT authentication. #537

Are you sure you want to change the base?

Added JWT authentication. #537

Conversation

dsmurrell commented Jul 28, 2017

coveralls commented Aug 24, 2017 • edited

OGKevin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsmurrell Aug 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsmurrell Aug 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsmurrell Aug 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothycrosley left a comment

Choose a reason for hiding this comment

OGKevin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OGKevin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 1, 2017 • edited

dsmurrell commented Sep 3, 2017

timothycrosley commented Sep 6, 2017

danigosa commented Nov 5, 2017

danigosa commented Dec 13, 2017 • edited

briceparent commented Apr 10, 2018

OGKevin commented Apr 10, 2018

briceparent commented Apr 10, 2018

tadeoiiit commented Feb 22, 2019

dsmurrell commented Feb 22, 2019 • edited

timothycrosley commented Feb 23, 2019

coveralls commented Aug 24, 2017 •

edited

dsmurrell Aug 24, 2017 •

edited

dsmurrell Aug 24, 2017 •

edited

dsmurrell Aug 24, 2017 •

edited

coveralls commented Sep 1, 2017 •

edited

danigosa commented Dec 13, 2017 •

edited

dsmurrell commented Feb 22, 2019 •

edited