[Geoserver-devel] Odd function used by AbstractResourceAccessManager slowing down jdbcconfig a lot

Hi,
the AbstractResourceAccessManager implements the security filter method is
a most unusual way:

@Override
public Filter getSecurityFilter(final Authentication user,
final Class<? extends CatalogInfo> clazz) {

org.opengis.filter.expression.Function visible = new InternalVolatileFunction() {
@Override
public Boolean evaluate(Object object) {
CatalogInfo info = (CatalogInfo) object;
if(info instanceof NamespaceInfo) {
info = getCatalog().getWorkspaceByName(((NamespaceInfo) info).getPrefix());
}
String name = (String) OwsUtils.property(info, “name”, String.class);
WrapperPolicy policy = getSecurityWrapper()
.buildWrapperPolicy(AbstractResourceAccessManager.this, user, info);
AccessLevel accessLevel = policy.getAccessLevel();
boolean visible = !AccessLevel.HIDDEN.equals(accessLevel);
return Boolean.valueOf(visible);
}
};

FilterFactory factory = Predicates.factory;

// create a filter combined with the security credentials check
Filter filter = factory.equals(factory.literal(Boolean.TRUE), visible);

return filter;
}

This filter is forcing JDBCConfig to a full scan, because it’s not supported encoding wise.
Which kind of defeats the purpose of the getSecurityFilter method, which was to
do a first pass and quickly remove everything that the security manage knows
is not needed, to then have the secure catalog check one by one the object
returned in memory later.

Instead this function is basically breaking the relationship between the access
manager and resource catalog making this function call back to the secure
catalog for each and every item in the configuration.

Why was it done this way? Are we sure this is really necessary?
This work was part of Kevin’s commit here:

https://github.com/geoserver/geoserver/commit/887027a07aab01ef12fd9b0f7fe699c3d1bd46e8

Cheers
Andrea

···

==

GeoServer Professional Services from the experts! Visit
http://goo.gl/NWWaa2 for more information.

==

Ing. Andrea Aime

@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

AVVERTENZE AI SENSI DEL D.Lgs. 196/2003

Le informazioni contenute in questo messaggio di posta elettronica e/o nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il loro utilizzo è consentito esclusivamente al destinatario del messaggio, per le finalità indicate nel messaggio stesso. Qualora riceviate questo messaggio senza esserne il destinatario, Vi preghiamo cortesemente di darcene notizia via e-mail e di procedere alla distruzione del messaggio stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso, divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od utilizzarlo per finalità diverse, costituisce comportamento contrario ai principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for the attention and use of the named addressee(s) and may be confidential or proprietary in nature or covered by the provisions of privacy act (Legislative Decree June, 30 2003, no.196 - Italy’s New Data Protection Code).Any use not in accord with its purpose, any disclosure, reproduction, copying, distribution, or either dissemination, either whole or partial, is strictly forbidden except previous formal approval of the named addressee(s). If you are not the intended recipient, please contact immediately the sender by telephone, fax or e-mail and delete the information in this message that has been received in error. The sender does not give any warranty or accept liability as the content, accuracy or completeness of sent messages and accepts no responsibility for changes made after they were sent or for other risks which arise as a result of e-mail transmission, viruses, etc.


Kevin will have to comment with more details but from what I can remember about his explanation it was that the idea was to implement it in such a way so that it could eventually be efficiently encoded by jdbcconfig, although he didn’t have time to actually do that. Hopefully he has more useful insight.

···

On Thu, Nov 20, 2014 at 4:48 AM, Andrea Aime <andrea.aime@anonymised.com> wrote:

Hi,
the AbstractResourceAccessManager implements the security filter method is
a most unusual way:

@Override
public Filter getSecurityFilter(final Authentication user,
final Class<? extends CatalogInfo> clazz) {

org.opengis.filter.expression.Function visible = new InternalVolatileFunction() {
@Override
public Boolean evaluate(Object object) {
CatalogInfo info = (CatalogInfo) object;
if(info instanceof NamespaceInfo) {
info = getCatalog().getWorkspaceByName(((NamespaceInfo) info).getPrefix());
}
String name = (String) OwsUtils.property(info, “name”, String.class);
WrapperPolicy policy = getSecurityWrapper()
.buildWrapperPolicy(AbstractResourceAccessManager.this, user, info);
AccessLevel accessLevel = policy.getAccessLevel();
boolean visible = !AccessLevel.HIDDEN.equals(accessLevel);
return Boolean.valueOf(visible);
}
};

FilterFactory factory = Predicates.factory;

// create a filter combined with the security credentials check
Filter filter = factory.equals(factory.literal(Boolean.TRUE), visible);

return filter;
}

