-
Notifications
You must be signed in to change notification settings - Fork 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
Interceptor security #386
Interceptor security #386
Conversation
Wow. Huge changeset and hard to review quickly for me as i currently dont have the time. But i like the idea/enhancements. Just one important thought on this: This may have a heavy performance impact! Especially when accessing all/many layers, iterating them and then accessing all sources (hibernate lazy loading i think)... Maybe we should have a sharp look and try to enhance such requests, measure the impact or tweak the SQL caching or similar. Just mentioning this as an idea/thought as i know that we already disabled the whole geoserver.action in a customer project due to it's performance impacts (in the customers case ~10 times slower than accessing the GeoServer directly) |
I know that this is a performance penalty, but you need to decided between security and speed here. It may of course be possible to save some information in memory about entity / permission for faster access. That would be an enhancement in a seperate PR. One can still switch to the old interceptor implementations or disable them, but with this PR the interface will be secured per default for those layer types |
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.
Awesome work @weskamm!
I noted some questions you may want to address, but others should chime in as well. A lot of sensitive code it is…
...un-core-main/src/main/java/de/terrestris/shoguncore/service/GeoServerInterceptorService.java
Show resolved
Hide resolved
...un-core-main/src/main/java/de/terrestris/shoguncore/service/GeoServerInterceptorService.java
Outdated
Show resolved
Hide resolved
public Response interceptGeoServerRequest(HttpServletRequest request, Optional<String> endpoint) | ||
public Response interceptGeoServerRequest( | ||
HttpServletRequest request, | ||
HashMap<String, Optional<String>> optionals) |
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.
Can we have the optionals
typed?
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.
not sure here, do you want me to instantiate the hashmap with null values as class variable?
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.
I was thinking of a simple class definition for optionals
that defines the structure of the parameter.
...in/src/main/java/de/terrestris/shoguncore/util/interceptor/secure/WfsRequestInterceptor.java
Show resolved
Hide resolved
...in/src/main/java/de/terrestris/shoguncore/util/interceptor/secure/WfsRequestInterceptor.java
Outdated
Show resolved
Hide resolved
...in/src/main/java/de/terrestris/shoguncore/util/interceptor/secure/WfsRequestInterceptor.java
Show resolved
Hide resolved
...in/src/main/java/de/terrestris/shoguncore/util/interceptor/secure/WmsRequestInterceptor.java
Show resolved
Hide resolved
...in/src/main/java/de/terrestris/shoguncore/util/interceptor/secure/WmsRequestInterceptor.java
Outdated
Show resolved
Hide resolved
...in/src/main/java/de/terrestris/shoguncore/util/interceptor/secure/WmsRequestInterceptor.java
Outdated
Show resolved
Hide resolved
…rvice/GeoServerInterceptorService.java Co-authored-by: Daniel Koch <[email protected]>
...in/src/main/java/de/terrestris/shoguncore/util/interceptor/secure/WfsRequestInterceptor.java
Show resolved
Hide resolved
Thanks for reviews. Merging now! |
This PR introduces enhanced and secured interceptor classes for
WMS
,WFS
andWMTS
.With these changes, layer security from shogun is respected when calling layers through the
geoserver.action
endpoint, so users may e.g. only see / getmap a layer when they have theREAD
permission.The
geoserver.action
endpoint is now capable of handling WMTS KVP and Restful requests. See below for example URLs.In the long term, the exisiting
wmts.action
should be removed, as itdoes not handle permissions at all andjust acts as a proxy.Capabilities and DescribeFeatureType Documents for the different service types are cleaned to only contain allowed layers and internal geoserver URLs get replaced b the
geoserver.action
interface.@terrestris/devs please review
Example URLs: