[Rt-commit] r5373 - in rt/branches/3.7-EXPERIMENTAL: . etc lib/t/regression

ruz at bestpractical.com ruz at bestpractical.com
Thu Jun 15 17:53:05 EDT 2006


Author: ruz
Date: Thu Jun 15 17:53:04 2006
New Revision: 5373

Modified:
   rt/branches/3.7-EXPERIMENTAL/   (props changed)
   rt/branches/3.7-EXPERIMENTAL/etc/RT_Config.pm.in
   rt/branches/3.7-EXPERIMENTAL/lib/RT/Record.pm
   rt/branches/3.7-EXPERIMENTAL/lib/RT/Ticket_Overlay.pm
   rt/branches/3.7-EXPERIMENTAL/lib/t/regression/14-linking.t

Log:
 r3115 at cubic-pc (orig r5292):  ruz | 2006-05-26 02:12:05 +0400
 * New option StrictLinkACL
   # When this feature is enabled an user need ModifyTicket right on both
   # tickets to link them together, otherwise he can have right on any of
   # two.
 ** update Create, _?AddLink, DeleteLink methods
 * fix: we created transaction if some tries to create link that allready
   exists
 * move all acl checks out from _AddLink to AddLink method
 * cover with every change with tests


Modified: rt/branches/3.7-EXPERIMENTAL/etc/RT_Config.pm.in
==============================================================================
--- rt/branches/3.7-EXPERIMENTAL/etc/RT_Config.pm.in	(original)
+++ rt/branches/3.7-EXPERIMENTAL/etc/RT_Config.pm.in	Thu Jun 15 17:53:04 2006
@@ -593,6 +593,11 @@
 # only one of the link transactions to have scrips run.
 Set($LinkTransactionsRun1Scrip , 0);
 
+# When this feature is enabled an user need ModifyTicket right on both
+# tickets to link them together, otherwise he can have right on any of
+# two.
+Set($StrictLinkACL, 1);
+
 # Set $PreviewScripMessages to 1 if the scrips preview on the ticket
 # reply page should include the content of the messages to be sent.
 

Modified: rt/branches/3.7-EXPERIMENTAL/lib/RT/Record.pm
==============================================================================
--- rt/branches/3.7-EXPERIMENTAL/lib/RT/Record.pm	(original)
+++ rt/branches/3.7-EXPERIMENTAL/lib/RT/Record.pm	Thu Jun 15 17:53:04 2006
@@ -186,7 +186,7 @@
                                       Description => $args{'Description'},
                                       Content     => $args{'Content'} );
 
-                                     
+
     # XXX TODO: Why won't RedoSearch work here?                                     
     $self->Attributes->_DoSearch;
     
@@ -1257,6 +1257,8 @@
 
 Takes a paramhash of Type and one of Base or Target. Adds that link to this ticket.
 
+Returns C<link id>, C<message> and C<exist> flag.
+
 
 =cut
 
@@ -1302,7 +1304,7 @@
                              Target => $args{'Target'} );
     if ( $old_link->Id ) {
         $RT::Logger->debug("$self Somebody tried to duplicate a link");
-        return ( $old_link->id, $self->loc("Link already exists") );
+        return ( $old_link->id, $self->loc("Link already exists"), 1 );
     }
 
     # }}}
@@ -1367,7 +1369,7 @@
         $direction='Base';
     }
     else {
-        $RT::Logger->debug("$self: Base or Target must be specified\n");
+        $RT::Logger->error("Base or Target must be specified\n");
         return ( 0, $self->loc('Either base or target must be specified') );
     }
 

