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