What does Controller_AACL::acl_instance() do?

[ghost]

1. the $this in static method results in exception being thrown upon usage;
2. what does "Manually override controller name and action" do? $instance->request is the current request singleton, and and its controller and action props are messed up by this for many times when AACL::list_resources(). Later my reverse routing depends on the current default values!

EDIT: alright I see Controller_ACL.acl_id() uses $this->request->controller. there must be better ways than the above one.

[banks]

acl_instance() is a slight hack which is required to make the auto discovery of ACL resources possible in list_resources. The problem is that you might legitimately have an abstract class as a resource but you can't simply construct this in order to query it's ACL methods (which must be instance vars). Further you can't simply construct a controller outside of the context of a request as it won't work so acl_instance is expected to return a resource instance ONLY for the purposes of calling it's acl methods for resource discovery. As such it doesn't matter if the object returned is essentially useless as lons it's acl methods will return correct values.

In the controller Acl resource class you must construct a controller with the current request however if the controller is NOT the actual controller that is executing the controller's request object must be modified to make the acl_methods work.

Reviewing this code you are right that it will break things because it passes Request::instance() as the object then alters it! clearly it should be passing a clone of Request::instance() so that altering only affects the temporarily created contoller.

Also note that I've not found this a problem in tests because you shouldn't really use list_resources() in anything other than an admin interface for setting up rules (i.e. in a controller action/library). By the time you would typically use it, all routing has been done and so (although not right) messing with the controller/action of the global request instance doesn't actually have any effect.

The $this is also clearly a bug. Not sure how I didn't spot that. It has been a while since I looked at this code!

[ghost]

By now I'm very happy with the high configurability brought by list_resources(), using it to set up a big and complex table to illustrate and configure the rules. But there is still one thing puzzling me, it's the acl_instance(). I don't quite understand what difference it really makes. Though I tried to make several tweaks to it, I'm still not sure if I'm doing right. I just want to make it clear here.

The acl_instance() method effectively only constructs an instance of controller, and "overrides" the current request context. Abstract classes are ignored prior to invocation of this method, so they don't really matter. All acl_methods of the object returned by acl_instance() used in static list_resources() have static outputs independent from the request context, while we don't need to manually override the context when these acl_methods are called in check().

Then my question is: is it ok if I just don't do anything to the current context in acl_instance(), or simply use new instead of acl_instance() ?

[banks]

Hmmm, I seem to remember there was a good reason for including this in the interface. I guess it decouples the logic so you could make any class a resource even if it had dependecies in the constructor (i.e. controller does - you must pass the constructor a request object for instance).

The return values form the acl methods usually are static in this context but not when evaluating them and I (perhaps foolishly) used the same methods for both.

I think you are right about this. I think the acl definition functions in the interface should be separate from the checking functions and made static. That should solve this and I'm really not sure now why I didn't design it that way.

So acl_action() is an instance method and returns the CURRENT action whereas static acl_actions() returns an array of supported actions for the resource.

Similarly with acl_check_condition(Model_User $user, $condition) and static acl_conditions(). That would mean there is no need at all for acl_instance() since all discovery in

wait a sec...

No I did do it this way for a reason. I've left the above discussion in so you can see my thought process. The point where static access falls down is acl_id() since for a model you want this to be unique down to the actual primary key value which means it must be an instance method (when checking rules). Now in the context of list_resources() there is no PK information since we can only automatically discover the model classes not all the possible primary key values but to make a static accessor for ACL ID for list_resources() and then duplicate the code with an instance accessor is also a nasty hack and confusing at that. Perhaps it is a better solution but it seemed not at the time.

I guess the idea of acl_instance() was that no matter how you decided to define your class - whether you inject dependencies in the constructor or not, it would always be possible to set up a class instance that would return the correct ACL actions, ID and conditions for auto discovery. Just calling the constructor directly (using new or similar) would limit the class you could make into resources to only ones that require no arguments to the constructor (i.e. controllers couldn't be resources since they require a request object to be passed in).

Now in Controller_AACL::acl_instance() the reason for overriding the Request->controller and action was because Controller_AACL::acl_id() relies on $this->request->controller to be set to the correct controller to get the ID right! So when discovering controllers that aren't the actual one executing you need to pass in a dummy request with the correct controller set otherwise the IDs will all come back as the currnely executing controller. This is a bit nasty and I guess it would be better to make Controller_AACL::acl_id() return the ID based only on the current class name:

// Controller namespace, controller name
    return 'c:'.strtolower(substr(get_class($this), 11));

Then you could make Controller_AACL::acl_instance() just this:

public static function acl_instance($class_name)
    {
            // Return controller instance populated with manipulated request details
    // The request object does nothing in this instance but is a dependency of the controller class
            $instance = new $class_name(Request::instance());

            return $instance;
    }

Can you try with these two simple changes and see if that works out for you? If so I'll pull the changes because it is clearly wrong as it is.

[ghost]

You're right, I've overlooked the fact acl_instance() is necessary for decoupling the interface from any constructor logic, and acl_id() is non-static for model because there is a pk part. The suggestion in the end is quite intuitive that I've done exactly the same way and it works fine! Now my puzzlement is resolved.

I think you can put off all the changes to when working with a jelly version.

A little off-topic stuff. The following is a list of my modifications to aacl by now; they may be hopefully helpful, or trivial, or even wrong:

• c: changed to c. (m: to m.), then rules can be expressed in this way: "$role:$resource:$action:$condition". feel like : is more delimiting than ., and they are both legal in css id attribute, just personal favor.
• NULL resource made for any resource, NULL role for any role, in accordance with NULL action and NULL condition.
  • changes in rule querying and matching in respect of NULL
  • extra care paid for dealing with rule overlapping and inclusion
  • anonymous is now inclusively represented by NULL role, so 401 exception is useless.
• check() accepts string $resource as acl_id (then get_resource($acl_id)).
• check() accepts array $actions to do batch checking (_check()).
• check() just returns the result, users throw 403 exception or not on their own, instead of trying and catching every time. use case: certain views are rendered or hidden according to user privileges.
• list_resources() ignores resources with no action.
• new list_rules(), return a multidimensional array.
• create an $acl_actions to configure acl_actions() rather than overload it. use case: specify which actions are to be checked.
• $this->acl_check() equivalent to ACL::check($this) as (kind of) shorthand in a better sense of "checking myself with my own method"

Oops I don't feel I have the capacity or experience to maintain a fork, so I just leave it here.

[ghost]

close this issue