[Rt-commit] rt branch, 4.2/link-validity-refactoring, created. rt-4.1.6-97-g9d9c7c5

Thomas Sibley trs at bestpractical.com
Thu Jan 17 23:05:54 EST 2013


The branch, 4.2/link-validity-refactoring has been created
        at  9d9c7c5bc3dffe05c1d0501d904bcceb7552524d (commit)

- Log -----------------------------------------------------------------
commit bbc01b2a33de6a0f2b5c047fdeafa2a84edb6508
Author: Thomas Sibley <trs at bestpractical.com>
Date:   Thu Jan 17 17:09:03 2013 -0800

    Disallow linking to deleted objects of any kind that do the Status role

diff --git a/lib/RT/Record.pm b/lib/RT/Record.pm
index b0f71de..8f1eddc 100644
--- a/lib/RT/Record.pm
+++ b/lib/RT/Record.pm
@@ -1267,6 +1267,10 @@ If Silent is true then no transactions will be recorded.  You can individually
 control transactions on both base and target and with SilentBase and
 SilentTarget respectively. By default both transactions are created.
 
+If the link destination is a local object and does the
+L<RT::Role::Record::Status> role, this method ensures object Status is not
+"deleted".  Linking to deleted objects is forbidden.
+
 Returns a tuple of (link ID, message, flag if link already existed).
 
 =cut
@@ -1306,6 +1310,20 @@ sub _AddLink {
         return ( 0, $self->loc('Either base or target must be specified') );
     }
 
