[Encoder] Handle OpenSSL/phpseclib engines and algorithms

[chalasr]

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	yes
Deprecations	yes
Fixed tickets	n/a
Tests pass?	yes

After discussed in #132, I open this PR representing one of the first new features of the v2.0.

@slashfan I need your opinion about:

• Structure and naming,
  I used \Encoder\NamshiJOSE to avoid possible conflicts between two bridges named JOSE, maybe you have a better name?
  Same for the class names, maybe prefixing encoders by JWT would make more sense?
• Tests,
  If you have any idea or suggestion about how write tests for the new encoders and BTW improve the existing (encoding), the current tests should not pass with the large feature added, I'll try to improve them (for the ones concerning encoders).
• Documentation,
  I think that all can be done in the configuration reference. Do you see any other part that need to be updated?
  Do we need to give a way to generate keys for phpseclib in addition of the openssl keys generation process? (I have a local work in progress containing a GenerateKeysCommand and theOpenSSLKeyGenerator+SecLibKeyGenerator services, I'd have your opinion about providing this feature)
  I think to add something about the key loaders, but not sure about if it shouldn't stay internal.

Waiting for your suggestions.
Also I'll be glad to have the opinion of any developer interested by this changes.

Steps:

• Ability to choose the encoder's encryption algorithm used by the encoder
• Ability to choose the encoder's encryption engine between phpseclib and openssl (default)
• Create a JWSProvider that manages Namshi\JOSE\JWS instances
• Create a new DefaultEncoder that takes the JWSProvider service as argument
• Create two base JWT[Decode/Encode]FailureException classes and their children
• Throw the good exception depending on the fail's reason during DefaultEncoder::[encode/decode]()
• Handle exceptions properly from the JWTProvider
• Update the existing tests / add new ones (unit & functional)
• Update the documentation
• Rebase after merged Add CHANGELOG #164 & update changelog
• Add an UPGRADE file and document this feature

The v2 is coming!

[slashfan]

Hi,

• structure and naming :
  +1 having the vendor name is mandatory to avoid possible conflicts.
• tests :
  I think we should only test the different encoders services initialization as the underlying encoding libraries are already tested.
• documentation :
  +1 for the configuration reference.
  I'm not sure it's the bundle's work to provide keys generation logic, a simple explanation in the documentation should suffice. What do you think ?

Again, thanks for the hard work @chalasr !

[chalasr]

@slashfan You're welcome, I'll personally benefit from this changes, plus it's a pleasure to work on cool things.

For the keys generation, you're right, I suspected that it is the reason of why you don't provide it before.
For the tests, agreed, no need of test third party tools, only that our services behave properly (and maybe the classes themselves, I'll investigate).

I be back when I've some news!

[chalasr]

@slashfan

• Tests added for Encoders (unit+functional, see Tests\Encoder & Tests\Functional\Encoder).
• Documentation updated:
  Configuration reference: Add the new options + Mention the encoder customization chapter.
  Encoder customization: Present the built-in encoders + Give a list of available algorithms.

Let me know about your eventual suggestions (especially about naming and documentation).
Could you setup Scrutinizer as PR-check for the 2.0 branch? It's useful to catch possible optimisations and typos.

Have a good review :)

[weaverryan]

For a 2.0 release, the class parameters could be removed. But let's do it in a different PR (later) to keep things focused here :)

[chalasr]

@weaverryan Totally agreed. As long as I worked on different files I saw things that need to be changed, such as the documentation that needs more clean examples with namespaces, following best practices. It itches me to change them, but as you said, it's out of the scope of this PR. So, I'll propose this changes in a next one. Thank you for the review!

[weaverryan]

Hi guys!

Really happy to see this stuff :). I started reviewing and I have one suggestion I'd like you to consider: avoid renaming services or classes (unless we have a real reason). For example, the OpenSSLKeyLoader class moved to a new namespace and its service name was changed slightly. If we can minimize these changes, we could make it much easier for people to upgrade. We could even considering making changes in a BC way like the Symfony core does, though this is a bit more work to accomplish :).

Let me know what you think. And thank you!

[chalasr]

@weaverryan
I don't considered this before so thank's for telling me.
Do you think that, because the bundle is based on the Namshi/JOSE library by default, it could be clean to don't differentiate it by its namespace, and so remove the NamshiJOSE\ part?
The goal is to provide more encoders based on different libraries in the future, and so differentiate them like this.
The main difficulty is that we now have the choice between OpenSSL and SecLib, and so it would be strange to have one JWTEncoder (old, instead of new OpenSSLEncoder) and one SecLibEncoder, right?
But, as the two classes represent ATM only a constructor (all the logic is inside the BaseEncoder), with a bit of imagination I can quickly remove them in order to use the BaseEncoder as the only class (can be keep its old name, JWTEncoder), have only one service, and play with arguments to make the service instantiates the class with the good ones, depending on configuration.
Like this, we could keep services and classes names pretty much same as before.
It would be nice to have your opinion.

Also, what do you mean by "making changes in a BC way like the Symfony core does"?
Keep the old things while adding new ones and mark them as deprecated before remove them in a next release?

