Skip to content

Commit

Permalink
Implement ban "editing"
Browse files Browse the repository at this point in the history 
This is done as UI sugar around an unban followed by an immediate ban.
This helps maintain the ban history for investigation of what happened
to certain requests.

Fixes #641
  • Loading branch information
@stwalkerster
stwalkerster committed Sep 22, 2023
1 parent 33cccd5 commit 65ff3a6
Show file tree
Hide file tree
Showing 9 changed files with 224 additions and 68 deletions.
9 changes: 1 addition & 8 deletions includes/DataObjects/Ban.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,21 +170,14 @@ public function save()
// update
$statement = $this->dbObject->prepare(<<<SQL
UPDATE `ban`
SET duration = :duration, active = :active, user = :user, action = :action, visibility = :visibility,
targetqueue = :targetqueue, domain = :domain, updateversion = updateversion + 1
SET active = :active, updateversion = updateversion + 1
WHERE id = :id AND updateversion = :updateversion;
SQL
);
$statement->bindValue(':id', $this->id);
$statement->bindValue(':updateversion', $this->updateversion);

$statement->bindValue(':duration', $this->duration);
$statement->bindValue(':active', $this->active);
$statement->bindValue(':user', $this->user);
$statement->bindValue(":action", $this->action);
$statement->bindValue(":targetqueue", $this->targetqueue);
$statement->bindValue(":visibility", $this->visibility);
$statement->bindValue(":domain", $this->domain);

if (!$statement->execute()) {
throw new Exception($statement->errorInfo());
Expand Down
2 changes: 2 additions & 0 deletions includes/Helpers/LogHelper.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ public static function getLogDescription(Log $entry)
'FlaggedComment' => 'flagged a comment',
'UnflaggedComment' => 'unflagged a comment',
'Unbanned' => 'unbanned',
'BanReplaced' => 'replaced ban',
'Promoted' => 'promoted to tool admin',
'BreakReserve' => 'forcibly broke the reservation',
'Prefchange' => 'changed user preferences',
Expand Down Expand Up @@ -230,6 +231,7 @@ public static function getLogActions(PdoDatabase $database)
"Bans" => [
'Banned' => 'banned',
'Unbanned' => 'unbanned',
'BanReplaced' => 'replaced ban',
],
"Site notice" => [
'Edited' => 'edited interface message',
Expand Down
5 changes: 5 additions & 0 deletions includes/Helpers/Logger.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,11 @@ public static function unbanned(PdoDatabase $database, Ban $object, $reason)
self::createLogEntry($database, $object, "Unbanned", $reason);
}

public static function banReplaced(PdoDatabase $database, Ban $object)
{
self::createLogEntry($database, $object, "BanReplaced");
}

#endregion

#region Requests
Expand Down
215 changes: 171 additions & 44 deletions includes/Pages/PageBan.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,133 @@ protected function set(): void
}
else {
$this->handleGetMethodForSetBan();

$user = User::getCurrent($this->getDatabase());
$banType = WebRequest::getString('type');
$banRequest = WebRequest::getInt('request');

// if the parameters are null, skip loading a request.
if ($banType !== null && $banRequest !== null && $banRequest !== 0) {
$this->preloadFormForRequest($banRequest, $banType, $user);
}
}
}

