[Rt-commit] r7253 - in rt/branches/3.6-RELEASE: .

[jesse at bestpractical.com]

Author: jesse
Date: Thu Mar 15 17:24:46 2007
New Revision: 7253

Modified:
   rt/branches/3.6-RELEASE/   (props changed)
   rt/branches/3.6-RELEASE/lib/RT/Ticket_Overlay.pm

Log:
 r53457 at 124:  jesse | 2007-03-15 17:18:44 -0400
 RT-Ticket: 8186
 RT-Status: resolved
 RT-Update: correspond
 
 * Patch for a possible race condition in the "SetOwner" routine that could be triggered when two users tried to take a ticket at the same time. Thanks to Todd Chapman!
 


Modified: rt/branches/3.6-RELEASE/lib/RT/Ticket_Overlay.pm
==============================================================================
--- rt/branches/3.6-RELEASE/lib/RT/Ticket_Overlay.pm	(original)
+++ rt/branches/3.6-RELEASE/lib/RT/Ticket_Overlay.pm	Thu Mar 15 17:24:46 2007
@@ -2979,14 +2979,28 @@
     my $NewOwner = shift;
     my $Type     = shift || "Give";
 
+    $RT::Handle->BeginTransaction();
+
+    $self->_SetLastUpdated(); # lock the ticket
+    $self->Load( $self->id ); # in case $self changed while waiting for lock
+
     my $OldOwnerObj = $self->OwnerObj;
 
+    my $NewOwnerObj = RT::User->new( $self->CurrentUser );
+    $NewOwnerObj->Load( $NewOwner );
+    unless ( $NewOwnerObj->Id ) {
+        $RT::Handle->Rollback();
+        return ( 0, $self->loc("That user does not exist") );
+    }
+
+
     # must have ModifyTicket rights
     # or TakeTicket/StealTicket and $NewOwner is self
     # see if it's a take
     if ( $OldOwnerObj->Id == $RT::Nobody->Id ) {
         unless (    $self->CurrentUserHasRight('ModifyTicket')
                  || $self->CurrentUserHasRight('TakeTicket') ) {
+            $RT::Handle->Rollback();
             return ( 0, $self->loc("Permission Denied") );
         }
     }
@@ -2997,27 +3011,24 @@
 
         unless (    $self->CurrentUserHasRight('ModifyTicket')
                  || $self->CurrentUserHasRight('StealTicket') ) {
+            $RT::Handle->Rollback();
             return ( 0, $self->loc("Permission Denied") );
         }
     }
     else {
         unless ( $self->CurrentUserHasRight('ModifyTicket') ) {
+            $RT::Handle->Rollback();
             return ( 0, $self->loc("Permission Denied") );
         }
     }
 
-    my $NewOwnerObj = RT::User->new( $self->CurrentUser );
-    $NewOwnerObj->Load( $NewOwner );
-    unless ( $NewOwnerObj->Id ) {
-        return ( 0, $self->loc("That user does not exist") );
-    }
-
-    # If we're not stealing and the ticket has an owner and it's not 
+    # If we're not stealing and the ticket has an owner and it's not
     # the current user
     if ( $Type ne 'Steal' and $Type ne 'Force'
          and $OldOwnerObj->Id != $RT::Nobody->Id
          and $OldOwnerObj->Id != $self->CurrentUser->Id )
     {
+        $RT::Handle->Rollback();
         return ( 0, $self->loc("You can only take tickets that are unowned") )
             if $NewOwnerObj->id == $self->CurrentUser->id;
         return (
@@ -3028,17 +3039,17 @@
 
     #If we've specified a new owner and that user can't modify the ticket
     elsif ( !$NewOwnerObj->HasRight( Right => 'OwnTicket', Object => $self ) ) {
+        $RT::Handle->Rollback();
         return ( 0, $self->loc("That user may not own tickets in that queue") );
     }
 
     # If the ticket has an owner and it's the new owner, we don't need
     # To do anything
     elsif ( $NewOwnerObj->Id == $OldOwnerObj->Id ) {
+        $RT::Handle->Rollback();
         return ( 0, $self->loc("That user already owns that ticket") );
     }
 
-    $RT::Handle->BeginTransaction();
-
     # Delete the owner in the owner group, then add a new one
     # TODO: is this safe? it's not how we really want the API to work
     # for most things, but it's fast.
@@ -3073,8 +3084,6 @@
         return ( 0, $self->loc("Could not change owner. ") . $msg );
     }
 
-    $RT::Handle->Commit();
-
     ($val, $msg) = $self->_NewTransaction(
         Type      => $Type,
         Field     => 'Owner',
@@ -3086,9 +3095,14 @@
     if ( $val ) {
         $msg = $self->loc( "Owner changed from [_1] to [_2]",
                            $OldOwnerObj->Name, $NewOwnerObj->Name );
-
-        # TODO: make sure the trans committed properly
     }
+    else {
+        $RT::Handle->Rollback();
+        return ( 0, $msg );
+    }
+
+    $RT::Handle->Commit();
+
     return ( $val, $msg );
 }