[Rt-commit] r5292 - in rt/branches/3.5-TESTING: etc lib/RT
ruz at bestpractical.com
ruz at bestpractical.com
Thu May 25 18:12:07 EDT 2006
Author: ruz
Date: Thu May 25 18:12:05 2006
New Revision: 5292
Modified:
rt/branches/3.5-TESTING/etc/RT_Config.pm.in
rt/branches/3.5-TESTING/lib/RT/Record.pm
rt/branches/3.5-TESTING/lib/RT/Ticket_Overlay.pm
rt/branches/3.5-TESTING/lib/t/regression/14linking.t
Log:
* 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.5-TESTING/etc/RT_Config.pm.in
==============================================================================
--- rt/branches/3.5-TESTING/etc/RT_Config.pm.in (original)
+++ rt/branches/3.5-TESTING/etc/RT_Config.pm.in Thu May 25 18:12:05 2006
@@ -512,6 +512,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);
+
# }}}
Modified: rt/branches/3.5-TESTING/lib/RT/Record.pm
==============================================================================
--- rt/branches/3.5-TESTING/lib/RT/Record.pm (original)
+++ rt/branches/3.5-TESTING/lib/RT/Record.pm Thu May 25 18:12:05 2006
@@ -188,7 +188,7 @@
Description => $args{'Description'},
Content => $args{'Content'} );
-
+
# XXX TODO: Why won't RedoSearch work here?
$self->Attributes->_DoSearch;
@@ -1259,6 +1259,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
@@ -1304,7 +1306,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 );
}
# }}}
@@ -1369,7 +1371,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.5-TESTING/lib/RT/Ticket_Overlay.pm
==============================================================================
--- rt/branches/3.5-TESTING/lib/RT/Ticket_Overlay.pm (original)
+++ rt/branches/3.5-TESTING/lib/RT/Ticket_Overlay.pm Thu May 25 18:12:05 2006
@@ -679,6 +679,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::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,
@@ -2480,11 +2494,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::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::StrictLinkACL && $right == 0 ) ||
+ ( $RT::StrictLinkACL && $right < 2 ) )
+ {
+ return ( 0, $self->loc("Permission Denied") );
}
my ($val, $Msg) = $self->SUPER::_DeleteLink(%args);
@@ -2552,13 +2584,52 @@
Silent => 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::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::StrictLinkACL && $right == 0 ) ||
+ ( $RT::StrictLinkACL && $right < 2 ) )
+ {
return ( 0, $self->loc("Permission Denied") );
}
+ 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'} );
- $self->_AddLink(%args);
+ 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
@@ -2575,47 +2646,8 @@
Silent => 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);
-
- if (!$val) {
- return ($val, $Msg);
- }
+ my ($val, $msg, $exist) = $self->SUPER::_AddLink(%args);
+ return ($val, $msg) if !$val || $exist;
my ($direction, $remote_link);
if ( $args{'Target'} ) {
@@ -2628,10 +2660,10 @@
# Don't write the transaction if we're doing this on create
if ( $args{'Silent'} ) {
- return ( $val, $Msg );
+ return ( $val, $msg );
}
else {
- my $remote_uri = RT::URI->new( $self->CurrentUser );
+ my $remote_uri = RT::URI->new( $self->CurrentUser );
$remote_uri->FromURI( $remote_link );
#Write the transaction
Modified: rt/branches/3.5-TESTING/lib/t/regression/14linking.t
==============================================================================
--- rt/branches/3.5-TESTING/lib/t/regression/14linking.t (original)
+++ rt/branches/3.5-TESTING/lib/t/regression/14linking.t Thu May 25 18:12:05 2006
@@ -1,4 +1,4 @@
-use Test::More tests => '39';
+use Test::More tests => '70';
use_ok('RT');
use_ok('RT::Ticket');
use_ok('RT::ScripConditions');
@@ -12,7 +12,9 @@
use File::Temp qw/tempfile/;
my ($fh, $filename) = tempfile( UNLINK => 1, SUFFIX => '.rt');
my $link_scrips_orig = $RT::LinkTransactionsRun1Scrip;
+my $link_acl_chacks_orig = $RT::StrictLinkACL;
$RT::LinkTransactionsRun1Scrip = 1;
+$RT::StrictLinkACL = 1;
my $condition = RT::ScripCondition->new( $RT::SystemUser );
$condition->Load('User Defined');
@@ -68,32 +70,123 @@
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);
+my $creator = RT::CurrentUser->new($u1->id);
+
($id,$msg) = $u1->PrincipalObj->GrantRight ( Object => $q1, Right => 'CreateTicket');
ok ($id,$msg);
+
+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);
ok(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');
@@ -104,6 +197,9 @@
($id,$msg) =$ticket->AddLink(Type => 'RefersTo', Target => -1);
ok(!$id,$msg);
ok(link_count($filename) == 1, "scrips ok");
+($id,$msg) = $ticket->AddLink(Type => 'RefersTo', Target => $ticket2->id);
+ok($id,$msg);
+is(link_count($filename), 1, "scrips ok");
my $transactions = $ticket2->Transactions;
$transactions->Limit( FIELD => 'Type', VALUE => 'AddLink' );
@@ -121,6 +217,7 @@
ok( $transactions->First->OldValue eq $ticket->URI );
$RT::LinkTransactionsRun1Scrip = 0;
+
($id,$msg) =$ticket->AddLink(Type => 'RefersTo', Target => $ticket2->id);
ok($id,$msg);
ok(link_count($filename) == 2, "scrips ok");
@@ -130,6 +227,9 @@
# restore
$RT::LinkTransactionsRun1Scrip = $link_scrips_orig;
+$RT::StrictLinkACL = $link_acl_checks_orig;
+
+exit(0);
sub link_count {
More information about the Rt-commit
mailing list