[Rt-commit] rt branch, 4.0/transaction-batch-twice, created. rt-4.0.2-142-ga793441
Kevin Falcone
falcone at bestpractical.com
Thu Oct 13 16:42:26 EDT 2011
The branch, 4.0/transaction-batch-twice has been created
at a7934415b3bc4a4b575e10d5ff4465b6c01fab2a (commit)
- Log -----------------------------------------------------------------
commit 1d6bfed3ad9e2e33a5af38e70cc067b169d01500
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Fri Sep 30 17:23:20 2011 -0400
TransactionBatch scrips are triggered twice
As a result of 2338cd19ed7a7f4c1e94f639ab2789d6586d01f3
we now clone the TicketObj and end up with two tickets being DESTROYed
and firing TransactionBatch. If your condition is clever, you may not
have noticed (this is why Approvals weren't affected)
We probably need to fix this clone to skip _TransactionBatch, although
we aren't sure why we're cloning rather than a simple second load
anyway.
We also need tests for mail notifications from TransactionBatch
It would also be nice if changes to a ticket on Modify.html that were
done by a TransactionBatch scrip were displayed in the UI without
reloading the Basics page. This behavior was changed in
b5aedb8db1cfcd46162785587002ecf8c27b1335
diff --git a/t/web/transaction_batch.t b/t/web/transaction_batch.t
new file mode 100644
index 0000000..8c032d2
--- /dev/null
+++ b/t/web/transaction_batch.t
@@ -0,0 +1,36 @@
+use strict;
+use warnings;
+use RT;
+use RT::Test tests => 7;
+
+
+my $q = RT::Test->load_or_create_queue ( Name => 'General' );
+
+my $s1 = RT::Scrip->new(RT->SystemUser);
+my ($val, $msg) =$s1->Create( Queue => $q->Id,
+ ScripCondition => 'User Defined',
+ ScripAction => 'User Defined',
+ CustomIsApplicableCode => 'return $self->TransactionObj->Field eq "TimeEstimated"',
+ CustomPrepareCode => 'return 1',
+ CustomCommitCode => '$self->TicketObj->SetPriority($self->TicketObj->Priority + 2); return 1;',
+ Template => 'Blank',
+ Stage => 'TransactionBatch',
+ );
+ok($val,$msg);
+
+my $ticket = RT::Ticket->new(RT->SystemUser);
+my ($tv,$ttv,$tm) = $ticket->Create(Queue => $q->Id,
+ Subject => "hair on fire",
+ );
+ok($tv, $tm);
+
+my ($baseurl, $m) = RT::Test->started_ok;
+$m->login;
+$m->get_ok("$baseurl/Ticket/Modify.html?id=".$ticket->Id);
+ $m->submit_form( form_name => 'TicketModify',
+ fields => { TimeEstimated => 5 }
+ );
+
+
+$ticket->Load($ticket->Id);
+is ($ticket->Priority , '2', "Ticket priority is set right");
commit dee4d21c7bbd8aead13f52000c675907fb261071
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Tue Oct 11 15:42:01 2011 -0400
Add a test that confirms that the CurrentUser isn't changed
This avoids the bug which 2338cd19ed7a7f4c1e94f639ab2789d6586d01f3
was fixing when it introduced the new "run twice" bug.
diff --git a/t/web/transaction_batch.t b/t/web/transaction_batch.t
index 8c032d2..3d1db73 100644
--- a/t/web/transaction_batch.t
+++ b/t/web/transaction_batch.t
@@ -1,7 +1,7 @@
use strict;
use warnings;
use RT;
-use RT::Test tests => 7;
+use RT::Test tests => 11;
my $q = RT::Test->load_or_create_queue ( Name => 'General' );
@@ -24,8 +24,23 @@ my ($tv,$ttv,$tm) = $ticket->Create(Queue => $q->Id,
);
ok($tv, $tm);
+my $testuser = RT::Test->load_or_create_user( Name => 'bob', EmailAddress => 'bob at example.com', Password => 'password' );
+ok($testuser->Id, "Created test user bob");
+
+ok( RT::Test->add_rights({ Principal => 'Privileged', Right => [qw(ShowTicket ModifyTicket SeeQueue)]}), 'Granted ticket management rights');
+
+my $test_current_user = RT::CurrentUser->new();
+$test_current_user->LoadByName($testuser->Name);
+my $api_test = RT::Ticket->new($test_current_user);
+$api_test->Load($ticket->Id);
+$api_test->SetTimeEstimated(12);
+$api_test->ApplyTransactionBatch;
+is($api_test->CurrentUser->UserObj->Name, $testuser->Name,"User didn't change running Transaction Batch scrips");
+$api_test->Load($api_test->Id);
+is($api_test->Priority,2,"Ticket priority updated");
+
my ($baseurl, $m) = RT::Test->started_ok;
-$m->login;
+$m->login('bob','password');
$m->get_ok("$baseurl/Ticket/Modify.html?id=".$ticket->Id);
$m->submit_form( form_name => 'TicketModify',
fields => { TimeEstimated => 5 }
@@ -33,4 +48,4 @@ $m->get_ok("$baseurl/Ticket/Modify.html?id=".$ticket->Id);
$ticket->Load($ticket->Id);
-is ($ticket->Priority , '2', "Ticket priority is set right");
+is ($ticket->Priority , 4, "Ticket priority is set right");
commit 771a9575a29d27d9f24be751b6dd2b0fab47a760
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Tue Oct 11 17:10:07 2011 -0400
Stop TransactionBatch scrips from running twice.
Also prevent the privilege leak originally fixed in
2338cd19ed7a7f4c1e94f639ab2789d6586d01f3.
Unfortunately, cloning the ticket passed in to ApplyTransactionBatch
into $scrips->{TicketObj} means that we run TransactionBatch Scrips once
for the ticket that called ApplyTransactionBatch and once for
$self->{TicketObj}.
Trying to solve this with a Load that makes $self->{TicketObj}
completely separate from $args{TicketObj} means that $self->{TicketObj}
doesn't get the _TransactionBatch cache and when we get into
RT::Scrip->IsApplicable we won't have the Transactions handy to check
our Condition against.
Copying the _TransactionBatch cache into $self->{TicketObj} and deleting
it from $args{TicketObj} results in TransactionBatch scrips running
twice again because $self->{TicketObj} will be DESTROYed, triggering
ApplyTransactionBatch.
The only non-bandaid and smallish solution to this is having a flag for
'run transaction batch' which is separate from the cache of Transactions
which are batched.
The correct long term solution is most likely a separate table to track
which transactions are batched together.
diff --git a/lib/RT/Scrips.pm b/lib/RT/Scrips.pm
index 077dbfe..6e2979c 100644
--- a/lib/RT/Scrips.pm
+++ b/lib/RT/Scrips.pm
@@ -164,9 +164,20 @@ sub Apply {
Type => undef,
@_ );
+ # This is a giant bandaid.
+ # 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.
+ my $old_current_user;
+ $old_current_user = $args{TicketObj}->CurrentUser if $args{TicketObj};
+
$self->Prepare(%args);
$self->Commit();
+ # Apply the bandaid.
+ $args{TicketObj}->CurrentUser($old_current_user) if $args{TicketObj};
+
}
=head2 Commit
@@ -280,9 +291,13 @@ sub _SetupSourceObjects {
@_ );
- if ( $args{'TicketObj'} ) {
- # clone the ticket here as we need to change CurrentUser
- $self->{'TicketObj'} = bless { %{$args{'TicketObj'} } }, 'RT::Ticket';
+ if ( $self->{'TicketObj'} = $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 );
}
else {
commit afc0decab3107c68b26f1ab654c0da38b9f938f0
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Thu Oct 13 15:14:44 2011 -0400
Push this logic down into Prepare and Commit
Right now, Scrips->Prepare/Commit or only ever called
from Apply and from Transaction->Create.
If anyone ever changed Transaction->Create from passing
Ticket to passing TicketObj into Prepare, we would suddenly
start returning tickets from Ticket->Create with a current user of
RT_System.
This would be really bad, so we'll complicate this fix for
TransactionBatch scrips leaking privileges and running twice by pushing
our save/restore of CurrentUser bandaid farther down into Scrips.
diff --git a/lib/RT/Scrips.pm b/lib/RT/Scrips.pm
index 6e2979c..46713b5 100644
--- a/lib/RT/Scrips.pm
+++ b/lib/RT/Scrips.pm
@@ -164,20 +164,9 @@ sub Apply {
Type => undef,
@_ );
- # This is a giant bandaid.
- # 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.
- my $old_current_user;
- $old_current_user = $args{TicketObj}->CurrentUser if $args{TicketObj};
-
$self->Prepare(%args);
$self->Commit();
- # Apply the bandaid.
- $args{TicketObj}->CurrentUser($old_current_user) if $args{TicketObj};
-
}
=head2 Commit
@@ -189,6 +178,15 @@ 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(
@@ -200,6 +198,9 @@ sub Commit {
$scrip->Commit( TicketObj => $self->{'TicketObj'},
TransactionObj => $self->{'TransactionObj'} );
}
+
+ # Apply the bandaid.
+ $self->_RestoreCurrentUser( TicketObj => $self->{TicketObj} ) if $self->{TicketObj};
}
@@ -220,6 +221,12 @@ 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'},
@@ -252,6 +259,10 @@ sub Prepare {
}
+ # Apply the bandaid.
+ $self->_RestoreCurrentUser( TicketObj => $args{TicketObj} ) if $args{TicketObj};
+
+
return (@{$self->Prepared});
};
@@ -268,7 +279,39 @@ 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 }
commit a7934415b3bc4a4b575e10d5ff4465b6c01fab2a
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Thu Oct 13 16:09:09 2011 -0400
Confirm that our Priority is 0
Suggested by Alex in case we ever change the default Priority
which would throw off my calculations.
diff --git a/t/web/transaction_batch.t b/t/web/transaction_batch.t
index 3d1db73..c01b5f5 100644
--- a/t/web/transaction_batch.t
+++ b/t/web/transaction_batch.t
@@ -1,7 +1,7 @@
use strict;
use warnings;
use RT;
-use RT::Test tests => 11;
+use RT::Test tests => 12;
my $q = RT::Test->load_or_create_queue ( Name => 'General' );
@@ -33,6 +33,7 @@ my $test_current_user = RT::CurrentUser->new();
$test_current_user->LoadByName($testuser->Name);
my $api_test = RT::Ticket->new($test_current_user);
$api_test->Load($ticket->Id);
+is($api_test->Priority,0,"Ticket priority starts at 0");
$api_test->SetTimeEstimated(12);
$api_test->ApplyTransactionBatch;
is($api_test->CurrentUser->UserObj->Name, $testuser->Name,"User didn't change running Transaction Batch scrips");
-----------------------------------------------------------------------
More information about the Rt-commit
mailing list