Hi,
Interesting approach. But I have two remarks.
Alec Thomas wrote:
Okay, I've changed tack yet again after some discussion with Noah on IRC.
http://swapoff.org/files/new-perms.diff
http://swapoff.org/files/new-perms-core.diff
This patch is much less intrusive, and cleaner. The permission cache
methods are thus:
def has_permission(self, action, resource=None):
def has_some_permission(self, action):
def assert_permission(self, action, resource=None):
def assert_some_permission(self, action):
def permissions(self): # TODO Remove the need for this. Only used in
templates I believe?
Do we really need to interpret `resource=None` as
"indicating that action can be applied to any resource." ?
I observe that you don't make use of this feature in your patch,
if I'm not mistaken.
If instead of the above, `resource=None` could be interpreted as
"the action can be applied in general, unless there's a specific
restriction for a given resource", the patches could be even less
disruptive.
I was at first a bit puzzled why we had to change things like, e.g.:
- if req.perm.has_permission('MILESTONE_VIEW'):
+ if req.perm.has_some_permission('MILESTONE_VIEW'):
The first line is self-explaining, whereas the second makes you wonder
"which permissions"? (and isn't there an 's' missing?)
Then, for the "resource" itself, I think we should pass the "type"
information as well as the "id" only. Relying on the action to
decide what kind of resource to we handle seems a bit kludgy.
In the future, those resources could even know a bit about themselves,
and for resource based permission, we could write things like:
obj.has_permission('WIKI_VIEW')
which would translate into:
def has_permission(action):
self.req.perm.has_permission(action, self.type, self.id)
-- Christian
_______________________________________________
Trac-dev mailing list
[email protected]
http://lists.edgewall.com/mailman/listinfo/trac-dev
