[Rt-commit] rt branch, 4.4/add-watcher-custom-role, created. rt-4.4.4-61-gec537f98e

[Jim Brandt]

The branch, 4.4/add-watcher-custom-role has been created
        at  ec537f98e6dd5ffb00960bd120808977986383ad (commit)

- Log -----------------------------------------------------------------
commit 95e7b1bb0b40ba0764cae50c4b51c159c093a313
Author: Jim Brandt <jbrandt at bestpractical.com>
Date:   Thu Sep 5 14:06:54 2019 -0400

    Add test showing custom roles can be set without ModifyTicket

diff --git a/t/ticket/add-watchers.t b/t/ticket/add-watchers.t
index 12b0bb3e2..558755c89 100644
--- a/t/ticket/add-watchers.t
+++ b/t/ticket/add-watchers.t
@@ -1,4 +1,4 @@
-use RT::Test nodata => 1, tests => 34;
+use RT::Test nodata => 1, tests => undef;
 
 use strict;
 use warnings;
@@ -23,6 +23,18 @@ my $queue = RT::Queue->new(RT->SystemUser);
 my ($queue_id) = $queue->Create( Name => 'watcher tests '.$$);
 ok( $queue_id, 'queue created for watcher tests' );
 
+# Create custom role on the queue
+my $custom_role = RT::CustomRole->new(RT->SystemUser);
+my ($ok, $msg) = $custom_role->Create(
+    Name      => 'Custom Role',
+    MaxValues => 0,
+);
+ok($ok, "Created custom role: $msg");
+
+($ok, $msg) = $custom_role->AddToObject($queue->Id);
+ok($ok, "Added custom role to queue: $msg");
+my $custom_role_type = $custom_role->GroupType;
+
 # new privileged user to check rights
 my $user = RT::User->new( RT->SystemUser );
 my ($user_id) = $user->Create(
@@ -45,7 +57,8 @@ ok( !$user->HasRight( Right => 'ModifyTicket', Object => $queue ), "user can't m
 ok( !$user->HasRight( Right => 'Watch',        Object => $queue ), "user can't watch queue tickets" );
 
 my $ticket = RT::Ticket->new( RT->SystemUser );
-my ($rv, $msg) = $ticket->Create( Subject => 'watcher tests', Queue => $queue->Name );
+my $rv;
+($rv, $msg) = $ticket->Create( Subject => 'watcher tests', Queue => $queue->Name );
 ok( $ticket->id, "ticket created" );
 
 my $ticket2 = RT::Ticket->new( $cu );
@@ -57,6 +70,9 @@ ok( $ticket2->Subject, "ticket load by user" );
 ok( !$rv, "user can't add self as Cc" );
 ($rv, $msg) = $ticket2->AddWatcher( Type => 'Requestor', PrincipalId => $user->PrincipalId );
 ok( !$rv, "user can't add self as Requestor" );
+($rv, $msg) = $ticket2->AddWatcher( Type => $custom_role_type, PrincipalId => $user->PrincipalId );
+ok( !$rv, "user can't add self to Custom Role $msg" );
+
 $principal->GrantRight( Right => 'Watch'  , Object => $queue );
 ok(  $user->HasRight( Right => 'Watch',        Object => $queue ), "user can watch queue tickets" );
 ($rv, $msg) = $ticket2->AddWatcher( Type => 'Cc', PrincipalId => $user->PrincipalId );
@@ -125,3 +141,4 @@ ok(  $rv, "user can add self as Cc by Email" );
 ($rv, $msg) = $queue2->AddWatcher( Type => 'Requestor', Email => $user->EmailAddress );
 ok(  $rv, "user can add self as Requestor by Email" );
 
+done_testing();

commit ec537f98e6dd5ffb00960bd120808977986383ad
Author: Jim Brandt <jbrandt at bestpractical.com>
Date:   Thu Sep 5 14:29:15 2019 -0400

    Fall through to false on watcher rights check
    
    _HasModifyWatcherRight previously checked known rights cases
    and returned false when the user didn't have rights for the
    corresponding change. If nothing matched, the fall through return
    value was true, which meant that any watcher change that didn't
    match was allowed. All new custom roles won't match the existing
    watcher type checks, so users could by default edit custom roles
    even without ModifyTicket.
    
    Reverse the logic to return true for know rights and types and
    default to 0 for any unknown cases. At this point, ModifyTicket
    is required to modify custom roles.

diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index 38708af5f..81224831c 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -620,12 +620,14 @@ sub _HasModifyWatcherRight {
     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;
+    # If it's an AdminCc and they have 'WatchAsAdminCc', they can modify
+    return 1 if $type eq 'AdminCc' and $self->CurrentUserHasRight('WatchAsAdminCc');
+    # If it's a Requestor or Cc and they have 'Watch', they can modify
+    return 1 if ($type eq "Cc" or $type eq 'Requestor')
+        and $self->CurrentUserHasRight('Watch');
+
+    # Unknown type, so default to denied.
+    return 0;
 }
 
 

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