Skip to content

Commit

Permalink
Remove deprecated Maniphest "Can Edit <Specific Property>" capabilities
Browse files Browse the repository at this point in the history
Summary:
Depends on D19579. Fixes T10003. These have been deprecated with a setup warning about their impending removal for about two and a half years.

Ref T13164. See PHI642. My overall goal here is to simplify how we handle transactions which have special policy behaviors. In particular, I'm hoping to replace `ApplicationTransactionEditor->requireCapabilities()` with a new, more clear policy check.

A problem with `requireCapabilities()` is that it doesn't actually enforce any policies in almost all cases: the default is "nothing", not CAN_EDIT. So it ends up looking like it's the right place to specialize policy checks, but it usually isn't.

For "Disable", I need to be able to weaken the check selectively (you can disable users if you have the permission, even if you can't edit them otherwise). We have a handful of other edits which work like this (notably, leaving and joining projects) but they're very rare.

Test Plan: Grepped for all removed classes. Edited a Maniphest task.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164, T10003

Differential Revision: https://secure.phabricator.com/D19581
  • Loading branch information
epriestley committed Aug 16, 2018
1 parent f9673a7 commit 296bf04
Show file tree
Hide file tree
Showing 10 changed files with 1 addition and 208 deletions.
10 changes: 0 additions & 10 deletions src/__phutil_library_map__.php
Expand Up @@ -1646,13 +1646,8 @@
'ManiphestDAO' => 'applications/maniphest/storage/ManiphestDAO.php',
'ManiphestDefaultEditCapability' => 'applications/maniphest/capability/ManiphestDefaultEditCapability.php',
'ManiphestDefaultViewCapability' => 'applications/maniphest/capability/ManiphestDefaultViewCapability.php',
'ManiphestEditAssignCapability' => 'applications/maniphest/capability/ManiphestEditAssignCapability.php',
'ManiphestEditConduitAPIMethod' => 'applications/maniphest/conduit/ManiphestEditConduitAPIMethod.php',
'ManiphestEditEngine' => 'applications/maniphest/editor/ManiphestEditEngine.php',
'ManiphestEditPoliciesCapability' => 'applications/maniphest/capability/ManiphestEditPoliciesCapability.php',
'ManiphestEditPriorityCapability' => 'applications/maniphest/capability/ManiphestEditPriorityCapability.php',
'ManiphestEditProjectsCapability' => 'applications/maniphest/capability/ManiphestEditProjectsCapability.php',
'ManiphestEditStatusCapability' => 'applications/maniphest/capability/ManiphestEditStatusCapability.php',
'ManiphestEmailCommand' => 'applications/maniphest/command/ManiphestEmailCommand.php',
'ManiphestGetTaskTransactionsConduitAPIMethod' => 'applications/maniphest/conduit/ManiphestGetTaskTransactionsConduitAPIMethod.php',
'ManiphestHovercardEngineExtension' => 'applications/maniphest/engineextension/ManiphestHovercardEngineExtension.php',
Expand Down Expand Up @@ -7152,13 +7147,8 @@
'ManiphestDAO' => 'PhabricatorLiskDAO',
'ManiphestDefaultEditCapability' => 'PhabricatorPolicyCapability',
'ManiphestDefaultViewCapability' => 'PhabricatorPolicyCapability',
'ManiphestEditAssignCapability' => 'PhabricatorPolicyCapability',
'ManiphestEditConduitAPIMethod' => 'PhabricatorEditEngineAPIMethod',
'ManiphestEditEngine' => 'PhabricatorEditEngine',
'ManiphestEditPoliciesCapability' => 'PhabricatorPolicyCapability',
'ManiphestEditPriorityCapability' => 'PhabricatorPolicyCapability',
'ManiphestEditProjectsCapability' => 'PhabricatorPolicyCapability',
'ManiphestEditStatusCapability' => 'PhabricatorPolicyCapability',
'ManiphestEmailCommand' => 'MetaMTAEmailTransactionCommand',
'ManiphestGetTaskTransactionsConduitAPIMethod' => 'ManiphestConduitAPIMethod',
'ManiphestHovercardEngineExtension' => 'PhabricatorHovercardEngineExtension',
Expand Down
65 changes: 0 additions & 65 deletions src/applications/config/check/PhabricatorExtraConfigSetupCheck.php
Expand Up @@ -361,69 +361,4 @@ public static function getAncientConfig() {
return $ancient_config;
}

private function executeManiphestFieldChecks() {
$maniphest_appclass = 'PhabricatorManiphestApplication';
if (!PhabricatorApplication::isClassInstalled($maniphest_appclass)) {
return;
}

$capabilities = array(
ManiphestEditAssignCapability::CAPABILITY,
ManiphestEditPoliciesCapability::CAPABILITY,
ManiphestEditPriorityCapability::CAPABILITY,
ManiphestEditProjectsCapability::CAPABILITY,
ManiphestEditStatusCapability::CAPABILITY,
);

// Check for any of these capabilities set to anything other than
// "All Users".

$any_set = false;
$app = new PhabricatorManiphestApplication();
foreach ($capabilities as $capability) {
$setting = $app->getPolicy($capability);
if ($setting != PhabricatorPolicies::POLICY_USER) {
$any_set = true;
break;
}
}

if (!$any_set) {
return;
}

$issue_summary = pht(
'Maniphest is currently configured with deprecated policy settings '.
'which will be removed in a future version of Phabricator.');


$message = pht(
'Some policy settings in Maniphest are now deprecated and will be '.
'removed in a future version of Phabricator. You are currently using '.
'at least one of these settings.'.
"\n\n".
'The deprecated settings are "Can Assign Tasks", '.
'"Can Edit Task Policies", "Can Prioritize Tasks", '.
'"Can Edit Task Projects", and "Can Edit Task Status". You can '.
'find these settings in Applications, or follow the link below.'.
"\n\n".
'You can find discussion of this change (including rationale and '.
'recommendations on how to configure similar features) in the upstream, '.
'at the link below.'.
"\n\n".
'To resolve this issue, set all of these policies to "All Users" after '.
'making any necessary form customization changes.');

$more_href = 'https://secure.phabricator.com/T10003';
$edit_href = '/applications/view/PhabricatorManiphestApplication/';

$issue = $this->newIssue('maniphest.T10003-per-field-policies')
->setShortName(pht('Deprecated Policies'))
->setName(pht('Deprecated Maniphest Field Policies'))
->setSummary($issue_summary)
->setMessage($message)
->addLink($more_href, pht('Learn More: Upstream Discussion'))
->addLink($edit_href, pht('Edit These Settings'));
}

}
Expand Up @@ -85,11 +85,6 @@ protected function getCustomCapabilities() {
'template' => ManiphestTaskPHIDType::TYPECONST,
'capability' => PhabricatorPolicyCapability::CAN_EDIT,
),
ManiphestEditStatusCapability::CAPABILITY => array(),
ManiphestEditAssignCapability::CAPABILITY => array(),
ManiphestEditPoliciesCapability::CAPABILITY => array(),
ManiphestEditPriorityCapability::CAPABILITY => array(),
ManiphestEditProjectsCapability::CAPABILITY => array(),
ManiphestBulkEditCapability::CAPABILITY => array(),
);
}
Expand Down

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

