[Rt-commit] rt branch, 4.2/role-group-membership, created. rt-4.1.5-158-gedb7c7a

Alex Vandiver alexmv at bestpractical.com
Fri Dec 28 10:45:25 EST 2012


The branch, 4.2/role-group-membership has been created
        at  edb7c7aacc0d4d83a832064cc9d23adbc88e5b06 (commit)

- Log -----------------------------------------------------------------
commit 67e6b13060be1f257a010f15beff8f728f5d6a65
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Thu Dec 27 23:01:08 2012 -0500

    Remove old method re-introduced in mismerge
    
    b2f9621 generalized _CreateTicketGroups and refactored it into
    RT::Record's _CreateRoleGroups.  Unfortunately, _CreateTicketGroups was
    mistakenly re-introduced during the merge from 4.0-trunk in 1fbb632.
    Remove it, again.

diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index 4d20523..d7864f1 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -651,40 +651,6 @@ sub _Parse822HeadersForAttributes {
     return (%args);
 }
 
-=head2 _CreateTicketGroups
-
-Create the ticket groups and links for this ticket. 
-This routine expects to be called from Ticket->Create _inside of a transaction_
-
-It will create four groups for this ticket: Requestor, Cc, AdminCc and Owner.
-
-It will return true on success and undef on failure.
-
-
-=cut
-
-
-sub _CreateTicketGroups {
-    my $self = shift;
-    
-    my @types = (qw(Requestor Owner Cc AdminCc));
-
-    foreach my $type (@types) {
-        my $type_obj = RT::Group->new($self->CurrentUser);
-        my ($id, $msg) = $type_obj->CreateRoleGroup(Domain => 'RT::Ticket-Role',
-                                                       Instance => $self->Id, 
-                                                       Type => $type);
-        unless ($id) {
-            $RT::Logger->error("Couldn't create a ticket group of type '$type' for ticket ".
-                               $self->Id.": ".$msg);     
-            return(undef);
-        }
-     }
-    return(1);
-    
-}
-
-
 =head2 OwnerGroup
 
 A constructor which returns an RT::Group object containing the owner of this ticket.

commit e987bd5d443b685fafd2aa201f76af34c9e61c73
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Thu Dec 27 22:59:59 2012 -0500

    Simplify ticket merging code for roles
    
    Make use of the new methods surrounding roles in the ticket merging
    code.

diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index d7864f1..5692a28 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -2294,25 +2294,21 @@ sub _MergeInto {
             ( $MergeInto->$type() || 0 ) + ( $self->$type() || 0 ) );
 
     }
