[Rt-commit] rt branch, 4.0/transactionbatch-currentuser-leak, updated. rt-4.0.6-217-ga4e3b36

Kevin Falcone falcone at bestpractical.com
Thu Jun 28 19:03:52 EDT 2012


The branch, 4.0/transactionbatch-currentuser-leak has been updated
       via  a4e3b36382c71a63e75b8cf3f8c2f4c136229acc (commit)
      from  c5de7b01683ce25e9bc3835859637ab127f0ffb6 (commit)

Summary of changes:
 lib/RT/Scrips.pm |   68 ++++++++----------------------------------------------
 1 file changed, 10 insertions(+), 58 deletions(-)

- Log -----------------------------------------------------------------
commit a4e3b36382c71a63e75b8cf3f8c2f4c136229acc
Author: Kevin Falcone <falcone at bestpractical.com>
Date:   Thu Jun 28 18:57:46 2012 -0400

    Start forcibly loading the Ticket object we use in Scrips
    
    We've done several things with this in the past.
    Take an alias to the TicketObj (which means changing CurrentUser leaks).
    Clone the object, which means that TransactionBatch Scrips run twice.
    
    This code forcibly loads a clean TicketObj for the use of the Scrip
    system.  It then removes the _TransactionBatch cache from tickets being
    processed from ApplyTransactionBatch.
    
    It does not re-process the source objects on Commit, so likely we need a
    bit more poking to make sure no code is just invoking Commit with
    non-superuser Objects.
    
    At the end of the Commit, it removes the _TransactionBatch objects from
    the copied ticket to ensure that we don't run TransactionBatch twice.
    
    We really need a way to say "Run transaction batch, for these objects"
    and not copy around an arrayref of Transaction Objects as a boolean
    flag.

diff --git a/lib/RT/Scrips.pm b/lib/RT/Scrips.pm
index 13a4b7d..9196456 100644
--- a/lib/RT/Scrips.pm
+++ b/lib/RT/Scrips.pm
@@ -178,16 +178,6 @@ Commit all of this object's prepared scrips
 sub Commit {
     my $self = shift;
 
-    # RT::Scrips->_SetupSourceObjects will clobber
-    # the CurrentUser, but we need to keep this ticket
-    # so that the _TransactionBatch cache is maintained
-    # and doesn't run twice.  sigh.
-    $self->_StashCurrentUser( TicketObj => $self->{TicketObj} ) if $self->{TicketObj};
-
-    #We're really going to need a non-acled ticket for the scrips to work
-    $self->_SetupSourceObjects( TicketObj      => $self->{'TicketObj'},
-                                TransactionObj => $self->{'TransactionObj'} );
-    
     foreach my $scrip (@{$self->Prepared}) {
         $RT::Logger->debug(
             "Committing scrip #". $scrip->id
@@ -199,8 +189,10 @@ sub Commit {
                         TransactionObj => $self->{'TransactionObj'} );
     }
 
-    # Apply the bandaid.
-    $self->_RestoreCurrentUser( TicketObj => $self->{TicketObj} ) if $self->{TicketObj};
+    # make sure our cached TicketObj doesn't trigger a DESTROY and
+    # run all the TransactionBatch code again.
+    delete $self->{'TicketObj'}->{'_TransactionBatch'};
+
 }
 
 
@@ -221,12 +213,6 @@ sub Prepare {
                  Type           => undef,
                  @_ );
 
-    # RT::Scrips->_SetupSourceObjects will clobber
-    # the CurrentUser, but we need to keep this ticket
-    # so that the _TransactionBatch cache is maintained
-    # and doesn't run twice.  sigh.
-    $self->_StashCurrentUser( TicketObj => $args{TicketObj} ) if $args{TicketObj};
-
     #We're really going to need a non-acled ticket for the scrips to work
     $self->_SetupSourceObjects( TicketObj      => $args{'TicketObj'},
                                 Ticket         => $args{'Ticket'},
@@ -259,10 +245,6 @@ sub Prepare {
 
     }
 
-    # Apply the bandaid.
-    $self->_RestoreCurrentUser( TicketObj => $args{TicketObj} ) if $args{TicketObj};
-
-
     return (@{$self->Prepared});
 
 };
@@ -279,40 +261,6 @@ sub Prepared {
     return ($self->{'prepared_scrips'} || []);
 }
 
-=head2 _StashCurrentUser TicketObj => RT::Ticket
-
-Saves aside the current user of the original ticket that was passed to these scrips.
-This is used to make sure that we don't accidentally leak the RT_System current user
-back to the calling code.
-
-=cut
-
-sub _StashCurrentUser {
-    my $self = shift;
-    my %args = @_;
-
-    $self->{_TicketCurrentUser} = $args{TicketObj}->CurrentUser;
-}
-
-=head2 _RestoreCurrentUser TicketObj => RT::Ticket
-
-Uses the current user saved by _StashCurrentUser to reset a Ticket object
-back to the caller's current user and avoid leaking an RT_System ticket to
-calling code.
-
-=cut
-
-sub _RestoreCurrentUser {
-    my $self = shift;
-    my %args = @_;
-    unless ( $self->{_TicketCurrentUser} ) {
-        RT->Logger->debug("Called _RestoreCurrentUser without a stashed current user object");
-        return;
-    }
-    $args{TicketObj}->CurrentUser($self->{_TicketCurrentUser});
-
-}
-
 =head2  _SetupSourceObjects { TicketObj , Ticket, Transaction, TransactionObj }
 
 Setup a ticket and transaction for this Scrip collection to work with as it runs through the 
@@ -334,14 +282,18 @@ sub _SetupSourceObjects {
             @_ );
 
 
-    if ( $self->{'TicketObj'} = $args{'TicketObj'} ) {
+    if ( $args{'TicketObj'} ) {
         # This clobbers the passed in TicketObj by turning it into one
         # whose current user is RT_System.  Anywhere in the Web UI
         # currently calling into this is thus susceptable to a privilege
         # leak; the only current call site is ->Apply, which bandaids
         # over the top of this by re-asserting the CurrentUser
         # afterwards.
-        $self->{'TicketObj'}->CurrentUser( $self->CurrentUser );
+        $self->{'TicketObj'} = RT::Ticket->new( $self->CurrentUser );
+        $self->{'TicketObj'}->Load( $args{'TicketObj'}->Id );
+        if ( $args{'TicketObj'}->TransactionBatch ) {
+            $self->{'TicketObj'}->{'_TransactionBatch'} = delete $args{'TicketObj'}->{'_TransactionBatch'};
+        }
     }
     else {
         $self->{'TicketObj'} = RT::Ticket->new( $self->CurrentUser );

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


More information about the Rt-commit mailing list