From 2b9ff1a8218bdbf4931fcea8c3676370669e1b24 Mon Sep 17 00:00:00 2001 From: Dianne Skoll Date: Wed, 4 Nov 2020 08:45:44 -0500 Subject: [PATCH 01/21] Clear all RT crypt headers from incoming email before processing The old code did not delete the X-RT-SMIME-Status or X-RT-GnuPG-Status headers by accident because of missing parens. --- lib/RT/Interface/Email/Crypt.pm | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/RT/Interface/Email/Crypt.pm b/lib/RT/Interface/Email/Crypt.pm index bc3427ca497..9d4d4fe5842 100644 --- a/lib/RT/Interface/Email/Crypt.pm +++ b/lib/RT/Interface/Email/Crypt.pm @@ -73,13 +73,14 @@ sub VerifyDecrypt { ); # we clean all possible headers - my @headers = + my @headers = ( qw( X-RT-Incoming-Encryption X-RT-Incoming-Signature X-RT-Privacy X-RT-Sign X-RT-Encrypt ), - map "X-RT-$_-Status", RT::Crypt->Protocols; + map "X-RT-$_-Status", RT::Crypt->Protocols + ); foreach my $p ( $args{'Message'}->parts_DFS ) { $p->head->delete($_) for @headers; } From 4cb028365b4ef7524bd1f1192b5c0b5f1f97d66a Mon Sep 17 00:00:00 2001 From: sunnavy Date: Tue, 12 Sep 2023 16:21:26 -0400 Subject: [PATCH 02/21] Sanitize non-crypt headers used in RT internally from incoming email --- lib/RT/Interface/Email.pm | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/RT/Interface/Email.pm b/lib/RT/Interface/Email.pm index e034e13daa2..c98223749a6 100644 --- a/lib/RT/Interface/Email.pm +++ b/lib/RT/Interface/Email.pm @@ -161,6 +161,10 @@ sub Gateway { ); } + # Clean up sensitive headers. Crypt related headers are cleaned up in RT::Interface::Email::Crypt::VerifyDecrypt + my @headers = qw( RT-Attach RT-Send-Cc RT-Send-Bcc RT-Message-ID RT-DetectedAutoGenerated RT-Squelch-Replies-To ); + $Message->head->delete($_) for @headers; + #Set up a queue object my $SystemQueueObj = RT::Queue->new( RT->SystemUser ); $SystemQueueObj->Load( $args{'queue'} ); From f5bfef5be8bf1ca60faa73caea9203d1d239da25 Mon Sep 17 00:00:00 2001 From: Jim Brandt Date: Fri, 25 Aug 2023 13:57:24 -0400 Subject: [PATCH 03/21] Document restricting access to REST 1.0 mail-gateway --- docs/web_deployment.pod | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/docs/web_deployment.pod b/docs/web_deployment.pod index 22c2ef30f51..5d2f3eccb65 100644 --- a/docs/web_deployment.pod +++ b/docs/web_deployment.pod @@ -154,6 +154,31 @@ RT to access the Authorization header. More information is available in L. +=head3 Restricting the REST 1.0 mail-gateway + +RT processes email via a REST 1.0 endpoint. If you accept email on the same +server as your running RT, you can restrict this endpoint to localhost only +with a configuration like the following: + + # Accept requests only from localhost + + Require local + + +If you run C on a separate server, you can update +the above to allow additional IP addresses. + + + Require ip 127.0.0.1 ::1 192.0.2.0 # Add you actual IPs + + +See the L +for additional configuration options. + +After adding this configuration, test receiving email and confirm +your C utility and C configurations +can successfully submit email to RT. + =head2 nginx C requires that you start RT's fastcgi process externally, for From ef958dadf4387f11817b1125ecd5cbfa973e4d66 Mon Sep 17 00:00:00 2001 From: Jim Brandt Date: Fri, 25 Aug 2023 13:32:08 -0400 Subject: [PATCH 04/21] Return mail processing details only in DevelMode Don't return any system details in responses to POSTs when not in DevelMode to avoid disclosing any information. --- share/html/REST/1.0/NoAuth/mail-gateway | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/share/html/REST/1.0/NoAuth/mail-gateway b/share/html/REST/1.0/NoAuth/mail-gateway index 9adad505d42..95ffe171373 100644 --- a/share/html/REST/1.0/NoAuth/mail-gateway +++ b/share/html/REST/1.0/NoAuth/mail-gateway @@ -59,9 +59,18 @@ use RT::Interface::Email; $r->content_type('text/plain; charset=utf-8'); $m->error_format('text'); my ( $status, $error, $Ticket ) = RT::Interface::Email::Gateway( \%ARGS ); + +# Obscure the message to avoid any information disclosure unless +# in DevelMode. +my $log_error; +unless ( RT->Config->Get('DevelMode') ) { + $log_error = $error; + $error = 'operation unsuccessful'; +} + if ( $status == 1 ) { $m->out("ok\n"); - if ( $Ticket && $Ticket->Id ) { + if ( $Ticket && $Ticket->Id && RT->Config->Get('DevelMode') ) { $m->out( 'Ticket: ' . ($Ticket->Id || '') . "\n" ); $m->out( 'Queue: ' . ($Ticket->QueueObj->Name || '') . "\n" ); $m->out( 'Owner: ' . ($Ticket->OwnerObj->Name || '') . "\n" ); @@ -73,9 +82,11 @@ if ( $status == 1 ) { } else { if ( $status == -75 ) { + RT->Logger->error("mail-gateway returned status -75: $log_error") if $log_error; $m->out( "temporary failure - $error\n" ); } else { + RT->Logger->error("mail-gateway error: $log_error") if $log_error; $m->out( "not ok - $error\n" ); } } From 0118c94906565a8d4536ef024bf4885fca717011 Mon Sep 17 00:00:00 2001 From: sunnavy Date: Wed, 7 Sep 2022 21:53:34 +0800 Subject: [PATCH 05/21] Support UseSQLForACLChecks for ticket transaction searches This makes ticket transaction search count more accurate. Note that currently only "ShowTicket" is checked, extra right checks like "ShowTicketComments" for "Comment" transactions are still done after search, so the search count is still not 100% accurate. --- lib/RT/SearchBuilder/Role/Roles.pm | 4 +- lib/RT/Tickets.pm | 3 +- lib/RT/Transactions.pm | 98 +++++++++++++++++++++++++++++- 3 files changed, 101 insertions(+), 4 deletions(-) diff --git a/lib/RT/SearchBuilder/Role/Roles.pm b/lib/RT/SearchBuilder/Role/Roles.pm index ac36e8538c6..06462d3e887 100644 --- a/lib/RT/SearchBuilder/Role/Roles.pm +++ b/lib/RT/SearchBuilder/Role/Roles.pm @@ -97,7 +97,7 @@ sub _RoleGroupClass { sub _RoleGroupsJoin { my $self = shift; - my %args = (New => 0, Class => '', Name => '', @_); + my %args = (New => 0, Class => '', Name => '', Alias => 'main', @_); $args{'Class'} ||= $self->_RoleGroupClass; @@ -118,7 +118,7 @@ sub _RoleGroupsJoin { # Previously (before 4.4) this used an inner join. my $groups = $self->Join( TYPE => 'left', - ALIAS1 => 'main', + ALIAS1 => $args{Alias}, FIELD1 => $instance, TABLE2 => 'Groups', FIELD2 => 'Instance', diff --git a/lib/RT/Tickets.pm b/lib/RT/Tickets.pm index a62b7de6a1e..4ef4368bffd 100644 --- a/lib/RT/Tickets.pm +++ b/lib/RT/Tickets.pm @@ -3000,7 +3000,8 @@ sub CurrentUserCanSee { return unless @queues; $self->Limit( SUBCLAUSE => 'ACL', - ALIAS => 'main', + # RT::Transactions::CurrentUserCanSee reuses RT::Tickets::CurrentUserCanSee + ALIAS => $self->isa('RT::Transactions') ? $self->_JoinTickets : 'main', FIELD => 'Queue', OPERATOR => 'IN', VALUE => [ @queues ], diff --git a/lib/RT/Transactions.pm b/lib/RT/Transactions.pm index 21ca3cd86ea..e03d2995a89 100644 --- a/lib/RT/Transactions.pm +++ b/lib/RT/Transactions.pm @@ -142,7 +142,28 @@ sub AddRecord { my $self = shift; my ($record) = @_; - return unless $record->CurrentUserCanSee; + if ( $self->{_is_ticket_only_search} && RT->Config->Get('UseSQLForACLChecks') ) { + # UseSQLForACLChecks implies ShowTicket only, need to check out extra rights here. + my $type = $record->__Value('Type'); + if ( $type eq 'Comment' ) { + return unless $record->CurrentUserHasRight('ShowTicketComments'); + } + elsif ( $type eq 'CommentEmailRecord' ) { + return + unless $record->CurrentUserHasRight('ShowTicketComments') + && $record->CurrentUserHasRight('ShowOutgoingEmail'); + } + elsif ( $type eq 'EmailRecord' ) { + return unless $record->CurrentUserHasRight('ShowOutgoingEmail'); + } + elsif ( $type eq 'CustomField' ) { + return unless $record->CurrentUserCanSee; + } + } + else { + return unless $record->CurrentUserCanSee; + } + return $self->SUPER::AddRecord($record); } @@ -1111,6 +1132,28 @@ sub _parser { return $self->_CloseParen unless $node->isLeaf; } ); + + # Determine if it's a ticket transaction search + $tree->traverse( + sub { + my $node = shift; + return unless $node->isLeaf and $node->getNodeValue; + my ($key, $subkey, $meta, $op, $value, $bundle) + = @{$node->getNodeValue}{qw/Key Subkey Meta Op Value Bundle/}; + return unless $key eq 'ObjectType' && $value eq 'RT::Ticket' && $op eq '='; + + my $is_ticket_only_search = 1; + while ( my $parent = $node->getParent ) { + last if $parent->isRoot; + if ( lc( $parent->getNodeValue // '' ) eq 'or' ) { + $is_ticket_only_search = 0; + last; + } + $node = $parent; + } + $self->{_is_ticket_only_search} ||= $is_ticket_only_search; + } + ); } sub FromSQL { @@ -1166,6 +1209,59 @@ sub Query { return $self->{_sql_query}; } +our $AUTOLOAD; +sub AUTOLOAD { + my $self = shift; + my ($method) = ( $AUTOLOAD =~ /::(\w+)$/ ); + + no strict 'refs'; + + # Reuse RT::Tickets methods for UseSQLForACLChecks related joins/limitations. + if ( $self->{_is_ticket_only_search} && RT::Tickets->can($method) ) { + my @args = @_; + if ( $method eq '_RoleGroupsJoin' ) { + push @args, Alias => $self->_JoinTickets; + } + + if ( $method eq '_RoleGroupClass' ) { + # We want ticket's role group class here + unshift @args, 'RT::Tickets'; + } + else { + unshift @args, $self; + } + + return "RT::Tickets::$method"->(@args); + } + elsif ( $method ne 'DESTROY' ) { + require Carp; + Carp::croak "Undefined subroutine &$AUTOLOAD called"; + } +} + +sub _DoSearch { + my $self = shift; + $self->CurrentUserCanSee if $self->{_is_ticket_only_search} && RT->Config->Get('UseSQLForACLChecks'); + return $self->SUPER::_DoSearch( @_ ); +} + +sub _DoCount { + my $self = shift; + $self->CurrentUserCanSee if $self->{_is_ticket_only_search} && RT->Config->Get('UseSQLForACLChecks'); + return $self->SUPER::_DoCount( @_ ); +} + +sub CleanSlate { + my $self = shift; + if ( $self->{_is_ticket_only_search} && RT->Config->Get('UseSQLForACLChecks') ) { + RT::Tickets::CleanSlate( $self, @_ ) ; + } + else { + $self->SUPER::CleanSlate(@_); + } + delete $self->{_is_ticket_only_search}; +} + RT::Base->_ImportOverlays(); 1; From 6091759205c3b4a178cf274c73d66802efe48f03 Mon Sep 17 00:00:00 2001 From: sunnavy Date: Thu, 8 Sep 2022 15:36:33 +0800 Subject: [PATCH 06/21] Disallow non-ticket transaction searches from web UI Currently only ticket transaction searches are supported via web UI, but user can manually specify ObjectType in URL whatever they like to search various transactions, which is a bit risky, especially considering some records don't have appropriate protection setup(like Groups). Besides, by explicitly limiting searches to be ticket only, the search count is more accurate(when UseSQLForACLChecks is enabled), which can reduce the possibility of info disclosure. --- lib/RT/Interface/Web.pm | 22 ++++++++++++++++++++++ share/html/Elements/CollectionList | 6 +----- share/html/Search/Results.html | 10 ++-------- share/html/Search/Results.tsv | 10 ++-------- 4 files changed, 27 insertions(+), 21 deletions(-) diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm index 64ce8dc643d..5ad8be29282 100644 --- a/lib/RT/Interface/Web.pm +++ b/lib/RT/Interface/Web.pm @@ -5792,6 +5792,28 @@ sub ParseCalendarData { return undef; } +sub PreprocessTransactionSearchQuery { + my %args = ( + Query => undef, + ObjectType => 'RT::Ticket', + @_ + ); + + my @limits; + if ( $args{ObjectType} eq 'RT::Ticket' ) { + @limits = ( + q{TicketType = 'ticket'}, + qq{ObjectType = '$args{ObjectType}'}, + $args{Query} =~ /^\s*\(.*\)$/ ? $args{Query} : "($args{Query})" + ); + } + else { + # Other ObjectTypes are not supported for now + @limits = 'id = 0'; + } + return join ' AND ', @limits; +} + package RT::Interface::Web; RT::Base->_ImportOverlays(); diff --git a/share/html/Elements/CollectionList b/share/html/Elements/CollectionList index 2895d2cc83f..2f6aeceeb9f 100644 --- a/share/html/Elements/CollectionList +++ b/share/html/Elements/CollectionList @@ -49,11 +49,7 @@ if (!$Collection) { $Collection = $Class->new( $session{'CurrentUser'} ); if ( $Class eq 'RT::Transactions' ) { - my @limits = ( "ObjectType = '$ObjectType'", $Query ? "($Query)" : () ); - if ( $ObjectType eq 'RT::Ticket' ) { - unshift @limits, "TicketType = 'ticket'"; - } - $Query = join ' AND ', @limits; + $Query = PreprocessTransactionSearchQuery( Query => $Query, ObjectType => $ObjectType ); } $Collection->FromSQL($Query); } diff --git a/share/html/Search/Results.html b/share/html/Search/Results.html index 53d901f3add..eb37986b3fc 100644 --- a/share/html/Search/Results.html +++ b/share/html/Search/Results.html @@ -177,15 +177,9 @@ my ( $ok, $msg ); if ( $Query ) { if ( $Class eq 'RT::Transactions' ) { - my @limits = ( "ObjectType = '$ObjectType'", "($Query)" ); - if ( $ObjectType eq 'RT::Ticket' ) { - unshift @limits, "TicketType = 'ticket'"; - } - ( $ok, $msg ) = $session{$session_name}->FromSQL( join ' AND ', @limits ); - } - else { - ( $ok, $msg ) = $session{$session_name}->FromSQL($Query); + $Query = PreprocessTransactionSearchQuery( Query => $Query, ObjectType => $ObjectType ); } + ( $ok, $msg ) = $session{$session_name}->FromSQL($Query); } # Provide an empty search if parsing failed diff --git a/share/html/Search/Results.tsv b/share/html/Search/Results.tsv index 6a3bfc442bc..cddb99dfc95 100644 --- a/share/html/Search/Results.tsv +++ b/share/html/Search/Results.tsv @@ -61,17 +61,11 @@ my $collection = $Class->new( $session{'CurrentUser'} ); my @limits; if ( $Class eq 'RT::Transactions' ) { - @limits = ( "ObjectType = '$ObjectType'", "($Query)" ); - if ( $ObjectType eq 'RT::Ticket' ) { - unshift @limits, "TicketType = 'ticket'"; - } -} -else { - push @limits, $Query; + $Query = PreprocessTransactionSearchQuery( Query => $Query, ObjectType => $ObjectType ); } if ( $Query ) { - $collection->FromSQL( join ' AND ', @limits ); + $collection->FromSQL( $Query ); } elsif ( $Class eq 'RT::Assets' ) { my $catalog_obj = LoadDefaultCatalog($ARGS{'Catalog'} || ''); From 54f25d67378862f6f181f9e4fec0758cc73d2bf0 Mon Sep 17 00:00:00 2001 From: sunnavy Date: Tue, 13 Sep 2022 22:02:08 +0800 Subject: [PATCH 07/21] Hide time fields in REST2 if $HideTimeFieldsFromUnprivilegedUsers is true This is to sync with web UI. --- lib/RT/Ticket.pm | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/RT/Ticket.pm b/lib/RT/Ticket.pm index 2f615bdcb58..6cdb5f1b9e7 100644 --- a/lib/RT/Ticket.pm +++ b/lib/RT/Ticket.pm @@ -3786,6 +3786,10 @@ sub Serialize { $store{EffectiveId} = \( join '-', 'RT::Ticket', $RT::Organization, $store{EffectiveId} ); + unless ( $self->CurrentUserCanSeeTime ) { + delete $store{$_} for qw/TimeEstimated TimeLeft TimeWorked/; + } + return %store; } From 361d2849af8a87795e62bed293ca508d615792f9 Mon Sep 17 00:00:00 2001 From: sunnavy Date: Fri, 16 Sep 2022 01:21:07 +0800 Subject: [PATCH 08/21] Show Plugins info in "/rt" REST2 endpoint for super users only Normal users are not supposed to know about it. --- lib/RT/REST2/Resource/RT.pm | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/RT/REST2/Resource/RT.pm b/lib/RT/REST2/Resource/RT.pm index 6a31ba3a712..724880af72b 100644 --- a/lib/RT/REST2/Resource/RT.pm +++ b/lib/RT/REST2/Resource/RT.pm @@ -71,7 +71,9 @@ sub to_json { my $self = shift; return JSON::to_json({ Version => $RT::VERSION, - Plugins => [ RT->Config->Get('Plugins') ], + $self->current_user->HasRight( Object => RT->System, Right => 'SuperUser' ) + ? ( Plugins => [ RT->Config->Get('Plugins') ] ) + : (), }, { pretty => 1 }); } __PACKAGE__->meta->make_immutable; From 3511c116f0b2fcb917b564f833b02560b981260c Mon Sep 17 00:00:00 2001 From: sunnavy Date: Fri, 16 Sep 2022 22:54:14 +0800 Subject: [PATCH 09/21] Protect queue/class/catalog info on ticket/article/asset create via REST2 Previously we showed "Invalid ..." if the passed queue/class/catalog argument doesn't exist, via which user can infer all queue/class/catalog names. This commit changes it to "Permission Denied", to get around the possiblity of info leakage. --- lib/RT/REST2/Resource/Article.pm | 10 ++-------- lib/RT/REST2/Resource/Asset.pm | 6 ++---- lib/RT/REST2/Resource/Ticket.pm | 11 +++-------- 3 files changed, 7 insertions(+), 20 deletions(-) diff --git a/lib/RT/REST2/Resource/Article.pm b/lib/RT/REST2/Resource/Article.pm index f3a0eb32d7d..d52fff739d0 100644 --- a/lib/RT/REST2/Resource/Article.pm +++ b/lib/RT/REST2/Resource/Article.pm @@ -80,17 +80,11 @@ sub create_record { return (\400, "Invalid Class") if !$data->{Class}; - my $class = RT::Class->new(RT->SystemUser); + my $class = RT::Class->new($self->current_user); $class->Load($data->{Class}); - return (\400, "Invalid Class") if !$class->Id; - return ( \403, $self->record->loc("Permission Denied") ) - unless $self->record->CurrentUser->HasRight( - Right => 'CreateArticle', - Object => $class, - ) - and $class->Disabled != 1; + unless $class->Id and !$class->__Value('Disabled') and $class->CurrentUserHasRight('CreateArticle'); my ($ok, $txn, $msg) = $self->_create_record($data); return ($ok, $msg); diff --git a/lib/RT/REST2/Resource/Asset.pm b/lib/RT/REST2/Resource/Asset.pm index a29f637b7c7..70e66464ba7 100644 --- a/lib/RT/REST2/Resource/Asset.pm +++ b/lib/RT/REST2/Resource/Asset.pm @@ -83,10 +83,8 @@ sub create_record { my $catalog = RT::Catalog->new($self->record->CurrentUser); $catalog->Load($data->{Catalog}); - return (\400, "Invalid Catalog") if !$catalog->Id; - - return (\403, $self->record->loc("Permission Denied", $catalog->Name)) - unless $catalog->CurrentUserHasRight('CreateAsset'); + return (\403, $self->record->loc("Permission Denied")) + unless $catalog->Id and !$catalog->__Value('Disabled') and $catalog->CurrentUserHasRight('CreateAsset'); return $self->_create_record($data); } diff --git a/lib/RT/REST2/Resource/Ticket.pm b/lib/RT/REST2/Resource/Ticket.pm index 1873f0a6366..64b4d67647d 100644 --- a/lib/RT/REST2/Resource/Ticket.pm +++ b/lib/RT/REST2/Resource/Ticket.pm @@ -225,16 +225,11 @@ sub validate_input { if ( $args{'Action'} eq 'create' ) { return (0, "Could not create ticket. Queue not set", 400) if !$data->{Queue}; - my $queue = RT::Queue->new(RT->SystemUser); + my $queue = RT::Queue->new($self->current_user); $queue->Load($data->{Queue}); - return (0, "Unable to find queue", 400) if !$queue->Id; - - return (0, $self->record->loc("No permission to create tickets in the queue '[_1]'", $queue->Name), 403) - unless $self->record->CurrentUser->HasRight( - Right => 'CreateTicket', - Object => $queue, - ) and $queue->Disabled != 1; + return (0, $self->record->loc("No permission to create tickets in the queue '[_1]'", $data->{Queue}), 403) + unless $queue->Id and $queue->__Value('Disabled') != 1 and $queue->CurrentUserHasRight('CreateTicket'); } if ( $args{'Action'} eq 'update' ) { From 8032d4ab00fd9ab5d3adfc11671bc37715a7bdd0 Mon Sep 17 00:00:00 2001 From: sunnavy Date: Tue, 13 Sep 2022 22:16:15 +0800 Subject: [PATCH 10/21] Disallow unprivileged users to access REST2 "/user" endpoint The only exception is current user's own endpoint. Previously unprivileged users got "forbidden" if the target user exists and "not found" if the user does not, via which they could info all user names, which led to info leakage. --- lib/RT/REST2/Resource/User.pm | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/RT/REST2/Resource/User.pm b/lib/RT/REST2/Resource/User.pm index 510a8c77404..af4c8c0c2b5 100644 --- a/lib/RT/REST2/Resource/User.pm +++ b/lib/RT/REST2/Resource/User.pm @@ -105,9 +105,8 @@ around 'serialize' => sub { sub forbidden { my $self = shift; - return 0 if not $self->record->id; - return 0 if $self->record->id == $self->current_user->id; return 0 if $self->current_user->Privileged; + return 0 if ( $self->record->id || 0 ) == $self->current_user->id; return 1; } From 446479dd140b1af510c75733d48db1524ccdd597 Mon Sep 17 00:00:00 2001 From: sunnavy Date: Sat, 17 Sep 2022 07:50:43 +0800 Subject: [PATCH 11/21] Clean up the forbidden method of REST2 "/group/.../member*" endpoint It's fine to deny the access if the targeted group doesn't exist. The code "return 0 unless $self->group->id" is usually to allow to create the corresponding object, which isn't applicable here as the endpoint doesn't support group creation. --- lib/RT/REST2/Resource/GroupMembers.pm | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/RT/REST2/Resource/GroupMembers.pm b/lib/RT/REST2/Resource/GroupMembers.pm index 4c0bb0a49af..df555babeca 100644 --- a/lib/RT/REST2/Resource/GroupMembers.pm +++ b/lib/RT/REST2/Resource/GroupMembers.pm @@ -114,9 +114,7 @@ sub dispatch_rules { sub forbidden { my $self = shift; - return 0 unless $self->group->id; return !$self->group->CurrentUserHasRight('AdminGroupMembership'); - return 1; } sub serialize { From 96f0305b8515214a22eb2d2b1f23695b81e3c3ae Mon Sep 17 00:00:00 2001 From: sunnavy Date: Sat, 17 Sep 2022 08:04:14 +0800 Subject: [PATCH 12/21] Clean up the forbidden method of REST2 "/download/cf/*" endpoint The code "return 0 unless $self->record->id" is usually to allow to create the corresponding object, which isn't applicable here as the endpoint doesn't support custom field creation. --- lib/RT/REST2/Resource/ObjectCustomFieldValue.pm | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/RT/REST2/Resource/ObjectCustomFieldValue.pm b/lib/RT/REST2/Resource/ObjectCustomFieldValue.pm index b9d6fa1cb31..8d36cb9d306 100644 --- a/lib/RT/REST2/Resource/ObjectCustomFieldValue.pm +++ b/lib/RT/REST2/Resource/ObjectCustomFieldValue.pm @@ -73,7 +73,6 @@ sub content_types_provided { sub forbidden { my $self = shift; - return 0 unless $self->record->id; return !$self->record->CurrentUserHasRight('SeeCustomField'); } From 666f8b43a7566b8aef4caedf258254eaf0e11586 Mon Sep 17 00:00:00 2001 From: sunnavy Date: Fri, 16 Sep 2022 03:58:03 +0800 Subject: [PATCH 13/21] Improve "forbidden" for REST2 record endpoints with more right checks. Previously returned "not found" if the corresponding record doesn't exist, even when user doesn't have proper rights granted, which led to infoleakage. This commit tweaks the logic to check "CurrentUserCanCreate" if the record doesn't exist, which eliminates the leakage. Besides that, it checks "CurrentUserCanModify" instead on object update, which is more appopriate than "CurrentUserCanSee". There is a couple of right changes for "/customfield/" and "/group" endpoints: previously modifying them required "SeeCustomField" and "SeeGroup", respectively, which was not quite necessary as it's really odd to grant users "Admin..." but not "See..." rights. This commit dropped the "SeeCustomField"/"SeeGroup" check, to be consistent with other objects. --- lib/RT/Attachment.pm | 11 ++++++ lib/RT/Catalog.pm | 22 +++++++++++ lib/RT/Class.pm | 33 ++++++++++++++++ lib/RT/CustomField.pm | 22 +++++++++++ lib/RT/CustomRole.pm | 22 +++++++++++ lib/RT/Group.pm | 21 ++++++++++ lib/RT/ObjectCustomFieldValue.pm | 1 + lib/RT/Queue.pm | 23 +++++++++++ lib/RT/REST2/Resource/CustomField.pm | 15 ------- lib/RT/REST2/Resource/Group.pm | 39 ++++++------------- .../REST2/Resource/ObjectCustomFieldValue.pm | 5 --- lib/RT/REST2/Resource/Record.pm | 17 ++++++-- 12 files changed, 181 insertions(+), 50 deletions(-) diff --git a/lib/RT/Attachment.pm b/lib/RT/Attachment.pm index 3b6379dd5b4..77a5f65ce5b 100644 --- a/lib/RT/Attachment.pm +++ b/lib/RT/Attachment.pm @@ -1092,6 +1092,17 @@ sub _CacheConfig { } +=head2 CurrentUserCanSee + +Returns true if the current user can see the attachment, via corresponding +transaction's rights check. + +=cut + +sub CurrentUserCanSee { + my $self = shift; + return $self->TransactionObj->CurrentUserCanSee; +} =head2 id diff --git a/lib/RT/Catalog.pm b/lib/RT/Catalog.pm index 13542075238..31946435b1d 100644 --- a/lib/RT/Catalog.pm +++ b/lib/RT/Catalog.pm @@ -297,6 +297,28 @@ sub CurrentUserCanSee { || $self->CurrentUserHasRight('AdminCatalog'); } +=head2 CurrentUserCanCreate + +Returns true if the current user can create a new catalog, using I. + +=cut + +sub CurrentUserCanCreate { + my $self = shift; + return $self->CurrentUserHasRight('AdminCatalog'); +} + +=head2 CurrentUserCanModify + +Returns true if the current user can modify the catalog, using I. + +=cut + +sub CurrentUserCanModify { + my $self = shift; + return $self->CurrentUserHasRight('AdminCatalog'); +} + =head2 Owner Returns an L object for this catalog's I role group. On error, diff --git a/lib/RT/Class.pm b/lib/RT/Class.pm index 01079040c0d..03a7926cc2e 100644 --- a/lib/RT/Class.pm +++ b/lib/RT/Class.pm @@ -507,6 +507,39 @@ sub IncludeArticleCFValue { return $self->{'_cf_include_hash'}{"Value-".$cfobj->Id}; } +=head2 CurrentUserCanSee + +Returns true if the current user can see the class, using I. + +=cut + +sub CurrentUserCanSee { + my $self = shift; + return $self->CurrentUserHasRight('SeeClass'); +} + +=head2 CurrentUserCanCreate + +Returns true if the current user can create a new class, using I. + +=cut + +sub CurrentUserCanCreate { + my $self = shift; + return $self->CurrentUserHasRight('AdminClass'); +} + +=head2 CurrentUserCanModify + +Returns true if the current user can modify the class, using I. + +=cut + +sub CurrentUserCanModify { + my $self = shift; + return $self->CurrentUserHasRight('AdminClass'); +} + =head2 id Returns the current value of id. diff --git a/lib/RT/CustomField.pm b/lib/RT/CustomField.pm index 79c1f1f5a5d..e261eb6f8ec 100644 --- a/lib/RT/CustomField.pm +++ b/lib/RT/CustomField.pm @@ -2072,6 +2072,28 @@ sub CurrentUserCanSee { return 0; } +=head2 CurrentUserCanCreate + +If the user has I they can create a new custom field. + +=cut + +sub CurrentUserCanCreate { + my $self = shift; + return $self->CurrentUserHasRight('AdminCustomField'); +} + +=head2 CurrentUserCanModify + +If the user has I they can modify the custom field. + +=cut + +sub CurrentUserCanModify { + my $self = shift; + return $self->CurrentUserHasRight('AdminCustomField'); +} + =head2 IncludeContentForValue [VALUE] (and SetIncludeContentForValue) Gets or sets the C for this custom field. RT diff --git a/lib/RT/CustomRole.pm b/lib/RT/CustomRole.pm index ec374029254..750aaf25756 100644 --- a/lib/RT/CustomRole.pm +++ b/lib/RT/CustomRole.pm @@ -544,6 +544,28 @@ sub GroupType { return 'RT::CustomRole-' . $self->id; } +=head2 CurrentUserCanCreate + +Returns true if the current user can create a new custom role, using I. + +=cut + +sub CurrentUserCanCreate { + my $self = shift; + return $self->CurrentUserHasRight('AdminClass'); +} + +=head2 CurrentUserCanModify + +Returns true if the current user can modify the custom role, using I. + +=cut + +sub CurrentUserCanModify { + my $self = shift; + return $self->CurrentUserHasRight('AdminClass'); +} + =head2 id Returns the current value of id. diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm index fd592835bd2..afa8465f6f8 100644 --- a/lib/RT/Group.pm +++ b/lib/RT/Group.pm @@ -1322,6 +1322,27 @@ sub CurrentUserCanSee { return 1; } +=head2 CurrentUserCanCreate + +Returns true if the current user can create a new group, using I. + +=cut + +sub CurrentUserCanCreate { + my $self = shift; + return $self->CurrentUserHasRight('AdminGroup'); +} + +=head2 CurrentUserCanModify + +Returns true if the current user can modify the group, using I. + +=cut + +sub CurrentUserCanModify { + my $self = shift; + return $self->CurrentUserHasRight('AdminGroup'); +} =head2 PrincipalObj diff --git a/lib/RT/ObjectCustomFieldValue.pm b/lib/RT/ObjectCustomFieldValue.pm index ffad1c4dddf..148bd1c61e5 100644 --- a/lib/RT/ObjectCustomFieldValue.pm +++ b/lib/RT/ObjectCustomFieldValue.pm @@ -823,6 +823,7 @@ object, otherwise false. sub CurrentUserCanSee { my $self = shift; + return undef unless $self->Id; return $self->CustomFieldObj->CurrentUserHasRight('SeeCustomField'); } diff --git a/lib/RT/Queue.pm b/lib/RT/Queue.pm index 29ba7db3ad1..609a7ebe5bf 100644 --- a/lib/RT/Queue.pm +++ b/lib/RT/Queue.pm @@ -810,6 +810,29 @@ sub CurrentUserCanSee { return $self->CurrentUserHasRight('SeeQueue'); } + +=head2 CurrentUserCanCreate + +Returns true if the current user can create a new queue, using I. + +=cut + +sub CurrentUserCanCreate { + my $self = shift; + return $self->CurrentUserHasRight('AdminQueue'); +} + +=head2 CurrentUserCanModify + +Returns true if the current user can modify the queue, using I. + +=cut + +sub CurrentUserCanModify { + my $self = shift; + return $self->CurrentUserHasRight('AdminQueue'); +} + =head2 id Returns the current value of id. diff --git a/lib/RT/REST2/Resource/CustomField.pm b/lib/RT/REST2/Resource/CustomField.pm index 62e2d737b75..1924a95f2f6 100644 --- a/lib/RT/REST2/Resource/CustomField.pm +++ b/lib/RT/REST2/Resource/CustomField.pm @@ -87,21 +87,6 @@ sub serialize { return $data; } -sub forbidden { - my $self = shift; - my $method = $self->request->method; - if ($self->record->id) { - if ($method eq 'GET') { - return !$self->record->CurrentUserHasRight('SeeCustomField'); - } else { - return !($self->record->CurrentUserHasRight('SeeCustomField') && $self->record->CurrentUserHasRight('AdminCustomField')); - } - } else { - return !$self->current_user->HasRight(Right => "AdminCustomField", Object => RT->System); - } - return 0; -} - sub hypermedia_links { my $self = shift; my $links = $self->_default_hypermedia_links(@_); diff --git a/lib/RT/REST2/Resource/Group.pm b/lib/RT/REST2/Resource/Group.pm index 58f46c12558..c955d1558ff 100644 --- a/lib/RT/REST2/Resource/Group.pm +++ b/lib/RT/REST2/Resource/Group.pm @@ -58,9 +58,7 @@ extends 'RT::REST2::Resource::Record'; with 'RT::REST2::Resource::Record::Readable' => { -alias => { serialize => '_default_serialize' } }, 'RT::REST2::Resource::Record::DeletableByDisabling', - => { -alias => { delete_resource => '_delete_resource' } }, 'RT::REST2::Resource::Record::Writable', - => { -alias => { create_record => '_create_record' } }, 'RT::REST2::Resource::Record::Hypermedia' => { -alias => { hypermedia_links => '_default_hypermedia_links' } }; @@ -102,33 +100,20 @@ sub hypermedia_links { return $links; } -sub create_record { +override forbidden => sub { my $self = shift; - my $data = shift; - return (\403, $self->record->loc("Permission Denied")) - unless $self->current_user->HasRight( - Right => "AdminGroup", - Object => RT->System, - ); - - return $self->_create_record($data); -} - -sub delete_resource { - my $self = shift; - - return (\403, $self->record->loc("Permission Denied")) - unless $self->record->CurrentUserHasRight('AdminGroup'); - - return $self->_delete_resource; -} - -sub forbidden { - my $self = shift; - return 0 unless $self->record->id; - return !$self->record->CurrentUserHasRight('SeeGroup'); -} + # For historical reasons, RT::Group::CurrentUserCanSee always returns true. + # For REST2, we want to check SeeGroup. + no warnings 'redefine'; + my $original_can_see = \&RT::Group::CurrentUserCanSee; + local *RT::Group::CurrentUserCanSee = sub { + my $self = shift; + return 0 unless $original_can_see->($self, @_); + return $self->CurrentUserHasRight('SeeGroup'); + }; + return super(); +}; __PACKAGE__->meta->make_immutable; diff --git a/lib/RT/REST2/Resource/ObjectCustomFieldValue.pm b/lib/RT/REST2/Resource/ObjectCustomFieldValue.pm index 8d36cb9d306..4dbc9cb7a8b 100644 --- a/lib/RT/REST2/Resource/ObjectCustomFieldValue.pm +++ b/lib/RT/REST2/Resource/ObjectCustomFieldValue.pm @@ -71,11 +71,6 @@ sub content_types_provided { { [ {$self->record->ContentType || 'text/plain; charset=utf-8' => 'to_binary'} ] }; } -sub forbidden { - my $self = shift; - return !$self->record->CurrentUserHasRight('SeeCustomField'); -} - sub to_binary { my $self = shift; unless ($self->record->CustomFieldObj->Type =~ /^(?:Image|Binary)$/) { diff --git a/lib/RT/REST2/Resource/Record.pm b/lib/RT/REST2/Resource/Record.pm index b335dab09e3..df1223e993b 100644 --- a/lib/RT/REST2/Resource/Record.pm +++ b/lib/RT/REST2/Resource/Record.pm @@ -100,10 +100,21 @@ sub resource_exists { sub forbidden { my $self = shift; - return 0 unless $self->record->id; + my $method = $self->request->method; + + my $right_method; + if ( $self->record->id ) { + $right_method = $method =~ /^(?:GET|HEAD)$/ ? 'CurrentUserCanSee' : 'CurrentUserCanModify'; + } + else { + # Even without id, the method can be GET, e.g. to access a not-exsting record. + $right_method = $method =~ /^(?:GET|HEAD)$/ ? 'CurrentUserCanSee' : 'CurrentUserCanCreate'; + } + + if ( $self->record->can($right_method) ) { + return !$self->record->$right_method; + } - my $can_see = $self->record->can("CurrentUserCanSee"); - return 1 if $can_see and not $self->record->$can_see(); return 0; } From c2d5d72cd5e6cfaed744a3a2defe716ca96d5375 Mon Sep 17 00:00:00 2001 From: sunnavy Date: Sat, 17 Sep 2022 20:45:06 +0800 Subject: [PATCH 14/21] Hide total/pages info from REST2 collection endpoints without proper rights As most collection searches do ACL filter after SQL, CountAll returns the number of results before ACL filter, the number is inaccurate and can cause info leakage via intentional search criteria. This commit sets total/pages to null, when user doesn't have proper right granted globally, to get around this issue. --- lib/RT/Articles.pm | 5 +++++ lib/RT/Assets.pm | 5 +++++ lib/RT/Catalogs.pm | 5 +++++ lib/RT/Classes.pm | 5 +++++ lib/RT/CustomFields.pm | 5 +++++ lib/RT/CustomRoles.pm | 6 ++++++ lib/RT/Groups.pm | 5 +++++ lib/RT/Queues.pm | 5 +++++ lib/RT/REST2/Resource/Collection.pm | 28 +++++++++++++++------------- lib/RT/SearchBuilder.pm | 5 +++++ lib/RT/Tickets.pm | 6 ++++++ lib/RT/Users.pm | 5 +++++ 12 files changed, 72 insertions(+), 13 deletions(-) diff --git a/lib/RT/Articles.pm b/lib/RT/Articles.pm index 2eb1743ee95..6b403900175 100644 --- a/lib/RT/Articles.pm +++ b/lib/RT/Articles.pm @@ -975,6 +975,11 @@ sub SimpleSearch { return $self; } +sub CurrentUserCanSeeAll { + my $self = shift; + return $self->CurrentUser->HasRight( Right => 'ShowArticle', Object => RT->System ) ? 1 : 0; +} + RT::Base->_ImportOverlays(); 1; diff --git a/lib/RT/Assets.pm b/lib/RT/Assets.pm index f6cf8ee647b..516869f9ccb 100644 --- a/lib/RT/Assets.pm +++ b/lib/RT/Assets.pm @@ -1932,6 +1932,11 @@ sub _ProcessRestrictions { } +sub CurrentUserCanSeeAll { + my $self = shift; + return $self->CurrentUser->HasRight( Right => 'ShowAsset', Object => RT->System ) ? 1 : 0; +} + 1; RT::Base->_ImportOverlays(); diff --git a/lib/RT/Catalogs.pm b/lib/RT/Catalogs.pm index 250793c47b8..6d67a47720d 100644 --- a/lib/RT/Catalogs.pm +++ b/lib/RT/Catalogs.pm @@ -114,6 +114,11 @@ sub _Init { sub Table { "Catalogs" } +sub CurrentUserCanSeeAll { + my $self = shift; + return $self->CurrentUser->HasRight( Right => 'ShowCatalog', Object => RT->System ) ? 1 : 0; +} + RT::Base->_ImportOverlays(); 1; diff --git a/lib/RT/Classes.pm b/lib/RT/Classes.pm index ff446dddfb3..9cac032d484 100644 --- a/lib/RT/Classes.pm +++ b/lib/RT/Classes.pm @@ -81,6 +81,11 @@ sub AddRecord { sub _SingularClass { "RT::Class" } +sub CurrentUserCanSeeAll { + my $self = shift; + return $self->CurrentUser->HasRight( Right => 'SeeClass', Object => RT->System ) ? 1 : 0; +} + RT::Base->_ImportOverlays(); 1; diff --git a/lib/RT/CustomFields.pm b/lib/RT/CustomFields.pm index 253513d0f93..0fc5569adb9 100644 --- a/lib/RT/CustomFields.pm +++ b/lib/RT/CustomFields.pm @@ -475,6 +475,11 @@ sub LimitToCatalog { } } +sub CurrentUserCanSeeAll { + my $self = shift; + return $self->CurrentUser->HasRight( Right => 'SeeCustomField', Object => RT->System ) ? 1 : 0; +} + RT::Base->_ImportOverlays(); 1; diff --git a/lib/RT/CustomRoles.pm b/lib/RT/CustomRoles.pm index 6cac6768588..75587c71ed7 100644 --- a/lib/RT/CustomRoles.pm +++ b/lib/RT/CustomRoles.pm @@ -213,6 +213,12 @@ sub LimitToAdded { return RT::ObjectCustomRoles->new( $self->CurrentUser )->LimitTargetToAdded( $self => @_ ); } +sub CurrentUserCanSeeAll { + my $self = shift; + # Not typo, user needs SeeQueue to see CustomRoles + return $self->CurrentUser->HasRight( Right => 'SeeQueue', Object => RT->System ) ? 1 : 0; +} + RT::Base->_ImportOverlays(); 1; diff --git a/lib/RT/Groups.pm b/lib/RT/Groups.pm index 2f341bbba34..51a43f95e3e 100644 --- a/lib/RT/Groups.pm +++ b/lib/RT/Groups.pm @@ -537,6 +537,11 @@ sub SimpleSearch { return $self; } +sub CurrentUserCanSeeAll { + my $self = shift; + return $self->CurrentUser->HasRight( Right => 'SeeGroup', Object => RT->System ) ? 1 : 0; +} + RT::Base->_ImportOverlays(); 1; diff --git a/lib/RT/Queues.pm b/lib/RT/Queues.pm index 5a916d70a08..0d031b8ebee 100644 --- a/lib/RT/Queues.pm +++ b/lib/RT/Queues.pm @@ -128,6 +128,11 @@ sub ItemsOrderBy { return $items; } +sub CurrentUserCanSeeAll { + my $self = shift; + return $self->CurrentUser->HasRight( Right => 'SeeQueue', Object => RT->System ) ? 1 : 0; +} + RT::Base->_ImportOverlays(); 1; diff --git a/lib/RT/REST2/Resource/Collection.pm b/lib/RT/REST2/Resource/Collection.pm index 2f0f16eab29..7653d5695c6 100644 --- a/lib/RT/REST2/Resource/Collection.pm +++ b/lib/RT/REST2/Resource/Collection.pm @@ -189,7 +189,7 @@ sub serialize { my %results = ( count => scalar(@results) + 0, - total => $collection->CountAll + 0, + total => $collection->CurrentUserCanSeeAll ? ( $collection->CountAll + 0 ) : undef, per_page => $collection->RowsPerPage + 0, page => ($collection->FirstRow / $collection->RowsPerPage) + 1, items => \@results, @@ -205,18 +205,20 @@ sub serialize { } } - $results{pages} = ceil($results{total} / $results{per_page}); - 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) ); - $results{prev_page} = $uri->as_string; - }; + $results{pages} = defined $results{total} ? ceil($results{total} / $results{per_page}) : 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) ); + $results{prev_page} = $uri->as_string; + } + } return \%results; } diff --git a/lib/RT/SearchBuilder.pm b/lib/RT/SearchBuilder.pm index adb1e4e953a..03088d93942 100644 --- a/lib/RT/SearchBuilder.pm +++ b/lib/RT/SearchBuilder.pm @@ -1150,6 +1150,11 @@ sub DistinctFieldValues { return @values; } +sub CurrentUserCanSeeAll { + my $self = shift; + return $self->CurrentUser->HasRight( Right => 'SuperUser', Object => RT->System ) ? 1 : 0; +} + RT::Base->_ImportOverlays(); 1; diff --git a/lib/RT/Tickets.pm b/lib/RT/Tickets.pm index a62b7de6a1e..5f6f3e54871 100644 --- a/lib/RT/Tickets.pm +++ b/lib/RT/Tickets.pm @@ -3716,6 +3716,12 @@ sub Query { return $self->{_sql_query}; } +sub CurrentUserCanSeeAll { + my $self = shift; + return 1 if RT->Config->Get('UseSQLForACLChecks'); + return $self->CurrentUser->HasRight( Right => 'ShowTicket', Object => RT->System ) ? 1 : 0; +} + RT::Base->_ImportOverlays(); 1; diff --git a/lib/RT/Users.pm b/lib/RT/Users.pm index 33bbbac1f98..16297e4ddf6 100644 --- a/lib/RT/Users.pm +++ b/lib/RT/Users.pm @@ -715,6 +715,11 @@ sub SimpleSearch { return $self; } +sub CurrentUserCanSeeAll { + my $self = shift; + return $self->CurrentUser->Privileged; +} + RT::Base->_ImportOverlays(); 1; From fcf8f76bb5ed19c35c7c5997588feb7b0b556ef2 Mon Sep 17 00:00:00 2001 From: sunnavy Date: Thu, 8 Sep 2022 21:54:09 +0800 Subject: [PATCH 15/21] Require SeeGroup to view group transactions This is to protect group history, which doesn't need to be globally visible. --- lib/RT/Group.pm | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/lib/RT/Group.pm b/lib/RT/Group.pm index afa8465f6f8..4d666f5ba13 100644 --- a/lib/RT/Group.pm +++ b/lib/RT/Group.pm @@ -1311,15 +1311,20 @@ sub _Set { =head2 CurrentUserCanSee -Always returns 1; unfortunately, for historical reasons, users have -always been able to examine groups they have indirect access to, even if -they do not have SeeGroup explicitly. +Unfortunately, for historical reasons, users have always been able to +examine groups they have indirect access to, even if they do not have +SeeGroup explicitly. + +We do require "SeeGroup" to see transactions of current group. =cut sub CurrentUserCanSee { my $self = shift; - return 1; + my ($what, $txn) = @_; + + return 1 if ( $what // '' ) ne 'Transaction'; + return $self->CurrentUserHasRight('SeeGroup'); } =head2 CurrentUserCanCreate From 7b0ff85dd18d0ac5f407edc138034dd5e0b35efd Mon Sep 17 00:00:00 2001 From: sunnavy Date: Tue, 20 Sep 2022 05:18:22 +0800 Subject: [PATCH 16/21] Update tests for the "forbidden" method improvement See also 666f8b43a7. --- t/rest2/cf-image.t | 2 +- t/rest2/group-members.t | 24 +++++------------------- 2 files changed, 6 insertions(+), 20 deletions(-) diff --git a/t/rest2/cf-image.t b/t/rest2/cf-image.t index f311c7abec5..5025ddf2b42 100644 --- a/t/rest2/cf-image.t +++ b/t/rest2/cf-image.t @@ -47,7 +47,7 @@ $user->PrincipalObj->GrantRight( Right => 'SeeCustomField' ); my $res = $mech->get("$rest_base_path/download/cf/666", 'Authorization' => $auth, ); - is($res->code, 404); + is($res->code, 403); } # Download cf text diff --git a/t/rest2/group-members.t b/t/rest2/group-members.t index 3ceeab8fb5b..58c3e675e53 100644 --- a/t/rest2/group-members.t +++ b/t/rest2/group-members.t @@ -59,19 +59,12 @@ $group2->Load($group2_id); ); is($res->code, 403, 'Cannot disable group without AdminGroup right'); - # Rights Test - With AdminGroup, no SeeGroup + # Rights Test - With AdminGroup $user->PrincipalObj->GrantRight(Right => 'AdminGroup', Object => $group2); $res = $mech->delete($group2_url, 'Authorization' => $auth, ); - is($res->code, 403, 'Cannot disable group without SeeGroup right'); - - # Rights Test - With AdminGroup, no SeeGroup - $user->PrincipalObj->GrantRight(Right => 'SeeGroup', Object => $group2); - $res = $mech->delete($group2_url, - 'Authorization' => $auth, - ); - is($res->code, 204, 'Disable group with AdminGroup & SeeGroup rights'); + is($res->code, 204, 'Disable group with AdminGroup rights'); is($group2->Disabled, 1, "Group disabled"); } @@ -91,19 +84,12 @@ $group2->Load($group2_id); 'Authorization' => $auth); is($res->code, 403, 'Cannot enable group without AdminGroup right'); - # Rights Test - With AdminGroup, no SeeGroup + # Rights Test - With AdminGroup $user->PrincipalObj->GrantRight(Right => 'AdminGroup', Object => $group2); $res = $mech->put_json($group2_url, $payload, 'Authorization' => $auth); - is($res->code, 403, 'Cannot enable group without SeeGroup right'); - - # Rights Test - With AdminGroup, no SeeGroup - $user->PrincipalObj->GrantRight(Right => 'SeeGroup', Object => $group2); - $res = $mech->put_json($group2_url, - $payload, - 'Authorization' => $auth); - is($res->code, 200, 'Enable group with AdminGroup & SeeGroup rights'); + is($res->code, 200, 'Enable group with AdminGroup rights'); is_deeply($mech->json_response, ['Group enabled']); $group2->Load( $group2->Id ); # Reload to refresh $group2->PrincipalObj @@ -112,7 +98,7 @@ $group2->Load($group2_id); my $group1_id = $group1->id; (my $group1_url = $group2_url) =~ s/$group2_id/$group1_id/; -$user->PrincipalObj->GrantRight(Right => 'SeeGroup', Object => $group1); +$user->PrincipalObj->GrantRight(Right => 'SeeGroup', Object => $_) for $group1, $group2; # Members addition { From c4159088d317827a7dd8ebfd981269e970bcb59a Mon Sep 17 00:00:00 2001 From: sunnavy Date: Tue, 20 Sep 2022 05:05:31 +0800 Subject: [PATCH 17/21] Update tests for the total/pages hiding change See also c2d5d72cd5. --- t/rest2/articles.t | 2 +- t/rest2/assets.t | 2 +- t/rest2/attachments.t | 4 ++-- t/rest2/customfields.t | 2 +- t/rest2/searches.t | 2 +- t/rest2/tickets.t | 4 +--- t/rest2/transactions.t | 4 ++-- 7 files changed, 9 insertions(+), 11 deletions(-) diff --git a/t/rest2/articles.t b/t/rest2/articles.t index 462a407dd77..3363a341762 100644 --- a/t/rest2/articles.t +++ b/t/rest2/articles.t @@ -172,7 +172,7 @@ TODO: { is( $content->{count}, 2 ); is( $content->{page}, 1 ); is( $content->{per_page}, 20 ); - is( $content->{total}, 2 ); + is( $content->{total}, undef, 'No total count'); is( scalar @{ $content->{items} }, 2 ); for my $txn ( @{ $content->{items} } ) { diff --git a/t/rest2/assets.t b/t/rest2/assets.t index 2685b2dff29..03585d87109 100644 --- a/t/rest2/assets.t +++ b/t/rest2/assets.t @@ -256,7 +256,7 @@ my ($asset_url, $asset_id); is($content->{count}, 3); is($content->{page}, 1); is($content->{per_page}, 20); - is($content->{total}, 3); + is($content->{total}, undef, 'No total'); is(scalar @{$content->{items}}, 3); for my $txn (@{ $content->{items} }) { diff --git a/t/rest2/attachments.t b/t/rest2/attachments.t index 416ea504e57..f76baf12b6a 100644 --- a/t/rest2/attachments.t +++ b/t/rest2/attachments.t @@ -109,8 +109,8 @@ $image_content = MIME::Base64::encode_base64($image_content); cmp_deeply( $mech->json_response, { per_page => 20, - pages => 1, - total => 4, + pages => undef, + total => undef, page => 1, count => 4, items => [ diff --git a/t/rest2/customfields.t b/t/rest2/customfields.t index 60d6790e4b1..463215731f0 100644 --- a/t/rest2/customfields.t +++ b/t/rest2/customfields.t @@ -82,7 +82,7 @@ my $freeform_cf_id; is($res->code, 200); my $content = $mech->json_response; - is($content->{total}, 3); + is($content->{total}, undef, 'No total'); is($content->{count}, 0); is_deeply($content->{items}, []); } diff --git a/t/rest2/searches.t b/t/rest2/searches.t index 067ba1fd63a..bfae6fdef65 100644 --- a/t/rest2/searches.t +++ b/t/rest2/searches.t @@ -65,7 +65,7 @@ ok( $ret, "created $msg" ); is( $content->{count}, 4, '4 searches' ); is( $content->{page}, 1, '1 page' ); is( $content->{per_page}, 20, '20 per_page' ); - is( $content->{total}, 4, '4 total' ); + is( $content->{total}, undef, 'No total' ); is( scalar @{ $content->{items} }, 4, 'items count' ); for my $item ( @{ $content->{items} } ) { diff --git a/t/rest2/tickets.t b/t/rest2/tickets.t index 7c6a378be26..fd563c82e73 100644 --- a/t/rest2/tickets.t +++ b/t/rest2/tickets.t @@ -396,9 +396,7 @@ my ($ticket_url, $ticket_id); is($content->{page}, 1); is($content->{per_page}, 20); - # TODO This 14 VS 15 inconsitency is because user lacks ShowOutgoingEmail. - # It'll be perfect if we can keep them in sync - is($content->{total}, 15); + is($content->{total}, undef, 'No total'); is(scalar @{$content->{items}}, 14); for my $txn (@{ $content->{items} }) { diff --git a/t/rest2/transactions.t b/t/rest2/transactions.t index a2f0a61cd60..fb5b13fbce5 100644 --- a/t/rest2/transactions.t +++ b/t/rest2/transactions.t @@ -57,7 +57,7 @@ my ($delete_link1_txn_url, $delete_link1_txn_id, $delete_link2_txn_url, $delete_ is($content->{count}, 10); is($content->{page}, 1); is($content->{per_page}, 20); - is($content->{total}, 10); + is($content->{total}, undef, 'No total'); is(scalar @{$content->{items}}, 10); my ( @@ -103,7 +103,7 @@ my ($delete_link1_txn_url, $delete_link1_txn_id, $delete_link2_txn_url, $delete_ is($content->{count}, 10); is($content->{page}, 1); is($content->{per_page}, 20); - is($content->{total}, 10); + is($content->{total}, undef, 'No total'); is(scalar @{$content->{items}}, 10); my ( From 3b7cfecc2758145e80ea0d6c10998d34c953a2fa Mon Sep 17 00:00:00 2001 From: sunnavy Date: Mon, 25 Sep 2023 16:55:33 -0400 Subject: [PATCH 18/21] Update tests as RT-Send-Cc is cleared now --- t/mail/sendmail-plaintext.t | 2 +- t/mail/sendmail.t | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/t/mail/sendmail-plaintext.t b/t/mail/sendmail-plaintext.t index b9eb719516a..141039244c7 100644 --- a/t/mail/sendmail-plaintext.t +++ b/t/mail/sendmail-plaintext.t @@ -132,7 +132,7 @@ for my $encoding ('ISO-8859-1', 'UTF-8') { { my ($ticket) = mail_in_ticket('rt-send-cc'); my $cc = first_attach($ticket)->GetHeader('RT-Send-Cc'); - like ($cc, qr/test$_/, "Found test $_") for 1..5; + ok (!$cc, "No RT-Send-Cc"); # RT-Send-Cc is supposed to be cleared } { diff --git a/t/mail/sendmail.t b/t/mail/sendmail.t index 4ef320611b8..d6ead4d802d 100644 --- a/t/mail/sendmail.t +++ b/t/mail/sendmail.t @@ -157,7 +157,7 @@ for my $encoding ('ISO-8859-1', 'UTF-8') { { my ($ticket) = mail_in_ticket('rt-send-cc'); my $cc = first_attach($ticket)->GetHeader('RT-Send-Cc'); - like ($cc, qr/test$_/, "Found test $_") for 1..5; + ok (!$cc, "No RT-Send-Cc"); # RT-Send-Cc is supposed to be cleared } { From 5f3e8b6675cdbc30ca8609707a4f6c11c2ad7fb7 Mon Sep 17 00:00:00 2001 From: sunnavy Date: Wed, 27 Sep 2023 12:16:07 -0400 Subject: [PATCH 19/21] Generate correct SQL for txn search when Owner has ShowTicket --- lib/RT/Tickets.pm | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/RT/Tickets.pm b/lib/RT/Tickets.pm index dae0d3bf2cb..88afccb28ec 100644 --- a/lib/RT/Tickets.pm +++ b/lib/RT/Tickets.pm @@ -3057,6 +3057,8 @@ sub CurrentUserCanSee { FIELD => 'Owner', VALUE => $id, ENTRYAGGREGATOR => $ea, + # RT::Transactions::CurrentUserCanSee reuses RT::Tickets::CurrentUserCanSee + ALIAS => $self->isa('RT::Transactions') ? $self->_JoinTickets : 'main', ); } else { From 5f76719a96e1a10d19943b70e961859cdd4138b3 Mon Sep 17 00:00:00 2001 From: sunnavy Date: Thu, 12 Oct 2023 16:43:33 -0400 Subject: [PATCH 20/21] Enable devel mode for mailgate tests that depend on detailed output Now we only show detailed output on devel mode, see also ef958dadf4. --- t/mail/gateway.t | 2 +- t/mail/han-encodings.t | 2 +- t/ticket/interface.t | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/t/mail/gateway.t b/t/mail/gateway.t index c51daa90925..8f9e941c40a 100644 --- a/t/mail/gateway.t +++ b/t/mail/gateway.t @@ -2,7 +2,7 @@ use strict; use warnings; -use RT::Test config => 'Set( @MailPlugins, "Action::Take", "Action::Resolve");', tests => undef, actual_server => 1; +use RT::Test config => 'Set( @MailPlugins, "Action::Take", "Action::Resolve"); Set($DevelMode, 1);', tests => undef, actual_server => 1; my ($baseurl, $m) = RT::Test->started_ok; use RT::Tickets; diff --git a/t/mail/han-encodings.t b/t/mail/han-encodings.t index bc522649613..8bd34e2572e 100644 --- a/t/mail/han-encodings.t +++ b/t/mail/han-encodings.t @@ -1,7 +1,7 @@ use strict; use warnings; -use RT::Test tests => undef, actual_server => 1; +use RT::Test tests => undef, config => 'Set($DevelMode, 1);', actual_server => 1; # we can't simply call RT::StaticUtil::RequireModule("Encode::HanExtra") here because we are testing # if Encode::HanExtra could be automatically loaded. diff --git a/t/ticket/interface.t b/t/ticket/interface.t index c43e13419e5..9067f86967e 100644 --- a/t/ticket/interface.t +++ b/t/ticket/interface.t @@ -1,7 +1,7 @@ use strict; use warnings; -use RT::Test tests => undef, actual_server => 1; +use RT::Test tests => undef, config => 'Set($DevelMode, 1);', actual_server => 1; my ( $baseurl, $m ) = RT::Test->started_ok; From 54a3f830eb4205550692ec6f57c5124acdbf5bf1 Mon Sep 17 00:00:00 2001 From: sunnavy Date: Tue, 17 Oct 2023 16:30:03 -0400 Subject: [PATCH 21/21] Fix typo --- docs/web_deployment.pod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/web_deployment.pod b/docs/web_deployment.pod index 5d2f3eccb65..d186800bc2f 100644 --- a/docs/web_deployment.pod +++ b/docs/web_deployment.pod @@ -169,7 +169,7 @@ If you run C on a separate server, you can update the above to allow additional IP addresses. - Require ip 127.0.0.1 ::1 192.0.2.0 # Add you actual IPs + Require ip 127.0.0.1 ::1 192.0.2.0 # Add your actual IPs See the L