UserGroup
Scott Merrill
First, there is no current way to create or destory permissions. We
can grant and revoke permissions to and from groups, but there's no
way to make new permissions.
Second, after working with this code for a bit, I think the UserGroup
class should extend the QueryRecord class, so that we can enjoy all
the OOPy goodness it offers, rather than rely on a bunch of static
method invocations.
Consider the current way:
UserGroup::add_group( 'admins' );
UserGroup::grant_permission( 'admins', 'create users', false );
UserGroup::add_member( 'admins', 'skippy' );
Wouldn't you all rather do this?
$admins= new UserGroup( array( 'name' => 'admins' ) );
$admins->grant('create users');
$admins->add('skippy');
$admins->update();
Attached is a modified UserGroup class. It is woefully incomplete,
but I think it's a start on the right path. Feedback and improvements
are encouraged.
I _think_ we might want to make a UserGroups (note the plural) class
which is primarily static methods, which would provide API for working
with sets of user groups. Discussion on this is welcome.
Cheers,
Scott
epi...@gmail.com
approve of the concept presented. I think that taking groups and
permissions in this direction would be the most reasonable way
forward.
Owen
Scott Merrill
Do folks have any opinion on how to tackle this? I can see several options:
1) The ACL class has static methods to create and destroy permissions:
ACL::create_permission() and ACL::delete_permission()
2) Permissions are created on the fly, as needed.
$admins->grant('foobar') would create the permisson "foobar" if it
does not yet exist. Ostensibly, when no groups use a permission, it
could be deleted from the permissions table. This is sub-optimal, and
does not provide decent support for permission descriptions.
3) A new permissions class is created. This seems a little overkill.
I _think_ option #1 is the best way forward, but it seems a little
brittle for reasons I can't articulate.
Feedback, please.
Michael C. Harris
#1 seems reasonable to me. I would expect ACL::create_permissions() to
return true if the permission exists after the call, regardless of
whether it was created by the call or existed previously. Same for
ACL::delete_permissions, but reverse.
Related musing (that might actually be in favour of #3); do we want to
have some automatically generated permissions, such as
view_status_{type}, publish_status_{type},
publish_content_type_{type}. That way I can have a plugin add a status
or content type and have a bunch of other permissions that
automatically exist. Another advantage of that would be ensuring
consistency of permission naming, so that we minimise the permissions
plugins need to create, and don't end up with view_private,
view_status_foobar, barbaz_can_view etc.
cheers, Michael
--
Michael C. Harris, School of CS&IT, RMIT University
http://twofishcreative.com/michael/blog
Michael C. Harris
>
> ... UserGroup
> class should extend the QueryRecord class, so that we can enjoy all
> the OOPy goodness it offers, rather than rely on a bunch of static
> method invocations.
> $admins->grant('create users');
> $admins->add('skippy');
> $admins->update();
This is all good. It might be good to be able to pass a single user or
an array of users to ->add(), as below.
$admins->add('skippy');
$admins->add(array('skippy','miklb'));
Same with ->remove(). I don't have time to patch right now, but will
do so if people think that's a good/not a bad idea.
Scott Merrill
>
> This is all good. It might be good to be able to pass a single user or
> an array of users to ->add(), as below.
>
> $admins->add('skippy');
> $admins->add(array('skippy','miklb'));
>
> Same with ->remove(). I don't have time to patch right now, but will
> do so if people think that's a good/not a bad idea.
This is a very good idea. Also, the current implementation only
supports user IDs, and not user names. It's probably worth the extra
few lines of code to fetch a user's ID so that one can pass string
names to the methods.
Chris J. Davis
Michael's additions.
Scott Merrill
> On Sun, Jan 27, 2008 at 12:02:17PM -0500, Scott Merrill wrote:
> > 1) The ACL class has static methods to create and destroy permissions:
> > ACL::create_permission() and ACL::delete_permission()
> #1 seems reasonable to me. I would expect ACL::create_permissions() to
> return true if the permission exists after the call, regardless of
> whether it was created by the call or existed previously. Same for
> ACL::delete_permissions, but reverse.
I'm not entirely sure about that. Certainly ACL::create_permission()
ought not duplicate an existing permission (ie: it should first check
to see if it exists before creating it); but I'm not sure that
returning true if it already exists is the right choice. Consider
something like:
if ( ACL::create_permission('foobar', 'description') ) {
EventLog::log('Added permission foobar'); }
Perhaps that's an overly trivial objection, though.
> Related musing (that might actually be in favour of #3); do we want to
> have some automatically generated permissions, such as
> view_status_{type}, publish_status_{type},
> publish_content_type_{type}. That way I can have a plugin add a status
> or content type and have a bunch of other permissions that
> automatically exist. Another advantage of that would be ensuring
> consistency of permission naming, so that we minimise the permissions
> plugins need to create, and don't end up with view_private,
> view_status_foobar, barbaz_can_view etc.
I actually _want_ "view_status_foobar" permissions. It allows for
groups to have content types usable and visible only to them, which is
extremely helpful on community sites and author/editor/publisher
workflows.
I don't think we should go so far as to automatically create
permissions for each content type created -- let those objects that
create the content types decide for themselves what additional
permissions they need.
Also, as part of this discussion, we need to consider how to actually
implement the permissions. The UserGroup::can() method in my most
recent patch has been modified to return boolean true/false based on
the permissions assigned to the current group. I think this actually
needs to be a ternary operation: granted, denied, or neither. Then,
$user->can() should compute the results of each of that user's groups'
permissions: if there is an explicit "deny" response from any group,
then the action is denied regardless of the presence of any number of
"grant" responses.
Stated differently: Deny ACLs explicitly trump all other granted
privileges. Membership is any group that denies access to something
means you do not have access to it, regardless of how many groups
you're in that grant you access to that thing.
Let's coordinate the discussion and development over on the Trac
ticket I opened:
http://trac.habariproject.org/habari/ticket/132
Attached to that ticket is the latest work-in-progress. Everyone
please feel free to bend, fold and manipulate that patch as needed to
implement the fruits of this discussion.
Michael C. Harris
>
> On 1/27/08, Michael C. Harris <michael...@gmail.com> wrote:
> > On Sun, Jan 27, 2008 at 12:02:17PM -0500, Scott Merrill wrote:
> > > 1) The ACL class has static methods to create and destroy permissions:
> > > ACL::create_permission() and ACL::delete_permission()
> ...
> > #1 seems reasonable to me. I would expect ACL::create_permissions() to
> > return true if the permission exists after the call, regardless of
> > whether it was created by the call or existed previously. Same for
> > ACL::delete_permissions, but reverse.
>
> I'm not entirely sure about that. Certainly ACL::create_permission()
> ought not duplicate an existing permission (ie: it should first check
> to see if it exists before creating it); but I'm not sure that
> returning true if it already exists is the right choice. Consider
> something like:
> if ( ACL::create_permission('foobar', 'description') ) {
> EventLog::log('Added permission foobar'); }
Then the coder has misunderstood the API (though ACL should probably
log creation of permissions).
The alternative is that the check moves to the calling code, because
otherwise the coder can't tell why the call failed - because the
permission already existed or because the addition failed for some
reason - and therefore won't know how to recover from it. In the first
instance it's likely that execution can continue because all I need is
for the permission to exist, but in the second something is wrong and
I may need to terminate and report it to the user.
I'm happy to hear arguments either way.
(Hmmm, do we need to worry about a clash of permission names? My
plugin uses permission foobar for this, your plugin independently uses
it for that. Namespaced permissions?)
> > Do we want to have some automatically generated permissions, such
> > as view_status_{type}, publish_status_{type},
> > publish_content_type_{type}. That way I can have a plugin add a
> > status or content type and have a bunch of other permissions that
> > automatically exist. Another advantage of that would be ensuring
> > consistency of permission naming, so that we minimise the
> > permissions plugins need to create, and don't end up with
> > view_private, view_status_foobar, barbaz_can_view etc.
>
> I actually _want_ "view_status_foobar" permissions.
Me too, hence my thought of automatic permissions. My point was about
a proliferation of permission formats. All three permissions in my
example were effectively view_status_{type}, and I was trying to
illustrate that it might become messy. If my plugin creates a status
it's likely because I want to do something ACL related (or style
the post differently), so why make plugins that create a status also
create view_status_{type} and publish_status_{type}. Anyway, I was
just throwing the thought out there.
Perhaps we just need to communicate best practice.
> Deny ACLs explicitly trump all other granted privileges. Membership
> is any group that denies access to something means you do not have
> access to it, regardless of how many groups you're in that grant you
> access to that thing.
+1
> Let's coordinate the discussion and development over on the Trac
> ticket I opened: http://trac.habariproject.org/habari/ticket/132
> Attached to that ticket is the latest work-in-progress. Everyone
> please feel free to bend, fold and manipulate that patch as needed
> to implement the fruits of this discussion.
I hope to be able to get to some time to work on this this afternoon
(specifically adding and removing members). I've been trying to think
how the UI for the groups/permissions/members might work, but my brain
exploded.
Scott Merrill
> > > return true if the permission exists after the call, regardless of
> > > whether it was created by the call or existed previously. Same for
> > > ACL::delete_permissions, but reverse.
> >
> > I'm not entirely sure about that. Certainly ACL::create_permission()
> > ought not duplicate an existing permission (ie: it should first check
> > to see if it exists before creating it); but I'm not sure that
> > returning true if it already exists is the right choice. Consider
> > something like:
> > if ( ACL::create_permission('foobar', 'description') ) {
> > EventLog::log('Added permission foobar'); }
>
> Then the coder has misunderstood the API (though ACL should probably
> log creation of permissions).
Fair enough. I guess I'm not passionate either way:
ACL::create_permission() can be fire-and-forget, happily doing nothing
when a permission exists; or it can require that the programmer check
first for himself whether the target permission exists.
Six of one, half dozen of the other, in most respects. Whatever is
easier to code is probably worth pursuing until specific
counter-examples can be produced.
> (Hmmm, do we need to worry about a clash of permission names? My
> plugin uses permission foobar for this, your plugin independently uses
> it for that. Namespaced permissions?)
I don't think we can meaningfully enforce this, unless
ACL::create_permission() can deduce that it's being invoked by a
plugin and then prepend that plugin name to the permission. But then
checks for that permission would need to also include the plugin name
prefix.
I think for now we'll have to settle with documenting how best to name
one's permissions to avoid a conflict.
> > > Do we want to have some automatically generated permissions, such
> > > as view_status_{type}, publish_status_{type},
> > > publish_content_type_{type}. That way I can have a plugin add a
> > > status or content type and have a bunch of other permissions that
> > > automatically exist. Another advantage of that would be ensuring
> > > consistency of permission naming, so that we minimise the
> > > permissions plugins need to create, and don't end up with
> > > view_private, view_status_foobar, barbaz_can_view etc.
> >
> > I actually _want_ "view_status_foobar" permissions.
>
> Me too, hence my thought of automatic permissions. My point was about
> a proliferation of permission formats. All three permissions in my
> example were effectively view_status_{type}, and I was trying to
> illustrate that it might become messy. If my plugin creates a status
> it's likely because I want to do something ACL related (or style
> the post differently), so why make plugins that create a status also
> create view_status_{type} and publish_status_{type}. Anyway, I was
> just throwing the thought out there.
Oh, do you mean for core permissions that we programatically provide
permissions without requiring an explicit step from the plugin author?
We simply compute against * in the post_status and post_type tables
for create_content_X, edit_content_X, etc? I'd support that notion.
Cheers,
Scott
Michael C. Harris
[snip stuff we agree on]
> > > > Do we want to have some automatically generated permissions, such
> > > > as view_status_{type}, publish_status_{type},
> > > > publish_content_type_{type}. That way I can have a plugin add a
> > > > status or content type and have a bunch of other permissions that
> > > > automatically exist. Another advantage of that would be ensuring
> > > > consistency of permission naming, so that we minimise the
> > > > permissions plugins need to create, and don't end up with
> > > > view_private, view_status_foobar, barbaz_can_view etc.
> > >
> > > I actually _want_ "view_status_foobar" permissions.
> >
> > Me too, hence my thought of automatic permissions. My point was about
> > a proliferation of permission formats. All three permissions in my
> > example were effectively view_status_{type}, and I was trying to
> > illustrate that it might become messy. If my plugin creates a status
> > it's likely because I want to do something ACL related (or style
> > the post differently), so why make plugins that create a status also
> > create view_status_{type} and publish_status_{type}. Anyway, I was
> > just throwing the thought out there.
>
> Oh, do you mean for core permissions that we programatically provide
> permissions without requiring an explicit step from the plugin author?
> We simply compute against * in the post_status and post_type tables
> for create_content_X, edit_content_X, etc? I'd support that notion.
Yes, if a plugin author creates a status or a content type, a small
set of permissions is automatically created.