From cbff91343254f41c9bbf3a12f4575b84cbf9411a Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 12 Jun 2018 07:19:05 -0700 Subject: [PATCH 1/7] Add a "members of all projects" (vs "...any project") custom policy rule to the upstream Summary: Ref T13151. See PHI702. An install is interested in a "members of all projects" (vs "members of any project", which is currently implemented) rule. Although this is fairly niche, I think it's reasonable and doesn't have much of a maintenance cost. This could already be implemented as an extension, but it would have to copy/paste a bunch of code. Test Plan: - Ran unit tests. - Used the UI to select this policy for a task, with various values. Joined/left projects to satisfy/fail the rule. Behavior seemed correct. - Used the UI to select the existing policy rule ("any project"), joined/left projects to satisfy/fail the rule. Doesn't look like I broke anything. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13151 Differential Revision: https://secure.phabricator.com/D19486 --- src/__phutil_library_map__.php | 6 +- .../PhabricatorProjectCoreTestCase.php | 94 +++++++++++++++++++ .../PhabricatorProjectsAllPolicyRule.php | 29 ++++++ .../PhabricatorProjectsBasePolicyRule.php | 64 +++++++++++++ .../PhabricatorProjectsPolicyRule.php | 61 +----------- 5 files changed, 196 insertions(+), 58 deletions(-) create mode 100644 src/applications/project/policyrule/PhabricatorProjectsAllPolicyRule.php create mode 100644 src/applications/project/policyrule/PhabricatorProjectsBasePolicyRule.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 1e9e4281f8..7d3e5553fd 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4026,7 +4026,9 @@ phutil_register_library_map(array( 'PhabricatorProjectWorkboardBackgroundTransaction' => 'applications/project/xaction/PhabricatorProjectWorkboardBackgroundTransaction.php', 'PhabricatorProjectWorkboardProfileMenuItem' => 'applications/project/menuitem/PhabricatorProjectWorkboardProfileMenuItem.php', 'PhabricatorProjectWorkboardTransaction' => 'applications/project/xaction/PhabricatorProjectWorkboardTransaction.php', + 'PhabricatorProjectsAllPolicyRule' => 'applications/project/policyrule/PhabricatorProjectsAllPolicyRule.php', 'PhabricatorProjectsAncestorsSearchEngineAttachment' => 'applications/project/engineextension/PhabricatorProjectsAncestorsSearchEngineAttachment.php', + 'PhabricatorProjectsBasePolicyRule' => 'applications/project/policyrule/PhabricatorProjectsBasePolicyRule.php', 'PhabricatorProjectsCurtainExtension' => 'applications/project/engineextension/PhabricatorProjectsCurtainExtension.php', 'PhabricatorProjectsEditEngineExtension' => 'applications/project/engineextension/PhabricatorProjectsEditEngineExtension.php', 'PhabricatorProjectsEditField' => 'applications/transactions/editfield/PhabricatorProjectsEditField.php', @@ -9893,7 +9895,9 @@ phutil_register_library_map(array( 'PhabricatorProjectWorkboardBackgroundTransaction' => 'PhabricatorProjectTransactionType', 'PhabricatorProjectWorkboardProfileMenuItem' => 'PhabricatorProfileMenuItem', 'PhabricatorProjectWorkboardTransaction' => 'PhabricatorProjectTransactionType', + 'PhabricatorProjectsAllPolicyRule' => 'PhabricatorProjectsBasePolicyRule', 'PhabricatorProjectsAncestorsSearchEngineAttachment' => 'PhabricatorSearchEngineAttachment', + 'PhabricatorProjectsBasePolicyRule' => 'PhabricatorPolicyRule', 'PhabricatorProjectsCurtainExtension' => 'PHUICurtainExtension', 'PhabricatorProjectsEditEngineExtension' => 'PhabricatorEditEngineExtension', 'PhabricatorProjectsEditField' => 'PhabricatorTokenizerEditField', @@ -9902,7 +9906,7 @@ phutil_register_library_map(array( 'PhabricatorProjectsMailEngineExtension' => 'PhabricatorMailEngineExtension', 'PhabricatorProjectsMembersSearchEngineAttachment' => 'PhabricatorSearchEngineAttachment', 'PhabricatorProjectsMembershipIndexEngineExtension' => 'PhabricatorIndexEngineExtension', - 'PhabricatorProjectsPolicyRule' => 'PhabricatorPolicyRule', + 'PhabricatorProjectsPolicyRule' => 'PhabricatorProjectsBasePolicyRule', 'PhabricatorProjectsSearchEngineAttachment' => 'PhabricatorSearchEngineAttachment', 'PhabricatorProjectsSearchEngineExtension' => 'PhabricatorSearchEngineExtension', 'PhabricatorProjectsWatchersSearchEngineAttachment' => 'PhabricatorSearchEngineAttachment', diff --git a/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php b/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php index f34f224521..e50c83ab5a 100644 --- a/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php +++ b/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php @@ -1177,6 +1177,100 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase { $this->assertTrue($can_edit); } + public function testProjectPolicyRules() { + $author = $this->generateNewTestUser(); + + $proj_a = PhabricatorProject::initializeNewProject($author) + ->setName('Policy A') + ->save(); + $proj_b = PhabricatorProject::initializeNewProject($author) + ->setName('Policy B') + ->save(); + + $user_none = $this->generateNewTestUser(); + $user_any = $this->generateNewTestUser(); + $user_all = $this->generateNewTestUser(); + + $this->joinProject($proj_a, $user_any); + $this->joinProject($proj_a, $user_all); + $this->joinProject($proj_b, $user_all); + + $any_policy = id(new PhabricatorPolicy()) + ->setRules( + array( + array( + 'action' => PhabricatorPolicy::ACTION_ALLOW, + 'rule' => 'PhabricatorProjectsPolicyRule', + 'value' => array( + $proj_a->getPHID(), + $proj_b->getPHID(), + ), + ), + )) + ->save(); + + $all_policy = id(new PhabricatorPolicy()) + ->setRules( + array( + array( + 'action' => PhabricatorPolicy::ACTION_ALLOW, + 'rule' => 'PhabricatorProjectsAllPolicyRule', + 'value' => array( + $proj_a->getPHID(), + $proj_b->getPHID(), + ), + ), + )) + ->save(); + + $any_task = ManiphestTask::initializeNewTask($author) + ->setViewPolicy($any_policy->getPHID()) + ->save(); + + $all_task = ManiphestTask::initializeNewTask($author) + ->setViewPolicy($all_policy->getPHID()) + ->save(); + + $map = array( + array( + pht('Project policy rule; user in no projects'), + $user_none, + false, + false, + ), + array( + pht('Project policy rule; user in some projects'), + $user_any, + true, + false, + ), + array( + pht('Project policy rule; user in all projects'), + $user_all, + true, + true, + ), + ); + + foreach ($map as $test_case) { + list($label, $user, $expect_any, $expect_all) = $test_case; + + $can_any = PhabricatorPolicyFilter::hasCapability( + $user, + $any_task, + PhabricatorPolicyCapability::CAN_VIEW); + + $can_all = PhabricatorPolicyFilter::hasCapability( + $user, + $all_task, + PhabricatorPolicyCapability::CAN_VIEW); + + $this->assertEqual($expect_any, $can_any, pht('%s / Any', $label)); + $this->assertEqual($expect_all, $can_all, pht('%s / All', $label)); + } + } + + private function moveToColumn( PhabricatorUser $viewer, PhabricatorProject $board, diff --git a/src/applications/project/policyrule/PhabricatorProjectsAllPolicyRule.php b/src/applications/project/policyrule/PhabricatorProjectsAllPolicyRule.php new file mode 100644 index 0000000000..25b7cf7c4e --- /dev/null +++ b/src/applications/project/policyrule/PhabricatorProjectsAllPolicyRule.php @@ -0,0 +1,29 @@ +getMemberships($viewer->getPHID()); + foreach ($value as $project_phid) { + if (empty($memberships[$project_phid])) { + return false; + } + } + + return true; + } + + public function getRuleOrder() { + return 205; + } + +} diff --git a/src/applications/project/policyrule/PhabricatorProjectsBasePolicyRule.php b/src/applications/project/policyrule/PhabricatorProjectsBasePolicyRule.php new file mode 100644 index 0000000000..fed217779b --- /dev/null +++ b/src/applications/project/policyrule/PhabricatorProjectsBasePolicyRule.php @@ -0,0 +1,64 @@ +memberships, $viewer_phid, array()); + } + + public function willApplyRules( + PhabricatorUser $viewer, + array $values, + array $objects) { + + $values = array_unique(array_filter(array_mergev($values))); + if (!$values) { + return; + } + + $projects = id(new PhabricatorProjectQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withMemberPHIDs(array($viewer->getPHID())) + ->withPHIDs($values) + ->execute(); + foreach ($projects as $project) { + $this->memberships[$viewer->getPHID()][$project->getPHID()] = true; + } + } + + public function getValueControlType() { + return self::CONTROL_TYPE_TOKENIZER; + } + + public function getValueControlTemplate() { + $datasource = id(new PhabricatorProjectDatasource()) + ->setParameters( + array( + 'policy' => 1, + )); + + return $this->getDatasourceTemplate($datasource); + } + + public function getValueForStorage($value) { + PhutilTypeSpec::newFromString('list')->check($value); + return array_values($value); + } + + public function getValueForDisplay(PhabricatorUser $viewer, $value) { + $handles = id(new PhabricatorHandleQuery()) + ->setViewer($viewer) + ->withPHIDs($value) + ->execute(); + + return mpull($handles, 'getFullName', 'getPHID'); + } + + public function ruleHasEffect($value) { + return (bool)$value; + } + +} diff --git a/src/applications/project/policyrule/PhabricatorProjectsPolicyRule.php b/src/applications/project/policyrule/PhabricatorProjectsPolicyRule.php index 3977b542c1..b7ad734641 100644 --- a/src/applications/project/policyrule/PhabricatorProjectsPolicyRule.php +++ b/src/applications/project/policyrule/PhabricatorProjectsPolicyRule.php @@ -1,32 +1,10 @@ setViewer(PhabricatorUser::getOmnipotentUser()) - ->withMemberPHIDs(array($viewer->getPHID())) - ->withPHIDs($values) - ->execute(); - foreach ($projects as $project) { - $this->memberships[$viewer->getPHID()][$project->getPHID()] = true; - } + return pht('members of any project'); } public function applyRule( @@ -34,8 +12,9 @@ final class PhabricatorProjectsPolicyRule $value, PhabricatorPolicyInterface $object) { + $memberships = $this->getMemberships($viewer->getPHID()); foreach ($value as $project_phid) { - if (isset($this->memberships[$viewer->getPHID()][$project_phid])) { + if (isset($memberships[$project_phid])) { return true; } } @@ -43,40 +22,8 @@ final class PhabricatorProjectsPolicyRule return false; } - public function getValueControlType() { - return self::CONTROL_TYPE_TOKENIZER; - } - - public function getValueControlTemplate() { - $datasource = id(new PhabricatorProjectDatasource()) - ->setParameters( - array( - 'policy' => 1, - )); - - return $this->getDatasourceTemplate($datasource); - } - public function getRuleOrder() { return 200; } - public function getValueForStorage($value) { - PhutilTypeSpec::newFromString('list')->check($value); - return array_values($value); - } - - public function getValueForDisplay(PhabricatorUser $viewer, $value) { - $handles = id(new PhabricatorHandleQuery()) - ->setViewer($viewer) - ->withPHIDs($value) - ->execute(); - - return mpull($handles, 'getFullName', 'getPHID'); - } - - public function ruleHasEffect($value) { - return (bool)$value; - } - } From 94752278f4cd0b4b9426339d2923bd5a8d77c6c9 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 12 Jun 2018 07:46:50 -0700 Subject: [PATCH 2/7] Add a generic PHID-based object redirection controller Summary: Ref T13151. See PHI647. This allows us to link to any object by PHID, without disclosing information in the monogram (like `#fire-steve`). This capability is relevant when building "secure mail", to provide a link to the object regardless of whether the monogram discloses information or not. Test Plan: Visited `/object/D123/` (redirect), `/object/xyz/` (404), `/object/PHID-DREV-.../` (redirect). Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13151 Differential Revision: https://secure.phabricator.com/D19487 --- src/__phutil_library_map__.php | 2 + .../PhabricatorSystemApplication.php | 1 + .../PhabricatorSystemObjectController.php | 39 +++++++++++++++++++ 3 files changed, 42 insertions(+) create mode 100644 src/applications/system/controller/PhabricatorSystemObjectController.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 7d3e5553fd..568c715ea1 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4451,6 +4451,7 @@ phutil_register_library_map(array( 'PhabricatorSystemDAO' => 'applications/system/storage/PhabricatorSystemDAO.php', 'PhabricatorSystemDestructionGarbageCollector' => 'applications/system/garbagecollector/PhabricatorSystemDestructionGarbageCollector.php', 'PhabricatorSystemDestructionLog' => 'applications/system/storage/PhabricatorSystemDestructionLog.php', + 'PhabricatorSystemObjectController' => 'applications/system/controller/PhabricatorSystemObjectController.php', 'PhabricatorSystemReadOnlyController' => 'applications/system/controller/PhabricatorSystemReadOnlyController.php', 'PhabricatorSystemRemoveDestroyWorkflow' => 'applications/system/management/PhabricatorSystemRemoveDestroyWorkflow.php', 'PhabricatorSystemRemoveLogWorkflow' => 'applications/system/management/PhabricatorSystemRemoveLogWorkflow.php', @@ -10406,6 +10407,7 @@ phutil_register_library_map(array( 'PhabricatorSystemDAO' => 'PhabricatorLiskDAO', 'PhabricatorSystemDestructionGarbageCollector' => 'PhabricatorGarbageCollector', 'PhabricatorSystemDestructionLog' => 'PhabricatorSystemDAO', + 'PhabricatorSystemObjectController' => 'PhabricatorController', 'PhabricatorSystemReadOnlyController' => 'PhabricatorController', 'PhabricatorSystemRemoveDestroyWorkflow' => 'PhabricatorSystemRemoveWorkflow', 'PhabricatorSystemRemoveLogWorkflow' => 'PhabricatorSystemRemoveWorkflow', diff --git a/src/applications/system/application/PhabricatorSystemApplication.php b/src/applications/system/application/PhabricatorSystemApplication.php index 0ec8f6a7a4..3fa40b3912 100644 --- a/src/applications/system/application/PhabricatorSystemApplication.php +++ b/src/applications/system/application/PhabricatorSystemApplication.php @@ -26,6 +26,7 @@ final class PhabricatorSystemApplication extends PhabricatorApplication { '/readonly/' => array( '(?P[^/]+)/' => 'PhabricatorSystemReadOnlyController', ), + '/object/(?P[^/]+)/' => 'PhabricatorSystemObjectController', ); } diff --git a/src/applications/system/controller/PhabricatorSystemObjectController.php b/src/applications/system/controller/PhabricatorSystemObjectController.php new file mode 100644 index 0000000000..464c36a59a --- /dev/null +++ b/src/applications/system/controller/PhabricatorSystemObjectController.php @@ -0,0 +1,39 @@ +getViewer(); + $name = $request->getURIData('name'); + + $object = id(new PhabricatorObjectQuery()) + ->setViewer($viewer) + ->withNames(array($name)) + ->executeOne(); + if (!$object) { + return new Aphront404Response(); + } + + $phid = $object->getPHID(); + $handles = $viewer->loadHandles(array($phid)); + $handle = $handles[$phid]; + + $object_uri = $handle->getURI(); + if (!strlen($object_uri)) { + return $this->newDialog() + ->setTitle(pht('No Object URI')) + ->appendParagraph( + pht( + 'Object "%s" exists, but does not have a URI to redirect to.', + $name)) + ->addCancelButton('/', pht('Done')); + } + + return id(new AphrontRedirectResponse())->setURI($object_uri); + } +} From 62a402491ae4a42ecdf5091032dfa9ac98a9e450 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 12 Jun 2018 07:57:50 -0700 Subject: [PATCH 3/7] Allow encrypted mail to be more specific about which object is affected Summary: Depends on D19487. Ref T13151. See PHI647. For some objects, like revisions, we can build slightly more useful secure email without actually disclosing anything. In the general case, the object monogram may disclose information (`#acquire-competitor`) but most do not, so applications can whitelist an acceptable nondisclosing subject and link. Support doing this, and make Differential do it. When we don't have a whitelisted URI but do know the object the mail is about, include a generic PHID-based URI; these are always nondisclosing. Test Plan: - Without the Differential changes, sent normal mail (no changes) and secure mail (new generic PHID-based link). - With the Differential changes, sent secure mail; got richer subject and body link. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13151 Differential Revision: https://secure.phabricator.com/D19488 --- .../editor/DifferentialTransactionEditor.php | 7 +-- .../storage/PhabricatorMetaMTAMail.php | 45 ++++++++++++++++--- 2 files changed, 44 insertions(+), 8 deletions(-) diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 64ee1d5e81..4a3e753292 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -592,12 +592,13 @@ final class DifferentialTransactionEditor } protected function buildMailTemplate(PhabricatorLiskDAO $object) { - $id = $object->getID(); + $monogram = $object->getMonogram(); $title = $object->getTitle(); - $subject = "D{$id}: {$title}"; return id(new PhabricatorMetaMTAMail()) - ->setSubject($subject); + ->setSubject(pht('%s: %s', $monogram, $title)) + ->setMustEncryptSubject(pht('%s: Revision Updated', $monogram)) + ->setMustEncryptURI($object->getURI()); } protected function getTransactionsForMail( diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php index 5326e10639..d759ef6583 100644 --- a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php @@ -291,17 +291,31 @@ final class PhabricatorMetaMTAMail } public function setMustEncrypt($bool) { - $this->setParam('mustEncrypt', $bool); - return $this; + return $this->setParam('mustEncrypt', $bool); } public function getMustEncrypt() { return $this->getParam('mustEncrypt', false); } + public function setMustEncryptURI($uri) { + return $this->setParam('mustEncrypt.uri', $uri); + } + + public function getMustEncryptURI() { + return $this->getParam('mustEncrypt.uri'); + } + + public function setMustEncryptSubject($subject) { + return $this->setParam('mustEncrypt.subject', $subject); + } + + public function getMustEncryptSubject() { + return $this->getParam('mustEncrypt.subject'); + } + public function setMustEncryptReasons(array $reasons) { - $this->setParam('mustEncryptReasons', $reasons); - return $this; + return $this->setParam('mustEncryptReasons', $reasons); } public function getMustEncryptReasons() { @@ -787,7 +801,11 @@ final class PhabricatorMetaMTAMail // If mail content must be encrypted, we replace the subject with // a generic one. if ($must_encrypt) { - $subject[] = pht('Object Updated'); + $encrypt_subject = $this->getMustEncryptSubject(); + if (!strlen($encrypt_subject)) { + $encrypt_subject = pht('Object Updated'); + } + $subject[] = $encrypt_subject; } else { $vary_prefix = idx($params, 'vary-subject-prefix'); if ($vary_prefix != '') { @@ -845,6 +863,23 @@ final class PhabricatorMetaMTAMail $body = $raw_body; if ($must_encrypt) { $parts = array(); + + $encrypt_uri = $this->getMustEncryptURI(); + if (!strlen($encrypt_uri)) { + $encrypt_phid = $this->getRelatedPHID(); + if ($encrypt_phid) { + $encrypt_uri = urisprintf( + '/object/%s/', + $encrypt_phid); + } + } + + if (strlen($encrypt_uri)) { + $parts[] = pht( + 'This secure message is notifying you of a change to this object:'); + $parts[] = PhabricatorEnv::getProductionURI($encrypt_uri); + } + $parts[] = pht( 'The content for this message can only be transmitted over a '. 'secure channel. To view the message content, follow this '. From c5b13a6be3b7262c49635ceade437345a02de2cc Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 12 Jun 2018 10:04:22 -0700 Subject: [PATCH 4/7] Allow object subtypes to be changed via bulk editor Summary: Ref T13151. See PHI683. Ref T12314. You can currently change object subtypes via Conduit (`maniphest.edit`) but not via the web UI. Changing object subtypes is inherently a somewhat-perilous operation that likely has a lot of rough edges we'll need to smooth over eventually, mostly around changing an object from subtype X to subtype Y, where some field exists on one but not the other. This isn't a huge issue, just not entirely intuitive. It should also, in theory, be fairly rare. As a reasonable middle ground, provide web UI access via the bulk editor. This makes it possible, but doesn't clutter the UI up with a rarely-used option with rough edges. Test Plan: - With subtypes not configured, saw a normal bulk editor with no new option. - With subtypes configured, swapped tasks subtypes via bulk editor. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13151, T12314 Differential Revision: https://secure.phabricator.com/D19490 --- .../PhabricatorSubtypeEditEngineExtension.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/applications/transactions/engineextension/PhabricatorSubtypeEditEngineExtension.php b/src/applications/transactions/engineextension/PhabricatorSubtypeEditEngineExtension.php index 260c0d2acf..338702478c 100644 --- a/src/applications/transactions/engineextension/PhabricatorSubtypeEditEngineExtension.php +++ b/src/applications/transactions/engineextension/PhabricatorSubtypeEditEngineExtension.php @@ -47,6 +47,11 @@ final class PhabricatorSubtypeEditEngineExtension ->setValue($object->getEditEngineSubtype()) ->setOptions($options); + // If subtypes are configured, enable changing them from the bulk editor. + if (count($map) > 1) { + $subtype_field->setBulkEditLabel(pht('Change subtype to')); + } + return array( $subtype_field, ); From 6011085b0fcd7bdcdeb422bd73571a79584c5c8f Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 12 Jun 2018 09:39:00 -0700 Subject: [PATCH 5/7] Respect "metamta.email-body-limit" when building mail HTML bodies Summary: Ref T13151. See T11767. See PHI686. Although we limit outbound mail text bodies, the limit doesn't currently apply to attachments, HTML bodies, or headers. T11767 discusses improving this in the general case. In the wild, an install hit an issue (see PHI686) where edits to Phriction pages generate very large HTML bodies. Check and respect the limit when building HTML bodies. If we don't have enough room for the HTML body, we just drop it. We have the text body to fall back to, and HTML is difficult to truncate safely. Test Plan: Added unit tests and made them pass. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13151 Differential Revision: https://secure.phabricator.com/D19489 --- ...abricatorMailImplementationTestAdapter.php | 8 ++ .../storage/PhabricatorMetaMTAMail.php | 21 +++-- .../PhabricatorMetaMTAMailTestCase.php | 80 +++++++++++++++++++ 3 files changed, 103 insertions(+), 6 deletions(-) diff --git a/src/applications/metamta/adapter/PhabricatorMailImplementationTestAdapter.php b/src/applications/metamta/adapter/PhabricatorMailImplementationTestAdapter.php index 8a8d0de0c2..80807398f1 100644 --- a/src/applications/metamta/adapter/PhabricatorMailImplementationTestAdapter.php +++ b/src/applications/metamta/adapter/PhabricatorMailImplementationTestAdapter.php @@ -123,4 +123,12 @@ final class PhabricatorMailImplementationTestAdapter return $this; } + public function getBody() { + return idx($this->guts, 'body'); + } + + public function getHTMLBody() { + return idx($this->guts, 'html-body'); + } + } diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php index d759ef6583..c2435499af 100644 --- a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php @@ -892,15 +892,16 @@ final class PhabricatorMetaMTAMail $body = $raw_body; } - $max = PhabricatorEnv::getEnvConfig('metamta.email-body-limit'); - if (strlen($body) > $max) { + $body_limit = PhabricatorEnv::getEnvConfig('metamta.email-body-limit'); + if (strlen($body) > $body_limit) { $body = id(new PhutilUTF8StringTruncator()) - ->setMaximumBytes($max) + ->setMaximumBytes($body_limit) ->truncateString($body); $body .= "\n"; - $body .= pht('(This email was truncated at %d bytes.)', $max); + $body .= pht('(This email was truncated at %d bytes.)', $body_limit); } $mailer->setBody($body); + $body_limit -= strlen($body); // If we sent a different message body than we were asked to, record // what we actually sent to make debugging and diagnostics easier. @@ -914,8 +915,16 @@ final class PhabricatorMetaMTAMail $send_html = $this->shouldSendHTML($preferences); } - if ($send_html && isset($params['html-body'])) { - $mailer->setHTMLBody($params['html-body']); + if ($send_html) { + $html_body = idx($params, 'html-body'); + + // NOTE: We just drop the entire HTML body if it won't fit. Safely + // truncating HTML is hard, and we already have the text body to fall + // back to. + if (strlen($html_body) <= $body_limit) { + $mailer->setHTMLBody($html_body); + $body_limit -= strlen($html_body); + } } // Pass the headers to the mailer, then save the state so we can show diff --git a/src/applications/metamta/storage/__tests__/PhabricatorMetaMTAMailTestCase.php b/src/applications/metamta/storage/__tests__/PhabricatorMetaMTAMailTestCase.php index c0045301fd..d20a28fc15 100644 --- a/src/applications/metamta/storage/__tests__/PhabricatorMetaMTAMailTestCase.php +++ b/src/applications/metamta/storage/__tests__/PhabricatorMetaMTAMailTestCase.php @@ -331,4 +331,84 @@ final class PhabricatorMetaMTAMailTestCase extends PhabricatorTestCase { $this->assertEqual(null, $mail->getMailerKey()); } + public function testMailSizeLimits() { + $env = PhabricatorEnv::beginScopedEnv(); + $env->overrideEnvConfig('metamta.email-body-limit', 1024 * 512); + + $user = $this->generateNewTestUser(); + $phid = $user->getPHID(); + + $string_1kb = str_repeat('x', 1024); + $html_1kb = str_repeat('y', 1024); + $string_1mb = str_repeat('x', 1024 * 1024); + $html_1mb = str_repeat('y', 1024 * 1024); + + // First, send a mail with a small text body and a small HTML body to make + // sure the basics work properly. + $mail = id(new PhabricatorMetaMTAMail()) + ->addTos(array($phid)) + ->setBody($string_1kb) + ->setHTMLBody($html_1kb); + + $mailer = new PhabricatorMailImplementationTestAdapter(); + $mail->sendWithMailers(array($mailer)); + $this->assertEqual( + PhabricatorMailOutboundStatus::STATUS_SENT, + $mail->getStatus()); + + $text_body = $mailer->getBody(); + $html_body = $mailer->getHTMLBody(); + + $this->assertEqual($string_1kb, $text_body); + $this->assertEqual($html_1kb, $html_body); + + + // Now, send a mail with a large text body and a large HTML body. We expect + // the text body to be truncated and the HTML body to be dropped. + $mail = id(new PhabricatorMetaMTAMail()) + ->addTos(array($phid)) + ->setBody($string_1mb) + ->setHTMLBody($html_1mb); + + $mailer = new PhabricatorMailImplementationTestAdapter(); + $mail->sendWithMailers(array($mailer)); + $this->assertEqual( + PhabricatorMailOutboundStatus::STATUS_SENT, + $mail->getStatus()); + + $text_body = $mailer->getBody(); + $html_body = $mailer->getHTMLBody(); + + // We expect the body was truncated, because it exceeded the body limit. + $this->assertTrue( + (strlen($text_body) < strlen($string_1mb)), + pht('Text Body Truncated')); + + // We expect the HTML body was dropped completely after the text body was + // truncated. + $this->assertTrue( + !strlen($html_body), + pht('HTML Body Removed')); + + + // Next send a mail with a small text body and a large HTML body. We expect + // the text body to be intact and the HTML body to be dropped. + $mail = id(new PhabricatorMetaMTAMail()) + ->addTos(array($phid)) + ->setBody($string_1kb) + ->setHTMLBody($html_1mb); + + $mailer = new PhabricatorMailImplementationTestAdapter(); + $mail->sendWithMailers(array($mailer)); + $this->assertEqual( + PhabricatorMailOutboundStatus::STATUS_SENT, + $mail->getStatus()); + + $text_body = $mailer->getBody(); + $html_body = $mailer->getHTMLBody(); + + $this->assertEqual($string_1kb, $text_body); + $this->assertTrue(!strlen($html_body)); + } + } From 1459fb303769af0903733bfdf6d7129d82665814 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 7 Jun 2018 09:04:25 -0700 Subject: [PATCH 6/7] Make re-running `rebuild-identities` a bit faster and add a little progress information Summary: Ref T13151. Ref T12164. Two small tweaks: - If we aren't actually going to change anything, just skip the writes. This makes re-running/resuming a lot faster (~20x, locally). - Print when we touch a commit so there's some kind of visible status. This is just a small quality-of-life tweak that I wrote anyway while investigating T13152, and will make finishing off db024, db025 and db010 manually a little easier. Test Plan: - Set `authorIdentityPHID` + `committerIdentityPHID` to `NULL`. - Ran `rebuild-identities`, saw status information. - Ran `rebuild-identiites` again, saw it go faster with status information. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13151, T12164 Differential Revision: https://secure.phabricator.com/D19484 --- ...oryManagementRebuildIdentitiesWorkflow.php | 54 +++++++++++++------ 1 file changed, 39 insertions(+), 15 deletions(-) diff --git a/src/applications/repository/management/PhabricatorRepositoryManagementRebuildIdentitiesWorkflow.php b/src/applications/repository/management/PhabricatorRepositoryManagementRebuildIdentitiesWorkflow.php index f7a0cf58f8..86cdcaa462 100644 --- a/src/applications/repository/management/PhabricatorRepositoryManagementRebuildIdentitiesWorkflow.php +++ b/src/applications/repository/management/PhabricatorRepositoryManagementRebuildIdentitiesWorkflow.php @@ -44,27 +44,51 @@ final class PhabricatorRepositoryManagementRebuildIdentitiesWorkflow $iterator = new PhabricatorQueryIterator($query); foreach ($iterator as $commit) { + $needs_update = false; + $data = $commit->getCommitData(); $author_name = $data->getAuthorName(); + $author_identity = $this->getIdentityForCommit( - $commit, $author_name); + $commit, + $author_name); - $commit->setAuthorIdentityPHID($author_identity->getPHID()); - $data->setCommitDetail( - 'authorIdentityPHID', $author_identity->getPHID()); - - $committer_name = $data->getCommitDetail('committer', null); - if ($committer_name) { - $committer_identity = $this->getIdentityForCommit( - $commit, $committer_name); - - $commit->setCommitterIdentityPHID($committer_identity->getPHID()); - $data->setCommitDetail( - 'committerIdentityPHID', $committer_identity->getPHID()); + $author_phid = $commit->getAuthorIdentityPHID(); + $identity_phid = $author_identity->getPHID(); + if ($author_phid !== $identity_phid) { + $commit->setAuthorIdentityPHID($identity_phid); + $data->setCommitDetail('authorIdentityPHID', $identity_phid); + $needs_update = true; } - $commit->save(); - $data->save(); + $committer_name = $data->getCommitDetail('committer', null); + $committer_phid = $commit->getCommitterIdentityPHID(); + if (strlen($committer_name)) { + $committer_identity = $this->getIdentityForCommit( + $commit, + $committer_name); + $identity_phid = $committer_identity->getPHID(); + } else { + $identity_phid = null; + } + + if ($committer_phid !== $identity_phid) { + $commit->setCommitterIdentityPHID($identity_phid); + $data->setCommitDetail('committerIdentityPHID', $identity_phid); + $needs_update = true; + } + + if ($needs_update) { + $commit->save(); + $data->save(); + echo tsprintf( + "Rebuilt identities for %s.\n", + $commit->getDisplayName()); + } else { + echo tsprintf( + "No changes for %s.\n", + $commit->getDisplayName()); + } } } From a7c681b54997b849c46054571fa2d0071c97fa32 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 15 Jun 2018 12:03:28 -0700 Subject: [PATCH 7/7] Don't set mail HTML bodies if there's no actual HTML body Summary: See . Some mailers get upset if we `setHTMLBody(...)` with an empty string. There's some possible argument they should be more graceful about this, but it's reasonably pretty ambiguous. Only try to set the HTML body if we actually have a nonempty HTML body. Test Plan: - Configured an "smtp" mailer. - Ran `echo hi | ./bin/mail send-test --to someone@somewhere.com --subject test`. - Before: error about empty message body. - After: no more message body error. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D19494 --- .../metamta/storage/PhabricatorMetaMTAMail.php | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php index c2435499af..cca3dfa335 100644 --- a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php @@ -917,13 +917,14 @@ final class PhabricatorMetaMTAMail if ($send_html) { $html_body = idx($params, 'html-body'); - - // NOTE: We just drop the entire HTML body if it won't fit. Safely - // truncating HTML is hard, and we already have the text body to fall - // back to. - if (strlen($html_body) <= $body_limit) { - $mailer->setHTMLBody($html_body); - $body_limit -= strlen($html_body); + if (strlen($html_body)) { + // NOTE: We just drop the entire HTML body if it won't fit. Safely + // truncating HTML is hard, and we already have the text body to fall + // back to. + if (strlen($html_body) <= $body_limit) { + $mailer->setHTMLBody($html_body); + $body_limit -= strlen($html_body); + } } }