From 8a2863e3f7f8de79fbdd25eb3dc05605b0be2784 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 30 Jan 2018 12:01:26 -0800 Subject: [PATCH] Change the "can see remote address?" policy to "is administrator?" everywhere Summary: Depends on D18970. Ref T13049. Currently, the policy for viewing remote addresses is: - In activity logs: administrators. - In push and pull logs: users who can edit the corresponding repository. This sort of makes sense, but is also sort of weird. Particularly, I think it's kind of hard to understand and predict, and hard to guess that this is the behavior we implement. The actual implementation is complex, too. Instead, just use the rule "administrators can see remote addresses" consistently across all applications. This should generally be more strict than the old rule, because administrators could usually have seen everyone's address in the activity logs anyway. It's also simpler and more expected, and I don't really know of any legit use cases for the "repository editor" rule. Test Plan: Viewed pull/push/activity logs as non-admin. Saw remote addresses as an admin, and none as a non-admin. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13049 Differential Revision: https://secure.phabricator.com/D18971 --- .../query/DiffusionPullLogSearchEngine.php | 20 +++++++++-- .../view/DiffusionPullLogListView.php | 36 +++++++------------ .../view/DiffusionPushLogListView.php | 25 ++++--------- 3 files changed, 37 insertions(+), 44 deletions(-) diff --git a/src/applications/diffusion/query/DiffusionPullLogSearchEngine.php b/src/applications/diffusion/query/DiffusionPullLogSearchEngine.php index db5d4b9fcb..dfdfceb519 100644 --- a/src/applications/diffusion/query/DiffusionPullLogSearchEngine.php +++ b/src/applications/diffusion/query/DiffusionPullLogSearchEngine.php @@ -60,7 +60,9 @@ final class DiffusionPullLogSearchEngine } protected function newExportFields() { - return array( + $viewer = $this->requireViewer(); + + $fields = array( id(new PhabricatorPHIDExportField()) ->setKey('repositoryPHID') ->setLabel(pht('Repository PHID')), @@ -86,6 +88,14 @@ final class DiffusionPullLogSearchEngine ->setKey('date') ->setLabel(pht('Date')), ); + + if ($viewer->getIsAdmin()) { + $fields[] = id(new PhabricatorStringExportField()) + ->setKey('remoteAddress') + ->setLabel(pht('Remote Address')); + } + + return $fields; } protected function newExportData(array $events) { @@ -117,7 +127,7 @@ final class DiffusionPullLogSearchEngine $puller_name = null; } - $export[] = array( + $map = array( 'repositoryPHID' => $repository_phid, 'repository' => $repository_name, 'pullerPHID' => $puller_phid, @@ -127,6 +137,12 @@ final class DiffusionPullLogSearchEngine 'code' => $event->getResultCode(), 'date' => $event->getEpoch(), ); + + if ($viewer->getIsAdmin()) { + $map['remoteAddress'] = $event->getRemoteAddress(); + } + + $export[] = $map; } return $export; diff --git a/src/applications/diffusion/view/DiffusionPullLogListView.php b/src/applications/diffusion/view/DiffusionPullLogListView.php index f2e3280eba..8df35e2922 100644 --- a/src/applications/diffusion/view/DiffusionPullLogListView.php +++ b/src/applications/diffusion/view/DiffusionPullLogListView.php @@ -22,24 +22,10 @@ final class DiffusionPullLogListView extends AphrontView { } $handles = $viewer->loadHandles($handle_phids); - // Figure out which repositories are editable. We only let you see remote - // IPs if you have edit capability on a repository. - $editable_repos = array(); - if ($events) { - $editable_repos = id(new PhabricatorRepositoryQuery()) - ->setViewer($viewer) - ->requireCapabilities( - array( - PhabricatorPolicyCapability::CAN_VIEW, - PhabricatorPolicyCapability::CAN_EDIT, - )) - ->withPHIDs(mpull($events, 'getRepositoryPHID')) - ->execute(); - $editable_repos = mpull($editable_repos, null, 'getPHID'); - } + // Only administrators can view remote addresses. + $remotes_visible = $viewer->getIsAdmin(); $rows = array(); - $any_host = false; foreach ($events as $event) { if ($event->getRepositoryPHID()) { $repository = $event->getRepository(); @@ -47,13 +33,10 @@ final class DiffusionPullLogListView extends AphrontView { $repository = null; } - // Reveal this if it's valid and the user can edit the repository. For - // invalid requests you currently have to go fishing in the database. - $remote_address = '-'; - if ($repository) { - if (isset($editable_repos[$event->getRepositoryPHID()])) { - $remote_address = $event->getRemoteAddress(); - } + if ($remotes_visible) { + $remote_address = $event->getRemoteAddress(); + } else { + $remote_address = null; } $event_id = $event->getID(); @@ -107,6 +90,13 @@ final class DiffusionPullLogListView extends AphrontView { '', 'n', 'right', + )) + ->setColumnVisibility( + array( + true, + true, + true, + $remotes_visible, )); return $table; diff --git a/src/applications/diffusion/view/DiffusionPushLogListView.php b/src/applications/diffusion/view/DiffusionPushLogListView.php index 303f0519f6..77f28671c6 100644 --- a/src/applications/diffusion/view/DiffusionPushLogListView.php +++ b/src/applications/diffusion/view/DiffusionPushLogListView.php @@ -25,31 +25,18 @@ final class DiffusionPushLogListView extends AphrontView { $handles = $viewer->loadHandles($handle_phids); - // Figure out which repositories are editable. We only let you see remote - // IPs if you have edit capability on a repository. - $editable_repos = array(); - if ($logs) { - $editable_repos = id(new PhabricatorRepositoryQuery()) - ->setViewer($viewer) - ->requireCapabilities( - array( - PhabricatorPolicyCapability::CAN_VIEW, - PhabricatorPolicyCapability::CAN_EDIT, - )) - ->withPHIDs(mpull($logs, 'getRepositoryPHID')) - ->execute(); - $editable_repos = mpull($editable_repos, null, 'getPHID'); - } + // Only administrators can view remote addresses. + $remotes_visible = $viewer->getIsAdmin(); $rows = array(); $any_host = false; foreach ($logs as $log) { $repository = $log->getRepository(); - // Reveal this if it's valid and the user can edit the repository. - $remote_address = '-'; - if (isset($editable_repos[$log->getRepositoryPHID()])) { + if ($remotes_visible) { $remote_address = $log->getPushEvent()->getRemoteAddress(); + } else { + $remote_address = null; } $event_id = $log->getPushEvent()->getID(); @@ -142,7 +129,7 @@ final class DiffusionPushLogListView extends AphrontView { true, true, true, - true, + $remotes_visible, true, $any_host, ));