I have to learn a lot about keeping BC as possible and letting the time to upgrade, I'm not sure to do the good choices, So many thanks for your help on these several questions.

@slashfan What do you think about this?

[weaverryan]

Do you think that, because the bundle is based on the Namshi/JOSE library by default, it could be clean to don't differentiate it by its namespace, and so remove the NamshiJOSE\ part?

If the library were new today, then I think having the NamshiJOSE makes sense. But I don't think adding it adds enough value for the added BC breaking. For that small reason, I would prefer to not have it.

The main difficulty is that we now have the choice between OpenSSL and SecLib, and so it would be strange to have one JWTEncoder (old, instead of new OpenSSLEncoder) and one SecLibEncoder, right?

This change - unlike moving the OpenSSLKeyLoader class - has a real reason behind it, so I think it makes sense. But, see my comment below about the Symfony way of doing deprecations.

Like this, we could keep services and classes names pretty much same as before.
It would be nice to have your opinion.

If you can make everything work with one class, and it's a nice solution (I mean: you're not forcing and hacking things together to make it work), that would be awesome! :)

Also, what do you mean by "making changes in a BC way like the Symfony core does"?
Keep the old things while adding new ones and mark them as deprecated before remove them in a next release?

Yes, you have exactly the idea :). If you kept the old way of doing things and just deprecated them, then we could merge this into the 1.x branch (as you wouldn't be breaking BC). If you followed the Symfony philosophy entirely, when 2.0 is released, we would simply remove the old deprecated features (this was done with Symfony 2.8 and 3.0). To give a specific example, if you did rename the JWTEncoder class, you could actually keep it, but make the new class extend the deprecated class, and trigger a deprecation warning in the old class. These things are tricky to get right (even for me) - I often look in Symfony's core for the different strategies on how to properly deprecate things. And honestly, in a smaller library like this (small compared to Symfony), we need to be pragmatic: doing perfect deprecations can take some time. But if you're interested in learning about it, this is a great opportunity.

Cheers!

[chalasr]

@weaverryan Thank you very much for take the time to educate me on this way, it will be helpful for my next contributions on Symfony and community bundles.

Create the two services is the "easy" way, because the logic is already in place , we can use the encoder_service configuration option to choose one of the available services, rather than creating a new encoder_engine option that could be useless if the bundle is used with an other library than NamshiJOSE.
However, I can't bring all the features added here inside the old JWTEncoder so, as you said, I'll deprecate it and keep its class+service intact, in order to remove it in a next release.
Also I'll remove the NamshiJOSE part of the new encoders namespace, and bring them into only one class (the BaseEncoder, I need help to name this new class, as it will be located in the same directory of the deprecated one, maybe DefaultEncoder?), then use it as class for the two new services (contrarily to the classes, I can't bring them together in one because their existence makes that's easy to switch).

Please let me know if I'm wrong, if I misunderstood or if you see a better way for something :)

I'll wait for the opinion of @slashfan and do my best for make upgrading as soft as possible for developers, while providing these new features.

EDIT

I just realize that you was talking about the OpenSSLKeyLoader and not the new OpenSSLEncoder.
I can effectively keep the old service and deprecate it, as the KeyLoader has been recently added in 1.x (master), it would be more soft.

[chalasr]

Encoder update:

• Moved the Encoder\NamshiJOSE\BaseEncoder to Encoder\DefaultEncoder
• Re-added the old JWTEncoder class+service and deprecate them since 2.0 (to be removed in ?? ATM I said 3.0)
• Made the new Encoder\DefaultEncoder extends the actual (old) JWTEncoder.
• Removed the NamshiJOSE\OpenSSLEncoder + NamshiJOSE\SecLibEncoder and made the two corresponding services use the DefaultEncoder class, and so the NamshiJOSE namespace has been totally removed as its our default implementation from the beginning.
• Changed the two new service names from lexik_jwt_authentication.encoder.namshi_jose.[engine] to lexik_jwt_authentication.encoder.default.[engine]

About the OpenSSLKeyLoader, I tried to follow the @weaverryan advices as possible and so marked it as deprecated while keeping the class and service, that are used in the deprecated JWTEncoder.
But, because of the new support of SecLib, I cannot avoid the new structure (Services\KeyLoader namespace) and make the new OpenSSLKeyLoader extending the old, because the new already extends the AbstractKeyLoader.
I think this old OpenSSLKeyLoader can be quickly removed and inject the new OpenSSLKeyLoader in the deprecated JWTEncoder.

I think that keep the JWTEncoder is a good thing even if this PR is for the 2.0 branch.

[weaverryan]

You can officially deprecate these - which will cause the user to see a deprecation warning :) http://symfony.com/blog/new-in-symfony-2-8-deprecated-service-definitions

[chalasr]

@weaverryan I was glad to discover this feature, but as we have some tests running on old symfony versions, the tests don't pass anymore :/ So I had to rollback. I need to discuss with maintainers to know if we keep the compatibility with symfony versions older than 2.8. What do you think about this?

[weaverryan]

Ah, that's interesting! As long as the class itself is deprecated (with the trigger), it's not big deal.