projects / freeside.git / blobdiff
? search:
summary | shortlog | log | commit | commitdiff | tree
raw | inline | side by side
import rt 3.8.10
[freeside.git] / rt / lib / RT / Group_Overlay.pm
diff --git a/rt/lib/RT/Group_Overlay.pm b/rt/lib/RT/Group_Overlay.pm
index 69ada31..09f3082 100644 (file)
--- a/rt/lib/RT/Group_Overlay.pm
+++ b/rt/lib/RT/Group_Overlay.pm
@@ -1,39 +1,41 @@
 
 
-# {{{ BEGIN BPS TAGGED BLOCK
-# 
+# BEGIN BPS TAGGED BLOCK {{{
+#
 # COPYRIGHT:
 # COPYRIGHT:
-#  
-# This software is Copyright (c) 1996-2004 Best Practical Solutions, LLC 
-#                                          <jesse@bestpractical.com>
-# 
+#
+# This software is Copyright (c) 1996-2011 Best Practical Solutions, LLC
+#                                          <sales@bestpractical.com>
+#
 # (Except where explicitly superseded by other copyright notices)
 # (Except where explicitly superseded by other copyright notices)
-# 
-# 
+#
+#
 # LICENSE:
 # LICENSE:
-# 
+#
 # This work is made available to you under the terms of Version 2 of
 # the GNU General Public License. A copy of that license should have
 # been provided with this software, but in any event can be snarfed
 # from www.gnu.org.
 # This work is made available to you under the terms of Version 2 of
 # the GNU General Public License. A copy of that license should have
 # been provided with this software, but in any event can be snarfed
 # from www.gnu.org.
-# 
+#
 # This work is distributed in the hope that it will be useful, but
 # WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
 # General Public License for more details.
 # This work is distributed in the hope that it will be useful, but
 # WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
 # General Public License for more details.
-# 
+#
 # You should have received a copy of the GNU General Public License
 # along with this program; if not, write to the Free Software
 # You should have received a copy of the GNU General Public License
 # along with this program; if not, write to the Free Software
-# Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
-# 
-# 
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+# 02110-1301 or visit their web page on the internet at
+# http://www.gnu.org/licenses/old-licenses/gpl-2.0.html.
+#
+#
 # CONTRIBUTION SUBMISSION POLICY:
 # CONTRIBUTION SUBMISSION POLICY:
-# 
+#
 # (The following paragraph is not intended to limit the rights granted
 # to you to modify and distribute this software under the terms of
 # the GNU General Public License and is only of importance to you if
 # you choose to contribute your changes and enhancements to the
 # community by submitting them to Best Practical Solutions, LLC.)
 # (The following paragraph is not intended to limit the rights granted
 # to you to modify and distribute this software under the terms of
 # the GNU General Public License and is only of importance to you if
 # you choose to contribute your changes and enhancements to the
 # community by submitting them to Best Practical Solutions, LLC.)
-# 
+#
 # By intentionally submitting any modifications, corrections or
 # derivatives to this work, or any other work intended for use with
 # Request Tracker, to Best Practical Solutions, LLC, you confirm that
 # By intentionally submitting any modifications, corrections or
 # derivatives to this work, or any other work intended for use with
 # Request Tracker, to Best Practical Solutions, LLC, you confirm that
@@ -42,8 +44,9 @@
 # royalty-free, perpetual, license to use, copy, create derivative
 # works based on those contributions, and sublicense and distribute
 # those contributions and any derivatives thereof.
 # royalty-free, perpetual, license to use, copy, create derivative
 # works based on those contributions, and sublicense and distribute
 # those contributions and any derivatives thereof.
-# 
-# }}} END BPS TAGGED BLOCK
+#
+# END BPS TAGGED BLOCK }}}
+
 # Released under the terms of version 2 of the GNU Public License
 
 =head1 NAME
 # Released under the terms of version 2 of the GNU Public License
 
 =head1 NAME
@@ -52,107 +55,23 @@
 
 =head1 SYNOPSIS
 
 
 =head1 SYNOPSIS
 
-  use RT::Group;
+use RT::Group;
 my $group = new RT::Group($CurrentUser);
 
 =head1 DESCRIPTION
 
 An RT group object.
 
 my $group = new RT::Group($CurrentUser);
 
 =head1 DESCRIPTION
 
 An RT group object.
 
-=head1 AUTHOR
-
-Jesse Vincent, jesse@bestpractical.com
-
-=head1 SEE ALSO
-
-RT
-
 =head1 METHODS
 
 
 =head1 METHODS
 
 
-=begin testing
-
-# {{{ Tests
-ok (require RT::Group);
-
-ok (my $group = RT::Group->new($RT::SystemUser), "instantiated a group object");
-ok (my ($id, $msg) = $group->CreateUserDefinedGroup( Name => 'TestGroup', Description => 'A test group',
-                    ), 'Created a new group');
-ok ($id != 0, "Group id is $id");
-ok ($group->Name eq 'TestGroup', "The group's name is 'TestGroup'");
-my $ng = RT::Group->new($RT::SystemUser);
-
-ok($ng->LoadUserDefinedGroup('TestGroup'), "Loaded testgroup");
-ok(($ng->id == $group->id), "Loaded the right group");
-
-
-ok (($id,$msg) = $ng->AddMember('1'), "Added a member to the group");
-ok($id, $msg);
-ok (($id,$msg) = $ng->AddMember('2' ), "Added a member to the group");
-ok($id, $msg);
-ok (($id,$msg) = $ng->AddMember('3' ), "Added a member to the group");
-ok($id, $msg);
-
-# Group 1 now has members 1, 2 ,3
-
-my $group_2 = RT::Group->new($RT::SystemUser);
-ok (my ($id_2, $msg_2) = $group_2->CreateUserDefinedGroup( Name => 'TestGroup2', Description => 'A second test group'), , 'Created a new group');
-ok ($id_2 != 0, "Created group 2 ok- $msg_2 ");
-ok (($id,$msg) = $group_2->AddMember($ng->PrincipalId), "Made TestGroup a member of testgroup2");
-ok($id, $msg);
-ok (($id,$msg) = $group_2->AddMember('1' ), "Added  member RT_System to the group TestGroup2");
-ok($id, $msg);
-
-# Group 2 how has 1, g1->{1, 2,3}
-
-my $group_3 = RT::Group->new($RT::SystemUser);
-ok (($id_3, $msg) = $group_3->CreateUserDefinedGroup( Name => 'TestGroup3', Description => 'A second test group'), 'Created a new group');
-ok ($id_3 != 0, "Created group 3 ok - $msg");
-ok (($id,$msg) =$group_3->AddMember($group_2->PrincipalId), "Made TestGroup a member of testgroup2");
-ok($id, $msg);
 
 
-# g3 now has g2->{1, g1->{1,2,3}}
-
-my $principal_1 = RT::Principal->new($RT::SystemUser);
-$principal_1->Load('1');
-
-my $principal_2 = RT::Principal->new($RT::SystemUser);
-$principal_2->Load('2');
-
-ok (($id,$msg) = $group_3->AddMember('1' ), "Added  member RT_System to the group TestGroup2");
-ok($id, $msg);
-
-# g3 now has 1, g2->{1, g1->{1,2,3}}
-
-ok($group_3->HasMember($principal_2) == undef, "group 3 doesn't have member 2");
-ok($group_3->HasMemberRecursively($principal_2), "group 3 has member 2 recursively");
-ok($ng->HasMember($principal_2) , "group ".$ng->Id." has member 2");
-my ($delid , $delmsg) =$ng->DeleteMember($principal_2->Id);
-ok ($delid !=0, "Sucessfully deleted it-".$delid."-".$delmsg);
-
-#Gotta reload the group objects, since we've been messing with various internals.
-# we shouldn't need to do this.
-#$ng->LoadUserDefinedGroup('TestGroup');
-#$group_2->LoadUserDefinedGroup('TestGroup2');
-#$group_3->LoadUserDefinedGroup('TestGroup');
-
-# G1 now has 1, 3
-# Group 2 how has 1, g1->{1, 3}
-# g3 now has  1, g2->{1, g1->{1, 3}}
-
-ok(!$ng->HasMember($principal_2)  , "group ".$ng->Id." no longer has member 2");
-ok($group_3->HasMemberRecursively($principal_2) == undef, "group 3 doesn't have member 2");
-ok($group_2->HasMemberRecursively($principal_2) == undef, "group 2 doesn't have member 2");
-ok($ng->HasMember($principal_2) == undef, "group 1 doesn't have member 2");;
-ok($group_3->HasMemberRecursively($principal_2) == undef, "group 3 has member 2 recursively");
-
-# }}}
 
 
-=end testing
 
 
+=cut
 
 
 
 
-=cut
+package RT::Group;
 
 use strict;
 no warnings qw(redefine);
 
 use strict;
 no warnings qw(redefine);