Modified: rt/branches/3.7-EXPERIMENTAL/lib/RT/Ticket_Overlay.pm
==============================================================================
--- rt/branches/3.7-EXPERIMENTAL/lib/RT/Ticket_Overlay.pm	(original)
+++ rt/branches/3.7-EXPERIMENTAL/lib/RT/Ticket_Overlay.pm	Thu Jun 15 17:53:04 2006
@@ -678,6 +678,20 @@
         foreach my $link (
             ref( $args{$type} ) ? @{ $args{$type} } : ( $args{$type} ) )
         {
+            # Check rights on the other end of the link if we must
+            # then run _AddLink that doesn't check for ACLs
+            if ( RT->Config->Get( 'StrictLinkACL' ) ) {
+                my ($val, $msg, $obj) = $self->__GetTicketFromURI( URI => $link );
+                unless ( $val ) {
+                    push @non_fatal_errors, $msg;
+                    next;
+                }
+                if ( $obj && !$obj->CurrentUserHasRight('ModifyTicket') ) {
+                    push @non_fatal_errors, $self->loc('Linking. Permission denied');
+                    next;
+                }
+            }
+            
             my ( $wval, $wmsg ) = $self->_AddLink(
                 Type                          => $LINKTYPEMAP{$type}->{'Type'},
                 $LINKTYPEMAP{$type}->{'Mode'} => $link,
@@ -2491,11 +2505,29 @@
         @_
     );
 
+    unless ( $args{'Target'} || $args{'Base'} ) {
+        $RT::Logger->error("Base or Target must be specified\n");
+        return ( 0, $self->loc('Either base or target must be specified') );
+    }
+
     #check acls
-    unless ( $self->CurrentUserHasRight('ModifyTicket') ) {
-        $RT::Logger->debug("No permission to delete links\n");
-        return ( 0, $self->loc('Permission Denied'))
+    my $right = 0;
+    $right++ if $self->CurrentUserHasRight('ModifyTicket');
+    if ( !$right && RT->Config->Get( 'StrictLinkACL' ) ) {
+        return ( 0, $self->loc("Permission Denied") );
+    }
 
+    # If the other URI is an RT::Ticket, we want to make sure the user
+    # can modify it too...
+    my ($status, $msg, $other_ticket) = $self->__GetTicketFromURI( URI => $args{'Target'} || $args{'Base'} );
+    return (0, $msg) unless $status;
+    if ( !$other_ticket || $other_ticket->CurrentUserHasRight('ModifyTicket') ) {
+        $right++;
+    }
+    if ( ( !RT->Config->Get( 'StrictLinkACL' ) && $right == 0 ) ||
+         ( RT->Config->Get( 'StrictLinkACL' ) && $right < 2 ) )
+    {
+        return ( 0, $self->loc("Permission Denied") );
     }
 
     my ($val, $Msg) = $self->SUPER::_DeleteLink(%args);
@@ -2521,7 +2553,7 @@
         my ( $Trans, $Msg, $TransObj ) = $self->_NewTransaction(
             Type      => 'DeleteLink',
             Field     => $LINKDIRMAP{$args{'Type'}}->{$direction},
-            OldValue  =>  $remote_uri->URI || $remote_link,
+            OldValue  => $remote_uri->URI || $remote_link,
             TimeTaken => 0
         );
         $RT::Logger->error("Couldn't create transaction: $Msg") unless $Trans;
@@ -2568,13 +2600,52 @@
                  SilentTarget => undef,
                  @_ );
 
+    unless ( $args{'Target'} || $args{'Base'} ) {
+        $RT::Logger->error("Base or Target must be specified\n");
+        return ( 0, $self->loc('Either base or target must be specified') );
+    }
 
-    unless ( $self->CurrentUserHasRight('ModifyTicket') ) {
+    my $right = 0;
+    $right++ if $self->CurrentUserHasRight('ModifyTicket');
+    if ( !$right && RT->Config->Get( 'StrictLinkACL' ) ) {
         return ( 0, $self->loc("Permission Denied") );
     }
 
+    # If the other URI is an RT::Ticket, we want to make sure the user
+    # can modify it too...
+    my ($status, $msg, $other_ticket) = $self->__GetTicketFromURI( URI => $args{'Target'} || $args{'Base'} );
+    return (0, $msg) unless $status;
+    if ( !$other_ticket || $other_ticket->CurrentUserHasRight('ModifyTicket') ) {
+        $right++;
+    }
+    if ( ( !RT->Config->Get( 'StrictLinkACL' ) && $right == 0 ) ||
+         ( RT->Config->Get( 'StrictLinkACL' ) && $right < 2 ) )
+    {
+        return ( 0, $self->loc("Permission Denied") );
+    }
 
-    $self->_AddLink(%args);
+    return $self->_AddLink(%args);
+}
+
+sub __GetTicketFromURI {
+    my $self = shift;
+    my %args = ( URI => '', @_ );
+
+    # If the other URI is an RT::Ticket, we want to make sure the user
+    # can modify it too...
+    my $uri_obj = RT::URI->new( $self->CurrentUser );
+    $uri_obj->FromURI( $args{'URI'} );
+
+    unless ( $uri_obj->Resolver && $uri_obj->Scheme ) {
+	    my $msg = $self->loc( "Couldn't resolve '[_1]' into a URI.", $args{'URI'} );
+        $RT::Logger->warning( "$msg\n" );
+        return( 0, $msg );
+    }
+    my $obj = $uri_obj->Resolver->Object;
+    unless ( UNIVERSAL::isa($obj, 'RT::Ticket') && $obj->id ) {
+        return (1, 'Found not a ticket', undef);
+    }
+    return (1, 'Found ticket', $obj);
 }
 
 =head2 _AddLink  
@@ -2593,45 +2664,9 @@
                  SilentTarget => undef,
                  @_ );
 