protected function replace(): void
{
$this->setHtmlTitle('Bans');

$database = $this->getDatabase();
$domain = Domain::getCurrent($database);

// dual-mode action
if (WebRequest::wasPosted()) {
try {
$originalBanId = WebRequest::postInt('replaceBanId');
$originalBanUpdateVersion = WebRequest::postInt('replaceBanUpdateVersion');

$originalBan = Ban::getActiveId($originalBanId, $database, $domain->getId());

if ($originalBan === false) {
throw new ApplicationLogicException("The specified ban is not currently active, or doesn't exist.");
}

// Discard original ban; we're replacing it.
$originalBan->setUpdateVersion($originalBanUpdateVersion);
$originalBan->setActive(false);
$originalBan->save();

Logger::banReplaced($database, $originalBan);

// Proceed as normal to save the new ban.
$this->handlePostMethodForSetBan();
}
catch (ApplicationLogicException $ex) {
$database->rollback();
SessionAlert::error($ex->getMessage());
$this->redirect("bans", "set");
}
}
else {
$this->handleGetMethodForSetBan();

$user = User::getCurrent($database);
$originalBanId = WebRequest::getString('id');

$originalBan = Ban::getActiveId($originalBanId, $database, $domain->getId());

if ($originalBan === false) {
throw new ApplicationLogicException("The specified ban is not currently active, or doesn't exist.");
}

if ($originalBan->getName() !== null) {
if (!$this->barrierTest('name', $user, 'BanType')) {
SessionAlert::error("You are not allowed to set this type of ban.");
$this->redirect("bans", "set");
return;
}

$this->assign('banName', $originalBan->getName());
}

if ($originalBan->getEmail() !== null) {
if (!$this->barrierTest('email', $user, 'BanType')) {
SessionAlert::error("You are not allowed to set this type of ban.");
$this->redirect("bans", "set");
return;
}

$this->assign('banEmail', $originalBan->getEmail());
}

if ($originalBan->getUseragent() !== null) {
if (!$this->barrierTest('useragent', $user, 'BanType')) {
SessionAlert::error("You are not allowed to set this type of ban.");
$this->redirect("bans", "set");
return;
}

$this->assign('banUseragent', $originalBan->getUseragent());
}

if ($originalBan->getIp() !== null) {
if (!$this->barrierTest('ip', $user, 'BanType')) {
SessionAlert::error("You are not allowed to set this type of ban.");
$this->redirect("bans", "set");
return;
}

$this->assign('banIP', $originalBan->getIp() . '/' . $originalBan->getIpMask());
}

$banIsGlobal = $originalBan->getDomain() === null;
if ($banIsGlobal) {
if (!$this->barrierTest('global', $user, 'BanType')) {
SessionAlert::error("You are not allowed to set this type of ban.");
$this->redirect("bans", "set");
return;
}
}

if (!$this->barrierTest($originalBan->getVisibility(), $user, 'BanVisibility')) {
SessionAlert::error("You are not allowed to set this type of ban.");
$this->redirect("bans", "set");
return;
}

$this->assign('banGlobal', $banIsGlobal);
$this->assign('banVisibility', $originalBan->getVisibility());

if ($originalBan->getDuration() !== null) {
$this->assign('banDuration', date('c', $originalBan->getDuration()));
}

$this->assign('banReason', $originalBan->getReason());
$this->assign('banAction', $originalBan->getAction());
$this->assign('banQueue', $originalBan->getTargetQueue());

$this->assign('replaceBanId', $originalBan->getId());
$this->assign('replaceBanUpdateVersion', $originalBan->getUpdateVersion());
}
}

Expand Down Expand Up @@ -275,7 +402,7 @@ private function handlePostMethodForSetBan()
* Handles the GET method on the set action
* @throws Exception
*/
protected function handleGetMethodForSetBan()
private function handleGetMethodForSetBan()
{
$this->setTemplate('bans/banform.tpl');
$this->assignCSRFToken();
Expand All @@ -291,47 +418,6 @@ protected function handleGetMethodForSetBan()
$queues = RequestQueue::getEnabledQueues($database);

$this->assign('requestQueues', $queues);

$banType = WebRequest::getString('type');
$banRequest = WebRequest::getInt('request');

// if the parameters are null, skip loading a request.
if ($banType === null || $banRequest === null || $banRequest === 0) {
return;
}

// Attempt to resolve the correct target
/** @var Request|false $request */
$request = Request::getById($banRequest, $database);
if ($request === false) {
$this->assign('bantarget', '');

return;
}

switch ($banType) {
case 'EMail':
if ($this->barrierTest('email', $user, 'BanType')) {
$this->assign('banEmail', $request->getEmail());
}
break;
case 'IP':
if ($this->barrierTest('ip', $user, 'BanType')) {
$this->assign('banIP', $this->getXffTrustProvider()
->getTrustedClientIp($request->getIp(), $request->getForwardedIp()));
}
break;
case 'Name':
if ($this->barrierTest('name', $user, 'BanType')) {
$this->assign('banName', $request->getName());
}
break;
case 'UA':
if ($this->barrierTest('useragent', $user, 'BanType')) {
$this->assign('banUseragent', $request->getEmail());
}
break;
}
}

