HomePhabricator

Fix failure to record `pullerPHID` in repository pull logs

Description

Fix failure to record pullerPHID in repository pull logs

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.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13046

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

Details

Provenance
epriestleyAuthored on Jan 22 2018, 11:54 PM
epriestleyPushed on Jan 23 2018, 10:09 PM
Reviewer
amckinley
Differential Revision
D18912: Fix failure to record `pullerPHID` in repository pull logs
Parents
rPbf2868c07016: Rename "PhabricatorPasswordHashInterface" to…
Branches
Unknown
Tags
Unknown
Tasks
T13046: Surface repository pull logs in the web UI
Build Status
Buildable 19158
Build 25868: Run Core Tests