Scoping permissions with a custom policy...
Felix Schwarz
after playing around with IPermissionPolicy for some time I suspect that
some use case are not really handled well by the current API and I would
like to resolve that for 0.12 :-)
1. For every policy check I have to load the db object again even if
the caller has a db object already. Can we make it
resource_or_dbobject?
2. Say I want to create a policy that prevents users to file tickets
with a specific milestone (this needs triage). How to do that in a
permission policy? There is no existing ticket so 'ticket:42' does
not work.
I guess we need to have some kind of virtual resource like:
ticket_attribute:milestone=0.11.6
fs
Christian Boos
> Hi,
>
> after playing around with IPermissionPolicy for some time I suspect
> that some use case are not really handled well by the current API and
> I would like to resolve that for 0.12 :-)
>
> 1. For every policy check I have to load the db object again even if
> the caller has a db object already. Can we make it
> resource_or_dbobject?
Hm no, that will break the existing API.
But we can think about alternatives. For example, having a thread local
cache for model objects which could make the retrieval of an already
instantiated object from its resource cheap. Other ideas welcomed.
> 2. Say I want to create a policy that prevents users to file tickets
> with a specific milestone (this needs triage). How to do that in a
> permission policy?
I don't think a permission policy is adequate here, at least not in the
current model where we don't check permissions on the milestone when
creating a ticket. So maybe the ticket validation approach would be more
adequate here (see ITicketManipulator).
-- Christian
Remy Blank
> But we can think about alternatives. For example, having a thread local
> cache for model objects which could make the retrieval of an already
> instantiated object from its resource cheap. Other ideas welcomed.
Yes, I had the same thought after implementing the new cache
infrastructure on trunk. We should have some kind of per-request cache.
I'd rather use the Request object for that, though, instead of a
thread-local. That way, it gets automatically discarded at the end of
the request.
-- Remy
Felix Schwarz
> Yes, I had the same thought after implementing the new cache
> infrastructure on trunk. We should have some kind of per-request cache.
> I'd rather use the Request object for that, though, instead of a
> thread-local. That way, it gets automatically discarded at the end of
> the request.
A general caching in the request sounds much better. However I'm a bit
hesitant to add even more caches all over the place. I don't know how it
works out for you but I have been bitten by the
DefaultPermissionPolicy's cache more than once in my unit tests. Anyway
a request-level cache would be much better.
fs
Andrea Tomasini
On 28 Sep, 2009, at 11:18 , Remy Blank wrote:
Hi Christian, Remy
I'd also would like to rationalize the cache usage, it is now a very
much localized approach, in some places (like the PermissionCache)
only partial information are stored, adding data to the request sounds
reasonable (and in Agilo we already do so, it might be a chance to
make things right once and for all :-) ). The option of using the
ITicketManipulator is too far away from the model, as you know in
Agilo we do change tickets in many ways (and not only in Agilo) think
at the import from CSV, or the Digital Whiteboard... or even the
Backlog.
A lot of third parties do the same. I really feel that permission
control should be more close to the model, and there must be a way to
make the permission checks on model property, this is just a thought.
I know Trac it is far away from being there now, but still we could
start moving things in the right direction... :-)
> -- Remy
ANdreaT
Felix Schwarz
True but in the context of IPermissionPolicy we don't have req :-)
fs
Felix Schwarz
>
> Felix Schwarz wrote:
>> Hi,
>>
>> after playing around with IPermissionPolicy for some time I suspect
>> that some use case are not really handled well by the current API and
>> I would like to resolve that for 0.12 :-)
>>
>> 1. For every policy check I have to load the db object again even if
>> the caller has a db object already. Can we make it
>> resource_or_dbobject?
>
> Hm no, that will break the existing API.
That's why I proposed it for 0.12 :-)
>> 2. Say I want to create a policy that prevents users to file tickets
>> with a specific milestone (this needs triage). How to do that in a
>> permission policy?
>
> I don't think a permission policy is adequate here, at least not in the
> current model where we don't check permissions on the milestone when
> creating a ticket. So maybe the ticket validation approach would be more
> adequate here (see ITicketManipulator).
The problem with ITicketManipulator is that it could be hard to hide the
options for which the user does not have sufficient permissions already
in the edit form...
fs
Christian Boos
> Am 28.09.2009 10:56, schrieb Christian Boos:
>>
>> Felix Schwarz wrote:
>>> Hi,
>>>
>>> after playing around with IPermissionPolicy for some time I suspect
>>> that some use case are not really handled well by the current API and
>>> I would like to resolve that for 0.12 :-)
>>>
>>> 1. For every policy check I have to load the db object again even if
>>> the caller has a db object already. Can we make it
>>> resource_or_dbobject?
>>
>> Hm no, that will break the existing API.
>
> That's why I proposed it for 0.12 :-)
Even there, we prefer to change the API in a backward compatible way,
unless there's really no way to do it differently.
As for the req proposal, I'd rather not do it this way, as the Request
is tied to the web layer which shouldn't play a role here. In 0.11, I
tried to "unbind" most of the API from the Request, in order to make it
possible to reuse Trac components outside of the web application context
(trac-admin, other scripts). We're certainly not there yet but it's not
a reason for allowing it to make a come back through the window ;-) The
thread local approach gives nearly the same "locality" to the cache, we
already do it for a number of other caches and I think this should
become the preferred way.
>>> 2. Say I want to create a policy that prevents users to file tickets
>>> with a specific milestone (this needs triage). How to do that in a
>>> permission policy?
>>
>> I don't think a permission policy is adequate here, at least not in the
>> current model where we don't check permissions on the milestone when
>> creating a ticket. So maybe the ticket validation approach would be more
>> adequate here (see ITicketManipulator).
>
> The problem with ITicketManipulator is that it could be hard to hide
> the options for which the user does not have sufficient permissions
> already in the edit form...
I see. That sounds like #8653. See also
http://trac.edgewall.org/wiki/TracDev/Proposals/EvenFinerGrainedPermissions
(the part about extending the target).
-- Christian
Jeff Hammel
>
> Felix Schwarz wrote:
> > Am 28.09.2009 10:56, schrieb Christian Boos:
> >>
> >> Felix Schwarz wrote:
> >>> Hi,
> >>>
> >>> after playing around with IPermissionPolicy for some time I suspect
> >>> that some use case are not really handled well by the current API and
> >>> I would like to resolve that for 0.12 :-)
> >>>
> >>> 1. For every policy check I have to load the db object again even if
> >>> the caller has a db object already. Can we make it
> >>> resource_or_dbobject?
> >>
> >> Hm no, that will break the existing API.
> >
> > That's why I proposed it for 0.12 :-)
>
> Even there, we prefer to change the API in a backward compatible way,
> unless there's really no way to do it differently.
>
> As for the req proposal, I'd rather not do it this way, as the Request
> is tied to the web layer which shouldn't play a role here. In 0.11, I
> tried to "unbind" most of the API from the Request, in order to make it
> possible to reuse Trac components outside of the web application context
> (trac-admin, other scripts). We're certainly not there yet but it's not
> a reason for allowing it to make a come back through the window ;-) The
> thread local approach gives nearly the same "locality" to the cache, we
> already do it for a number of other caches and I think this should
> become the preferred way.
Jeff
Remy Blank
> As for the req proposal, I'd rather not do it this way, as the Request
> is tied to the web layer which shouldn't play a role here. In 0.11, I
> tried to "unbind" most of the API from the Request, in order to make it
> possible to reuse Trac components outside of the web application context
> (trac-admin, other scripts).
Good point, I hadn't thought of that. So yes, I agree with the
thread-local idea.
-- Remy