[Rt-commit] rt branch, 4.4/custom-role-check-right, created. rt-4.4.3-122-g59b4401256

? sunnavy sunnavy at bestpractical.com
Thu Dec 20 10:10:54 EST 2018


The branch, 4.4/custom-role-check-right has been created
        at  59b440125640a4bd75f0d05cc00d8909229f3cb8 (commit)

- Log -----------------------------------------------------------------
commit e5d9033c3bf1dd82f6e32e39c5d33449c8f1bc95
Author: sunnavy <sunnavy at bestpractical.com>
Date:   Wed May 2 05:06:00 2018 +0800

    Make sure RT::Queue::CustomRoles returns an empty collection if no rights
    
    Previously it did behave like an empty collection(i.e. when there are no
    Limit or UnLimit calls on it), but in mason, we call extra
    limits(LimitToSingleValue/LimitToMultipleValue) on it, which breaked it.

diff --git a/lib/RT/Queue.pm b/lib/RT/Queue.pm
index 52b9f10330..a05bb1c83f 100644
--- a/lib/RT/Queue.pm
+++ b/lib/RT/Queue.pm
@@ -483,6 +483,9 @@ sub CustomRoles {
         $roles->LimitToObjectId( $self->Id );
         $roles->ApplySortOrder;
     }
+    else {
+        $roles->Limit( FIELD => 'id', VALUE => 0, SUBCLAUSE => 'ACL' );
+    }
     return ($roles);
 }
 

commit e7c293b65e20067de41e6ab75ab87136eb6798b3
Author: sunnavy <sunnavy at bestpractical.com>
Date:   Wed May 2 05:16:55 2018 +0800

    Filter queue custom roles by checking current user's right
    
    Previously, "Roles" rerturned all the roles registered for the object
    and didn't consider if current user has right to see them or not.
    "Role" and "HasRole" have the same issue.
    
    This commit adds the rights check to AppliesToObjectPredicate, which is
    called in "Roles".  As "HasRole" calls "Roles" internally, it's fixed
    automatically.  "Role" is tweaked to add the rights check via "HasRole".

diff --git a/lib/RT/CustomRole.pm b/lib/RT/CustomRole.pm
index 8c9b084519..4283eed6bd 100644
--- a/lib/RT/CustomRole.pm
+++ b/lib/RT/CustomRole.pm
@@ -207,14 +207,16 @@ sub _RegisterAsRole {
                 return 1;
             }
 
-            # custom roles apply to queues, so canonicalize a ticket
-            # into its queue
-            if ($object->isa('RT::Ticket')) {
-                $object = $object->QueueObj;
-            }
+            if ( $object->isa('RT::Ticket') || $object->isa('RT::Queue') ) {
+                return 0 unless $object->CurrentUserHasRight('SeeQueue');
 
-            if ($object->isa('RT::Queue')) {
-                return $role->IsAdded($object->Id);
+                # custom roles apply to queues, so canonicalize a ticket
+                # into its queue
+                if ( $object->isa('RT::Ticket') ) {
+                    $object = $object->QueueObj;
+                }
+
+                return $role->IsAdded( $object->Id );
             }
 
             return 0;
diff --git a/lib/RT/Record/Role/Roles.pm b/lib/RT/Record/Role/Roles.pm
index 1a88793aec..6f79ea89f9 100644
--- a/lib/RT/Record/Role/Roles.pm
+++ b/lib/RT/Record/Role/Roles.pm
@@ -248,7 +248,10 @@ Returns an empty hashref if the role doesn't exist.
 =cut
 
 sub Role {
-    return \%{ $_[0]->_ROLES->{$_[1]} || {} };
+    my $self = shift;
+    my $type = shift;
+    return {} unless $self->HasRole( $type );
+    return \%{ $self->_ROLES->{$type} };
 }
 
 =head2 Roles
@@ -287,7 +290,7 @@ sub Roles {
                 $ok }
             grep { !$_->[1]{AppliesToObjectPredicate}
                  or $_->[1]{AppliesToObjectPredicate}->($self) }
-             map { [ $_, $self->Role($_) ] }
+             map { [ $_, $self->_ROLES->{$_} ] }
             keys %{ $self->_ROLES };
 }
 

commit 59b440125640a4bd75f0d05cc00d8909229f3cb8
Author: sunnavy <sunnavy at bestpractical.com>
Date:   Wed May 2 05:35:15 2018 +0800

    Test custom roles for user rights

