[Geoserver-devel] RestControllerAdvice eating all security exceptions (bon appetit!)

GeoServer GeoServer Developer
1

Hi,
I am looking into an issue where the catalog security throws a security exception, and the user gets back a 500 instead
of the intended 401.

After much fiddling I’ve found that the RestControllerAdvice in gs-rest acts as a catch-all, it advices all controllers, including
the OWS one, and has an exception handler method catching “Exception”:

https://github.com/geoserver/geoserver/blob/master/src/rest/src/main/java/org/geoserver/rest/RestControllerAdvice.java#L104

Now, my first reaction was that this advice should not be messing with the OWS controller… however, there is no way to map
an exclude, but only to give a set of classes (or base classes) that should be adviced… and that was my first attempt.
Problem, while most REST controllers extend RestBaseController, not all do, and there is no requirement to do so.
To just fix the build one would have to alter 3-4 controllers to extend RestBaseController but… in theory we’d have to go
and check each one of them.

So I went for a plan B, catch all security exceptions in a dedicated handling method, and rethrow them. Much smaller change,
appears to work. For reference, here:
https://github.com/geoserver/geoserver/pull/3476

However… there is still a catch. If the advice applies to all Spring dispatchers, including the OWS one, any exception going
out from there would be caught by RestControllerAdvice and reported to the REST callbacks. It’s unlikely but not impossible.

Hmm… comments? :slight_smile:

Also, a bit of a rant if I can, these approaches based on annotations look nice when coding them, but one never knows where
the side effects end up, while with XML hand wiring we get better control (see also the mapml module breaking REST completely).
I guess we should try to discourage using annotations in a project as large and complex as GeoServer?

Cheers
Andrea

···

GeoServer Professional Services from the experts! Visit http://goo.gl/it488V for more information. == Ing. Andrea Aime @geowolf Technical Lead GeoSolutions S.A.S. Via di Montramito 3/A 55054 Massarosa (LU) phone: +39 0584 962313 fax: +39 0584 1660272 mob: +39 339 8844549 http://www.geo-solutions.it http://twitter.com/geosolutions_it ------------------------------------------------------- Con riferimento alla normativa sul trattamento dei dati personali (Reg. UE 2016/679 - Regolamento generale sulla protezione dei dati “GDPR”), si precisa che ogni circostanza inerente alla presente email (il suo contenuto, gli eventuali allegati, etc.) è un dato la cui conoscenza è riservata al/i solo/i destinatario/i indicati dallo scrivente. Se il messaggio Le è giunto per errore, è tenuta/o a cancellarlo, ogni altra operazione è illecita. Le sarei comunque grato se potesse darmene notizia. This email is intended only for the person or entity to which it is addressed and may contain information that is privileged, confidential or otherwise protected from disclosure. We remind that - as provided by European Regulation 2016/679 “GDPR” - copying, dissemination or use of this e-mail or the information herein by anyone other than the intended recipient is prohibited. If you have received this email by mistake, please notify us immediately by telephone or e-mail.

2

I’ll move on with this one, fixes one problem and does not seem to introduce others

Cheers
Andrea

···

GeoServer Professional Services from the experts! Visit http://goo.gl/it488V for more information. == Ing. Andrea Aime @geowolf Technical Lead GeoSolutions S.A.S. Via di Montramito 3/A 55054 Massarosa (LU) phone: +39 0584 962313 fax: +39 0584 1660272 mob: +39 339 8844549 http://www.geo-solutions.it http://twitter.com/geosolutions_it ------------------------------------------------------- Con riferimento alla normativa sul trattamento dei dati personali (Reg. UE 2016/679 - Regolamento generale sulla protezione dei dati “GDPR”), si precisa che ogni circostanza inerente alla presente email (il suo contenuto, gli eventuali allegati, etc.) è un dato la cui conoscenza è riservata al/i solo/i destinatario/i indicati dallo scrivente. Se il messaggio Le è giunto per errore, è tenuta/o a cancellarlo, ogni altra operazione è illecita. Le sarei comunque grato se potesse darmene notizia. This email is intended only for the person or entity to which it is addressed and may contain information that is privileged, confidential or otherwise protected from disclosure. We remind that - as provided by European Regulation 2016/679 “GDPR” - copying, dissemination or use of this e-mail or the information herein by anyone other than the intended recipient is prohibited. If you have received this email by mistake, please notify us immediately by telephone or e-mail.

