Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add flexible and complete verification options #131

Merged
merged 7 commits into from
Apr 9, 2015

Conversation

michaeldavis-wf
Copy link
Contributor

This adds the ability to use a more flexible option dictionary. The options dictionary can be specified at the PyJWT object level or as an override passed directly into decode()

Attempts to fix #127

@jpadilla
Copy link
Owner

jpadilla commented Apr 8, 2015

Oh yeah, this looks good to me.

@mark-adams would like your feedback, since you created #127.

'verify_exp': True,
'verify_nbf': True,
'verify_iat': True,
'verify_aud': True,

Choose a reason for hiding this comment

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

I would like to see these abbreviations expanded. The only reason they are abbreviated in the standard is to reduce the amount of data transmitted. Here it just serves to obfuscate.

Choose a reason for hiding this comment

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

How many people will choose to solve, non-verification messages by figuring out they can set all of these to False rather than taking the time to understand what it all means?

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 only way to avoid consumers skipping verification is to completely disallow it, which ignores a huge number of use cases that need to turn off various checks.

Copy link
Contributor

Choose a reason for hiding this comment

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

I support keeping these abbreviated so they are consistent with the claim names. This is similar to how the Ruby JWT library does things. I think we should keep them the way they are.

Regarding @johnlockwood-wf's comment about setting everything to False; I'm not sure we can avoid people being stupid with their usage. Defaulting to secure (and to the standard) is the best behavior here.

Choose a reason for hiding this comment

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

they should at least be labeled with expansions, or have a "constants" class that labels the keys, such as:

class CLAIMS_KEYS(object):
    class VERIFY(object):
        EXPIRATION = 'verify_exp'
        ISSUED_AT = 'verify_iat'

@mark-adams
Copy link
Contributor

Also, @michaeldavis-wf, make sure you add yourself to the AUTHORS file

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 99.76% when pulling e62df13 on michaeldavis-wf:options-dict into a2601ad on jpadilla:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 99.76% when pulling e62df13 on michaeldavis-wf:options-dict into a2601ad on jpadilla:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 99.76% when pulling e62df13 on michaeldavis-wf:options-dict into a2601ad on jpadilla:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 99.76% when pulling 13d2420 on michaeldavis-wf:options-dict into a2601ad on jpadilla:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 99.76% when pulling 13d2420 on michaeldavis-wf:options-dict into a2601ad on jpadilla:master.

@@ -16,7 +16,7 @@


class PyJWT(object):
def __init__(self, algorithms=None):
def __init__(self, algorithms=None, options=None):

Choose a reason for hiding this comment

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

what do you think about the keyword being, verification_options, so they are well categorized.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's no reason why we couldn't use options= for other options later. I feel like verification_options would restrict what we could do with this argument unnecessarily.

In addition, all the current options start with verify_ so I feel like it'd be a bit redundant to call it verification_options.

Choose a reason for hiding this comment

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

Grouping arguments into a category is better than having a lot of uncategorized arguments. If there are more unrelated types of arguments in the future, they could be added to another group.

Choose a reason for hiding this comment

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

and to reduce redundancy, the verification_options dict could leave out the verify_ prefix of the keys.

Choose a reason for hiding this comment

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

verify_claims = {'exp': False, 'aud': False}

Choose a reason for hiding this comment

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

As maintainers it is also easier to commit to a more restricted change to the interface.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the long-run, I'd rather add some sort of custom extensibility point (read: callable) that you could pass for additional validations on top of those provided by the library.

I don't feel like we're at risk here of adding "too many unrelated arguments or options" even though I do understand your concern. We aren't going to add all sorts of crazy willy-nilly validations that will require new options; our main focus is on parity with the validations with the JWT spec and I can't see it requiring us to add too many more options here outside of what's already in the spec. A couple more may be added from some additional work we'll probably wind up doing with JWK, but I think the number of new options would be very limited.

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 have a problem discussing more ideas in future PRs but I believe this is better than what we currently have for sure.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 99.76% when pulling 0ab287b on michaeldavis-wf:options-dict into a2601ad on jpadilla:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 99.76% when pulling 0ab287b on michaeldavis-wf:options-dict into a2601ad on jpadilla:master.

@mark-adams
Copy link
Contributor

@michaeldavis-wf Last thing and I'm cool with merging this... can you add something to the CHANGELOG?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 99.76% when pulling 6e6046c on michaeldavis-wf:options-dict into a2601ad on jpadilla:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 99.76% when pulling 6e6046c on michaeldavis-wf:options-dict into a2601ad on jpadilla:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 99.76% when pulling 6e6046c on michaeldavis-wf:options-dict into a2601ad on jpadilla:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 99.76% when pulling 6e6046c on michaeldavis-wf:options-dict into a2601ad on jpadilla:master.

mark-adams added a commit that referenced this pull request Apr 9, 2015
Added `options=` argument to decode()
@mark-adams mark-adams merged commit 2f4c770 into jpadilla:master Apr 9, 2015
@michaeldavis-wf
Copy link
Contributor Author

@mark-adams @jpadilla Would it be possible to cut a release with this functionality in it, or possibly after #132 goes in?

@jpadilla
Copy link
Owner

jpadilla commented Apr 9, 2015

@mark-adams I'm fine with a new release whenever you want. You could create a PR with the version bump, etc and I'll go ahead and publish the release.

@mark-adams
Copy link
Contributor

Cool. I'll try to wrap things up tonight. I had a couple small changes I wanted to make

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add more flexible and complete verification options
5 participants