-#add all of this ticket's watchers to that ticket.
-    foreach my $watcher_type (qw(Requestors Cc AdminCc)) {
-
-        my $people = $self->$watcher_type->MembersObj;
-        my $addwatcher_type =  $watcher_type;
-        $addwatcher_type  =~ s/s$//;
-
+    # add all of this ticket's watchers to that ticket.
+    for my $role ($self->Roles) {
+        next if $self->RoleGroup($role)->SingleMemberRoleGroup;
+        my $people = $self->RoleGroup($role)->MembersObj;
         while ( my $watcher = $people->Next ) {
-            
-           my ($val, $msg) =  $MergeInto->_AddWatcher(
-                Type        => $addwatcher_type,
-                Silent => 1,
-                PrincipalId => $watcher->MemberId
+            my ($val, $msg) =  $MergeInto->AddRoleMember(
+                Type              => $role,
+                Silent            => 1,
+                PrincipalId       => $watcher->MemberId,
+                InsideTransaction => 1,
             );
             unless ($val) {
                 $RT::Logger->debug($msg);
             }
-    }
-
+        }
     }
 
     #find all of the tickets that were merged into this ticket. 

commit 97a6962a696de4474195122a62de45fc7db96ab0
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Thu Dec 27 22:59:22 2012 -0500

    Push logic into RT::Record->AddRoleMember from _AddWatcher
    
    RT::Ticket and RT::Queue contain very similar code in _AddWatcher, which
    attempts to check IsRTAddress and resolve the passed email address to a
    user, creating it as necessary -- though RT::Queue's implementation
    predates ->LoadOrCreateByEmail.  This should not be surprising, as the
    code in RT::Queue was copied wholesale from RT::Ticket in 40b1ea0.
    
    Unify the two divergent codepaths by pushing additional the additional
    logic down into RT::Record->AddRoleMember.  This also serves to resolve
    a bug when AddRoleMember was called with a non-existant email address.

diff --git a/lib/RT/Queue.pm b/lib/RT/Queue.pm
index b161411..a4123e5 100644
--- a/lib/RT/Queue.pm
+++ b/lib/RT/Queue.pm
@@ -820,17 +820,11 @@ sub _HasModifyWatcherRight {
 
 =head2 AddWatcher
 
-AddWatcher takes a parameter hash. The keys are as follows:
+Applies access control checking, then calls L<RT::Record/AddRoleMember>.
+Additionally, C<Email> is accepted as an alternative argument name for
+C<User>.
 
-Type        One of Requestor, Cc, AdminCc
-
-PrinicpalId The RT::Principal id of the user or group that's being added as a watcher
-Email       The email address of the new watcher. If a user with this 
-            email address can't be found, a new nonprivileged user will be created.
-
-If the watcher you're trying to set has an RT account, set the Owner parameter to their User Id. Otherwise, set the Email parameter to their Email address.
-
-Returns a tuple of (status/id, message).
+Returns a tuple of (status, message).
 
 =cut
 
@@ -848,7 +842,7 @@ sub AddWatcher {
 
     if ( !$args{'PrincipalId'} && $args{'Email'} ) {
         my $user = RT::User->new( $self->CurrentUser );
-        $user->LoadByEmail( $args{'Email'} );
+        $user->LoadByEmail( delete $args{'Email'} );
         $args{'PrincipalId'} = $user->PrincipalId if $user->id;
     }
 
@@ -861,8 +855,6 @@ sub AddWatcher {
     return $self->_AddWatcher(%args);
 }
 
-#This contains the meat of AddWatcher. but can be called from a routine like
-# Create, which doesn't need the additional acl check
 sub _AddWatcher {
     my $self = shift;
     my %args = (
@@ -873,73 +865,12 @@ sub _AddWatcher {
         @_
     );
 
+    $args{User} ||= delete $args{Email};
+    my ($principal, $msg) = $self->AddRoleMember( %args );
+    return ( 0, $msg) unless $principal;
 
-    my $principal = RT::Principal->new( $self->CurrentUser );
-    if ( $args{'PrincipalId'} ) {
-        $principal->Load( $args{'PrincipalId'} );
-        if ( $principal->id and $principal->IsUser and my $email = $principal->Object->EmailAddress ) {
-            return (0, $self->loc("[_1] is an address RT receives mail at. Adding it as a '[_2]' would create a mail loop", $email, $self->loc($args{'Type'})))
-                if RT::EmailParser->IsRTAddress( $email );
-        }
-    }
-    elsif ( $args{'Email'} ) {
-        if ( RT::EmailParser->IsRTAddress( $args{'Email'} ) ) {
-            return (0, $self->loc("[_1] is an address RT receives mail at. Adding it as a '[_2]' would create a mail loop", $args{'Email'}, $self->loc($args{'Type'})));
-        }
-        my $user = RT::User->new($self->CurrentUser);
-        $user->LoadByEmail( $args{'Email'} );
-        $user->Load( $args{'Email'} )
-            unless $user->id;
-
-        if ( $user->Id ) { # If the user exists
-            $principal->Load( $user->PrincipalId );
-        } else {
-            # if the user doesn't exist, we need to create a new user
-            my $new_user = RT::User->new(RT->SystemUser);
-
-            my ( $Address, $Name ) =  
-               RT::Interface::Email::ParseAddressFromHeader($args{'Email'});
-
-            my ( $Val, $Message ) = $new_user->Create(
-                Name         => $Address,
-                EmailAddress => $Address,
-                RealName     => $Name,
-                Privileged   => 0,
-                Comments     => 'Autocreated when added as a watcher'
-            );
-            unless ($Val) {
-                $RT::Logger->error("Failed to create user ".$args{'Email'} .": " .$Message);
-                # Deal with the race condition of two account creations at once
-                $new_user->LoadByEmail( $args{'Email'} );
-            }
-            $principal->Load( $new_user->PrincipalId );
-        }
-    }
-    # If we can't find this watcher, we need to bail.
-    unless ( $principal->Id ) {
-        return(0, $self->loc("Could not find or create that user"));
-    }
-
-    my $group = $self->RoleGroup( $args{'Type'} );
-    unless ($group->id) {
-        return(0,$self->loc("Group not found"));
-    }
-
-    if ( $group->HasMember( $principal)) {
-
-        return ( 0, $self->loc('[_1] is already a [_2] for this queue',
-                    $principal->Object->Name, $args{'Type'}) );
-    }
-
-
-    my ($m_id, $m_msg) = $group->_AddMember(PrincipalId => $principal->Id);
-    unless ($m_id) {
-        $RT::Logger->error("Failed to add ".$principal->Id." as a member of group ".$group->Id.": ".$m_msg);
-
-        return ( 0, $self->loc('Could not make [_1] a [_2] for this queue',
-                    $principal->Object->Name, $args{'Type'}) );
-    }
-    return ( 1, $self->loc("Added [_1] to members of [_2] for this queue.", $principal->Object->Name, $args{'Type'} ));
+    return ( 1, $self->loc("Added [_1] to members of [_2] for this queue.",
+                           $principal->Object->Name, $self->loc($args{'Type'}) ));
 }
 
 
diff --git a/lib/RT/Record.pm b/lib/RT/Record.pm
index ee5a8bc..2830c4a 100644
--- a/lib/RT/Record.pm
+++ b/lib/RT/Record.pm
@@ -2294,10 +2294,13 @@ Optional.  The ID of the L<RT::Principal> object to add.
 
 =item User
 
+Optional.  The Name or EmailAddress of an L<RT::User> to use as the
+principal.  If an email address is given, but a user matching it cannot
+be found, a new user will be created.
+
 =item Group
 
-Optional.  The Name of an L<RT::User> or L<RT::Group>, respectively, to use as
-the principal.
+Optional.  The Name of an L<RT::Group> to use as the principal.
 
 =item Type
 
@@ -2307,7 +2310,7 @@ Required.  One of the valid roles for this record, as returned by L</Roles>.
 
 One, and only one, of I<PrincipalId>, I<User>, or I<Group> is required.
 
-Returns a tuple of (status, message).
+Returns a tuple of (principal object which was added, message).
 
 =cut
 
@@ -2322,25 +2325,65 @@ sub AddRoleMember {
     return (0, $self->loc("No valid Type specified"))
         unless $type and $self->HasRole($type);
 
-    unless ($args{PrincipalId}) {
-        my $object;
+    if ($args{PrincipalId}) {
+        # Check the PrincipalId for loops
+        my $principal = RT::Principal->new( $self->CurrentUser );
+        $principal->Load($args{'PrincipalId'});
+        if ( $principal->id and $principal->IsUser and my $email = $principal->Object->EmailAddress ) {
+            return (0, $self->loc("[_1] is an address RT receives mail at. Adding it as a '[_2]' would create a mail loop",
+                                  $email, $self->loc($type)))
+                if RT::EmailParser->IsRTAddress( $email );
+        }
+    } else {
         if ($args{User}) {
-            $object = RT::User->new( $self->CurrentUser );
-            $object->Load(delete $args{User});
+            my $name = delete $args{User};
+            # Sanity check the address
+            return (0, $self->loc("[_1] is an address RT receives mail at. Adding it as a '[_2]' would create a mail loop",
+                                  $name, $self->loc($type) ))
+                if RT::EmailParser->IsRTAddress( $name );
+
+            # Create as the SystemUser, not the current user
+            my $user = RT::User->new(RT->SystemUser);
+            my ($pid, $msg) = $user->LoadOrCreateByEmail( $name );
+            unless ($pid) {
+                # If we can't find this watcher, we need to bail.
+                $RT::Logger->error("Could not load or create a user '$name' to add as a watcher: $msg");
+                return (0, $self->loc("Could not find or create user '$name'"));
+            }
+            $args{PrincipalId} = $pid;
         }
         elsif ($args{Group}) {
-            $object = RT::Group->new( $self->CurrentUser );
-            $object->LoadUserDefinedGroup(delete $args{Group});
+            my $name = delete $args{Group};
+            my $group = RT::Group->new( $self->CurrentUser );
+            $group->LoadUserDefinedGroup($name);
+            unless ($group->id) {
+                $RT::Logger->error("Could not load group '$name' to add as a watcher");
+                return (0, $self->loc("Could not find group '$name'"));
+            }
+            $args{PrincipalId} = $group->PrincipalObj->id;
         }
-        $args{PrincipalId} = $object->PrincipalObj->id;
     }
 
-    return (0, $self->loc("No valid PrincipalId"))
-        unless $args{PrincipalId};
+    my $principal = RT::Principal->new( $self->CurrentUser );
+    $principal->Load( $args{PrincipalId} );
 
-    my ($ok, $msg) = $self->RoleGroup($type)->_AddMember(%args);
+    my $group = $self->RoleGroup( $type );
+    return (0, $self->loc("Role group '$type' not found"))
+        unless $group->id;
 
-    if ($ok and not $args{Silent}) {
+    return (0, $self->loc('[_1] is already a [_2]',
+                          $principal->Object->Name, $self->loc($type)) )
+            if $group->HasMember( $principal );
+
+    my ( $ok, $msg ) = $group->_AddMember( %args );
+    unless ($ok) {
+        $RT::Logger->error("Failed to add $args{PrincipalId} as a member of group ".$group->Id.": ".$msg);
+
+        return ( 0, $self->loc('Could not make [_1] a [_2]',
+                    $principal->Object->Name, $self->loc($type)) );
+    }
+
+    unless ($args{Silent}) {
         $self->_NewTransaction(
             Type     => 'AddWatcher', # use "watcher" for history's sake
             NewValue => $args{PrincipalId},
@@ -2348,7 +2391,7 @@ sub AddRoleMember {
         );
     }
 
-    return ($ok, $msg);
+    return ($principal, $msg);
 }
 
 =head2 DeleteRoleMember
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index 5692a28..7bcf313 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -667,16 +667,11 @@ sub OwnerGroup {
 
 =head2 AddWatcher
 
-AddWatcher takes a parameter hash. The keys are as follows:
+Applies access control checking, then calls L<RT::Record/AddRoleMember>.
+Additionally, C<Email> is accepted as an alternative argument name for
+C<User>.
 
-Type        One of Requestor, Cc, AdminCc
-
-PrincipalId The RT::Principal id of the user or group that's being added as a watcher
-
-Email       The email address of the new watcher. If a user with this 
-            email address can't be found, a new nonprivileged user will be created.
-
-If the watcher you're trying to set has an RT account, set the PrincipalId paremeter to their User Id. Otherwise, set the Email parameter to their Email address.
+Returns a tuple of (status, message).
 
 =cut
 
@@ -732,8 +727,6 @@ sub AddWatcher {
     return $self->_AddWatcher( %args );
 }
 
-#This contains the meat of AddWatcher. but can be called from a routine like
-# Create, which doesn't need the additional acl check
 sub _AddWatcher {
     my $self = shift;
     my %args = (
@@ -744,61 +737,12 @@ sub _AddWatcher {
         @_
     );
 
-
-    my $principal = RT::Principal->new($self->CurrentUser);
-    if ($args{'Email'}) {
-        if ( RT::EmailParser->IsRTAddress( $args{'Email'} ) ) {
-            return (0, $self->loc("[_1] is an address RT receives mail at. Adding it as a '[_2]' would create a mail loop", $args{'Email'}, $self->loc($args{'Type'})));
-        }
-        my $user = RT::User->new(RT->SystemUser);
-        my ($pid, $msg) = $user->LoadOrCreateByEmail( $args{'Email'} );
-        $args{'PrincipalId'} = $pid if $pid; 
-    }
-    if ($args{'PrincipalId'}) {
-        $principal->Load($args{'PrincipalId'});
-        if ( $principal->id and $principal->IsUser and my $email = $principal->Object->EmailAddress ) {
-            return (0, $self->loc("[_1] is an address RT receives mail at. Adding it as a '[_2]' would create a mail loop", $email, $self->loc($args{'Type'})))
-                if RT::EmailParser->IsRTAddress( $email );
-
-        }
-    } 
-
- 
-    # If we can't find this watcher, we need to bail.
-    unless ($principal->Id) {
-            $RT::Logger->error("Could not load create a user with the email address '".$args{'Email'}. "' to add as a watcher for ticket ".$self->Id);
-        return(0, $self->loc("Could not find or create that user"));
-    }
-
-
-    my $group = $self->RoleGroup( $args{'Type'} );
-    unless ($group->id) {
-        return(0,$self->loc("Group not found"));
-    }
-
-    if ( $group->HasMember( $principal)) {
-
-        return ( 0, $self->loc('[_1] is already a [_2] for this ticket',
-                    $principal->Object->Name, $self->loc($args{'Type'})) );
-    }
-
-
-    my ( $m_id, $m_msg ) = $group->_AddMember( PrincipalId => $principal->Id,
-                                               InsideTransaction => 1 );
-    unless ($m_id) {
-        $RT::Logger->error("Failed to add ".$principal->Id." as a member of group ".$group->Id.": ".$m_msg);
-
-        return ( 0, $self->loc('Could not make [_1] a [_2] for this ticket',
-                    $principal->Object->Name, $self->loc($args{'Type'})) );
-    }
-
-    unless ( $args{'Silent'} ) {
-        $self->_NewTransaction(
-            Type     => 'AddWatcher',
-            NewValue => $principal->Id,
-            Field    => $args{'Type'}
-        );
-    }
+    $args{User} ||= delete $args{Email};
+    my ($principal, $msg) = $self->AddRoleMember(
+        %args,
+        InsideTransaction => 1,
+    );
+    return ( 0, $msg) unless $principal;
 
     return ( 1, $self->loc('Added [_1] as a [_2] for this ticket',
                 $principal->Object->Name, $self->loc($args{'Type'})) );
diff --git a/t/web/ticket_modify_all.t b/t/web/ticket_modify_all.t
index 6d19b28..cf398b2 100644
--- a/t/web/ticket_modify_all.t
+++ b/t/web/ticket_modify_all.t
@@ -76,7 +76,7 @@ $m->field(WatcherTypeEmail => 'Requestor');
 $m->field(WatcherAddressEmail => 'root at localhost');
 $m->click('SubmitTicket');
 $m->text_contains(
-    "root is already a Requestor for this ticket",
+    "root is already a Requestor",
     'no duplicate watchers',
 );
 

commit 587ee69c9782245ed67eee335a0197b9df5dff3e
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Thu Dec 27 23:00:52 2012 -0500

    Reduce duplication by placing ACL callback in AddRoleMember
    
    Both RT::Ticket and RT::Queue's AddWatcher method attempt to resolve the
    passed-in email address in order to ascertain if it has appropriate
    permissions.  This requires duplicating code in _AddWatcher (now found
    in AddRoleMember).  In the case of RT::Queue's AddWatcher code, this
    also effectively prevented passing in not-yet-in-the-database email
    addresses, as ACLs were checked with ->Load, despite _AddWatcher later
    using LoadOrCreateByEmail.
    
    Provide a hook in AddRoleMember after the principal has been resolved,
    which can be used to apply ACLs.  This also removes the need for
    separation between _AddMember and AddMember.

diff --git a/lib/RT/Queue.pm b/lib/RT/Queue.pm
index a4123e5..e6be027 100644
--- a/lib/RT/Queue.pm
+++ b/lib/RT/Queue.pm
@@ -837,33 +837,14 @@ sub AddWatcher {
         @_
     );
 
-    return ( 0, "No principal specified" )
-        unless $args{'Email'} or $args{'PrincipalId'};
-
-    if ( !$args{'PrincipalId'} && $args{'Email'} ) {
-        my $user = RT::User->new( $self->CurrentUser );
-        $user->LoadByEmail( delete $args{'Email'} );
-        $args{'PrincipalId'} = $user->PrincipalId if $user->id;
-    }
-
-    return ( 0, "Unknown watcher type [_1]", $args{Type} )
-        unless $self->HasRole($args{Type});
-
-    my ($ok, $msg) = $self->_HasModifyWatcherRight(%args);
-    return ($ok, $msg) if !$ok;
-
-    return $self->_AddWatcher(%args);
-}
-
-sub _AddWatcher {
-    my $self = shift;
-    my %args = (
-        Type   => undef,
-        Silent => undef,
-        PrincipalId => undef,
-        Email => undef,
-        @_
-    );
+    $args{ACL} = sub {
+        my $principal = shift;
+        my ($ok, $msg) = $self->_HasModifyWatcherRight(
+            Type        => $args{Type},
+            PrincipalId => $principal->id
+        );
+        return $ok;
+    };
 
     $args{User} ||= delete $args{Email};
     my ($principal, $msg) = $self->AddRoleMember( %args );
diff --git a/lib/RT/Record.pm b/lib/RT/Record.pm
index 2830c4a..faf783a 100644
--- a/lib/RT/Record.pm
+++ b/lib/RT/Record.pm
@@ -2306,6 +2306,12 @@ Optional.  The Name of an L<RT::Group> to use as the principal.
 
 Required.  One of the valid roles for this record, as returned by L</Roles>.
 
+=item ACL
+
+Optional.  A subroutine reference which will be passed the principal
+being added, once it has been resolved.  If it returns false, the method
+will fail with a status of "Permission denied".
+
 =back
 
 One, and only one, of I<PrincipalId>, I<User>, or I<Group> is required.
@@ -2367,6 +2373,10 @@ sub AddRoleMember {
     my $principal = RT::Principal->new( $self->CurrentUser );
     $principal->Load( $args{PrincipalId} );
 
+    my $acl = delete $args{ACL};
+    return (0, $self->loc("Permission denied"))
+        if $acl and not $acl->($principal);
+
     my $group = $self->RoleGroup( $type );
     return (0, $self->loc("Role group '$type' not found"))
         unless $group->id;
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index 7bcf313..e4163f9 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -684,58 +684,19 @@ sub AddWatcher {
         @_
     );
 
-    # ModifyTicket works in any case
-    return $self->_AddWatcher( %args )
-        if $self->CurrentUserHasRight('ModifyTicket');
-    if ( $args{'Email'} ) {
-        my ($addr) = RT::EmailParser->ParseEmailAddress( $args{'Email'} );
-        return (0, $self->loc("Couldn't parse address from '[_1]' string", $args{'Email'} ))
-            unless $addr;
-
-        if ( lc $self->CurrentUser->EmailAddress
-            eq lc RT::User->CanonicalizeEmailAddress( $addr->address ) )
-        {
-            $args{'PrincipalId'} = $self->CurrentUser->id;
-            delete $args{'Email'};
-        }
-    }
-
-    # If the watcher isn't the current user then the current user has no right
-    # bail
-    unless ( $args{'PrincipalId'} && $self->CurrentUser->id == $args{'PrincipalId'} ) {
-        return ( 0, $self->loc("Permission Denied") );
-    }
-
-    #  If it's an AdminCc and they don't have 'WatchAsAdminCc', bail
-    if ( $args{'Type'} eq 'AdminCc' ) {
-        unless ( $self->CurrentUserHasRight('WatchAsAdminCc') ) {
-            return ( 0, $self->loc('Permission Denied') );
-        }
-    }
-
-    #  If it's a Requestor or Cc and they don't have 'Watch', bail
-    elsif ( $args{'Type'} eq 'Cc' || $args{'Type'} eq 'Requestor' ) {
-        unless ( $self->CurrentUserHasRight('Watch') ) {
-            return ( 0, $self->loc('Permission Denied') );
-        }
-    }
-    else {
-        $RT::Logger->warning( "AddWatcher got passed a bogus type");
-        return ( 0, $self->loc('Error in parameters to Ticket->AddWatcher') );
-    }
-
-    return $self->_AddWatcher( %args );
-}
-
-sub _AddWatcher {
-    my $self = shift;
-    my %args = (
-        Type   => undef,
-        Silent => undef,
-        PrincipalId => undef,
-        Email => undef,
-        @_
-    );
+    $args{ACL} = sub {
+        my $principal = shift;
+        # ModifyTicket works in any case
+        return 1 if $self->CurrentUserHasRight('ModifyTicket');
+        # If the watcher isn't the current user then the current user has no right
+        return 0 unless $self->CurrentUser->id == $principal->id;
+        # If it's an AdminCc and they don't have 'WatchAsAdminCc', bail
+        return 0 if $args{Type} eq "AdminCc" and not $self->CurrentUserHasRight('WatchAsAdminCc');
+        # If it's a Requestor or Cc and they don't have 'Watch', bail
+        return 0 if ($args{Type} eq "Cc" or $args{'Type'} eq 'Requestor')
+            and not $self->CurrentUserHasRight('Watch');
+        return 1;
+    };
 
     $args{User} ||= delete $args{Email};
     my ($principal, $msg) = $self->AddRoleMember(

commit 58e8e8f76d674a74fea53827de4842dae08394d6
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Fri Dec 28 01:59:02 2012 -0500

    Consolidate DeleteWatcher code into DeleteRoleMember, similarly

diff --git a/lib/RT/Queue.pm b/lib/RT/Queue.pm
index e6be027..dd10757 100644
--- a/lib/RT/Queue.pm
+++ b/lib/RT/Queue.pm
@@ -856,84 +856,41 @@ sub AddWatcher {
 
 
 
-=head2 DeleteWatcher { Type => TYPE, PrincipalId => PRINCIPAL_ID, Email => EMAIL_ADDRESS }
+=head2 DeleteWatcher
 
+Applies access control checking, then calls L<RT::Record/DeleteRoleMember>.
+Additionally, C<Email> is accepted as an alternative argument name for
+C<User>.
 
-Deletes a queue  watcher.  Takes two arguments:
-
-Type  (one of Requestor,Cc,AdminCc)
-
-and one of
-
-PrincipalId (an RT::Principal Id of the watcher you want to remove)
-    OR
-Email (the email address of an existing wathcer)
-
+Returns a tuple of (status, message).
 
 =cut
 
-
 sub DeleteWatcher {
     my $self = shift;
 
-    my %args = ( Type => undef,
-                 PrincipalId => undef,
-                 Email => undef,
-                 @_ );
-
-    unless ( $args{'PrincipalId'} || $args{'Email'} ) {
-        return ( 0, $self->loc("No principal specified") );
-    }
-
-    if ( !$args{PrincipalId} and $args{Email} ) {
-        my $user = RT::User->new( $self->CurrentUser );
-        my ($rv, $msg) = $user->LoadByEmail( $args{Email} );
-        $args{PrincipalId} = $user->PrincipalId if $rv;
-    }
-    
-    my $principal = RT::Principal->new( $self->CurrentUser );
-    if ( $args{'PrincipalId'} ) {
-        $principal->Load( $args{'PrincipalId'} );
-    }
-    else {
-        my $user = RT::User->new( $self->CurrentUser );
-        $user->LoadByEmail( $args{'Email'} );
-        $principal->Load( $user->Id );
-    }
-
-    # If we can't find this watcher, we need to bail.
-    unless ( $principal->Id ) {
-        return ( 0, $self->loc("Could not find that principal") );
-    }
-
-    my $group = $self->RoleGroup( $args{'Type'} );
-    unless ($group->id) {
-        return(0,$self->loc("Group not found"));
-    }
-
-    return ( 0, $self->loc('Unknown watcher type [_1]', $args{Type}) )
-        unless $self->HasRole($args{Type});
-
-    my ($ok, $msg) = $self->_HasModifyWatcherRight(%args);
-    return ($ok, $msg) if !$ok;
-
-    # see if this user is already a watcher.
-
-    unless ( $group->HasMember($principal)) {
-        return ( 0, $self->loc('[_1] is not a [_2] for this queue',
-            $principal->Object->Name, $args{'Type'}) );
-    }
+    my %args = (
+        Type => undef,
+        PrincipalId => undef,
+        Email => undef,
+        @_
+    );
 
-    my ($m_id, $m_msg) = $group->_DeleteMember($principal->Id);
-    unless ($m_id) {
-        $RT::Logger->error("Failed to delete ".$principal->Id.
-                           " as a member of group ".$group->Id.": ".$m_msg);
+    $args{ACL} = sub {
+        my $principal = shift;
+        my ($ok, $msg) = $self->_HasModifyWatcherRight(
+            Type        => $args{Type},
+            PrincipalId => $principal->id
+        );
+        return $ok;
+    };
 
-        return ( 0, $self->loc('Could not remove [_1] as a [_2] for this queue',
-                    $principal->Object->Name, $args{'Type'}) );
-    }
+    $args{User} ||= delete $args{Email};
+    my ($principal, $msg) = $self->DeleteRoleMember( %args );
+    return ( 0, $msg) unless $principal;
 
-    return ( 1, $self->loc("Removed [_1] from members of [_2] for this queue.", $principal->Object->Name, $args{'Type'} ));
+    return ( 1, $self->loc("Removed [_1] from members of [_2] for this queue.",
+                           $principal->Object->Name, $self->loc($args{'Type'}) ));
 }
 
 
diff --git a/lib/RT/Record.pm b/lib/RT/Record.pm
index faf783a..4cf971e 100644
--- a/lib/RT/Record.pm
+++ b/lib/RT/Record.pm
@@ -2415,7 +2415,12 @@ Takes a set of key-value pairs:
 
 =item PrincipalId
 
-Required.  The ID of the L<RT::Principal> object to remove.
+Optional.  The ID of the L<RT::Principal> object to remove.
+
+=item User
+
+Optional.  The Name or EmailAddress of an L<RT::User> to use as the
+principal
 
 =item Type
 
@@ -2423,7 +2428,9 @@ Required.  One of the valid roles for this record, as returned by L</Roles>.
 
 =back
 
-Returns a tuple of (status, message).
+One, and only one, of I<PrincipalId> or I<User> is required.
+
+Returns a tuple of (principal object that was removed, message).
 
 =cut
 
@@ -2434,19 +2441,49 @@ sub DeleteRoleMember {
     return (0, $self->loc("No valid Type specified"))
         unless $args{Type} and $self->HasRole($args{Type});
 
+    if ($args{User}) {
+        my $user = RT::User->new( $self->CurrentUser );
+        $user->LoadByEmail( $args{User} );
+        $user->Load( $args{User} ) unless $user->id;
+        return (0, $self->loc("Could not load user '$args{User}'") )
+            unless $user->id;
+        $args{PrincipalId} = $user->PrincipalId;
+    }
+
     return (0, $self->loc("No valid PrincipalId"))
         unless $args{PrincipalId};
 
-    my ($ok, $msg) = $self->RoleGroup($args{Type})->_DeleteMember($args{PrincipalId});
+    my $principal = RT::Principal->new( $self->CurrentUser );
+    $principal->Load( $args{PrincipalId} );
+
+    my $acl = delete $args{ACL};
+    return (0, $self->loc("Permission denied"))
+        if $acl and not $acl->($principal);
+
+    my $group = $self->RoleGroup( $args{Type} );
+    return (0, $self->loc("Role group '$args{Type}' not found"))
+        unless $group->id;
+
+    return ( 0, $self->loc( '[_1] is not a [_2]',
+                            $principal->Object->Name, $self->loc($args{Type}) ) )
+        unless $group->HasMember($principal);
 
-    if ($ok and not $args{Silent}) {
+    my ($ok, $msg) = $group->_DeleteMember($args{PrincipalId});
+    unless ($ok) {
+        $RT::Logger->error("Failed to remove $args{PrincipalId} as a member of group ".$group->Id.": ".$msg);
+
+        return ( 0, $self->loc('Could not remove [_1] as a [_2]',
+                    $principal->Object->Name, $self->loc($args{Type})) );
+    }
+
+    unless ($args{Silent}) {
         $self->_NewTransaction(
             Type     => 'DelWatcher', # use "watcher" for history's sake
             OldValue => $args{PrincipalId},
             Field    => $args{Type},
         );
     }
-    return ($ok, $msg);
+    return ($principal, $msg);
 }
 
 sub _ResolveRoles {
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index e4163f9..da7f100 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -710,21 +710,13 @@ sub AddWatcher {
 }
 
 
+=head2 DeleteWatcher
 
+Applies access control checking, then calls L<RT::Record/DeleteRoleMember>.
+Additionally, C<Email> is accepted as an alternative argument name for
+C<User>.
 
-=head2 DeleteWatcher { Type => TYPE, PrincipalId => PRINCIPAL_ID, Email => EMAIL_ADDRESS }
-
-
-Deletes a Ticket watcher.  Takes two arguments:
-
-Type  (one of Requestor,Cc,AdminCc)
-
-and one of
-
-PrincipalId (an RT::Principal Id of the watcher you want to remove)
-    OR
-Email (the email address of an existing wathcer)
-
+Returns a tuple of (status, message).
 
 =cut
 
@@ -737,99 +729,28 @@ sub DeleteWatcher {
                  Email       => undef,
                  @_ );
 
-    unless ( $args{'PrincipalId'} || $args{'Email'} ) {
-        return ( 0, $self->loc("No principal specified") );
-    }
-    my $principal = RT::Principal->new( $self->CurrentUser );
-    if ( $args{'PrincipalId'} ) {
-
-        $principal->Load( $args{'PrincipalId'} );
-    }
-    else {
-        my $user = RT::User->new( $self->CurrentUser );
-        $user->LoadByEmail( $args{'Email'} );
-        $principal->Load( $user->Id );
-    }
-
-    # If we can't find this watcher, we need to bail.
-    unless ( $principal->Id ) {
-        return ( 0, $self->loc("Could not find that principal") );
-    }
-
-    my $group = $self->RoleGroup( $args{'Type'} );
-    unless ( $group->id ) {
-        return ( 0, $self->loc("Group not found") );
-    }
-
-    # Check ACLS
-    #If the watcher we're trying to add is for the current user
-    if ( $self->CurrentUser->PrincipalId == $principal->id ) {
-
-        #  If it's an AdminCc and they don't have
-        #   'WatchAsAdminCc' or 'ModifyTicket', bail
-        if ( $args{'Type'} eq 'AdminCc' ) {
-            unless (    $self->CurrentUserHasRight('ModifyTicket')
-                     or $self->CurrentUserHasRight('WatchAsAdminCc') ) {
-                return ( 0, $self->loc('Permission Denied') );
-            }
-        }
-
-        #  If it's a Requestor or Cc and they don't have
-        #   'Watch' or 'ModifyTicket', bail
-        elsif ( ( $args{'Type'} eq 'Cc' ) or ( $args{'Type'} eq 'Requestor' ) )
-        {
-            unless (    $self->CurrentUserHasRight('ModifyTicket')
-                     or $self->CurrentUserHasRight('Watch') ) {
-                return ( 0, $self->loc('Permission Denied') );
-            }
-        }
-        else {
-            $RT::Logger->warning("$self -> DeleteWatcher got passed a bogus type");
-            return ( 0,
-                     $self->loc('Error in parameters to Ticket->DeleteWatcher') );
-        }
-    }
-
-    # If the watcher isn't the current user
-    # and the current user  doesn't have 'ModifyTicket' bail
-    else {
-        unless ( $self->CurrentUserHasRight('ModifyTicket') ) {
-            return ( 0, $self->loc("Permission Denied") );
-        }
-    }
-
-    # see if this user is already a watcher.
-
-    unless ( $group->HasMember($principal) ) {
-        return ( 0,
-                 $self->loc( '[_1] is not a [_2] for this ticket',
-                             $principal->Object->Name, $args{'Type'} ) );
-    }
-
-    my ( $m_id, $m_msg ) = $group->_DeleteMember( $principal->Id );
-    unless ($m_id) {
-        $RT::Logger->error( "Failed to delete "
-                            . $principal->Id
-                            . " as a member of group "
-                            . $group->Id . ": "
-                            . $m_msg );
-
-        return (0,
-                $self->loc(
-                    'Could not remove [_1] as a [_2] for this ticket',
-                    $principal->Object->Name, $args{'Type'} ) );
-    }
+    $args{ACL} = sub {
+        my $principal = shift;
+        # ModifyTicket works in any case
+        return 1 if $self->CurrentUserHasRight('ModifyTicket');
+        # If the watcher isn't the current user then the current user has no right
+        return 0 unless $self->CurrentUser->id == $principal->id;
+        # If it's an AdminCc and they don't have 'WatchAsAdminCc', bail
+        return 0 if $args{Type} eq "AdminCc" and not $self->CurrentUserHasRight('WatchAsAdminCc');
+        # If it's a Requestor or Cc and they don't have 'Watch', bail
+        return 0 if ($args{Type} eq "Cc" or $args{'Type'} eq 'Requestor')
+            and not $self->CurrentUserHasRight('Watch');
+        return 1;
+    };
 
-    unless ( $args{'Silent'} ) {
-        $self->_NewTransaction( Type     => 'DelWatcher',
-                                OldValue => $principal->Id,
-                                Field    => $args{'Type'} );
-    }
+    $args{User} ||= delete $args{Email};
+    my ($principal, $msg) = $self->DeleteRoleMember( %args );
+    return ( 0, $msg ) unless $principal;
 
     return ( 1,
              $self->loc( "[_1] is no longer a [_2] for this ticket.",
                          $principal->Object->Name,
-                         $args{'Type'} ) );
+                         $self->loc($args{'Type'}) ) );
 }
 
 

commit edb7c7aacc0d4d83a832064cc9d23adbc88e5b06
Author: Alex Vandiver <alexmv at bestpractical.com>
Date:   Fri Dec 28 02:08:38 2012 -0500

    Factor out common _HasModifyWatcherRight code

diff --git a/lib/RT/Queue.pm b/lib/RT/Queue.pm
index dd10757..5a459b9 100644
--- a/lib/RT/Queue.pm
+++ b/lib/RT/Queue.pm
@@ -788,33 +788,20 @@ sub IsManageableRoleGroupType {
 }
 
 
-# _HasModifyWatcherRight {{{
 sub _HasModifyWatcherRight {
     my $self = shift;
-    my %args = (
-        Type  => undef,
-        PrincipalId => undef,
-        Email => undef,
-        @_
-    );
+    my ($type, $principal) = @_;
 
+    # ModifyQueueWatchers works in any case
     return 1 if $self->CurrentUserHasRight('ModifyQueueWatchers');
-
-    #If the watcher we're trying to add is for the current user
-    if ( defined $args{'PrincipalId'} && $self->CurrentUser->PrincipalId  eq $args{'PrincipalId'}) {
-        if ( $args{'Type'} eq 'AdminCc' ) {
-            return 1 if $self->CurrentUserHasRight('WatchAsAdminCc');
-        }
-        elsif ( $args{'Type'} eq 'Cc' or $args{'Type'} eq 'Requestor' ) {
-            return 1 if $self->CurrentUserHasRight('Watch');
-        }
-        else {
-            $RT::Logger->warning( "$self -> _HasModifyWatcher got passed a bogus type $args{Type}");
-            return ( 0, $self->loc('Invalid queue role group type [_1]', $args{Type}) );
-        }
-    }
-
-    return ( 0, $self->loc("Permission Denied") );
+    # If the watcher isn't the current user then the current user has no right
+    return 0 unless $self->CurrentUser->PrincipalId == $principal->id;
+    # If it's an AdminCc and they don't have 'WatchAsAdminCc', bail
+    return 0 if $type eq 'AdminCc' and not $self->CurrentUserHasRight('WatchAsAdminCc');
+    # If it's a Requestor or Cc and they don't have 'Watch', bail
+    return 0 if ($type eq "Cc" or $type eq 'Requestor')
+        and not $self->CurrentUserHasRight('Watch');
+    return 1;
 }
 
 
@@ -837,15 +824,7 @@ sub AddWatcher {
         @_
     );
 
-    $args{ACL} = sub {
-        my $principal = shift;
-        my ($ok, $msg) = $self->_HasModifyWatcherRight(
-            Type        => $args{Type},
-            PrincipalId => $principal->id
-        );
-        return $ok;
-    };
-
+    $args{ACL} = sub { $self->_HasModifyWatcherRight( @_ ) };
     $args{User} ||= delete $args{Email};
     my ($principal, $msg) = $self->AddRoleMember( %args );
     return ( 0, $msg) unless $principal;
@@ -855,7 +834,6 @@ sub AddWatcher {
 }
 
 
-
 =head2 DeleteWatcher
 
 Applies access control checking, then calls L<RT::Record/DeleteRoleMember>.
@@ -876,15 +854,7 @@ sub DeleteWatcher {
         @_
     );
 
-    $args{ACL} = sub {
-        my $principal = shift;
-        my ($ok, $msg) = $self->_HasModifyWatcherRight(
-            Type        => $args{Type},
-            PrincipalId => $principal->id
-        );
-        return $ok;
-    };
-
+    $args{ACL} = sub { $self->_HasModifyWatcherRight( @_ ) };
     $args{User} ||= delete $args{Email};
     my ($principal, $msg) = $self->DeleteRoleMember( %args );
     return ( 0, $msg) unless $principal;
diff --git a/lib/RT/Record.pm b/lib/RT/Record.pm
index 4cf971e..1e03f7d 100644
--- a/lib/RT/Record.pm
+++ b/lib/RT/Record.pm
@@ -2308,9 +2308,9 @@ Required.  One of the valid roles for this record, as returned by L</Roles>.
 
 =item ACL
 
-Optional.  A subroutine reference which will be passed the principal
-being added, once it has been resolved.  If it returns false, the method
-will fail with a status of "Permission denied".
+Optional.  A subroutine reference which will be passed the role type and
+principal being added.  If it returns false, the method will fail with a
+status of "Permission denied".
 
 =back
 
@@ -2375,7 +2375,7 @@ sub AddRoleMember {
 
     my $acl = delete $args{ACL};
     return (0, $self->loc("Permission denied"))
-        if $acl and not $acl->($principal);
+        if $acl and not $acl->($type => $principal);
 
     my $group = $self->RoleGroup( $type );
     return (0, $self->loc("Role group '$type' not found"))
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index da7f100..095cc18 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -663,6 +663,21 @@ sub OwnerGroup {
 }
 
 
+sub _HasModifyWatcherRight {
+    my $self = shift;
+    my ($type, $principal) = @_;
+
+    # ModifyTicket works in any case
+    return 1 if $self->CurrentUserHasRight('ModifyTicket');
+    # If the watcher isn't the current user then the current user has no right
+    return 0 unless $self->CurrentUser->PrincipalId == $principal->id;
+    # If it's an AdminCc and they don't have 'WatchAsAdminCc', bail
+    return 0 if $type eq 'AdminCc' and not $self->CurrentUserHasRight('WatchAsAdminCc');
+    # If it's a Requestor or Cc and they don't have 'Watch', bail
+    return 0 if ($type eq "Cc" or $type eq 'Requestor')
+        and not $self->CurrentUserHasRight('Watch');
+    return 1;
+}
 
 
 =head2 AddWatcher
@@ -684,20 +699,7 @@ sub AddWatcher {
         @_
     );
 
-    $args{ACL} = sub {
-        my $principal = shift;
-        # ModifyTicket works in any case
-        return 1 if $self->CurrentUserHasRight('ModifyTicket');
-        # If the watcher isn't the current user then the current user has no right
-        return 0 unless $self->CurrentUser->id == $principal->id;
-        # If it's an AdminCc and they don't have 'WatchAsAdminCc', bail
-        return 0 if $args{Type} eq "AdminCc" and not $self->CurrentUserHasRight('WatchAsAdminCc');
-        # If it's a Requestor or Cc and they don't have 'Watch', bail
-        return 0 if ($args{Type} eq "Cc" or $args{'Type'} eq 'Requestor')
-            and not $self->CurrentUserHasRight('Watch');
-        return 1;
-    };
-
+    $args{ACL} = sub { $self->_HasModifyWatcherRight( @_ ) };
     $args{User} ||= delete $args{Email};
     my ($principal, $msg) = $self->AddRoleMember(
         %args,
@@ -729,20 +731,7 @@ sub DeleteWatcher {
                  Email       => undef,
                  @_ );
 
-    $args{ACL} = sub {
-        my $principal = shift;
-        # ModifyTicket works in any case
-        return 1 if $self->CurrentUserHasRight('ModifyTicket');
-        # If the watcher isn't the current user then the current user has no right
-        return 0 unless $self->CurrentUser->id == $principal->id;
-        # If it's an AdminCc and they don't have 'WatchAsAdminCc', bail
-        return 0 if $args{Type} eq "AdminCc" and not $self->CurrentUserHasRight('WatchAsAdminCc');
-        # If it's a Requestor or Cc and they don't have 'Watch', bail
-        return 0 if ($args{Type} eq "Cc" or $args{'Type'} eq 'Requestor')
-            and not $self->CurrentUserHasRight('Watch');
-        return 1;
-    };
-
+    $args{ACL} = sub { $self->_HasModifyWatcherRight( @_ ) };
     $args{User} ||= delete $args{Email};
     my ($principal, $msg) = $self->DeleteRoleMember( %args );
     return ( 0, $msg ) unless $principal;

-----------------------------------------------------------------------


More information about the Rt-commit mailing list