3

In fairness, it was actually the XML configuration that broke it (although it happened to be the XML configuration that specified allowing annotation-drive config). I'm not sure that the problem is really with where the configuration is, whether it is in an xml or a class annotation, the problem is that every module needs to be aware of what all other modules are doing so that they don't step on each other's toes. It is just a problem inherent to IoC that misbehaving modules can bring down the whole thing, particularly when there aren't enough controls in place. In this case, the whole Spring config door is wide open for a bad module author to abuse.

In my case, I didn't realize that I was sharing the same spring mvc configuration as all the other geoserver modules, and I also didn't realize the entire effect of the spring config directive(s) that I was using. Then it was all compounded by my erroneous commit to include the module by default.

I think the problem of every module needing to be aware of what all other modules are doing is improved somewhat by using a single XML config for module over annotations spread throughout the code because it is fewer places for other module authors to look. However, I think an even better solution is to reduce the need to be aware of what other modules are doing at all, with better encapsulation of each modules functions, and better documentation about what we do need to know about other modules. In this case it would be handy to know that that geoserver-rest module was basically replacing the entire spring MVC configuration system with its own mechanisms. Maybe a list somewhere in the geoserver docs of all extension points? Maybe I can contribute something to the docs about this, I see there is a placeholder there now.

Chris

On 5/6/2019 9:24 AM, Andrea Aime wrote:

Also, a bit of a rant if I can, these approaches based on annotations look nice when coding them, but one never knows where
the side effects end up, while with XML hand wiring we get better control (see also the mapml module breaking REST completely).
I guess we should try to discourage using annotations in a project as large and complex as GeoServer?

Cheers
Andrea

4

Hi Chris,
thinking about this, I agree in part. It’s true that it would be easier if everything was more encapsulated,
I think we just have no idea how to get there, or maybe sometimes we do, but it would take more time that
we have available.

I’m not totally sold on the “we need everything documented” though, the source code itself
is full of examples, I normally just go and see how things were done in other places in the code
and mimic what I see.

Maybe a starting point for docs would be just pointing out that plenty of examples can be found looking
in the source code (e.g., scan the application context files for usages of the same extension point,
look for the hierarchy of the extension point) and in tests.
An that GeoServer is an atypical Spring application, with lots of historical baggage,
which means one should not assume what works for application of recent writing should work for GeoServer,
and thus go and see what the existing and working code does instead.

Cheers
Andrea

···

Regards, Andrea Aime == GeoServer Professional Services from the experts! Visit http://goo.gl/it488V for more information. == Ing. Andrea Aime @geowolf Technical Lead GeoSolutions S.A.S. Via di Montramito 3/A 55054 Massarosa (LU) phone: +39 0584 962313 fax: +39 0584 1660272 mob: +39 339 8844549 http://www.geo-solutions.it http://twitter.com/geosolutions_it ------------------------------------------------------- Con riferimento alla normativa sul trattamento dei dati personali (Reg. UE 2016/679 - Regolamento generale sulla protezione dei dati “GDPR”), si precisa che ogni circostanza inerente alla presente email (il suo contenuto, gli eventuali allegati, etc.) è un dato la cui conoscenza è riservata al/i solo/i destinatario/i indicati dallo scrivente. Se il messaggio Le è giunto per errore, è tenuta/o a cancellarlo, ogni altra operazione è illecita. Le sarei comunque grato se potesse darmene notizia. This email is intended only for the person or entity to which it is addressed and may contain information that is privileged, confidential or otherwise protected from disclosure. We remind that - as provided by European Regulation 2016/679 “GDPR” - copying, dissemination or use of this e-mail or the information herein by anyone other than the intended recipient is prohibited. If you have received this email by mistake, please notify us immediately by telephone or e-mail.

5

In teaching recent developers workshop for GeoServer I indeed take this approach, pointing out “useful” examples after doing my best to explain what parts are in play.

···

–
Jody Garnett

6