/**
Expand Down Expand Up @@ -362,7 +448,7 @@ private function getBanForUnban(): Ban
/**
* Sets up Smarty variables for access control
*/
protected function setupSecurity(User $user): void
private function setupSecurity(User $user): void
{
$this->assign('canSeeIpBan', $this->barrierTest('ip', $user, 'BanType'));
$this->assign('canSeeNameBan', $this->barrierTest('name', $user, 'BanType'));
Expand Down Expand Up @@ -441,7 +527,7 @@ private function validateIpBan(string $targetIp, int $targetMask, User $user, st
*
* @param Ban[] $bans
*/
protected function setupBanList(array $bans): void
private function setupBanList(array $bans): void
{
$userIds = array_map(fn(Ban $entry) => $entry->getUser(), $bans);
$userList = UserSearchHelper::get($this->getDatabase())->inIds($userIds)->fetchMap('username');
Expand Down Expand Up @@ -542,4 +628,45 @@ private function getRawBanTargets(User $user): array

return array($targetName, $targetIp, $targetEmail, $targetUseragent);
}

private function preloadFormForRequest(int $banRequest, string $banType, User $user): void
{
$database = $this->getDatabase();

// Attempt to resolve the correct target
/** @var Request|false $request */
$request = Request::getById($banRequest, $database);
if ($request === false) {
$this->assign('bantarget', '');

return;
}

switch ($banType) {
case 'EMail':
if ($this->barrierTest('email', $user, 'BanType')) {
$this->assign('banEmail', $request->getEmail());
}
break;
case 'IP':
if ($this->barrierTest('ip', $user, 'BanType')) {
$trustedIp = $this->getXffTrustProvider()->getTrustedClientIp(
$request->getIp(),
$request->getForwardedIp());

$this->assign('banIP', $trustedIp);
}
break;
case 'Name':
if ($this->barrierTest('name', $user, 'BanType')) {
$this->assign('banName', $request->getName());
}
break;
case 'UA':
if ($this->barrierTest('useragent', $user, 'BanType')) {
$this->assign('banUseragent', $request->getEmail());
}
break;
}
}
}
2 changes: 1 addition & 1 deletion includes/Router/RequestRouter.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ class RequestRouter implements IRequestRouter
'bans' =>
array(
'class' => PageBan::class,
'actions' => array('set', 'remove', 'show'),
'actions' => array('set', 'remove', 'show', 'replace'),
),
'userManagement' =>
array(
Expand Down
1 change: 1 addition & 0 deletions includes/Security/RoleConfiguration.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,7 @@ class RoleConfiguration
self::MAIN => self::ACCESS_ALLOW,
'set' => self::ACCESS_ALLOW,
'remove' => self::ACCESS_ALLOW,
'replace' => self::ACCESS_ALLOW,
),
'BanType' => array(
'ip' => self::ACCESS_ALLOW,
Expand Down
10 changes: 10 additions & 0 deletions resources/global.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,16 @@ $("#banAction").change(function() {
}
})

$("#banDuration").change(function() {
var selectedOption = $(this).children("option:selected").val();

if (selectedOption === 'other') {
$("#banCustomDurationSelection").removeClass('d-none');
} else {
$("#banCustomDurationSelection").addClass('d-none');
}
})

$("#commentVisibilityDropdown").on("change", "input[type='radio']", function() {
if($(this).val() == 'user') {
$("#commentVisibilityButton").addClass('btn-outline-secondary').removeClass('btn-danger').removeClass('btn-visited');
Expand Down
Loading

0 comments on commit 65ff3a6

Please sign in to comment.