Hey Andrea,
Thanks for the feedback. Comments inline.
On Sun, Feb 26, 2012 at 8:04 AM, Andrea Aime
<[email protected]>wrote:
> Hi,
> reading GSIP 74 and commenting as I go.
>
> The proposal looks generally good, I like the idea of setting
> a "admin request" context and have the code check it.
>
> The concerns I have is that like the workspace local stuff
> this code is taking a ride on ResourecAccessManager but
> modifies it in a way that breaks backwards compatibility
> (the only constructuctor for the class has a new parameter,
> breaking all existing users).
> Is it done because there is no real clean upgrade path?
>
Right... I guess i never really considered others implementing this
interface. Certainly I am willing to make the changes in a nicer way
maintaining the current api. So first thing is as you mention
the WorkspaceAccessLimits. Question is though what makes sense for default
value of the adminable flag?
Just make it false? I could see this perhaps being an leading to access to
the web amin no longer being enabled. Perhaps we should check read + write
and if both set make admirable true? Perhaps also checking the current
authentication for ROLE_ADMINISTRATOR?
As for the ResourceAccessManager itself should we introduce an abstract
base class that people can implement so we can stub out new methods?
>
> The other thing is that it introduces an assymetry in
> ResourceAccessManager,
> I can authorize workspaces for administrator activity, but I cannot
> to the same for the single resource/style/layer group?
> I realize adding that that is a lot of work to add, but the proposal should
> imho mention this limitation.
>
Right, will do that.
> I'm also wondering if things could be setup so that adding
> layer/style/group admin abilites could be added later without
> breaking implementors of ResoureAccessManager.
>
Yeah, I guess what it would amount to is adding the "adminable" flag to all
the limits classes. We could to that now and just have them lay dormant for
things other than workspace?
>
> The proposal imho should state how a ResourceAccessManager
> needs to be modified in order to play with the new set of rights
> (thinking at least about GeoRepository, GeoShield and GeoNode here,
> but there is a growing set of ResourceAccessManager implementations
> that tie in directly to pre-existing in-house authorization sysystems too).
>
Makes sense, i would be happy to add some notes in the
backward compatibility section for this. Unfortunately i am not that
familiar with any of these implementations (didn't know they were playing
with the ResourceAccessManager api directly) until now so i am not sure
what would be particularly relevant. What would be ideal is if folks closer
to these projects could comment.
>
> In terms of GUI, the way the proposal is setup any authenticated
> user will see the workspace/store/layers menu entries, even if the user in
> question
> might not have any kind of admin abilities.
> I cannot say that this is necessarily bad, but it makes me wonder if
> the GUI should first check with the ResourceAccessManager if
> the user has any kind of admin rights whatsoever before starting
> to show pages that contain "add" links/buttons even if the
> user has no admin rights.
>
Yeah, this should be doable. The current check is simply for being
authenticated, doing this check should be straight forward enough, and also
disabling links should be easy as well.
For example, what would happen if a user that has no admin
> rights clicks the "add new store" link in the stores page?
> Or in the workspace page?
> I hope there will be an error, but it would be better if the user could
> not get there at all, no?
>
Indeed they get an access denied page but i agree having the links disabled
all together is much nicer.
>
> Actually.... say a user is an admin of a specific workspace. What will
> happen if he tries to add a new one? How is that disallowed?
> An error message is probably fine, but not clean GUI wise.
> The proposal does not say much about REST-config, but I guess
> there we are supposed to get an error?
> I looked again at the patch but cannot see the part that is supposed
> to do this check (patch is large, apologies in advance if I just missed
> it).
>
In the ui they are denied access to the page but with the rest api there
access is determined by rest.properties. So if rest.properties gives the
user access they could add a new workspace. I agree we need to tighten this
up. I think it makes sense to have SecureCatalog enforce access to
modification methods (add + save) and if the user is not ROLE_ADMINISTRATOR
don't allow it. Thoughts?
>
> Cheers
> Andrea
>
> --
> -------------------------------------------------------
> Ing. Andrea Aime
> GeoSolutions S.A.S.
> Tech lead
>
> Via Poggio alle Viti 1187
> 55054 Massarosa (LU)
> Italy
>
> phone: +39 0584 962313
> fax: +39 0584 962313
> mob: +39 339 8844549
>
> http://www.geo-solutions.it
> http://geo-solutions.blogspot.com/
> http://www.youtube.com/user/GeoSolutionsIT
> http://www.linkedin.com/in/andreaaime
> http://twitter.com/geowolf
>
> -------------------------------------------------------
>
>
> ------------------------------------------------------------------------------
> Virtualization & Cloud Management Using Capacity Planning
> Cloud computing makes use of virtualization - but cloud computing
> also focuses on allowing computing to be delivered as a service.
> http://www.accelacomm.com/jaw/sfnl/114/51521223/
> _______________________________________________
> Geoserver-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/geoserver-devel
>
>
--
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.
------------------------------------------------------------------------------
Try before you buy = See our experts in action!
The most comprehensive online learning library for Microsoft developers
is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3,
Metro Style Apps, more. Free future releases when you subscribe now!
http://p.sf.net/sfu/learndevnow-dev2
_______________________________________________
Geoserver-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/geoserver-devel