I agree, getting caught up in ideals and “perfection” prevents you from moving forward with basic function.

Also I know the source is really the ultimate developer documentation, and any attempt at developer docs always gets stale. But as the code gets bigger, it gets harder to find relevant examples. I went through all of the other community modules and none of them were doing something like what I was trying to do for MapML.

The pointers to examples is exactly what I’m going for with the idea of listing the extension points - if you at least have those names, you can look for implementations, and you have a nice entry point into the code. I was already aware of the output format extension points so those were relatively easy to find and mimic. When I saw that the REST module was using Spring MVC, and I’ve recently been building Spring MVC REST APIs, I thought that’s all I needed to know to be off and running. Apparently I know just enough Spring to be dangerous :wink:

Anyways, just wondering if there is a way to turn my recent experience with Geoserver development (as someone who has typically been more a user of Geoserver as well as a GIS-oriented Java developer) into something useful for those who follow (docs or otherwise).

Chris

···

On 5/9/2019 6:11 AM, Andrea Aime wrote:

Hi Chris,
thinking about this, I agree in part. It’s true that it would be easier if everything was more encapsulated,
I think we just have no idea how to get there, or maybe sometimes we do, but it would take more time that
we have available.

I’m not totally sold on the “we need everything documented” though, the source code itself
is full of examples, I normally just go and see how things were done in other places in the code
and mimic what I see.

Maybe a starting point for docs would be just pointing out that plenty of examples can be found looking
in the source code (e.g., scan the application context files for usages of the same extension point,
look for the hierarchy of the extension point) and in tests.
An that GeoServer is an atypical Spring application, with lots of historical baggage,
which means one should not assume what works for application of recent writing should work for GeoServer,
and thus go and see what the existing and working code does instead.

Cheers
Andrea

On Wed, May 8, 2019 at 8:03 PM Chris Hodgson <chodgson@anonymised.com> wrote:

In fairness, it was actually the XML configuration that broke it
(although it happened to be the XML configuration that specified
allowing annotation-drive config). I’m not sure that the problem is
really with where the configuration is, whether it is in an xml or a
class annotation, the problem is that every module needs to be aware of
what all other modules are doing so that they don’t step on each other’s
toes. It is just a problem inherent to IoC that misbehaving modules can
bring down the whole thing, particularly when there aren’t enough
controls in place. In this case, the whole Spring config door is wide
open for a bad module author to abuse.

In my case, I didn’t realize that I was sharing the same spring mvc
configuration as all the other geoserver modules, and I also didn’t
realize the entire effect of the spring config directive(s) that I was
using. Then it was all compounded by my erroneous commit to include the
module by default.

I think the problem of every module needing to be aware of what all
other modules are doing is improved somewhat by using a single XML
config for module over annotations spread throughout the code because it
is fewer places for other module authors to look. However, I think an
even better solution is to reduce the need to be aware of what other
modules are doing at all, with better encapsulation of each modules
functions, and better documentation about what we do need to know about
other modules. In this case it would be handy to know that that
geoserver-rest module was basically replacing the entire spring MVC
configuration system with its own mechanisms. Maybe a list somewhere in
the geoserver docs of all extension points? Maybe I can contribute
something to the docs about this, I see there is a placeholder there now.

Chris

On 5/6/2019 9:24 AM, Andrea Aime wrote:

Also, a bit of a rant if I can, these approaches based on annotations
look nice when coding them, but one never knows where
the side effects end up, while with XML hand wiring we get better
control (see also the mapml module breaking REST completely).
I guess we should try to discourage using annotations in a project as
large and complex as GeoServer?

Cheers
Andrea

Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

–

