Page MenuHomePhorge

Show login page if a search token requires a valid viewer
ClosedPublic

Authored by valerio.bozzolan on May 6 2024, 02:10.

Details

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.

Diff Detail

Repository
rP Phorge
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

valerio.bozzolan held this revision as a draft.
src/applications/people/typeahead/PhabricatorViewerDatasource.php
42

✅ Just a cosmetic change.

52

✅ Just a cosmetic change.

Help welcome about that TODO, about how to render a login page. I got lost in digging Aphront stuff :(

valerio.bozzolan retitled this revision from Show login page if a search token requires that to Show login page if a search token requires a valid viewer.
valerio.bozzolan edited the summary of this revision. (Show Details)
valerio.bozzolan edited the test plan for this revision. (Show Details)
valerio.bozzolan added inline comments.
src/applications/search/controller/PhabricatorApplicationSearchController.php
356

fix abstract function visibility, add more PHPDoc

Sharing some comments

src/applications/people/typeahead/PhabricatorViewerDatasource.php
37–40

✅ This overrides the one in PhabricatorTypeaheadDatasource

src/applications/typeahead/datasource/PhabricatorTypeaheadCompositeDatasource.php
307–315

✅ This overrides the one in PhabricatorTypeaheadDatasource. Code adapted from above line 298

src/applications/typeahead/datasource/PhabricatorTypeaheadDatasource.php
368–378

I tried to make this an abstract method, and then have all datasources overriding this. But that approach was a bit overkill.

So here it's just a sane default.

Thanks for working on this! Works pretty well locally. From a quick test of this branch, while being logged out,

  • Maniphest Advanced Search: Setting Assigned to to Current Viewer, I get a Login page.
  • Diffusion Commits Advanced Search: Setting Authors to Current Viewer, I get a Login page.
  • Differential Advanced Search: Setting both Authors and Reviewers to Current Viewer, I get a Login page.
  • Differential Advanced Search: Setting Responsible Users to Current Viewer, I get an exception (DifferentialResponsibleDatasource - probably another source to cover here?)

Cover more cases that require login:

  • DifferentialResponsibleViewerFunctionDatasource
  • PhabricatorCalendarInviteeViewerFunctionDatasource
  • PhabricatorProjectLogicalViewerDatasource

Uhm. Incredibly, it seems to work. I have good feelings.

Tested this with nearly all DataSources mentioned above before and after applying the patch in both a logged-in and a private browser window. Behaves as expected and no errors in the logs.

Interestingly, http://phorge.localhost/conduit/log/query/viewerdeprecated/ (and other Conduit queries setting Current Viewer as Callers on http://phorge.localhost/conduit/log/query/advanced/ ) already worked as expected without applying this patch (and behavior did not change after applying the patch).

This revision is now accepted and ready to land.Jun 4 2024, 12:18