jhildreth / falcon-jwt-checker

MIT License
12 stars 5 forks source link

Mandatory claims #3

Closed jdno closed 7 years ago

jdno commented 7 years ago

Hey,

nice project. Exactly what I was looking for. 👍 One question, though:

My application requires that the JWT contains the sub claim. Right now, I manually check in every endpoint if the claim is set. It would be awesome if that could be done in one place, e.g. by specifying mandatory claims when initializing the JwtChecker. What do you think about that?

jhildreth commented 7 years ago

Hi! Thanks a lot, glad it's useful to you! Thanks for the suggestions as well. I'll have to give this a little more thought - I'm not quite understanding the use case. Any case I can think of at the moment for a route that cares about the sub claim would care about the actual value in the claim as well, not just that it was set. Would you be able to provide a little more detail about what you're trying to do? Thanks!

jdno commented 7 years ago

I use JWT to authenticate users in my API and to populate a current_user object with the user's id. The id is taken from the token's sub claim. Right now, I check manually in every endpoint if sub exists:

if not isinstance(jwt_claims, dict) or 'sub' not in jwt_claims:
    raise HTTPUnauthorized()

I was wondering if it makes sense to make this check part of the library. In my endpoint, I would then only need to validate the content of sub but not its existence.

jhildreth commented 7 years ago

Thanks for clarifying! Sure, I do something similar with the user's id. I'm still not seeing a need to have the JwtChecker be checking for the sub claim - in my mind that is rolled up in checking that the value of the sub claim is authorized in my application. Something along the lines of:

def on_post(self, req, resp, jwt_claims):
    current_user_id = jwt_claims.get('sub', '')  # I would inline this if I wasn't using the value beyond the authorization check
    if not is_authorized(current_user_id):  # is_authorized() would return False for an empty string
        raise HTTPUnauthorized()
    # Do authorized stuff here

In the way I'm thinking about this, to perform the actual authorization I need to examine the content of the sub claim. As I do so, I am in effect checking that it exists on the jwt as well.

So my initial feeling is that I'd rather not add this, just to keep things as simple as possible. Right now I'm relying on PyJWT to do all the actual validation, and I'm not sure I want to add logic around that process if I can help it. (As a side note, PyJWT does provide the option to specify the values you expect in some of the other claims such as aud. This does make those claims mandatory in effect. I have aud integrated, and have an issue to utilize iss as well.)

I apologize if I'm not understanding clearly, I'm open to continuing the discussion. I'll keep mulling it over and leave the issue open for now - I'll have a bit more time to look at it again after the new year.

jhildreth commented 7 years ago

I'm going to go ahead and close this issue. I appreciate the suggestion, but I don't have plans to implement this feature at this time.