Emanuele,
I just wanted to verify that the geofence rest API PR okay to merge now:
https://github.com/geoserver/geoserver/pull/1044
Regards
Niels
Hi Niels,
I just wanted to verify that the geofence rest API PR okay to merge now:
https://github.com/geoserver/geoserver/pull/1044
still checking it.
I'll continue sending you the minor technical issues in private mails, in
order not to fill the list with details which may not be generally interesting.
Some general considerations:
- I see that for the geofence REST API we only have XML output, while the
other geoserver REST calls also allows json and html. Is that ok? Should it be
made expicitly clear in the doc?
- Information provided by the geofence REST API are quite sensitive, so
probably all the geofence/ paths should require user authentication.
It would be a good thing to point out in the doc that the geofence/ path
should be at least protected in a reverse proxied environment.
- The Geofence refactoring implemented to allow to plug the server part into
geoserver has made the roles/groups configuration an external responsability;
given that, I guess that the user/role calls should not be in the geofence
API.
- Servicename in user / role API should be somehow hidden.
Currently we have paths such as:
/rest/usergroup/{serviceName}/users
/rest/usergroup/{serviceName}/group/{group
In order to merge this point with the previous one, would it be a good idea to
create a "geofence" "user group service", and have all the calls to the
user/group interface only interact with that service? It would simplify the
REST URLs, and would restrict the scope of what could be handled using the
REST API.
Cheers,
Emanuele
Alle 23:27:54 di Thursday 4 June 2015, Niels Charlier ha scritto:
Emanuele,
I just wanted to verify that the geofence rest API PR okay to merge now:
https://github.com/geoserver/geoserver/pull/1044Regards
Niels
--
GeoServer Professional Services from the experts!
Visit http://goo.gl/NWWaa2 for more information.
Ing. Emanuele Tajariol
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 380 2116282
http://www.geo-solutions.it
http://twitter.com/geosolutions_it
-------------------------------------------------------
Hi Niels,
I found other NPEs which return a 500 to the caller, such as
GET http://localhost:8080/geoserver/geofence/rest/roles/service/XXX
Dunno if it's the case to report and fix these issues now, or if we should move
this kind of tests after the pullreq has been merged, since it builds properly
and gives no problems at runtime.
Cheers,
Emanuele
Alle 23:27:54 di Thursday 4 June 2015, Niels Charlier ha scritto:
> Emanuele,
>
> I just wanted to verify that the geofence rest API PR okay to merge now:
> https://github.com/geoserver/geoserver/pull/1044
>
>
> Regards
> Niels
--
GeoServer Professional Services from the experts!
Visit http://goo.gl/NWWaa2 for more information.
Ing. Emanuele Tajariol
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 380 2116282
http://www.geo-solutions.it
http://twitter.com/geosolutions_it
-------------------------------------------------------
Hello Emanuele,
thank for your thorough review!
On 06/05/2015 11:50 AM, Emanuele Tajariol wrote:
- I see that for the geofence REST API we only have XML output, while the
other geoserver REST calls also allows json and html. Is that ok? Should it be
made expicitly clear in the doc?
Good point, it should be a simple configuration to allow json, I will add this as well.
- Information provided by the geofence REST API are quite sensitive, so
probably all the geofence/ paths should require user authentication.
It would be a good thing to point out in the doc that the geofence/ path
should be at least protected in a reverse proxied environment.
Indeed, that would be sort of defeat the purpose of security if the rest was open to the public. As a simple built-in solution we could limit the use of the rest api to certain role(s). This could be role_admin or it could be configurable.
- The Geofence refactoring implemented to allow to plug the server part into
geoserver has made the roles/groups configuration an external responsability;
given that, I guess that the user/role calls should not be in the geofence
API.
That is true, but we have discussed this issue extensively. The issue is that the geoserver rest module uses another system (restful instead of spring) and Jody and Andrea do not want to two to mix together, but they also don't want to start a new module. For now this is threaten as a special bonus feature of the geofence-server community module, unfortunately unavailable to those who wish to use the normal security instead of geofence...
- Servicename in user / role API should be somehow hidden.
Currently we have paths such as:
/rest/usergroup/{serviceName}/users
/rest/usergroup/{serviceName}/group/{group
In order to merge this point with the previous one, would it be a good idea to
create a "geofence" "user group service", and have all the calls to the
user/group interface only interact with that service? It would simplify the
REST URLs, and would restrict the scope of what could be handled using the
REST API.
I'm not sure about this one. Why build in limitations that are not programmatically necessary. There are many user/group services possible so I would allow all to be used and modified (if they are modifiable).
I guess we can create a configuration setting for a "default" user/group service so we can treat it in a similar manner as the role services. I think that would be a good idea, the setting can be created with spring and by default be the word "default" (because normally, the geoserver has a user/role service called "default").
On 06/05/2015 11:52 AM, Emanuele Tajariol wrote:
RulesRestController update() has a couple of issues:
- Long comparison is performed with a "!=". Both of them are objects, and so
they will not be automatically unboxed.
- If a rule with the given priority does not exists, it will throw an NPE
You are right, this is a bug and I will resolve this immediately.
On 06/05/2015 12:24 PM, Emanuele Tajariol wrote:
Hi Niels,
I found other NPEs which return a 500 to the caller, such as
GEThttp://localhost:8080/geoserver/geofence/rest/roles/service/XXX
Dunno if it's the case to report and fix these issues now, or if we should move
this kind of tests after the pullreq has been merged, since it builds properly
and gives no problems at runtime.
Yes, I am aware that exception reporting is not perfect yet, something more understandable by an end user should be thrown instead of NPE. This is not a blocking issue, but I guess I can do add some NULL checks for service names, ids and the sorts and throw something more readable without too much effort. I will do that with the next commit.
Regards
Niels
Hi Emanuele,
Just to let you know I update the PR. For your convenience I put all the changes in a separate commit, but I will squash them before merging.
Changes: added JSON support, added security, usage of a default servicename for user/group service, better exception reporting with invalid service/username/groupname/rolename, fixed priority bug, added test for it, updated docs.
Thank you in advance for your review.
Kind Regards
Niels
On 09-06-15 00:15, Niels Charlier wrote:
Hello Emanuele,
thank for your thorough review!
On 06/05/2015 11:50 AM, Emanuele Tajariol wrote:
- I see that for the geofence REST API we only have XML output, while the
other geoserver REST calls also allows json and html. Is that ok? Should it be
made expicitly clear in the doc?Good point, it should be a simple configuration to allow json, I will
add this as well.- Information provided by the geofence REST API are quite sensitive, so
probably all the geofence/ paths should require user authentication.
It would be a good thing to point out in the doc that the geofence/ path
should be at least protected in a reverse proxied environment.Indeed, that would be sort of defeat the purpose of security if the rest
was open to the public. As a simple built-in solution we could limit the
use of the rest api to certain role(s). This could be role_admin or it
could be configurable.- The Geofence refactoring implemented to allow to plug the server part into
geoserver has made the roles/groups configuration an external responsability;
given that, I guess that the user/role calls should not be in the geofence
API.That is true, but we have discussed this issue extensively. The issue is
that the geoserver rest module uses another system (restful instead of
spring) and Jody and Andrea do not want to two to mix together, but they
also don't want to start a new module. For now this is threaten as a
special bonus feature of the geofence-server community module,
unfortunately unavailable to those who wish to use the normal security
instead of geofence...- Servicename in user / role API should be somehow hidden.
Currently we have paths such as:
/rest/usergroup/{serviceName}/users
/rest/usergroup/{serviceName}/group/{group
In order to merge this point with the previous one, would it be a good idea to
create a "geofence" "user group service", and have all the calls to the
user/group interface only interact with that service? It would simplify the
REST URLs, and would restrict the scope of what could be handled using the
REST API.I'm not sure about this one. Why build in limitations that are not
programmatically necessary. There are many user/group services possible
so I would allow all to be used and modified (if they are modifiable).
I guess we can create a configuration setting for a "default" user/group
service so we can treat it in a similar manner as the role services. I
think that would be a good idea, the setting can be created with spring
and by default be the word "default" (because normally, the geoserver
has a user/role service called "default").On 06/05/2015 11:52 AM, Emanuele Tajariol wrote:
RulesRestController update() has a couple of issues:
- Long comparison is performed with a "!=". Both of them are objects, and so
they will not be automatically unboxed.
- If a rule with the given priority does not exists, it will throw an NPEYou are right, this is a bug and I will resolve this immediately.
On 06/05/2015 12:24 PM, Emanuele Tajariol wrote:
Hi Niels,
I found other NPEs which return a 500 to the caller, such as
GEThttp://localhost:8080/geoserver/geofence/rest/roles/service/XXX
Dunno if it's the case to report and fix these issues now, or if we should move
this kind of tests after the pullreq has been merged, since it builds properly
and gives no problems at runtime.Yes, I am aware that exception reporting is not perfect yet, something
more understandable by an end user should be thrown instead of NPE. This
is not a blocking issue, but I guess I can do add some NULL checks for
service names, ids and the sorts and throw something more readable
without too much effort. I will do that with the next commit.Regards
Niels------------------------------------------------------------------------------
_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel