[Rt-commit] rt branch, 3.8/transaction-batch-twice, created. rt-3.8.10-47-g18ef6a5
Kevin Falcone
falcone at bestpractical.com
Thu Oct 13 18:22:19 EDT 2011
The branch, 3.8/transaction-batch-twice has been created
at 18ef6a5a2d7f993d1902d65bf72ed0a04e984c2b (commit)
- Log -----------------------------------------------------------------
commit 821a1998125e527cad281907e0f719d3318261bd
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..9549929
--- /dev/null
+++ b/t/web/transaction_batch.t
@@ -0,0 +1,35 @@
+use strict;
+use warnings;
+use RT;
+use RT::Test tests => 5;
+
+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_number => 3,
+ fields => { TimeEstimated => 5 }
+ );
+
+
+$ticket->Load($ticket->Id);
+is ($ticket->Priority , '2', "Ticket priority is set right");
commit 779e7139fe27d7a22ae08dbd9419ff85d3992acc
Author: Kevin Falcone <falcone at bestpractical.com>
Date: Thu Oct 13 18:08:01 2011 -0400
We were running afoul DBIx::SearchBuilder::Record::Cachable
Flush the cache so that the Load pulls from the DB
diff --git a/t/web/transaction_batch.t b/t/web/transaction_batch.t
index 9549929..aa7b3b3 100644
--- a/t/web/transaction_batch.t
+++ b/t/web/transaction_batch.t
@@ -30,6 +30,6 @@ $m->get_ok("$baseurl/Ticket/Modify.html?id=".$ticket->Id);
fields => { TimeEstimated => 5 }
);
-
+$ticket->FlushCache;
$ticket->Load($ticket->Id);
is ($ticket->Priority , '2', "Ticket priority is set right");
commit 18b1c0c1f30101f99cd6739b52261c6c2f7ce404
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 aa7b3b3..de9754f 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 => 5;
+use RT::Test tests => 9;
my $q = RT::Test->load_or_create_queue ( Name => 'General' );
@@ -23,8 +23,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_number => 3,
fields => { TimeEstimated => 5 }
@@ -32,4 +47,4 @@ $m->get_ok("$baseurl/Ticket/Modify.html?id=".$ticket->Id);
$ticket->FlushCache;
$ticket->Load($ticket->Id);
-is ($ticket->Priority , '2', "Ticket priority is set right");
+is ($ticket->Priority , 4, "Ticket priority is set right");
commit cda75b2b959fa89a4b23ef5e4e834bb93342ffe6
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_Overlay.pm b/lib/RT/Scrips_Overlay.pm
index eecf293..367c2f7 100644
--- a/lib/RT/Scrips_Overlay.pm
+++ b/lib/RT/Scrips_Overlay.pm
@@ -171,9 +171,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
@@ -288,9 +299,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 1fb42a2cbdb2b461ebc99e7c4f04734d760320a0
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_Overlay.pm b/lib/RT/Scrips_Overlay.pm
index 367c2f7..5dd83b7 100644
--- a/lib/RT/Scrips_Overlay.pm
+++ b/lib/RT/Scrips_Overlay.pm
@@ -171,20 +171,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
@@ -196,6 +185,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(
@@ -207,6 +205,9 @@ sub Commit {
$scrip->Commit( TicketObj => $self->{'TicketObj'},
TransactionObj => $self->{'TransactionObj'} );
}
+
+ # Apply the bandaid.
+ $self->_RestoreCurrentUser( TicketObj => $self->{TicketObj} ) if $self->{TicketObj};
}
@@ -227,6 +228,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'},
@@ -259,6 +266,10 @@ sub Prepare {
}
+ # Apply the bandaid.
+ $self->_RestoreCurrentUser( TicketObj => $args{TicketObj} ) if $args{TicketObj};
+
+
return (@{$self->Prepared});
};
@@ -275,6 +286,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});
+
+}
# {{{ sup _SetupSourceObjects
commit 18ef6a5a2d7f993d1902d65bf72ed0a04e984c2b
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 de9754f..f8e6bbc 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 => 9;
+use RT::Test tests => 10;
my $q = RT::Test->load_or_create_queue ( Name => 'General' );
@@ -32,6 +32,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