[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