Skip to content
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

WMS GetMap HEAD request error response - check response status defined #8023

Merged
merged 3 commits into from
May 21, 2024

Conversation

josegar74
Copy link
Member

If CORS is not allowed for WMS GetMap HEAD request, the error response doesn't contain the status property. Protect the related check.

Follow up of #7000

Checklist

  • I have read the contribution guidelines
  • Pull request provided for main branch, backports managed with label
  • Good housekeeping of code, cleaning up comments, tests, and documentation
  • Clean commit history broken into understandable chucks, avoiding big commits with hundreds of files, cautious of reformatting and whitespace changes
  • Clean commit messages, longer verbose messages are encouraged
  • API Changes are identified in commit messages
  • Testing provided for features or enhancements using automatic tests
  • User documentation provided for new features or enhancements in manual
  • Build documentation provided for development instructions in README.md files
  • Library management using pom.xml dependency management. Update build documentation with intended library use and library tutorials or documentation

@josegar74 josegar74 requested a review from fxprunayre May 8, 2024 13:59
@josegar74 josegar74 added this to the 4.4.5 milestone May 8, 2024
@@ -900,7 +900,7 @@
imageTile.getImage().src = src;
},
function (r) {
if (r.status === 414) {
if (r.status && r.status === 414) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does that new check achieve? It seems superfluous

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If CORS is not enabled, the response object has no status

image

It looks like some servers add CORS header on GET and not on HEAD. Also depending on Apache config, if there is an error, then CORS header are not always present (depends on virtual host config and the set header directive).

The CORS interceptor does not make the difference between operation so it will start using the proxy for GET if an HEAD request fails (and then GET request may also be not allowed causing subsequent issues).

Copy link
Member Author

@josegar74 josegar74 May 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jahow without that check, when the response has no status (as described previously by @fxprunayre ), it fails accessing r.status, what causes the CORS interceptor to add the server in the list to use the http proxy when accessing any url from that server.

So if you are unlogged and try to add a layer from that server in the map viewer (Add layers > Services > WMS), you get the following error:

map-error

Due to the http proxy default policy to require the service to be defined in the metadata and analysed with the link analysis tool, for unlogged users. The service seems working fine with CORS for GET operations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No but I meant, if r.status equals 414 then it has to be truthy; I just don't get the r.status && part

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jahow see the commment from @fxprunayre in his reply:

If CORS is not enabled, the response object has no status

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand but:

  • if r.status is undefined then r.status === 414 will return false
  • if r.status is undefined then r.status && r.status === 414 will return undefined
  • if r.status is 414 then r.status === 414 will return true
  • if r.status is 414 then r.status && r.status === 414 will also return true
  • if r.status is 200 then r.status === 414 will return false
  • if r.status is 200 then r.status && r.status === 414 will also return false

I'm just not sure this change is going to do anything at all?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jahow you are totally right, apologies. I was testing several options and I seems I got confused with the one that worked.

I just updated the code to avoid the CORS interceptor for the HEAD request, that is working fine. Doesn't seem optimal, but at least looks working fine.

If the map service doesn't support HEAD (or CORS for HEAD requests), all other requests to the server go through the http proxy, even if the server supports CORS for these requests
@fxprunayre
Copy link
Member

As this service is supporting CORS for GET but not for HEAD, an other option is to add the method to the requireProxy

image

The proxy may still be useful in some case for HEAD requests?

For the other case, when a service is not providing CORS header on HTTP errors, then it is probably more difficult and has to be properly configured on the WMS side.

@josegar74
Copy link
Member Author

josegar74 commented May 13, 2024

@fxprunayre what should contain requireProxy? The method name (HEAD) or the URL+Method?

Some servers provides CORS header for some method eg. GET and not for other eg. HEAD.
In such case, if a failure occurs on a HEAD request, then the proxy is activated for that domain but is not needed for GET request. If user is anonymous, then proxy forbid access to the domain (if the domain is not in the link analysis table with default configuration) and simple action like `GetCapabilities` will fail.

Add the method to the domain as key to know if the proxy has to be used or not.

Can be tested with https://haleconnect.com/ows/services/org.789.b36c0053-db6b-4fe7-9402-4e5d8ac35e06_wms
@fxprunayre fxprunayre merged commit e25c722 into geonetwork:main May 21, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants