Page MenuHomePhabricator

Fix failure to record `pullerPHID` in repository pull logs
ClosedPublic

Authored by epriestley on Jan 23 2018, 12:09 AM.
Tags
None
Referenced Files
F13283004: D18912.diff
Sun, Jun 2, 12:42 PM
F13270795: D18912.diff
Wed, May 29, 11:46 AM
F13251891: D18912.diff
Sat, May 25, 12:51 AM
F13236430: D18912.id45373.diff
Tue, May 21, 9:41 AM
F13230045: D18912.id45341.diff
Mon, May 20, 8:19 PM
F13228294: D18912.id45373.diff
Mon, May 20, 8:30 AM
F13218204: D18912.id.diff
Sat, May 18, 10:54 AM
F13214524: D18912.diff
Fri, May 17, 11:07 AM
Subscribers
None

Details

Summary

See PHI305. Ref T13046.

The SSH workflows currently extend PhabricatorManagementWorkflow to benefit from sharing all the standard argument parsing code. Sharing the parsing code is good, but it also means they inherit a getViewer() method which returns the ommnipotent viewer.

This is appropriate for everything else which extends ManagementWorkflow (like bin/storage, bin/auth, etc.) but not appropriate for SSH workflows, which have a real user.

This caused a bug with the pull logs where pullerPHID was not recorded properly. We used $this->getViewer()->getPHID() but the correct code was $this->getUser()->getPHID().

To harden this against future mistakes:

  • Don't extend ManagementWorkflow. Extend PhutilArgumentWorkflow instead. We only want the argument parsing code.
  • Rename get/setUser() to get/setSSHUser() to make them explicit.

Then, fix the pull log bug by calling getSSHUser() instead of getViewer().

Test Plan
  • Pulled and pushed to a repository over SSH.
  • Grepped all the SSH stuff for the altered symbols.
  • Saw pulls record a valid pullerPHID in the pull log.
  • Used echo {} | ssh ... conduit conduit.ping to test conduit over SSH.

Diff Detail

Repository
rP Phabricator
Branch
ssh1
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 19128
Build 25827: Run Core Tests
Build 25826: arc lint + arc unit

Event Timeline

src/applications/diffusion/ssh/DiffusionSSHWorkflow.php
268

This is the actual bugfix.

I also grepped for anything else calling getViewer(), since it would probably indicate another bug, but it looks like this was the only one.

This revision is now accepted and ready to land.Jan 23 2018, 8:52 PM
This revision was automatically updated to reflect the committed changes.