From 5a40739451c1b3073128c7f472e5c2c7edaca534 Mon Sep 17 00:00:00 2001 From: Valerio Bozzolan Date: Tue, 4 Jun 2024 15:57:20 +0200 Subject: [PATCH] Show login page if a search token requires a valid viewer Summary: A saved query can have tokens that require a valid current viewer. For example, this token: viewer() Before this change, visiting such saved queries would cause this: This datasource ("PhabricatorPeopleUserFunctionDatasource") can not evaluate the function "viewer(...)". After this change, instead of that, you are just redirected to the login page, so, after you do the login, you are redirected back to that saved query and it works. This fix was boosted during the Wikimedia Hackaton (wmhack) in Tallinn. Thanks Tallinn! https://phabricator.wikimedia.org/T356384 Fixes T15704 Test Plan: Go to Maniphest > Advanced Search > Assigned to > "Viewer". It still works. Visit the same page in a new anonymous tab: now it redirects to the login page. You login, and that page works again. Do the same specific test for all these cases: - Maniphest - Assigned To: viewer - Tags: current Viewer's Projects - Authors: viewer - Subscribers: ... - Closed by - Badges - Subscribers - Differential - Responsible Users - Authors - Reviewers - Subscribers - Tags - Dashboards - Authored By - Tags - Dashboard Panels - Authored By - Dashboard Portals - Tags - Calendar: - Hosts - Invited - Subscribers - Tags - Countdown - Authors - Diffusion - Tags - Subscribers - Tags - Diffusion commit - Responsible Users - Authors - Subscribers - Tags - Diffusion identities - Matching Users - Feed - Include Users - Include Projects (interestingly it does not support "current Viewer's Projects") - Files - Authors - Herald - Authors - Subscribers - Legalpad - Subscribers - Nuance (none of their entity support search by token) - Passphrase - Subscribers - Paste - Authors - Subscribers - Tags - Phame - Subscribers - Tags - Pholio - Authors - Subscribers - Tags - Phrequent - Users (interestingly it does not support "viewer") - Ponder - Authors - Answered By - Projects - Members - Watchers - Transactions - /feed/transactions/ - Authors - General search at /search/query/ - Authors - Owners - Subscribers - Tags All the above fields were tested in a clean search, one at a time, both logged-in and logged-out, with the function "viewer" or anything similar like "current Viewer's Projects": For all cases, the login page appeared successfully where needed, instead of a crash. Reviewers: O1 Blessed Committers, aklapper Reviewed By: O1 Blessed Committers, aklapper Subscribers: aklapper, avivey, tobiaswiese, Matthew, Cigaryno Maniphest Tasks: T15704 Differential Revision: https://we.phorge.it/D25621 --- src/__phutil_library_map__.php | 2 ++ ...alendarInviteeViewerFunctionDatasource.php | 8 ++++-- ...ialResponsibleViewerFunctionDatasource.php | 8 ++++-- .../typeahead/PhabricatorViewerDatasource.php | 8 ++++-- ...bricatorProjectLogicalViewerDatasource.php | 8 ++++-- ...PhabricatorApplicationSearchController.php | 9 +++++++ ...habricatorTypeaheadCompositeDatasource.php | 9 +++++++ .../PhabricatorTypeaheadDatasource.php | 25 +++++++++++++++++++ ...ricatorTypeaheadLoginRequiredException.php | 6 +++++ 9 files changed, 75 insertions(+), 8 deletions(-) create mode 100644 src/applications/typeahead/exception/PhabricatorTypeaheadLoginRequiredException.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 02e4c7f2b3..9287085d03 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -5078,6 +5078,7 @@ phutil_register_library_map(array( 'PhabricatorTypeaheadDatasourceTestCase' => 'applications/typeahead/datasource/__tests__/PhabricatorTypeaheadDatasourceTestCase.php', 'PhabricatorTypeaheadFunctionHelpController' => 'applications/typeahead/controller/PhabricatorTypeaheadFunctionHelpController.php', 'PhabricatorTypeaheadInvalidTokenException' => 'applications/typeahead/exception/PhabricatorTypeaheadInvalidTokenException.php', + 'PhabricatorTypeaheadLoginRequiredException' => 'applications/typeahead/exception/PhabricatorTypeaheadLoginRequiredException.php', 'PhabricatorTypeaheadModularDatasourceController' => 'applications/typeahead/controller/PhabricatorTypeaheadModularDatasourceController.php', 'PhabricatorTypeaheadMonogramDatasource' => 'applications/typeahead/datasource/PhabricatorTypeaheadMonogramDatasource.php', 'PhabricatorTypeaheadProxyDatasource' => 'applications/typeahead/datasource/PhabricatorTypeaheadProxyDatasource.php', @@ -11819,6 +11820,7 @@ phutil_register_library_map(array( 'PhabricatorTypeaheadDatasourceTestCase' => 'PhabricatorTestCase', 'PhabricatorTypeaheadFunctionHelpController' => 'PhabricatorTypeaheadDatasourceController', 'PhabricatorTypeaheadInvalidTokenException' => 'Exception', + 'PhabricatorTypeaheadLoginRequiredException' => 'Exception', 'PhabricatorTypeaheadModularDatasourceController' => 'PhabricatorTypeaheadDatasourceController', 'PhabricatorTypeaheadMonogramDatasource' => 'PhabricatorTypeaheadDatasource', 'PhabricatorTypeaheadProxyDatasource' => 'PhabricatorTypeaheadCompositeDatasource', diff --git a/src/applications/calendar/typeahead/PhabricatorCalendarInviteeViewerFunctionDatasource.php b/src/applications/calendar/typeahead/PhabricatorCalendarInviteeViewerFunctionDatasource.php index d653c9bdea..ef8cb76dc1 100644 --- a/src/applications/calendar/typeahead/PhabricatorCalendarInviteeViewerFunctionDatasource.php +++ b/src/applications/calendar/typeahead/PhabricatorCalendarInviteeViewerFunctionDatasource.php @@ -28,8 +28,12 @@ final class PhabricatorCalendarInviteeViewerFunctionDatasource ); } + protected function isFunctionWithLoginRequired($function) { + return true; + } + public function loadResults() { - if ($this->getViewer()->getPHID()) { + if ($this->getViewer()->isLoggedIn()) { $results = array($this->renderViewerFunctionToken()); } else { $results = array(); @@ -39,7 +43,7 @@ final class PhabricatorCalendarInviteeViewerFunctionDatasource } protected function canEvaluateFunction($function) { - if (!$this->getViewer()->getPHID()) { + if (!$this->getViewer()->isLoggedIn()) { return false; } diff --git a/src/applications/differential/typeahead/DifferentialResponsibleViewerFunctionDatasource.php b/src/applications/differential/typeahead/DifferentialResponsibleViewerFunctionDatasource.php index 05ae39ec2e..4d735b0c94 100644 --- a/src/applications/differential/typeahead/DifferentialResponsibleViewerFunctionDatasource.php +++ b/src/applications/differential/typeahead/DifferentialResponsibleViewerFunctionDatasource.php @@ -28,8 +28,12 @@ final class DifferentialResponsibleViewerFunctionDatasource ); } + protected function isFunctionWithLoginRequired($function) { + return true; + } + public function loadResults() { - if ($this->getViewer()->getPHID()) { + if ($this->getViewer()->isLoggedIn()) { $results = array($this->renderViewerFunctionToken()); } else { $results = array(); @@ -39,7 +43,7 @@ final class DifferentialResponsibleViewerFunctionDatasource } protected function canEvaluateFunction($function) { - if (!$this->getViewer()->getPHID()) { + if (!$this->getViewer()->isLoggedIn()) { return false; } diff --git a/src/applications/people/typeahead/PhabricatorViewerDatasource.php b/src/applications/people/typeahead/PhabricatorViewerDatasource.php index 6f6d1181fe..cb367ccd4d 100644 --- a/src/applications/people/typeahead/PhabricatorViewerDatasource.php +++ b/src/applications/people/typeahead/PhabricatorViewerDatasource.php @@ -34,8 +34,12 @@ final class PhabricatorViewerDatasource ); } + protected function isFunctionWithLoginRequired($function) { + return true; + } + public function loadResults() { - if ($this->getViewer()->getPHID()) { + if ($this->getViewer()->isLoggedIn()) { $results = array($this->renderViewerFunctionToken()); } else { $results = array(); @@ -45,7 +49,7 @@ final class PhabricatorViewerDatasource } protected function canEvaluateFunction($function) { - if (!$this->getViewer()->getPHID()) { + if (!$this->getViewer()->isLoggedIn()) { return false; } diff --git a/src/applications/project/typeahead/PhabricatorProjectLogicalViewerDatasource.php b/src/applications/project/typeahead/PhabricatorProjectLogicalViewerDatasource.php index 9d1a91376a..acd4ea8aa7 100644 --- a/src/applications/project/typeahead/PhabricatorProjectLogicalViewerDatasource.php +++ b/src/applications/project/typeahead/PhabricatorProjectLogicalViewerDatasource.php @@ -35,8 +35,12 @@ final class PhabricatorProjectLogicalViewerDatasource ); } + protected function isFunctionWithLoginRequired($function) { + return true; + } + public function loadResults() { - if ($this->getViewer()->getPHID()) { + if ($this->getViewer()->isLoggedIn()) { $results = array($this->renderViewerProjectsFunctionToken()); } else { $results = array(); @@ -46,7 +50,7 @@ final class PhabricatorProjectLogicalViewerDatasource } protected function canEvaluateFunction($function) { - if (!$this->getViewer()->getPHID()) { + if (!$this->getViewer()->isLoggedIn()) { return false; } diff --git a/src/applications/search/controller/PhabricatorApplicationSearchController.php b/src/applications/search/controller/PhabricatorApplicationSearchController.php index d8f53538e4..5178a1a1d9 100644 --- a/src/applications/search/controller/PhabricatorApplicationSearchController.php +++ b/src/applications/search/controller/PhabricatorApplicationSearchController.php @@ -346,6 +346,15 @@ final class PhabricatorApplicationSearchController $body[] = $pager_box; } } + } catch (PhabricatorTypeaheadLoginRequiredException $ex) { + + // A specific token requires login. Show login page. + $auth_class = PhabricatorAuthApplication::class; + $auth_application = PhabricatorApplication::getByClass($auth_class); + $login_controller = new PhabricatorAuthStartController(); + $this->setCurrentApplication($auth_application); + return $this->delegateToController($login_controller); + } catch (PhabricatorTypeaheadInvalidTokenException $ex) { $exec_errors[] = pht( 'This query specifies an invalid parameter. Review the '. diff --git a/src/applications/typeahead/datasource/PhabricatorTypeaheadCompositeDatasource.php b/src/applications/typeahead/datasource/PhabricatorTypeaheadCompositeDatasource.php index 8a396c1ed3..338c4db564 100644 --- a/src/applications/typeahead/datasource/PhabricatorTypeaheadCompositeDatasource.php +++ b/src/applications/typeahead/datasource/PhabricatorTypeaheadCompositeDatasource.php @@ -304,6 +304,15 @@ abstract class PhabricatorTypeaheadCompositeDatasource return parent::evaluateFunction($function, $argv); } + protected function isFunctionWithLoginRequired($function) { + foreach ($this->getUsableDatasources() as $source) { + if ($source->isFunctionWithLoginRequired($function)) { + return true; + } + } + return parent::isFunctionWithLoginRequired($function); + } + public function renderFunctionTokens($function, array $argv_list) { foreach ($this->getUsableDatasources() as $source) { if ($source->canEvaluateFunction($function)) { diff --git a/src/applications/typeahead/datasource/PhabricatorTypeaheadDatasource.php b/src/applications/typeahead/datasource/PhabricatorTypeaheadDatasource.php index a35a8e8f0f..308a8bfe1a 100644 --- a/src/applications/typeahead/datasource/PhabricatorTypeaheadDatasource.php +++ b/src/applications/typeahead/datasource/PhabricatorTypeaheadDatasource.php @@ -365,6 +365,19 @@ abstract class PhabricatorTypeaheadDatasource extends Phobject { } + /** + * Check if this datasource requires a logged-in viewer. + * @task functions + * @param string $function Function name. + * @return bool + */ + protected function isFunctionWithLoginRequired($function) { + // This is just a default. + // Make sure to override this method to require login. + return false; + } + + /** * @task functions */ @@ -498,6 +511,18 @@ abstract class PhabricatorTypeaheadDatasource extends Phobject { if (!$this->canEvaluateFunction($function)) { if (!$allow_partial) { + + if ($this->isFunctionWithLoginRequired($function)) { + if (!$this->getViewer() || !$this->getViewer()->isLoggedIn()) { + throw new PhabricatorTypeaheadLoginRequiredException( + pht( + 'This datasource ("%s") requires to be logged-in to use the '. + 'function "%s(...)".', + get_class($this), + $function)); + } + } + throw new PhabricatorTypeaheadInvalidTokenException( pht( 'This datasource ("%s") can not evaluate the function "%s(...)".', diff --git a/src/applications/typeahead/exception/PhabricatorTypeaheadLoginRequiredException.php b/src/applications/typeahead/exception/PhabricatorTypeaheadLoginRequiredException.php new file mode 100644 index 0000000000..3b29df5df8 --- /dev/null +++ b/src/applications/typeahead/exception/PhabricatorTypeaheadLoginRequiredException.php @@ -0,0 +1,6 @@ +