Page MenuHomePhorge
Differential D25621

Show login page if a search token requires a valid viewer
Needs ReviewPublic
Actions

Authored by valerio.bozzolan on Mon, May 6, 02:10.
Tags
None
Referenced Files
F2194901: D25621.1716103303.diff
Sat, May 18, 07:21
Unknown Object (File)
Thu, May 16, 00:54
Unknown Object (File)
Thu, May 16, 00:27
Unknown Object (File)
Thu, May 16, 00:27
Unknown Object (File)
Wed, May 15, 20:23
Unknown Object (File)
Wed, May 15, 11:41
Unknown Object (File)
Wed, May 15, 09:00
Unknown Object (File)
Wed, May 15, 08:45
View All 32 Files
Subscribers
aklapper
avivey
Cigaryno
Matthew
tobiaswiese
Time Spent
valerio.bozzolan4 h

Details

Reviewers
None
Group Reviewers
O1: Blessed Committers
Maniphest Tasks
T15704: Cryptic exception when listing tasks "Closed By: Current Viewer" while logged out
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
arcpatch-D25621_2
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1241
Build 1241: arc lint + arc unit

Event Timeline

valerio.bozzolan created this revision.Mon, May 6, 02:10
valerio.bozzolan held this revision as a draft.
Owners added a reviewer: O1: Blessed Committers.Mon, May 6, 02:10
Herald added subscribers: Cigaryno, Matthew, tobiaswiese. · View Herald TranscriptMon, May 6, 02:10
Harbormaster completed remote builds in B1225: Diff 1939.Mon, May 6, 02:10
valerio.bozzolan added inline comments.Mon, May 6, 02:18
src/applications/people/typeahead/PhabricatorViewerDatasource.php
42

✅ Just a cosmetic change.

52

✅ Just a cosmetic change.

valerio.bozzolan mentioned this in D25505: Improve handling queries with "Current Viewer" set while not logged in.Mon, May 6, 02:18

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.Tue, May 7, 14:42
valerio.bozzolan updated this revision to Diff 1946.Tue, May 7, 15:28
valerio.bozzolan added a subscriber: avivey.

thanks to @avivey now it works

Harbormaster completed remote builds in B1230: Diff 1946.Tue, May 7, 15:28
valerio.bozzolan published this revision for review.Tue, May 7, 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

valerio.bozzolan updated this revision to Diff 1947.Tue, May 7, 16:29

fix "abstract" function name

Harbormaster completed remote builds in B1231: Diff 1947.Tue, May 7, 16:29
valerio.bozzolan updated this revision to Diff 1953.Wed, May 8, 19:28

fix abstract function visibility, add more PHPDoc

Harbormaster completed remote builds in B1236: Diff 1953.Wed, May 8, 19:28
valerio.bozzolan added a comment.EditedThu, May 9, 07:26

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.

aklapper subscribed.Thu, May 9, 11:08

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?)
valerio.bozzolan updated this revision to Diff 1960.Thu, May 9, 12:23

Cover more cases that require login:

  • DifferentialResponsibleViewerFunctionDatasource
  • PhabricatorCalendarInviteeViewerFunctionDatasource
  • PhabricatorProjectLogicalViewerDatasource
Harbormaster completed remote builds in B1241: Diff 1960.Thu, May 9, 12:23
valerio.bozzolan edited the test plan for this revision. (Show Details)Thu, May 9, 15:53
valerio.bozzolan edited the test plan for this revision. (Show Details)Thu, May 9, 16:04
valerio.bozzolan edited the test plan for this revision. (Show Details)Tue, May 14, 08:58

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

Revision Contents
Changeset List

Diff 1960

View Options

src/__phutil_library_map__.php

Loading...
View Options

src/applications/calendar/typeahead/PhabricatorCalendarInviteeViewerFunctionDatasource.php

Loading...
View Options

src/applications/differential/typeahead/DifferentialResponsibleViewerFunctionDatasource.php

Loading...
View Options

src/applications/people/typeahead/PhabricatorViewerDatasource.php

Loading...
View Options

src/applications/project/typeahead/PhabricatorProjectLogicalViewerDatasource.php

Loading...
View Options

src/applications/search/controller/PhabricatorApplicationSearchController.php

Loading...
View Options

src/applications/typeahead/datasource/PhabricatorTypeaheadCompositeDatasource.php

Loading...
View Options

src/applications/typeahead/datasource/PhabricatorTypeaheadDatasource.php

Loading...
View Options

src/applications/typeahead/exception/PhabricatorTypeaheadLoginRequiredException.php

Loading...
Content licensed under Creative Commons Attribution-ShareAlike 4.0 (CC-BY-SA) unless otherwise noted; code licensed under Apache 2.0 or other open source licenses. · CC BY-SA 4.0 · Apache 2.0