This filter is forcing JDBCConfig to a full scan, because it’s not supported encoding wise.
Which kind of defeats the purpose of the getSecurityFilter method, which was to
do a first pass and quickly remove everything that the security manage knows
is not needed, to then have the secure catalog check one by one the object
returned in memory later.

Instead this function is basically breaking the relationship between the access
manager and resource catalog making this function call back to the secure
catalog for each and every item in the configuration.

Why was it done this way? Are we sure this is really necessary?
This work was part of Kevin’s commit here:

https://github.com/geoserver/geoserver/commit/887027a07aab01ef12fd9b0f7fe699c3d1bd46e8

Cheers
Andrea

==

GeoServer Professional Services from the experts! Visit
http://goo.gl/NWWaa2 for more information.

==

Ing. Andrea Aime

@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

AVVERTENZE AI SENSI DEL D.Lgs. 196/2003

Le informazioni contenute in questo messaggio di posta elettronica e/o nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il loro utilizzo è consentito esclusivamente al destinatario del messaggio, per le finalità indicate nel messaggio stesso. Qualora riceviate questo messaggio senza esserne il destinatario, Vi preghiamo cortesemente di darcene notizia via e-mail e di procedere alla distruzione del messaggio stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso, divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od utilizzarlo per finalità diverse, costituisce comportamento contrario ai principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for the attention and use of the named addressee(s) and may be confidential or proprietary in nature or covered by the provisions of privacy act (Legislative Decree June, 30 2003, no.196 - Italy’s New Data Protection Code).Any use not in accord with its purpose, any disclosure, reproduction, copying, distribution, or either dissemination, either whole or partial, is strictly forbidden except previous formal approval of the named addressee(s). If you are not the intended recipient, please contact immediately the sender by telephone, fax or e-mail and delete the information in this message that has been received in error. The sender does not give any warranty or accept liability as the content, accuracy or completeness of sent messages and accepts no responsibility for changes made after they were sent or for other risks which arise as a result of e-mail transmission, viruses, etc.



Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk


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

Justin Deoliveira
VP Engineering | Boundless
jdeolive@anonymised.com
@boundlessgeo

Yes, the idea is that subclasses should override this default implementation with something that’s properly encoded where possible (Sometimes it’s impossible). I wasn’t able to provide such as many implementations as I intended as I ran out of time.

There are also some parts of the UI like the layer preview page that need to be re-written to make use of the filter based API rather than filtering things themselves. I didn’t have time to get to that either.

···

On 24 November 2014 at 07:54, Justin Deoliveira <jdeolive@anonymised.com> wrote:

Kevin will have to comment with more details but from what I can remember about his explanation it was that the idea was to implement it in such a way so that it could eventually be efficiently encoded by jdbcconfig, although he didn’t have time to actually do that. Hopefully he has more useful insight.

On Thu, Nov 20, 2014 at 4:48 AM, Andrea Aime <andrea.aime@…1268…> wrote:

Hi,
the AbstractResourceAccessManager implements the security filter method is
a most unusual way:

@Override
public Filter getSecurityFilter(final Authentication user,
final Class<? extends CatalogInfo> clazz) {

org.opengis.filter.expression.Function visible = new InternalVolatileFunction() {
@Override
public Boolean evaluate(Object object) {
CatalogInfo info = (CatalogInfo) object;
if(info instanceof NamespaceInfo) {
info = getCatalog().getWorkspaceByName(((NamespaceInfo) info).getPrefix());
}
String name = (String) OwsUtils.property(info, “name”, String.class);
WrapperPolicy policy = getSecurityWrapper()
.buildWrapperPolicy(AbstractResourceAccessManager.this, user, info);
AccessLevel accessLevel = policy.getAccessLevel();
boolean visible = !AccessLevel.HIDDEN.equals(accessLevel);
return Boolean.valueOf(visible);
}
};

FilterFactory factory = Predicates.factory;

// create a filter combined with the security credentials check
Filter filter = factory.equals(factory.literal(Boolean.TRUE), visible);

return filter;
}

This filter is forcing JDBCConfig to a full scan, because it’s not supported encoding wise.
Which kind of defeats the purpose of the getSecurityFilter method, which was to
do a first pass and quickly remove everything that the security manage knows
is not needed, to then have the secure catalog check one by one the object
returned in memory later.

