From 7622f6afccf44556b2c00deed9c033e5d36f6b7b Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 21 Apr 2018 09:08:13 -0700 Subject: [PATCH 01/12] Fix excessively severe CSP URI error during first-time setup Summary: See D19394. Currently, during first-time setup before you configure "phabricator.base-uri", we may attempt to generate a setup page, try to generate a CSP header for it, and fail to access the environmental config. This causes a too-severe error page ("configure phabricator.base-uri") instead of preflight guidance (like "can't connect to MySQL"). Instead, treat this more like "security.alternate-file-domain" and just bail on CSP if we can't fetch it. Test Plan: On a fresh (non-explodey laptop) install with critical setup errors (no MySQL installed yet), loaded Phabricator. Before: error about phabricator.base-uri. After: more helpful guidance about installing/configuring MySQL. Reviewers: amckinley, avivey Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D19396 --- src/aphront/response/AphrontResponse.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/aphront/response/AphrontResponse.php b/src/aphront/response/AphrontResponse.php index fe1e80318f..2e6513c7a8 100644 --- a/src/aphront/response/AphrontResponse.php +++ b/src/aphront/response/AphrontResponse.php @@ -113,6 +113,7 @@ abstract class AphrontResponse extends Phobject { try { $cdn = PhabricatorEnv::getEnvConfig('security.alternate-file-domain'); + $base_uri = PhabricatorEnv::getURI('/'); } catch (Exception $ex) { return null; } @@ -124,8 +125,6 @@ abstract class AphrontResponse extends Phobject { // If an alternate file domain is not configured and the user is viewing // a Phame blog on a custom domain or some other custom site, we'll still // serve resources from the main site. Include the main site explicitly. - - $base_uri = PhabricatorEnv::getURI('/'); $base_uri = $this->newContentSecurityPolicySource($base_uri); $default = "'self' {$base_uri}"; From 15e6e2adea61f27431b1c8a4d2a4552a89fed6b1 Mon Sep 17 00:00:00 2001 From: Aviv Eyal Date: Fri, 20 Apr 2018 21:42:18 -0700 Subject: [PATCH 02/12] Update install_ubuntu.sh to the new age Summary: Ref T4200. Since the last time this script was written, Ubuntu has made lots of changes. Try to keep up with those. Test Plan: Ran this on frash installs of Ubuntu 16.04 LTS and 18.04 LTS (Pre-release). Got to see Phabricator running. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: amckinley, Korvin Maniphest Tasks: T4200 Differential Revision: https://secure.phabricator.com/D19394 --- scripts/install/install_ubuntu.sh | 91 +++++++++++++++++++++---------- 1 file changed, 61 insertions(+), 30 deletions(-) diff --git a/scripts/install/install_ubuntu.sh b/scripts/install/install_ubuntu.sh index 5408cdb3e1..ab83c3a46c 100755 --- a/scripts/install/install_ubuntu.sh +++ b/scripts/install/install_ubuntu.sh @@ -5,9 +5,22 @@ confirm() { read -e ignored } -GIT='git' +INSTALL_URI=" https://phurl.io/u/install" + +failed() { + echo + echo + echo "Installation has failed." + echo "Text above this message might be useful to understanding what exactly failed." + echo + echo "Please follow this guide to manually complete installation:" + echo + echo $INSTALL_URI + echo + echo "We apologize for the inconvenience." + exit 3 +} -LTS="Ubuntu 10.04" ISSUE=`cat /etc/issue` if [[ $ISSUE != Ubuntu* ]] then @@ -15,20 +28,13 @@ then echo "to be something else. Your results may vary."; echo confirm -elif [[ `expr match "$ISSUE" "$LTS"` -eq ${#LTS} ]] -then - GIT='git-core' fi echo "PHABRICATOR UBUNTU INSTALL SCRIPT"; -echo "This script will install Phabricator and all of its core dependencies."; +echo "This script will install Apache, Phabricator and its core dependencies."; echo "Run it from the directory you want to install into."; echo -ROOT=`pwd` -echo "Phabricator will be installed to: ${ROOT}."; -confirm - echo "Testing sudo..." sudo true if [ $? -ne 0 ] @@ -37,31 +43,56 @@ then exit 1; fi; -echo "Installing dependencies: git, apache, mysql, php..."; -echo +echo 'Testing Ubuntu version...' -set +x +VERSION=`lsb_release -rs` +MAJOR=`expr match "$VERSION" '\([0-9]*\)'` -sudo apt-get -qq update -sudo apt-get install \ - $GIT mysql-server apache2 dpkg-dev \ - php5 php5-mysqlnd php5-gd php5-dev php5-curl php-apc php5-cli php5-json - -# Enable mod_rewrite -sudo a2enmod rewrite - -HAVEPCNTL=`php -r "echo extension_loaded('pcntl');"` -if [ $HAVEPCNTL != "1" ] +if [ "$MAJOR" -lt 16 ] then - echo "Installing pcntl..."; + echo 'This script is intented to install on modern operating systems; Your ' + echo 'operating system is too old for this script.' + echo 'You can still install Phabricator manually - please consult the installation' + echo 'guide to see how:' echo - apt-get source php5 - PHP5=`ls -1F | grep '^php5-.*/$'` - (cd $PHP5/ext/pcntl && phpize && ./configure && make && sudo make install) -else - echo "pcntl already installed"; + echo $INSTALL_URI + echo + exit 2 fi +# Ubuntu 16.04 LTS only has php 7.0 in their repos, so they need this extra ppa. +# Ubuntu 17.4 and up have official 7.2 builds. +if [ "$MAJOR" -eq 16 ] +then + echo 'This version of Ubuntu requires additional resources in order to install' + echo 'and run Phabricator.' + echo 'We will now add a the following package repository to your system:' + echo ' https://launchpad.net/~ondrej/+archive/ubuntu/php' + echo + echo 'This repository is generally considered safe to use.' + confirm + + sudo add-apt-repository -y ppa:ondrej/php || failed +fi + +ROOT=`pwd` +echo "Phabricator will be installed to: ${ROOT}."; +confirm + +echo "Installing dependencies: git, apache, mysql, php..."; +echo +sudo apt-get -qq update +sudo apt-get install \ + git mysql-server apache2 libapache2-mod-php \ + php php-mysql php-gd php-curl php-apcu php-cli php-json php-mbstring \ + || failed + +echo "Enabling mod_rewrite in Apache..." +echo +sudo a2enmod rewrite || failed + +echo "Downloading Phabricator and dependencies..." +echo if [ ! -e libphutil ] then git clone https://github.com/phacility/libphutil.git @@ -89,4 +120,4 @@ echo "Install probably worked mostly correctly. Continue with the 'Configuration echo echo " https://secure.phabricator.com/book/phabricator/article/configuration_guide/"; echo -echo "You can delete any php5-* stuff that's left over in this directory if you want."; +echo 'Next step is "Configuring Apache webserver".' From 16af0d35e5109b89a74175e93002e3a6d3745a6f Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 21 Apr 2018 13:00:54 -0700 Subject: [PATCH 03/12] In Differential, prevent "Accept" and "Reject" from "Plan Changes + Draft" Summary: Ref T13130. See PHI483. Currently, "Plan Changes + Draft" uses rules like "Plan Changes", not rules like "Draft", and allows "Accept". This isn't consistent with how "Draft" and "Accept" work in other cases. Make "Plan Changes + Draft" more like "Draft" for consistency. Also fix a string that didn't have a natural English version. Test Plan: - Added a failing build plan. - Created a revision. - Loaded the revision before builds completed, saw a nicer piece of text about "waiting for builds" instead of "waiting for 2 build(s)". - Builds failed, which automatically demoted the reivsion to "Changes Planned + Draft". - As the author and as a reviewer, verified all the actions available to me made sense (particularly, no "Accept"). - Abandoned the revision to test "Abandoned + Draft". - As the author and as a reviewer, verified all the actions available to me made sense. - Reclaimed the revision, then used "Request Review" to send it to "Needs Review". Verified that actions made sense and, e.g., reviewers could now "Accept" normally. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13130 Differential Revision: https://secure.phabricator.com/D19398 --- .../xaction/DifferentialRevisionAcceptTransaction.php | 2 +- .../xaction/DifferentialRevisionRejectTransaction.php | 2 +- .../translation/PhabricatorUSEnglishTranslation.php | 8 ++++++++ 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php b/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php index 190a6a4920..81544f08ff 100644 --- a/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php @@ -162,7 +162,7 @@ final class DifferentialRevisionAcceptTransaction 'closed. Only open revisions can be accepted.')); } - if ($object->isDraft()) { + if ($object->isDraft() || !$object->getShouldBroadcast()) { throw new Exception( pht('You can not accept a draft revision.')); } diff --git a/src/applications/differential/xaction/DifferentialRevisionRejectTransaction.php b/src/applications/differential/xaction/DifferentialRevisionRejectTransaction.php index a15ca0641b..78f88489ff 100644 --- a/src/applications/differential/xaction/DifferentialRevisionRejectTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionRejectTransaction.php @@ -73,7 +73,7 @@ final class DifferentialRevisionRejectTransaction 'not own.')); } - if ($object->isDraft()) { + if ($object->isDraft() || !$object->getShouldBroadcast()) { throw new Exception( pht('You can not request changes to a draft revision.')); } diff --git a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php b/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php index d33f042d0b..a1e7ae4eae 100644 --- a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php +++ b/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php @@ -1659,6 +1659,14 @@ final class PhabricatorUSEnglishTranslation 'you can no longer see were discarded.', ), + 'This draft revision will be sent for review once %s '. + 'build(s) pass: %s.' => array( + 'This draft revision will be sent for review once this build '. + 'passes: %2$s.', + 'This draft revision will be sent for review once these builds '. + 'pass: %2$s.', + ), + ); } From 1b24b486f58afaba7d254773f536f18e40381f8b Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 23 Apr 2018 13:03:59 -0700 Subject: [PATCH 04/12] Manage object mailKeys automatically in Mail instead of storing them on objects Summary: Ref T13065. `mailKey`s are a private secret for each object. In some mail configurations, they help us ensure that inbound mail is authentic: when we send you mail, the "Reply-To" is "T123+456+abcdef". - The `T123` is the object you're actually replying to. - The `456` is your user ID. - The `abcdef` is a hash of your user account with the `mailKey`. Knowing this hash effectively proves that Phabricator has sent you mail about the object before, i.e. that you legitimately control the account you're sending from. Without this, anyone could send mail to any object "From" someone else, and have comments post under their username. To generate this hash, we need a stable secret per object. (We can't use properties like the PHID because the secret has to be legitimately secret.) Today, we store these in `mailKey` properties on the actual objects, and manually generate them. This results in tons and tons and tons of copies of this same ~10 lines of code. Instead, just store them in the Mail application and generate them on demand. This change also anticipates possibly adding flags like "must encrypt" and "original subject", which are other "durable metadata about mail transmission" properties we may have use cases for eventually. Test Plan: - See next change for additional testing and context. - Sent mail about Herald rules (next change); saw mail keys generate cleanly. - Destroyed a Herald rule with a mail key, saw the mail properties get nuked. - Grepped for `getMailKey()` and converted all callsites I could which aren't the copy/pasted boilerplate present in 50 places. - Used `bin/mail receive-test --to T123` to test normal mail receipt of older-style objects and make sure that wasn't broken. Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13065 Differential Revision: https://secure.phabricator.com/D19399 --- .../20180423.mail.01.properties.sql | 8 ++ src/__phutil_library_map__.php | 9 ++ .../auth/storage/PhabricatorAuthSSHKey.php | 6 -- ...ilPropertiesDestructionEngineExtension.php | 28 ++++++ ...catorMailManagementReceiveTestWorkflow.php | 4 +- .../PhabricatorMetaMTAMailPropertiesQuery.php | 51 +++++++++++ .../PhabricatorObjectMailReceiver.php | 3 +- .../PhabricatorObjectMailReceiverTestCase.php | 5 +- .../PhabricatorMailReplyHandler.php | 10 +- .../PhabricatorMetaMTAMailProperties.php | 91 +++++++++++++++++++ 10 files changed, 204 insertions(+), 11 deletions(-) create mode 100644 resources/sql/autopatches/20180423.mail.01.properties.sql create mode 100644 src/applications/metamta/engineextension/PhabricatorMailPropertiesDestructionEngineExtension.php create mode 100644 src/applications/metamta/query/PhabricatorMetaMTAMailPropertiesQuery.php create mode 100644 src/applications/metamta/storage/PhabricatorMetaMTAMailProperties.php diff --git a/resources/sql/autopatches/20180423.mail.01.properties.sql b/resources/sql/autopatches/20180423.mail.01.properties.sql new file mode 100644 index 0000000000..d4fc008023 --- /dev/null +++ b/resources/sql/autopatches/20180423.mail.01.properties.sql @@ -0,0 +1,8 @@ +CREATE TABLE {$NAMESPACE}_metamta.metamta_mailproperties ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + objectPHID VARBINARY(64) NOT NULL, + mailProperties LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT}, + dateCreated INT UNSIGNED NOT NULL, + dateModified INT UNSIGNED NOT NULL, + UNIQUE KEY `key_object` (objectPHID) +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index beec5058a6..4ab77d5207 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3346,6 +3346,7 @@ phutil_register_library_map(array( 'PhabricatorMailOutboundRoutingSelfEmailHeraldAction' => 'applications/metamta/herald/PhabricatorMailOutboundRoutingSelfEmailHeraldAction.php', 'PhabricatorMailOutboundRoutingSelfNotificationHeraldAction' => 'applications/metamta/herald/PhabricatorMailOutboundRoutingSelfNotificationHeraldAction.php', 'PhabricatorMailOutboundStatus' => 'applications/metamta/constants/PhabricatorMailOutboundStatus.php', + 'PhabricatorMailPropertiesDestructionEngineExtension' => 'applications/metamta/engineextension/PhabricatorMailPropertiesDestructionEngineExtension.php', 'PhabricatorMailReceiver' => 'applications/metamta/receiver/PhabricatorMailReceiver.php', 'PhabricatorMailReceiverTestCase' => 'applications/metamta/receiver/__tests__/PhabricatorMailReceiverTestCase.php', 'PhabricatorMailReplyHandler' => 'applications/metamta/replyhandler/PhabricatorMailReplyHandler.php', @@ -3403,6 +3404,8 @@ phutil_register_library_map(array( 'PhabricatorMetaMTAMailHasRecipientEdgeType' => 'applications/metamta/edge/PhabricatorMetaMTAMailHasRecipientEdgeType.php', 'PhabricatorMetaMTAMailListController' => 'applications/metamta/controller/PhabricatorMetaMTAMailListController.php', 'PhabricatorMetaMTAMailPHIDType' => 'applications/metamta/phid/PhabricatorMetaMTAMailPHIDType.php', + 'PhabricatorMetaMTAMailProperties' => 'applications/metamta/storage/PhabricatorMetaMTAMailProperties.php', + 'PhabricatorMetaMTAMailPropertiesQuery' => 'applications/metamta/query/PhabricatorMetaMTAMailPropertiesQuery.php', 'PhabricatorMetaMTAMailQuery' => 'applications/metamta/query/PhabricatorMetaMTAMailQuery.php', 'PhabricatorMetaMTAMailSearchEngine' => 'applications/metamta/query/PhabricatorMetaMTAMailSearchEngine.php', 'PhabricatorMetaMTAMailSection' => 'applications/metamta/view/PhabricatorMetaMTAMailSection.php', @@ -9038,6 +9041,7 @@ phutil_register_library_map(array( 'PhabricatorMailOutboundRoutingSelfEmailHeraldAction' => 'PhabricatorMailOutboundRoutingHeraldAction', 'PhabricatorMailOutboundRoutingSelfNotificationHeraldAction' => 'PhabricatorMailOutboundRoutingHeraldAction', 'PhabricatorMailOutboundStatus' => 'Phobject', + 'PhabricatorMailPropertiesDestructionEngineExtension' => 'PhabricatorDestructionEngineExtension', 'PhabricatorMailReceiver' => 'Phobject', 'PhabricatorMailReceiverTestCase' => 'PhabricatorTestCase', 'PhabricatorMailReplyHandler' => 'Phobject', @@ -9106,6 +9110,11 @@ phutil_register_library_map(array( 'PhabricatorMetaMTAMailHasRecipientEdgeType' => 'PhabricatorEdgeType', 'PhabricatorMetaMTAMailListController' => 'PhabricatorMetaMTAController', 'PhabricatorMetaMTAMailPHIDType' => 'PhabricatorPHIDType', + 'PhabricatorMetaMTAMailProperties' => array( + 'PhabricatorMetaMTADAO', + 'PhabricatorPolicyInterface', + ), + 'PhabricatorMetaMTAMailPropertiesQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorMetaMTAMailQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorMetaMTAMailSearchEngine' => 'PhabricatorApplicationSearchEngine', 'PhabricatorMetaMTAMailSection' => 'Phobject', diff --git a/src/applications/auth/storage/PhabricatorAuthSSHKey.php b/src/applications/auth/storage/PhabricatorAuthSSHKey.php index 2a9a6273bf..5bbb7de834 100644 --- a/src/applications/auth/storage/PhabricatorAuthSSHKey.php +++ b/src/applications/auth/storage/PhabricatorAuthSSHKey.php @@ -70,12 +70,6 @@ final class PhabricatorAuthSSHKey return parent::save(); } - public function getMailKey() { - // NOTE: We don't actually receive mail for these objects. It's OK for - // the mail key to be predictable until we do. - return PhabricatorHash::digestForIndex($this->getPHID()); - } - public function toPublicKey() { return PhabricatorAuthSSHPublicKey::newFromStoredKey($this); } diff --git a/src/applications/metamta/engineextension/PhabricatorMailPropertiesDestructionEngineExtension.php b/src/applications/metamta/engineextension/PhabricatorMailPropertiesDestructionEngineExtension.php new file mode 100644 index 0000000000..4e9e0fe38f --- /dev/null +++ b/src/applications/metamta/engineextension/PhabricatorMailPropertiesDestructionEngineExtension.php @@ -0,0 +1,28 @@ +getPHID(); + $viewer = $engine->getViewer(); + + $properties = id(new PhabricatorMetaMTAMailPropertiesQuery()) + ->setViewer($viewer) + ->withObjectPHIDs(array($object_phid)) + ->executeOne(); + if ($properties) { + $properties->delete(); + } + } + +} diff --git a/src/applications/metamta/management/PhabricatorMailManagementReceiveTestWorkflow.php b/src/applications/metamta/management/PhabricatorMailManagementReceiveTestWorkflow.php index 3cbe2345eb..46c444571b 100644 --- a/src/applications/metamta/management/PhabricatorMailManagementReceiveTestWorkflow.php +++ b/src/applications/metamta/management/PhabricatorMailManagementReceiveTestWorkflow.php @@ -139,8 +139,10 @@ final class PhabricatorMailManagementReceiveTestWorkflow throw new Exception(pht("No such object '%s'!", $to)); } + $mail_key = PhabricatorMetaMTAMailProperties::loadMailKey($object); + $hash = PhabricatorObjectMailReceiver::computeMailHash( - $object->getMailKey(), + $mail_key, $user->getPHID()); $header_content['to'] = $to.'+'.$user->getID().'+'.$hash.'@test.com'; diff --git a/src/applications/metamta/query/PhabricatorMetaMTAMailPropertiesQuery.php b/src/applications/metamta/query/PhabricatorMetaMTAMailPropertiesQuery.php new file mode 100644 index 0000000000..e6ec09f4ff --- /dev/null +++ b/src/applications/metamta/query/PhabricatorMetaMTAMailPropertiesQuery.php @@ -0,0 +1,51 @@ +ids = $ids; + return $this; + } + + public function withObjectPHIDs(array $object_phids) { + $this->objectPHIDs = $object_phids; + return $this; + } + + public function newResultObject() { + return new PhabricatorMetaMTAMailProperties(); + } + + protected function loadPage() { + return $this->loadStandardPage($this->newResultObject()); + } + + protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { + $where = parent::buildWhereClauseParts($conn); + + if ($this->ids !== null) { + $where[] = qsprintf( + $conn, + 'id IN (%Ld)', + $this->ids); + } + + if ($this->objectPHIDs !== null) { + $where[] = qsprintf( + $conn, + 'objectPHID IN (%Ls)', + $this->objectPHIDs); + } + + return $where; + } + + public function getQueryApplicationClass() { + return 'PhabricatorMetaMTAApplication'; + } + +} diff --git a/src/applications/metamta/receiver/PhabricatorObjectMailReceiver.php b/src/applications/metamta/receiver/PhabricatorObjectMailReceiver.php index 5985458ab4..801f7a8e6f 100644 --- a/src/applications/metamta/receiver/PhabricatorObjectMailReceiver.php +++ b/src/applications/metamta/receiver/PhabricatorObjectMailReceiver.php @@ -124,7 +124,8 @@ abstract class PhabricatorObjectMailReceiver extends PhabricatorMailReceiver { $check_phid = $sender->getPHID(); } - $expect_hash = self::computeMailHash($object->getMailKey(), $check_phid); + $mail_key = PhabricatorMetaMTAMailProperties::loadMailKey($object); + $expect_hash = self::computeMailHash($mail_key, $check_phid); if (!phutil_hashes_are_identical($expect_hash, $parts['hash'])) { throw new PhabricatorMetaMTAReceivedMailProcessingException( diff --git a/src/applications/metamta/receiver/__tests__/PhabricatorObjectMailReceiverTestCase.php b/src/applications/metamta/receiver/__tests__/PhabricatorObjectMailReceiverTestCase.php index 2c2e0561cf..a697436d21 100644 --- a/src/applications/metamta/receiver/__tests__/PhabricatorObjectMailReceiverTestCase.php +++ b/src/applications/metamta/receiver/__tests__/PhabricatorObjectMailReceiverTestCase.php @@ -98,8 +98,11 @@ final class PhabricatorObjectMailReceiverTestCase if ($is_bad_hash) { $hash = PhabricatorObjectMailReceiver::computeMailHash('x', 'y'); } else { + + $mail_key = PhabricatorMetaMTAMailProperties::loadMailKey($task); + $hash = PhabricatorObjectMailReceiver::computeMailHash( - $task->getMailKey(), + $mail_key, $is_public ? $task->getPHID() : $user->getPHID()); } diff --git a/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php b/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php index f8dd784e3b..70b524b52f 100644 --- a/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php +++ b/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php @@ -136,8 +136,11 @@ abstract class PhabricatorMailReplyHandler extends Phobject { // We compute a hash using the object's own PHID to prevent an attacker // from blindly interacting with objects that they haven't ever received // mail about by just sending to D1@, D2@, etc... + + $mail_key = PhabricatorMetaMTAMailProperties::loadMailKey($receiver); + $hash = PhabricatorObjectMailReceiver::computeMailHash( - $receiver->getMailKey(), + $mail_key, $receiver->getPHID()); $address = "{$prefix}{$receiver_id}+public+{$hash}@{$domain}"; @@ -159,8 +162,11 @@ abstract class PhabricatorMailReplyHandler extends Phobject { $receiver = $this->getMailReceiver(); $receiver_id = $receiver->getID(); $user_id = $user->getID(); + + $mail_key = PhabricatorMetaMTAMailProperties::loadMailKey($receiver); + $hash = PhabricatorObjectMailReceiver::computeMailHash( - $receiver->getMailKey(), + $mail_key, $user->getPHID()); $domain = $this->getReplyHandlerDomain(); diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAMailProperties.php b/src/applications/metamta/storage/PhabricatorMetaMTAMailProperties.php new file mode 100644 index 0000000000..eb454b650e --- /dev/null +++ b/src/applications/metamta/storage/PhabricatorMetaMTAMailProperties.php @@ -0,0 +1,91 @@ + array( + 'mailProperties' => self::SERIALIZATION_JSON, + ), + self::CONFIG_COLUMN_SCHEMA => array(), + self::CONFIG_KEY_SCHEMA => array( + 'key_object' => array( + 'columns' => array('objectPHID'), + 'unique' => true, + ), + ), + ) + parent::getConfiguration(); + } + + public function getMailProperty($key, $default = null) { + return idx($this->mailProperties, $key, $default); + } + + public function setMailProperty($key, $value) { + $this->mailProperties[$key] = $value; + return $this; + } + + public static function loadMailKey($object) { + // If this is an older object with an onboard "mailKey" property, just + // use it. + // TODO: We should eventually get rid of these and get rid of this + // piece of code. + if ($object->hasProperty('mailKey')) { + return $object->getMailKey(); + } + + $viewer = PhabricatorUser::getOmnipotentUser(); + $object_phid = $object->getPHID(); + + $properties = id(new PhabricatorMetaMTAMailPropertiesQuery()) + ->setViewer($viewer) + ->withObjectPHIDs(array($object_phid)) + ->executeOne(); + if (!$properties) { + $properties = id(new self()) + ->setObjectPHID($object_phid); + } + + $mail_key = $properties->getMailProperty('mailKey'); + if ($mail_key !== null) { + return $mail_key; + } + + $mail_key = Filesystem::readRandomCharacters(20); + $properties->setMailProperty('mailKey', $mail_key); + + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + $properties->save(); + unset($unguarded); + + return $mail_key; + } + + +/* -( PhabricatorPolicyInterface )----------------------------------------- */ + + + public function getCapabilities() { + return array( + PhabricatorPolicyCapability::CAN_VIEW, + ); + } + + public function getPolicy($capability) { + switch ($capability) { + case PhabricatorPolicyCapability::CAN_VIEW: + return PhabricatorPolicies::POLICY_NOONE; + } + } + + public function hasAutomaticCapability($capability, PhabricatorUser $viewer) { + return false; + } + +} From cac41d1e48580a584152f9e6b54154ad702e771d Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 22 Apr 2018 19:57:22 -0700 Subject: [PATCH 05/12] Support Herald rules for Herald rules Summary: Depends on D19399. Ref T13130. This adds basic support for writing Herald rules against Herald rules. See T13130 for a lot more detail. This needs a bit more work to be useful: for example, there's no way to specify the rule type or subject, so you can't say "notify me when global rules are edited" or "notify me when Maniphest rules are edited". I'll add some fields for that in followup changes to actually solve the original use case. Test Plan: - Wrote Herald rules against Herald rules. - Ran them by editing rules and in the test console. - Verified they sent some mail with `bin/mail list-outbound`. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13130 Differential Revision: https://secure.phabricator.com/D19400 --- src/__phutil_library_map__.php | 4 + .../herald/adapter/HeraldRuleAdapter.php | 74 +++++++++++++++++++ .../herald/editor/HeraldRuleEditor.php | 50 +++++++++++++ .../herald/mail/HeraldRuleReplyHandler.php | 16 ++++ 4 files changed, 144 insertions(+) create mode 100644 src/applications/herald/adapter/HeraldRuleAdapter.php create mode 100644 src/applications/herald/mail/HeraldRuleReplyHandler.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 4ab77d5207..cccd82e882 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1485,12 +1485,14 @@ phutil_register_library_map(array( 'HeraldRemarkupFieldValue' => 'applications/herald/value/HeraldRemarkupFieldValue.php', 'HeraldRemarkupRule' => 'applications/herald/remarkup/HeraldRemarkupRule.php', 'HeraldRule' => 'applications/herald/storage/HeraldRule.php', + 'HeraldRuleAdapter' => 'applications/herald/adapter/HeraldRuleAdapter.php', 'HeraldRuleController' => 'applications/herald/controller/HeraldRuleController.php', 'HeraldRuleDatasource' => 'applications/herald/typeahead/HeraldRuleDatasource.php', 'HeraldRuleEditor' => 'applications/herald/editor/HeraldRuleEditor.php', 'HeraldRuleListController' => 'applications/herald/controller/HeraldRuleListController.php', 'HeraldRulePHIDType' => 'applications/herald/phid/HeraldRulePHIDType.php', 'HeraldRuleQuery' => 'applications/herald/query/HeraldRuleQuery.php', + 'HeraldRuleReplyHandler' => 'applications/herald/mail/HeraldRuleReplyHandler.php', 'HeraldRuleSearchEngine' => 'applications/herald/query/HeraldRuleSearchEngine.php', 'HeraldRuleSerializer' => 'applications/herald/editor/HeraldRuleSerializer.php', 'HeraldRuleTestCase' => 'applications/herald/storage/__tests__/HeraldRuleTestCase.php', @@ -6918,12 +6920,14 @@ phutil_register_library_map(array( 'PhabricatorDestructibleInterface', 'PhabricatorSubscribableInterface', ), + 'HeraldRuleAdapter' => 'HeraldAdapter', 'HeraldRuleController' => 'HeraldController', 'HeraldRuleDatasource' => 'PhabricatorTypeaheadDatasource', 'HeraldRuleEditor' => 'PhabricatorApplicationTransactionEditor', 'HeraldRuleListController' => 'HeraldController', 'HeraldRulePHIDType' => 'PhabricatorPHIDType', 'HeraldRuleQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', + 'HeraldRuleReplyHandler' => 'PhabricatorApplicationTransactionReplyHandler', 'HeraldRuleSearchEngine' => 'PhabricatorApplicationSearchEngine', 'HeraldRuleSerializer' => 'Phobject', 'HeraldRuleTestCase' => 'PhabricatorTestCase', diff --git a/src/applications/herald/adapter/HeraldRuleAdapter.php b/src/applications/herald/adapter/HeraldRuleAdapter.php new file mode 100644 index 0000000000..8ed851a1a9 --- /dev/null +++ b/src/applications/herald/adapter/HeraldRuleAdapter.php @@ -0,0 +1,74 @@ +rule = $this->newObject(); + } + + public function supportsApplicationEmail() { + return true; + } + + public function supportsRuleType($rule_type) { + switch ($rule_type) { + case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL: + case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL: + return true; + case HeraldRuleTypeConfig::RULE_TYPE_OBJECT: + default: + return false; + } + } + + public function setRule(HeraldRule $rule) { + $this->rule = $rule; + return $this; + } + + public function getRule() { + return $this->rule; + } + + public function setObject($object) { + $this->rule = $object; + return $this; + } + + public function getObject() { + return $this->rule; + } + + public function getAdapterContentName() { + return pht('Herald Rules'); + } + + public function getHeraldName() { + return $this->getRule()->getMonogram(); + } + +} diff --git a/src/applications/herald/editor/HeraldRuleEditor.php b/src/applications/herald/editor/HeraldRuleEditor.php index 30480108f4..8c83ef6597 100644 --- a/src/applications/herald/editor/HeraldRuleEditor.php +++ b/src/applications/herald/editor/HeraldRuleEditor.php @@ -87,4 +87,54 @@ final class HeraldRuleEditor return; } + protected function shouldApplyHeraldRules( + PhabricatorLiskDAO $object, + array $xactions) { + return true; + } + + protected function buildHeraldAdapter( + PhabricatorLiskDAO $object, + array $xactions) { + return id(new HeraldRuleAdapter()) + ->setRule($object); + } + + protected function shouldSendMail( + PhabricatorLiskDAO $object, + array $xactions) { + return true; + } + + protected function getMailTo(PhabricatorLiskDAO $object) { + $phids = array(); + + $phids[] = $this->getActingAsPHID(); + + if ($object->isPersonalRule()) { + $phids[] = $object->getAuthorPHID(); + } + + return $phids; + } + + protected function buildReplyHandler(PhabricatorLiskDAO $object) { + return id(new HeraldRuleReplyHandler()) + ->setMailReceiver($object); + } + + protected function buildMailTemplate(PhabricatorLiskDAO $object) { + $monogram = $object->getMonogram(); + $name = $object->getName(); + + $subject = pht('%s: %s', $monogram, $name); + + return id(new PhabricatorMetaMTAMail()) + ->setSubject($subject); + } + + protected function getMailSubjectPrefix() { + return pht('[Herald]'); + } + } diff --git a/src/applications/herald/mail/HeraldRuleReplyHandler.php b/src/applications/herald/mail/HeraldRuleReplyHandler.php new file mode 100644 index 0000000000..c4bf9fad44 --- /dev/null +++ b/src/applications/herald/mail/HeraldRuleReplyHandler.php @@ -0,0 +1,16 @@ + Date: Tue, 24 Apr 2018 15:55:56 -0700 Subject: [PATCH 06/12] Fix a typo in the Diffusion importing user guide Summary: This word isn't the right word. Test Plan: Reading? Reviewers: avivey, amckinley Reviewed By: avivey Differential Revision: https://secure.phabricator.com/D19404 --- src/docs/user/userguide/diffusion_existing.diviner | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/docs/user/userguide/diffusion_existing.diviner b/src/docs/user/userguide/diffusion_existing.diviner index bf09d9ef1f..341146f0fd 100644 --- a/src/docs/user/userguide/diffusion_existing.diviner +++ b/src/docs/user/userguide/diffusion_existing.diviner @@ -47,7 +47,7 @@ There are two primary ways to import an existing repository: Once the import completes, change the "I/O Type" on the **Observe** URI to "No I/O" mode to automatically convert it into a hosted repository. -**Push to Empty Repository**: Create an activate an empty repository, then push +**Push to Empty Repository**: Create and activate an empty repository, then push all of your changes to the empty repository. In Git and Mercurial, you can do this with `git push` or `hg push`. From b4796d28374c604e9e3cb94877249afebe60b0a0 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 24 Apr 2018 15:31:18 -0700 Subject: [PATCH 07/12] Add "Content type" and "Rule type" fields to Herald rules for Herald rules Summary: Depends on D19400. Ref T13130. Currently, when you write Herald rules about other Herald rules, you can't pick a rule type or content type, so there's no way to get notified about edits to just global rules (which is the primary driving use case). Add a "Content type" field to let the rule match rules that affect revisions, tasks, commits, etc. Add a "Rule type" field to let the rule match global, personal, or object rules. Test Plan: - Wrote a global rule for other rules about global Herald rules: {F5540307} {F5540308} - Ran it against itself which matched: {F5540309} - Ran it against another rule (not a global rule about Herald rules), which did not match: {F5540311} - Also reviewed the fields in those transcripts in more detail to make sure they were extracting matching correctly. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13130 Differential Revision: https://secure.phabricator.com/D19403 --- src/__phutil_library_map__.php | 12 +++++ .../field/rule/HeraldRuleAdapterField.php | 29 ++++++++++++ .../herald/field/rule/HeraldRuleField.php | 14 ++++++ .../field/rule/HeraldRuleFieldGroup.php | 16 +++++++ .../herald/field/rule/HeraldRuleTypeField.php | 28 ++++++++++++ .../typeahead/HeraldAdapterDatasource.php | 45 +++++++++++++++++++ .../typeahead/HeraldRuleTypeDatasource.php | 42 +++++++++++++++++ 7 files changed, 186 insertions(+) create mode 100644 src/applications/herald/field/rule/HeraldRuleAdapterField.php create mode 100644 src/applications/herald/field/rule/HeraldRuleField.php create mode 100644 src/applications/herald/field/rule/HeraldRuleFieldGroup.php create mode 100644 src/applications/herald/field/rule/HeraldRuleTypeField.php create mode 100644 src/applications/herald/typeahead/HeraldAdapterDatasource.php create mode 100644 src/applications/herald/typeahead/HeraldRuleTypeDatasource.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index cccd82e882..9de57b2530 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1430,6 +1430,7 @@ phutil_register_library_map(array( 'HeraldActionGroup' => 'applications/herald/action/HeraldActionGroup.php', 'HeraldActionRecord' => 'applications/herald/storage/HeraldActionRecord.php', 'HeraldAdapter' => 'applications/herald/adapter/HeraldAdapter.php', + 'HeraldAdapterDatasource' => 'applications/herald/typeahead/HeraldAdapterDatasource.php', 'HeraldAlwaysField' => 'applications/herald/field/HeraldAlwaysField.php', 'HeraldAnotherRuleField' => 'applications/herald/field/HeraldAnotherRuleField.php', 'HeraldApplicationActionGroup' => 'applications/herald/action/HeraldApplicationActionGroup.php', @@ -1486,9 +1487,12 @@ phutil_register_library_map(array( 'HeraldRemarkupRule' => 'applications/herald/remarkup/HeraldRemarkupRule.php', 'HeraldRule' => 'applications/herald/storage/HeraldRule.php', 'HeraldRuleAdapter' => 'applications/herald/adapter/HeraldRuleAdapter.php', + 'HeraldRuleAdapterField' => 'applications/herald/field/rule/HeraldRuleAdapterField.php', 'HeraldRuleController' => 'applications/herald/controller/HeraldRuleController.php', 'HeraldRuleDatasource' => 'applications/herald/typeahead/HeraldRuleDatasource.php', 'HeraldRuleEditor' => 'applications/herald/editor/HeraldRuleEditor.php', + 'HeraldRuleField' => 'applications/herald/field/rule/HeraldRuleField.php', + 'HeraldRuleFieldGroup' => 'applications/herald/field/rule/HeraldRuleFieldGroup.php', 'HeraldRuleListController' => 'applications/herald/controller/HeraldRuleListController.php', 'HeraldRulePHIDType' => 'applications/herald/phid/HeraldRulePHIDType.php', 'HeraldRuleQuery' => 'applications/herald/query/HeraldRuleQuery.php', @@ -1500,6 +1504,8 @@ phutil_register_library_map(array( 'HeraldRuleTransactionComment' => 'applications/herald/storage/HeraldRuleTransactionComment.php', 'HeraldRuleTranscript' => 'applications/herald/storage/transcript/HeraldRuleTranscript.php', 'HeraldRuleTypeConfig' => 'applications/herald/config/HeraldRuleTypeConfig.php', + 'HeraldRuleTypeDatasource' => 'applications/herald/typeahead/HeraldRuleTypeDatasource.php', + 'HeraldRuleTypeField' => 'applications/herald/field/rule/HeraldRuleTypeField.php', 'HeraldRuleViewController' => 'applications/herald/controller/HeraldRuleViewController.php', 'HeraldSchemaSpec' => 'applications/herald/storage/HeraldSchemaSpec.php', 'HeraldSelectFieldValue' => 'applications/herald/value/HeraldSelectFieldValue.php', @@ -6852,6 +6858,7 @@ phutil_register_library_map(array( 'HeraldActionGroup' => 'HeraldGroup', 'HeraldActionRecord' => 'HeraldDAO', 'HeraldAdapter' => 'Phobject', + 'HeraldAdapterDatasource' => 'PhabricatorTypeaheadDatasource', 'HeraldAlwaysField' => 'HeraldField', 'HeraldAnotherRuleField' => 'HeraldField', 'HeraldApplicationActionGroup' => 'HeraldActionGroup', @@ -6921,9 +6928,12 @@ phutil_register_library_map(array( 'PhabricatorSubscribableInterface', ), 'HeraldRuleAdapter' => 'HeraldAdapter', + 'HeraldRuleAdapterField' => 'HeraldRuleField', 'HeraldRuleController' => 'HeraldController', 'HeraldRuleDatasource' => 'PhabricatorTypeaheadDatasource', 'HeraldRuleEditor' => 'PhabricatorApplicationTransactionEditor', + 'HeraldRuleField' => 'HeraldField', + 'HeraldRuleFieldGroup' => 'HeraldFieldGroup', 'HeraldRuleListController' => 'HeraldController', 'HeraldRulePHIDType' => 'PhabricatorPHIDType', 'HeraldRuleQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', @@ -6935,6 +6945,8 @@ phutil_register_library_map(array( 'HeraldRuleTransactionComment' => 'PhabricatorApplicationTransactionComment', 'HeraldRuleTranscript' => 'Phobject', 'HeraldRuleTypeConfig' => 'Phobject', + 'HeraldRuleTypeDatasource' => 'PhabricatorTypeaheadDatasource', + 'HeraldRuleTypeField' => 'HeraldRuleField', 'HeraldRuleViewController' => 'HeraldController', 'HeraldSchemaSpec' => 'PhabricatorConfigSchemaSpec', 'HeraldSelectFieldValue' => 'HeraldFieldValue', diff --git a/src/applications/herald/field/rule/HeraldRuleAdapterField.php b/src/applications/herald/field/rule/HeraldRuleAdapterField.php new file mode 100644 index 0000000000..bcb57f520f --- /dev/null +++ b/src/applications/herald/field/rule/HeraldRuleAdapterField.php @@ -0,0 +1,29 @@ +getContentType(); + } + + protected function getHeraldFieldStandardType() { + return self::STANDARD_PHID; + } + + protected function getDatasource() { + return new HeraldAdapterDatasource(); + } + + protected function getDatasourceValueMap() { + $adapters = HeraldAdapter::getAllAdapters(); + return mpull($adapters, 'getAdapterContentName', 'getAdapterContentType'); + } + +} diff --git a/src/applications/herald/field/rule/HeraldRuleField.php b/src/applications/herald/field/rule/HeraldRuleField.php new file mode 100644 index 0000000000..00ade268b3 --- /dev/null +++ b/src/applications/herald/field/rule/HeraldRuleField.php @@ -0,0 +1,14 @@ +getRuleType(); + } + + protected function getHeraldFieldStandardType() { + return self::STANDARD_PHID; + } + + protected function getDatasource() { + return new HeraldRuleTypeDatasource(); + } + + protected function getDatasourceValueMap() { + return HeraldRuleTypeConfig::getRuleTypeMap(); + } + +} diff --git a/src/applications/herald/typeahead/HeraldAdapterDatasource.php b/src/applications/herald/typeahead/HeraldAdapterDatasource.php new file mode 100644 index 0000000000..1fffd1bb6b --- /dev/null +++ b/src/applications/herald/typeahead/HeraldAdapterDatasource.php @@ -0,0 +1,45 @@ +buildResults(); + return $this->filterResultsAgainstTokens($results); + } + + protected function renderSpecialTokens(array $values) { + return $this->renderTokensFromResults($this->buildResults(), $values); + } + + private function buildResults() { + $results = array(); + + $adapters = HeraldAdapter::getAllAdapters(); + foreach ($adapters as $adapter) { + $value = $adapter->getAdapterContentType(); + $name = $adapter->getAdapterContentName(); + + $result = id(new PhabricatorTypeaheadResult()) + ->setPHID($value) + ->setName($name); + + $results[$value] = $result; + } + + return $results; + } + +} diff --git a/src/applications/herald/typeahead/HeraldRuleTypeDatasource.php b/src/applications/herald/typeahead/HeraldRuleTypeDatasource.php new file mode 100644 index 0000000000..8dfa7d0f6a --- /dev/null +++ b/src/applications/herald/typeahead/HeraldRuleTypeDatasource.php @@ -0,0 +1,42 @@ +buildResults(); + return $this->filterResultsAgainstTokens($results); + } + + protected function renderSpecialTokens(array $values) { + return $this->renderTokensFromResults($this->buildResults(), $values); + } + + private function buildResults() { + $results = array(); + + $type_map = HeraldRuleTypeConfig::getRuleTypeMap(); + foreach ($type_map as $type => $name) { + $result = id(new PhabricatorTypeaheadResult()) + ->setPHID($type) + ->setName($name); + + $results[$type] = $result; + } + + return $results; + } + +} From 9f8e0ad473a653e38c850b9314bf32fcfcc8bd23 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 25 Apr 2018 06:17:17 -0700 Subject: [PATCH 08/12] Remove unusual unicode marks in Differential action dropdown Summary: See . Currently, the action dropdown in Differential shows a heavy "X" after "Request Changes" and a heavy checkmark after "Accept Revision". Although I'm not convinced that the messaging around "Request Changes" is too strong, I do think these marks are out of place in modern Differential. They came from a simpler time when this dropdown had fewer actions, but feel a little weird and inconsistent to me in the modern UI. Let's try getting rid of them and see how it goes? Test Plan: - Viewed these actions in the dropdown, no longer saw the mark icons. - Grepped for these unicode sequences without getting any other hits. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D19405 --- .../xaction/DifferentialRevisionAcceptTransaction.php | 2 +- .../xaction/DifferentialRevisionRejectTransaction.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php b/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php index 81544f08ff..42cd798584 100644 --- a/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php @@ -7,7 +7,7 @@ final class DifferentialRevisionAcceptTransaction const ACTIONKEY = 'accept'; protected function getRevisionActionLabel() { - return pht("Accept Revision \xE2\x9C\x94"); + return pht('Accept Revision'); } protected function getRevisionActionDescription( diff --git a/src/applications/differential/xaction/DifferentialRevisionRejectTransaction.php b/src/applications/differential/xaction/DifferentialRevisionRejectTransaction.php index 78f88489ff..0ed6db96bf 100644 --- a/src/applications/differential/xaction/DifferentialRevisionRejectTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionRejectTransaction.php @@ -7,7 +7,7 @@ final class DifferentialRevisionRejectTransaction const ACTIONKEY = 'reject'; protected function getRevisionActionLabel() { - return pht("Request Changes \xE2\x9C\x98"); + return pht('Request Changes'); } protected function getRevisionActionDescription( From 28517110c6cb41d95ba52b4ac2021e9a06baf8e2 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 27 Apr 2018 10:58:28 -0700 Subject: [PATCH 09/12] Fix an issue in the new Harbormaster build log view where clicking the "^" icon doesn't work right Summary: Ref T13130. See PHI617. The new build log UI has tags like `Show More Above ^`. If you click the little "^" icon, the event target is the `` instead of the `` so we expand on the wrong node. Instead, select the `` by sigil explicitly. Test Plan: Viewed new log UI in Harbormaster, clicked "^" icon and text, got the same (correct) behavior on both. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13130 Differential Revision: https://secure.phabricator.com/D19410 --- resources/celerity/map.php | 10 +++++----- .../harbormaster/behavior-harbormaster-log.js | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index dd4d978fed..480c4b602d 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -391,7 +391,7 @@ return array( 'rsrc/js/application/files/behavior-document-engine.js' => 'ee0deff8', 'rsrc/js/application/files/behavior-icon-composer.js' => '8499b6ab', 'rsrc/js/application/files/behavior-launch-icon-composer.js' => '48086888', - 'rsrc/js/application/harbormaster/behavior-harbormaster-log.js' => '191b4909', + 'rsrc/js/application/harbormaster/behavior-harbormaster-log.js' => '549459b8', 'rsrc/js/application/herald/HeraldRuleEditor.js' => 'dca75c0e', 'rsrc/js/application/herald/PathTypeahead.js' => '662e9cea', 'rsrc/js/application/herald/herald-rule-editor.js' => '7ebaeed3', @@ -609,7 +609,7 @@ return array( 'javelin-behavior-event-all-day' => 'b41537c9', 'javelin-behavior-fancy-datepicker' => 'ecf4e799', 'javelin-behavior-global-drag-and-drop' => '960f6a39', - 'javelin-behavior-harbormaster-log' => '191b4909', + 'javelin-behavior-harbormaster-log' => '549459b8', 'javelin-behavior-herald-rule-editor' => '7ebaeed3', 'javelin-behavior-high-security-warning' => 'a464fe03', 'javelin-behavior-history-install' => '7ee2b591', @@ -960,9 +960,6 @@ return array( '185bbd53' => array( 'javelin-install', ), - '191b4909' => array( - 'javelin-behavior', - ), '1ad0a787' => array( 'javelin-install', 'javelin-reactor', @@ -1257,6 +1254,9 @@ return array( 'javelin-vector', 'javelin-typeahead-static-source', ), + '549459b8' => array( + 'javelin-behavior', + ), '54b612ba' => array( 'javelin-color', 'javelin-install', diff --git a/webroot/rsrc/js/application/harbormaster/behavior-harbormaster-log.js b/webroot/rsrc/js/application/harbormaster/behavior-harbormaster-log.js index 5bff2c1bb0..db60c47055 100644 --- a/webroot/rsrc/js/application/harbormaster/behavior-harbormaster-log.js +++ b/webroot/rsrc/js/application/harbormaster/behavior-harbormaster-log.js @@ -16,7 +16,7 @@ JX.behavior('harbormaster-log', function(config) { e.kill(); - expand(e.getTarget(), true); + expand(e.getNode('harbormaster-log-expand'), true); }); function expand(node, is_action) { From 223d7b84dd18d13cbb2df0107402e48563b97e6f Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 25 Apr 2018 06:16:32 -0700 Subject: [PATCH 10/12] Recover more gracefully when favicon configuration points at a corrupt/damaged file Summary: Ref T13103. Locally, I managed to break the data for a bunch of files by doing `git clean -df` in a working copy that I'd updated to a commit from many many years ago. Since `conf/local.json` wasn't on the gitignore list many years ago, this removed it, and I lost my encryption keyring. I've symlinked my local config to a version-controlled file now to avoid this specific type of creative self-sabotage in the future, but this has exposed a few cases where we could handle things more gracefully. One issue is that if your favicon is customized but the file it points at can't actually be loaded, we fail explosively and you really can't do anything to move forward except somehow guess that you need to fix your favicon. Instead, recover more gracefully. Test Plan: - Configure file encryption. - Configure a favicon. - Remove the encryption key from your keyring. - Purge Phabricator's caches. - Before: you pretty much dead-end on a fatal that's hard to understand/fix. - After: everything works except your favicon. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13103 Differential Revision: https://secure.phabricator.com/D19406 --- src/applications/files/favicon/PhabricatorFaviconRef.php | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/applications/files/favicon/PhabricatorFaviconRef.php b/src/applications/files/favicon/PhabricatorFaviconRef.php index d6c2fd80a8..08ade5283f 100644 --- a/src/applications/files/favicon/PhabricatorFaviconRef.php +++ b/src/applications/files/favicon/PhabricatorFaviconRef.php @@ -247,7 +247,14 @@ final class PhabricatorFaviconRef extends Phobject { $src_w = $template['width']; $src_h = $template['height']; - $template_data = $template['file']->loadFileData(); + try { + $template_data = $template['file']->loadFileData(); + } catch (Exception $ex) { + // In rare cases, we can end up with a corrupted or inaccessible file. + // If we do, just give up: otherwise, it's impossible to get pages to + // generate and not obvious how to fix it. + return null; + } if (!function_exists('imagecreatefromstring')) { return $template_data; From d40007aa32ae674627d1228c367991c17ac6bc94 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 27 Apr 2018 11:13:00 -0700 Subject: [PATCH 11/12] Fix an issue where the Herald test console doesn't work with "Content source" rules Summary: Ref T13130. See PHI619. Currently, the Herald "Test Console" doesn't pass a "Content Source" to the adapter, so if any rules of the given type execute a "Content source" field rule, they'll fatal. Provide a content source: - If possible, use the content source from the most recent transaction. - Otherwise, build a default "web" content source from the current request. Test Plan: - Wrote a "When [content source][is][whatever]" rule for tasks. - Ran test console against a task. - Before: got a fatal trying to interact with the content source. - After: transcript reports sensible content source. - Also commented out the "xaction" logic to test the fallback behavior. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13130 Differential Revision: https://secure.phabricator.com/D19411 --- .../HeraldTestConsoleController.php | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/applications/herald/controller/HeraldTestConsoleController.php b/src/applications/herald/controller/HeraldTestConsoleController.php index 4ddab2669b..5c2d26cf5a 100644 --- a/src/applications/herald/controller/HeraldTestConsoleController.php +++ b/src/applications/herald/controller/HeraldTestConsoleController.php @@ -38,8 +38,10 @@ final class HeraldTestConsoleController extends HeraldController { $object = $this->getTestObject(); $adapter = $this->getTestAdapter(); + $source = $this->newContentSource($object); $adapter + ->setContentSource($source) ->setIsNewObject(false) ->setActingAsPHID($viewer->getPHID()) ->setViewer($viewer); @@ -218,4 +220,29 @@ final class HeraldTestConsoleController extends HeraldController { ->appendChild($view); } + private function newContentSource($object) { + $viewer = $this->getViewer(); + + // Try using the content source associated with the most recent transaction + // on the object. + + $query = PhabricatorApplicationTransactionQuery::newQueryForObject($object); + + $xaction = $query + ->setViewer($viewer) + ->withObjectPHIDs(array($object->getPHID())) + ->setLimit(1) + ->setOrder('newest') + ->executeOne(); + if ($xaction) { + return $xaction->getContentSource(); + } + + // If we couldn't find a transaction (which should be rare), fall back to + // building a new content source from the test console request itself. + + $request = $this->getRequest(); + return PhabricatorContentSource::newFromRequest($request); + } + } From 5f774f7008b48d22839ced7040fbe5b1d48c2d34 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 27 Apr 2018 11:22:49 -0700 Subject: [PATCH 12/12] Stop build target start times from being overwritten on reentry Summary: See PHI615. Ref T13130. An install is reporting that "Lease Working Copy" build steps always report "Built instantly" after completion. I'm not 100% sure that this is the fix, but I'm like 99% sure: "Lease Working Copy" build steps yield after they ask Drydock for a lease. They will later reenter `doWork()`, see that the lease is filled, and complete. Right now, we reset the start time every time we enter `doWork()`. Instead, set it only if it hasn't been set yet. Test Plan: This is low-risk and a bit tricky to reproduce locally, but I'll run some production builds and see what they look like. Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13130 Differential Revision: https://secure.phabricator.com/D19412 --- .../harbormaster/worker/HarbormasterTargetWorker.php | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/applications/harbormaster/worker/HarbormasterTargetWorker.php b/src/applications/harbormaster/worker/HarbormasterTargetWorker.php index fa3a01272b..a6860f579c 100644 --- a/src/applications/harbormaster/worker/HarbormasterTargetWorker.php +++ b/src/applications/harbormaster/worker/HarbormasterTargetWorker.php @@ -46,7 +46,13 @@ final class HarbormasterTargetWorker extends HarbormasterWorker { $build = $target->getBuild(); $viewer = $this->getViewer(); - $target->setDateStarted(time()); + // If this is the first time we're starting work on this target, mark the + // current time as the start time. If the target yields or waits, we may + // end up here again later, so we don't want to overwrite the start time if + // we already have a value. + if (!$target->getDateStarted()) { + $target->setDateStarted(PhabricatorTime::getNow()); + } try { if ($target->getBuildGeneration() !== $build->getBuildGeneration()) {