@@ -168,9 +87,17 @@ $RIGHTS = {
     AdminGroup           => 'Modify group metadata or delete group',  # loc_pair
     AdminGroupMembership =>
       'Modify membership roster for this group',                      # loc_pair
     AdminGroup           => 'Modify group metadata or delete group',  # loc_pair
     AdminGroupMembership =>
       'Modify membership roster for this group',                      # loc_pair
+    DelegateRights =>
+        "Delegate specific rights which have been granted to you.",   # loc_pair
     ModifyOwnMembership => 'Join or leave this group',                 # loc_pair
     EditSavedSearches => 'Edit saved searches for this group',        # loc_pair
     ShowSavedSearches => 'Display saved searches for this group',        # loc_pair
     ModifyOwnMembership => 'Join or leave this group',                 # loc_pair
     EditSavedSearches => 'Edit saved searches for this group',        # loc_pair
     ShowSavedSearches => 'Display saved searches for this group',        # loc_pair
+    SeeGroup => 'Make this group visible to user',                    # loc_pair
+
+    SeeGroupDashboard       => 'View dashboards for this group', #loc_pair
+    CreateGroupDashboard    => 'Create dashboards for this group', #loc_pair
+    ModifyGroupDashboard    => 'Modify dashboards for this group', #loc_pair
+    DeleteGroupDashboard    => 'Delete dashboards for this group', #loc_pair
 };
 
 # Tell RT::ACE that this sort of object can get acls granted
 };
 
 # Tell RT::ACE that this sort of object can get acls granted
@@ -186,6 +113,20 @@ foreach my $right ( keys %{$RIGHTS} ) {
     $RT::ACE::LOWERCASERIGHTNAMES{ lc $right } = $right;
 }
 
     $RT::ACE::LOWERCASERIGHTNAMES{ lc $right } = $right;
 }
 
+=head2 AddRights C<RIGHT>, C<DESCRIPTION> [, ...]
+
+Adds the given rights to the list of possible rights.  This method
+should be called during server startup, not at runtime.
+
+=cut
+
+sub AddRights {
+    my $self = shift;
+    my %new = @_;
+    $RIGHTS = { %$RIGHTS, %new };
+    %RT::ACE::LOWERCASERIGHTNAMES = ( %RT::ACE::LOWERCASERIGHTNAMES,
+                                      map { lc($_) => $_ } keys %new);
+}
 
 =head2 AvailableRights
 
 
 =head2 AvailableRights
 
@@ -257,7 +198,6 @@ sub Load {
     my $self       = shift;
     my $identifier = shift || return undef;
 
     my $self       = shift;
     my $identifier = shift || return undef;
 
-    #if it's an int, load by id. otherwise, load by name.
     if ( $identifier !~ /\D/ ) {
         $self->SUPER::LoadById($identifier);
     }
     if ( $identifier !~ /\D/ ) {
         $self->SUPER::LoadById($identifier);
     }
@@ -283,34 +223,44 @@ sub LoadUserDefinedGroup {
     my $self       = shift;
     my $identifier = shift;
 
     my $self       = shift;
     my $identifier = shift;
 
-        $self->LoadByCols( "Domain" => 'UserDefined',
-                           "Name" => $identifier );
+    if ( $identifier =~ /^\d+$/ ) {
+        return $self->LoadByCols(
+            Domain => 'UserDefined',
+            id     => $identifier,
+        );
+    } else {
+        return $self->LoadByCols(
+            Domain => 'UserDefined',
+            Name   => $identifier,
+        );
+    }
 }
 
 # }}}
 
 # {{{ sub LoadACLEquivalenceGroup 
 
 }
 
 # }}}
 
 # {{{ sub LoadACLEquivalenceGroup 
 
-=head2 LoadACLEquivalenceGroup  PRINCIPAL
+=head2 LoadACLEquivalenceGroup PRINCIPAL
 
 
-Loads a user's acl equivalence group. Takes a principal object.
+Loads a user's acl equivalence group. Takes a principal object or its ID.
 ACL equivalnce groups are used to simplify the acl system. Each user
 has one group that only he is a member of. Rights granted to the user
 are actually granted to that group. This greatly simplifies ACL checks.
 While this results in a somewhat more complex setup when creating users
 and granting ACLs, it _greatly_ simplifies acl checks.
 
 ACL equivalnce groups are used to simplify the acl system. Each user
 has one group that only he is a member of. Rights granted to the user
 are actually granted to that group. This greatly simplifies ACL checks.
 While this results in a somewhat more complex setup when creating users
 and granting ACLs, it _greatly_ simplifies acl checks.
 
