Files
tinker_tickets/models/BulkOperationsModel.php
T
jaredandClaude Sonnet 5 d205a9577a Fix TOCTOU race in bulk operations by row-locking tickets (#34)
processBulkOperation() validated each ticket's status/priority transition
against a pre-transaction snapshot (ticketsById) instead of re-reading
inside the transaction, so two concurrent bulk operations touching the
same ticket could both pass validation against stale data and one
transition could silently clobber the other. Add lockTicketForUpdate(),
which re-fetches a ticket via SELECT ... FOR UPDATE, and use it wherever
the loop needs current status/priority, removing the three redundant
re-reads from the stale snapshot in the bulk_close/bulk_priority/
bulk_status branches.

Verified against real MariaDB with two concurrent OS processes: the
second process blocked ~1.2s on the first's held row lock, then correctly
observed the first's committed status and rejected an otherwise-stale-
data-permitted invalid transition. Also regression-tested normal
bulk_close/bulk_priority operation on real tickets.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0117oBw2jN4kALYeS8HPq4zV
2026-09-11 21:41:41 -04:00

627 lines
28 KiB
PHP

<?php
/**
* BulkOperationsModel - Handles bulk ticket operations (Admin only)
*/
class BulkOperationsModel
{
private $conn;
/** @var WorkflowModel|null Lazily created; only needed by status-changing operations */
private $workflowModel = null;
/** @var CommentModel|null Lazily created; only needed when a status change carries a comment */
private $commentModel = null;
/** @var array<int,string> user_id → display name, resolved once per request */
private $userNames = [];
public function __construct($conn)
{
$this->conn = $conn;
}
/**
* Workflow model, created on first use.
*/
private function workflow(): WorkflowModel
{
if ($this->workflowModel === null) {
require_once dirname(__DIR__) . '/models/WorkflowModel.php';
$this->workflowModel = new WorkflowModel($this->conn);
}
return $this->workflowModel;
}
/**
* Re-fetch a ticket row inside the current transaction with a row lock
* (FOR UPDATE), so a concurrent transaction touching the same row blocks
* until this one commits or rolls back instead of both validating
* against the same stale snapshot. Must be called after
* begin_transaction() and before the row is written.
*/
private function lockTicketForUpdate(string $ticketId): ?array
{
$stmt = $this->conn->prepare("SELECT * FROM tickets WHERE ticket_id = ? FOR UPDATE");
$stmt->bind_param('s', $ticketId);
$stmt->execute();
$row = $stmt->get_result()->fetch_assoc();
$stmt->close();
return $row ?: null;
}
/**
* The status a bulk operation is trying to move tickets into, or null for
* operations that don't change status.
*/
private function targetStatusFor(string $operationType, array $parameters): ?string
{
if ($operationType === 'bulk_close') {
return 'Closed';
}
if ($operationType === 'bulk_status') {
return isset($parameters['status']) ? (string)$parameters['status'] : null;
}
return null;
}
/**
* Create a new bulk operation record
*
* @param string $type Operation type (bulk_close, bulk_assign, bulk_priority)
* @param array $ticketIds Array of ticket IDs
* @param int $userId User performing the operation
* @param array|null $parameters Operation parameters
* @return int|false Operation ID or false on failure
*/
public function createBulkOperation($type, $ticketIds, $userId, $parameters = null)
{
// Validate ticket IDs to prevent injection via implode
$ticketIds = array_values(array_filter(
array_map('strval', $ticketIds),
fn($id) => preg_match('/^[0-9]+$/', $id)
));
if (empty($ticketIds)) {
return false;
}
$ticketIdsStr = implode(',', $ticketIds);
$totalTickets = count($ticketIds);
$parametersJson = $parameters ? json_encode($parameters) : null;
$sql = "INSERT INTO bulk_operations (operation_type, ticket_ids, performed_by, parameters, total_tickets)
VALUES (?, ?, ?, ?, ?)";
$stmt = $this->conn->prepare($sql);
$stmt->bind_param("ssisi", $type, $ticketIdsStr, $userId, $parametersJson, $totalTickets);
if ($stmt->execute()) {
$operationId = $this->conn->insert_id;
$stmt->close();
return $operationId;
}
$stmt->close();
return false;
}
/**
* Process a bulk operation
*
* Uses database transaction to ensure atomicity - either all tickets
* are updated or none are (on failure, changes are rolled back).
*
* @param int $operationId Operation ID
* @param bool $atomic If true, rollback all changes on any failure
* @return array Result with processed and failed counts
*/
public function processBulkOperation($operationId, bool $atomic = false)
{
// Get operation details
$sql = "SELECT * FROM bulk_operations WHERE operation_id = ?";
$stmt = $this->conn->prepare($sql);
$stmt->bind_param("i", $operationId);
$stmt->execute();
$result = $stmt->get_result();
$operation = $result->fetch_assoc();
$stmt->close();
if (!$operation) {
return ['processed' => 0, 'failed' => 0, 'error' => 'Operation not found'];
}
$ticketIds = explode(',', $operation['ticket_ids']);
$parameters = $operation['parameters'] ? json_decode($operation['parameters'], true) : [];
// Validate operation parameters up front so invalid values (out-of-range
// priority, unknown status, nonexistent assignee) are rejected cleanly
// instead of corrupting tickets or throwing mid-transaction.
$paramError = $this->validateOperationParameters($operation['operation_type'], is_array($parameters) ? $parameters : []);
if ($paramError !== null) {
return ['processed' => 0, 'failed' => count($ticketIds), 'error' => $paramError];
}
$processed = 0;
$failed = 0;
$errors = [];
// Status-change notifications collected during the loop below and
// sent only after a successful commit, matching how the single-ticket
// and Bearer API paths never notify for a change that didn't durably
// land (and how an atomic-mode rollback must not fire any at all).
$notificationQueue = [];
// Load required models
require_once dirname(__DIR__) . '/models/TicketModel.php';
require_once dirname(__DIR__) . '/models/AuditLogModel.php';
require_once dirname(__DIR__) . '/models/UserModel.php';
require_once dirname(__DIR__) . '/helpers/NotificationHelper.php';
$ticketModel = new TicketModel($this->conn);
$auditLogModel = new AuditLogModel($this->conn);
$userModel = new UserModel($this->conn);
$actor = $operation['performed_by'] ? $userModel->getUserById((int)$operation['performed_by']) : null;
$changedByDisplay = $actor['display_name'] ?? $actor['username'] ?? null;
// Batch load all tickets in one query to eliminate N+1 problem
$ticketsById = $ticketModel->getTicketsByIds($ticketIds);
// Status-changing operations honour the Workflow Designer. If any ticket in
// the selection needs a comment for its transition, reject the whole batch
// before mutating anything so the client can collect one — a partially
// applied batch is worse than none.
$targetStatus = $this->targetStatusFor($operation['operation_type'], is_array($parameters) ? $parameters : []);
$bulkComment = trim((string)($parameters['comment'] ?? ''));
if ($targetStatus !== null && $bulkComment === '') {
foreach ($ticketIds as $tid) {
$t = $ticketsById[trim($tid)] ?? null;
if (!$t || $t['status'] === $targetStatus) {
continue;
}
if ($this->workflow()->transitionRequiresComment($t['status'], $targetStatus)) {
return [
'processed' => 0,
'failed' => count($ticketIds),
'error' => 'A comment is required to change status from '
. $t['status'] . ' → ' . $targetStatus,
'requires_comment' => true,
];
}
}
}
// Start transaction for data consistency
$this->conn->begin_transaction();
// Attachment files for deleted tickets are removed only AFTER a successful
// commit, so a rollback can't leave tickets with their files already gone.
$filesToDelete = [];
try {
foreach ($ticketIds as $ticketId) {
$ticketId = trim($ticketId);
$success = false;
try {
// Re-fetch and row-lock the ticket inside the transaction for any
// operation that validates against or reads its current fields —
// the pre-transaction $ticketsById snapshot (loaded before
// begin_transaction()) can be stale by the time we get here if a
// concurrent request (a single-ticket edit, or another bulk op)
// changed the row in between. Validating a transition against a
// stale status, or writing back stale title/description/etc.,
// could silently bypass Workflow Designer rules or clobber a
// concurrent edit. FOR UPDATE blocks a concurrent transaction
// from reading/writing this row until ours commits or rolls back.
$needsCurrentTicket = $targetStatus !== null || $operation['operation_type'] === 'bulk_priority';
$currentTicket = $needsCurrentTicket
? $this->lockTicketForUpdate($ticketId)
: ($ticketsById[$ticketId] ?? null);
// bulk_status / bulk_close enforce the same Workflow Designer
// rules as the single-ticket path: a transition the designer
// doesn't define is refused, and requires_comment is honoured
// (checked up front, above). requires_admin is satisfied because
// api/bulk_operation.php already gates the endpoint on admin.
if ($targetStatus !== null) {
if ($currentTicket && $currentTicket['status'] === $targetStatus) {
// Already in the requested state — nothing to do, and
// reporting a no-op as a failure would just confuse.
$processed++;
continue;
}
$allowed = $currentTicket === null || $this->workflow()->isTransitionAllowed(
$currentTicket['status'],
$targetStatus,
true
);
if (!$allowed) {
$failed++;
$errors[] = "Ticket $ticketId: transition not allowed ("
. $currentTicket['status'] . ' → ' . $targetStatus . ')';
continue;
}
}
switch ($operation['operation_type']) {
case 'bulk_close':
// $currentTicket is the fresh, row-locked read from above.
if ($currentTicket) {
$updateResult = $ticketModel->updateTicket([
'ticket_id' => $ticketId,
'title' => $currentTicket['title'],
'description' => $currentTicket['description'],
'category' => $currentTicket['category'],
'type' => $currentTicket['type'],
'status' => 'Closed',
'priority' => $currentTicket['priority']
], $operation['performed_by']);
$success = $updateResult['success'];
if ($success) {
$auditLogModel->log(
$operation['performed_by'],
'update',
'ticket',
$ticketId,
[
'status' => ['from' => $currentTicket['status'], 'to' => 'Closed'],
'bulk_operation_id' => $operationId,
]
);
$notificationQueue[] = [
'ticketId' => $ticketId,
'title' => $currentTicket['title'],
'visibility' => $currentTicket['visibility'] ?? 'public',
'oldStatus' => $currentTicket['status'],
'newStatus' => 'Closed',
];
}
}
break;
case 'bulk_assign':
if (isset($parameters['assigned_to'])) {
$success = $ticketModel->assignTicket($ticketId, $parameters['assigned_to'], $operation['performed_by']);
if ($success) {
$auditLogModel->log(
$operation['performed_by'],
'assign',
'ticket',
$ticketId,
['assigned_to' => $parameters['assigned_to'], 'bulk_operation_id' => $operationId]
);
}
}
break;
case 'bulk_priority':
if (isset($parameters['priority'])) {
// $currentTicket is the fresh, row-locked read from above.
if ($currentTicket) {
$updateResult = $ticketModel->updateTicket([
'ticket_id' => $ticketId,
'title' => $currentTicket['title'],
'description' => $currentTicket['description'],
'category' => $currentTicket['category'],
'type' => $currentTicket['type'],
'status' => $currentTicket['status'],
'priority' => $parameters['priority']
], $operation['performed_by']);
$success = $updateResult['success'];
if ($success) {
$auditLogModel->log(
$operation['performed_by'],
'update',
'ticket',
$ticketId,
['priority' => $parameters['priority'], 'bulk_operation_id' => $operationId]
);
}
}
}
break;
case 'bulk_status':
if (isset($parameters['status'])) {
// $currentTicket is the fresh, row-locked read from above.
if ($currentTicket) {
$updateResult = $ticketModel->updateTicket([
'ticket_id' => $ticketId,
'title' => $currentTicket['title'],
'description' => $currentTicket['description'],
'category' => $currentTicket['category'],
'type' => $currentTicket['type'],
'status' => $parameters['status'],
'priority' => $currentTicket['priority']
], $operation['performed_by']);
$success = $updateResult['success'];
if ($success) {
$auditLogModel->log(
$operation['performed_by'],
'update',
'ticket',
$ticketId,
[
'status' => ['from' => $currentTicket['status'], 'to' => $parameters['status']],
'bulk_operation_id' => $operationId,
]
);
$notificationQueue[] = [
'ticketId' => $ticketId,
'title' => $currentTicket['title'],
'visibility' => $currentTicket['visibility'] ?? 'public',
'oldStatus' => $currentTicket['status'],
'newStatus' => $parameters['status'],
];
}
}
}
break;
case 'bulk_delete':
$success = $ticketModel->deleteTicket($ticketId, $filesToDelete);
if ($success) {
$auditLogModel->log(
$operation['performed_by'],
'delete',
'ticket',
$ticketId,
['bulk_operation_id' => $operationId]
);
}
break;
}
if ($success) {
// Persist the status-change reason as a real comment, so a
// bulk close is as auditable on the ticket as a single close
// (where the client posts the comment before updating).
if ($targetStatus !== null && $bulkComment !== '') {
$this->postBulkComment($ticketId, (int)$operation['performed_by'], $bulkComment);
}
$processed++;
} else {
$failed++;
$errors[] = "Ticket $ticketId: Update failed";
}
} catch (Exception $e) {
$failed++;
$errors[] = "Ticket $ticketId: " . $e->getMessage();
error_log("Bulk operation error for ticket $ticketId: " . $e->getMessage());
}
}
// If atomic mode and any failures, rollback everything
if ($atomic && $failed > 0) {
$this->conn->rollback();
error_log("Bulk operation $operationId rolled back due to $failed failures");
// Update operation status as failed
$sql = "UPDATE bulk_operations SET status = 'failed', processed_tickets = 0, failed_tickets = ?,
completed_at = NOW() WHERE operation_id = ?";
$stmt = $this->conn->prepare($sql);
$stmt->bind_param("ii", $failed, $operationId);
$stmt->execute();
$stmt->close();
return [
'processed' => 0,
'failed' => $failed,
'rolled_back' => true,
'errors' => $errors
];
}
// Commit the transaction
$this->conn->commit();
// Now that the DB delete is durable, remove the physical files. Files
// are deleted first; directory entries (no trailing filename) last.
foreach ($filesToDelete as $path) {
if (is_dir($path)) {
@rmdir($path); // only succeeds if empty
} elseif (file_exists($path)) {
@unlink($path);
}
}
// Fire the same Matrix/watcher notifications the single-ticket and
// Bearer API status-change paths send, now that every change in
// this batch is durably committed. Best-effort: a notification
// failure must never turn an otherwise-successful bulk operation
// into an error.
foreach ($notificationQueue as $n) {
try {
NotificationHelper::sendStatusChangeNotification(
$n['ticketId'],
$n['oldStatus'],
$n['newStatus'],
$n['title'],
$changedByDisplay,
$n['visibility']
);
NotificationHelper::notifyWatchers(
$this->conn,
$n['ticketId'],
$n['title'],
'status_changed',
['old_status' => $n['oldStatus'], 'new_status' => $n['newStatus'], 'changed_by' => $changedByDisplay],
(int)$operation['performed_by'],
$n['visibility']
);
} catch (Throwable $e) {
error_log("Bulk operation $operationId: notification failed for ticket {$n['ticketId']}: " . $e->getMessage());
}
}
} catch (Exception $e) {
// Rollback on any unexpected error
$this->conn->rollback();
error_log("Bulk operation $operationId failed with exception: " . $e->getMessage());
return [
'processed' => 0,
'failed' => count($ticketIds),
'error' => 'Transaction failed: ' . $e->getMessage(),
'rolled_back' => true
];
}
// Update operation status. This is bookkeeping only and runs after the
// ticket changes are committed, so a failure here (e.g. the status column
// not yet widened by 001_widen_bulk_operations_status.sql on an instance
// deployed ahead of its migrations) must not turn a completed operation
// into an error response.
try {
$status = $failed > 0 ? 'completed_with_errors' : 'completed';
$sql = "UPDATE bulk_operations SET status = ?, processed_tickets = ?, failed_tickets = ?,
completed_at = NOW() WHERE operation_id = ?";
$stmt = $this->conn->prepare($sql);
$stmt->bind_param("siii", $status, $processed, $failed, $operationId);
$stmt->execute();
$stmt->close();
} catch (Throwable $e) {
error_log("Bulk operation $operationId completed but status bookkeeping failed: " . $e->getMessage());
}
$result = ['processed' => $processed, 'failed' => $failed];
if (!empty($errors)) {
$result['errors'] = $errors;
}
return $result;
}
/**
* Validate the parameters for a bulk operation before any ticket is mutated.
*
* @return string|null Error message, or null if the parameters are valid
*/
private function validateOperationParameters(string $type, array $parameters): ?string
{
switch ($type) {
case 'bulk_priority':
if (!isset($parameters['priority'])) {
return 'Missing priority parameter';
}
$priority = $parameters['priority'];
// tickets.priority has a CHECK constraint (between 1 and 6).
if (!is_numeric($priority) || (int)$priority < 1 || (int)$priority > 6) {
return 'Invalid priority: must be between 1 and 6';
}
break;
case 'bulk_status':
if (!isset($parameters['status'])) {
return 'Missing status parameter';
}
$validStatuses = $GLOBALS['config']['TICKET_STATUSES']
?? ['Open', 'Pending', 'In Progress', 'Closed'];
if (!in_array($parameters['status'], $validStatuses, true)) {
return 'Invalid status value';
}
break;
case 'bulk_assign':
if (!isset($parameters['assigned_to'])) {
return 'Missing assigned_to parameter';
}
$assignedTo = $parameters['assigned_to'];
if (!is_numeric($assignedTo) || (int)$assignedTo <= 0 || !$this->userExists((int)$assignedTo)) {
return 'Invalid assigned_to: user does not exist';
}
break;
}
return null;
}
/**
* Post the bulk status-change reason as a comment on one ticket.
*
* Runs inside the caller's transaction, so a rollback drops the comment along
* with the status change.
*/
private function postBulkComment(string $ticketId, int $userId, string $text): void
{
require_once dirname(__DIR__) . '/models/CommentModel.php';
if ($this->commentModel === null) {
$this->commentModel = new CommentModel($this->conn);
}
if (!isset($this->userNames[$userId])) {
$stmt = $this->conn->prepare(
"SELECT COALESCE(NULLIF(display_name, ''), username) AS name FROM users WHERE user_id = ? LIMIT 1"
);
$stmt->bind_param("i", $userId);
$stmt->execute();
$row = $stmt->get_result()->fetch_assoc();
$stmt->close();
$this->userNames[$userId] = $row['name'] ?? 'User';
}
$this->commentModel->addComment($ticketId, [
'user_name' => $this->userNames[$userId],
'comment_text' => $text,
'markdown_enabled' => 0,
], $userId);
}
/**
* Check whether a user ID exists.
*/
private function userExists(int $userId): bool
{
$stmt = $this->conn->prepare("SELECT 1 FROM users WHERE user_id = ? LIMIT 1");
$stmt->bind_param("i", $userId);
$stmt->execute();
$exists = $stmt->get_result()->num_rows > 0;
$stmt->close();
return $exists;
}
/**
* Get bulk operation by ID
*
* @param int $operationId Operation ID
* @return array|null Operation record or null
*/
public function getOperationById($operationId)
{
$sql = "SELECT * FROM bulk_operations WHERE operation_id = ?";
$stmt = $this->conn->prepare($sql);
$stmt->bind_param("i", $operationId);
$stmt->execute();
$result = $stmt->get_result();
$operation = $result->fetch_assoc();
$stmt->close();
return $operation;
}
/**
* Get bulk operations performed by a user
*
* @param int $userId User ID
* @param int $limit Result limit
* @return array Array of operations
*/
public function getOperationsByUser($userId, $limit = 50)
{
$sql = "SELECT * FROM bulk_operations WHERE performed_by = ?
ORDER BY created_at DESC LIMIT ?";
$stmt = $this->conn->prepare($sql);
$stmt->bind_param("ii", $userId, $limit);
$stmt->execute();
$result = $stmt->get_result();
$operations = [];
while ($row = $result->fetch_assoc()) {
if ($row['parameters']) {
$row['parameters'] = json_decode($row['parameters'], true);
}
$operations[] = $row;
}
$stmt->close();
return $operations;
}
}