Fixed a small bug with the JWT Bearer Grant Type #38
Conversation
…is not included in the clientData array.
|
We should add a test for this |
| @@ -150,20 168,27 @@ public function getClientDataFromRequest(OAuth2_RequestInterface $request) | |||
| */ | |||
| public function validateClientData(array $clientData) | |||
There was a problem hiding this comment.
Now that I think about it, all the checking in here is redundant because they would get caught in getClientDataFromRequest(). What do you think if we just return true here?
There was a problem hiding this comment.
Well it looks like the validation is slightly different in either case, so it seems fine to me.
|
Test added and some minor improvements. |
| @@ -172,6 172,20 @@ public function testBadSbuject() | |||
| $this->assertEquals($response->getParameter('error'), 'invalid_grant'); | |||
| $this->assertEquals($response->getParameter('error_description'), 'Invalid issuer (iss) or subject (sub) provided'); | |||
| } | |||
|
|
|||
| public function testMissingKey(){ | |||
There was a problem hiding this comment.
this needs to have the curly brace in the next line
| return null; | ||
| if (!empty($diff)) { | ||
|
|
||
| $this->response = new OAuth2_Response_Error(400, 'invalid_grant', "Invalid issuer (iss) or subject (sub) provided"); |
There was a problem hiding this comment.
This error message seems to be incorrect. the parameters being checked for are not the names of the parameters in the error message
There was a problem hiding this comment.
Yes, that is true, however from the client's point of view, the cause of the parameters being incorrect would be because they had an invalid iss or sub.
Having said that, Perhaps it might be better to remove this after all, because having an invalid iss and sub will be the only issues for causing an error, and this is already being done in getClientDataFromRequest().
There was a problem hiding this comment.
I would advocate throwing a logic exception here, as it's good to check for
these things, but this should never be broken unless the implementer of the
library has done something wrong.
Sincerely,
Brent Shaffer
@bshaffer http://twitter.com/bshaffer
On Mon, Jan 21, 2013 at 4:23 PM, F21 [email protected] wrote:
In src/OAuth2/GrantType/JWTBearer.php:
return null;if (!empty($diff)) {
$this->response = new OAuth2_Response_Error(400, 'invalid_grant', "Invalid issuer (iss) or subject (sub) provided");Yes, that is true, however from the client's point of view, the cause of
the parameters being incorrect would be because they had an invalid iss or
sub.Having said that, Perhaps it might be better to remove this after all,
because having an invalid iss and sub will be the only issues for causing
an error, and this is already being done in getClientDataFromRequest().—
Reply to this email directly or view it on GitHubhttps://github.com//pull/38/files#r2719620.
There was a problem hiding this comment.
Ahh! That seems to be much nicer than the current implementation! :)
…use the errors can only be triggered by developer error.
…es that uses ClientAssertionType does not require it.
|
I have just removed the check to see if the storage implements the |
…e client_credentials storage.
…added variable to store undecoded JWT.
…l_sign(): supplied key param cannot be coerced into a private key".
|
Strange that the build failed. I will get this fixed asap. |
|
Looks like in PHP 5.2 on travis-ci, openssl is not available. On 5.3, OPENSSL_ALGO_SHA256 is not available :( |
|
This is because due to incompatibility problems, openssl is disabled on PHP 5.2. |
…ackwards compatibility with PHP 5.3.
…thm since openssl is not avaliable for PHP 5.2 on travis-ci.
|
Now, for PHP 5.2, we test using a symmetric algorithm such as SHA-256 instead of a public/private key pair, since openssl is not available in PHP 5.2 on travis-ci. |
Fixed a small bug with the JWT Bearer Grant Type
There was a small bug where I forgot to include the key to decode the JWT in
$clientData.This has now been fixed.