Changeset View
Standalone View
src/applications/badges/controller/PhabricatorBadgesEditRecipientsController.php
- This file was added.
<?php | |||||
final class PhabricatorBadgesEditRecipientsController | |||||
extends PhabricatorBadgesController { | |||||
public function handleRequest(AphrontRequest $request) { | |||||
$viewer = $request->getViewer(); | |||||
$id = $request->getURIData('id'); | |||||
$badge = id(new PhabricatorBadgesQuery()) | |||||
->setViewer($viewer) | |||||
epriestley: Remove now that you're using getURIData(). | |||||
->withIDs(array($id)) | |||||
->needRecipients(true) | |||||
->requireCapabilities( | |||||
array( | |||||
Done Inline ActionshandleRequest / getViewer (see below) epriestley: handleRequest / getViewer (see below) | |||||
PhabricatorPolicyCapability::CAN_EDIT, | |||||
PhabricatorPolicyCapability::CAN_VIEW, | |||||
)) | |||||
->executeOne(); | |||||
if (!$badge) { | |||||
return new Aphront404Response(); | |||||
} | |||||
$recipient_phids = $badge->getRecipientPHIDs(); | |||||
Done Inline ActionsI think this controller does not enforce policies correctly. Although it changes the form based on the user's edit permissions, it does not enforce that a user must actually have permission in order to perform edits. An attacker can just edit the HTML of the form on the client, then submit whatever parameters they want, removing other users' badges. The actual edits need to have the checks applied to them. (Or maybe I'm misunderstanding, I haven't actually patched this locally.) epriestley: I think this controller does not enforce policies correctly.
Although it changes the //form//… | |||||
Done Inline ActionsWorth noting then that this is copy/pastey from Project Membership. Not sure what you want to to change here (iNoob). chad: Worth noting then that this is copy/pastey from Project Membership. Not sure what you want to… | |||||
if ($request->isFormPost()) { | |||||
$recipient_spec = array(); | |||||
$remove = $request->getStr('remove'); | |||||
if ($remove) { | |||||
$recipient_spec['-'] = array_fuse(array($remove)); | |||||
} | |||||
$add_recipients = $request->getArr('phids'); | |||||
if ($add_recipients) { | |||||
$recipient_spec['+'] = array_fuse($add_recipients); | |||||
} | |||||
$type_recipient = PhabricatorBadgeHasRecipientEdgeType::EDGECONST; | |||||
$xactions = array(); | |||||
$xactions[] = id(new PhabricatorBadgesTransaction()) | |||||
->setTransactionType(PhabricatorTransactions::TYPE_EDGE) | |||||
->setMetadataValue('edge:type', $type_recipient) | |||||
->setNewValue($recipient_spec); | |||||
$editor = id(new PhabricatorBadgesEditor($badge)) | |||||
->setActor($viewer) | |||||
->setContentSourceFromRequest($request) | |||||
->setContinueOnNoEffect(true) | |||||
->setContinueOnMissingFields(true) | |||||
->applyTransactions($badge, $xactions); | |||||
return id(new AphrontRedirectResponse()) | |||||
->setURI($request->getRequestURI()); | |||||
} | |||||
$recipient_phids = array_reverse($recipient_phids); | |||||
$handles = $this->loadViewerHandles($recipient_phids); | |||||
$state = array(); | |||||
foreach ($handles as $handle) { | |||||
$state[] = array( | |||||
'phid' => $handle->getPHID(), | |||||
'name' => $handle->getFullName(), | |||||
); | |||||
Done Inline ActionsPrefer $handles = $viewer->loadHandles($phids) in modern code. epriestley: Prefer `$handles = $viewer->loadHandles($phids)` in modern code. | |||||
} | |||||
Done Inline ActionsI couldn't get that to work (returns different results). chad: I couldn't get that to work (returns different results). | |||||
$can_edit = PhabricatorPolicyFilter::hasCapability( | |||||
$viewer, | |||||
$badge, | |||||
PhabricatorPolicyCapability::CAN_EDIT); | |||||
$form_box = null; | |||||
$title = pht('Add Recipient'); | |||||
if ($can_edit) { | |||||
$header_name = pht('Edit Recipients'); | |||||
$view_uri = $this->getApplicationURI('view/'.$badge->getID().'/'); | |||||
$form = new AphrontFormView(); | |||||
$form | |||||
->setUser($viewer) | |||||
->appendControl( | |||||
id(new AphrontFormTokenizerControl()) | |||||
->setName('phids') | |||||
->setLabel(pht('Add Recipients')) | |||||
->setDatasource(new PhabricatorPeopleDatasource())) | |||||
->appendChild( | |||||
id(new AphrontFormSubmitControl()) | |||||
->addCancelButton($view_uri) | |||||
->setValue(pht('Add Recipients'))); | |||||
$form_box = id(new PHUIObjectBoxView()) | |||||
->setHeaderText($title) | |||||
->setForm($form); | |||||
} | |||||
$recipient_list = id(new PhabricatorBadgesRecipientsListView()) | |||||
->setBadge($badge) | |||||
->setHandles($handles) | |||||
->setUser($viewer); | |||||
$badge_url = $this->getApplicationURI('view/'.$id.'/'); | |||||
$crumbs = $this->buildApplicationCrumbs(); | |||||
$crumbs->addTextCrumb($badge->getName(), $badge_url); | |||||
$crumbs->addTextCrumb(pht('Recipients')); | |||||
return $this->buildApplicationPage( | |||||
array( | |||||
Done Inline ActionsPrefer $this->getApplicationURI('x') over $this->getApplicationURI().'x'. The former is more flexible, since we can, e.g., correct or verify URIs inside getApplicationURI(...). epriestley: Prefer `$this->getApplicationURI('x')` over `$this->getApplicationURI().'x'`. The former is… | |||||
$crumbs, | |||||
$form_box, | |||||
$recipient_list, | |||||
), | |||||
array( | |||||
'title' => $title, | |||||
)); | |||||
} | |||||
} |
Remove now that you're using getURIData().