mirror of
https://we.phorge.it/source/phorge.git
synced 2024-11-26 16:52:41 +01:00
Reduce visibility of "Host" and "Path" Differential fields by default
Summary: See discussion in T838. These fields expose information which it isn't necessary or useful to expose in the general case. - Disable fields by default, allow them to be enabled in config (these fields were useful for me at Facebook when I had access to all the machines). - Remove 'sourcePath' from Conduit methods other than differential.query. - Condition 'sourcePath' field in Conduit on the caller being the revision author. This is a bit hacky but not so awful. Test Plan: - Verified fields are gone by default and restored by configuration. - Verified Conduit no longer returns these fields other than differential.query. - Verified field presence/absence according to authorship in differential.query. - Grepped around in arcanist to make sure we aren't relying on sourcePath. There's a workflow in "arc merge" that technically might hit it, but I think it's unreachable, definitely irrelvant (we never use source path as a distinguisher under git/hg, and can't 'arc merge' in SVN) and it's going away Real Soon Now anyway. Reviewers: btrahan, arice Reviewed By: arice CC: aran, epriestley Maniphest Tasks: T838 Differential Revision: https://secure.phabricator.com/D1582
This commit is contained in:
parent
15f6216634
commit
36e72639de
5 changed files with 38 additions and 10 deletions
|
@ -597,6 +597,13 @@ return array(
|
||||||
|
|
||||||
'differential.field-selector' => 'DifferentialDefaultFieldSelector',
|
'differential.field-selector' => 'DifferentialDefaultFieldSelector',
|
||||||
|
|
||||||
|
// Differential can show "Host" and "Path" fields on revisions, with
|
||||||
|
// information about the machine and working directory where the change
|
||||||
|
// came from. These fields are disabled by default because they may
|
||||||
|
// occasionally have sensitive information; you can set this to true to
|
||||||
|
// enable them.
|
||||||
|
'differential.show-host-field' => false,
|
||||||
|
|
||||||
// If you set this to true, users can "!accept" revisions via email (normally,
|
// If you set this to true, users can "!accept" revisions via email (normally,
|
||||||
// they can take other actions but can not "!accept"). This action is disabled
|
// they can take other actions but can not "!accept"). This action is disabled
|
||||||
// by default because email authentication can be configured to be very weak,
|
// by default because email authentication can be configured to be very weak,
|
||||||
|
|
|
@ -166,7 +166,7 @@ class ConduitAPI_differential_query_Method extends ConduitAPIMethod {
|
||||||
}
|
}
|
||||||
|
|
||||||
$id = $revision->getID();
|
$id = $revision->getID();
|
||||||
$results[] = array(
|
$result = array(
|
||||||
'id' => $id,
|
'id' => $id,
|
||||||
'phid' => $revision->getPHID(),
|
'phid' => $revision->getPHID(),
|
||||||
'title' => $revision->getTitle(),
|
'title' => $revision->getTitle(),
|
||||||
|
@ -178,7 +178,6 @@ class ConduitAPI_differential_query_Method extends ConduitAPIMethod {
|
||||||
'statusName' =>
|
'statusName' =>
|
||||||
ArcanistDifferentialRevisionStatus::getNameForRevisionStatus(
|
ArcanistDifferentialRevisionStatus::getNameForRevisionStatus(
|
||||||
$revision->getStatus()),
|
$revision->getStatus()),
|
||||||
'sourcePath' => $diff->getSourcePath(),
|
|
||||||
'branch' => $diff->getBranch(),
|
'branch' => $diff->getBranch(),
|
||||||
'summary' => $revision->getSummary(),
|
'summary' => $revision->getSummary(),
|
||||||
'testPlan' => $revision->getTestPlan(),
|
'testPlan' => $revision->getTestPlan(),
|
||||||
|
@ -188,6 +187,14 @@ class ConduitAPI_differential_query_Method extends ConduitAPIMethod {
|
||||||
'reviewers' => array_values($revision->getReviewers()),
|
'reviewers' => array_values($revision->getReviewers()),
|
||||||
'ccs' => array_values($revision->getCCPHIDs()),
|
'ccs' => array_values($revision->getCCPHIDs()),
|
||||||
);
|
);
|
||||||
|
|
||||||
|
// TODO: This is a hacky way to put permissions on this field until we
|
||||||
|
// have first-class support, see T838.
|
||||||
|
if ($revision->getAuthorPHID() == $request->getUser()->getPHID()) {
|
||||||
|
$result['sourcePath'] = $diff->getSourcePath();
|
||||||
|
}
|
||||||
|
|
||||||
|
$results[] = $result;
|
||||||
}
|
}
|
||||||
|
|
||||||
return $results;
|
return $results;
|
||||||
|
|
|
@ -20,7 +20,7 @@ final class DifferentialDefaultFieldSelector
|
||||||
extends DifferentialFieldSelector {
|
extends DifferentialFieldSelector {
|
||||||
|
|
||||||
public function getFieldSpecifications() {
|
public function getFieldSpecifications() {
|
||||||
return array(
|
$fields = array(
|
||||||
new DifferentialTitleFieldSpecification(),
|
new DifferentialTitleFieldSpecification(),
|
||||||
new DifferentialSummaryFieldSpecification(),
|
new DifferentialSummaryFieldSpecification(),
|
||||||
new DifferentialTestPlanFieldSpecification(),
|
new DifferentialTestPlanFieldSpecification(),
|
||||||
|
@ -34,13 +34,27 @@ final class DifferentialDefaultFieldSelector
|
||||||
new DifferentialCommitsFieldSpecification(),
|
new DifferentialCommitsFieldSpecification(),
|
||||||
new DifferentialDependenciesFieldSpecification(),
|
new DifferentialDependenciesFieldSpecification(),
|
||||||
new DifferentialManiphestTasksFieldSpecification(),
|
new DifferentialManiphestTasksFieldSpecification(),
|
||||||
new DifferentialHostFieldSpecification(),
|
|
||||||
new DifferentialPathFieldSpecification(),
|
|
||||||
new DifferentialArcanistProjectFieldSpecification(),
|
|
||||||
new DifferentialApplyPatchFieldSpecification(),
|
|
||||||
new DifferentialRevisionIDFieldSpecification(),
|
|
||||||
new DifferentialGitSVNIDFieldSpecification(),
|
|
||||||
);
|
);
|
||||||
|
|
||||||
|
if (PhabricatorEnv::getEnvConfig('differential.show-host-field')) {
|
||||||
|
$fields = array_merge(
|
||||||
|
$fields,
|
||||||
|
array(
|
||||||
|
new DifferentialHostFieldSpecification(),
|
||||||
|
new DifferentialPathFieldSpecification(),
|
||||||
|
));
|
||||||
|
}
|
||||||
|
|
||||||
|
$fields = array_merge(
|
||||||
|
$fields,
|
||||||
|
array(
|
||||||
|
new DifferentialArcanistProjectFieldSpecification(),
|
||||||
|
new DifferentialApplyPatchFieldSpecification(),
|
||||||
|
new DifferentialRevisionIDFieldSpecification(),
|
||||||
|
new DifferentialGitSVNIDFieldSpecification(),
|
||||||
|
));
|
||||||
|
|
||||||
|
return $fields;
|
||||||
}
|
}
|
||||||
|
|
||||||
}
|
}
|
||||||
|
|
|
@ -26,6 +26,7 @@ phutil_require_module('phabricator', 'applications/differential/field/specificat
|
||||||
phutil_require_module('phabricator', 'applications/differential/field/specification/testplan');
|
phutil_require_module('phabricator', 'applications/differential/field/specification/testplan');
|
||||||
phutil_require_module('phabricator', 'applications/differential/field/specification/title');
|
phutil_require_module('phabricator', 'applications/differential/field/specification/title');
|
||||||
phutil_require_module('phabricator', 'applications/differential/field/specification/unit');
|
phutil_require_module('phabricator', 'applications/differential/field/specification/unit');
|
||||||
|
phutil_require_module('phabricator', 'infrastructure/env');
|
||||||
|
|
||||||
|
|
||||||
phutil_require_source('DifferentialDefaultFieldSelector.php');
|
phutil_require_source('DifferentialDefaultFieldSelector.php');
|
||||||
|
|
|
@ -160,7 +160,6 @@ class DifferentialDiff extends DifferentialDAO {
|
||||||
'sourceControlBaseRevision' => $this->getSourceControlBaseRevision(),
|
'sourceControlBaseRevision' => $this->getSourceControlBaseRevision(),
|
||||||
'sourceControlPath' => $this->getSourceControlPath(),
|
'sourceControlPath' => $this->getSourceControlPath(),
|
||||||
'sourceControlSystem' => $this->getSourceControlSystem(),
|
'sourceControlSystem' => $this->getSourceControlSystem(),
|
||||||
'sourcePath' => $this->getSourcePath(),
|
|
||||||
'branch' => $this->getBranch(),
|
'branch' => $this->getBranch(),
|
||||||
'unitStatus' => $this->getUnitStatus(),
|
'unitStatus' => $this->getUnitStatus(),
|
||||||
'lintStatus' => $this->getLintStatus(),
|
'lintStatus' => $this->getLintStatus(),
|
||||||
|
|
Loading…
Reference in a new issue