From 944539a786796d460de673acc25774e7c82ab1fc Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 23 Feb 2016 16:23:40 -0800 Subject: [PATCH] Simplify locking of Almanac cluster services Summary: Fixes T6741. Ref T10246. Broadly, we want to protect Almanac cluster services: - Today, against users in the Phacility cluster accidentally breaking their own instances. - In the future, against attackers compromising administrative accounts and adding a new "cluster database" which points at hardware they control. The way this works right now is really complicated: there's a global "can create cluster services" setting, and then separate per-service and per-device locks. Instead, change "Can Create Cluster Services" into "Can Manage Cluster Services". Require this permission (in addition to normal permissions) to edit or create any cluster service. This permission can be locked to "No One" via config (as we do in the Phacility cluster) so we only need this one simple setting. There's also zero reason to individually lock //some// of the cluster services. Also improve extended policy errors. The UI here is still a little heavy-handed, but should be good enough for the moment. Test Plan: - Ran migrations. - Verified that cluster services and bindings reported that they belonged to the cluster. - Edited a cluster binding. - Verified that the bound device was marked as a cluster device - Moved a cluster binding, verified the old device was unmarked as a cluster device. - Tried to edit a cluster device as an unprivileged user, got a sensible error. {F1126552} Reviewers: chad Reviewed By: chad Maniphest Tasks: T6741, T10246 Differential Revision: https://secure.phabricator.com/D15339 --- .../autopatches/20160223.almanac.1.bound.sql | 2 + .../20160223.almanac.2.lockbind.sql | 2 + .../20160223.almanac.3.devicelock.sql | 2 + .../20160223.almanac.4.servicelock.sql | 2 + src/__phutil_library_map__.php | 11 ++- .../PhabricatorAlmanacApplication.php | 2 +- ...lmanacManageClusterServicesCapability.php} | 6 +- .../AlmanacBindingViewController.php | 8 ++- .../almanac/controller/AlmanacController.php | 24 ++++++- .../AlmanacDeviceViewController.php | 22 +++--- .../AlmanacServiceEditController.php | 4 +- .../AlmanacServiceViewController.php | 14 ++-- .../almanac/editor/AlmanacBindingEditor.php | 30 ++++++++ .../almanac/editor/AlmanacServiceEditor.php | 25 ------- .../AlmanacManagementLockWorkflow.php | 49 ------------- .../AlmanacManagementUnlockWorkflow.php | 49 ------------- .../query/AlmanacDeviceSearchEngine.php | 4 ++ .../almanac/query/AlmanacServiceQuery.php | 13 ---- .../query/AlmanacServiceSearchEngine.php | 9 --- .../servicetype/AlmanacClusterServiceType.php | 24 ------- .../servicetype/AlmanacServiceType.php | 4 -- .../almanac/storage/AlmanacBinding.php | 30 +++++--- .../almanac/storage/AlmanacDevice.php | 72 +++++++++++-------- .../almanac/storage/AlmanacInterface.php | 7 -- .../almanac/storage/AlmanacService.php | 37 ++++++---- .../policy/filter/PhabricatorPolicyFilter.php | 11 +-- src/docs/user/configuration/cluster.diviner | 19 +---- src/docs/user/userguide/almanac.diviner | 32 --------- 28 files changed, 193 insertions(+), 321 deletions(-) create mode 100644 resources/sql/autopatches/20160223.almanac.1.bound.sql create mode 100644 resources/sql/autopatches/20160223.almanac.2.lockbind.sql create mode 100644 resources/sql/autopatches/20160223.almanac.3.devicelock.sql create mode 100644 resources/sql/autopatches/20160223.almanac.4.servicelock.sql rename src/applications/almanac/capability/{AlmanacCreateClusterServicesCapability.php => AlmanacManageClusterServicesCapability.php} (57%) delete mode 100644 src/applications/almanac/management/AlmanacManagementLockWorkflow.php delete mode 100644 src/applications/almanac/management/AlmanacManagementUnlockWorkflow.php diff --git a/resources/sql/autopatches/20160223.almanac.1.bound.sql b/resources/sql/autopatches/20160223.almanac.1.bound.sql new file mode 100644 index 0000000000..3e30fa458a --- /dev/null +++ b/resources/sql/autopatches/20160223.almanac.1.bound.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_almanac.almanac_device + ADD isBoundToClusterService BOOL NOT NULL; diff --git a/resources/sql/autopatches/20160223.almanac.2.lockbind.sql b/resources/sql/autopatches/20160223.almanac.2.lockbind.sql new file mode 100644 index 0000000000..93f5e8b0b5 --- /dev/null +++ b/resources/sql/autopatches/20160223.almanac.2.lockbind.sql @@ -0,0 +1,2 @@ +UPDATE {$NAMESPACE}_almanac.almanac_device + SET isBoundToClusterService = isLocked; diff --git a/resources/sql/autopatches/20160223.almanac.3.devicelock.sql b/resources/sql/autopatches/20160223.almanac.3.devicelock.sql new file mode 100644 index 0000000000..fdb879fe28 --- /dev/null +++ b/resources/sql/autopatches/20160223.almanac.3.devicelock.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_almanac.almanac_device + DROP isLocked; diff --git a/resources/sql/autopatches/20160223.almanac.4.servicelock.sql b/resources/sql/autopatches/20160223.almanac.4.servicelock.sql new file mode 100644 index 0000000000..e716054c0c --- /dev/null +++ b/resources/sql/autopatches/20160223.almanac.4.servicelock.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_almanac.almanac_service + DROP isLocked; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index a4b133893b..8ea70ec452 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -27,7 +27,6 @@ phutil_register_library_map(array( 'AlmanacConduitAPIMethod' => 'applications/almanac/conduit/AlmanacConduitAPIMethod.php', 'AlmanacConsoleController' => 'applications/almanac/controller/AlmanacConsoleController.php', 'AlmanacController' => 'applications/almanac/controller/AlmanacController.php', - 'AlmanacCreateClusterServicesCapability' => 'applications/almanac/capability/AlmanacCreateClusterServicesCapability.php', 'AlmanacCreateDevicesCapability' => 'applications/almanac/capability/AlmanacCreateDevicesCapability.php', 'AlmanacCreateNamespacesCapability' => 'applications/almanac/capability/AlmanacCreateNamespacesCapability.php', 'AlmanacCreateNetworksCapability' => 'applications/almanac/capability/AlmanacCreateNetworksCapability.php', @@ -57,10 +56,9 @@ phutil_register_library_map(array( 'AlmanacInterfaceQuery' => 'applications/almanac/query/AlmanacInterfaceQuery.php', 'AlmanacInterfaceTableView' => 'applications/almanac/view/AlmanacInterfaceTableView.php', 'AlmanacKeys' => 'applications/almanac/util/AlmanacKeys.php', - 'AlmanacManagementLockWorkflow' => 'applications/almanac/management/AlmanacManagementLockWorkflow.php', + 'AlmanacManageClusterServicesCapability' => 'applications/almanac/capability/AlmanacManageClusterServicesCapability.php', 'AlmanacManagementRegisterWorkflow' => 'applications/almanac/management/AlmanacManagementRegisterWorkflow.php', 'AlmanacManagementTrustKeyWorkflow' => 'applications/almanac/management/AlmanacManagementTrustKeyWorkflow.php', - 'AlmanacManagementUnlockWorkflow' => 'applications/almanac/management/AlmanacManagementUnlockWorkflow.php', 'AlmanacManagementUntrustKeyWorkflow' => 'applications/almanac/management/AlmanacManagementUntrustKeyWorkflow.php', 'AlmanacManagementWorkflow' => 'applications/almanac/management/AlmanacManagementWorkflow.php', 'AlmanacNames' => 'applications/almanac/util/AlmanacNames.php', @@ -3996,6 +3994,7 @@ phutil_register_library_map(array( 'PhabricatorApplicationTransactionInterface', 'AlmanacPropertyInterface', 'PhabricatorDestructibleInterface', + 'PhabricatorExtendedPolicyInterface', ), 'AlmanacBindingEditController' => 'AlmanacServiceController', 'AlmanacBindingEditor' => 'AlmanacEditor', @@ -4013,7 +4012,6 @@ phutil_register_library_map(array( 'AlmanacConduitAPIMethod' => 'ConduitAPIMethod', 'AlmanacConsoleController' => 'AlmanacController', 'AlmanacController' => 'PhabricatorController', - 'AlmanacCreateClusterServicesCapability' => 'PhabricatorPolicyCapability', 'AlmanacCreateDevicesCapability' => 'PhabricatorPolicyCapability', 'AlmanacCreateNamespacesCapability' => 'PhabricatorPolicyCapability', 'AlmanacCreateNetworksCapability' => 'PhabricatorPolicyCapability', @@ -4030,6 +4028,7 @@ phutil_register_library_map(array( 'PhabricatorDestructibleInterface', 'PhabricatorNgramsInterface', 'PhabricatorConduitResultInterface', + 'PhabricatorExtendedPolicyInterface', ), 'AlmanacDeviceController' => 'AlmanacController', 'AlmanacDeviceEditController' => 'AlmanacDeviceController', @@ -4057,10 +4056,9 @@ phutil_register_library_map(array( 'AlmanacInterfaceQuery' => 'AlmanacQuery', 'AlmanacInterfaceTableView' => 'AphrontView', 'AlmanacKeys' => 'Phobject', - 'AlmanacManagementLockWorkflow' => 'AlmanacManagementWorkflow', + 'AlmanacManageClusterServicesCapability' => 'PhabricatorPolicyCapability', 'AlmanacManagementRegisterWorkflow' => 'AlmanacManagementWorkflow', 'AlmanacManagementTrustKeyWorkflow' => 'AlmanacManagementWorkflow', - 'AlmanacManagementUnlockWorkflow' => 'AlmanacManagementWorkflow', 'AlmanacManagementUntrustKeyWorkflow' => 'AlmanacManagementWorkflow', 'AlmanacManagementWorkflow' => 'PhabricatorManagementWorkflow', 'AlmanacNames' => 'Phobject', @@ -4130,6 +4128,7 @@ phutil_register_library_map(array( 'PhabricatorDestructibleInterface', 'PhabricatorNgramsInterface', 'PhabricatorConduitResultInterface', + 'PhabricatorExtendedPolicyInterface', ), 'AlmanacServiceController' => 'AlmanacController', 'AlmanacServiceDatasource' => 'PhabricatorTypeaheadDatasource', diff --git a/src/applications/almanac/application/PhabricatorAlmanacApplication.php b/src/applications/almanac/application/PhabricatorAlmanacApplication.php index 2e413b9fee..d0c543e9e1 100644 --- a/src/applications/almanac/application/PhabricatorAlmanacApplication.php +++ b/src/applications/almanac/application/PhabricatorAlmanacApplication.php @@ -93,7 +93,7 @@ final class PhabricatorAlmanacApplication extends PhabricatorApplication { AlmanacCreateNamespacesCapability::CAPABILITY => array( 'default' => PhabricatorPolicies::POLICY_ADMIN, ), - AlmanacCreateClusterServicesCapability::CAPABILITY => array( + AlmanacManageClusterServicesCapability::CAPABILITY => array( 'default' => PhabricatorPolicies::POLICY_ADMIN, ), ); diff --git a/src/applications/almanac/capability/AlmanacCreateClusterServicesCapability.php b/src/applications/almanac/capability/AlmanacManageClusterServicesCapability.php similarity index 57% rename from src/applications/almanac/capability/AlmanacCreateClusterServicesCapability.php rename to src/applications/almanac/capability/AlmanacManageClusterServicesCapability.php index e0b1a8fb22..35d701b592 100644 --- a/src/applications/almanac/capability/AlmanacCreateClusterServicesCapability.php +++ b/src/applications/almanac/capability/AlmanacManageClusterServicesCapability.php @@ -1,17 +1,17 @@ setHeader($header) ->addPropertyList($property_list); - if ($binding->getService()->getIsLocked()) { - $this->addLockMessage( + if ($binding->getService()->isClusterService()) { + $this->addClusterMessage( $box, + pht('The service for this binding is a cluster service.'), pht( - 'This service for this binding is locked, so the binding can '. + 'The service for this binding is a cluster service. You do not '. + 'have permission to manage cluster services, so this binding can '. 'not be edited.')); } diff --git a/src/applications/almanac/controller/AlmanacController.php b/src/applications/almanac/controller/AlmanacController.php index 9673a0ef4a..369da4a4e7 100644 --- a/src/applications/almanac/controller/AlmanacController.php +++ b/src/applications/almanac/controller/AlmanacController.php @@ -166,7 +166,14 @@ abstract class AlmanacController ->setTable($table); } - protected function addLockMessage(PHUIObjectBoxView $box, $message) { + protected function addClusterMessage( + PHUIObjectBoxView $box, + $positive, + $negative) { + + $can_manage = $this->hasApplicationCapability( + AlmanacManageClusterServicesCapability::CAPABILITY); + $doc_link = phutil_tag( 'a', array( @@ -175,11 +182,22 @@ abstract class AlmanacController ), pht('Learn More')); + if ($can_manage) { + $severity = PHUIInfoView::SEVERITY_NOTICE; + $message = $positive; + } else { + $severity = PHUIInfoView::SEVERITY_WARNING; + $message = $negative; + } + + $icon = id(new PHUIIconView()) + ->setIcon('fa-sitemap'); + $error_view = id(new PHUIInfoView()) - ->setSeverity(PHUIInfoView::SEVERITY_WARNING) + ->setSeverity($severity) ->setErrors( array( - array($message, ' ', $doc_link), + array($icon, ' ', $message, ' ', $doc_link), )); $box->setInfoView($error_view); diff --git a/src/applications/almanac/controller/AlmanacDeviceViewController.php b/src/applications/almanac/controller/AlmanacDeviceViewController.php index 89ec7198e3..4836198763 100644 --- a/src/applications/almanac/controller/AlmanacDeviceViewController.php +++ b/src/applications/almanac/controller/AlmanacDeviceViewController.php @@ -21,10 +21,6 @@ final class AlmanacDeviceViewController return new Aphront404Response(); } - // We rebuild locks on a device when viewing the detail page, so they - // automatically get corrected if they fall out of sync. - $device->rebuildDeviceLocks(); - $title = pht('Device %s', $device->getName()); $property_list = $this->buildPropertyList($device); @@ -40,12 +36,14 @@ final class AlmanacDeviceViewController ->setHeader($header) ->addPropertyList($property_list); - if ($device->getIsLocked()) { - $this->addLockMessage( + if ($device->isClusterDevice()) { + $this->addClusterMessage( $box, + pht('This device is bound to a cluster service.'), pht( - 'This device is bound to a locked service, so it can not be '. - 'edited.')); + 'This device is bound to a cluster service. You do not have '. + 'permission to manage cluster services, so the device can not '. + 'be edited.')); } $interfaces = $this->buildInterfaceList($device); @@ -219,14 +217,14 @@ final class AlmanacDeviceViewController $handles = $viewer->loadHandles(mpull($services, 'getPHID')); - $icon_lock = id(new PHUIIconView()) - ->setIcon('fa-lock'); + $icon_cluster = id(new PHUIIconView()) + ->setIcon('fa-sitemap'); $rows = array(); foreach ($services as $service) { $rows[] = array( - ($service->getIsLocked() - ? $icon_lock + ($service->isClusterService() + ? $icon_cluster : null), $handles->renderHandle($service->getPHID()), ); diff --git a/src/applications/almanac/controller/AlmanacServiceEditController.php b/src/applications/almanac/controller/AlmanacServiceEditController.php index 20a7528704..9a0d8d5d17 100644 --- a/src/applications/almanac/controller/AlmanacServiceEditController.php +++ b/src/applications/almanac/controller/AlmanacServiceEditController.php @@ -43,7 +43,7 @@ final class AlmanacServiceEditController $service_type = $service_types[$service_class]; if ($service_type->isClusterServiceType()) { $this->requireApplicationCapability( - AlmanacCreateClusterServicesCapability::CAPABILITY); + AlmanacManageClusterServicesCapability::CAPABILITY); } $service = AlmanacService::initializeNewService(); @@ -190,7 +190,7 @@ final class AlmanacServiceEditController } list($can_cluster, $cluster_link) = $this->explainApplicationCapability( - AlmanacCreateClusterServicesCapability::CAPABILITY, + AlmanacManageClusterServicesCapability::CAPABILITY, pht('You have permission to create cluster services.'), pht('You do not have permission to create new cluster services.')); diff --git a/src/applications/almanac/controller/AlmanacServiceViewController.php b/src/applications/almanac/controller/AlmanacServiceViewController.php index debc1bc063..735da73bcf 100644 --- a/src/applications/almanac/controller/AlmanacServiceViewController.php +++ b/src/applications/almanac/controller/AlmanacServiceViewController.php @@ -36,15 +36,13 @@ final class AlmanacServiceViewController ->setHeader($header) ->addPropertyList($property_list); - $messages = $service->getServiceType()->getStatusMessages($service); - if ($messages) { - $box->setFormErrors($messages); - } - - if ($service->getIsLocked()) { - $this->addLockMessage( + if ($service->isClusterService()) { + $this->addClusterMessage( $box, - pht('This service is locked, and can not be edited.')); + pht('This is a cluster service.'), + pht( + 'This service is a cluster service. You do not have permission to '. + 'edit cluster services, so you can not edit this service.')); } $bindings = $this->buildBindingList($service); diff --git a/src/applications/almanac/editor/AlmanacBindingEditor.php b/src/applications/almanac/editor/AlmanacBindingEditor.php index 6eea189cdd..86646c132f 100644 --- a/src/applications/almanac/editor/AlmanacBindingEditor.php +++ b/src/applications/almanac/editor/AlmanacBindingEditor.php @@ -3,6 +3,8 @@ final class AlmanacBindingEditor extends AlmanacEditor { + private $devicePHID; + public function getEditorObjectsDescription() { return pht('Almanac Binding'); } @@ -62,6 +64,34 @@ final class AlmanacBindingEditor switch ($xaction->getTransactionType()) { case AlmanacBindingTransaction::TYPE_INTERFACE: + $interface_phids = array(); + + $interface_phids[] = $xaction->getOldValue(); + $interface_phids[] = $xaction->getNewValue(); + + $interface_phids = array_filter($interface_phids); + $interface_phids = array_unique($interface_phids); + + $interfaces = id(new AlmanacInterfaceQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withPHIDs($interface_phids) + ->execute(); + + $device_phids = array(); + foreach ($interfaces as $interface) { + $device_phids[] = $interface->getDevicePHID(); + } + + $device_phids = array_unique($device_phids); + + $devices = id(new AlmanacDeviceQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withPHIDs($device_phids) + ->execute(); + + foreach ($devices as $device) { + $device->rebuildClusterBindingStatus(); + } return; } diff --git a/src/applications/almanac/editor/AlmanacServiceEditor.php b/src/applications/almanac/editor/AlmanacServiceEditor.php index 8b8d2d2d67..1f6fe09cfa 100644 --- a/src/applications/almanac/editor/AlmanacServiceEditor.php +++ b/src/applications/almanac/editor/AlmanacServiceEditor.php @@ -11,7 +11,6 @@ final class AlmanacServiceEditor $types = parent::getTransactionTypes(); $types[] = AlmanacServiceTransaction::TYPE_NAME; - $types[] = AlmanacServiceTransaction::TYPE_LOCK; $types[] = PhabricatorTransactions::TYPE_VIEW_POLICY; $types[] = PhabricatorTransactions::TYPE_EDIT_POLICY; @@ -25,8 +24,6 @@ final class AlmanacServiceEditor switch ($xaction->getTransactionType()) { case AlmanacServiceTransaction::TYPE_NAME: return $object->getName(); - case AlmanacServiceTransaction::TYPE_LOCK: - return (bool)$object->getIsLocked(); } return parent::getCustomTransactionOldValue($object, $xaction); @@ -39,8 +36,6 @@ final class AlmanacServiceEditor switch ($xaction->getTransactionType()) { case AlmanacServiceTransaction::TYPE_NAME: return $xaction->getNewValue(); - case AlmanacServiceTransaction::TYPE_LOCK: - return (bool)$xaction->getNewValue(); } return parent::getCustomTransactionNewValue($object, $xaction); @@ -54,9 +49,6 @@ final class AlmanacServiceEditor case AlmanacServiceTransaction::TYPE_NAME: $object->setName($xaction->getNewValue()); return; - case AlmanacServiceTransaction::TYPE_LOCK: - $object->setIsLocked((int)$xaction->getNewValue()); - return; } return parent::applyCustomInternalTransaction($object, $xaction); @@ -69,23 +61,6 @@ final class AlmanacServiceEditor switch ($xaction->getTransactionType()) { case AlmanacServiceTransaction::TYPE_NAME: return; - case AlmanacServiceTransaction::TYPE_LOCK: - $service = id(new AlmanacServiceQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withPHIDs(array($object->getPHID())) - ->needBindings(true) - ->executeOne(); - - $devices = array(); - foreach ($service->getBindings() as $binding) { - $device = $binding->getInterface()->getDevice(); - $devices[$device->getPHID()] = $device; - } - - foreach ($devices as $device) { - $device->rebuildDeviceLocks(); - } - return; } return parent::applyCustomExternalTransaction($object, $xaction); diff --git a/src/applications/almanac/management/AlmanacManagementLockWorkflow.php b/src/applications/almanac/management/AlmanacManagementLockWorkflow.php deleted file mode 100644 index 60783d1bc5..0000000000 --- a/src/applications/almanac/management/AlmanacManagementLockWorkflow.php +++ /dev/null @@ -1,49 +0,0 @@ -setName('lock') - ->setSynopsis(pht('Lock a service to prevent it from being edited.')) - ->setArguments( - array( - array( - 'name' => 'services', - 'wildcard' => true, - ), - )); - } - - public function execute(PhutilArgumentParser $args) { - $console = PhutilConsole::getConsole(); - - $services = $this->loadServices($args->getArg('services')); - if (!$services) { - throw new PhutilArgumentUsageException( - pht('Specify at least one service to lock.')); - } - - foreach ($services as $service) { - if ($service->getIsLocked()) { - throw new PhutilArgumentUsageException( - pht( - 'Service "%s" is already locked!', - $service->getName())); - } - } - - foreach ($services as $service) { - $this->updateServiceLock($service, true); - - $console->writeOut( - "** %s ** %s\n", - pht('LOCKED'), - pht('Service "%s" was locked.', $service->getName())); - } - - return 0; - } - -} diff --git a/src/applications/almanac/management/AlmanacManagementUnlockWorkflow.php b/src/applications/almanac/management/AlmanacManagementUnlockWorkflow.php deleted file mode 100644 index da8ba3dcc7..0000000000 --- a/src/applications/almanac/management/AlmanacManagementUnlockWorkflow.php +++ /dev/null @@ -1,49 +0,0 @@ -setName('unlock') - ->setSynopsis(pht('Unlock a service to allow it to be edited.')) - ->setArguments( - array( - array( - 'name' => 'services', - 'wildcard' => true, - ), - )); - } - - public function execute(PhutilArgumentParser $args) { - $console = PhutilConsole::getConsole(); - - $services = $this->loadServices($args->getArg('services')); - if (!$services) { - throw new PhutilArgumentUsageException( - pht('Specify at least one service to unlock.')); - } - - foreach ($services as $service) { - if (!$service->getIsLocked()) { - throw new PhutilArgumentUsageException( - pht( - 'Service "%s" is not locked!', - $service->getName())); - } - } - - foreach ($services as $service) { - $this->updateServiceLock($service, false); - - $console->writeOut( - "** %s ** %s\n", - pht('UNLOCKED'), - pht('Service "%s" was unlocked.', $service->getName())); - } - - return 0; - } - -} diff --git a/src/applications/almanac/query/AlmanacDeviceSearchEngine.php b/src/applications/almanac/query/AlmanacDeviceSearchEngine.php index a85c38e188..6e28d7e5b8 100644 --- a/src/applications/almanac/query/AlmanacDeviceSearchEngine.php +++ b/src/applications/almanac/query/AlmanacDeviceSearchEngine.php @@ -84,6 +84,10 @@ final class AlmanacDeviceSearchEngine ->setHref($device->getURI()) ->setObject($device); + if ($device->isClusterDevice()) { + $item->addIcon('fa-sitemap', pht('Cluster Device')); + } + $list->addItem($item); } diff --git a/src/applications/almanac/query/AlmanacServiceQuery.php b/src/applications/almanac/query/AlmanacServiceQuery.php index 2cfb7edb8f..e45cfec04b 100644 --- a/src/applications/almanac/query/AlmanacServiceQuery.php +++ b/src/applications/almanac/query/AlmanacServiceQuery.php @@ -8,7 +8,6 @@ final class AlmanacServiceQuery private $names; private $serviceClasses; private $devicePHIDs; - private $locked; private $namePrefix; private $nameSuffix; @@ -39,11 +38,6 @@ final class AlmanacServiceQuery return $this; } - public function withLocked($locked) { - $this->locked = $locked; - return $this; - } - public function withNamePrefix($prefix) { $this->namePrefix = $prefix; return $this; @@ -129,13 +123,6 @@ final class AlmanacServiceQuery $this->devicePHIDs); } - if ($this->locked !== null) { - $where[] = qsprintf( - $conn, - 'service.isLocked = %d', - (int)$this->locked); - } - if ($this->namePrefix !== null) { $where[] = qsprintf( $conn, diff --git a/src/applications/almanac/query/AlmanacServiceSearchEngine.php b/src/applications/almanac/query/AlmanacServiceSearchEngine.php index 4011d4fbc5..1a15e17d46 100644 --- a/src/applications/almanac/query/AlmanacServiceSearchEngine.php +++ b/src/applications/almanac/query/AlmanacServiceSearchEngine.php @@ -101,15 +101,6 @@ final class AlmanacServiceSearchEngine $service->getServiceType()->getServiceTypeIcon(), $service->getServiceType()->getServiceTypeShortName()); - if ($service->getIsLocked() || - $service->getServiceType()->isClusterServiceType()) { - if ($service->getIsLocked()) { - $item->addIcon('fa-lock', pht('Locked')); - } else { - $item->addIcon('fa-unlock-alt red', pht('Unlocked')); - } - } - $list->addItem($item); } diff --git a/src/applications/almanac/servicetype/AlmanacClusterServiceType.php b/src/applications/almanac/servicetype/AlmanacClusterServiceType.php index 7edbe52a73..2873141185 100644 --- a/src/applications/almanac/servicetype/AlmanacClusterServiceType.php +++ b/src/applications/almanac/servicetype/AlmanacClusterServiceType.php @@ -11,28 +11,4 @@ abstract class AlmanacClusterServiceType return 'fa-sitemap'; } - public function getStatusMessages(AlmanacService $service) { - $messages = parent::getStatusMessages($service); - - if (!$service->getIsLocked()) { - $doc_href = PhabricatorEnv::getDoclink( - 'User Guide: Phabricator Clusters'); - - $doc_link = phutil_tag( - 'a', - array( - 'href' => $doc_href, - 'target' => '_blank', - ), - pht('Learn More')); - - $messages[] = pht( - 'This is an unlocked cluster service. After you finish editing '. - 'it, you should lock it. %s.', - $doc_link); - } - - return $messages; - } - } diff --git a/src/applications/almanac/servicetype/AlmanacServiceType.php b/src/applications/almanac/servicetype/AlmanacServiceType.php index 408224f1a9..043da7d52e 100644 --- a/src/applications/almanac/servicetype/AlmanacServiceType.php +++ b/src/applications/almanac/servicetype/AlmanacServiceType.php @@ -55,10 +55,6 @@ abstract class AlmanacServiceType extends Phobject { return array(); } - public function getStatusMessages(AlmanacService $service) { - return array(); - } - /** * List all available service type implementations. * diff --git a/src/applications/almanac/storage/AlmanacBinding.php b/src/applications/almanac/storage/AlmanacBinding.php index 0d9cfa6fa3..030a3c7c57 100644 --- a/src/applications/almanac/storage/AlmanacBinding.php +++ b/src/applications/almanac/storage/AlmanacBinding.php @@ -6,7 +6,8 @@ final class AlmanacBinding PhabricatorPolicyInterface, PhabricatorApplicationTransactionInterface, AlmanacPropertyInterface, - PhabricatorDestructibleInterface { + PhabricatorDestructibleInterface, + PhabricatorExtendedPolicyInterface { protected $servicePHID; protected $devicePHID; @@ -157,17 +158,30 @@ final class AlmanacBinding 'interface.'), ); - if ($capability === PhabricatorPolicyCapability::CAN_EDIT) { - if ($this->getService()->getIsLocked()) { - $notes[] = pht( - 'The service for this binding is locked, so it can not be edited.'); - } - } - return $notes; } +/* -( PhabricatorExtendedPolicyInterface )--------------------------------- */ + + + public function getExtendedPolicy($capability, PhabricatorUser $viewer) { + switch ($capability) { + case PhabricatorPolicyCapability::CAN_EDIT: + if ($this->getService()->isClusterService()) { + return array( + array( + new PhabricatorAlmanacApplication(), + AlmanacManageClusterServicesCapability::CAPABILITY, + ), + ); + } + break; + } + + return array(); + } + /* -( PhabricatorApplicationTransactionInterface )------------------------- */ diff --git a/src/applications/almanac/storage/AlmanacDevice.php b/src/applications/almanac/storage/AlmanacDevice.php index 7ef17bbc2b..f46b03600f 100644 --- a/src/applications/almanac/storage/AlmanacDevice.php +++ b/src/applications/almanac/storage/AlmanacDevice.php @@ -10,14 +10,15 @@ final class AlmanacDevice AlmanacPropertyInterface, PhabricatorDestructibleInterface, PhabricatorNgramsInterface, - PhabricatorConduitResultInterface { + PhabricatorConduitResultInterface, + PhabricatorExtendedPolicyInterface { protected $name; protected $nameIndex; protected $mailKey; protected $viewPolicy; protected $editPolicy; - protected $isLocked; + protected $isBoundToClusterService; private $almanacProperties = self::ATTACHABLE; @@ -26,7 +27,7 @@ final class AlmanacDevice ->setViewPolicy(PhabricatorPolicies::POLICY_USER) ->setEditPolicy(PhabricatorPolicies::POLICY_ADMIN) ->attachAlmanacProperties(array()) - ->setIsLocked(0); + ->setIsBoundToClusterService(0); } protected function getConfiguration() { @@ -36,7 +37,7 @@ final class AlmanacDevice 'name' => 'text128', 'nameIndex' => 'bytes12', 'mailKey' => 'bytes20', - 'isLocked' => 'bool', + 'isBoundToClusterService' => 'bool', ), self::CONFIG_KEY_SCHEMA => array( 'key_name' => array( @@ -70,30 +71,28 @@ final class AlmanacDevice return '/almanac/device/view/'.$this->getName().'/'; } - - /** - * Find locked services which are bound to this device, updating the device - * lock flag if necessary. - * - * @return list List of locking service PHIDs. - */ - public function rebuildDeviceLocks() { + public function rebuildClusterBindingStatus() { $services = id(new AlmanacServiceQuery()) ->setViewer(PhabricatorUser::getOmnipotentUser()) ->withDevicePHIDs(array($this->getPHID())) - ->withLocked(true) ->execute(); - $locked = (bool)count($services); + $is_cluster = false; + foreach ($services as $service) { + if ($service->isClusterService()) { + $is_cluster = true; + break; + } + } - if ($locked != $this->getIsLocked()) { - $this->setIsLocked((int)$locked); + if ($is_cluster != $this->getIsBoundToClusterService()) { + $this->setIsBoundToClusterService((int)$is_cluster); $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); queryfx( $this->establishConnection('w'), - 'UPDATE %T SET isLocked = %d WHERE id = %d', + 'UPDATE %T SET isBoundToClusterService = %d WHERE id = %d', $this->getTableName(), - $this->getIsLocked(), + $this->getIsBoundToClusterService(), $this->getID()); unset($unguarded); } @@ -101,6 +100,10 @@ final class AlmanacDevice return $this; } + public function isClusterDevice() { + return $this->getIsBoundToClusterService(); + } + /* -( AlmanacPropertyInterface )------------------------------------------- */ @@ -156,11 +159,7 @@ final class AlmanacDevice case PhabricatorPolicyCapability::CAN_VIEW: return $this->getViewPolicy(); case PhabricatorPolicyCapability::CAN_EDIT: - if ($this->getIsLocked()) { - return PhabricatorPolicies::POLICY_NOONE; - } else { - return $this->getEditPolicy(); - } + return $this->getEditPolicy(); } } @@ -169,15 +168,28 @@ final class AlmanacDevice } public function describeAutomaticCapability($capability) { - if ($capability === PhabricatorPolicyCapability::CAN_EDIT) { - if ($this->getIsLocked()) { - return pht( - 'This device is bound to a locked service, so it can not '. - 'be edited.'); - } + return null; + } + + +/* -( PhabricatorExtendedPolicyInterface )--------------------------------- */ + + + public function getExtendedPolicy($capability, PhabricatorUser $viewer) { + switch ($capability) { + case PhabricatorPolicyCapability::CAN_EDIT: + if ($this->isClusterDevice()) { + return array( + array( + new PhabricatorAlmanacApplication(), + AlmanacManageClusterServicesCapability::CAPABILITY, + ), + ); + } + break; } - return null; + return array(); } diff --git a/src/applications/almanac/storage/AlmanacInterface.php b/src/applications/almanac/storage/AlmanacInterface.php index 729e95aa9a..d1bd4f0aa4 100644 --- a/src/applications/almanac/storage/AlmanacInterface.php +++ b/src/applications/almanac/storage/AlmanacInterface.php @@ -101,13 +101,6 @@ final class AlmanacInterface 'view the interface.'), ); - if ($capability === PhabricatorPolicyCapability::CAN_EDIT) { - if ($this->getDevice()->getIsLocked()) { - $notes[] = pht( - 'The device for this interface is locked, so it can not be edited.'); - } - } - return $notes; } diff --git a/src/applications/almanac/storage/AlmanacService.php b/src/applications/almanac/storage/AlmanacService.php index 48eee1700b..92a5551188 100644 --- a/src/applications/almanac/storage/AlmanacService.php +++ b/src/applications/almanac/storage/AlmanacService.php @@ -9,7 +9,8 @@ final class AlmanacService AlmanacPropertyInterface, PhabricatorDestructibleInterface, PhabricatorNgramsInterface, - PhabricatorConduitResultInterface { + PhabricatorConduitResultInterface, + PhabricatorExtendedPolicyInterface { protected $name; protected $nameIndex; @@ -17,7 +18,6 @@ final class AlmanacService protected $viewPolicy; protected $editPolicy; protected $serviceClass; - protected $isLocked; private $almanacProperties = self::ATTACHABLE; private $bindings = self::ATTACHABLE; @@ -27,8 +27,7 @@ final class AlmanacService return id(new AlmanacService()) ->setViewPolicy(PhabricatorPolicies::POLICY_USER) ->setEditPolicy(PhabricatorPolicies::POLICY_ADMIN) - ->attachAlmanacProperties(array()) - ->setIsLocked(0); + ->attachAlmanacProperties(array()); } protected function getConfiguration() { @@ -39,7 +38,6 @@ final class AlmanacService 'nameIndex' => 'bytes12', 'mailKey' => 'bytes20', 'serviceClass' => 'text64', - 'isLocked' => 'bool', ), self::CONFIG_KEY_SCHEMA => array( 'key_name' => array( @@ -94,6 +92,10 @@ final class AlmanacService return $this; } + public function isClusterService() { + return $this->getServiceType()->isClusterServiceType(); + } + /* -( AlmanacPropertyInterface )------------------------------------------- */ @@ -149,11 +151,7 @@ final class AlmanacService case PhabricatorPolicyCapability::CAN_VIEW: return $this->getViewPolicy(); case PhabricatorPolicyCapability::CAN_EDIT: - if ($this->getIsLocked()) { - return PhabricatorPolicies::POLICY_NOONE; - } else { - return $this->getEditPolicy(); - } + return $this->getEditPolicy(); } } @@ -162,15 +160,28 @@ final class AlmanacService } public function describeAutomaticCapability($capability) { + return null; + } + + +/* -( PhabricatorExtendedPolicyInterface )--------------------------------- */ + + + public function getExtendedPolicy($capability, PhabricatorUser $viewer) { switch ($capability) { case PhabricatorPolicyCapability::CAN_EDIT: - if ($this->getIsLocked()) { - return pht('This service is locked and can not be edited.'); + if ($this->isClusterService()) { + return array( + array( + new PhabricatorAlmanacApplication(), + AlmanacManageClusterServicesCapability::CAPABILITY, + ), + ); } break; } - return null; + return array(); } diff --git a/src/applications/policy/filter/PhabricatorPolicyFilter.php b/src/applications/policy/filter/PhabricatorPolicyFilter.php index f70b77f99a..c3ac7fc016 100644 --- a/src/applications/policy/filter/PhabricatorPolicyFilter.php +++ b/src/applications/policy/filter/PhabricatorPolicyFilter.php @@ -381,10 +381,13 @@ final class PhabricatorPolicyFilter extends Phobject { $reject = $extended_objects[$extended_key]; unset($extended_objects[$extended_key]); - // TODO: This isn't as user-friendly as it could be. It's possible - // that we're rejecting this object for multiple capability/policy - // failures, though. - $this->rejectObject($reject, false, ''); + // It's possible that we're rejecting this object for multiple + // capability/policy failures, but just pick the first one to show + // to the user. + $first_capability = head($capabilities); + $first_policy = $object_in->getPolicy($first_capability); + + $this->rejectObject($reject, $first_policy, $first_capability); } } } diff --git a/src/docs/user/configuration/cluster.diviner b/src/docs/user/configuration/cluster.diviner index 708376ec23..160c42c8bc 100644 --- a/src/docs/user/configuration/cluster.diviner +++ b/src/docs/user/configuration/cluster.diviner @@ -12,20 +12,7 @@ yet. This document is mostly a placeholder. Locking Services ================ -Because cluster configuration is defined in Phabricator itself, an attacker -who compromises an account that can edit the cluster definition has significant -power. For example, the attacker might be able to configure Phabricator to -replicate the database to a server they control. +Very briefly, you can set "Can Manage Cluster Services" to "No One" to lock +the cluster definition. -To mitigate this attack, services in Almanac can be locked to prevent them -from being edited from the web UI. An attacker would then need significantly -greater access (to the CLI, or directly to the database) in order to change -the cluster configuration. - -You should normally keep cluster services in a locked state, and unlock them -only to edit them. Once you're finished making changes, lock the service again. -The web UI will warn you when you're viewing an unlocked cluster service, as -a reminder that you should lock it again once you're finished editing. - -For details on how to lock and unlock a service, see -@{article:Almanac User Guide}. +See also @{article:Almanac User Guide}. diff --git a/src/docs/user/userguide/almanac.diviner b/src/docs/user/userguide/almanac.diviner index d84176c57d..5a820c6ae2 100644 --- a/src/docs/user/userguide/almanac.diviner +++ b/src/docs/user/userguide/almanac.diviner @@ -177,35 +177,3 @@ Users can edit services and devices within a namespace if they have edit permission on the service or device itself, as long as they don't try to rename the service or device to move it into a namespace they don't have permission to access. - - -Locking and Unlocking Services -============================== - -Services can be locked to prevent edits from the web UI. This primarily hardens -Almanac against attacks involving account compromise. Notably, locking cluster -services prevents an attacker from modifying the Phabricator cluster definition. -For more details on this scenario, see -@{article:User Guide: Phabricator Clusters}. - -Beyond hardening cluster definitions, you might also want to lock a critical -service to prevent accidental edits. - -To lock a service, run: - - phabricator/ $ ./bin/almanac lock - -To unlock a service later, run: - - phabricator/ $ ./bin/almanac unlock - -Locking a service also locks all of the service's bindings and properties, as -well as the devices connected to the service. Generally, no part of the -service definition can be modified while it is locked. - -Devices (and their properties) will remain locked as long as they are bound to -at least one locked service. To edit a device, you'll need to unlock all the -services it is bound to. - -Locked services and devices will show that they are locked in the web UI, and -editing options will be unavailable. -- 2.51.2