Consolidate readers of "differential.revisionID" property

Summary:
Depends on D20467. Ref T13277. Currently, the "MessageParserWorker" writes this property on commits, then Herald and Audit both read it.

Make them share code so this property has one writer and one reader. This property isn't great, but at least now the badness is hidden.

Currently, we can't just use edges because they may not have been written yet. I am likely to just do this, soon:

- Just write the edges (in "MessageParserWorker").
- Hide the edges from mail.

However, we'll sort-of lose the "revisionMatchData" explanation thing if I do that. Maybe this is fine? But when commits match because hashes match, it legitimately isn't obvious.

For now, just reduce the amount of harm/badness here.

Test Plan:
- Ran `bin/repository reparse --publish ...`.
- Ran a Herald "Audit" rule using the "Accepted Differential revision" field.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277

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

epriestley 7 years ago 56f5c244 45f01dc7

+47 -31

3 changed files

expand all

src

applications

diffusion

herald

HeraldCommitAdapter.php

query

DiffusionCommitRevisionQuery.php

repository

worker

PhabricatorRepositoryCommitPublishWorker.php

+24 -21

src/applications/diffusion/herald/HeraldCommitAdapter.php

··· 175 175 } 176 176 177 177 public function loadDifferentialRevision() { 178 - $viewer = $this->getViewer(); 179 - 180 178 if ($this->affectedRevision === null) { 181 - $this->affectedRevision = false; 179 + $viewer = $this->getViewer(); 180 + 181 + // NOTE: The viewer here is omnipotent, which means that Herald discloses 182 + // some information users do not normally have access to when rules load 183 + // the revision related to a commit. See D20468. 184 + 185 + // A user who wants to learn about "Dxyz" can write a Herald rule which 186 + // uses all the "Related revision..." fields, then push a commit which 187 + // contains "Differential Revision: Dxyz" in the message to make Herald 188 + // evaluate the commit with "Dxyz" as the related revision. 182 189 183 - $commit = $this->getObject(); 184 - $data = $commit->getCommitData(); 190 + // At time of writing, this commit will link to the revision and the 191 + // transcript for the commit will disclose some information about the 192 + // revision (like reviewers, subscribers, and build status) which the 193 + // commit author could not otherwise see. 185 194 186 - $revision_id = $data->getCommitDetail('differential.revisionID'); 187 - if ($revision_id) { 188 - // NOTE: The Herald rule owner might not actually have access to 189 - // the revision, and can control which revision a commit is 190 - // associated with by putting text in the commit message. However, 191 - // the rules they can write against revisions don't actually expose 192 - // anything interesting, so it seems reasonable to load unconditionally 193 - // here. 195 + // For now, we just accept this. The disclosures are relatively 196 + // uninteresting and you have to jump through a lot of hoops (and leave 197 + // a lot of evidence) to get this information. 194 198 195 - $revision = id(new DifferentialRevisionQuery()) 196 - ->withIDs(array($revision_id)) 197 - ->setViewer($viewer) 198 - ->needReviewers(true) 199 - ->executeOne(); 200 - if ($revision) { 201 - $this->affectedRevision = $revision; 202 - } 199 + $revision = DiffusionCommitRevisionQuery::loadRevisionForCommit( 200 + $viewer, 201 + $this->getObject()); 202 + if ($revision) { 203 + $this->affectedRevision = $revision; 204 + } else { 205 + $this->affectedRevision = false; 203 206 } 204 207 } 205 208

+19

src/applications/diffusion/query/DiffusionCommitRevisionQuery.php

··· 46 46 return $map; 47 47 } 48 48 49 + public static function loadRevisionForCommit( 50 + PhabricatorUser $viewer, 51 + PhabricatorRepositoryCommit $commit) { 52 + 53 + $data = $commit->getCommitData(); 54 + 55 + $revision_id = $data->getCommitDetail('differential.revisionID'); 56 + if (!$revision_id) { 57 + return null; 58 + } 59 + 60 + return id(new DifferentialRevisionQuery()) 61 + ->setViewer($viewer) 62 + ->withIDs(array($revision_id)) 63 + ->needReviewers(true) 64 + ->executeOne(); 65 + } 66 + 67 + 49 68 }

+4 -10

src/applications/repository/worker/PhabricatorRepositoryCommitPublishWorker.php

··· 135 135 $data = $commit->getCommitData(); 136 136 137 137 $author_phid = $data->getCommitDetail('authorPHID'); 138 - $revision_id = $data->getCommitDetail('differential.revisionID'); 139 - if ($revision_id) { 140 - $revision = id(new DifferentialRevisionQuery()) 141 - ->setViewer($viewer) 142 - ->withIDs(array($revision_id)) 143 - ->needReviewers(true) 144 - ->executeOne(); 145 - } else { 146 - $revision = null; 147 - } 138 + 139 + $revision = DiffusionCommitRevisionQuery::loadRevisionForCommit( 140 + $viewer, 141 + $commit); 148 142 149 143 $requests = $commit->getAudits(); 150 144 $requests = mpull($requests, null, 'getAuditorPHID');

Configure Feed

Configure Feed