ProxyUrl: Determine whether to proxy via web service #3199
Conversation
Co-authored-by: Joe Corall <joe@libops.io>
|
@maccabeelevine, this is looking pretty good -- I'll wait for the encoding conversation to get resolved before doing hands-on testing, but I expect this can be approved and merged pretty quickly once it's finalized. Please just let me know when you're feeling ready! |
@demiankatz I think it's ready for you. |
| || $this->config->EZproxy->prefixLinks; | ||
| $useWebService = $this->config->EZproxy->prefixLinksWebServiceUrl ?? false; | ||
| if ($useWebService) { | ||
| $usePrefix = $this->checkUrl($url) ?? $this->checkConfig(); |
There was a problem hiding this comment.
On closer examination, I'm not really sure if it makes sense to use checkConfig() as the fallback here. It looks to me like the main purpose of the prefixLinks setting is to allow prefixing to be globally disabled. I think if somebody configures both prefixLinks and prefixLinksWebServiceUrl at the same time, that's essentially a configuration error. I suppose it doesn't necessarily hurt anything to leave this as it is, but it just feels to me like it might not be the best option. Maybe we should just fall back to true instead of falling back to checkConfig, for example...
There was a problem hiding this comment.
I disagree on this, although not strongly. That fallback to checkConfig() is going to happen on any exception checking the web service, such as a connection error, which itself is likely enough from time to time (even if the services are co-hosted; things break). So people should think about what the fallback default should be, i.e. it's better as a configured param than hard-coded, and so we may as well reuse prefixLinks. Admittedly the config.ini isn't clear that it's a fallback, so I just fixed that.
There was a problem hiding this comment.
Yes, that makes sense -- I was thinking about it differently, but tweaking the comments is a good solution. :-)
| $this->logError('Exception during EZproxy web service request: ' . $ex->getMessage()); | ||
| return null; | ||
| } | ||
| return '1' === $responseData; |
There was a problem hiding this comment.
Do you think it might be a good idea to trim $responseData? I tested this by putting a text file in VuFind's public directory and pointing the configuration at that -- I could then edit the file to change it to 0 or 1 to test both cases. However, my editor initially added line breaks, and 1\n is interpreted as "not 1." This caused me a little bit of confusion, and I imagine it's possible if somebody builds their own custom web service that the responses might end up having stray whitespace in them.
The current behavior of the ProxyUrl view helper is to proxy everything or nothing. However an institution's EZproxy server is often set up to selectively identify specific URLs that do not need to be proxied, i.e. local or open resources. This is problematic for two reasons:
This PR takes advantage of a separate, lightweight OSS tool from @joecorall that integrates with EZproxy, parses its config files, and exposes a web service that translates a URL (or domain) into a yes/no answer of whether it requires the proxy prefix. This PR integrates that web service into the existing ProxyUrl view helper, to prefix only when needed and avoid both the UnsafeRedirectUnknown security issues and all the RedirectSafe configuration setup & maintenance.
TODO