JWT header alteration #352
Conversation
JWT Header conveyed by the event
Encoder/DefaultEncoder.php
Outdated
| @@ -29,10 +29,10 @@ public function __construct(JWSProviderInterface $jwsProvider) | |||
| /** | |||
| * {@inheritdoc} | |||
| */ | |||
| public function encode(array $payload) | |||
| public function encode(array $header, array $payload) | |||
There was a problem hiding this comment.
Given this class can be an extension point, I think we should keep BC. We can do it by not changing the signature right now, handling the new argument only if passed and triggering a deprecation if it is not, saying that this method takes a new argument that will be mandatory in 3.0.
It can be done using func_num_args()/func_get_arg(), see https://github.com/symfony/symfony/blob/3.3/src/Symfony/Component/Console/Helper/QuestionHelper.php#L104 for inspiration
There was a problem hiding this comment.
Btw, let's make $header the last arg, that will give a simpler upgrade path. I would also make it optional, so we can let the interface as is and rethink about changing it in 3.0 add a new HeaderAwareEncoderInterface implemented by our encoders (no upgrade path possible for interfaces, we definitely have to make this new arg optional, which is fine to me, and we don't have to trigger any deprecation).
|
Thanks for working on this @Spomky! |
|
You're welcome. |
|
Hi @chalasr, Sorry for the delay. Comments are welcome. Kind regards. |
Event/JWTDecodedEvent.php
Outdated
| */ | ||
| public function getHeader() | ||
| { | ||
| return $this->payload; |
There was a problem hiding this comment.
should be $this->header :) I guess this is what breaks tests
Encoder/JWTEncoderInterface.php
Outdated
| * | ||
| * @return string the encoded token string | ||
| * | ||
| * @throws JWTEncodeFailureException If an error occurred while trying to create | ||
| * the token (invalid crypto key, invalid payload...) | ||
| */ | ||
| public function encode(array $data); | ||
| public function encode(array $data, array $header = []); |
There was a problem hiding this comment.
Adding an argument here still breaks BC, even if optional. The signature in concrete implementations must match the interface one. What we need is to introduce a HeaderAwareEncoderInterface which extends JWTEncoderInterface and just redefine encode() with this new arg, then make our built-in encoders implement it and check that the passed encoder does implement it before relying on the new argument internally
Encoder/JWTEncoderInterface.php
Outdated
| * | ||
| * @return array | ||
| * | ||
| * @throws JWTDecodeFailureException If an error occurred while trying to load the token | ||
| * (invalid signature, invalid crypto key, expired token...) | ||
| */ | ||
| public function decode($token); | ||
| public function decode($token, array &$header = []); |
There was a problem hiding this comment.
Actually I would not change decode() as we don't allow to pass a payload here, just an immutable (stringish) token, its content should not be altered at this stage I think. But maybe you have a specific use case in mind?
There was a problem hiding this comment.
The idea is to find a way to return the header after the decoding process, not to modify it.
But I have no specific use case and I will remove it.
|
Thanks a lot @Spomky |
This PR tries to fix #351 .
Please note that method signatures are changed.
BC is kept but a minor release seems to be needed.