Instead this function is basically breaking the relationship between the access
manager and resource catalog making this function call back to the secure
catalog for each and every item in the configuration.

Why was it done this way? Are we sure this is really necessary?
This work was part of Kevin’s commit here:

https://github.com/geoserver/geoserver/commit/887027a07aab01ef12fd9b0f7fe699c3d1bd46e8

Cheers
Andrea

==

GeoServer Professional Services from the experts! Visit
http://goo.gl/NWWaa2 for more information.

==

Ing. Andrea Aime

@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

AVVERTENZE AI SENSI DEL D.Lgs. 196/2003

Le informazioni contenute in questo messaggio di posta elettronica e/o nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il loro utilizzo è consentito esclusivamente al destinatario del messaggio, per le finalità indicate nel messaggio stesso. Qualora riceviate questo messaggio senza esserne il destinatario, Vi preghiamo cortesemente di darcene notizia via e-mail e di procedere alla distruzione del messaggio stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso, divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od utilizzarlo per finalità diverse, costituisce comportamento contrario ai principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for the attention and use of the named addressee(s) and may be confidential or proprietary in nature or covered by the provisions of privacy act (Legislative Decree June, 30 2003, no.196 - Italy’s New Data Protection Code).Any use not in accord with its purpose, any disclosure, reproduction, copying, distribution, or either dissemination, either whole or partial, is strictly forbidden except previous formal approval of the named addressee(s). If you are not the intended recipient, please contact immediately the sender by telephone, fax or e-mail and delete the information in this message that has been received in error. The sender does not give any warranty or accept liability as the content, accuracy or completeness of sent messages and accepts no responsibility for changes made after they were sent or for other risks which arise as a result of e-mail transmission, viruses, etc.



Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server
from Actuate! Instantly Supercharge Your Business Reports and Dashboards
with Interactivity, Sharing, Native Excel Exports, App Integration & more
Get technology previously reserved for billion-dollar corporations, FREE
http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk


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

Justin Deoliveira
VP Engineering | Boundless
jdeolive@anonymised.com9…
@boundlessgeo

Kevin Smith

Junior Software Engineer | Boundless

ksmith@anonymised.com

+1-778-785-7459

@boundlessgeo

http://boundlessgeo.com/

On Tue, Nov 25, 2014 at 9:32 PM, Kevin Smith <ksmith@anonymised.com>
wrote:

