Hi Darby
Sorry for the delay in replying. In between jobs I've been revisiting
this response to ensure I'm answering you properly and that my
thought process isn't being clouded by what I'm used to.
I must say at the outset that I agree with your last signoff - we
need some further critical feedback from the community, otherwise
it's more or less a debate between the both of us that may or may not
be resolved satisfactorily. If it's left as-is then I would probably
rewrite my own wrapper for Zend_Acl to do what I want it to do, but I
do think that the benefits of my proposed changes are worthwhile to
the framework.
For this reason I've cross-posted into fw-general in the hope it
grabs some attention. For those who wish to see the previous posts,
it's at http://www.nabble.com/Zend_Acl-resource-definitions-
tf3130511s16154.html
I've edit your previous post for the sake of brevity and have
responses below:-
Using magic methods for virtual resources
I remain unconvinced that using magic property access for this (i.e.,
using __get() to provide $acl->magic->property->path->isAllowed())
is a
good idea. I cannot think of any compelling reason that this magic
should be supplied with the framework component, given that the
component already offers a simple query API that supports automatic
code
completion.
Coupling the syntax of the authorization query to the depth of the
resource in the resource tree does not represent best practices and
presents code maintainability problems for dynamic ACLs. That is,
if the
position of a resource in the tree should change, then the
developer has
to change her code in order to query against it. The current query API
is not so coupled with the queried resource's position in the tree. At
this point, therefore, I cannot recommend that we include this with
the
framework component, but certainly such __get() magic be done in user
classes where it may be desirable for whatever reasons.
To my mind, I don't see this as being any different from Zend_Config
which does allow magic access. Imagine how clunky the interface would
be if we had to perform: $config->getSection('db')->getProperty
('hostname').
Whether or not a resource changes position within a tree is a
challenge for the programmer that implements such a strategy, and I
don't believe that the framework should be enforcing such choices
upon the programmer by restricting the API. Nor should automatic code
completion be a pre-requisite for the interface - it's handy to have,
but are we designing this simply for the programmer's IDE or for the
end result?
However I do acknowledge that my changes do potentially represent a
change to the isAllowed() method, which is detailed below:-
Virtual resource implementation
Currently, Zend_Acl encapsulates the complexities of single
inheritance
(between resources) and multiple inheritance (between roles). You need
not know the entire ancestry of a resource (or role) in order to query
against it. You need only its identifier or a reference to the
object. I
think that such encapsulation of complexity is a good thing.
Nothing prevents you from identifying your resource objects with
complete paths. This is not a solution for you, of course, since
you are
attempting to prevent having to register the object before querying
against it:
That's right, and the identifier may not completely represent its
relationship. Whilst you don't need to know the ancestry of a
resource - and Zend_Acl may not need to know it internally - it is
still likely that some form of tree structure is required by a
developer to maintain the Acl.
$acl->add(new Zend_Acl_Resource('/news/archived/codea'));
$acl->isAllowed('guest', '/news/archived/codea');
The main principle on which the current design is based is that role
objects request access to resource objects. The concept of querying
"virtual resources," not actually objects at all, breaks this
foundation
principle.
I don't believe virtual resources break this principle. Role objects
will still requests access to resource objects, it's just that they
don't physically need to exist in order to provide an answer.
However, for virtual resources to be accessed, they do need to fit
into some form of hierarchy. You have already implemented the 'has'
and 'inherits' methods, so really all that needs attention is the
method by which these resources are traversed.
Quickly developers will want to do more with virtual resources than
just
query against them. Some developers will find that they need to add a
rule specifically for what were queried as virtual resources. What
then?
Using your previous example, we define a rule thusly:
acl -> section (allow to all)
And now we need to query for access to:
acl -> section -> myothersection -> edit -> 328
To what object does "328" map in the application? I suspect that "328"
would map to, for example, an Article object having id 328 in a CMS.
Now, assume that role 'sally' needs the ability to edit the article,
too. A rule must be created upon Article 328. Now, either:
(1) we must make drastic changes to Zend_Acl in order to support
making
rules upon virtual resources, or
(2) Article 328 must be added to the ACL, anyway, as under the current
design, rules can only be added for resource objects that Zend_Acl
knows
about. Thus, for objects to which ACL rules will directly apply (i.e.,
not by inheritance), virtual resource query support does not prevent
having to add the objects to the ACL.
My fault for the lack of clarity.
I meant to write the relationship as:-
acl -> news -> mycategory -> id328
'328' represents the id of an article in a database, which has been
assigned to category 'mycategory'.
Assuming the following rules:-
$acl->addRole(new Zend_Acl_Role('sally'));
$acl->addResource(new Zend_Acl_Resource('news')); // 'Real' resource
$acl->allow('sally', 'news'); // NOTE: We can only define rules on
'real' resources or 'null' meaning all. This is important.
$acl->deny('sally', 'news', 'delete'); As above
I should be able to perform the following queries:-
$acl->news->isAllowed('sally'); // returns true - news is 'real' and
role 'guest' does not have access to it.
$acl->news->mycategory->isAllowed('sally', 'edit'); // returns true -
'mycategory' inherits news, which resolves to true
$acl->news->mycategory->id328->isAllowed('sally', 'delete'); //
returns false - 'id328' inherits 'mycategory' inherits news, which
resolves to false for an action of 'delete'.
NOTE: The isAllowed API here has changed slightly.
Before: isAllowed([role], [resource], [privilege])
After: isAllowed([role], [privilege], [resource])
The 'virtual' resource and magic methods are handled by a new
component called a 'Zend_Acl_Resource_Locator'. It would look
something like:-
class Zend_Acl_Resource_Locator
{
public function __construct(Zend_Acl $acl, $id)
{
$this->_acl = $acl;
$this->_resource = $id;
}
public function __get($id)
{
if ($this->has($id)) {
$this->_resource = $id;
}
return $this;
}
public function __call($method, $args)
{
return call_user_func_array(array($this->_acl, $method),
$args);
}
public function has($id)
{
return $this->_acl->has($id) && $this->_acl->inherits($id,
$this->_resource);
}
public function isAllowed($role = null, $privilege = null)
{
return $this->_acl->isAllowed($role, $privilege, $this-
>_resource);
}
}
This is to accommodate the fact that the call can be made on either a
'real' resource or a 'virtual' resource without explicitly naming it.
I'll explain by showing a quick 'log'.
With the above query:- $acl->news->mycategory->id328->isAllowed
('sally', 'delete');
$acl->__get('news') :
'news' exists, so it returns an instance of
Zend_Acl_Resource_Locator ($arl for the sake of clarity)).
$arl->__get('mycategory') :
'mycategory' doesn't exist as a child of the current path
( = news), so the Zend_Acl_Resource_Locator returns an instance of
itself.
$arl->__get('id328') :
'id328' : as above.
$arl->isAllowed('sally', 'delete') :
Because 'news' is the last in the stack, it is the 'real'
resource that is queried through the Zend_Acl_Resource_Locator and
returns the appropriate response.
Potential problems with this method
There is, however, one problem with my proposal - naming conventions
and unique Ids. The above examples assume that we will always have
unique ids to access, but this isn't likely in the real world.
For example, if I now wish to do this:-
$acl->addRole(new Zend_Acl_Role('sally'));
$acl->addResource(new Zend_Acl_Resource('news')); // 'Real' resource
$acl->addResource(new Zend_Acl_Resource('article'), 'news'); //
'Real' resource inheriting 'news'
$acl->allow('sally', 'news'); // NOTE: We can only define rules on
'real' resources or 'null' meaning all.
$acl->deny('sally', 'news', 'delete'); As above
This works for $acl->news->article and it returns a 'real' resource.
But what if I wanted to create a unique resource for $acl->tutorial-
>article that also returns a 'real' resource? They can't both have
the unique name of 'article'!
My solution would be to allow ACL 'path' ids to sit alongside the
unique id:-
new Zend_Acl_Resource('news'); // Unique id == news, path id = news
new Zend_Acl_Resource(array('/news/index', 'index')); // Unique id = /
news/index, path = index
new Zend_Acl_Resource(array('ruleset328', 'index'), 'news'); //
Unique id = ruleset328, path = index, parent = news
This way a resource can be queried by one or both methods. The unique
id can still represent anything the developer wants, but the path id
is used to identify itself within a tree-like path structure. It
means we can have:-
$acl->addResource(new Zend_Acl_Resource('news')); // 'Real' resource
$acl->addResource(new Zend_Acl_Resource(array('newsarticle',
'article')), 'news'); // 'Real' resource inheriting 'news'
$acl->addResource(new Zend_Acl_Resource('tutorial')); // 'Real' resource
$acl->addResource(new Zend_Acl_Resource(array('tutorialarticle',
'article')), 'tutorial'); // 'Real' resource inheriting 'tutorial'
I think it can work - it allows flexibility and retains the
simplicity of the existing structure.
For these reasons and other questions that remain unanswered in my
mind,
it is difficult for me to immediately go to modifying the core
Zend_Acl
code, in light of the impending "API freeze" for 0.9.0 in two weeks. I
perceive a high risk of an instable API in two weeks if we were to
begin
down the path of supporting "virtual resources."
To be honest, I admit that I have not wrapped my mind around the total
related impact to the codebase. If the changes you seek are as minimal
as you seem to indicate, would you mind creating patches and attaching
them to a JIRA issue? Of course, we'll need unit tests and
documentation
to be able to include it with core. If doing this work in the
laboratory
would be more appropriate, I would be happy to facilitate this for
us, too.
I'll put my money where my mouth is and provide a lab version with
unit tests to cover the new functionality, but only if I get some
form of support for my proposal. After all, it'd be a whole lot of
effort to put into something that was rejected from the outset! :)
Would greatly appreciate both Darby's and other interested developers
feedback on this to make sure my thinking is straight and - more
importantly - worthwhile making changes to the existing infrastructure.
--
Simon Mundy | Director | PEPTOLAB
""" " "" """""" "" "" """"""" " "" """"" " """"" " """""" "" "
202/258 Flinders Lane | Melbourne | Victoria | Australia | 3000
Voice +61 (0) 3 9654 4324 | Mobile 0438 046 061 | Fax +61 (0) 3 9654
4124
http://www.peptolab.com