-
-
 =cut
 
 sub LoadACLEquivalenceGroup {
 =cut
 
 sub LoadACLEquivalenceGroup {
-    my $self       = shift;
-    my $princ = shift;
+    my $self = shift;
+    my $principal = shift;
+    $principal = $principal->id if ref $principal;
 
 
-        $self->LoadByCols( "Domain" => 'ACLEquivalence',
-                            "Type" => 'UserEquiv',
-                           "Instance" => $princ->Id);
+    return $self->LoadByCols(
+        Domain   => 'ACLEquivalence',
+        Type     => 'UserEquiv',
+        Instance => $principal,
+    );
 }
 
 # }}}
 }
 
 # }}}
@@ -351,8 +301,10 @@ sub LoadSystemInternalGroup {
     my $self       = shift;
     my $identifier = shift;
 
     my $self       = shift;
     my $identifier = shift;
 
-        $self->LoadByCols( "Domain" => 'SystemInternal',
-                           "Type" => $identifier );
+    return $self->LoadByCols(
+        Domain => 'SystemInternal',
+        Type   => $identifier,
+    );
 }
 
 # }}}
 }
 
 # }}}
@@ -435,6 +387,7 @@ sub LoadSystemRoleGroup {
 # }}}
 
 # {{{ sub Create
 # }}}
 
 # {{{ sub Create
+
 =head2 Create
 
 You need to specify what sort of group you're creating by calling one of the other
 =head2 Create
 
 You need to specify what sort of group you're creating by calling one of the other
@@ -469,6 +422,7 @@ sub _Create {
         Type        => undef,
         Instance    => '0',
         InsideTransaction => undef,
         Type        => undef,
         Instance    => '0',
         InsideTransaction => undef,
+        _RecordTransaction => 1,
         @_
     );
 
         @_
     );
 
@@ -493,13 +447,14 @@ sub _Create {
     );
     my $id = $self->Id;
     unless ($id) {
     );
     my $id = $self->Id;
     unless ($id) {
+        $RT::Handle->Rollback() unless ($args{'InsideTransaction'});
         return ( 0, $self->loc('Could not create group') );
     }
 
     # If we couldn't create a principal Id, get the fuck out.
     unless ($principal_id) {
         $RT::Handle->Rollback() unless ($args{'InsideTransaction'});
         return ( 0, $self->loc('Could not create group') );
     }
 
     # If we couldn't create a principal Id, get the fuck out.
     unless ($principal_id) {
         $RT::Handle->Rollback() unless ($args{'InsideTransaction'});
-        $self->crit( "Couldn't create a Principal on new user create. Strange things are afoot at the circle K" );
+        $RT::Logger->crit( "Couldn't create a Principal on new user create. Strange things are afoot at the circle K" );
         return ( 0, $self->loc('Could not create group') );
     }
 
         return ( 0, $self->loc('Could not create group') );
     }
 
@@ -514,8 +469,12 @@ sub _Create {
     $cgm->Create(Group =>$self->PrincipalObj, Member => $self->PrincipalObj, ImmediateParent => $self->PrincipalObj);
 
 
     $cgm->Create(Group =>$self->PrincipalObj, Member => $self->PrincipalObj, ImmediateParent => $self->PrincipalObj);
 
 
+    if ( $args{'_RecordTransaction'} ) {
+        $self->_NewTransaction( Type => "Create" );
+    }
 
     $RT::Handle->Commit() unless ($args{'InsideTransaction'});
 
     $RT::Handle->Commit() unless ($args{'InsideTransaction'});
+
     return ( $id, $self->loc("Group created") );
 }
 
     return ( $id, $self->loc("Group created") );
 }
 
@@ -566,7 +525,8 @@ sub _CreateACLEquivalenceGroup {
                            Name => 'User '. $princ->Object->Id,
                            Description => 'ACL equiv. for user '.$princ->Object->Id,
                            Instance => $princ->Id,
                            Name => 'User '. $princ->Object->Id,
                            Description => 'ACL equiv. for user '.$princ->Object->Id,
                            Instance => $princ->Id,
-                           InsideTransaction => 1);
+                           InsideTransaction => 1,
+                           _RecordTransaction => 0 );
       unless ($id) {
         $RT::Logger->crit("Couldn't create ACL equivalence group");
         return undef;
       unless ($id) {
         $RT::Logger->crit("Couldn't create ACL equivalence group");
         return undef;
@@ -710,6 +670,7 @@ If passed a positive value, this group will be disabled. No rights it commutes o
 It will not appear in most group listings.
 
 This routine finds all the cached group members that are members of this group  (recursively) and disables them.
 It will not appear in most group listings.
 
 This routine finds all the cached group members that are members of this group  (recursively) and disables them.
+
 =cut 
 
  # }}}
 =cut 
 
  # }}}
@@ -754,7 +715,7 @@ This routine finds all the cached group members that are members of this group
 
     #Clear the key cache. TODO someday we may want to just clear a little bit of the keycache space. 
     # TODO what about the groups key cache?
 
     #Clear the key cache. TODO someday we may want to just clear a little bit of the keycache space. 
     # TODO what about the groups key cache?
-    RT::Principal->_InvalidateACLCache();
+    RT::Principal->InvalidateACLCache();
 
 
 
 
 
 
@@ -767,8 +728,14 @@ This routine finds all the cached group members that are members of this group
         }
     }
 
         }
     }
 
