-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add ES384 support #324
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 ES384 support #324
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
1 similar comment
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
@googlebot Is this blocked by anyhing? What can we do to get this merged? Thanks! |
I guess we need a review but this repo is quite dead IMHO |
That's too bad .. I think it's the most used PHP JWT library at the moment... |
@BramRoets @sdrsdr it's not dead yet!! Sorry for the late reply, I missed this PR. This is a fantastic addition, I'll review / merge today! |
|
||
// Verify decoding succeeds | ||
$publicKey = file_get_contents(__DIR__ . '/ecdsa384-public.pem'); | ||
$decoded = JWT::decode($encoded, $publicKey, array('ES384')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I switch both occurences of ES384
above to ES256
, and likewise if I switch ES256
to ES384
in the test function testEncodeAndDecodeEcdsaToken
, the tests still pass. Can we use test-cases which are not interchangeable?
Also, since these two tests are so similar, it would be better to use a data provider:
/**
* @runInSeparateProcess
* @dataProvider provideEncodeAndDecodeToken
*/
public function testEncodeAndDecodeToken($privateKeyFile, $publicKeyFile, $alg)
{
$privateKey = file_get_contents($privateKey);
$payload = array('foo' => 'bar');
$encoded = JWT::encode($payload, $privateKeyFile, $alg);
// Verify decoding succeeds
$publicKey = file_get_contents($publicKeyFile);
$decoded = JWT::decode($encoded, $publicKey, array($alg));
$this->assertEquals('bar', $decoded->foo);
}
public function provideEncodeAndDecodeToken()
{
return [
[__DIR__ . '/ecdsa-private.pem', __DIR__ . '/ecdsa-public.pem', 'ES256'],
[__DIR__ . '/ecdsa384-private.pem', __DIR__ . '/ecdsa384-public.pem', 'ES384'],
];
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ES384 and ES256 are meant to be interchangeable the test should and will fail if you try to have only one place changed. I'm not really familiar with PHP code testing. PHP is not in my daily workset, I've long ago removed the setup on my pc needed for testing php code. Can you please merge it as is and then fix it to you liking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will submit test refactor in another PR
@bshaffer Could you make a release including these changes? is there anything we can do to help this process? Many thanks! |
@BramRoets tagged in v5.3.0 |
Wow that was quick! Thanks for the help. |
Taste case plus I've verified to be correctly working against ES384 token from NodeJS system