[Rt-commit] rt branch, 4.4/unpriv-lifecycle, created. rt-4.4.4-91-g80e607744
? sunnavy
sunnavy at bestpractical.com
Wed Feb 26 13:58:52 EST 2020
The branch, 4.4/unpriv-lifecycle has been created
at 80e607744beba13717ee97ece4bc1d9b8f5f8543 (commit)
- Log -----------------------------------------------------------------
commit 9e8e20189402c8ad65fe100f897fe2883efcffdd
Author: sunnavy <sunnavy at bestpractical.com>
Date: Thu Feb 27 02:10:57 2020 +0800
Revert "Add method for lifecycle rights check"
This reverts commit e4071599e6642f7ac35578bcc74652f5de047a8e.
The purpose of the context object param to Lifecycle method is to check
right on the passed ticket/asset object in case corresponding rights are
granted at role level. Unfortunately this is incomplete, as it can't fix
similar right issue on ticket/asset create.
As lifecycle is not a sensitive info generally, we will bypass right
check when necessary, which will be implemented in another commit.
diff --git a/lib/RT/Catalog.pm b/lib/RT/Catalog.pm
index 6c14c5c8b..6713b3d0e 100644
--- a/lib/RT/Catalog.pm
+++ b/lib/RT/Catalog.pm
@@ -471,28 +471,6 @@ sub _Set {
return ($txn_id, scalar $txn->BriefDescription);
}
-=head2 Lifecycle [CONTEXT_OBJ]
-
-Returns the current value of Lifecycle.
-
-Provide an optional asset object as context to check role-level rights
-in addition to catalog-level rights for ShowCatalog and AdminCatalog.
-
-(In the database, Lifecycle is stored as varchar(32).)
-=cut
-
-sub Lifecycle {
- my $self = shift;
- my $context_obj = shift;
-
- if ( $context_obj && $context_obj->CatalogObj->Id eq $self->Id &&
- ( $context_obj->CurrentUserHasRight('ShowCatalog') or $context_obj->CurrentUserHasRight('AdminCatalog') ) ) {
- return ( $self->__Value('Lifecycle') );
- }
-
- return ( $self->_Value('Lifecycle') );
-}
-
=head2 _Value
Checks L</CurrentUserCanSee> before calling C<SUPER::_Value>.
diff --git a/lib/RT/Queue.pm b/lib/RT/Queue.pm
index 370e3b9b2..560cc1661 100644
--- a/lib/RT/Queue.pm
+++ b/lib/RT/Queue.pm
@@ -786,16 +786,7 @@ sub _Set {
return ( $ret, $msg );
}
-sub Lifecycle {
- my $self = shift;
- my $context_obj = shift;
- if ( $context_obj && $context_obj->QueueObj->Id eq $self->Id && $context_obj->CurrentUserHasRight('SeeQueue') ) {
- return ( $self->__Value('Lifecycle') );
- }
-
- return ( $self->_Value('Lifecycle') );
-}
sub _Value {
my $self = shift;
@@ -900,13 +891,9 @@ Returns (1, 'Status message') on success and (0, 'Error Message') on failure.
=cut
-=head2 Lifecycle [CONTEXT_OBJ]
+=head2 Lifecycle
Returns the current value of Lifecycle.
-
-Provide an optional ticket object as context to check role-level rights
-in addition to queue-level rights for SeeQueue.
-
(In the database, Lifecycle is stored as varchar(32).)
diff --git a/lib/RT/Record/Role/Lifecycle.pm b/lib/RT/Record/Role/Lifecycle.pm
index 5f3abaf1d..8696b473b 100644
--- a/lib/RT/Record/Role/Lifecycle.pm
+++ b/lib/RT/Record/Role/Lifecycle.pm
@@ -83,18 +83,16 @@ requires 'LifecycleType';
=head1 PROVIDES
-=head2 LifecycleObj [CONTEXT_OBJ]
+=head2 LifecycleObj
Returns an L<RT::Lifecycle> object for this record's C<Lifecycle>. If called
as a class method, returns an L<RT::Lifecycle> object which is an aggregation
-of all lifecycles of the appropriate type. Pass an additional L<CONTEXT_OBJ>
-to check rights at the objects context.
+of all lifecycles of the appropriate type.
=cut
sub LifecycleObj {
my $self = shift;
- my $context_obj = shift || undef;
my $type = $self->LifecycleType;
my $fallback = $self->_Accessible( Lifecycle => "default" );
@@ -102,11 +100,7 @@ sub LifecycleObj {
return RT::Lifecycle->Load( Type => $type );
}
- my $name = $self->Lifecycle($context_obj);
- if ( !$name ) {
- RT::Logger->debug('Failing back to default lifecycle value');
- $name = $fallback;
- }
+ my $name = $self->Lifecycle || $fallback;
my $res = RT::Lifecycle->Load( Name => $name, Type => $type );
unless ( $res ) {
RT->Logger->error(
diff --git a/lib/RT/Record/Role/Status.pm b/lib/RT/Record/Role/Status.pm
index d76b2c32d..fab4be8aa 100644
--- a/lib/RT/Record/Role/Status.pm
+++ b/lib/RT/Record/Role/Status.pm
@@ -116,7 +116,7 @@ of all lifecycles of the appropriate type.
sub LifecycleObj {
my $self = shift;
my $obj = $self->LifecycleColumn . "Obj";
- return $self->$obj->LifecycleObj($self);
+ return $self->$obj->LifecycleObj;
}
=head2 Lifecycle
commit 6583a2ab00c1b6d179305fcbea7f61b7543ed888
Author: sunnavy <sunnavy at bestpractical.com>
Date: Thu Feb 27 02:23:25 2020 +0800
Revert "Test lifecycle rights by passing context object"
This reverts commit 53d766b5b0601c2138a65d6da090a8386edf51c0.
See also 9e8e201894
diff --git a/t/lifecycles/basics.t b/t/lifecycles/basics.t
index 00ba375db..2927af67f 100644
--- a/t/lifecycles/basics.t
+++ b/t/lifecycles/basics.t
@@ -266,7 +266,7 @@ diag "Role rights are checked for lifecycles at ticket level";
my ($ret, $msg) = $ticket->Load($id);
ok $ticket->id, 'Loaded ticket in user context';
- is $ticket->QueueObj->Lifecycle($ticket), 'default', "Rights check for role at ticket level passes";
+ is $ticket->QueueObj->Lifecycle, 'default', "Rights check at ticket level passes";
}
diag "Role rights are checked for lifecycles at asset level";
@@ -293,18 +293,14 @@ diag "Role rights are checked for lifecycles at asset level";
ok $asset->id, 'Loaded asset in user_a context';
is $asset->CatalogObj->Lifecycle, undef, "user_a can\'t see lifecycle without ShowCatalog and AdminCatalog";
- is $asset->CatalogObj->Lifecycle($asset), undef, "user_a can\'t see lifecycle without ShowCatalog and AdminCatalog";
($ret, $msg) = $asset->AddRoleMember(Type => 'Owner', User => $user_a);
ok $ret, $msg;
- is $asset->CatalogObj->Lifecycle($asset), 'assets', 'Successfully loaded lifecycle with rights check at role level';
+ is $asset->CatalogObj->Lifecycle, 'assets', 'Successfully loaded lifecycle with rights check at role level';
my $lifecycle = $asset->CatalogObj->LifecycleObj;
is $lifecycle->Name, 'assets', 'Test LifecycleObj method';
-
- $lifecycle = $asset->CatalogObj->LifecycleObj($asset);
- is $lifecycle->Name, 'assets', 'Test LifecycleObj method';
}
done_testing;
commit 7f247c1394427393607c22c2d739c15b5dae4631
Author: sunnavy <sunnavy at bestpractical.com>
Date: Thu Feb 27 02:24:14 2020 +0800
Revert "Test lifecycle rights check at context object level"
This reverts commit eee86e6cc17b1c998797dce7914f75a2523a1ea4.
See also 9e8e201894
diff --git a/t/lifecycles/basics.t b/t/lifecycles/basics.t
index 2927af67f..f53f077cc 100644
--- a/t/lifecycles/basics.t
+++ b/t/lifecycles/basics.t
@@ -242,65 +242,4 @@ diag "'!inactive -> inactive' actions are shown even if ticket has unresolved de
);
}
-diag "Role rights are checked for lifecycles at ticket level";
-{
-
- my $user_a = RT::Test->load_or_create_user(
- Name => 'user_a', Password => 'password',
- );
- ok $user_a && $user_a->id, 'loaded or created user';
-
- RT::Test->set_rights(
- { Principal => 'AdminCc', Right => [qw(SeeQueue)] },
- { Principal => 'Everyone', Right => [qw(WatchAsAdminCc)] },
- );
-
- my $ticket = RT::Test->create_ticket(Queue => 'General');
- ok $ticket->id, 'Created new ticket';
- my $id = $ticket->id;
-
- is $ticket->QueueObj->Lifecycle, 'default', 'Successfully loaded lifecycle';
- $ticket->AddWatcher(Type => 'AdminCc', PrincipalId => $user_a->PrincipalId);
-
- $ticket = RT::Ticket->new($user_a);
- my ($ret, $msg) = $ticket->Load($id);
- ok $ticket->id, 'Loaded ticket in user context';
-
- is $ticket->QueueObj->Lifecycle, 'default', "Rights check at ticket level passes";
-}
-
-diag "Role rights are checked for lifecycles at asset level";
-{
- my $user_a = RT::Test->load_or_create_user(
- Name => 'user_a', Password => 'password',
- );
- ok $user_a && $user_a->id, 'loaded or created user';
-
- RT::Test->set_rights(
- { Principal => 'Owner', Right => [qw(ShowCatalog AdminCatalog)] },
- { Principal => 'Everyone', Right => [qw(ShowAsset ModifyAsset)] },
- );
-
- my $asset = RT::Asset->new(RT->SystemUser);
- my ($ret, $msg) = $asset->Create(Catalog => 'General assets');
- ok $asset->id, 'Created new asset';
- my $id = $asset->id;
-
- is $asset->CatalogObj->Lifecycle, 'assets', "System user can load asset without context object";
-
- $asset = RT::Asset->new($user_a);
- $asset->Load($id);
- ok $asset->id, 'Loaded asset in user_a context';
-
- is $asset->CatalogObj->Lifecycle, undef, "user_a can\'t see lifecycle without ShowCatalog and AdminCatalog";
-
- ($ret, $msg) = $asset->AddRoleMember(Type => 'Owner', User => $user_a);
- ok $ret, $msg;
-
- is $asset->CatalogObj->Lifecycle, 'assets', 'Successfully loaded lifecycle with rights check at role level';
-
- my $lifecycle = $asset->CatalogObj->LifecycleObj;
- is $lifecycle->Name, 'assets', 'Test LifecycleObj method';
-}
-
done_testing;
commit 72ce575e3705bd1693865c8f3d43d4b8045cf9c4
Author: Shawn M Moore <shawn at bestpractical.com>
Date: Tue May 23 20:12:32 2017 +0000
Failing tests for lifecycles without SeeQueue
See I#32799
diff --git a/t/lifecycles/types.t b/t/lifecycles/types.t
index 84cfd8627..20ab91404 100644
--- a/t/lifecycles/types.t
+++ b/t/lifecycles/types.t
@@ -3,13 +3,13 @@ use warnings;
BEGIN {require './t/lifecycles/utils.pl'};
-is_deeply( [ RT::Lifecycle->ListAll ], [qw/ approvals default delivery /],
+is_deeply( [ RT::Lifecycle->ListAll ], [qw/ approvals default delivery triage /],
"Get the list of all lifecycles (implicitly for for tickets)");
-is_deeply( [ RT::Lifecycle->ListAll('ticket') ], [qw/ approvals default delivery /],
+is_deeply( [ RT::Lifecycle->ListAll('ticket') ], [qw/ approvals default delivery triage /],
"Get the list of all lifecycles for tickets");
-is_deeply( [ RT::Lifecycle->List], [qw/ default delivery /],
+is_deeply( [ RT::Lifecycle->List], [qw/ default delivery triage /],
"Get the list of lifecycles without approvals (implicitly for for tickets)");
-is_deeply( [ RT::Lifecycle->List('ticket') ], [qw/ default delivery /],
+is_deeply( [ RT::Lifecycle->List('ticket') ], [qw/ default delivery triage /],
"Get the list of lifecycles without approvals for tickets");
is_deeply( [ RT::Lifecycle->List('racecar') ], [qw/ racing /],
"Get the list of lifecycles for other types");
@@ -18,7 +18,7 @@ my $tickets = RT::Lifecycle->Load( Name => '', Type => 'ticket' );
ok($tickets, "Got a generalized lifecycle for tickets");
isa_ok( $tickets, "RT::Lifecycle::Ticket", "Is the right subclass" );
is_deeply( [ sort $tickets->Valid ],
- [ sort qw(new open stalled resolved rejected deleted ordered),
+ [ sort qw(new open stalled resolved rejected deleted ordered untriaged ordinary escalated),
'on way', 'delayed', 'delivered' ],
"Only gets ticket statuses" );
diff --git a/t/lifecycles/unprivileged.t b/t/lifecycles/unprivileged.t
new file mode 100644
index 000000000..cc17e827c
--- /dev/null
+++ b/t/lifecycles/unprivileged.t
@@ -0,0 +1,147 @@
+use strict;
+use warnings;
+
+BEGIN { require './t/lifecycles/utils.pl' }
+
+my $triage = RT::Test->load_or_create_queue(
+ Name => 'triage',
+ Lifecycle => 'triage',
+);
+ok $triage && $triage->id, 'loaded or created a queue';
+
+my $user = RT::User->new( RT->SystemUser );
+$user->Create( Name => "SelfService", Password => "password", Privileged => 0 );
+
+ok( RT::Test->add_rights(
+ { Principal => 'Everyone', Object => $triage, Right => [qw(CreateTicket ShowTicket ModifyTicket)] }
+ )
+ );
+
+# disable autoopen scrip to make tests more straightforward
+# otherwise, RT System will automatically set tickets from "untriaged"
+# to "ordinary". there's little other recourse because unprivileged can
+# only use the reply page to update tickets
+my $scrip = RT::Scrip->new( RT->SystemUser );
+$scrip->LoadByCols( Description => 'On Correspond Open Inactive Tickets' );
+my ( $ok, $msg ) = $scrip->SetDisabled(1);
+ok( $ok, $msg );
+
+my $tstatus = sub {
+ DBIx::SearchBuilder::Record::Cachable->FlushCache;
+ my $ticket = RT::Ticket->new( RT->SystemUser );
+ $ticket->Load( $_[0] );
+ return $ticket->Status;
+};
+
+my $txn_creator = sub {
+ DBIx::SearchBuilder::Record::Cachable->FlushCache;
+ my $ticket = RT::Ticket->new( RT->SystemUser );
+ $ticket->Load( $_[0] );
+ my $txns = $ticket->Transactions;
+ $txns->Limit( FIELD => 'Type', VALUE => 'Status' );
+ die "Got " . $txns->Count . " transactions; expected 1" if $txns->Count != 1;
+ return $txns->First->Creator;
+};
+
+my ($baseurl) = RT::Test->started_ok;
+my $m = RT::Test::Web->new;
+$m->get_ok("$baseurl/index.html?user=SelfService&pass=password");
+
+my $ticket_id;
+
+diag "create a ticket";
+{
+ $m->get_ok( "$baseurl/SelfService/Create.html?Queue=" . $triage->Id );
+ $m->text_contains( "Create a ticket in #" . $triage->Id );
+ $m->submit_form_ok( { with_fields => { Subject => "can't see queue" }, } );
+ $m->text_like(qr/Ticket \d+ created in queue 'triage'/);
+ ($ticket_id) = ( $m->content =~ /Ticket (\d+) created in queue/ );
+ is( $tstatus->($ticket_id), 'untriaged', 'used default status' );
+}
+
+diag "update a ticket without any special permissions required";
+{
+ $m->follow_link_ok( { text => 'Reply' }, "reply to the ticket" );
+ $m->text_contains( "Update ticket #" . $ticket_id );
+ ok my $form = $m->form_name('TicketUpdate'), 'found form';
+ ok my $input = $form->find_input('Status'), 'found status selector';
+ my @form_values = $input->possible_values;
+ is_deeply( \@form_values, [ '', 'untriaged', 'ordinary', ], "possible statuses" );
+
+ $m->submit_form_ok(
+ { with_fields => {
+ Status => "ordinary",
+ UpdateContent => "hello world",
+ },
+ button => 'SubmitTicket',
+ }
+ );
+ $m->text_contains("Correspondence added");
+ $m->text_contains("Status changed from 'untriaged' to 'ordinary'");
+ $m->text_lacks("Permission Denied");
+ is( $tstatus->($ticket_id), "ordinary", "updated ticket" );
+ is( $txn_creator->($ticket_id), $user->Id, "txn creator" );
+}
+
+my $ticket2_id;
+
+diag "create a ticket";
+{
+ $m->get_ok( "$baseurl/SelfService/Create.html?Queue=" . $triage->Id );
+ $m->text_contains( "Create a ticket in #" . $triage->Id );
+ $m->submit_form_ok( { with_fields => { Subject => "can't see queue" }, } );
+ $m->text_like(qr/Ticket \d+ created in queue 'triage'/);
+ ($ticket2_id) = ( $m->content =~ /Ticket (\d+) created in queue/ );
+ is( $tstatus->($ticket2_id), 'untriaged', 'used default status' );
+}
+
+diag "update a ticket with necessary special permissions missing";
+{
+ $m->follow_link_ok( { text => 'Reply' }, "reply to the ticket" );
+ $m->text_contains( "Update ticket #" . $ticket2_id );
+ ok my $form = $m->form_name('TicketUpdate'), 'found form';
+ ok my $input = $form->find_input('Status'), 'found status selector';
+ my @form_values = $input->possible_values;
+ is_deeply( \@form_values, [ '', 'untriaged', 'ordinary', ], "possible statuses" );
+
+ $m->submit_form_ok(
+ { with_fields => {
+ Status => "escalated",
+ UpdateContent => "hello world",
+ },
+ button => 'SubmitTicket',
+ }
+ );
+ $m->text_contains("Correspondence added");
+ $m->text_contains("Permission Denied");
+ $m->text_lacks("Status changed from 'untriaged' to 'escalated'");
+ is( $tstatus->($ticket2_id), "untriaged", "no update" );
+}
+
+ok( RT::Test->add_rights( { Principal => 'Everyone', Object => $triage, Right => [qw(EscalateTicket)] } ) );
+
+diag "update a ticket with necessary special permissions granted";
+{
+ $m->follow_link_ok( { text => 'Reply' }, "reply to the ticket" );
+ $m->text_contains( "Update ticket #" . $ticket2_id );
+ ok my $form = $m->form_name('TicketUpdate'), 'found form';
+ ok my $input = $form->find_input('Status'), 'found status selector';
+ my @form_values = $input->possible_values;
+ is_deeply( \@form_values, [ '', 'untriaged', 'ordinary', 'escalated', ], "possible statuses" );
+
+ $m->submit_form_ok(
+ { with_fields => {
+ Status => "escalated",
+ UpdateContent => "hello world",
+ },
+ button => 'SubmitTicket',
+ }
+ );
+ $m->text_contains("Correspondence added");
+ $m->text_contains("Status changed from 'untriaged' to 'escalated'");
+ $m->text_lacks("Permission Denied");
+ is( $tstatus->($ticket2_id), "escalated", "now updated" );
+ is( $txn_creator->($ticket2_id), $user->Id, "txn creator" );
+}
+
+done_testing;
diff --git a/t/lifecycles/utils.pl b/t/lifecycles/utils.pl
index cd167d668..ff32fd4c5 100644
--- a/t/lifecycles/utils.pl
+++ b/t/lifecycles/utils.pl
@@ -61,6 +61,24 @@ Set(\%Lifecycles,
'delayed -> on way' => {label => 'Put On Way', update => 'Respond'},
},
},
+ triage => {
+ initial => ['untriaged'],
+ active => ['ordinary', 'escalated'],
+ inactive => ['resolved'],
+ defaults => {
+ on_create => 'untriaged',
+ },
+ transitions => {
+ '' => ['untriaged'],
+ untriaged => ['ordinary', 'escalated'],
+ ordinary => ['resolved'],
+ escalated => ['resolved'],
+ resolved => [],
+ },
+ rights => {
+ '* -> escalated' => 'EscalateTicket',
+ },
+ },
racing => {
type => 'racecar',
active => ['on-your-mark', 'get-set', 'go'],
commit e59c1f2c0f4a412b8d96142ad54a20bc3ccd1887
Author: Shawn M Moore <shawn at bestpractical.com>
Date: Tue May 23 20:19:23 2017 +0000
Walk around ACLs when working with lifecycles
This fixes an issue where unprivileged users - essentially anyone
without SeeQueue - would be given the default lifecycle instead of the
queue's lifecycle (because $queue->Lifecycle is guarded by SeeQueue).
This is wholly inappropriate when the queue's lifecycle does not
resemble the default one. The symptom is that users see unexpected
errors like "Status 'custom' isn't a valid status for this ticket" even
when "custom" is a valid status for that ticket's lifecycle, and can be
transitioned to.
So, rather than ignoring the queue's lifecycle, open it up so that when
you're working with a ticket, permissions no longer hide the lifecycle.
This does potentially open up lifecycles that had been previously hidden
by rights, but there are a few mitigating factors. A lifecycle is
unlikely to have private or sensitive information in it. There's also
very little UI for lifecycles beyond the "select status" dropdown.
This mirrors prior art to walk around ACLs to maintain consistency, such
as 68b6a66f.
Fixes: I#32799
diff --git a/lib/RT/Record/Role/Lifecycle.pm b/lib/RT/Record/Role/Lifecycle.pm
index 8696b473b..19c0473d0 100644
--- a/lib/RT/Record/Role/Lifecycle.pm
+++ b/lib/RT/Record/Role/Lifecycle.pm
@@ -94,18 +94,18 @@ of all lifecycles of the appropriate type.
sub LifecycleObj {
my $self = shift;
my $type = $self->LifecycleType;
- my $fallback = $self->_Accessible( Lifecycle => "default" );
unless (blessed($self) and $self->id) {
return RT::Lifecycle->Load( Type => $type );
}
- my $name = $self->Lifecycle || $fallback;
+ my $name = $self->__Value('Lifecycle');
my $res = RT::Lifecycle->Load( Name => $name, Type => $type );
unless ( $res ) {
RT->Logger->error(
sprintf "Lifecycle '%s' of type %s for %s #%d doesn't exist",
$name, $type, ref($self), $self->id);
+ my $fallback = $self->_Accessible( Lifecycle => "default" );
return RT::Lifecycle->Load( Name => $fallback, Type => $type );
}
return $res;
commit 80e607744beba13717ee97ece4bc1d9b8f5f8543
Author: sunnavy <sunnavy at bestpractical.com>
Date: Thu Feb 27 02:57:15 2020 +0800
Update tests as now user could modify status without SeeQueue
diff --git a/t/web/lifecycle_rights.t b/t/web/lifecycle_rights.t
index efa9c93fa..0ad300a41 100644
--- a/t/web/lifecycle_rights.t
+++ b/t/web/lifecycle_rights.t
@@ -3,7 +3,7 @@ use warnings;
BEGIN {require './t/lifecycles/utils.pl'};
-diag 'Test web UI for ticket status when rights granted at role level';
+diag 'Test web UI for ticket status without SeeQueue right';
{
my ( $url, $agent ) = RT::Test->started_ok;
@@ -22,7 +22,6 @@ diag 'Test web UI for ticket status when rights granted at role level';
ok $user_a && $user_a->id, 'loaded or created user';
RT::Test->set_rights(
- { Principal => 'AdminCc', Right => [qw(SeeQueue)] },
{ Principal => 'Everyone', Right => [qw(ModifyTicket ShowTicket)] },
);
@@ -36,11 +35,6 @@ diag 'Test web UI for ticket status when rights granted at role level';
name => 'Status',
);
- # Greater equal to 2 because you can change to current status and current status (Unchanged) without 'SeeQueu'
- ok $inputs->value_names eq 2, 'We are unable to transition to other statuses without role rights';
-
- ok $ticket->AddRoleMember(Type => 'AdminCc', User => $user_a);
-
$agent->get_ok($url . '/Ticket/Modify.html?id=' . $ticket->Id);
$agent->form_name('TicketModify');
-----------------------------------------------------------------------
More information about the rt-commit
mailing list