From c86c5749bafb30e3dbe6961a64077fdccfd5a72a Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 21 Nov 2018 16:48:00 -0800 Subject: [PATCH] Make the repository "Filesize Limit" and "Clone/Fetch Timeout" configurable in the UI Summary: Depends on D19830. Ref T13216. See PHI908. See PHI750. See PHI885. Allow users to configure a filesize limit, and allow them to adjust the clone/fetch timeout. Test Plan: {F6021356} - Configured a filesize limit and pushed, hit it. Made the limit larger and pushed, change went through. Reviewers: amckinley Reviewed By: amckinley Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13216 Differential Revision: https://secure.phabricator.com/D19831 --- src/__phutil_library_map__.php | 6 ++ .../editor/DiffusionRepositoryEditEngine.php | 19 ++++ .../engine/DiffusionCommitHookEngine.php | 4 +- ...ffusionRepositoryLimitsManagementPanel.php | 101 ++++++++++++++++++ .../protocol/DiffusionCommandEngine.php | 2 +- ...dockWorkingCopyBlueprintImplementation.php | 6 +- .../storage/PhabricatorRepository.php | 24 +++++ ...atorRepositoryCopyTimeLimitTransaction.php | 77 +++++++++++++ ...atorRepositoryFilesizeLimitTransaction.php | 78 ++++++++++++++ .../user/userguide/diffusion_managing.diviner | 69 +++++++----- 10 files changed, 352 insertions(+), 34 deletions(-) create mode 100644 src/applications/diffusion/management/DiffusionRepositoryLimitsManagementPanel.php create mode 100644 src/applications/repository/xaction/PhabricatorRepositoryCopyTimeLimitTransaction.php create mode 100644 src/applications/repository/xaction/PhabricatorRepositoryFilesizeLimitTransaction.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 89d8a6bfac..2b6732b7d5 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -950,6 +950,7 @@ phutil_register_library_map(array( 'DiffusionRepositoryHistoryManagementPanel' => 'applications/diffusion/management/DiffusionRepositoryHistoryManagementPanel.php', 'DiffusionRepositoryIdentityEditor' => 'applications/diffusion/editor/DiffusionRepositoryIdentityEditor.php', 'DiffusionRepositoryIdentitySearchEngine' => 'applications/diffusion/query/DiffusionRepositoryIdentitySearchEngine.php', + 'DiffusionRepositoryLimitsManagementPanel' => 'applications/diffusion/management/DiffusionRepositoryLimitsManagementPanel.php', 'DiffusionRepositoryListController' => 'applications/diffusion/controller/DiffusionRepositoryListController.php', 'DiffusionRepositoryManageController' => 'applications/diffusion/controller/DiffusionRepositoryManageController.php', 'DiffusionRepositoryManagePanelsController' => 'applications/diffusion/controller/DiffusionRepositoryManagePanelsController.php', @@ -4104,6 +4105,7 @@ phutil_register_library_map(array( 'PhabricatorRepositoryCommitRef' => 'applications/repository/engine/PhabricatorRepositoryCommitRef.php', 'PhabricatorRepositoryCommitTestCase' => 'applications/repository/storage/__tests__/PhabricatorRepositoryCommitTestCase.php', 'PhabricatorRepositoryConfigOptions' => 'applications/repository/config/PhabricatorRepositoryConfigOptions.php', + 'PhabricatorRepositoryCopyTimeLimitTransaction' => 'applications/repository/xaction/PhabricatorRepositoryCopyTimeLimitTransaction.php', 'PhabricatorRepositoryDAO' => 'applications/repository/storage/PhabricatorRepositoryDAO.php', 'PhabricatorRepositoryDangerousTransaction' => 'applications/repository/xaction/PhabricatorRepositoryDangerousTransaction.php', 'PhabricatorRepositoryDefaultBranchTransaction' => 'applications/repository/xaction/PhabricatorRepositoryDefaultBranchTransaction.php', @@ -4115,6 +4117,7 @@ phutil_register_library_map(array( 'PhabricatorRepositoryEngine' => 'applications/repository/engine/PhabricatorRepositoryEngine.php', 'PhabricatorRepositoryEnormousTransaction' => 'applications/repository/xaction/PhabricatorRepositoryEnormousTransaction.php', 'PhabricatorRepositoryFerretEngine' => 'applications/repository/search/PhabricatorRepositoryFerretEngine.php', + 'PhabricatorRepositoryFilesizeLimitTransaction' => 'applications/repository/xaction/PhabricatorRepositoryFilesizeLimitTransaction.php', 'PhabricatorRepositoryFulltextEngine' => 'applications/repository/search/PhabricatorRepositoryFulltextEngine.php', 'PhabricatorRepositoryGitCommitChangeParserWorker' => 'applications/repository/worker/commitchangeparser/PhabricatorRepositoryGitCommitChangeParserWorker.php', 'PhabricatorRepositoryGitCommitMessageParserWorker' => 'applications/repository/worker/commitmessageparser/PhabricatorRepositoryGitCommitMessageParserWorker.php', @@ -6361,6 +6364,7 @@ phutil_register_library_map(array( 'DiffusionRepositoryHistoryManagementPanel' => 'DiffusionRepositoryManagementPanel', 'DiffusionRepositoryIdentityEditor' => 'PhabricatorApplicationTransactionEditor', 'DiffusionRepositoryIdentitySearchEngine' => 'PhabricatorApplicationSearchEngine', + 'DiffusionRepositoryLimitsManagementPanel' => 'DiffusionRepositoryManagementPanel', 'DiffusionRepositoryListController' => 'DiffusionController', 'DiffusionRepositoryManageController' => 'DiffusionController', 'DiffusionRepositoryManagePanelsController' => 'DiffusionRepositoryManageController', @@ -10070,6 +10074,7 @@ phutil_register_library_map(array( 'PhabricatorRepositoryCommitRef' => 'Phobject', 'PhabricatorRepositoryCommitTestCase' => 'PhabricatorTestCase', 'PhabricatorRepositoryConfigOptions' => 'PhabricatorApplicationConfigOptions', + 'PhabricatorRepositoryCopyTimeLimitTransaction' => 'PhabricatorRepositoryTransactionType', 'PhabricatorRepositoryDAO' => 'PhabricatorLiskDAO', 'PhabricatorRepositoryDangerousTransaction' => 'PhabricatorRepositoryTransactionType', 'PhabricatorRepositoryDefaultBranchTransaction' => 'PhabricatorRepositoryTransactionType', @@ -10081,6 +10086,7 @@ phutil_register_library_map(array( 'PhabricatorRepositoryEngine' => 'Phobject', 'PhabricatorRepositoryEnormousTransaction' => 'PhabricatorRepositoryTransactionType', 'PhabricatorRepositoryFerretEngine' => 'PhabricatorFerretEngine', + 'PhabricatorRepositoryFilesizeLimitTransaction' => 'PhabricatorRepositoryTransactionType', 'PhabricatorRepositoryFulltextEngine' => 'PhabricatorFulltextEngine', 'PhabricatorRepositoryGitCommitChangeParserWorker' => 'PhabricatorRepositoryCommitChangeParserWorker', 'PhabricatorRepositoryGitCommitMessageParserWorker' => 'PhabricatorRepositoryCommitMessageParserWorker', diff --git a/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php b/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php index 78baeba71a..355a67480c 100644 --- a/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php +++ b/src/applications/diffusion/editor/DiffusionRepositoryEditEngine.php @@ -472,6 +472,25 @@ final class DiffusionRepositoryEditEngine pht('Change the push policy of the repository.')) ->setConduitTypeDescription(pht('New policy PHID or constant.')) ->setValue($object->getPolicy(DiffusionPushCapability::CAPABILITY)), + id(new PhabricatorTextEditField()) + ->setKey('filesizeLimit') + ->setLabel(pht('Filesize Limit')) + ->setTransactionType( + PhabricatorRepositoryFilesizeLimitTransaction::TRANSACTIONTYPE) + ->setDescription(pht('Maximum permitted file size.')) + ->setConduitDescription(pht('Change the filesize limit.')) + ->setConduitTypeDescription(pht('New repository filesize limit.')) + ->setValue($object->getFilesizeLimit()), + id(new PhabricatorTextEditField()) + ->setKey('copyTimeLimit') + ->setLabel(pht('Clone/Fetch Timeout')) + ->setTransactionType( + PhabricatorRepositoryCopyTimeLimitTransaction::TRANSACTIONTYPE) + ->setDescription( + pht('Maximum permitted duration of internal clone/fetch.')) + ->setConduitDescription(pht('Change the copy time limit.')) + ->setConduitTypeDescription(pht('New repository copy time limit.')) + ->setValue($object->getCopyTimeLimit()), ); } diff --git a/src/applications/diffusion/engine/DiffusionCommitHookEngine.php b/src/applications/diffusion/engine/DiffusionCommitHookEngine.php index 07d598cfa5..17f037471a 100644 --- a/src/applications/diffusion/engine/DiffusionCommitHookEngine.php +++ b/src/applications/diffusion/engine/DiffusionCommitHookEngine.php @@ -1268,9 +1268,7 @@ final class DiffusionCommitHookEngine extends Phobject { private function rejectOversizedFiles(array $content_updates) { $repository = $this->getRepository(); - // TODO: Allow repositories to be configured for a maximum filesize. - $limit = 0; - + $limit = $repository->getFilesizeLimit(); if (!$limit) { return; } diff --git a/src/applications/diffusion/management/DiffusionRepositoryLimitsManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryLimitsManagementPanel.php new file mode 100644 index 0000000000..861e1a22fb --- /dev/null +++ b/src/applications/diffusion/management/DiffusionRepositoryLimitsManagementPanel.php @@ -0,0 +1,101 @@ +isGit(); + } + + public function getManagementPanelIcon() { + $repository = $this->getRepository(); + + $any_limit = false; + + if ($repository->getFilesizeLimit()) { + $any_limit = true; + } + + if ($any_limit) { + return 'fa-signal'; + } else { + return 'fa-signal grey'; + } + } + + protected function getEditEngineFieldKeys() { + return array( + 'filesizeLimit', + 'copyTimeLimit', + ); + } + + public function buildManagementPanelCurtain() { + $repository = $this->getRepository(); + $viewer = $this->getViewer(); + $action_list = $this->newActionList(); + + $can_edit = PhabricatorPolicyFilter::hasCapability( + $viewer, + $repository, + PhabricatorPolicyCapability::CAN_EDIT); + + $limits_uri = $this->getEditPageURI(); + + $action_list->addAction( + id(new PhabricatorActionView()) + ->setIcon('fa-pencil') + ->setName(pht('Edit Limits')) + ->setHref($limits_uri) + ->setDisabled(!$can_edit) + ->setWorkflow(!$can_edit)); + + return $this->newCurtainView() + ->setActionList($action_list); + } + + public function buildManagementPanelContent() { + $repository = $this->getRepository(); + $viewer = $this->getViewer(); + + $view = id(new PHUIPropertyListView()) + ->setViewer($viewer); + + $byte_limit = $repository->getFilesizeLimit(); + if ($byte_limit) { + $filesize_display = pht('%s Bytes', new PhutilNumber($byte_limit)); + } else { + $filesize_display = pht('Unlimited'); + $filesize_display = phutil_tag('em', array(), $filesize_display); + } + + $view->addProperty(pht('Filesize Limit'), $filesize_display); + + $copy_limit = $repository->getCopyTimeLimit(); + if ($copy_limit) { + $copy_display = pht('%s Seconds', new PhutilNumber($copy_limit)); + } else { + $copy_default = $repository->getDefaultCopyTimeLimit(); + $copy_display = pht( + 'Default (%s Seconds)', + new PhutilNumber($copy_default)); + $copy_display = phutil_tag('em', array(), $copy_display); + } + + $view->addProperty(pht('Clone/Fetch Timeout'), $copy_display); + + return $this->newBox(pht('Limits'), $view); + } + +} diff --git a/src/applications/diffusion/protocol/DiffusionCommandEngine.php b/src/applications/diffusion/protocol/DiffusionCommandEngine.php index 9d52482b23..a0fe568168 100644 --- a/src/applications/diffusion/protocol/DiffusionCommandEngine.php +++ b/src/applications/diffusion/protocol/DiffusionCommandEngine.php @@ -139,7 +139,7 @@ abstract class DiffusionCommandEngine extends Phobject { // to try to avoid cases where `git fetch` hangs for some reason and we're // left sitting with a held lock forever. $repository = $this->getRepository(); - $future->setTimeout($repository->getCopyTimeLimit()); + $future->setTimeout($repository->getEffectiveCopyTimeLimit()); return $future; } diff --git a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php index eb3a361ea8..d82f5c2c15 100644 --- a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php +++ b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprintImplementation.php @@ -187,7 +187,7 @@ final class DrydockWorkingCopyBlueprintImplementation (string)$repository->getCloneURIObject(), $path); - $future->setTimeout($repository->getCopyTimeLimit()); + $future->setTimeout($repository->getEffectiveCopyTimeLimit()); $futures[$directory] = $future; } @@ -284,7 +284,7 @@ final class DrydockWorkingCopyBlueprintImplementation } $this->newExecvFuture($interface, $cmd, $arg) - ->setTimeout($repository->getCopyTimeLimit()) + ->setTimeout($repository->getEffectiveCopyTimeLimit()) ->resolvex(); if (idx($spec, 'default')) { @@ -310,7 +310,7 @@ final class DrydockWorkingCopyBlueprintImplementation try { $this->newExecvFuture($interface, $cmd, $arg) - ->setTimeout($repository->getCopyTimeLimit()) + ->setTimeout($repository->getEffectiveCopyTimeLimit()) ->resolvex(); } catch (CommandException $ex) { $display_command = csprintf( diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index 2b0d63cf47..83c5b411e4 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -1898,9 +1898,33 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO * @return int Maximum number of seconds to spend copying this repository. */ public function getCopyTimeLimit() { + return $this->getDetail('limit.copy'); + } + + public function setCopyTimeLimit($limit) { + return $this->setDetail('limit.copy', $limit); + } + + public function getDefaultCopyTimeLimit() { return phutil_units('15 minutes in seconds'); } + public function getEffectiveCopyTimeLimit() { + $limit = $this->getCopyTimeLimit(); + if ($limit) { + return $limit; + } + + return $this->getDefaultCopyTimeLimit(); + } + + public function getFilesizeLimit() { + return $this->getDetail('limit.filesize'); + } + + public function setFilesizeLimit($limit) { + return $this->setDetail('limit.filesize', $limit); + } /** * Retrieve the service URI for the device hosting this repository. diff --git a/src/applications/repository/xaction/PhabricatorRepositoryCopyTimeLimitTransaction.php b/src/applications/repository/xaction/PhabricatorRepositoryCopyTimeLimitTransaction.php new file mode 100644 index 0000000000..6be3466ae7 --- /dev/null +++ b/src/applications/repository/xaction/PhabricatorRepositoryCopyTimeLimitTransaction.php @@ -0,0 +1,77 @@ +getCopyTimeLimit(); + } + + public function generateNewValue($object, $value) { + if (!strlen($value)) { + return null; + } + + $value = (int)$value; + if (!$value) { + return null; + } + + return $value; + } + + public function applyInternalEffects($object, $value) { + $object->setCopyTimeLimit($value); + } + + public function getTitle() { + $old = $this->getOldValue(); + $new = $this->getNewValue(); + + if ($old && $new) { + return pht( + '%s changed the copy time limit for this repository from %s seconds '. + 'to %s seconds.', + $this->renderAuthor(), + $this->renderOldValue(), + $this->renderNewValue()); + } else if ($new) { + return pht( + '%s set the copy time limit for this repository to %s seconds.', + $this->renderAuthor(), + $this->renderNewValue()); + } else { + return pht( + '%s reset the copy time limit (%s seconds) for this repository '. + 'to the default value.', + $this->renderAuthor(), + $this->renderOldValue()); + } + } + + public function validateTransactions($object, array $xactions) { + $errors = array(); + + foreach ($xactions as $xaction) { + $new = $xaction->getNewValue(); + + if (!strlen($new)) { + continue; + } + + if (!preg_match('/^\d+\z/', $new)) { + $errors[] = $this->newInvalidError( + pht( + 'Unable to parse copy time limit, specify a positive number '. + 'of seconds.'), + $xaction); + continue; + } + } + + return $errors; + } + +} diff --git a/src/applications/repository/xaction/PhabricatorRepositoryFilesizeLimitTransaction.php b/src/applications/repository/xaction/PhabricatorRepositoryFilesizeLimitTransaction.php new file mode 100644 index 0000000000..6bf74cc757 --- /dev/null +++ b/src/applications/repository/xaction/PhabricatorRepositoryFilesizeLimitTransaction.php @@ -0,0 +1,78 @@ +getFilesizeLimit(); + } + + public function generateNewValue($object, $value) { + if (!strlen($value)) { + return null; + } + + $value = phutil_parse_bytes($value); + if (!$value) { + return null; + } + + return $value; + } + + public function applyInternalEffects($object, $value) { + $object->setFilesizeLimit($value); + } + + public function getTitle() { + $old = $this->getOldValue(); + $new = $this->getNewValue(); + + if ($old && $new) { + return pht( + '%s changed the filesize limit for this repository from %s bytes to '. + '%s bytes.', + $this->renderAuthor(), + $this->renderOldValue(), + $this->renderNewValue()); + } else if ($new) { + return pht( + '%s set the filesize limit for this repository to %s bytes.', + $this->renderAuthor(), + $this->renderNewValue()); + } else { + return pht( + '%s removed the filesize limit (%s bytes) for this repository.', + $this->renderAuthor(), + $this->renderOldValue()); + } + } + + public function validateTransactions($object, array $xactions) { + $errors = array(); + + foreach ($xactions as $xaction) { + $new = $xaction->getNewValue(); + + if (!strlen($new)) { + continue; + } + + try { + $value = phutil_parse_bytes($new); + } catch (Exception $ex) { + $errors[] = $this->newInvalidError( + pht( + 'Unable to parse filesize limit: %s', + $ex->getMessage()), + $xaction); + continue; + } + } + + return $errors; + } + +} diff --git a/src/docs/user/userguide/diffusion_managing.diviner b/src/docs/user/userguide/diffusion_managing.diviner index cc03433532..432b4de13d 100644 --- a/src/docs/user/userguide/diffusion_managing.diviner +++ b/src/docs/user/userguide/diffusion_managing.diviner @@ -236,37 +236,20 @@ fetch from, serve from, and push to. These options are covered in detail in @{article:Diffusion User Guide: URIs}. -Staging Area -============ - -The **Staging Area** panel configures staging areas, used to make proposed -changes available to build and continuous integration systems. - -For more details, see @{article:Harbormaster User Guide}. - - -Automation -========== - -The **Automation** panel configures support for allowing Phabricator to make -writes directly to the repository, so that it can perform operations like -automatically landing revisions from the web UI. - -For details on repository automation, see -@{article:Drydock User Guide: Repository Automation}. - - -Symbols +Limits ====== -The **Symbols** panel allows you to customize how symbols (like class and -function names) are linked when viewing code in the repository, and when -viewing revisions which propose code changes to the repository. +The **Limits** panel allows you to configure limits and timeouts. -To take advantage of this feature, you need to do additional work to build -symbol indexes. For details on configuring and populating symbol indexes, see -@{article:User Guide: Symbol Indexes}. +**Filesize Limit**: Allows you to set a maximum filesize for any file in the +repository. If a commit creates a larger file (or modifies an existing file so +it becomes too large) it will be rejected. This option only applies to hosted +repositories. +**Clone/Fetch Timeout**: Configure the internal timeout for creating copies +of this repository during operations like intracluster synchronization and +Drydock working copy construction. This timeout does not affect external +users. Branches ======== @@ -311,6 +294,38 @@ revisions and tasks. If you don't want Phabricator to close objects when it discovers new commits, disable **Autoclose** for the repository. +Staging Area +============ + +The **Staging Area** panel configures staging areas, used to make proposed +changes available to build and continuous integration systems. + +For more details, see @{article:Harbormaster User Guide}. + + +Automation +========== + +The **Automation** panel configures support for allowing Phabricator to make +writes directly to the repository, so that it can perform operations like +automatically landing revisions from the web UI. + +For details on repository automation, see +@{article:Drydock User Guide: Repository Automation}. + + +Symbols +====== + +The **Symbols** panel allows you to customize how symbols (like class and +function names) are linked when viewing code in the repository, and when +viewing revisions which propose code changes to the repository. + +To take advantage of this feature, you need to do additional work to build +symbol indexes. For details on configuring and populating symbol indexes, see +@{article:User Guide: Symbol Indexes}. + + Repository Identifiers and Names ================================