Details

Reviewers

aklapper

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15704: Cryptic exception when listing tasks "Closed By: Current Viewer" while logged out

Commits

rP5a40739451c1: 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.

Diff Detail

Repository

rP Phorge

Branch

D25505-ciao-cipollina

Lint

Lint Passed

Severity	Location	Code	Message
Advice	src/applications/search/controller/PhabricatorApplicationSearchController.php:350	XHP16	TODO Comment

Unit

Tests Passed

Build Status

Buildable 1225
Build 1225: arc lint + arc unit

Event Timeline

valerio.bozzolan created this revision.May 6 2024, 02:10

valerio.bozzolan held this revision as a draft.

Owners added a reviewer: O1: Blessed Committers.May 6 2024, 02:10

Herald added subscribers: Cigaryno, Matthew, tobiaswiese. · View Herald TranscriptMay 6 2024, 02:10

Harbormaster completed remote builds in B1225: Diff 1939.May 6 2024, 02:10

valerio.bozzolan added inline comments.May 6 2024, 02:18

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 mentioned this in Z1: Phorge.May 7 2024, 14:42

thanks to @avivey now it works

Harbormaster completed remote builds in B1230: Diff 1946.May 7 2024, 15:28

valerio.bozzolan published this revision for review.May 7 2024, 15:38

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	This code was copied from here: https://we.phorge.it/source/phorge/browse/master/src/applications/base/controller/PhabricatorController.php;af300016b678fd0694a4ecde00de167677acff80$147-156

fix "abstract" function name

Harbormaster completed remote builds in B1231: Diff 1947.May 7 2024, 16:29

fix abstract function visibility, add more PHPDoc

Harbormaster completed remote builds in B1236: Diff 1953.May 8 2024, 19:28

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

Harbormaster completed remote builds in B1241: Diff 1960.May 9 2024, 12:23

valerio.bozzolan edited the test plan for this revision. (Show Details)May 9 2024, 15:53

valerio.bozzolan edited the test plan for this revision. (Show Details)May 9 2024, 16:04

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

valerio.bozzolan mentioned this in Next Up.Jun 4 2024, 14:10

Closed by commit rP5a40739451c1: Show login page if a search token requires a valid viewer. · Explain WhyJun 4 2024, 14:10

This revision was automatically updated to reflect the committed changes.

valerio.bozzolan added a commit: rP5a40739451c1: Show login page if a search token requires a valid viewer.

valerio.bozzolan mentioned this in 2024 week 35.Aug 28 2024, 10:19

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

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 1939

src/__phutil_library_map__.php

src/applications/people/typeahead/PhabricatorViewerDatasource.php

src/applications/search/controller/PhabricatorApplicationSearchController.php

src/applications/typeahead/datasource/PhabricatorTypeaheadCompositeDatasource.php

src/applications/typeahead/datasource/PhabricatorTypeaheadDatasource.php

src/applications/typeahead/exception/PhabricatorTypeaheadLoginRequiredException.php

Show login page if a search token requires a valid viewerClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 1939

src/__phutil_library_map__.php

src/applications/people/typeahead/PhabricatorViewerDatasource.php

src/applications/search/controller/PhabricatorApplicationSearchController.php

src/applications/typeahead/datasource/PhabricatorTypeaheadCompositeDatasource.php

src/applications/typeahead/datasource/PhabricatorTypeaheadDatasource.php

src/applications/typeahead/exception/PhabricatorTypeaheadLoginRequiredException.php

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

Revision Contents
Changeset List