Page MenuHomePhabricator

Fix join and remove policy checks for Conpherence
ClosedPublic

Authored by epriestley on Apr 15 2017, 3:03 AM.

Details

Summary

I think these got munged when I removed CAN_JOIN.

  • If you can view the room, you can join it.
  • If you can view the room, you can add others to it. This rule adjustment was removed, see discussion on the revision.
  • If you are a participant in the room, you can remove yourself.
  • If you can edit a room, you can remove anyone.
Test Plan

Normal feature set:

  • Create a new room that only I can edit, viewable by all users.
  • Leave room (bye k thx)
  • Create another room, myself only
  • Join room from second account
  • See ability to only remove myself
  • Remove myself
  • Rejoin
  • Add third account
  • Log into first account
  • Boot off randos
  • Test joining by green button, message, and by + sign.

Policy consistency:

  • As a user who can not edit the room, tried to add other members. Received policy exception. The + button is currently visible and enabled for all users (even users who have not joined the room) but this is pre-existing.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

chad created this revision.Apr 15 2017, 3:03 AM
chad added a comment.Apr 15 2017, 3:37 AM

I'm guessing the CAN_VIEW stuff isn't actually needed, since we always check that for every transaction?

chad updated this revision to Diff 42562.Apr 15 2017, 4:36 AM
  • tighter code, comments
epriestley edited edge metadata.Apr 15 2017, 12:47 PM

If you can view the room, you can add others to it.

I'm not sure we should have this rule, at least for now. (I don't think it worked like this before?)

This is approximately the same issue as T4411. The concern is generally stuff like this:

User Alice improperly adds Bailey to room "Security", not realizing he doesn't have permission to see the room, and shouldn't be able see the room (maybe even for serious legal/policy reasons).
User Alice mentions user @bailey in room "Performance Feedback". Bailey definitely should not be notified or added to the room. This change doesn't do this, but it feels very similar to me and would be a small mistake to make.

My current thinking is that we should support this capability in the product, but users should need to explicitly confirm that they want to expose something to someone. That is, the flow would go like this:

Add Particicipants
Add Bailey
Save
New confirmation dialog: Really add @bailey? This user can not currently see this room/task/revision. [ Back ] [ Grant Permission ].
This dialog probably requires "Can Edit" permission? If you don't have "Can Edit", you get "This user can't see the room/task/revision and you don't have permission to edit it. [ Back ] [ Add Other Participants ]"
In the transaction log, make it clear when users force-added CCs/participants in a way that overrides configured policies.
This gets real complicated with object policies like "Room Participants", probably?

Until we can build a flow with guard rails like this, I don't think we should support this capability. I think it's too easy to make a mistake accidentally.

Users who can view something can always copy/paste it into an email to their co-worker anyway, but this is an explicit action which is clearly evades policy rules. When the action is permissible it's better if they go through the application, but I don't want to provide an application pathway that makes the actions "ping user to notify them" and "expand policy scope of private resources" look and feel exactly the same.

Since I want to get the push rolling before too many installs wake up I'm going to commandeer this and implement a reduced-strength version of this rule instead (where "can view" does not imply "can add anyone else"), then run through your test plan, land it, and cherry-pick it. I'm also going to verify the previous behavior and check the UI state of the + button first, since this reduced-strength rule will make + require CAN_EDIT.

I'd like to get test coverage on this too, but if we cover this code as it exists today all the tests will probably break the next time we touch it.

Yeah, the old behavior was: "You need CAN_EDIT to change (add or remove) participants other than yourself."

https://secure.phabricator.com/source/phabricator/browse/master/src/applications/conpherence/editor/ConpherenceEditor.php;03f2a41b16a414dd68a379b9914db993f8e732d4$389-403

I'm going to retain that behavior for now.

The "+" button seems to just always render and never have a disabled state (both before and after these changes) so I'm not going to touch it, but that behavior is slightly wrong: for now, at least, it should be disabled if the user doesn't have CAN_EDIT permission.

This makes policies a bit more open and consistent with how all other subsription policies work.

Particularly, I think this isn't really correct. Subscribing a user to something does not let them view it (see T4411).

The current behavior is bad -- either rejecting the subscription or doing a confirmation and making it work would be better -- but we at least provide a hint about this with the disabled mention style, albeit only in some cases.

I think the most direct analog is projects: you can't add users to a project just because you're a member. If we made Project use this rule (if you're a member, you can add others), anyone in a project like "Access to X" could add anyone else. I think this clearly isn't right: "Access to X" does not imply "Access to control who has access to X".

And instance members on Phacility shouldn't be able to invite whoever they want, and so on.

We can do this rule safely:

Anyone who can view a room can add anyone else if that other person can also already view the room.

...but if we do that, we need to make the UI clear about the "X can't view this room so you can't add them" case.

epriestley commandeered this revision.Apr 15 2017, 1:07 PM
epriestley edited reviewers, added: chad; removed: epriestley.
epriestley edited the summary of this revision. (Show Details)Apr 15 2017, 1:15 PM
epriestley edited the test plan for this revision. (Show Details)
epriestley updated this revision to Diff 42563.Apr 15 2017, 1:16 PM
  • Keep policy rules the same for now.
  • Repeated test plan locally to verify behavior.
Harbormaster completed remote builds in B16496: Diff 42563.

I'm going to land this without review since it's push-blocking and I think the risk of pushing forward here is smaller than the risk of trying to revert D17675, then cherry-pick it and get the deploy underway.

We can also hotfix followups to this change easily (they should only need to go to the web tier) but other changes going out this week need to go everywhere and I'm less thrilled about pushing the db tier during working hours if we can help it.

This revision was automatically updated to reflect the committed changes.
chad edited edge metadata.Apr 15 2017, 1:25 PM

We can do this rule safely:

Anyone who can view a room can add anyone else if that other person can also already view the room.

...but if we do that, we need to make the UI clear about the "X can't view this room so you can't add them" case.

Yes, sorry, that was all I was intending to loosen up to match subscriptions.

Yeah, the way this works is currently pretty inconsistent.

The core difference is that adding users as subscribers to something they can't see doesn't do anything (they still can't see it) but adding them as participants does (it grants them permissions they might not have had before).

So the subscriptions behavior was confusing but harmless, while this could, at least in theory, have been less-harmless in some cases.

(And even this isn't entirely true because the object policy "Subscribers" now exists, so sometimes adding a subscriber does do something.)