diff --git a/t/customroles/basic.t b/t/customroles/basic.t
index d703eee395..0ab458483b 100644
--- a/t/customroles/basic.t
+++ b/t/customroles/basic.t
@@ -133,9 +133,30 @@ diag 'roles now added to queues' if $ENV{'TEST_VERBOSE'};
     is_deeply([sort RT::Ticket->Roles], ['AdminCc', 'Cc', 'Owner', 'RT::CustomRole-1', 'RT::CustomRole-2', 'Requestor'], 'Ticket->Roles');
     is_deeply([sort RT::Queue->ManageableRoleGroupTypes], ['AdminCc', 'Cc', 'RT::CustomRole-2'], 'Queue->ManageableRoleTypes');
 
+    my $alice = RT::Test->load_or_create_user( EmailAddress => 'alice at example.com' );
+    for my $q ( $general, $inbox, $specs, $development ) {
+        my $queue = RT::Queue->new( $alice );
+        $queue->Load( $q->id );
+        ok( $queue->id, 'Load queue' );
+
+        my $qroles = $queue->CustomRoles;
+        is( $qroles->Count, 0, 'No custom roles for users without rights' );
+        $qroles->LimitToSingleValue;
+        is( $qroles->Count, 0, 'No single custom roles for users without rights' );
+
+        is_deeply( [ sort $queue->Roles ], [ 'AdminCc', 'Cc', 'Owner', 'Requestor' ], 'Roles' );
+        is_deeply( [ sort $queue->ManageableRoleGroupTypes ], [ 'AdminCc', 'Cc' ], 'ManageableRoleTypes' );
+        ok( !$queue->HasRole( $engineer->GroupType ), 'HasRole returns false for users without rights' );
+        ok( !$queue->HasRole( $sales->GroupType ), 'HasRole returns false for users without rights' );
+    }
+
+    RT::Test->set_rights( { Principal => $alice->PrincipalObj, Right => ['SeeQueue'] } );
+
+    my @users = ( RT->SystemUser, $alice );
+    for my $user ( @users ) {
     # General
     {
-        my $roles = RT::CustomRoles->new(RT->SystemUser);
+        my $roles = RT::CustomRoles->new($user);
         $roles->LimitToObjectId($general->Id);
         is($roles->Count, 0, 'no roles for General');
 
@@ -152,7 +173,7 @@ diag 'roles now added to queues' if $ENV{'TEST_VERBOSE'};
 
     # Inbox
     {
-        my $roles = RT::CustomRoles->new(RT->SystemUser);
+        my $roles = RT::CustomRoles->new($user);
         $roles->LimitToObjectId($inbox->Id);
         is($roles->Count, 1, 'one role for Inbox');
         is($roles->Next->Name, 'Sales-' . $$, 'and the one role is Sales');
@@ -171,7 +192,7 @@ diag 'roles now added to queues' if $ENV{'TEST_VERBOSE'};
 
     # Specs
     {
-        my $roles = RT::CustomRoles->new(RT->SystemUser);
+        my $roles = RT::CustomRoles->new($user);
         $roles->LimitToObjectId($specs->Id);
         $roles->OrderBy(
             FIELD => 'id',
@@ -200,7 +221,7 @@ diag 'roles now added to queues' if $ENV{'TEST_VERBOSE'};
 
     # Development
     {
-        my $roles = RT::CustomRoles->new(RT->SystemUser);
+        my $roles = RT::CustomRoles->new($user);
         $roles->LimitToObjectId($development->Id);
         is($roles->Count, 1, 'one role for Development');
         is($roles->Next->Name, 'Engineer-' . $$, 'and the one role is sales');
@@ -216,6 +237,7 @@ diag 'roles now added to queues' if $ENV{'TEST_VERBOSE'};
         is_deeply([sort $development->ManageableRoleGroupTypes], ['AdminCc', 'Cc'], 'Development->ManageableRoleTypes');
         is_deeply([grep { $development->IsManageableRoleGroupType($_) } 'AdminCc', 'Cc', 'Owner', 'RT::CustomRole-1', 'RT::CustomRole-2', 'Requestor', 'Nonexistent'], ['AdminCc', 'Cc'], 'Development IsManageableRoleGroupType');
     }
+    }
 }
 
 diag 'role names' if $ENV{'TEST_VERBOSE'};

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


More information about the rt-commit mailing list