[Rt-commit] rt branch 5.0/rest2-transaction-pages created. rt-5.0.5-44-g419f0267c0
BPS Git Server
git at git.bestpractical.com
Tue Nov 14 21:37:46 UTC 2023
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "rt".
The branch, 5.0/rest2-transaction-pages has been created
at 419f0267c03ef1f34f835f3ee95d55bf7c9d0753 (commit)
- Log -----------------------------------------------------------------
commit 419f0267c03ef1f34f835f3ee95d55bf7c9d0753
Author: sunnavy <sunnavy at bestpractical.com>
Date: Tue Nov 14 12:53:12 2023 -0500
Tweak rights check for transactions of a single ticket
This is initially for REST2 endpoints like
"/REST/2.0/ticket/123/history". Previously only superusers can get
total/pages, this commit tweaks it so normal users can also get
total/pages if they can see the specified ticket's all returned
transactions.
diff --git a/lib/RT/SearchBuilder.pm b/lib/RT/SearchBuilder.pm
index c134f17601..a5fbe32689 100644
--- a/lib/RT/SearchBuilder.pm
+++ b/lib/RT/SearchBuilder.pm
@@ -1152,6 +1152,7 @@ sub DistinctFieldValues {
sub CurrentUserCanSeeAll {
my $self = shift;
+ return 1 if $self->{_current_user_can_see_all};
return $self->CurrentUser->HasRight( Right => 'SuperUser', Object => RT->System ) ? 1 : 0;
}
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index 63a71789d3..fb3cb266fa 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -3104,6 +3104,11 @@ sub Transactions {
ENTRYAGGREGATOR => 'AND'
);
}
+
+ if ( $self->CurrentUserHasRight('SeeCustomField') ) {
+ # We have checked all related rights, current user should be able to see all results
+ $transactions->{_current_user_can_see_all} = 1;
+ }
} else {
$transactions->Limit(
SUBCLAUSE => 'acl',
diff --git a/lib/RT/Transactions.pm b/lib/RT/Transactions.pm
index e03d2995a8..6b1dc6859e 100644
--- a/lib/RT/Transactions.pm
+++ b/lib/RT/Transactions.pm
@@ -141,6 +141,7 @@ sub LimitToTicket {
sub AddRecord {
my $self = shift;
my ($record) = @_;
+ return $self->SUPER::AddRecord($record) if $self->{_current_user_can_see_all};
if ( $self->{_is_ticket_only_search} && RT->Config->Get('UseSQLForACLChecks') ) {
# UseSQLForACLChecks implies ShowTicket only, need to check out extra rights here.
@@ -1260,6 +1261,7 @@ sub CleanSlate {
$self->SUPER::CleanSlate(@_);
}
delete $self->{_is_ticket_only_search};
+ delete $self->{_current_user_can_see_all};
}
RT::Base->_ImportOverlays();
commit a501d14d79cb1ae8a8ba1234713f835bc6aed7c1
Author: sunnavy <sunnavy at bestpractical.com>
Date: Tue Nov 14 11:45:15 2023 -0500
Exclude EmailRecord txns if current user can not see them
diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm
index 933d315f7a..63a71789d3 100644
--- a/lib/RT/Ticket.pm
+++ b/lib/RT/Ticket.pm
@@ -3084,22 +3084,25 @@ sub Transactions {
if ( $self->CurrentUserHasRight('ShowTicket') ) {
$transactions->LimitToTicket($self->id);
+ my @types;
+
# if the user may not see comments do not return them
unless ( $self->CurrentUserHasRight('ShowTicketComments') ) {
+ push @types, 'Comment', 'CommentEmailRecord';
+ }
+
+ unless ( $self->CurrentUserHasRight('ShowOutgoingEmail') ) {
+ push @types, 'EmailRecord';
+ }
+
+ if (@types) {
$transactions->Limit(
- SUBCLAUSE => 'acl',
- FIELD => 'Type',
- OPERATOR => '!=',
- VALUE => "Comment"
- );
- $transactions->Limit(
- SUBCLAUSE => 'acl',
- FIELD => 'Type',
- OPERATOR => '!=',
- VALUE => "CommentEmailRecord",
+ SUBCLAUSE => 'acl',
+ FIELD => 'Type',
+ OPERATOR => 'NOT IN',
+ VALUE => \@types,
ENTRYAGGREGATOR => 'AND'
);
-
}
} else {
$transactions->Limit(
diff --git a/t/rest2/tickets.t b/t/rest2/tickets.t
index a880ef47bc..07066bf013 100644
--- a/t/rest2/tickets.t
+++ b/t/rest2/tickets.t
@@ -422,14 +422,14 @@ my ($ticket_url, $ticket_id);
is($res->code, 200);
my $content = $mech->json_response;
- is($content->{count}, 9);
+ is($content->{count}, 10);
is($content->{page}, 1);
is($content->{per_page}, 10);
is($content->{prev_page}, undef, 'No prev_page');
is($content->{next_page}, $history_pages_url . '&page=2');
is($content->{total}, undef, 'No total');
- is(scalar @{$content->{items}}, 9);
+ is(scalar @{$content->{items}}, 10);
for my $txn (@{ $content->{items} }) {
is($txn->{type}, 'transaction');
@@ -443,14 +443,14 @@ my ($ticket_url, $ticket_id);
is($res->code, 200);
$content = $mech->json_response;
- is($content->{count}, 5);
+ is($content->{count}, 4);
is($content->{page}, 2);
is($content->{per_page}, 10);
is($content->{next_page}, undef, 'No next_page');
is($content->{prev_page}, $history_pages_url . '&page=1');
is($content->{total}, undef, 'No total');
- is(scalar @{$content->{items}}, 5);
+ is(scalar @{$content->{items}}, 4);
for my $txn (@{ $content->{items} }) {
is($txn->{type}, 'transaction');
@@ -464,14 +464,14 @@ my ($ticket_url, $ticket_id);
is($res->code, 200);
$content = $mech->json_response;
- is($content->{count}, 9);
+ is($content->{count}, 10);
is($content->{page}, 1);
is($content->{per_page}, 10);
is($content->{prev_page}, undef, 'No prev_page');
is($content->{next_page}, $history_pages_url . '&page=2');
is($content->{total}, undef, 'No total');
- is(scalar @{$content->{items}}, 9);
+ is(scalar @{$content->{items}}, 10);
for my $txn (@{ $content->{items} }) {
is($txn->{type}, 'transaction');
commit af7fe1b8eeb3eeb445432e013da77402222b7926
Author: sunnavy <sunnavy at bestpractical.com>
Date: Tue Nov 14 11:26:07 2023 -0500
Set to the last page if the given page exceeds it
This completes the validation of "page" param and is also consistent
with search results page on web UI.
diff --git a/lib/RT/REST2/Resource/Collection.pm b/lib/RT/REST2/Resource/Collection.pm
index 89eb621f32..0ee92a98cb 100644
--- a/lib/RT/REST2/Resource/Collection.pm
+++ b/lib/RT/REST2/Resource/Collection.pm
@@ -92,6 +92,11 @@ sub setup_paging {
my $page = $self->request->param('page') || 1;
if ( $page !~ /^\d+$/ ) { $page = 1 }
elsif ( $page == 0 ) { $page = 1 }
+ elsif ( $page > 1 && $self->collection->CurrentUserCanSeeAll ) {
+ if ( my $pages = ceil( $self->collection->CountAll / $per_page ) ) {
+ $page = $pages if $page > $pages;
+ }
+ }
$self->collection->GotoPage($page - 1);
}
@@ -212,14 +217,12 @@ sub serialize {
$results{pages} = defined $results{total} ? $pages : undef;
if ( $results{pages} ) {
if ($results{page} < $results{pages}) {
- my $page = $results{page} + 1;
$uri->query_form( @query_form, page => $results{page} + 1 );
$results{next_page} = $uri->as_string;
}
- if ($results{page} > 1) {
- # If we're beyond the last page, set prev_page as the last page
- # available, otherwise, the previous page.
- $uri->query_form( @query_form, page => ($results{page} > $results{pages} ? $results{pages} : $results{page} - 1) );
+
+ if ( $results{page} > 1 ) {
+ $uri->query_form( @query_form, page => $results{page} - 1 );
$results{prev_page} = $uri->as_string;
}
}
diff --git a/t/rest2/pagination.t b/t/rest2/pagination.t
index 41af5d379e..acc759b3f9 100644
--- a/t/rest2/pagination.t
+++ b/t/rest2/pagination.t
@@ -138,10 +138,10 @@ my $bravo_id = $bravo->Id;
my $content = $mech->json_response;
if ($param eq 'page') {
if ($value eq '30') {
- is($content->{count}, 0);
- is($content->{page}, 30);
- is(scalar @{$content->{items}}, 0);
- like($content->{prev_page}, qr[$url\?page=1]);
+ is($content->{count}, 3);
+ is($content->{page}, 1);
+ is(scalar @{$content->{items}}, 3);
+ is($content->{prev_page}, undef);
} else {
is($content->{count}, 3);
is($content->{page}, 1);
commit b905d03cfbff7e59e68613da614f571302cf175a
Author: Jim Brandt <jbrandt at bestpractical.com>
Date: Tue Nov 7 17:09:31 2023 -0500
Set next_page and prev_page without leaking any info
c2d5d72cd removed total and pages if a user doesn't have
sufficient rights for security, and this makes paging
difficult because the caller doesn't know if there are
more results.
Refactor next_page/prev_page to set them based on looking
ahead/behind, but still be secure and not reveal any extra
information about counts. If next_page exists,
a caller now knows they should call for more records.
Fixes: I#37712
diff --git a/lib/RT/REST2.pm b/lib/RT/REST2.pm
index ef5c953851..bd3f9e1978 100644
--- a/lib/RT/REST2.pm
+++ b/lib/RT/REST2.pm
@@ -1275,6 +1275,11 @@ or previous page, then the URL for that page is returned in the next_page
and prev_page variables respectively. It is up to you to store the required
JSON to pass with the following page request.
+If current user doesn't have enough rights to see all returned results, then
+returned C<pages> and C<total> will be C<null>, in which case you can still
+loop through results by means of C<next_page> that contains the link to the next
+page of results, until it becomes absent.
+
=head2 Disabled items
By default, only enabled objects are returned. To include disabled objects
diff --git a/lib/RT/REST2/Resource/Collection.pm b/lib/RT/REST2/Resource/Collection.pm
index 7653d5695c..89eb621f32 100644
--- a/lib/RT/REST2/Resource/Collection.pm
+++ b/lib/RT/REST2/Resource/Collection.pm
@@ -61,6 +61,7 @@ use Module::Runtime qw( require_module );
use RT::REST2::Util qw( expand_uid format_datetime error_as_json );
use POSIX qw( ceil );
use Encode;
+our $PREVIEW_LIMIT = 200;
has 'collection_class' => (
is => 'ro',
@@ -187,9 +188,12 @@ sub serialize {
push @results, $result;
}
+ my $total = $collection->CountAll;
+ my $pages = ceil( $total / $collection->RowsPerPage );
+
my %results = (
count => scalar(@results) + 0,
- total => $collection->CurrentUserCanSeeAll ? ( $collection->CountAll + 0 ) : undef,
+ total => $collection->CurrentUserCanSeeAll ? $total : undef,
per_page => $collection->RowsPerPage + 0,
page => ($collection->FirstRow / $collection->RowsPerPage) + 1,
items => \@results,
@@ -205,7 +209,7 @@ sub serialize {
}
}
- $results{pages} = defined $results{total} ? ceil($results{total} / $results{per_page}) : undef;
+ $results{pages} = defined $results{total} ? $pages : undef;
if ( $results{pages} ) {
if ($results{page} < $results{pages}) {
my $page = $results{page} + 1;
@@ -220,6 +224,49 @@ sub serialize {
}
}
+ if ( (not exists $results{next_page}) && (not defined $results{total}) ) {
+ # If total is undef, this collection checks ACLs in code so we can't
+ # use the collection count directly. Try to peek ahead to see if there
+ # are more records available so we can return next_page without giving
+ # away specific counts.
+
+ my $page = $results{page};
+
+ # If current user can't find any records after checking about $PREVIEW_LIMIT
+ # items, assuming no records left.
+ for ( 1 .. ceil( $PREVIEW_LIMIT / $results{per_page} ) ) {
+ $collection->NextPage();
+ $page++;
+ last if $page > $pages;
+
+ while ( my $item = $collection->Next ) {
+
+ # As soon as we get one record, we know it's the next page
+ $uri->query_form( @query_form, page => $page );
+ $results{next_page} = $uri->as_string;
+ last;
+ }
+ last if $results{next_page};
+ }
+
+ $page = $results{page};
+ $collection->GotoPage( $page - 1 );
+ for ( 1 .. ceil( $PREVIEW_LIMIT / $results{per_page} ) ) {
+ $collection->PrevPage();
+ $page--;
+ last if $page < 1;
+
+ while ( my $item = $collection->Next ) {
+
+ # As soon as we get one record, we know it's the prev page
+ $uri->query_form( @query_form, page => $page );
+ $results{prev_page} = $uri->as_string;
+ last;
+ }
+ last if $results{prev_page};
+ }
+ }
+
return \%results;
}
diff --git a/t/rest2/tickets.t b/t/rest2/tickets.t
index fd563c82e7..a880ef47bc 100644
--- a/t/rest2/tickets.t
+++ b/t/rest2/tickets.t
@@ -405,6 +405,80 @@ my ($ticket_url, $ticket_id);
}
}
+# Transactions with paging
+{
+ my $res = $mech->get($ticket_url,
+ 'Authorization' => $auth,
+ );
+ is($res->code, 200);
+
+ my $history_pages_url = $mech->url_for_hypermedia('history');
+
+ # Set low per_page to get at least two pages
+ $history_pages_url .= '?per_page=10';
+ $res = $mech->get($history_pages_url,
+ 'Authorization' => $auth,
+ );
+ is($res->code, 200);
+
+ my $content = $mech->json_response;
+ is($content->{count}, 9);
+ is($content->{page}, 1);
+ is($content->{per_page}, 10);
+ is($content->{prev_page}, undef, 'No prev_page');
+ is($content->{next_page}, $history_pages_url . '&page=2');
+
+ is($content->{total}, undef, 'No total');
+ is(scalar @{$content->{items}}, 9);
+
+ for my $txn (@{ $content->{items} }) {
+ is($txn->{type}, 'transaction');
+ like($txn->{_url}, qr{$rest_base_path/transaction/\d+$});
+ }
+
+ # Get next_page
+ $res = $mech->get($content->{next_page},
+ 'Authorization' => $auth,
+ );
+ is($res->code, 200);
+
+ $content = $mech->json_response;
+ is($content->{count}, 5);
+ is($content->{page}, 2);
+ is($content->{per_page}, 10);
+ is($content->{next_page}, undef, 'No next_page');
+ is($content->{prev_page}, $history_pages_url . '&page=1');
+
+ is($content->{total}, undef, 'No total');
+ is(scalar @{$content->{items}}, 5);
+
+ for my $txn (@{ $content->{items} }) {
+ is($txn->{type}, 'transaction');
+ like($txn->{_url}, qr{$rest_base_path/transaction/\d+$});
+ }
+
+ # Back to prev_page
+ $res = $mech->get($content->{prev_page},
+ 'Authorization' => $auth,
+ );
+ is($res->code, 200);
+
+ $content = $mech->json_response;
+ is($content->{count}, 9);
+ is($content->{page}, 1);
+ is($content->{per_page}, 10);
+ is($content->{prev_page}, undef, 'No prev_page');
+ is($content->{next_page}, $history_pages_url . '&page=2');
+
+ is($content->{total}, undef, 'No total');
+ is(scalar @{$content->{items}}, 9);
+
+ for my $txn (@{ $content->{items} }) {
+ is($txn->{type}, 'transaction');
+ like($txn->{_url}, qr{$rest_base_path/transaction/\d+$});
+ }
+}
+
# Ticket Reply
{
# we know from earlier tests that look at hypermedia without ReplyToTicket
-----------------------------------------------------------------------
hooks/post-receive
--
rt
More information about the rt-commit
mailing list