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

[GEOS-8240] Support X-Forwarded-Proto header in ResponseUtils.baseURL #2955

Merged
merged 11 commits into from Jul 3, 2018

Conversation

Projects
None yet
3 participants
@lmoran
Contributor

lmoran commented Jun 30, 2018

Use of request headers in composing the Proxy URL, (see GEOS-8240). NOTE: one validator in GlobalSettingsPage.java ha to be dropped.

lmoran added some commits Jun 28, 2018

[GEOS-8240] Support X-Forwarded-Proto header in ResponseUtils.baseURL
- Added useHeadersProxyURL parameter to the global settings page
- Refactored ProxifyingURLMangler.java to follow use the Proxy Base URL
  only when the useHeadersProxyUERL is not set
[GEOS-8240] Support X-Forwarded-Proto header in ResponseUtils.baseURL
- Added template expansion with QuickTemplate
- Added headers of interest to the template
- Added more than one template in the smae base proxy URL
[GEOS-8240] Support X-Forwarded-Proto header in ResponseUtils.baseURL
- Adde case-insensitive header names template expansion
[GEOS-8240] Support X-Forwarded-Proto header in ResponseUtils.baseURL
- Added a check against base proxy URL being null
- Dropped base proxy URL validator in the gloabl settings page
  (template expressions do not pass the URL validator, of course)
[GEOS-8240] Support X-Forwarded-Proto header in ResponseUtils.baseURL
- Used enum in lieu of static members to hold header names
[GEOS-8240] Support X-Forwarded-Proto header in ResponseUtils.baseURL
- Used enum in lieu of static members to hold header names

@bencaradocdavies bencaradocdavies changed the title from GEOS-8240 to [GEOS-8240] Support X-Forwarded-Proto header in ResponseUtils.baseURL Jun 30, 2018

