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

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.

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.

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.)