While I can see an active discussion on the maven repo proposal, I am a bit more interested the the delay to tomorrow’s release.
The problem has been reproduced - is it severe enough to be a blocker? Probably since it is security related ( I think it results in less content being visible than before? )
If this change was intensional as part of the JDBCConfig filter work we should at least document and provide update instructions.
Hi,
I have found the issue. The regression seems to be introduced in https://github.com/geoserver/geoserver/commit/c1750a1499fc059ecce153322eef8ff119684881 by n-lagomarsini
In DefaultResourceAccessManager method getSecurityFilter, where the layer security tree is converted to a filter, it will create a negative workspace filter whenever the root is accessible, even if the workspace is accessible as well. The method is quite confusing.
I suggest this fix: https://github.com/NielsCharlier/geoserver/commit/933f3c64c9fff980352eb89fc703f73e71f4398e
That solves the problem and the code looks a bit more understandable.
We need to build a test from scratch because apparently there is no test for this method present at all (or any usable test case for that matter). I think that is rather necessary to prevent such regressions. Will get this done later in the day.
Regards
Niels
On 18-03-15 07:54, Jody Garnett wrote:
While I can see an active discussion on the maven repo proposal, I am a bit more interested the the delay to tomorrow's release.
The problem has been reproduced - is it severe enough to be a blocker? Probably since it is security related ( I think it results in less content being visible than before? )
If this change was intensional as part of the JDBCConfig filter work we should at least document and provide update instructions.
------------------------------------------------------------------------------
Dive into the World of Parallel Programming The Go Parallel Website, sponsored
by Intel and developed in partnership with Slashdot Media, is your hub for all
things parallel software development, from weekly thought leadership blogs to
news, videos, case studies, tutorials and more. Take a look and join the
conversation now. http://goparallel.sourceforge.net/_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel
Hi all,
sorry for the late reply. We decided to replace the layer security tree with a filter in order to speed up performances when accessing the Catalog with the JDBCConfig plugin.
We have not managed to introduce any intentional security regression, so, as you already stated, this one is a bug.
Thank you for preparing the fix.
Regards,
Nicola Lagomarsini.
2015-03-18 12:10 GMT+01:00 Niels Charlier <niels@…2918…>:
Hi,
I have found the issue. The regression seems to be introduced in https://github.com/geoserver/geoserver/commit/c1750a1499fc059ecce153322eef8ff119684881 by n-lagomarsini
In DefaultResourceAccessManager method getSecurityFilter, where the layer security tree is converted to a filter, it will create a negative workspace filter whenever the root is accessible, even if the workspace is accessible as well. The method is quite confusing.
I suggest this fix: https://github.com/NielsCharlier/geoserver/commit/933f3c64c9fff980352eb89fc703f73e71f4398e
That solves the problem and the code looks a bit more understandable.We need to build a test from scratch because apparently there is no test for this method present at all (or any usable test case for that matter). I think that is rather necessary to prevent such regressions. Will get this done later in the day.
Regards
NielsOn 18-03-15 07:54, Jody Garnett wrote:
While I can see an active discussion on the maven repo proposal, I am a bit more interested the the delay to tomorrow’s release.
The problem has been reproduced - is it severe enough to be a blocker? Probably since it is security related ( I think it results in less content being visible than before? )
If this change was intensional as part of the JDBCConfig filter work we should at least document and provide update instructions.
------------------------------------------------------------------------------ Dive into the World of Parallel Programming The Go Parallel Website, sponsored by Intel and developed in partnership with Slashdot Media, is your hub for all things parallel software development, from weekly thought leadership blogs to news, videos, case studies, tutorials and more. Take a look and join the conversation now. [http://goparallel.sourceforge.net/](http://goparallel.sourceforge.net/)_______________________________________________ Geoserver-devel mailing list [Geoserver-devel@lists.sourceforge.net](mailto:Geoserver-devel@lists.sourceforge.net) [https://lists.sourceforge.net/lists/listinfo/geoserver-devel](https://lists.sourceforge.net/lists/listinfo/geoserver-devel)
Dive into the World of Parallel Programming The Go Parallel Website, sponsored
by Intel and developed in partnership with Slashdot Media, is your hub for all
things parallel software development, from weekly thought leadership blogs to
news, videos, case studies, tutorials and more. Take a look and join the
conversation now. http://goparallel.sourceforge.net/
Geoserver-devel mailing list
Geoserver-devel@anonymised.comsts.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel
–
Ing. Nicola Lagomarsini
Junior Software Engineer
GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
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 18-03-15 14:01, Nicola Lagomarsini wrote:
sorry for the late reply. We decided to replace the layer security tree with a filter in order to speed up performances when accessing the Catalog with the JDBCConfig plugin.
We have not managed to introduce any intentional security regression, so, as you already stated, this one is a bug.
Ah okay so this is a completely new feature that you introduced recently? I wasn't aware of that. In that case, could you please write a test case for this method that tests the tree is properly converted to filters?
Kind Regards
Niels
Ciao Niels,
below you can find a few remarks from my side. Maybe Andrea will chime
in later on.
Ing. Simone Giannecchini
@simogeo
Founder/Director
GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 333 8128928
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 Wed, Mar 18, 2015 at 12:10 PM, Niels Charlier <niels@anonymised.com> wrote:
Hi,
I have found the issue. The regression seems to be introduced in
https://github.com/geoserver/geoserver/commit/c1750a1499fc059ecce153322eef8ff119684881
by n-lagomarsiniIn DefaultResourceAccessManager method getSecurityFilter, where the layer
security tree is converted to a filter, it will create a negative workspace
filter whenever the root is accessible, even if the workspace is accessible
as well. The method is quite confusing.
I will ask Nicola to clarify further.
This work was performed in order to make GeoServer work nicely when
JDBCConfig is used and there is a ton of layers around (order of
100K).
This means, if you make a fix we need to make sure the fix does not
jeopardize performance.
I suggest this fix:
https://github.com/NielsCharlier/geoserver/commit/933f3c64c9fff980352eb89fc703f73e71f4398e
That solves the problem and the code looks a bit more understandable.
I will make sure Nicola has double checked.
We need to build a test from scratch because apparently there is no test for
this method present at all (or any usable test case for that matter). I
think that is rather necessary to prevent such regressions. Will get this
done later in the day.
I will ask Nicola to coordinate also in order to do some live testing
with an instance where we have thousands of layers.
As I said this fix, despite to the problem you found is crucial for
performance reasons in some (real world use cases).
Regards
NielsOn 18-03-15 07:54, Jody Garnett wrote:
While I can see an active discussion on the maven repo proposal, I am a bit
more interested the the delay to tomorrow's release.The problem has been reproduced - is it severe enough to be a blocker?
Probably since it is security related ( I think it results in less content
being visible than before? )If this change was intensional as part of the JDBCConfig filter work we
should at least document and provide update instructions.------------------------------------------------------------------------------
Dive into the World of Parallel Programming The Go Parallel Website,
sponsored
by Intel and developed in partnership with Slashdot Media, is your hub for
all
things parallel software development, from weekly thought leadership blogs
to
news, videos, case studies, tutorials and more. Take a look and join the
conversation now. http://goparallel.sourceforge.net/_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel------------------------------------------------------------------------------
Dive into the World of Parallel Programming The Go Parallel Website,
sponsored
by Intel and developed in partnership with Slashdot Media, is your hub for
all
things parallel software development, from weekly thought leadership blogs
to
news, videos, case studies, tutorials and more. Take a look and join the
conversation now. http://goparallel.sourceforge.net/
_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel
Hello Simone,
On 18-03-15 15:23, Simone Giannecchini wrote:
I will ask Nicola to clarify further. This work was performed in order to make GeoServer work nicely when JDBCConfig is used and there is a ton of layers around (order of 100K). This means, if you make a fix we need to make sure the fix does not jeopardize performance.
The fix doesn't influence performance. I didn't change anything about the fact that filters are being used for performance. The filter is just being wrongly created at the moment, it includes a filter that shouldn't be there. If anything my change improves performance because there is sometimes one filter less.
I will ask Nicola to coordinate also in order to do some live testing with an instance where we have thousands of layers. As I said this fix, despite to the problem you found is crucial for performance reasons in some (real world use cases).
That's cool, but I think in the first place there should be a simple test (it does not need many layers) that merely confirms that the conversion from tree to filter is being done correctly. At the moment there is no such test. This new method has been added and became part of the security system of geoserver; when this very crucial change was made a test should have been added that proves the correctness of this method, which unfortunately didn't happen.
Regards
Niels
Ciao Niels,
please, read below....
On Wed, Mar 18, 2015 at 3:35 PM, Niels Charlier <niels@anonymised.com> wrote:
Hello Simone,
On 18-03-15 15:23, Simone Giannecchini wrote:
I will ask Nicola to clarify further. This work was performed in order to
make GeoServer work nicely when JDBCConfig is used and there is a ton of
layers around (order of 100K). This means, if you make a fix we need to make
sure the fix does not jeopardize performance.The fix doesn't influence performance. I didn't change anything about the
fact that filters are being used for performance. The filter is just being
wrongly created at the moment, it includes a filter that shouldn't be there.
If anything my change improves performance because there is sometimes one
filter less.
cool
I will ask Nicola to coordinate also in order to do some live testing with
an instance where we have thousands of layers. As I said this fix, despite
to the problem you found is crucial for performance reasons in some (real
world use cases).That's cool, but I think in the first place there should be a simple test
(it does not need many layers) that merely confirms that the conversion from
agreed.
tree to filter is being done correctly. At the moment there is no such test.
This new method has been added and became part of the security system of
geoserver; when this very crucial change was made a test should have been
added that proves the correctness of this method, which unfortunately didn't
happen.
Yep, sorry about that.
Regards
Niels
On Wed, Mar 18, 2015 at 3:19 PM, Niels Charlier <niels@anonymised.com> wrote:
On 18-03-15 14:01, Nicola Lagomarsini wrote:
> sorry for the late reply. We decided to replace the layer security
> tree with a filter in order to speed up performances when accessing
> the Catalog with the JDBCConfig plugin.
>
> We have not managed to introduce any intentional security regression,
> so, as you already stated, this one is a bug.
>
Ah okay so this is a completely new feature that you introduced
recently? I wasn't aware of that. In that case, could you please write a
test case for this method that tests the tree is properly converted to
filters?
It's not really a new feature, it's a different way to do the same thing we
did before (and we lost bits in the translation).
As for tests, there is quite a bit of test classes checking the security
subsystem as a whole, integration ones, spread
around in the code base (a few in main, one in wms, one in wfs, one in
wcs), but I agree having a unit test would also be
a good thing
Cheers
Andrea
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 Wed, Mar 18, 2015 at 12:10 PM, Niels Charlier <niels@anonymised.com> wrote:
Hi,
I have found the issue. The regression seems to be introduced in
https://github.com/geoserver/geoserver/commit/c1750a1499fc059ecce153322eef8ff119684881
by n-lagomarsiniIn DefaultResourceAccessManager method getSecurityFilter, where the layer
security tree is converted to a filter, it will create a negative workspace
filter whenever the root is accessible, even if the workspace is accessible
as well. The method is quite confusing.
The method requires some understading of how the existing security
subsystem works... which is by always matching
the most specific item in the tree. Basically, you walk from the root to
the leaves, and use the access rule of the
most specific item you find in your walk.
When you have to turn that into a filter, at the workspace level you
basically have two options:
* You can access the workspace, but there might be overrides that disallow
access to certain layers to the current user
* You cannot access the workspace, but there might be overrides that allow
access to certain layers to the current user
However you also have to take into account what root access you had, so
it's really a 3 levels logic, and you're really
building a set of exceptions to your root level access.
I suggest this fix:
https://github.com/NielsCharlier/geoserver/commit/933f3c64c9fff980352eb89fc703f73e71f4398e
That solves the problem and the code looks a bit more understandable.
I don't really understand how it makes it more readable, but a cursory look
suggest the patch is correct.
But if we want to make the code more readable, we should ad add the above
explanation to the comments instead.
Cheers
Andrea
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.
-------------------------------------------------------
Hi Andrea,
I understand how it works. I studied the tree system quite thoroughly, remember 
What I found confusing is this code:
if (rootAccess) {
wsFilter= Predicates.notEqual(wsNameProperty, wsName);
} else {
wsFilter= Predicates.equal(wsNameProperty, wsName);
}
so the workspace filter is created based on the rootAccess, irrespective of what wsAccess is, irrespective whether is actually going to be need to be used or not.
That does not only look like very strange and confusing, it also is what introduced the bug.
In my change you can clearly see in the if block when the filter is needed and why, and it is not created otherwise. That is much more logical and readable.
Regards
Niels
On 18-03-15 16:41, Andrea Aime wrote:
On Wed, Mar 18, 2015 at 12:10 PM, Niels Charlier <niels@anonymised.com <mailto:niels@anonymised.com>> wrote:
Hi,
I have found the issue. The regression seems to be introduced in
https://github.com/geoserver/geoserver/commit/c1750a1499fc059ecce153322eef8ff119684881
by n-lagomarsiniIn DefaultResourceAccessManager method getSecurityFilter, where
the layer security tree is converted to a filter, it will create a
negative workspace filter whenever the root is accessible, even if
the workspace is accessible as well. The method is quite confusing.The method requires some understading of how the existing security subsystem works... which is by always matching
the most specific item in the tree. Basically, you walk from the root to the leaves, and use the access rule of the
most specific item you find in your walk.
When you have to turn that into a filter, at the workspace level you basically have two options:
* You can access the workspace, but there might be overrides that disallow access to certain layers to the current user
* You cannot access the workspace, but there might be overrides that allow access to certain layers to the current user
However you also have to take into account what root access you had, so it's really a 3 levels logic, and you're really
building a set of exceptions to your root level access.I suggest this fix:
https://github.com/NielsCharlier/geoserver/commit/933f3c64c9fff980352eb89fc703f73e71f4398e
That solves the problem and the code looks a bit more understandable.I don't really understand how it makes it more readable, but a cursory look suggest the patch is correct.
But if we want to make the code more readable, we should ad add the above explanation to the comments instead.Cheers
Andrea
org.geoserver.security.impl.On Wed, Mar 18, 2015 at 3:35 PM, Niels Charlier
<niels@anonymised.com> wrote:
That's cool, but I think in the first place there should be a simple
test (it does not need many layers) that merely confirms that the
conversion from tree to filter is being done correctly. At the moment
there is no such test. This new method has been added and became part of
the security system of geoserver; when this very crucial change was made
a test should have been added that proves the correctness of this
method, which unfortunately didn't happen.
So far we had rules to add tests for new features, and bug fixes.
This work has been a performance optimization, no expected behavioral
changes, and it used the pre-existing
integration test coverage to confirm its correctness.
So, you're basically saying that when performance optimization is done,
with no intent to change behavior, tests should be added to cover any
replacement code path, ignoring the pre-existing coverage, of course,
assuming there is already a good amount of it, like in this case.
I think I'm good with the direction, but we need a clear agreement, since
that drives what
we ask contributors to do when we receive pull requests (like, I don't
want different core devs to give different advices on this on different
pull requests, or worse, in the same pull request, confusing the
contributor).
Cheers
Andrea
PS: to give everybody some context, here are the pre-existing test classes
that end up invoking the method in question, and checking its behavior
from an integration testing point of view:
* org.geoserver.security.impl.SecureCatalogImplTest
* org.geoserver.security.impl.SecureCatalogIntegrationTest
I thought wms/wcs/wfs also had tests covering the
DefaultResourceAccessManager,
but in fact the are using a mock one
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 Wed, Mar 18, 2015 at 4:51 PM, Niels Charlier <niels@anonymised.com> wrote:
In my change you can clearly see in the if block when the filter is
needed and why, and it is not created otherwise. That is much more logical
and readable.
We can agree to disagree here... the core is more correct, I don't find it
more readable, of course
we have different ways to approach code and so on, but believe your
statement is just a matter
of personal taste (as such, I advise to better qualify how things are not
readable for other people
to understand your point of view).
Cheers
Andrea
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.
-------------------------------------------------------
Don’t think it is personal taste. First, you are creating something that is not needed and thereby waste memory and time. Even though something as small as that doesn’t matter any more in modern computer systems, it is bad code practice. Second, the third party reader must put two things together that are in a different place in the code and aggregate them logically to understand the real condition: (rootAccess) { …} else {…}" and later " ( ) {" . Third, this separation of the condition in the code is what introduced the bug which proves the point, and it would more likely create bugs in later edits because the logic is spread around. Regards Niels
On 18-03-15 16:57, Andrea Aime wrote:
On Wed, Mar 18, 2015 at 4:51 PM, Niels Charlier <niels@anonymised.com> wrote:
In my change you can clearly see in the if block when the filter is needed and why, and it is not created otherwise. That is much more logical and readable.
We can agree to disagree here… the core is more correct, I don’t find it more readable, of course
we have different ways to approach code and so on, but believe your statement is just a matter
of personal taste (as such, I advise to better qualify how things are not readable for other people
to understand your point of view).
"ififwsAccess!=rootAccess
On Wed, Mar 18, 2015 at 5:05 PM, Niels Charlier <niels@anonymised.com> wrote:
On 18-03-15 16:57, Andrea Aime wrote:
On Wed, Mar 18, 2015 at 4:51 PM, Niels Charlier <niels@anonymised.com> wrote:
In my change you can clearly see in the if block when the filter is
needed and why, and it is not created otherwise. That is much more logical
and readable.We can agree to disagree here... the core is more correct, I don't find
it more readable, of course
we have different ways to approach code and so on, but believe your
statement is just a matter
of personal taste (as such, I advise to better qualify how things are not
readable for other people
to understand your point of view).Don't think it is personal taste. First, you are creating something
that is not needed and thereby waste memory and time. Even though something
as small as that doesn't matter any more in modern computer systems, it is
bad code practice. Second, the third party reader must put two things
together that are in a different place in the code and aggregate them
logically to understand the real condition: "if (rootAccess) { ...} else
{...}" and later "if (wsAccess != rootAccess) {" . Third, this separation
of the condition in the code is what introduced the bug which proves the
point, and it would more likely create bugs in later edits because the
logic is spread around.
Thank Niels, I don't think we have anyone reviewing pull requests from this
angle... I invite you to join
the team that checks the incoming pull requests and thus help increase the
GeoServer code quality
Cheers
Andrea
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.
-------------------------------------------------------
Hmmm okay perhaps you're right in theory, but I just think that even though the behaviour of the end result is supposed to be the same, this method alters the internal behaviour a lot and adds a very specific procedure to the mix (creating a filter from a tree) that I normally would have expected to see individually tested. I often get asked to make tests for much more trivial things that that, I assumed every significant change needs to be tested, and I thought that integration tests are not sufficient but that new classes and significant methods need to be tested as well. But perhaps I am mistaken.
Regards
Niels
On 18-03-15 16:55, Andrea Aime wrote:
org.geoserver.security.impl.On Wed, Mar 18, 2015 at 3:35 PM, Niels Charlier <niels@anonymised.com <mailto:niels@anonymised.com>> wrote:
That's cool, but I think in the first place there should be a simple
test (it does not need many layers) that merely confirms that the
conversion from tree to filter is being done correctly. At the moment
there is no such test. This new method has been added and became
part of
the security system of geoserver; when this very crucial change
was made
a test should have been added that proves the correctness of this
method, which unfortunately didn't happen.So far we had rules to add tests for new features, and bug fixes.
This work has been a performance optimization, no expected behavioral
changes, and it used the pre-existing
integration test coverage to confirm its correctness.So, you're basically saying that when performance optimization is done,
with no intent to change behavior, tests should be added to cover any
replacement code path, ignoring the pre-existing coverage, of course,
assuming there is already a good amount of it, like in this case.I think I'm good with the direction, but we need a clear agreement, since that drives what
we ask contributors to do when we receive pull requests (like, I don't
want different core devs to give different advices on this on different
pull requests, or worse, in the same pull request, confusing the contributor).Cheers
AndreaPS: to give everybody some context, here are the pre-existing test classes
that end up invoking the method in question, and checking its behavior
from an integration testing point of view:
* org.geoserver.security.impl.SecureCatalogImplTest
* org.geoserver.security.impl.SecureCatalogIntegrationTestI thought wms/wcs/wfs also had tests covering the DefaultResourceAccessManager,
but in fact the are using a mock one--
GeoServer Professional Services from the experts! Visit
http://goo.gl/NWWaa2 for more information.Ing. Andrea Aime
@geowolf
Technical LeadGeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313 <tel:%2B39%200584%20962313>
fax: +39 0584 1660272 <tel:%2B39%200584%201660272>
mob: +39 339 8844549 <tel:%2B39%20%C2%A0339%208844549>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 18-03-15 17:10, Andrea Aime wrote:
Thank Niels, I don't think we have anyone reviewing pull requests from this angle... I invite you to join
the team that checks the incoming pull requests and thus help increase the GeoServer code quality
Andrea, I didn't mean to criticize this code so much, I'm convinced I sometimes write confusing code as well, I don't think anyone here doesn't. I was just surprised to read you say my fix didn't improve readability and then that it was merely personal and I had no arguments why it would be more readable for someone else, those remarks pushed me to go in to detail. I didn't originally mean any harm or blame with my original remark, I was just saying, this is a bug fix and makes it a little bit clearer what is happening, that is all.
Also I have reviewed pull requests and will do so in the future as well.
Regards
Niels
So it looks like Niels / Andrea are reviewing a pull request - can you ping the list when this has been merged in for testing.
We can probably start the geotools release today, but I would like to ensure we have an example of CITE tests passing first.
Jody
On 17 March 2015 at 23:54, Jody Garnett <jody.garnett@anonymised.com> wrote:
While I can see an active discussion on the maven repo proposal, I am a bit more interested the the delay to tomorrow’s release.
The problem has been reproduced - is it severe enough to be a blocker? Probably since it is security related ( I think it results in less content being visible than before? )
If this change was intensional as part of the JDBCConfig filter work we should at least document and provide update instructions.
–
Jody Garnett
On Wed, Mar 18, 2015 at 5:16 PM, Niels Charlier <niels@anonymised.com> wrote:
Hmmm okay perhaps you're right in theory, but I just think that even
though the behaviour of the end result is supposed to be the same, this
method alters the internal behaviour a lot and adds a very specific
procedure to the mix (creating a filter from a tree) that I normally would
have expected to see individually tested. I often get asked to make tests
for much more trivial things that that, I assumed every significant change
needs to be tested, and I thought that integration tests are not sufficient
but that new classes and significant methods need to be tested as well. But
perhaps I am mistaken.
As said, I'm good with the direction, more testing is good, we just need to
find a shared agreement to get a level playing field for everybody (and
especially
for random contributions making pull requests).
Also, I stress that you seem to have a different point of view to things
that would be useful when reviewing pull requests,
so I renew my invite to help checking the incoming pull requests, be them
from external contributors, or core developers.
Ben was also stressing in a separate thread that we need more people to
take care of the incoming set of pull requests
(this code we are talking about has been one afaik), your help there would
be very much appreciated.
Cheers
Andrea
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 18-03-15 17:29, Andrea Aime wrote:
Also, I stress that you seem to have a different point of view to things that would be useful when reviewing pull requests,
so I renew my invite to help checking the incoming pull requests, be them from external contributors, or core developers.
Ben was also stressing in a separate thread that we need more people to take care of the incoming set of pull requests
(this code we are talking about has been one afaik), your help there would be very much appreciated.
I will do my best. Sorry I thought your other email was sarcasm, perhaps it wasn't. Then you may disregard my response.
Regards
Niels
On Wed, Mar 18, 2015 at 5:28 PM, Jody Garnett <jody.garnett@anonymised.com>
wrote:
So it looks like Niels / Andrea are reviewing a pull request - can you
ping the list when this has been merged in for testing.
Not really, we have been discussing a single change with no tests, and more
in general how we should approach
performance enhancing pull requests and their review.
As Niels suggests, we need a new unit test for that method, which is
missing, Nicola is going to work on it
tomorrow as far as I know.
Personally I'm stuck at a customer site and I've already taken too much
time off in order to participate to this (important)
discussion.
Cheers
Andrea
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.
-------------------------------------------------------