-    # {{{ If the other URI is an RT::Ticket, we want to make sure the user
-    # can modify it too...
-    my $other_ticket_uri = RT::URI->new($self->CurrentUser);
-
-    if ( $args{'Target'} ) {
-        $other_ticket_uri->FromURI( $args{'Target'} );
-    }
-    elsif ( $args{'Base'} ) {
-        $other_ticket_uri->FromURI( $args{'Base'} );
-    }
-
-    unless ( $other_ticket_uri->Resolver && $other_ticket_uri->Scheme ) {
-	my $msg = $args{'Target'} ? $self->loc("Couldn't resolve target '[_1]' into a URI.", $args{'Target'})
-          : $self->loc("Couldn't resolve base '[_1]' into a URI.", $args{'Base'});
-        $RT::Logger->warning( "$self $msg\n" );
-
-        return( 0, $msg );
-    }
-
-    # By popular demand, adding a ticket link doesn't check ACLs on the second ticket 
-    #
-    #    if ( $other_ticket_uri->Resolver->Scheme eq 'fsck.com-rt') {
-    #        my $object = $other_ticket_uri->Resolver->Object;
-    #
-    #        if (   UNIVERSAL::isa( $object, 'RT::Ticket' )
-    #            && $object->id
-    #            && !$object->CurrentUserHasRight('ModifyTicket') )
-    #        {
-    #            return ( 0, $self->loc("Permission Denied") );
-    #        }
-    #
-    #    }
-    #
-    # }}}
-
-    my ($val, $Msg) = $self->SUPER::_AddLink(%args);
-    return ($val, $Msg) unless $val;
-
-    return ($val, $Msg) if $args{'Silent'};
+    my ($val, $msg, $exist) = $self->SUPER::_AddLink(%args);
+    return ($val, $msg) if !$val || $exist;
+    return ($val, $msg) if $args{'Silent'};
 
     my ($direction, $remote_link);
     if ( $args{'Target'} ) {
@@ -2657,7 +2692,7 @@
 
     if ( !$args{ 'Silent'. ( $direction eq 'Target'? 'Base': 'Target' ) } && $remote_uri->IsLocal ) {
         my $OtherObj = $remote_uri->Object;
-        my ( $val, $Msg ) = $OtherObj->_NewTransaction(
+        my ( $val, $msg ) = $OtherObj->_NewTransaction(
             Type           => 'AddLink',
             Field          => $direction eq 'Target' ? $LINKDIRMAP{$args{'Type'}}->{Base}
                                             : $LINKDIRMAP{$args{'Type'}}->{Target},
@@ -2668,8 +2703,7 @@
         $RT::Logger->error("Couldn't create transaction: $Msg") unless $val;
     }
 
-    return ( $val, $Msg );
-
+    return ( $val, $msg );
 }
 
 # }}}

Modified: rt/branches/3.7-EXPERIMENTAL/lib/t/regression/14-linking.t
==============================================================================
--- rt/branches/3.7-EXPERIMENTAL/lib/t/regression/14-linking.t	(original)
+++ rt/branches/3.7-EXPERIMENTAL/lib/t/regression/14-linking.t	Thu Jun 15 17:53:04 2006
@@ -1,4 +1,5 @@
-use Test::More tests => '59';
+use Test::More  tests => '90';
+
 use strict;
 use warnings;
 
@@ -17,6 +18,9 @@
 my $link_scrips_orig = RT->Config->Get( 'LinkTransactionsRun1Scrip' );
 RT->Config->Set( 'LinkTransactionsRun1Scrip', 1 );
 
+my $link_acl_checks_orig = RT->Config->Get( 'StrictLinkACL' );
+RT->Config->Set( 'StrictLinkACL', 1);
+
 my $condition = RT::ScripCondition->new( $RT::SystemUser );
 $condition->Load('User Defined');
 ok($condition->id);
@@ -80,8 +84,7 @@
 ok($id, "Scrip created");
 
 my $u1 = RT::User->new($RT::SystemUser);
-($id,$msg) =$u1->Create(Name => "LinkTestUser.$$");
-
+($id,$msg) = $u1->Create(Name => "LinkTestUser.$$");
 ok ($id,$msg);
 
 # grant ShowTicket right to allow count transactions
@@ -89,29 +92,120 @@
 ok ($id,$msg);
 ($id,$msg) = $u1->PrincipalObj->GrantRight ( Object => $q2, Right => 'ShowTicket');
 ok ($id,$msg);
-
 ($id,$msg) = $u1->PrincipalObj->GrantRight ( Object => $q1, Right => 'CreateTicket');
 ok ($id,$msg);
+
+my $creator = RT::CurrentUser->new($u1->id);
+
+diag('Create tickets without rights to link') if $ENV{'TEST_VERBOSE'};
+{
+    # on q2 we have no rights, yet
+    my $parent = RT::Ticket->new( $RT::SystemUser );
+    ($id,$tid,$msg) = $parent->Create( Subject => 'Link test 1', Queue => $q2->id );
+    ok($id,$msg);
+    my $child = RT::Ticket->new( $creator );
+    ($id,$tid,$msg) = $child->Create( Subject => 'Link test 1', Queue => $q1->id, MemberOf => $parent->id );
+    ok($id,$msg);
+    $child->CurrentUser( $RT::SystemUser );
+    is($child->_Links('Base')->Count, 0, 'link was not created, no permissions');
+    is($child->_Links('Target')->Count, 0, 'link was not create, no permissions');
+}
+
+diag('Create tickets with rights checks on one end of a link') if $ENV{'TEST_VERBOSE'};
+{
+    # on q2 we have no rights, but use checking one only on thing
+    local $RT::StrictLinkACL = 0;
+    my $parent = RT::Ticket->new( $RT::SystemUser );
+    ($id,$tid,$msg) = $parent->Create( Subject => 'Link test 1', Queue => $q2->id );
+    ok($id,$msg);
+    my $child = RT::Ticket->new( $creator );
+    ($id,$tid,$msg) = $child->Create( Subject => 'Link test 1', Queue => $q1->id, MemberOf => $parent->id );
+    ok($id,$msg);
+    $child->CurrentUser( $RT::SystemUser );
+    is($child->_Links('Base')->Count, 1, 'link was created');
+    is($child->_Links('Target')->Count, 0, 'link was created only one');
+    # no scrip run on second ticket accroding to config option
+    is(link_count($filename), 0, "scrips ok"); 
+}
+
 ($id,$msg) = $u1->PrincipalObj->GrantRight ( Object => $q1, Right => 'ModifyTicket');
 ok ($id,$msg);
 
-my $tid;
+diag('try to add link without rights') if $ENV{'TEST_VERBOSE'};
+{
+    # on q2 we have no rights, yet
+    my $parent = RT::Ticket->new( $RT::SystemUser );
+    ($id,$tid,$msg) = $parent->Create( Subject => 'Link test 1', Queue => $q2->id );
+    ok($id,$msg);
+    my $child = RT::Ticket->new( $creator );
+    ($id,$tid,$msg) = $child->Create( Subject => 'Link test 1', Queue => $q1->id );
+    ok($id,$msg);
+    my ($id, $msg) = $child->AddLink(Type => 'MemberOf', Target => $parent->id);
+    ok(!$id, $msg);
+    is(link_count($filename), 0, "scrips ok");
+    $child->CurrentUser( $RT::SystemUser );
+    is($child->_Links('Base')->Count, 0, 'link was not created, no permissions');
+    is($child->_Links('Target')->Count, 0, 'link was not create, no permissions');
+}
 
-my $creator = RT::CurrentUser->new($u1->id);
+diag('add link with rights only on base') if $ENV{'TEST_VERBOSE'};
+{
+    # on q2 we have no rights, but use checking one only on thing
+    local $RT::StrictLinkACL = 0;
+    my $parent = RT::Ticket->new( $RT::SystemUser );
+    ($id,$tid,$msg) = $parent->Create( Subject => 'Link test 1', Queue => $q2->id );
+    ok($id,$msg);
+    my $child = RT::Ticket->new( $creator );
+    ($id,$tid,$msg) = $child->Create( Subject => 'Link test 1', Queue => $q1->id );
+    ok($id,$msg);
+    my ($id, $msg) = $child->AddLink(Type => 'MemberOf', Target => $parent->id);
+    ok($id, $msg);
+    is(link_count($filename), 1, "scrips ok");
+    $child->CurrentUser( $RT::SystemUser );
+    is($child->_Links('Base')->Count, 1, 'link was created');
+    is($child->_Links('Target')->Count, 0, 'link was created only one');
+    $child->CurrentUser( $creator );
+
+    # turn off feature and try to delete link, we should fail
+    $RT::StrictLinkACL = 1;
+    my ($id, $msg) = $child->AddLink(Type => 'MemberOf', Target => $parent->id);
+    ok(!$id, $msg);
+    is(link_count($filename), 1, "scrips ok");
+    $child->CurrentUser( $RT::SystemUser );
+    $child->_Links('Base')->_DoCount;
+    is($child->_Links('Base')->Count, 1, 'link was not deleted');
+    $child->CurrentUser( $creator );
+
+    # try to delete link, we should success as feature is active
+    $RT::StrictLinkACL = 0;
+    my ($id, $msg) = $child->DeleteLink(Type => 'MemberOf', Target => $parent->id);
+    ok($id, $msg);
+    is(link_count($filename), 0, "scrips ok");
+    $child->CurrentUser( $RT::SystemUser );
+    $child->_Links('Base')->_DoCount;
+    is($child->_Links('Base')->Count, 0, 'link was deleted');
+}
 
+my $tid;
 my $ticket = RT::Ticket->new( $creator);
 ok($ticket->isa('RT::Ticket'));
 ($id,$tid, $msg) = $ticket->Create(Subject => 'Link test 1', Queue => $q1->id);
 ok ($id,$msg);
 
+diag('try link to itself') if $ENV{'TEST_VERBOSE'};
+{
+    my ($id, $msg) = $ticket->AddLink(Type => 'RefersTo', Target => $ticket->id);
+    ok(!$id, $msg);
+    is(link_count($filename), 0, "scrips ok");
+}
 
 my $ticket2 = RT::Ticket->new($RT::SystemUser);
 ($id, $tid, $msg) = $ticket2->Create(Subject => 'Link test 2', Queue => $q2->id);
 ok ($id, $msg);
-
 ($id,$msg) =$ticket->AddLink(Type => 'RefersTo', Target => $ticket2->id);
 ok(!$id,$msg);
 is(link_count($filename), 0, "scrips ok");
+
 ($id,$msg) = $u1->PrincipalObj->GrantRight ( Object => $q2, Right => 'CreateTicket');
 ok ($id,$msg);
 ($id,$msg) = $u1->PrincipalObj->GrantRight ( Object => $q2, Right => 'ModifyTicket');
@@ -121,6 +215,8 @@
 is(link_count($filename), 1, "scrips ok");
 ($id,$msg) = $ticket->AddLink(Type => 'RefersTo', Target => -1);
 ok(!$id,$msg);
+($id,$msg) = $ticket->AddLink(Type => 'RefersTo', Target => $ticket2->id);
+ok($id,$msg);
 is(link_count($filename), 1, "scrips ok");
 
 my $transactions = $ticket2->Transactions;
@@ -201,6 +297,7 @@
 
 # restore
 RT->Config->Set( LinkTransactionsRun1Scrip => $link_scrips_orig );
+RT->Config->Set( StrictLinkACL => $link_acl_checks_orig );
 
 {
     my $Scrips = RT::Scrips->new( $RT::SystemUser );


More information about the Rt-commit mailing list