I think a clean / general way to do this is:

• Don't perform the can view / can edit checks when creating a new object.
• In validateTransaction(), add a check for both TYPE_VIEW_POLICY and TYPE_EDIT_POLICY, which executes only if $xactions is empty and the object is new -- something like this:
  
  if ($is_new) { if (!$xactions) { if (!$object->hasCapability(...)) { $errors[] = new PhabricatorApplicationTransactionValidationError( $type, pht('Invalid'), pht( 'The selected view/edit policy excludes you. Choose a view/edit '. 'policy which allows you to view/edit the object.')); } } }

Then I think our bases are covered:

• When editing an existing object, the current edit policy will be checked in requireCapabilities().
• When editing an existing object, the new edit policy (if you set one) will be checked in validateTransaction().
• When editing an existing object, the current view policy will be checked in validateEditParameters().
• When editing an existing object, the new view policy should be checked in validateTransaction(), although it currently is not (it should look like the similar EDIT block there).
• When creating a new object and not changing a policy, we'll check the default policy in the new block introduced above.
• When creating a new object and changing the policy, we'll check the selected policy instead.

That might not cover everything, but I can't think of reasons why it won't work offhand.

I guess a reason that doesn't work is that it doesn't let "Owner" punch a hole through policies, since the new owner won't be set yet.

Instead of putting the new block in validateTransaction(), we could add it between applyInternalEffects() and applyExternalEffects(), and take another shot at raising a PhabricatorApplicationTransactionValidationException there. That would let us catch any policy issues that don't depend on external changes. I feel like we're getting into dangerous territory if we do it any later, since rollback won't necessarily be effective against applyExternalEffects().

Alternatively, we could make these checks explicit and let the application Editor classes handle special cases. I think that would look something like:

• Implement something like adjustObjectForPolicyChecks(), which returns clone $object by default.
• Maniphest would walk through the transactions and apply any owner changes.
• Then, force policy check that object.
• More complex objects have a reasonable chance to do something magical-but-not-too-gross here?

I kind of favor the last approach, so my recommendation overall is:

• Disable the base view/edit checks for new objects. They aren't meaningful or correct.
• Rely on the PhabricatorPolicyFilter::requireCapabilityWithForcedPolicy() call (or, for new objects with no relevant transactions, a new requireCapability() call) to do all the other validation, but we're going to pass it a fake $object which the Editor can monkey with first.
• The Maniphest editor monkeys with the object by setting the owner.

If that makes zero sense, I can shoot you a skeleton diff for this.

I had some network transience so I saw https://secure.phabricator.com/D8508#8 and then went to work for a a few hours, heh. I assumed that you wanted me to still do ManiphestEditor hyjinx as I have them here, just confined to owner stuff.

I think I get the suggested approach but I think there is a fundamental problem here that I also hit in my other approach.

There is the actor, the old owner, the new owner, the old policy, and the new policy. The editor has the goals of 1) making sure the actor has the proper view / edit privileges relative to policy to begin with and 2) making sure actor doesn't change old => new policy such that they have lost privileges.

Consider the case where the actor is the old owner with a policy of no one, and then they change both the owner to someone else *AND* the policy to (e.g.) users.

If the Maniphest Editor just adjusts the owner from the actor to the new owner (leaving policy as no-one for the purposes of policy checks), the check will fail since the actor is no longer owner and no longer punching through that policy. If both owner and the policy are adjusted, policy is not enforced.

Its almost like we need to check the initial state (1 from above) - isn't this done in the controller via the query method for CAN_EDIT, CAN_VIEW permissions to lead the object to begin with? - then check the "final" proposed state in the editor (2 from above).

This would mean in the adjusting method to adjust both the new owner and new policy before any policy checks, and policy checks would have to become more generic to something like "the view policy you set let's no one see the task; try again."

So I guess I would propose:

• MAYBE - adding a check for can_edit / can_view permissions first thing-ish in applyTransactions; this is unnecessary if we are sure we have query discipline in the controller or we could tackle it by adding such discipline
• basically rock the adjustObjectForPolicyChecks approach, but
  • consolidate any and all policy filtering logic to around lines 599 in the diff as is
  • have the adjustObjectForPolicyChecks base implementation update the object's policy
    • have some hook method that gets handed this and returns it be default. subclasses like Maniphest should do more stuff like set the owner phid as appropos

If the Maniphest Editor just adjusts the owner from the actor to the new owner (leaving policy as no-one for the purposes of policy checks), the check will fail since the actor is no longer owner and no longer punching through that policy. If both owner and the policy are adjusted, policy is not enforced.

The policy checks use requireCapabilityWithForcedPolicy() to check the policy as though the edit had applied, so the check will be with the new owner and with the new policy. I'm not clear on the second half -- it seems like policy is enforced?

adding a check for can_edit / can_view permissions first thing-ish in applyTransactions

We basically already have this, I think? In validateEditParameters() (view check) / requireCapabilities() (soft edit check)?

have the adjustObjectForPolicyChecks base implementation update the object's policy

requireCapabilityWithForcedPolicy() should make this unnecessary (it's magic which pretends we updated the policy).

yah, that was some meat. :D