Yes, the idea is that subclasses should override this default
implementation with something that's properly encoded where possible
(Sometimes it's impossible). I wasn't able to provide such as many
implementations as I intended as I ran out of time.

I can see that, but see the main point of my first mail: it seems to me
this filter is not necessary, we got a great speedup by just replacing it
with Filter.INCLUDE, and leaving the
secure catalog to do the filtering in memory (which is done right after
calling this method).

So I'm asking, can't we just kill it? For the in memory catalog it seems to
do no useful work, and for JDBCConfig it's actually dragging down
performance.
Unless I'm missing something of course.

There are also some parts of the UI like the layer preview page that need
to be re-written to make use of the filter based API rather than filtering
things themselves. I didn't have time to get to that either.

We're working on it

Cheers
Andrea

--

GeoServer Professional Services from the experts! Visit
http://goo.gl/NWWaa2 for more information.

Ing. Andrea Aime
@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

*AVVERTENZE AI SENSI DEL D.Lgs. 196/2003*

Le informazioni contenute in questo messaggio di posta elettronica e/o
nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il
loro utilizzo è consentito esclusivamente al destinatario del messaggio,
per le finalità indicate nel messaggio stesso. Qualora riceviate questo
messaggio senza esserne il destinatario, Vi preghiamo cortesemente di
darcene notizia via e-mail e di procedere alla distruzione del messaggio
stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso,
divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od
utilizzarlo per finalità diverse, costituisce comportamento contrario ai
principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for
the attention and use of the named addressee(s) and may be confidential or
proprietary in nature or covered by the provisions of privacy act
(Legislative Decree June, 30 2003, no.196 - Italy's New Data Protection
Code).Any use not in accord with its purpose, any disclosure, reproduction,
copying, distribution, or either dissemination, either whole or partial, is
strictly forbidden except previous formal approval of the named
addressee(s). If you are not the intended recipient, please contact
immediately the sender by telephone, fax or e-mail and delete the
information in this message that has been received in error. The sender
does not give any warranty or accept liability as the content, accuracy or
completeness of sent messages and accepts no responsibility for changes
made after they were sent or for other risks which arise as a result of
e-mail transmission, viruses, etc.

-------------------------------------------------------

On 25 November 2014 at 12:36, Andrea Aime <andrea.aime@anonymised.com>
wrote:

On Tue, Nov 25, 2014 at 9:32 PM, Kevin Smith <ksmith@anonymised.com>
wrote:

Yes, the idea is that subclasses should override this default
implementation with something that's properly encoded where possible
(Sometimes it's impossible). I wasn't able to provide such as many
implementations as I intended as I ran out of time.

I can see that, but see the main point of my first mail: it seems to me
this filter is not necessary, we got a great speedup by just replacing it
with Filter.INCLUDE, and leaving the
secure catalog to do the filtering in memory (which is done right after
calling this method).

So I'm asking, can't we just kill it? For the in memory catalog it seems
to do no useful work, and for JDBCConfig it's actually dragging down
performance.
Unless I'm missing something of course.

The filter that was being produced before did the same thing.
https://github.com/geoserver/geoserver/commit/887027a07aab01ef12fd9b0f7fe699c3d1bd46e8#diff-a9283370e69a44d55dfac0862028f589L1452

I tried to unravel buildPolicyWrapper to figure out if I could avoid this
but I couldn't convince myself that I could do without it without breaking
anything and so I gave up and just moved the whole thing into the default
implementation in AbstractResourceAccessManager.

There are also some parts of the UI like the layer preview page that need
to be re-written to make use of the filter based API rather than filtering
things themselves. I didn't have time to get to that either.

We're working on it

Cool.

--

Kevin Smith

Junior Software Engineer | Boundless <http://boundlessgeo.com/&gt;

ksmith@anonymised.com

+1-778-785-7459

@boundlessgeo <http://twitter.com/boundlessgeo/&gt;

<http://twitter.com/boundlessgeo/&gt;

[image: http://boundlessgeo.com/\]
<http://boundlessgeo.com/&gt;

On Wed, Nov 26, 2014 at 1:24 AM, Kevin Smith <ksmith@anonymised.com>
wrote:

I can see that, but see the main point of my first mail: it seems to me

this filter is not necessary, we got a great speedup by just replacing it
with Filter.INCLUDE, and leaving the
secure catalog to do the filtering in memory (which is done right after
calling this method).

So I'm asking, can't we just kill it? For the in memory catalog it seems
to do no useful work, and for JDBCConfig it's actually dragging down
performance.
Unless I'm missing something of course.

The filter that was being produced before did the same thing.
https://github.com/geoserver/geoserver/commit/887027a07aab01ef12fd9b0f7fe699c3d1bd46e8#diff-a9283370e69a44d55dfac0862028f589L1452

I tried to unravel buildPolicyWrapper to figure out if I could avoid this
but I couldn't convince myself that I could do without it without breaking
anything and so I gave up and just moved the whole thing into the default
implementation in AbstractResourceAccessManager.

Thanks for the pointer. I'll have a deeper look and see if that function is
really needed. From my cursory check so far, it's just replicating other
work the
SecureCatalogImpl is already doing, but I'll dig deeper

Cheers
Andrea

--

GeoServer Professional Services from the experts! Visit
http://goo.gl/NWWaa2 for more information.

Ing. Andrea Aime
@geowolf
Technical Lead

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

*AVVERTENZE AI SENSI DEL D.Lgs. 196/2003*

Le informazioni contenute in questo messaggio di posta elettronica e/o
nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il
loro utilizzo è consentito esclusivamente al destinatario del messaggio,
per le finalità indicate nel messaggio stesso. Qualora riceviate questo
messaggio senza esserne il destinatario, Vi preghiamo cortesemente di
darcene notizia via e-mail e di procedere alla distruzione del messaggio
stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso,
divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od
utilizzarlo per finalità diverse, costituisce comportamento contrario ai
principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for
the attention and use of the named addressee(s) and may be confidential or
proprietary in nature or covered by the provisions of privacy act
(Legislative Decree June, 30 2003, no.196 - Italy's New Data Protection
Code).Any use not in accord with its purpose, any disclosure, reproduction,
copying, distribution, or either dissemination, either whole or partial, is
strictly forbidden except previous formal approval of the named
addressee(s). If you are not the intended recipient, please contact
immediately the sender by telephone, fax or e-mail and delete the
information in this message that has been received in error. The sender
does not give any warranty or accept liability as the content, accuracy or
completeness of sent messages and accepts no responsibility for changes
made after they were sent or for other risks which arise as a result of
e-mail transmission, viruses, etc.

-------------------------------------------------------