Resolve #453, better options for revoking both refresh and access

[tgross35]

There are a handful of changes in this PR:

• Add verify_type argument to jwt methods to allow accepting both refresh & access tokens
• Added relevant tests
• Documentation updates to help explain refresh token revoking necessity and options
• Added type hints to everything possible for better code autocomplete, documentation, etc.
• Fixed insure/ensure mistake

Let me know if anything else is needed to clean up. Verified tox passes except for pypy3 (didn't have it available to test)

[vimalloc]

Thanks for putting this together!! I'm super busy this week, I'll try to find the time to properly look over it as soon as I can! 🥳 🎉

[tgross35]

Hey @vimalloc, not to press but have you had a chance to take a look at this?

I think I will at some point try to roll #441 into the changes, but would like some direction. My current thought is to allow verify_type to accept a bool, a string, or a Iterable[str]/list[str], with the following logic:

• If verify_type=False, don't check the type at all
• If verify_type=True, follow the default conventions to check 'access' or 'refresh' if refresh=true. This is the default
• If verify_type=string, verify that the type = the string. Can be 'access', 'refresh', or a custom type implemented via the logic in Added support for custom JWT types #441

In #441 (comment) you mentioned being unsure about having both refresh and token_type as kwargs - with my implementation, the problem unfortunately doesn't go away. token_type and verify_type are essentially equivalent, I kind of vouch for verify_type because it seems to work better with allowing true/false.

Anyway, I don't think there's a good way to get out of that part without removing refresh= which is majorly breaking. That being said, I think it's OK to just make sure the documentation states that refresh will be ignored if verify_type is provided, and that should be pretty confusion-free. Could also push verify_type and state that refresh is deprecated.

Also think it would be a good idea to add flask_jwt_extended.TokenTypes as an enum with TokenTypes.ACCESS and TokenTypes.REFRESH, just to avoid magic strings like verify_type=("access", "refresh", "custom") (of course wouldn't help custom but would remove any possible typos when using builtins)

[vimalloc]

Looks great, thanks for all the work you put into this!

Left one super small comment that I wanted to check, but it's tiny.

One question, do you think it would be worth adding a mypy check for CI to verify the types that we defined match up with how we are using them? If so, I don't think it would need to happen as part of this PR

Cheers!

[vimalloc]

Probably don't need this, I'm guessing it's a dependency of one of the other packages we explicitly need here right?

[vimalloc]

I only ask because I ran into situations where conflicts came about before when doing a pip freeze on all of the packages used here, instead of defining only the ones that are explicitly needed at letting pip handle their dependencies.

[tgross35]

Whoops you're right, that was added by accident! However, this requirements.txt is sort of meant for dev right? (since setup.py defines actual production requirements). If so, probably doesn't hurt to leave it on there, since black is already on the list (and both are required for precommit). I sort of started migrating my personal stuff from requirements.txt -> Pipfile to avoid these confusions, but that's an annoying process on the CI side.

[tgross35]

Your call for keep or remove, but I'd say do the same for black & flake8

[vimalloc]

I vote remove. Keeping all the ~60 dependencies that are brought in from this file has given me various issues in the past that I would like to avoid 👍

[tgross35]

Flake8 is gone, wasn't sure if you meant black too (or sphinx too? Kind of fall in the same boat of not always necessary dev tools) so just let me know!

[vimalloc]

Hey @vimalloc, not to press but have you had a chance to take a look at this?

I think I will at some point try to roll #441 into the changes, but would like some direction. My current thought is to allow verify_type to accept a bool, a string, or a Iterable[str]/list[str], with the following logic:

* If verify_type=False, don't check the type at all

* If verify_type=True, follow the default conventions to check 'access' or 'refresh' if refresh=true. This is the default

* If verify_type=string, verify that the type = the string. Can be 'access', 'refresh', or a custom type implemented via the logic in [Added support for custom JWT types #441](https://github.com/vimalloc/flask-jwt-extended/pull/441)

In #441 (comment) you mentioned being unsure about having both refresh and token_type as kwargs - with my implementation, the problem unfortunately doesn't go away. token_type and verify_type are essentially equivalent, I kind of vouch for verify_type because it seems to work better with allowing true/false.

Anyway, I don't think there's a good way to get out of that part without removing refresh= which is majorly breaking. That being said, I think it's OK to just make sure the documentation states that refresh will be ignored if verify_type is provided, and that should be pretty confusion-free. Could also push verify_type and state that refresh is deprecated.

Yeah, I can see the arguments for all of these. on the flip side, with the work you've done it would be possible for someone to use a custom decorator with verify_type=False to not have this extension be in charge of types, and letting users customize it to their specific applications needs. On one hand I like that idea because it keeps the core API simpler and I imagine most use cases aren't going to need to verify a custom type, but on the other it might be annoying to have to manage custom decorators if this is something that is widely used. What are your thoughts about those two approaches?

Also think it would be a good idea to add flask_jwt_extended.TokenTypes as an enum with TokenTypes.ACCESS and TokenTypes.REFRESH, just to avoid magic strings like verify_type=("access", "refresh", "custom") (of course wouldn't help custom but would remove any possible typos when using builtins)

Yeah, I think that would be a great change 👍

[tgross35]

Yeah, I can see the arguments for all of these. on the flip side, with the work you've done it would be possible for someone to use a custom decorator with verify_type=False to not have this extension be in charge of types, and letting users customize it to their specific applications needs. On one hand I like that idea because it keeps the core API simpler and I imagine most use cases aren't going to need to verify a custom type, but on the other it might be annoying to have to manage custom decorators if this is something that is widely used. What are your thoughts about those two approaches?

I don't think types outside of access/refresh will likely be all that widely used so making it slightly less accessible likely isn't an issue - there wasn't a ton of buzz around #441. That being said, verify_token_type is the function that would need to be adapted in any case, which is called from _decode_jwt_from_request which already gets verify_type from the decorator, so implementation in the decorator wouldn't really require any extra work.

Quick aside - I'd vote to simplify name create_custom_token(token_type) to create_token(type=TokenType.ACCESS), could see this being an alternative to the create_access_token/create_refresh_token when type needs to be specified on the fly. Or, long-term plan could be to unify the currently split functions.

Also think it would be a good idea to add flask_jwt_extended.TokenTypes as an enum with TokenTypes.ACCESS and TokenTypes.REFRESH, just to avoid magic strings like verify_type=("access", "refresh", "custom") (of course wouldn't help custom but would remove any possible typos when using builtins)

Yeah, I think that would be a great change 👍

Easy enough 😄

Is just fixing requirements.txt all you want to see in this merge? I think it makes sense to keep custom types as a separate pull, since there's a tiny bit more discussion to be had and I don't think anything about it would block implementing verify_type as it currently is. Adding TokenTypes fits with that task as well (and I'd be happy to do it)

Let me know what you need. Thanks!

[tgross35]

Looks great, thanks for all the work you put into this!

No problem, thanks for checking!

One question, do you think it would be worth adding a mypy check for CI to verify the types that we defined match up with how we are using them? If so, I don't think it would need to happen as part of this PR

Not a bad idea if you're OK sticking with typing going forward. There are likely a handful of things that need to be resolved before mypy passes, so handling it in a separate issue is probably a good idea. Your call, doesn't matter to me either way.

[vimalloc]

One question, do you think it would be worth adding a mypy check for CI to verify the types that we defined match up with how we are using them? If so, I don't think it would need to happen as part of this PR

Not a bad idea if you're OK sticking with typing going forward. There are likely a handful of things that need to be resolved before mypy passes, so handling it in a separate issue is probably a good idea. Your call, doesn't matter to me either way.

Yep, I think a separate PR would be just fine. 👍

[tgross35]

Heya @vimalloc, are you waiting on #441 to tag a release for pypi? I've been patiently waiting to use the new features, wasn't sure if it's waiting on me :)

[vimalloc]

Nope! I’ve just been meaning to clean up a few last things. I’ll try to get the release out when I’m back at home in a few days :)

[tgross35]

Sounds good, thanks for the quick reply! Let me know if there's anywhere to help.