@@ -0,0 +1,47 @@
/* (c) 2017 Open Source Geospatial Foundation - all rights reserved

This comment has been minimized.

@bencaradocdavies

bencaradocdavies Jun 30, 2018

Member

If this is a new file, it needs the correct date in the copyright header. @lmoran I think you have signed a contributor agreement; please confirm.

This comment has been minimized.

@aaime

aaime Jul 1, 2018

Member

It's a copy of one class I made for the OpenSearch for EO.

This comment has been minimized.

@bencaradocdavies

bencaradocdavies Jul 1, 2018

Member

If the class is a copy from elsewhere in the codebase, then it best to leave the copyright date and author tag the same.

* string. No escaping, no extras, but avoids building lots of strings to do its work and the
* overhead of template instantiation of a true template engine.
*
* @author Andrea Aime - GeoSolutions

This comment has been minimized.

@bencaradocdavies

bencaradocdavies Jun 30, 2018

Member

@lmoran if this is a new file, please check the author tag. Was this copied from elsewhere?

This comment has been minimized.

@lmoran

lmoran Jul 3, 2018

Contributor

Yes, copied and, later, adapted.

// Cycles through templates in the proxy base URL until one is competely matched
Map<String, String> headers = this.compileHeadersMap();
for (String template : Arrays.asList(proxyBase.split(TEMPLATE_SEPARATOR))) {

This comment has been minimized.

@bencaradocdavies

bencaradocdavies Jun 30, 2018

Member

Did you consider using a Pattern to implement template expansion? See InterpolationProperties in gs-app-schema.

Note to GeoTools developers: we should really move InterpolationProperties to gt-metadata for wider use as this is the second thing I have seen this year that could have used it.

This comment has been minimized.

@aaime

aaime Jul 1, 2018

Member

The implementation was meant, as described in the javadoc, to be as fast as possible (Freemarker was too slow for the job, given that we potentially have a different template for each record). I did not know about InterpolationProperties, performance should be comparable (relative to large Freemarker init time for new templates, at the very least).

This comment has been minimized.

@lmoran

lmoran Jul 3, 2018

Contributor

IIUC InterpolationProperties is case-sensitive in its substitutions, which clashes with the case-in-sensitiveness of HTTP headers: what about adding this option to it?

if (grayChannel != null)
this.grayChannelName = grayChannel.getChannelName().evaluate(null, String.class);
/* XXX FIXME: to savoid a compilation error

This comment has been minimized.

@bencaradocdavies

bencaradocdavies Jun 30, 2018

Member

savoid -> avoid. And is this something that has been fixed or something to be fixed? If required for compilation, perhaps just remove?

This comment has been minimized.

@lmoran

lmoran Jul 3, 2018

Contributor

My bad: his shouldn't have been added to the commit, as it is an unrelated issue. I will return it to its original state in a another commit.

@@ -0,0 +1,270 @@
/* (c) 2014 Open Source Geospatial Foundation - all rights reserved

This comment has been minimized.

@bencaradocdavies

bencaradocdavies Jun 30, 2018

Member

Incorrect header date in this new file.

lmoran added some commits Jul 3, 2018

[GEOS-8240] Support X-Forwarded-Proto header in ResponseUtils.baseURL
- Added "Forwarded" header the templates
- For simmmetry with the "Forwrded-Path" header, a non-standard
  "path=<path value" header value component  is supprtorted.
  For example, this template
  ${Forwarded.proto}://${Forwarded.host}/${Forwarded.path}/geoserver
  can be expanded with this header
  Forwarded: proto=http; host=example.com:8080; path=public
  to
  http://example.com:8080/public/geoserver
[GEOS-8240] Support X-Forwarded-Proto header in ResponseUtils.baseURL
- Corrected copyright years in headers
- Reverted ColorMapLegendCreatore.java to its original state
[GEOS-8240] Support X-Forwarded-Proto header in ResponseUtils.baseURL
- Reverted ColorMapLegendCreatore.java to its original state

Requested changes and clarifications made.

@bencaradocdavies bencaradocdavies merged commit 3c05cfb into geoserver:master Jul 3, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bencaradocdavies

This comment has been minimized.

Member

bencaradocdavies commented Jul 3, 2018

Thanks @lmoran.

@aaime

This comment has been minimized.

Member

aaime commented Jul 4, 2018

Ouch, was this tested interactively? http://cloudsdi.geo-solutions.it/geoserver/ is completely bricked by this change (we now deployed a build from yesterday to restore service), capabilities documents, preview page, services building backlinks, they all fail with a stack trace like:

{code}
Caused by: java.lang.NullPointerException
at org.geoserver.ows.ProxifyingURLMangler.mangleURL(ProxifyingURLMangler.java:93)
at org.geoserver.ows.util.ResponseUtils.buildURL(ResponseUtils.java:372)
at org.geoserver.ows.util.ResponseUtils.buildSchemaURL(ResponseUtils.java:403)
at org.geoserver.wms.WMSServiceExceptionHandler.handleXmlException(WMSServiceExceptionHandler.java:321)
at org.geoserver.wms.WMSServiceExceptionHandler.handleServiceException(WMSServiceExceptionHandler.java:124)
at org.geoserver.ows.Dispatcher.handleServiceException(Dispatcher.java:1799)
{code}

// (for two reasons: a) speed; b) to make the admin aware of
// possible security liabilities)
baseURL =
(this.geoServer.getGlobal().isUseHeadersProxyURL() == true && proxyBase != null)

This comment has been minimized.

@aaime

aaime Jul 4, 2018

Member

Believe the NPE is here, isUseHeadersProxyURL() return a Boolean, which can be null

This comment has been minimized.

@lmoran

lmoran Jul 4, 2018

Contributor

Yes, I tested it interactively, but not the on the latest GeoTools.
Is this.geoServer guaranteed to be non-null?

This comment has been minimized.

@aaime

aaime Jul 4, 2018

Member

Yes, the geoserver field is always not null, this class is instantiated by Spring

This comment has been minimized.

@bencaradocdavies

bencaradocdavies Jul 4, 2018

Member

The NPE has also broken all CITE tests with java.lang.NullPointerException at org.geoserver.ows.ProxifyingURLMangler.mangleURL(ProxifyingURLMangler.java:93)
https://build.geoserver.org/job/cite-wms-1.3/1735/consoleFull

@@ -40,6 +40,8 @@
protected Boolean globalServices = true;
protected Boolean useHeadersProxyURL = false;

This comment has been minimized.

@aaime

aaime Jul 4, 2018

Member

This is guaranteed to be null when XStream deserializes from XML, if the element is missing, as it won't go through normal class construction (so forget about constructors and field initializers, that are also invoked as part of normal constructor invocation).

This comment has been minimized.

@lmoran

lmoran Jul 4, 2018

Contributor

OK, I will solve this tomorrow.

This comment has been minimized.

@lmoran

lmoran Jul 5, 2018

Contributor

Hopefully this is solved: please, refer to this other PR: #2968

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment