Fix Auth0::exchange() to handle missing id_token #318
Conversation
joshcanhelp
force-pushed
the
fix-exchange-method-id-token-missing
branch
from
4c98328
to
3ef19d3
Compare
| * | ||
| * @var array | ||
| */ | ||
| protected $idTokenDecoded; |
There was a problem hiding this comment.
Store a decoded version of the ID token (remains null if none was returned/stored).
| @@ -283,6 +301,11 @@ public function __construct(array $config) | |||
| $this->guzzleOptions = $config['guzzle_options']; | |||
| } | |||
|
|
|||
| $this->skipUserinfo = false; | |||
There was a problem hiding this comment.
Default to false so existing behavior is retained.
| @@ -492,8 +515,10 @@ public function getRefreshToken() | |||
| /** | |||
| * Exchange authorization code for access, ID, and refresh tokens | |||
| * | |||
| * @throws CoreException - if an active session already or state cannot be validated. | |||
| * @throws ApiException - if access token is invalid. | |||
| * @throws CoreException If the state value is missing or invalid. | |||
There was a problem hiding this comment.
This method does not throw any new Exceptions, just adding docs for missing ones.
| $idToken = false; | ||
| if (isset($response['id_token'])) { | ||
| $idToken = $response['id_token']; | ||
| if (! empty($response['id_token'])) { |
There was a problem hiding this comment.
Main bug fix is here ... only call setIdToken() (which validates) if there is one returned.
There was a problem hiding this comment.
I see you're using 3 different ways of checking if the value is set for the given properties before calling the associated "setProperty" method.
if (isset($response['refresh_token']))if (! empty($response['id_token']))if ($user)
Why is that?
There was a problem hiding this comment.
I kept if (isset($response['refresh_token'])) because that's what it was before, no really good reason to change. This one won't error out attempting to set an empty one.
if (! empty($response['id_token']) is there because if it is empty then it will error out so need to be more careful. It's unlikely that it would be there but empty but still, better to be safe in this case.
if ($user) needs to be not empty and not null. empty($user), in this case would be the same.
|
|
||
| $user = $this->authentication->userinfo($accessToken); | ||
| $this->setUser($user); | ||
| if ($user) { |
There was a problem hiding this comment.
Method requires an array so we need to make sure we have something before attempting to set.
There was a problem hiding this comment.
isn't user something that must always be set? what happens if you don't set it, shouldn't it be considered an error scenario?
In that case, why is this function returning true always?
There was a problem hiding this comment.
isn't user something that must always be set?
Not if you only ask for API scopes including an audience param when authorizing. It's possible to use this function to handle a code exchange for an scoped API token without an ID token or access to /userinfo.
In that case, why is this function returning true always?
The exchange happened without an error so exchange() is true. It's not the most well-composed method but it's well-documented and used so we need to retain the basic functionality.
| @@ -0,0 +1,152 @@ | |||
| <?php | |||
There was a problem hiding this comment.
See Add failing test commit to see initial tests failing with existing functionality.
| * @throws \Auth0\SDK\Exception\ApiException | ||
| * @throws \Auth0\SDK\Exception\CoreException | ||
| */ | ||
| public function testThatExchangeSucceedsWithIdToken() |
There was a problem hiding this comment.
This succeeded before the changes and after.
| * @throws \Auth0\SDK\Exception\ApiException | ||
| * @throws \Auth0\SDK\Exception\CoreException | ||
| */ | ||
| public function testThatExchangeSucceedsWithNoIdToken() |
There was a problem hiding this comment.
This failed before the change and after.
| * @throws \Auth0\SDK\Exception\ApiException | ||
| * @throws \Auth0\SDK\Exception\CoreException | ||
| */ | ||
| public function testThatExchangeSkipsUserinfo() |
There was a problem hiding this comment.
Additional test for the skip_userinfo config.
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Changes
Fix Auth0::exchange() to properly handle code exchanges that do not return an
id_token.References
Issue #317
Testing