Page MenuHomePhabricator

Make "Subscribe/Unsubscribe" require only "CAN_VIEW", not "CAN_INTERACT"
ClosedPublic

Authored by epriestley on Feb 14 2019, 9:35 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 20, 6:48 PM
Unknown Object (File)
Fri, Dec 13, 5:56 AM
Unknown Object (File)
Mon, Dec 9, 3:57 PM
Unknown Object (File)
Fri, Dec 6, 5:13 AM
Unknown Object (File)
Tue, Dec 3, 8:06 PM
Unknown Object (File)
Fri, Nov 29, 11:49 PM
Unknown Object (File)
Fri, Nov 29, 10:04 PM
Unknown Object (File)
Fri, Nov 29, 10:03 PM
Subscribers
None

Details

Summary

Ref T13249. See PHI1059. Currently, Subscribe/Unsubscribe require CAN_INTERACT via the web UI and no permissions (i.e., effectively CAN_VIEW) via the API.

Weaken the requirements from the web UI so that you do not need "CAN_INTERACT". This is a product change to the effect that it's okay to subscribe/unsubscribe from anything you can see, even hard-locked tasks. This generally seems reasonable.

Increase the requirements for the actual transaction, which mostly applies to API changes:

  • To remove subscribers other than yourself, require CAN_EDIT.
  • To add subscribers other than yourself, require CAN_EDIT or CAN_INTERACT. You may have CAN_EDIT but not CAN_INTERACT on "soft locked" tasks. It's okay to click "Edit" on these, click "Yes, override lock", then remove subscribers other than yourself.

This technically plugs some weird, mostly theoretical holes in the API where "attackers" could sometimes make more subscription changes than they should have been able to. Now that we send you email when you're unsubscribed this could only really be used to be mildly mischievous, but no harm in making the policy enforcement more correct.

Test Plan

Against normal, soft-locked, and hard-locked tasks: subscribed, unsubscribed, added and removed subscribers, overrode locks, edited via API. Everything worked like it should and I couldn't find any combination of lock state, policy state, and edit pathway that did anything suspicious.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

amckinley added inline comments.
src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
1657

"uesrs"

This revision is now accepted and ready to land.Feb 19 2019, 6:41 PM
This revision was automatically updated to reflect the committed changes.