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.

When updating revisions in responset to commits, use the omnipotent viewer to pull diffs

Summary:
Ref T13625. See that task for discussion.

Currently, the Viewer when performing revision updates in response to commits may be an arbitrary low-privilege user (an Application, a disabled User, a bot, a mailing list, etc).

Today, this leads to an exception when trying to make API calls.

Ideally, we probably would not perform the update in these cases. However, performing the update isn't a policy violation and is generally less surprising than not performing it, so continue performing it for now: just use the omnipotent user to interact with the API.

Test Plan:
- Authored a commit as a bot user without permission to view the repository or revision.
- Commented out a couple of caches, and used `bin/repository reparse --publish ...` to republish the commit.
- Before: exception when trying to interact with the API.
- After: clean publish.

Maniphest Tasks: T13625

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

epriestley 55f4a258 0a3093ef

+27 -5
+27 -5
src/applications/diffusion/worker/DiffusionUpdateObjectAfterCommitWorker.php
··· 148 148 PhabricatorRepositoryCommit $commit, 149 149 DifferentialRevision $revision) { 150 150 151 + $acting_phid = $this->getActingPHID($commit); 152 + $acting_user = $this->loadActingUser($acting_phid); 153 + 154 + // See T13625. The "Acting User" is the author of the commit based on the 155 + // author string, or the Diffusion application PHID if we could not 156 + // identify an author. 157 + 158 + // This user may not be able to view the commit or the revision, and may 159 + // also be unable to make API calls. Here, we execute queries and apply 160 + // transactions as the omnipotent user. 161 + 162 + // It would probably be better to use the acting user everywhere here, and 163 + // exit gracefully if they can't see the revision (this is how the flow 164 + // on tasks works). However, without a positive indicator in the UI 165 + // explaining "no revision was updated because the author of this commit 166 + // can't see anything", this might be fairly confusing, and break workflows 167 + // which have worked historically. 168 + 169 + // This isn't, per se, a policy violation (you can't get access to anything 170 + // you don't already have access to by making commits that reference 171 + // revisions, even if you can't see the commits or revisions), so just 172 + // leave it for now. 173 + 174 + $viewer = $this->getViewer(); 175 + 151 176 // Reload the revision to get the active diff, which is currently required 152 177 // by "updateRevisionWithCommit()". 153 178 $revision = id(new DifferentialRevisionQuery()) 154 - ->setViewer($this->getViewer()) 179 + ->setViewer($viewer) 155 180 ->withIDs(array($revision->getID())) 156 181 ->needActiveDiffs(true) 157 182 ->executeOne(); 158 - 159 - $acting_phid = $this->getActingPHID($commit); 160 - $acting_user = $this->loadActingUser($acting_phid); 161 183 162 184 $xactions = array(); 163 185 ··· 177 199 ->setMetadataValue('commitPHID', $commit->getPHID()); 178 200 179 201 $extraction_engine = id(new DifferentialDiffExtractionEngine()) 180 - ->setViewer($acting_user) 202 + ->setViewer($viewer) 181 203 ->setAuthorPHID($acting_phid); 182 204 183 205 $content_source = $this->newContentSource();