Regards, Andrea Aime == GeoServer Professional Services from the experts! Visit http://goo.gl/it488V for more information. == Ing. Andrea Aime @geowolf Technical Lead GeoSolutions S.A.S. Via di Montramito 3/A 55054 Massarosa (LU) phone: +39 0584 962313 fax: +39 0584 1660272 mob: +39 339 8844549 http://www.geo-solutions.it http://twitter.com/geosolutions_it ------------------------------------------------------- Con riferimento alla normativa sul trattamento dei dati personali (Reg. UE 2016/679 - Regolamento generale sulla protezione dei dati “GDPR”), si precisa che ogni circostanza inerente alla presente email (il suo contenuto, gli eventuali allegati, etc.) è un dato la cui conoscenza è riservata al/i solo/i destinatario/i indicati dallo scrivente. Se il messaggio Le è giunto per errore, è tenuta/o a cancellarlo, ogni altra operazione è illecita. Le sarei comunque grato se potesse darmene notizia. This email is intended only for the person or entity to which it is addressed and may contain information that is privileged, confidential or otherwise protected from disclosure. We remind that - as provided by European Regulation 2016/679 “GDPR” - copying, dissemination or use of this e-mail or the information herein by anyone other than the intended recipient is prohibited. If you have received this email by mistake, please notify us immediately by telephone or e-mail.

-- 
Chris Hodgson
Refractions Research
Suite 419 – 1207 Douglas Street
Victoria, British Columbia
Canada, V8W 2E7
1-250-383-3022
[http://www.refractions.net](http://www.refractions.net)
7

Can you assemble the good examples you found in to a page for the developers guide?

···

–
Jody Garnett

8

I agree, getting caught up in ideals and “perfection” prevents you from moving forward with basic function.

Also I know the source is really the ultimate developer documentation, and any attempt at developer docs always gets stale. But as the code gets bigger, it gets harder to find relevant examples. I went through all of the other community modules and none of them were doing something like what I was trying to do for MapML.

Rule of thumb: never look in community modules unless you have to. They are un-maintained, have never been checked by a larger group, can be stale, if they have been around for a while they will likely not pass their tests, they are not subject to QA checks either (e.g. spotbugs, errorprone, pmd).

Check core modules and extensions instead, they have a better chance of containing suitable example (they are not gold plated mind, but the average supported module is in better shape than the average community module, exceptions might exist in both directions).

The pointers to examples is exactly what I’m going for with the idea of listing the extension points - if you at least have those names, you can look for implementations, and you have a nice entry point into the code. I was already aware of the output format extension points so those were relatively easy to find and mimic. When I saw that the REST module was using Spring MVC, and I’ve recently been building Spring MVC REST APIs, I thought that’s all I needed to know to be off and running. Apparently I know just enough Spring to be dangerous :wink:

See above about not assuming GeoServer behaves like a recent/small app. If you need to extend the REST API gs-restconfig is your “bible”, there are a number of other extension modules contributing to it too.

Anyways, just wondering if there is a way to turn my recent experience with Geoserver development (as someone who has typically been more a user of Geoserver as well as a GIS-oriented Java developer) into something useful for those who follow (docs or otherwise).

Minimal docs listing extension points and significant usage points are likely useful.
Or even just a summary of these mails, showing people how to gather the information they need.
Other things that could be useful in terms of information gathering are significant breakpoints to add in a debug session to learn how the code works step by step (I personally do that a lot)

Cheers
Andrea

···

GeoServer Professional Services from the experts! Visit http://goo.gl/it488V for more information. == Ing. Andrea Aime @geowolf Technical Lead GeoSolutions S.A.S. Via di Montramito 3/A 55054 Massarosa (LU) phone: +39 0584 962313 fax: +39 0584 1660272 mob: +39 339 8844549 http://www.geo-solutions.it http://twitter.com/geosolutions_it ------------------------------------------------------- Con riferimento alla normativa sul trattamento dei dati personali (Reg. UE 2016/679 - Regolamento generale sulla protezione dei dati “GDPR”), si precisa che ogni circostanza inerente alla presente email (il suo contenuto, gli eventuali allegati, etc.) è un dato la cui conoscenza è riservata al/i solo/i destinatario/i indicati dallo scrivente. Se il messaggio Le è giunto per errore, è tenuta/o a cancellarlo, ogni altra operazione è illecita. Le sarei comunque grato se potesse darmene notizia. This email is intended only for the person or entity to which it is addressed and may contain information that is privileged, confidential or otherwise protected from disclosure. We remind that - as provided by European Regulation 2016/679 “GDPR” - copying, dissemination or use of this e-mail or the information herein by anyone other than the intended recipient is prohibited. If you have received this email by mistake, please notify us immediately by telephone or e-mail.