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

Fixes #70. Implemented registry pattern for class-based algorithms #71

Merged
merged 9 commits into from
Jan 18, 2015

Conversation

mark-adams
Copy link
Contributor

Algorithm-specific logic has been moved to the algorithms module and refactored into classes to make it a bit easier for others to add new standard or non-standard algorithms to the library.

Things to note:

  • The public APIs (jwt.encode() and jwt.decode()) are completely unchanged
  • jwt.register_algorithm() has been added to the public API for extensibility
  • Crypto code that formally resided in signing_methods, verify_methods, and prepare_key_methods lists / functions has been rearranged into classes in algorithms.py. The sign, verify, and prepare_key methods are all nearly identical to their original versions.

@mark-adams
Copy link
Contributor Author

Agreed. I meant to do that (NotImplementedError) but I was too excited about getting to the rest of the refactoring and then I missed it when I picked this branch back up after the holidays. :-)

I'll push up the commit in just a sec

@mark-adams
Copy link
Contributor Author

There we go. Tests are now green again.

@jpadilla
Copy link
Owner

jpadilla commented Jan 6, 2015

@progrium I'd like your take on this.

@mark-adams
Copy link
Contributor Author

@jpadilla It's always scary whenever someone moves around 80% of the code in a library, huh? :-)

@wbolster
Copy link
Contributor

wbolster commented Jan 7, 2015

This PR needs some rework to apply cleanly on top of current master.

@mark-adams
Copy link
Contributor Author

Thanks for the heads up @wbolster!

@progrium @jpadilla This pull request has been rebased against the master with the PRs from the last 24 hours and is ready for your review.

@jpadilla
Copy link
Owner

jpadilla commented Jan 7, 2015

@mark-adams @wbolster thanks! Will try to have @progrium check this out to have his feedback since it's a "big" fundamental change before we make any decisions.

@mark-adams
Copy link
Contributor Author

Absolutely!! I totally understand.

However, I do want to point out two very important things to keep in mind here:

  • The public API (jwt.encode() and jwt.decode()) is completely unchanged with the exception of the fact that register_algorithm has been added as an extensibility point.
  • The crypto related code has simply been shifted form using signing_methods, verify_methods, and prepare_key_methods to using an OO approach that is a little more understandable and a little easier to maintain. The contents of the sign, verify, and prepare methods are nearly identical to their original versions in __init__.py

A LOT of code looks like it changed here, but a lot of the code actually didn't change here. It's just organized a bit differently. :-)

That being said, I totally understand the need for review. I'll anxiously await @progrium's review. Let me know if y'all have any questions.

@mark-adams
Copy link
Contributor Author

@progrium, This has been rebased against the current master and is waiting for your review.

cc: @jpadilla @wbolster

@jpadilla
Copy link
Owner

@mark-adams ready to move this forward. One thing I'd like to do before merging this in would be to write some comment blocks for the algorithms.

@mark-adams
Copy link
Contributor Author

Rebased against the latest master and added some comments for jwt.algorithms and jwt.register_algorithm

jpadilla added a commit that referenced this pull request Jan 18, 2015
Fixes #70. Implemented registry pattern for class-based algorithms
@jpadilla jpadilla merged commit 6290a1c into jpadilla:master Jan 18, 2015
@jpadilla
Copy link
Owner

@mark-adams awesome thanks!

@mark-adams mark-adams deleted the algorithm-registry branch January 18, 2015 19:34
@skion
Copy link
Contributor

skion commented May 8, 2015

A bit late to the party, but I'm trying this in view of #42 at the moment...

The Algorithm abstraction doesn't provide access to the unprotected header, just the payload. This can be inconvenient if you want the algorithm to manage e.g. the kid header automatically.

Problem is that since the jwt.header() method has been removed, you also can't work around it on the application level.

Any tips?

@mark-adams
Copy link
Contributor Author

In the 2.0 release (#144 to be specific), we're planning on adding additional support for JWK that should make key management a bit easier without having to resort to a lot of trickery. I think this will help resolve a lot of what you're dealing with.

I wouldn't be opposed to adding a function to retrieve the unverified header values. Created #155

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

Successfully merging this pull request may close these issues.

4 participants