+    my $remote_uri = RT::URI->new( $self->CurrentUser );
+    if ($remote_uri->FromURI( $remote_link )) {
+        # Prevent linking to deleted objects
+        my $remote_obj = $remote_uri->IsLocal ? $remote_uri->Object : undef;
+        if (    $remote_obj
+            and $remote_obj->id
+            and $remote_obj->DOES("RT::Role::Record::Status")
+            and $remote_obj->Status eq "deleted") {
+            return (0, $self->loc("Linking to a deleted [_1] is not allowed", $self->loc(lc($remote_obj->RecordType))));
+        }
+    } else {
+        return (0, $self->loc("Couldn't resolve '[_1]' into a link.", $remote_link));
+    }
+
     # Check if the link already exists - we don't want duplicates
     my $old_link = RT::Link->new( $self->CurrentUser );
     $old_link->LoadByParams( Base   => $args{'Base'},
@@ -1337,12 +1355,9 @@ sub _AddLink {
     # No transactions for you!
     return ($linkid, $TransString) if $args{'Silent'};
 
-    # Some transactions?
-    my $remote_uri = RT::URI->new( $self->CurrentUser );
-    $remote_uri->FromURI( $remote_link );
-
     my $opposite_direction = $direction eq 'Target' ? 'Base': 'Target';
 
+    # Some transactions?
     unless ( $args{ 'Silent'. $direction } ) {
         my ( $Trans, $Msg, $TransObj ) = $self->_NewTransaction(
             Type      => 'AddLink',
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index 9765938..47050b9 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -515,12 +515,6 @@ sub Create {
                 }
             }
 
-            if ( $obj && $obj->Status eq 'deleted' ) {
-                push @non_fatal_errors,
-                  $self->loc("Linking. Can't link to a deleted ticket");
-                next;
-            }
-
             my ( $wval, $wmsg ) = $self->_AddLink(
                 Type                          => $RT::Link::TYPEMAP{$type}->{'Type'},
                 $RT::Link::TYPEMAP{$type}->{'Mode'} => $link,
@@ -1913,9 +1907,6 @@ sub AddLink {
         return ( 0, $self->loc("Permission Denied") );
     }
 
-    return ( 0, "Can't link to a deleted ticket" )
-      if $other_ticket && $other_ticket->Status eq 'deleted';
-
     return $self->_AddLink(%args);
 }
 
diff --git a/t/web/ticket_links.t b/t/web/ticket_links.t
index efb6151..d739a85 100644
--- a/t/web/ticket_links.t
+++ b/t/web/ticket_links.t
@@ -52,7 +52,7 @@ for my $type ( "DependsOn", "MemberOf", "RefersTo" ) {
 
             $m->submit;
             $m->content_like(qr/Ticket \d+ created/, 'created ticket');
-            $m->content_contains("Can't link to a deleted ticket");
+            $m->content_contains("Linking to a deleted ticket is not allowed");
             $id = RT::Test->last_ticket->id;
         }
 
@@ -75,7 +75,7 @@ for my $type ( "DependsOn", "MemberOf", "RefersTo" ) {
                 $m->field( "$type-$id", "$deleted_id $active_id $inactive_id" );
             }
             $m->submit;
-            $m->content_contains("Can't link to a deleted ticket");
+            $m->content_contains("Linking to a deleted ticket is not allowed");
 
             if ( $c eq 'base' ) {
                 $m->content_like(

commit 872b9595e0d78c5c9e33378c8ea16c51f1161262
Author: Thomas Sibley <trs at bestpractical.com>
Date:   Thu Jan 17 18:14:28 2013 -0800

    Refactor and generalize StrictLinkACL checking into RT::Record
    
    This lets other record classes start to use StrictLinkACL too, simply by
    defining a StrictLinkACLRight function.
    
    Slowly, slowly the RT::Ticket linking methods become less special and
    more standard.

diff --git a/lib/RT/Record.pm b/lib/RT/Record.pm
index 8f1eddc..547ccc8 100644
--- a/lib/RT/Record.pm
+++ b/lib/RT/Record.pm
@@ -1271,6 +1271,13 @@ If the link destination is a local object and does the
 L<RT::Role::Record::Status> role, this method ensures object Status is not
 "deleted".  Linking to deleted objects is forbidden.
 
+If the link destination (i.e. not C<$self>) is a local object and the
+C<$StrictLinkACL> option is enabled, this method checks the appropriate right
+on the destination object (if any, as returned by the L</StrictLinkACLRight>
+method).  B<< The subclass is expected to check the appropriate right on the
+source object (i.e.  C<$self>) before calling this method. >>  This allows a
+different right to be used on the source object during creation, for example.
+
 Returns a tuple of (link ID, message, flag if link already existed).
 
 =cut
@@ -1312,13 +1319,22 @@ sub _AddLink {
 
     my $remote_uri = RT::URI->new( $self->CurrentUser );
     if ($remote_uri->FromURI( $remote_link )) {
-        # Prevent linking to deleted objects
         my $remote_obj = $remote_uri->IsLocal ? $remote_uri->Object : undef;
-        if (    $remote_obj
-            and $remote_obj->id
-            and $remote_obj->DOES("RT::Role::Record::Status")
-            and $remote_obj->Status eq "deleted") {
-            return (0, $self->loc("Linking to a deleted [_1] is not allowed", $self->loc(lc($remote_obj->RecordType))));
+        if ($remote_obj and $remote_obj->id) {
+            # Enforce the remote end of StrictLinkACL
+            if (RT->Config->Get("StrictLinkACL")) {
+                my $right = $remote_obj->StrictLinkACLRight;
+
+                return (0, $self->loc("Permission denied"))
+                    if $right and
+                   not $self->CurrentUser->HasRight( Right => $right, Object => $remote_obj );
+            }
+
+            # Prevent linking to deleted objects
+            if ($remote_obj->DOES("RT::Role::Record::Status")
+                and $remote_obj->Status eq "deleted") {
+                return (0, $self->loc("Linking to a deleted [_1] is not allowed", $self->loc(lc($remote_obj->RecordType))));
+            }
         }
     } else {
         return (0, $self->loc("Couldn't resolve '[_1]' into a link.", $remote_link));
@@ -1390,6 +1406,12 @@ If Silent is true then no transactions will be recorded.  You can individually
 control transactions on both base and target and with SilentBase and
 SilentTarget respectively. By default both transactions are created.
 
+If the link destination (i.e. not C<$self>) is a local object and the
+C<$StrictLinkACL> option is enabled, this method checks the appropriate right
+on the destination object (if any, as returned by the L</StrictLinkACLRight>
+method).  B<< The subclass is expected to check the appropriate right on the
+source object (i.e.  C<$self>) before calling this method. >>
+
 Returns a tuple of (status flag, message).
 
 =cut 
@@ -1429,6 +1451,21 @@ sub _DeleteLink {
         return ( 0, $self->loc('Either base or target must be specified') );
     }
 
+    my $remote_uri = RT::URI->new( $self->CurrentUser );
+    if ($remote_uri->FromURI( $remote_link )) {
+        # Enforce the remote end of StrictLinkACL
+        my $remote_obj = $remote_uri->IsLocal ? $remote_uri->Object : undef;
+        if ($remote_obj and $remote_obj->id and RT->Config->Get("StrictLinkACL")) {
+            my $right = $remote_obj->StrictLinkACLRight;
+
+            return (0, $self->loc("Permission denied"))
+                if $right and
+               not $self->CurrentUser->HasRight( Right => $right, Object => $remote_obj );
+        }
+    } else {
+        return (0, $self->loc("Couldn't resolve '[_1]' into a link.", $remote_link));
+    }
+
     my $link = RT::Link->new( $self->CurrentUser );
     $RT::Logger->debug( "Trying to load link: "
             . $args{'Base'} . " "
@@ -1462,12 +1499,9 @@ sub _DeleteLink {
     # No transactions for you!
     return (1, $TransString) if $args{'Silent'};
 
-    # Some transactions?
-    my $remote_uri = RT::URI->new( $self->CurrentUser );
-    $remote_uri->FromURI( $remote_link );
-
     my $opposite_direction = $direction eq 'Target' ? 'Base': 'Target';
 
+    # Some transactions?
     unless ( $args{ 'Silent'. $direction } ) {
         my ( $Trans, $Msg, $TransObj ) = $self->_NewTransaction(
             Type      => 'DeleteLink',
@@ -2181,6 +2215,8 @@ sub LoadCustomFieldByIdentifier {
 
 sub ACLEquivalenceObjects { } 
 
+sub StrictLinkACLRight { }
+
 sub BasicColumns { }
 
 sub WikiBase {
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index 47050b9..70c6b6d 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -506,15 +506,6 @@ sub Create {
                 next;
             }
 
-            # 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' ) ) {
-                if ( $obj && !$obj->CurrentUserHasRight('ModifyTicket') ) {
-                    push @non_fatal_errors, $self->loc('Linking. Permission denied');
-                    next;
-                }
-            }
-
             my ( $wval, $wmsg ) = $self->_AddLink(
                 Type                          => $RT::Link::TYPEMAP{$type}->{'Type'},
                 $RT::Link::TYPEMAP{$type}->{'Mode'} => $link,
@@ -1841,25 +1832,8 @@ sub DeleteLink {
         return ( 0, $self->loc('Either base or target must be specified') );
     }
 
-    #check acls
-    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") );
-    }
+    return (0, $self->loc("Permission Denied"))
+        unless $self->CurrentUserHasRight('ModifyTicket');
 
     return $self->_DeleteLink(%args);
 }
@@ -1888,24 +1862,8 @@ sub AddLink {
         return ( 0, $self->loc('Either base or target must be specified') );
     }
 
-    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") );
-    }
+    return (0, $self->loc("Permission Denied"))
+        unless $self->CurrentUserHasRight('ModifyTicket');
 
     return $self->_AddLink(%args);
 }
@@ -3061,6 +3019,12 @@ sub ACLEquivalenceObjects {
 
 }
 
+=head2 StrictLinkACLRight
+
+=cut
+
+sub StrictLinkACLRight { "ModifyTicket" }
+
 1;
 
 =head1 AUTHOR
diff --git a/t/security/CVE-2011-2083-clickable-xss.t b/t/security/CVE-2011-2083-clickable-xss.t
index 008c803..753d8c7 100644
--- a/t/security/CVE-2011-2083-clickable-xss.t
+++ b/t/security/CVE-2011-2083-clickable-xss.t
@@ -25,7 +25,7 @@ for my $link ( map { ($_, ucfirst $_) } @links ) {
             Type    => 'RefersTo',
             Target  => $link,
         );
-    } [qr/Could not determine a URI scheme/, qr/Couldn't resolve/];
+    } [qr/Could not determine a URI scheme/];
     ok !$ok, $msg;
 
     ok $m->login, "logged in";
@@ -40,7 +40,6 @@ for my $link ( map { ($_, ucfirst $_) } @links ) {
     }, 'submitted links page');
     $m->content_contains("Couldn't resolve ");
     $m->next_warning_like(qr/Could not determine a URI scheme/, 'expected warning');
-    $m->next_warning_like(qr/Couldn't resolve/, 'expected warning');
 
     my $element = $m->find_link( url => $link );
     ok !$element, "no <a> link";
diff --git a/t/ticket/linking.t b/t/ticket/linking.t
index 20f63f5..9eaf939 100644
--- a/t/ticket/linking.t
+++ b/t/ticket/linking.t
@@ -220,7 +220,6 @@ warnings_like {
     ($id,$msg) = $ticket->AddLink(Type => 'RefersTo', Target => -1);
 } [
     qr/Could not determine a URI scheme for -1/,
-    qr/Couldn't resolve '-1' into a URI/,
 ];
 
 ($id,$msg) = $ticket->AddLink(Type => 'RefersTo', Target => $ticket2->id);

commit ae64a102a3972ad4f36d8a9ce86b2f8fb7dfc9f2
Author: Thomas Sibley <trs at bestpractical.com>
Date:   Thu Jan 17 18:41:56 2013 -0800

    Remove RT::Ticket->__GetTicketFromURI
    
    RT::Record->_AddLink checks validity for us now, so the only remaining
    callsite is unnecessary.  This does not get a deprecation warning and
    stay of execution until 4.4, because it is a private method.

diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index 70c6b6d..c88f7ae 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -500,21 +500,16 @@ sub Create {
         foreach my $link (
             ref( $args{$type} ) ? @{ $args{$type} } : ( $args{$type} ) )
         {
-            my ( $val, $msg, $obj ) = $self->__GetTicketFromURI( URI => $link );
-            unless ($val) {
-                push @non_fatal_errors, $msg;
-                next;
-            }
-
-            my ( $wval, $wmsg ) = $self->_AddLink(
+            my ($ok, $msg) = $self->_AddLink(
                 Type                          => $RT::Link::TYPEMAP{$type}->{'Type'},
                 $RT::Link::TYPEMAP{$type}->{'Mode'} => $link,
                 Silent                        => !$args{'_RecordTransaction'} || $self->Type eq 'reminder',
                 'Silent'. ( $RT::Link::TYPEMAP{$type}->{'Mode'} eq 'Base'? 'Target': 'Base' )
                                               => 1,
             );
-
-            push @non_fatal_errors, $wmsg unless ($wval);
+            push @non_fatal_errors,
+                 $self->loc("Unable to add [_1] link: [_2]", $self->loc($args{type}), $msg)
+                    unless $ok;
         }
     }
 
@@ -1868,27 +1863,6 @@ sub AddLink {
     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 );
-        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 MergeInto
 
 MergeInto take the id of the ticket to merge this ticket into.

commit 5e17e099d3cccedc69a1b330e8cad86ff05ac6ae
Author: Thomas Sibley <trs at bestpractical.com>
Date:   Thu Jan 17 18:49:20 2013 -0800

    Remove duplicate link validation checks from RT::Record subclasses
    
    _AddLink and _DeleteLink now handle these cases.

diff --git a/lib/RT/Article.pm b/lib/RT/Article.pm
index fba41ff..a0c73a2 100644
--- a/lib/RT/Article.pm
+++ b/lib/RT/Article.pm
@@ -397,16 +397,6 @@ sub AddLink {
         return ( 0, $self->loc("Cannot add link to plain number") );
     }
 
-    # Check that we're actually getting a valid URI
-    my $uri_obj = RT::URI->new( $self->CurrentUser );
-    $uri_obj->FromURI( $args{'Target'}||$args{'Base'} );
-    unless ( $uri_obj->Resolver && $uri_obj->Scheme ) {
-        my $msg = $self->loc( "Couldn't resolve '[_1]' into a Link.", $args{'Target'} );
-        $RT::Logger->warning( $msg );
-        return( 0, $msg );
-    }
-
-
     $self->_AddLink(%args);
 }
 
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index c88f7ae..674ca87 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -1822,11 +1822,6 @@ sub DeleteLink {
         @_
     );
 
-    unless ( $args{'Target'} || $args{'Base'} ) {
-        $RT::Logger->error("Base or Target must be specified");
-        return ( 0, $self->loc('Either base or target must be specified') );
-    }
-
     return (0, $self->loc("Permission Denied"))
         unless $self->CurrentUserHasRight('ModifyTicket');
 
@@ -1852,11 +1847,6 @@ sub AddLink {
                  SilentTarget => undef,
                  @_ );
 
-    unless ( $args{'Target'} || $args{'Base'} ) {
-        $RT::Logger->error("Base or Target must be specified");
-        return ( 0, $self->loc('Either base or target must be specified') );
-    }
-
     return (0, $self->loc("Permission Denied"))
         unless $self->CurrentUserHasRight('ModifyTicket');
 
diff --git a/t/api/link.t b/t/api/link.t
index a9e54a7..3066388 100644
--- a/t/api/link.t
+++ b/t/api/link.t
@@ -1,7 +1,7 @@
 use strict;
 use warnings;
 
-use RT::Test nodata => 1, tests => 84;
+use RT::Test nodata => 1, tests => 83;
 use RT::Test::Web;
 use Test::Warn;
 
@@ -35,9 +35,7 @@ ok $cid, 'created a ticket #'. $cid or diag "error: $msg";
     my ($status, $msg);
     clean_links();
 
-    warning_like {
-        ($status, $msg) = $parent->AddLink;
-    } qr/Base or Target must be specified/, "warned about linking a ticket to itself";
+    ($status, $msg) = $parent->AddLink;
     ok(!$status, "didn't create a link: $msg");
 
     warning_like {

commit 9d9c7c5bc3dffe05c1d0501d904bcceb7552524d
Author: Thomas Sibley <trs at bestpractical.com>
Date:   Thu Jan 17 19:38:40 2013 -0800

    Refactor link processing on create, AddLink, and DeleteLink into a role
    
    This also fixes the queue right checked in AddLink/DeleteLink to be
    AdminQueue instead of ModifyQueue.  ModifyQueue never existed, and the
    original code was incorrect when added by 5d4a3be.

diff --git a/lib/RT/Article.pm b/lib/RT/Article.pm
index a0c73a2..d6d0fbf 100644
--- a/lib/RT/Article.pm
+++ b/lib/RT/Article.pm
@@ -50,9 +50,11 @@ use strict;
 use warnings;
 
 package RT::Article;
-
 use base 'RT::Record';
 
+use Role::Basic 'with';
+with "RT::Role::Record::Links" => { -excludes => ["AddLink", "_AddLinksOnCreate"] };
+
 use RT::Articles;
 use RT::ObjectTopics;
 use RT::Classes;
@@ -352,27 +354,11 @@ sub Children {
 
 =head2 AddLink
 
-Takes a paramhash of Type and one of Base or Target. Adds that link to this tick
-et.
+Takes a paramhash of Type and one of Base or Target. Adds that link to this article.
 
-=cut
+Prevents the use of plain numbers to avoid confusing behaviour.
 
-sub DeleteLink {
-    my $self = shift;
-    my %args = (
-        Target => '',
-        Base   => '',
-        Type   => '',
-        Silent => undef,
-        @_
-    );
-
-    unless ( $self->CurrentUserHasRight('ModifyArticle') ) {
-        return ( 0, $self->loc("Permission Denied") );
-    }
-
-    $self->_DeleteLink(%args);
-}
+=cut
 
 sub AddLink {
     my $self = shift;
@@ -606,6 +592,8 @@ sub ACLEquivalenceObjects {
     return $self->ClassObj;
 }
 
+sub ModifyLinkRight { "ModifyArticle" }
+
 =head2 LoadByInclude Field Value
 
 Takes the name of a form field from "Include Article"
diff --git a/lib/RT/Queue.pm b/lib/RT/Queue.pm
index 6d68940..2844b77 100644
--- a/lib/RT/Queue.pm
+++ b/lib/RT/Queue.pm
@@ -70,12 +70,16 @@ use warnings;
 use base 'RT::Record';
 
 use Role::Basic 'with';
-with "RT::Role::Record::Lifecycle", "RT::Role::Record::Roles";
+with "RT::Role::Record::Lifecycle",
+     "RT::Role::Record::Links" => { -excludes => ["_AddLinksOnCreate"] },
+     "RT::Role::Record::Roles";
 
 sub Table {'Queues'}
 
 sub LifecycleType { "ticket" }
 
+sub ModifyLinkRight { "AdminQueue" }
+
 
 use RT::Groups;
 use RT::ACL;
@@ -188,39 +192,6 @@ sub AddRightCategories {
     $RIGHT_CATEGORIES = { %$RIGHT_CATEGORIES, %new };
 }
 
-sub AddLink {
-    my $self = shift;
-    my %args = ( Target => '',
-                 Base   => '',
-                 Type   => '',
-                 Silent => undef,
-                 @_ );
-
-    unless ( $self->CurrentUserHasRight('ModifyQueue') ) {
-        return ( 0, $self->loc("Permission Denied") );
-    }
-
-    return $self->SUPER::_AddLink(%args);
-}
-
-sub DeleteLink {
-    my $self = shift;
-    my %args = (
-        Base   => undef,
-        Target => undef,
-        Type   => undef,
-        @_
-    );
-
-    #check acls
-    unless ( $self->CurrentUserHasRight('ModifyQueue') ) {
-        $RT::Logger->debug("No permission to delete links");
-        return ( 0, $self->loc('Permission Denied'))
-    }
-
-    return $self->SUPER::_DeleteLink(%args);
-}
-
 =head2 AvailableRights
 
 Returns a hash of available rights for this object. The keys are the right names and the values are a description of what the rights do
diff --git a/lib/RT/Record.pm b/lib/RT/Record.pm
index 547ccc8..ae26d6a 100644
--- a/lib/RT/Record.pm
+++ b/lib/RT/Record.pm
@@ -1273,7 +1273,7 @@ L<RT::Role::Record::Status> role, this method ensures object Status is not
 
 If the link destination (i.e. not C<$self>) is a local object and the
 C<$StrictLinkACL> option is enabled, this method checks the appropriate right
-on the destination object (if any, as returned by the L</StrictLinkACLRight>
+on the destination object (if any, as returned by the L</ModifyLinkRight>
 method).  B<< The subclass is expected to check the appropriate right on the
 source object (i.e.  C<$self>) before calling this method. >>  This allows a
 different right to be used on the source object during creation, for example.
@@ -1323,7 +1323,7 @@ sub _AddLink {
         if ($remote_obj and $remote_obj->id) {
             # Enforce the remote end of StrictLinkACL
             if (RT->Config->Get("StrictLinkACL")) {
-                my $right = $remote_obj->StrictLinkACLRight;
+                my $right = $remote_obj->ModifyLinkRight;
 
                 return (0, $self->loc("Permission denied"))
                     if $right and
@@ -1408,7 +1408,7 @@ SilentTarget respectively. By default both transactions are created.
 
 If the link destination (i.e. not C<$self>) is a local object and the
 C<$StrictLinkACL> option is enabled, this method checks the appropriate right
-on the destination object (if any, as returned by the L</StrictLinkACLRight>
+on the destination object (if any, as returned by the L</ModifyLinkRight>
 method).  B<< The subclass is expected to check the appropriate right on the
 source object (i.e.  C<$self>) before calling this method. >>
 
@@ -1456,7 +1456,7 @@ sub _DeleteLink {
         # Enforce the remote end of StrictLinkACL
         my $remote_obj = $remote_uri->IsLocal ? $remote_uri->Object : undef;
         if ($remote_obj and $remote_obj->id and RT->Config->Get("StrictLinkACL")) {
-            my $right = $remote_obj->StrictLinkACLRight;
+            my $right = $remote_obj->ModifyLinkRight;
 
             return (0, $self->loc("Permission denied"))
                 if $right and
@@ -2215,7 +2215,7 @@ sub LoadCustomFieldByIdentifier {
 
 sub ACLEquivalenceObjects { } 
 
-sub StrictLinkACLRight { }
+sub ModifyLinkRight { }
 
 sub BasicColumns { }
 
diff --git a/lib/RT/Role/Record/Links.pm b/lib/RT/Role/Record/Links.pm
new file mode 100644
index 0000000..eb6bfd1
--- /dev/null
+++ b/lib/RT/Role/Record/Links.pm
@@ -0,0 +1,174 @@
+# BEGIN BPS TAGGED BLOCK {{{
+#
+# COPYRIGHT:
+#
+# This software is Copyright (c) 1996-2013 Best Practical Solutions, LLC
+#                                          <sales at bestpractical.com>
+#
+# (Except where explicitly superseded by other copyright notices)
+#
+#
+# LICENSE:
+#
+# This work is made available to you under the terms of Version 2 of
+# the GNU General Public License. A copy of that license should have
+# been provided with this software, but in any event can be snarfed
+# from www.gnu.org.
+#
+# This work is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+# 02110-1301 or visit their web page on the internet at
+# http://www.gnu.org/licenses/old-licenses/gpl-2.0.html.
+#
+#
+# CONTRIBUTION SUBMISSION POLICY:
+#
+# (The following paragraph is not intended to limit the rights granted
+# to you to modify and distribute this software under the terms of
+# the GNU General Public License and is only of importance to you if
+# you choose to contribute your changes and enhancements to the
+# community by submitting them to Best Practical Solutions, LLC.)
+#
+# By intentionally submitting any modifications, corrections or
+# derivatives to this work, or any other work intended for use with
+# Request Tracker, to Best Practical Solutions, LLC, you confirm that
+# you are the copyright holder for those contributions and you grant
+# Best Practical Solutions,  LLC a nonexclusive, worldwide, irrevocable,
+# royalty-free, perpetual, license to use, copy, create derivative
+# works based on those contributions, and sublicense and distribute
+# those contributions and any derivatives thereof.
+#
+# END BPS TAGGED BLOCK }}}
+
+use strict;
+use warnings;
+
+package RT::Role::Record::Links;
+use Role::Basic;
+
+=head1 NAME
+
+RT::Role::Record::Links - Common methods for records which handle links
+
+=head1 REQUIRES
+
+=head2 L<RT::Role::Record>
+
+=head2 _AddLink
+
+Usually provided by L<RT::Record/_AddLink>.
+
+=head2 _DeleteLink
+
+Usually provided by L<RT::Record/_DeleteLink>.
+
+=head2 ModifyLinkRight
+
+The right name to check in L<AddLink> and L<DeleteLink>.
+
+=head2 CurrentUserHasRight
+
+=cut
+
+with 'RT::Role::Record';
+
+requires '_AddLink';
+requires '_DeleteLink';
+
+requires 'ModifyLinkRight';
+requires 'CurrentUserHasRight';
+
+=head1 PROVIDES
+
+=head2 _AddLinksOnCreate
+
+Calls _AddLink (usually L<RT::Record/_AddLink>) for all valid link types and
+aliases found in the hash.  Refer to L<RT::Link/%TYPEMAP> for details of link
+types.  Key values may be a single URI or an arrayref of URIs.
+
+Takes two hashrefs.  The first is the argument hash provided to the consuming
+class's Create method.  The second is optional and contains extra arguments to
+pass to _AddLink.
+
+By default records a transaction on the link's destination object (if any), but
+not on the origin object.
+
+Returns an array of localized error messages, if any.
+
+=cut
+
+sub _AddLinksOnCreate {
+    my $self    = shift;
+    my %args    = %{shift || {}};
+    my %AddLink = %{shift || {}};
+    my @results;
+
+    foreach my $type ( keys %RT::Link::TYPEMAP ) {
+        next unless defined $args{$type};
+
+        my $links = $args{$type};
+           $links = [$links] unless ref $links;
+
+        for my $link (@$links) {
+            my $typemap       = $RT::Link::TYPEMAP{$type};
+            my $opposite_mode = $typemap->{Mode} eq "Base" ? "Target" : "Base";
+            my ($ok, $msg) = $self->_AddLink(
+                Type                    => $typemap->{Type},
+                $typemap->{Mode}        => $link,
+                "Silent$opposite_mode"  => 1,
+                %AddLink,
+            );
+            push @results,
+                 $self->loc("Unable to add [_1] link: [_2]", $self->loc($type), $msg)
+                     unless $ok;
+        }
+    }
+    return @results;
+}
+
+=head2 AddLink
+
+Takes a paramhash of Type and one of Base or Target. Adds that link to this
+record.
+
+Refer to L<RT::Record/_AddLink> for full documentation.  This method implements
+permissions and ticket validity checks before calling into L<RT::Record>
+(usually).
+
+=cut
+
+sub AddLink {
+    my $self = shift;
+
+    return (0, $self->loc("Permission Denied"))
+        unless $self->CurrentUserHasRight($self->ModifyLinkRight);
+
+    return $self->_AddLink(@_);
+}
+
+=head2 DeleteLink
+
+Takes a paramhash of Type and one of Base or Target. Removes that link from the
+record.
+
+Refer to L<RT::Record/_DeleteLink> for full documentation.  This method
+implements permission checks before calling into L<RT::Record> (usually).
+
+=cut
+
+sub DeleteLink {
+    my $self = shift;
+
+    return (0, $self->loc("Permission Denied"))
+        unless $self->CurrentUserHasRight($self->ModifyLinkRight);
+
+    return $self->_DeleteLink(@_);
+}
+
+1;
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index 674ca87..9979f66 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -75,6 +75,7 @@ use Role::Basic 'with';
 # role) to deal with ACLs, moving tickets between queues, and automatically
 # setting dates.
 with "RT::Role::Record::Status" => { -excludes => [qw(SetStatus _SetStatus)] },
+     "RT::Role::Record::Links",
      "RT::Role::Record::Roles";
 
 use RT::Queue;
@@ -494,24 +495,9 @@ sub Create {
     # transaction and only then fire scrips on the other ends of links.
     #
     # //RUZ
-
-    foreach my $type ( keys %RT::Link::TYPEMAP ) {
-        next unless ( defined $args{$type} );
-        foreach my $link (
-            ref( $args{$type} ) ? @{ $args{$type} } : ( $args{$type} ) )
-        {
-            my ($ok, $msg) = $self->_AddLink(
-                Type                          => $RT::Link::TYPEMAP{$type}->{'Type'},
-                $RT::Link::TYPEMAP{$type}->{'Mode'} => $link,
-                Silent                        => !$args{'_RecordTransaction'} || $self->Type eq 'reminder',
-                'Silent'. ( $RT::Link::TYPEMAP{$type}->{'Mode'} eq 'Base'? 'Target': 'Base' )
-                                              => 1,
-            );
-            push @non_fatal_errors,
-                 $self->loc("Unable to add [_1] link: [_2]", $self->loc($args{type}), $msg)
-                    unless $ok;
-        }
-    }
+    push @non_fatal_errors, $self->_AddLinksOnCreate(\%args, {
+        Silent => !$args{'_RecordTransaction'} || ($self->Type || '') eq 'reminder',
+    });
 
     # Try to add roles once more.
     push @non_fatal_errors, $self->_AddRolesOnCreate( $roles, %acls );
@@ -1803,56 +1789,6 @@ sub _Links {
     return $links;
 }
 
-
-
-=head2 DeleteLink
-
-Takes a paramhash of Type and one of Base or Target. Removes that link from this ticket.
-
-Refer to L<RT::Record/_DeleteLink> for full documentation.  This method
-implements permission checks before calling into L<RT::Record>.
-
-=cut 
-
-sub DeleteLink {
-    my $self = shift;
-    my %args = (
-        Base   => undef,
-        Target => undef,
-        @_
-    );
-
-    return (0, $self->loc("Permission Denied"))
-        unless $self->CurrentUserHasRight('ModifyTicket');
-
-    return $self->_DeleteLink(%args);
-}
-
-=head2 AddLink
-
-Takes a paramhash of Type and one of Base or Target. Adds that link to this ticket.
-
-Refer to L<RT::Record/_AddLink> for full documentation.  This method implements
-permissions and ticket validity checks before calling into L<RT::Record>.
-
-=cut
-
-sub AddLink {
-    my $self = shift;
-    my %args = ( Target       => '',
-                 Base         => '',
-                 Type         => '',
-                 Silent       => undef,
-                 SilentBase   => undef,
-                 SilentTarget => undef,
-                 @_ );
-
-    return (0, $self->loc("Permission Denied"))
-        unless $self->CurrentUserHasRight('ModifyTicket');
-
-    return $self->_AddLink(%args);
-}
-
 =head2 MergeInto
 
 MergeInto take the id of the ticket to merge this ticket into.
@@ -2983,11 +2919,11 @@ sub ACLEquivalenceObjects {
 
 }
 
-=head2 StrictLinkACLRight
+=head2 ModifyLinkRight
 
 =cut
 
-sub StrictLinkACLRight { "ModifyTicket" }
+sub ModifyLinkRight { "ModifyTicket" }
 
 1;
 

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


More information about the Rt-commit mailing list