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
------------------------------------------------------------------------------
_______________________________________________
Geoserver-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/geoserver-devel
