Skip to content

Supports user authentication and better error handling #23

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

Closed
wants to merge 1 commit into from

Conversation

zhanghaowx
Copy link

  • Use getallheaders() to fetch request headers so that fields like
    "Authorization" can be forwarded
  • remove urldecode when creating $request_url so that if csurl contains
    encoded part it won't be decoded by mistake
  • If curl_exec failed, send back plain text error message. E.g., Curl
    error: SSL certificate problem: unable to get local issuer certificate.

* Use getallheaders() to fetch request headers so that fields like
"Authorization" can be forwarded
* remove urldecode when creating $request_url so that if csurl contains
encoded part it won't be decoded by mistake
* If curl_exec failed, send back plain text error message. E.g., Curl
error: SSL certificate problem: unable to get local issuer certificate.
@softius
Copy link
Owner

softius commented Jul 25, 2016

Hi @zhanghaowx, I understand that this PR introduce three fixes / improvements. Is it possible to explain won't be decoded by mistake further by providing a test case?

Last, there is another PR dealing ( #24 ) with errors .

@zhanghaowx
Copy link
Author

Hi @softius, here is an example in real world usage: we want to request this url () through the proxy, https:///rest/v1/searchResults/page;firstResult=1;maxResults=50?fields=["ASSET_ID"]&filters=%7B%22classes%22:%5B%22GEO_NOTE_METADATA%22%5D,%22spatial%22:%7B%22operator%22:%22intersect%22,%22boundingBox%22:%7B%22lowerLeftLatY%22:48.164062500001,%22lowerLeftLonX%22:11.579589843751,%22upperRightLatY%22:48.175048828124,%22upperRightLonX%22:11.590576171874%7D%7D%7D

As you can see, we have some encoded data in the URL parameters which will cause problem if we decode them before sending the actual request. For now it works fine for us, both request through ?csurl and HTTP_X_PROXY_URL

@zhanghaowx
Copy link
Author

For point 3, looks like #24 provides a more completed solution. Once you merged their changes, I will do some tests and see if it is still valid. I can also make 3 split PR requests if you prefer, which might be easier for you to manage.

@softius
Copy link
Owner

softius commented Aug 1, 2016

I agree that the urldecode might not be necessary for HTTP_X_PROXY_URL. But I still think is necessary for csurl option since we are passing a URL as a query parameter and we need that to be valid.

Also referring to the example provided the URL to be encoded / decoded is https:///rest/v1/searchResults/page;firstResult=1;maxResults=50 . Anything else can be passed as GET or POST parameters and hence won't be encoded / decoded by the proxy. Hope that's clear.

Last, indeed #24 provides a more completed solution in regards to error handling so I think this PR should be focused on necessary header adjustments to support user authentication and drop unnecessary url encode.

Thanks

@softius softius closed this Jan 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants