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

Refactor api.py functions into an object (PyJWT) #101

Merged
merged 7 commits into from
Mar 17, 2015

Conversation

mark-adams
Copy link
Contributor

I know this is a big PR but really, the changes are quite simple and the public API is 100% unchanged except that the PyJWT class is exposed as a public part of the API. I think this paves the way for us to make more extensibility improvements going forward.

This PR refactors the functions in api.py into a PyJWT object. Changing these into an object instead of standalone functions makes extensibility (pluggable components, algorithms, etc.) much easier to implement and also allows for non-global configuration changes to be made by the consumer. For instance, by creating two instances of the PyJWT class, a consumer could configure two instances with entirely different algorithm sets. This would be a huge challenge using the current API.

  • New class in api.py (PyJWT). All existing api.py functions (encode(), decode(), load(), verify_signature(), and register_algorithm() have been moved into this class.
  • load() and verify_signature() are private parts of the API so they have been named PyJWT._load() and PyJWT.__verify_signature.
  • _jwt_global_obj singleton has been added to api.py to allow for a global instance of PyJWT to preserve existing API functionality. init.py has been changed to import encode(), decode(), and register_algorithm() from this global object
  • Tests have been modified to cope with the new design.
  • test_jwt.py has been renamed to test_api.py since its tests focus on api.py code exclusively.
  • A new test_jwt.py has been created to verify that jwt.encode and jwt.decode functions still work. (Since most tests focus on the behavior of the PyJWT class, this is primarily to make sure that our public APIs properly pass calls to the PyJWT global object)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 6fe696f on mark-adams:pyjwt-obj into 2d0e827 on jpadilla:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 99.3% when pulling 66b6d87 on mark-adams:pyjwt-obj into 2d0e827 on jpadilla:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 66b6d87 on mark-adams:pyjwt-obj into 2d0e827 on jpadilla:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.37%) to 98.63% when pulling a8e86b5 on mark-adams:pyjwt-obj into 2d0e827 on jpadilla:master.

@wbolster
Copy link
Contributor

I understand why you did it, but it feels ugly to me to have a PyJWT class that needs to be instantiated. First, a PyJWT instance is not a "class representing a token", so the name confused me a bit. Additionally, having instances (and hence state) mentally clashes with the idea of "stateless tokens". :-)

Perhaps some nicer API can be devised? I don't really have inspiration TBH.

@mark-adams
Copy link
Contributor Author

Ironically, I called it PyJWT because I thought JWT was a confusing object name. I thought PyJWT was at least as good as the jwt module name. :-)

We have state already with the existing ability to register algorithms. This change only makes it easier for us to have configurable options such as custom pluggable validators, etc. For instance, in some applications, somebody may want to accept only HMAC tokens in certain scenarios or only RSA tokens and this allows that sort of configuration without descending into argument hell when people execute the methods.

For those who use the existing APIs, its really completely identical. They will use a singleton instance of the class with global state and won't know any different. The object is only instantiated once and works exactly like it does today if you call jwt.encode or jwt.decode

@wbolster
Copy link
Contributor

The alternative would be context=... objects or something like that, but that's not fundamentally better indeed. Though cryptography uses backend=... all over the place.

@mark-adams
Copy link
Contributor Author

That's true. That is an option. However, it is sometimes one of the more tiring things in cryptography since you have to add the backend parameter to every call. In their case, it does make sense because you have the option of entirely different backends. I think its kind of argument overload for this situation, in my opinion.

@jpadilla jpadilla modified the milestone: v1.0.0 Mar 17, 2015
…lass.

- Created a singleton instance to preserve jwt.encode, jwt.decode,
  jwt.register_algorithms existing public APIs
- Renamed load and verify_signature to _load and _verify_signature since
  they are not part of the existing public API
- Modified related tests to use PyJWT._load and PyJWT._verify_signature
- test_jwt.py has been renamed to test_api.py since it focuses entirely
  on api.py
- A new test_jwt.py has been introduced that focuses exclusively on
  testing the public API. (Specifically, making sure that encode and
  decode still exist and function). This test can be extremely simple
  since the jwt.encode and jwt.decode functions are backed by PyJWT()
  which is tested elsewhere.
… parameter to the PyJWT constructor. Also, PyJWT now has a get_supported_algorithms() method that returns back valid values for 'alg'
@coveralls
Copy link

Coverage Status

Coverage decreased (-24.23%) to 75.77% when pulling 5dec6e2 on mark-adams:pyjwt-obj into d471631 on jpadilla:master.

jpadilla added a commit that referenced this pull request Mar 17, 2015
Refactor api.py functions into an object (PyJWT)
@jpadilla jpadilla merged commit 1e6b6c5 into jpadilla:master Mar 17, 2015
@jpadilla
Copy link
Owner

Gotta add some of this to the README.

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.

None yet

4 participants