Re: r745 - trunk/htdocs/system/classes/acl.php
Scott Merrill
> + * Return an array of the groups to which this user belongs
> + * @param int A user ID
> + * @return array An array of group IDs
> + **/
> + public static function user_group_list( $user_id )
Should we consider a more formal UserGroup class, to compliment the User
class with respect to ACLs? The above method feels to me like it
belongs in User, and would invoke something in UserGroup, perhaps.
--
GPG 9CFA4B35 | ski...@skippy.net | http://skippy.net/
Owen Winkler
>
> codesite...@google.com wrote:
> > + * Return an array of the groups to which this user belongs
> > + * @param int A user ID
> > + * @return array An array of group IDs
> > + **/
> > + public static function user_group_list( $user_id )
>
> Should we consider a more formal UserGroup class, to compliment the User
> class with respect to ACLs? The above method feels to me like it
> belongs in User, and would invoke something in UserGroup, perhaps.
Can you provide an example of the syntax you're thinking of?
Were you thinking like:
foreach($user->groups as $group) {
$group->do_something();
}
And what would do_something() be? Maybe $group->add_permission('foo')?
Or perhaps:
$group = ACL::get_groups(array('editors'));
$group->add_user('alice');
$group->remove_user('bob');
$group->grant('edit posts');
$group->deny('edit users');
$group->remove_permission('admin');
$user = User::identify();
$user->add_group('editors');
$user->remove_group('administrators');
Or something.
The primary reason that the function user_group_list() is in ACL is
because ACL caches all permissions when it is first loaded. So if the
User class implements ACL-related functions for purposes of clarity,
it will be more efficient to forward those requests on to the static
ACL methods than to move the function entirely to the User class.
Owen
Scott Merrill
> On 8/12/07, Scott Merrill <ski...@skippy.net> wrote:
>> codesite...@google.com wrote:
>>> + * Return an array of the groups to which this user belongs
>>> + * @param int A user ID
>>> + * @return array An array of group IDs
>>> + **/
>>> + public static function user_group_list( $user_id )
>> Should we consider a more formal UserGroup class, to compliment the User
>> class with respect to ACLs? The above method feels to me like it
>> belongs in User, and would invoke something in UserGroup, perhaps.
>
> Can you provide an example of the syntax you're thinking of?
> Were you thinking like:
>
> foreach($user->groups as $group) {
> $group->do_something();
> }
>
> And what would do_something() be? Maybe $group->add_permission('foo')?
The example above is one possible use for a dedicated UserGroup class.
Perhaps the better term should be "role". We assign permissions (or
actions) to roles, and then say that a user has a certain role.
The benefit for using a dedicated UserRole class would be for new users
learning how Habari implements permissions. They follow AdminHandler to
see that an HTTP POST sends some data specifying that UserX has RoleY.
They then look in UserRole.php to see how that actually occurs.
UserRole might well invoke static methods from the ACL class.
When we get UI to manipulate roles / groups / what-have-you, we ought to
have a class to properly handle that manipulation.
> The primary reason that the function user_group_list() is in ACL is
> because ACL caches all permissions when it is first loaded. So if the
> User class implements ACL-related functions for purposes of clarity,
> it will be more efficient to forward those requests on to the static
> ACL methods than to move the function entirely to the User class.
Sure, that's fine.
I suspect people will want to do non-ACL related things with groups / roles.
For example, say a role is made "email monitor". This role gets a BCC
copy of every email generated by Habari. This isn't strictly an ACL
related action, and would be handled by some plugin. But it seems odd
to invoke ACL::add_member_to_role('skippy', 'email monitor'). Seems
better to invoke Role::add_member('skippy', 'email monitor') or
something similar.
Bob
Hi Owen,
I am a PHP noob but I am trying to follow along on this class. How
long do static vars persist in PHP?
If persistence is long, is there a problem if permissions are revoked
during the persistence time?:
/**
* An array in which specific user permissions are cached as they are
built from the other structures.
**/
private static $user_permissions= array();
Also, what is the rationale for adding permissions as they are
requested, rather than populating $user_permissions with all the
permissions on the first request for that user?
Also, I don't understand why the parameter in this class is $group_id
instead of something like $group_name:
/**
* Return the id of a group, provided the name
* @param string $group_id The name of the group
* @return int The group id
**/
public static function group_id( $group_id )
{
if(!is_numeric($group_id)) {
$group_id = array_search($group_id, self::$group_names);
}
return $group_id;
}
Thanks,
Bob
Chris J. Davis
before. I will leave the answers on this one to Owen, since he is
more familiar with the ACL class than I am.
Chris
Owen Winkler
>
> I am a PHP noob but I am trying to follow along on this class. How
> long do static vars persist in PHP?
Static vars persist for the duration of script execution.
> If persistence is long, is there a problem if permissions are revoked
> during the persistence time?:
Yes, that is an issue. Functions that would allow you to change the
permissions for a group would have to either re-cache all permissions
or change the cached data specific to the changes. These functions
are not in the ACL class yet, but when they are written, I expect that
they will do this.
> Also, what is the rationale for adding permissions as they are
> requested, rather than populating $user_permissions with all the
> permissions on the first request for that user?
I'm a bit unclear on your question, but I think you're asking for the
rationale for caching user permissions (all permissions are cached
when the class is loaded, just not per-user) when making the first
request for a specific permission for a user.
The alternative would be computing the individual permissions for each
user when all permissions are cached. This could be an expensive
operation, as it would do the equivalent of ACL::user_can() for every
user when the class is created.
I'm not sure where else you would determine that a specific user's
permissions should be computed from role permissions. Have a
suggestion?
It seems sensible to me not to cache any permissions unless they're
requested, and it also seems sensible not to compute a user's
permissions from roles unless that user's permissions are specifically
queried.
> Also, I don't understand why the parameter in this class is $group_id
> instead of something like $group_name:
Uh, I was lazy in my cut and paste? :)
Patches to my sloppy code are welcome.
Owen
Bob
> > requested, rather than populating $user_permissions with all the
> > permissions on the first request for that user?
>
> I'm a bit unclear on your question, but I think you're asking for the
> rationale for caching user permissions (all permissions are cached
> when the class is loaded, just not per-user) when making the first
> request for a specific permission for a user.
>
> The alternative would be computing the individual permissions for each
> user when all permissions are cached. This could be an expensive
> operation, as it would do the equivalent of ACL::user_can() for every
> user when the class is created.
>
> I'm not sure where else you would determine that a specific user's
> permissions should be computed from role permissions. Have a
> suggestion?
>
> It seems sensible to me not to cache any permissions unless they're
> requested, and it also seems sensible not to compute a user's
> permissions from roles unless that user's permissions are specifically
> queried.
First of all, no, I never thought that it did or should build a
structure for the permissions of *all* users. That would be silly.
My question was based on a likely misunderstanding of your user_can
method. Looking at again, it looks to me like it doesn't only tell
you whether a user has a permission, but also populates
$user_permissions with *all* of this-user's permissions if it needs
to. Am I correct about that?
My previous (mis)understanding was that user_can added each requested
permission individually to the $user_permissions collection when asked
about that specific permission. But now I think what I'm seeing is
that it puts all the permissions into $user_permissions regardless of
what was requested.
I guess maybe I expected $user_permissions to be populated by a
separate function, even if that function is triggered by user_can.
For instance a user_permissions_get( $user_id ) and
user_permissions_set( $user_id ) pattern.
Sorry for the confusion. I am new to mentally parsing complex
associative arrays.
Owen Winkler
> method. Looking at again, it looks to me like it doesn't only tell
> you whether a user has a permission, but also populates
> $user_permissions with *all* of this-user's permissions if it needs
> to. Am I correct about that?
Yes, $user_permissions is populated with all of that user's
permissions upon the first call to user_can() for that user, and only
upon that first call.
> I guess maybe I expected $user_permissions to be populated by a
> separate function, even if that function is triggered by user_can.
> For instance a user_permissions_get( $user_id ) and
> user_permissions_set( $user_id ) pattern.
Using a separate function is probably a better way to go, and would
likely be required when additional functions are added to ACL to allow
modifications.
> Sorry for the confusion. I am new to mentally parsing complex
> associative arrays.
Not a problem. It's good to have people scrutinizing your code.
Owen