Page MenuHomePhabricator

Make "Can Interact" and logged-out users interact more gracefully
ClosedPublic

Authored by epriestley on Mar 9 2017, 4:46 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Mar 10, 11:58 AM
Unknown Object (File)
Jan 24 2024, 5:28 PM
Unknown Object (File)
Dec 27 2023, 8:29 AM
Unknown Object (File)
Nov 30 2023, 6:17 AM
Unknown Object (File)
Nov 24 2023, 8:20 PM
Unknown Object (File)
Nov 19 2023, 8:55 AM
Unknown Object (File)
Oct 24 2023, 8:15 PM
Unknown Object (File)
Oct 23 2023, 5:03 PM
Subscribers
None

Details

Summary

Fixes T12378. Two minor issues here:

  • CAN_INTERACT on tasks uses "USER", but should just use the view policy, which may be more permissive ("PUBLIC").
  • CAN_INTERACT is currently prevented from being "PUBLIC" by additional safeguards. Define an explicit capability object for the permission which returns true from shouldAllowPublicPolicySetting().
Test Plan
  • Viewed an unlocked task as a logged-out user, saw "login to comment" instead of "locked".
  • Viewed a locked task as a logged-out user, saw "locked".

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

src/applications/policy/capability/PhabricatorPolicyCanInteractCapability.php
14

We can't reasonably specialize this string, but it won't currently ever appear in the UI.

This revision is now accepted and ready to land.Mar 9 2017, 4:48 PM

I had mistakenly believed this block was handling the "log in to comment" case, but it doesn't:

https://secure.phabricator.com/source/phabricator/browse/master/src/applications/transactions/editengine/PhabricatorEditEngine.php;4c7d464f8bf2627ef7455596fde2ae5667e67a57$1565-1570

The block already has a comment describing the case it does handle, so I think this was just carelessness on my part and don't see any technical change we can make to make this more clear.

We could add CAN_INTERACT tests to the test suite to shore this up a little bit, but I think the feature is potentially still in flux so I want to wait to pursue that.

This revision was automatically updated to reflect the committed changes.