45 changes: 0 additions & 45 deletions src/applications/maniphest/editor/ManiphestTransactionEditor.php
Expand Up @@ -279,51 +279,6 @@ protected function buildHeraldAdapter(
->setTask($object);
}

protected function requireCapabilities(
PhabricatorLiskDAO $object,
PhabricatorApplicationTransaction $xaction) {

parent::requireCapabilities($object, $xaction);

$app_capability_map = array(
ManiphestTaskPriorityTransaction::TRANSACTIONTYPE =>
ManiphestEditPriorityCapability::CAPABILITY,
ManiphestTaskStatusTransaction::TRANSACTIONTYPE =>
ManiphestEditStatusCapability::CAPABILITY,
ManiphestTaskOwnerTransaction::TRANSACTIONTYPE =>
ManiphestEditAssignCapability::CAPABILITY,
PhabricatorTransactions::TYPE_EDIT_POLICY =>
ManiphestEditPoliciesCapability::CAPABILITY,
PhabricatorTransactions::TYPE_VIEW_POLICY =>
ManiphestEditPoliciesCapability::CAPABILITY,
);


$transaction_type = $xaction->getTransactionType();

$app_capability = null;
if ($transaction_type == PhabricatorTransactions::TYPE_EDGE) {
switch ($xaction->getMetadataValue('edge:type')) {
case PhabricatorProjectObjectHasProjectEdgeType::EDGECONST:
$app_capability = ManiphestEditProjectsCapability::CAPABILITY;
break;
}
} else {
$app_capability = idx($app_capability_map, $transaction_type);
}

if ($app_capability) {
$app = id(new PhabricatorApplicationQuery())
->setViewer($this->getActor())
->withClasses(array('PhabricatorManiphestApplication'))
->executeOne();
PhabricatorPolicyFilter::requireCapability(
$this->getActor(),
$app,
$app_capability);
}
}

protected function adjustObjectForPolicyChecks(
PhabricatorLiskDAO $object,
array $xactions) {
Expand Down
Expand Up @@ -369,11 +369,7 @@ protected function renderResultList(
$can_edit_priority = false;
$can_bulk_edit = false;
} else {
$can_edit_priority = PhabricatorPolicyFilter::hasCapability(
$viewer,
$this->getApplication(),
ManiphestEditPriorityCapability::CAPABILITY);

$can_edit_priority = true;
$can_bulk_edit = PhabricatorPolicyFilter::hasCapability(
$viewer,
$this->getApplication(),
Expand Down

3 comments on commit 296bf04

@woutwoot
Copy link

@woutwoot woutwoot commented on 296bf04 Aug 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure this commit broke our install.
Call to undefined method PhabricatorExtraConfigSetupCheck::executeManiphestFieldChecks()
See: https://github.com/phacility/phabricator/blob/master/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php#L88

@epriestley
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See https://secure.phabricator.com/D19593, but please report bugs on the Discourse forum (https://discourse.phabricator-community.org), not GitHub.

@woutwoot
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick fix. I know I should. I was lazy... Sorry!

Please sign in to comment.