+    $self->_NewTransaction( Type => ($val == 1) ? "Disabled" : "Enabled" );
+
     $RT::Handle->Commit();
     $RT::Handle->Commit();
-    return (1, $self->loc("Succeeded"));
+    if ( $val == 1 ) {
+        return (1, $self->loc("Group disabled"));
+    } else {
+        return (1, $self->loc("Group enabled"));
+    }
 
 }
 
 
 }
 
@@ -786,7 +753,8 @@ sub Disabled {
 
 =head2 DeepMembersObj
 
 
 =head2 DeepMembersObj
 
-Returns an RT::CachedGroupMembers object of this group's members.
+Returns an RT::CachedGroupMembers object of this group's members,
+including all members of subgroups.
 
 =cut
 
 
 =cut
 
@@ -804,55 +772,108 @@ sub DeepMembersObj {
 
 # }}}
 
 
 # }}}
 
-# {{{ UserMembersObj
+# {{{ MembersObj
 
 
-=head2 UserMembersObj
+=head2 MembersObj
 
 
-Returns an RT::Users object of this group's members, including
-all members of subgroups
+Returns an RT::GroupMembers object of this group's direct members.
 
 =cut
 
 
 =cut
 
-sub UserMembersObj {
+sub MembersObj {
     my $self = shift;
     my $self = shift;
-
-    my $users = RT::Users->new($self->CurrentUser);
+    my $members_obj = RT::GroupMembers->new( $self->CurrentUser );
 
     #If we don't have rights, don't include any results
     # TODO XXX  WHY IS THERE NO ACL CHECK HERE?
 
     #If we don't have rights, don't include any results
     # TODO XXX  WHY IS THERE NO ACL CHECK HERE?
+    $members_obj->LimitToMembersOfGroup( $self->PrincipalId );
 
 
-    my $cached_members = $users->NewAlias('CachedGroupMembers');
-    $users->Join(ALIAS1 => $cached_members, FIELD1 => 'MemberId',
-                 ALIAS2 => $users->PrincipalsAlias, FIELD2 => 'id');
-    $users->Limit(ALIAS => $cached_members, 
-                  FIELD => 'GroupId',
-                  OPERATOR => '=',
-                  VALUE => $self->PrincipalId);
+    return ( $members_obj );
 
 
-    return ( $users);
+}
 
 
+# }}}
+
+# {{{ GroupMembersObj
+
+=head2 GroupMembersObj [Recursively => 1]
+
+Returns an L<RT::Groups> object of this group's members.
+By default returns groups including all subgroups, but
+could be changed with C<Recursively> named argument.
+
+B<Note> that groups are not filtered by type and result
+may contain as well system groups, personal and other.
+
+=cut
+
+sub GroupMembersObj {
+    my $self = shift;
+    my %args = ( Recursively => 1, @_ );
+
+    my $groups = RT::Groups->new( $self->CurrentUser );
+    my $members_table = $args{'Recursively'}?
+        'CachedGroupMembers': 'GroupMembers';
+
+    my $members_alias = $groups->NewAlias( $members_table );
+    $groups->Join(
+        ALIAS1 => $members_alias,           FIELD1 => 'MemberId',
+        ALIAS2 => $groups->PrincipalsAlias, FIELD2 => 'id',
+    );
+    $groups->Limit(
+        ALIAS    => $members_alias,
+        FIELD    => 'GroupId',
+        VALUE    => $self->PrincipalId,
+    );
+    $groups->Limit(
+        ALIAS => $members_alias,
+        FIELD => 'Disabled',
+        VALUE => 0,
+    ) if $args{'Recursively'};
+
+    return $groups;
 }
 
 # }}}
 
 }
 
 # }}}
 
-# {{{ MembersObj
+# {{{ UserMembersObj
 
 
-=head2 MembersObj
+=head2 UserMembersObj
 
 
-Returns an RT::CachedGroupMembers object of this group's members.
+Returns an L<RT::Users> object of this group's members, by default
+returns users including all members of subgroups, but could be
+changed with C<Recursively> named argument.
 
 =cut
 
 
 =cut
 
-sub MembersObj {
+sub UserMembersObj {
     my $self = shift;
     my $self = shift;
-    my $members_obj = RT::GroupMembers->new( $self->CurrentUser );
+    my %args = ( Recursively => 1, @_ );
 
     #If we don't have rights, don't include any results
     # TODO XXX  WHY IS THERE NO ACL CHECK HERE?
 
     #If we don't have rights, don't include any results
     # TODO XXX  WHY IS THERE NO ACL CHECK HERE?
-    $members_obj->LimitToMembersOfGroup( $self->PrincipalId );
 
 
-    return ( $members_obj );
+    my $members_table = $args{'Recursively'}?
+        'CachedGroupMembers': 'GroupMembers';
+
+    my $users = RT::Users->new($self->CurrentUser);
+    my $members_alias = $users->NewAlias( $members_table );
+    $users->Join(
+        ALIAS1 => $members_alias,           FIELD1 => 'MemberId',
+        ALIAS2 => $users->PrincipalsAlias, FIELD2 => 'id',
+    );
+    $users->Limit(
+        ALIAS => $members_alias,
+        FIELD => 'GroupId',
+        VALUE => $self->PrincipalId,
+    );
+    $users->Limit(
+        ALIAS => $members_alias,
+        FIELD => 'Disabled',
+        VALUE => 0,
+    ) if $args{'Recursively'};
 
 
+    return ( $users);
 }
 
 # }}}
 }
 
 # }}}
@@ -976,7 +997,7 @@ sub _AddMember {
     if ( $self->HasMember( $new_member_obj ) ) {
 
         #User is already a member of this group. no need to add it
     if ( $self->HasMember( $new_member_obj ) ) {
 
         #User is already a member of this group. no need to add it
-        return ( 0, $self->loc("Group already has member") );
+        return ( 0, $self->loc("Group already has member: [_1]", $new_member_obj->Object->Name) );
     }
     if ( $new_member_obj->IsGroup &&
          $new_member_obj->Object->HasMemberRecursively($self->PrincipalObj) ) {
     }
     if ( $new_member_obj->IsGroup &&
          $new_member_obj->Object->HasMemberRecursively($self->PrincipalObj) ) {
@@ -993,7 +1014,7 @@ sub _AddMember {
         InsideTransaction => $args{'InsideTransaction'}
     );
     if ($id) {
         InsideTransaction => $args{'InsideTransaction'}
     );
     if ($id) {
-        return ( 1, $self->loc("Member added") );
+        return ( 1, $self->loc("Member added: [_1]", $new_member_obj->Object->Name) );
     }
     else {
         return(0, $self->loc("Couldn't add member to group"));
     }
     else {
         return(0, $self->loc("Couldn't add member to group"));
@@ -1003,9 +1024,9 @@ sub _AddMember {
 
 # {{{ HasMember
 
 
 # {{{ HasMember
 
-=head2 HasMember RT::Principal
+=head2 HasMember RT::Principal|id
 
 
-Takes an RT::Principal object returns a GroupMember Id if that user is a 
+Takes an L<RT::Principal> object or its id returns a GroupMember Id if that user is a 
 member of this group.
 Returns undef if the user isn't a member of the group or if the current
 user doesn't have permission to find out. Arguably, it should differentiate
 member of this group.
 Returns undef if the user isn't a member of the group or if the current
 user doesn't have permission to find out. Arguably, it should differentiate
@@ -1017,29 +1038,28 @@ sub HasMember {
     my $self    = shift;
     my $principal = shift;
 
     my $self    = shift;
     my $principal = shift;
 
-
-    unless (UNIVERSAL::isa($principal,'RT::Principal')) {
-        $RT::Logger->crit("Group::HasMember was called with an argument that".
-                          "isn't an RT::Principal. It's $principal");
-        return(undef);
-    }
-
-    unless ($principal->Id) {
+    my $id;
+    if ( UNIVERSAL::isa($principal,'RT::Principal') ) {
+        $id = $principal->id;
+    } elsif ( $principal =~ /^\d+$/ ) {
+        $id = $principal;
+    } else {
+        $RT::Logger->error("Group::HasMember was called with an argument that".
+                          " isn't an RT::Principal or id. It's ".($principal||'(undefined)'));
         return(undef);
     }
         return(undef);
     }
+    return undef unless $id;
 
     my $member_obj = RT::GroupMember->new( $self->CurrentUser );
 
     my $member_obj = RT::GroupMember->new( $self->CurrentUser );
-    $member_obj->LoadByCols( MemberId => $principal->id, 
-                             GroupId => $self->PrincipalId );
+    $member_obj->LoadByCols(
+        MemberId => $id, 
+        GroupId  => $self->PrincipalId
+    );
 
 
-    #If we have a member object
-    if ( defined $member_obj->id ) {
-        return ( $member_obj->id );
+    if ( my $member_id = $member_obj->id ) {
+        return $member_id;
     }
     }
-
-    #If Load returns no objects, we have an undef id. 
     else {
     else {
-        #$RT::Logger->debug($self." does not contain principal ".$principal->id);
         return (undef);
     }
 }
         return (undef);
     }
 }
@@ -1048,9 +1068,9 @@ sub HasMember {
 
 # {{{ HasMemberRecursively
 
 
 # {{{ HasMemberRecursively
 
-=head2 HasMemberRecursively RT::Principal
+=head2 HasMemberRecursively RT::Principal|id
 
 
-Takes an RT::Principal object and returns true if that user is a member of 
+Takes an L<RT::Principal> object or its id and returns true if that user is a member of 
 this group.
 Returns undef if the user isn't a member of the group or if the current
 user doesn't have permission to find out. Arguably, it should differentiate
 this group.
 Returns undef if the user isn't a member of the group or if the current
 user doesn't have permission to find out. Arguably, it should differentiate
@@ -1062,23 +1082,27 @@ sub HasMemberRecursively {
     my $self    = shift;
     my $principal = shift;
 
     my $self    = shift;
     my $principal = shift;
 
-    unless (UNIVERSAL::isa($principal,'RT::Principal')) {
-        $RT::Logger->crit("Group::HasMemberRecursively was called with an argument that".
-                          "isn't an RT::Principal. It's $principal");
+    my $id;
+    if ( UNIVERSAL::isa($principal,'RT::Principal') ) {
+        $id = $principal->id;
+    } elsif ( $principal =~ /^\d+$/ ) {
+        $id = $principal;
+    } else {
+        $RT::Logger->error("Group::HasMemberRecursively was called with an argument that".
+                          " isn't an RT::Principal or id. It's $principal");
         return(undef);
     }
         return(undef);
     }
+    return undef unless $id;
+
     my $member_obj = RT::CachedGroupMember->new( $self->CurrentUser );
     my $member_obj = RT::CachedGroupMember->new( $self->CurrentUser );
-    $member_obj->LoadByCols( MemberId => $principal->Id,
-                             GroupId => $self->PrincipalId ,
-                             Disabled => 0
-                             );
-
-    #If we have a member object
-    if ( defined $member_obj->id ) {
-        return ( 1);
-    }
+    $member_obj->LoadByCols(
+        MemberId => $id, 
+        GroupId  => $self->PrincipalId
+    );
 
 
-    #If Load returns no objects, we have an undef id. 
+    if ( my $member_id = $member_obj->id ) {
+        return $member_id;
+    }
     else {
         return (undef);
     }
     else {
         return (undef);
     }
@@ -1165,11 +1189,73 @@ sub _DeleteMember {
 
 # }}}
 
 
 # }}}
 
+# {{{ sub CleanupInvalidDelegations
+
+=head2 CleanupInvalidDelegations { InsideTransaction => undef }
+
+Revokes all ACE entries delegated by members of this group which are
+inconsistent with their current delegation rights.  Does not perform
+permission checks.  Should only ever be called from inside the RT
+library.
+
+If called from inside a transaction, specify a true value for the
+InsideTransaction parameter.
+
+Returns a true value if the deletion succeeded; returns a false value
+and logs an internal error if the deletion fails (should not happen).
+
+=cut
+
+# XXX Currently there is a CleanupInvalidDelegations method in both
+# RT::User and RT::Group.  If the recursive cleanup call for groups is
+# ever unrolled and merged, this code will probably want to be
+# factored out into RT::Principal.
+
+# backcompat for 3.8.8 and before
+*_CleanupInvalidDelegations = \&CleanupInvalidDelegations;
+
+sub CleanupInvalidDelegations {
+    my $self = shift;
+    my %args = ( InsideTransaction => undef,
+                 @_ );
+
+    unless ( $self->Id ) {
+       $RT::Logger->warning("Group not loaded.");
+       return (undef);
+    }
+
+    my $in_trans = $args{InsideTransaction};
+
+    # TODO: Can this be unrolled such that the number of DB queries is constant rather than linear in exploded group size?
+    my $members = $self->DeepMembersObj();
+    $members->LimitToUsers();
+    $RT::Handle->BeginTransaction() unless $in_trans;
+    while ( my $member = $members->Next()) {
+       my $ret = $member->MemberObj->CleanupInvalidDelegations(InsideTransaction => 1,
+                                                                Object => $args{Object});
+       unless ($ret) {
+           $RT::Handle->Rollback() unless $in_trans;
+           return (undef);
+       }
+    }
+    $RT::Handle->Commit() unless $in_trans;
+    return(1);
+}
+
+# }}}
+
 # {{{ ACL Related routines
 
 # {{{ sub _Set
 sub _Set {
     my $self = shift;
 # {{{ ACL Related routines
 
 # {{{ sub _Set
 sub _Set {
     my $self = shift;
+    my %args = (
+        Field => undef,
+        Value => undef,
+       TransactionType   => 'Set',
+       RecordTransaction => 1,
+        @_
+    );
 
        if ($self->Domain eq 'Personal') {
                if ($self->CurrentUser->PrincipalId == $self->Instance) {
 
        if ($self->Domain eq 'Personal') {
                if ($self->CurrentUser->PrincipalId == $self->Instance) {
@@ -1187,7 +1273,30 @@ sub _Set {
                return ( 0, $self->loc('Permission Denied') );
        }
        }
                return ( 0, $self->loc('Permission Denied') );
        }
        }
-    return ( $self->SUPER::_Set(@_) );
+
+    my $Old = $self->SUPER::_Value("$args{'Field'}");
+    
+    my ($ret, $msg) = $self->SUPER::_Set( Field => $args{'Field'},
+                                         Value => $args{'Value'} );
+    
+    #If we can't actually set the field to the value, don't record
+    # a transaction. instead, get out of here.
+    if ( $ret == 0 ) { return ( 0, $msg ); }
+
+    if ( $args{'RecordTransaction'} == 1 ) {
+
+        my ( $Trans, $Msg, $TransObj ) = $self->_NewTransaction(
+                                               Type => $args{'TransactionType'},
+                                               Field     => $args{'Field'},
+                                               NewValue  => $args{'Value'},
+                                               OldValue  => $Old,
+                                               TimeTaken => $args{'TimeTaken'},
+        );
+        return ( $Trans, scalar $TransObj->Description );
+    }
+    else {
+        return ( $ret, $msg );
+    }
 }
 
 # }}}
 }
 
 # }}}
@@ -1195,7 +1304,7 @@ sub _Set {
 
 
 
 
 
 
-=item CurrentUserHasRight RIGHTNAME
+=head2 CurrentUserHasRight RIGHTNAME
 
 Returns true if the current user has the specified right for this group.
 
 
 Returns true if the current user has the specified right for this group.
 
@@ -1237,23 +1346,17 @@ Returns the principal object for this user. returns an empty RT::Principal
 if there's no principal object matching this user. 
 The response is cached. PrincipalObj should never ever change.
 
 if there's no principal object matching this user. 
 The response is cached. PrincipalObj should never ever change.
 
-=begin testing
-
-ok(my $u = RT::Group->new($RT::SystemUser));
-ok($u->Load(4), "Loaded the first user");
-ok($u->PrincipalObj->ObjectId == 4, "user 4 is the fourth principal");
-ok($u->PrincipalObj->PrincipalType eq 'Group' , "Principal 4 is a group");
-
-=end testing
 
 =cut
 
 
 sub PrincipalObj {
     my $self = shift;
 
 =cut
 
 
 sub PrincipalObj {
     my $self = shift;
-    unless ($self->{'PrincipalObj'} &&
+    unless ( defined $self->{'PrincipalObj'} &&
+             defined $self->{'PrincipalObj'}->ObjectId &&
             ($self->{'PrincipalObj'}->ObjectId == $self->Id) &&
             ($self->{'PrincipalObj'}->ObjectId == $self->Id) &&
-            ($self->{'PrincipalObj'}->PrincipalType eq 'Group')) {
+            (defined $self->{'PrincipalObj'}->PrincipalType && 
+                $self->{'PrincipalObj'}->PrincipalType eq 'Group')) {
 
             $self->{'PrincipalObj'} = RT::Principal->new($self->CurrentUser);
             $self->{'PrincipalObj'}->LoadByCols('ObjectId' => $self->Id,
 
             $self->{'PrincipalObj'} = RT::Principal->new($self->CurrentUser);
             $self->{'PrincipalObj'}->LoadByCols('ObjectId' => $self->Id,
@@ -1275,5 +1378,21 @@ sub PrincipalId {
 }
 
 # }}}
 }
 
 # }}}
+
+sub BasicColumns {
+    (
+       [ Name => 'Name' ],
+       [ Description => 'Description' ],
+    );
+}
+
 1;
 
 1;
 
+=head1 AUTHOR
+
+Jesse Vincent, jesse@bestpractical.com
+
+=head1 SEE ALSO
+
+RT
+
Freeside billing, trouble ticketing, network monitoring and provisioning