recaptime.dev / phorge
@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

Fix a race between Harbormaster and reviewers (often bots) to publish drafts for review

Summary:
See PHI309. There is a window of time between when all builds pass and when Harbormaster actually publishes a revision out of draft.

If any other user tries to interact with the revision during that window, they'll pick up the undraft transaction as a side effect. However, they won't have permission to apply it and will be stopped by a validation error.

Instead, only automatically publish a revision if the actor is the revision author or some system/application user (essentially always Harbormaster).

Test Plan:
- Added a `echo ...; sleep(30);` to `HarbormasterBuildEngine->updateBuildable()` before the `applyTransactions()` at the bottom.
- Wrote an "Always, run an HTTP request" Herald rule and Harbormaster build plan.
- Ran daemons with `bin/phd debug task`.
- Created a new revision with `arc diff`, as user A.
- Waited for `phd` to enter the race window.
- In a separate browser, as user B, submitted a comment via `differential.revision.edit`.
- Before patch: edits during the race window were rejected with a validation error, "you don't have permission to request review".
- After patch: edits go through cleanly.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18921

epriestley dabd3f0b 167e7932

+21 -2
+21 -2
src/applications/differential/editor/DifferentialTransactionEditor.php
··· 1565 1565 1566 1566 1567 1567 protected function didApplyTransactions($object, array $xactions) { 1568 + // In a moment, we're going to try to publish draft revisions which have 1569 + // completed all their builds. However, we only want to do that if the 1570 + // actor is either the revision author or an omnipotent user (generally, 1571 + // the Harbormaster application). 1572 + 1573 + // If we let any actor publish the revision as a side effect of other 1574 + // changes then an unlucky third party who innocently comments on the draft 1575 + // can end up racing Harbormaster and promoting the revision. At best, this 1576 + // is confusing. It can also run into validation problems with the "Request 1577 + // Review" transaction. See PHI309 for some discussion. 1578 + $author_phid = $object->getAuthorPHID(); 1579 + $viewer = $this->requireActor(); 1580 + $can_undraft = 1581 + ($this->getActingAsPHID() === $author_phid) || 1582 + ($viewer->isOmnipotent()); 1583 + 1568 1584 // If a draft revision has no outstanding builds and we're automatically 1569 1585 // making drafts public after builds finish, make the revision public. 1570 - $auto_undraft = !$object->getHoldAsDraft(); 1586 + if ($can_undraft) { 1587 + $auto_undraft = !$object->getHoldAsDraft(); 1588 + } else { 1589 + $auto_undraft = false; 1590 + } 1571 1591 1572 1592 if ($object->isDraft() && $auto_undraft) { 1573 1593 $active_builds = $this->hasActiveBuilds($object); ··· 1575 1595 // When Harbormaster moves a revision out of the draft state, we 1576 1596 // attribute the action to the revision author since this is more 1577 1597 // natural and more useful. 1578 - $author_phid = $object->getAuthorPHID(); 1579 1598 1580 1599 // Additionally, we change the acting PHID for the transaction set 1581 1600 // to the author if it isn't already a user so that mail comes from