-
Notifications
You must be signed in to change notification settings - Fork 952
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
Fix wrong redirect following authorization step #171
Fix wrong redirect following authorization step #171
Conversation
👍 I encountered this issue after pressing Cancel on the authorization request form. No redirect_uri param was given in the URL and the lib was not using the redirect_uri from storage, so Response threw an exception about redirecting to an empty URL. This PR fixed it. |
As the existing test suite did not catch this error, we need to include tests to check this case. |
@@ -86,6 +93,11 @@ public function handleAuthorizeRequest(RequestInterface $request, ResponseInterf | |||
$authResult = $this->responseTypes[$this->response_type]->getAuthorizeResponse($params, $user_id); | |||
|
|||
list($redirect_uri, $uri_params) = $authResult; | |||
|
|||
if ( empty($redirect_uri) && ! empty($registered_redirect_uri) ) { |
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.
We need to keep whitespace consistent here
I fixed problematic whitespace indentation and amended the commit for this PR, as a side affect @bshaffer's comments where hidden. Sorry about that. As for unit test, I'm having problems running the suite locally, will try again later today if time permits. On a side note, do you have or plan on having a contribute guide? |
Good call, I do not have a contribute guide, but will add one shortly. Thank you for the fix! The problem running the suite probably has to do with the redis tests... Unless you have a redis server running on localhost, they will fail. |
Fix for #163 (#163) allowed authorize flow to handle `redirect_uri` as an optional parameter. The change resulted in user not being redirected back to the client after granting or denying client's access. This fix verifies whether `redirect_uri` is set or not in `OAuth2\Controller\AuthorizeController`'s `handleAuthorizeRequest()` method. If it's not set, it grabs the client's registered value. If/When support for PHP 5.3 is dropped, the code can be modified to `$this->clientStorage->getClientDetails($this->client_id)['redirect_uri']`.
I apologize for taking so long. Test added to verify #163 was not checking the actual value contained in Changes from |
Looking good! 😄 |
👍 |
…on-flow Fix wrong redirect following authorization step
#163 fix broke redirect after granting or denying client's access. If there is no
redirect_uri
,null
is stored inoauth_access_tokens
, which is correct. However, when user grants or denies access,OAuth2\Controller\AuthorizeController->handleAuthorizeRequest()
redirected to?code=...
. The fix in this PR checks the value, and if empty, loads client's registered URI.