diff --git a/README.md b/README.md index 0177c49..f9c7d4d 100644 --- a/README.md +++ b/README.md @@ -97,12 +97,15 @@ The following features are intentionally **not planned** for this system: - **Admin UI**: Generate and manage API keys at `/admin/api-keys` (paginated) - **Bearer Token Auth**: Use API keys with `Authorization: Bearer YOUR_KEY` header - **Key Scopes**: `read` (GET only) or `read_write` (create/comment/close). A `read` key cannot mutate anything, including creating tickets. Existing keys default to `read_write`. +- **Visibility Scope**: keys default to **public-visibility tickets only** — Confidential and Internal tickets are excluded from `/api/tickets_api.php`, same as they'd be for a regular user with no special access. Check **See all visibility** when generating a key only if that specific integration genuinely needs the full queue; this is a deliberate opt-in, not something a key gets by having `read_write` scope or any other setting. - **Expiration**: Optional expiration dates for keys - **Revocation**: Revoke compromised keys instantly ### Bearer API (automation / triage) All Bearer-authenticated, rate-limited, and (like `create_ticket_api.php`) exempt from Authelia at the reverse proxy — the API key is the only credential. Comments/closes made via the API are attributed to the **key's name** (linked to the key's owner). +`/api/tickets_api.php` filters by ticket visibility according to the key's **Visibility Scope** setting above (public-only by default); every other Bearer endpoint below operates on a single ticket_id supplied by the caller and does not filter a list, so this setting doesn't apply to them. + | Endpoint | Method | Scope | Purpose | |----------|--------|-------|---------| | `/create_ticket_api.php` | POST | read_write | Create a ticket (hwmonDaemon, external tools) | @@ -522,6 +525,9 @@ Add to crontab for recurring tickets and maintenance cleanup: # Delete orphaned upload files with no attachment row, past a 24h grace period (daily). # Add --dry-run to preview without deleting. 0 4 * * * php /path/to/tinkertickets/scripts/cleanup_orphan_uploads.php + +# Retry failed Matrix webhook notifications (exponential backoff, every 5 minutes) +*/5 * * * * php /path/to/tinkertickets/cron/retry_failed_notifications.php ``` ### 3. File Uploads diff --git a/api/generate_api_key.php b/api/generate_api_key.php index 394c203..936d6a2 100644 --- a/api/generate_api_key.php +++ b/api/generate_api_key.php @@ -67,6 +67,7 @@ try { $keyName = trim($input['key_name'] ?? ''); $expiresInDays = $input['expires_in_days'] ?? null; $scope = $input['scope'] ?? 'read_write'; + $seeAllVisibility = !empty($input['see_all_visibility']); if (empty($keyName)) { http_response_code(400); @@ -100,7 +101,7 @@ try { // Generate API key $apiKeyModel = new ApiKeyModel($conn); - $result = $apiKeyModel->createKey($keyName, $_SESSION['user']['user_id'], $expiresInDays, $scope); + $result = $apiKeyModel->createKey($keyName, $_SESSION['user']['user_id'], $expiresInDays, $scope, $seeAllVisibility); if (!$result['success']) { throw new Exception($result['error'] ?? "Failed to generate API key"); @@ -113,7 +114,7 @@ try { 'create', 'api_key', $result['key_id'], - ['key_name' => $keyName, 'expires_in_days' => $expiresInDays, 'scope' => $scope] + ['key_name' => $keyName, 'expires_in_days' => $expiresInDays, 'scope' => $scope, 'see_all_visibility' => $seeAllVisibility] ); // Clear output buffer @@ -127,6 +128,7 @@ try { 'key_prefix' => $result['key_prefix'], 'key_id' => $result['key_id'], 'scope' => $result['scope'], + 'see_all_visibility' => $result['see_all_visibility'], 'expires_at' => $result['expires_at'] ]); } catch (Exception $e) { diff --git a/api/tickets_api.php b/api/tickets_api.php index 95e220c..7bb85e0 100644 --- a/api/tickets_api.php +++ b/api/tickets_api.php @@ -4,8 +4,10 @@ * tickets_api.php — Bearer-key read endpoint (list/triage + read-one). * * GET only. Requires 'read' scope (a 'read_write' key also satisfies it). - * Acts as a trusted automation/server credential: reads return the full queue - * (no per-user visibility filtering). + * By default, a key only sees public-visibility tickets — Confidential and + * Internal tickets are excluded, the same as they would be for a regular user + * with no special access. An admin can mark a specific key see_all_visibility + * (API Key Management) when it genuinely needs the full queue. * * GET ?ticket_id=NNN -> {success, ticket, comments} * GET ?status=&priority=&host= -> {success, tickets, page, total, pages} @@ -48,6 +50,20 @@ try { // Reads only need the 'read' scope. $apiKeyAuth->requireScope('read'); +// Keys are public-ticket-only by default (#70) — a key must be explicitly +// marked see_all_visibility to bypass Confidential/Internal restrictions. +// Reuse TicketModel's existing per-user visibility plumbing with a synthetic +// "no special access" user rather than a separate SQL path, so this stays in +// lockstep with however visibility rules evolve for real users. user_id is +// -1, not 0: canUserAccessTicket() does a PHP-level (int) cast for the +// confidential-ticket check, and (int)null === 0, so an unassigned +// confidential ticket's assigned_to would otherwise false-positive-match a +// synthetic user_id of 0. No real user_id is ever <= 0, so -1 can't collide. +$keyContext = $apiKeyAuth->getKeyContext(); +$visibilityUser = !empty($keyContext['see_all_visibility']) + ? null + : ['user_id' => -1, 'is_admin' => false, 'groups' => '']; + if ($_SERVER['REQUEST_METHOD'] !== 'GET') { http_response_code(405); echo json_encode(['success' => false, 'error' => 'Method not allowed. Use GET.']); @@ -67,6 +83,15 @@ if (isset($_GET['ticket_id']) && trim((string)$_GET['ticket_id']) !== '') { exit; } + // A public-only key gets a plain 404 for a non-public ticket — same as a + // regular user hitting a ticket they can't see — rather than a 403 that + // would confirm the ticket exists. + if ($visibilityUser !== null && !$ticketModel->canUserAccessTicket($ticket, $visibilityUser)) { + http_response_code(404); + echo json_encode(['success' => false, 'error' => 'Ticket not found']); + exit; + } + // Flat list of comments (newest first) — same fetch the ticket view uses. $commentModel = new CommentModel($conn); $comments = $commentModel->getCommentsByTicketId($ticketId, false); @@ -114,8 +139,8 @@ if (isset($_GET['host']) && trim((string)$_GET['host']) !== '') { $search = trim((string)$_GET['host']); } -// user = null => getAllTickets skips visibility filtering and returns the full -// queue (this is a trusted server credential, not an end user). +// $visibilityUser is null (skip filtering, full queue) only for a key marked +// see_all_visibility; otherwise it restricts to public tickets (see above). $result = $ticketModel->getAllTickets( $page, $limit, @@ -126,7 +151,7 @@ $result = $ticketModel->getAllTickets( null, $search, $filters, - null + $visibilityUser ); echo json_encode([ diff --git a/api/watch_ticket.php b/api/watch_ticket.php index 40e9867..a9448f5 100644 --- a/api/watch_ticket.php +++ b/api/watch_ticket.php @@ -9,6 +9,7 @@ require_once __DIR__ . '/bootstrap.php'; require_once dirname(__DIR__) . '/models/TicketModel.php'; +require_once dirname(__DIR__) . '/models/AuditLogModel.php'; $data = json_decode(file_get_contents('php://input'), true) ?? []; @@ -43,6 +44,7 @@ if ($_SERVER['REQUEST_METHOD'] === 'POST') { ); $stmt->bind_param("si", $ticketId, $userId); $stmt->execute(); + $rowsChanged = $stmt->affected_rows; $stmt->close(); } else { $stmt = $conn->prepare( @@ -50,9 +52,17 @@ if ($_SERVER['REQUEST_METHOD'] === 'POST') { ); $stmt->bind_param("si", $ticketId, $userId); $stmt->execute(); + $rowsChanged = $stmt->affected_rows; $stmt->close(); } + // Only log an actual state change — INSERT IGNORE/DELETE are no-ops when + // the user was already watching/not watching, and that shouldn't show up + // in the ticket's timeline as a new event. + if ($rowsChanged > 0) { + (new AuditLogModel($conn))->log($userId, $action, 'ticket', $ticketId); + } + // Return updated state $countStmt = $conn->prepare( "SELECT COUNT(*) as cnt FROM ticket_watchers WHERE ticket_id = ?" diff --git a/cron/retry_failed_notifications.php b/cron/retry_failed_notifications.php new file mode 100644 index 0000000..36e3b19 --- /dev/null +++ b/cron/retry_failed_notifications.php @@ -0,0 +1,133 @@ +#!/usr/bin/env php +getMessage()); + exit(1); +} + +// Process a bounded batch per run so one cron tick can't run indefinitely if +// the queue has backed up. +$batchLimit = 50; + +$stmt = $conn->prepare( + "SELECT retry_id, payload, attempts, max_attempts + FROM notification_retry_queue + WHERE next_attempt_at <= NOW() AND attempts < max_attempts + ORDER BY retry_id ASC + LIMIT ?" +); +$stmt->bind_param('i', $batchLimit); +$stmt->execute(); +$dueRows = $stmt->get_result()->fetch_all(MYSQLI_ASSOC); +$stmt->close(); + +if (empty($dueRows)) { + logMessage('No notifications due for retry.'); + exit(0); +} + +$succeeded = 0; +$failed = 0; +$exhausted = 0; + +foreach ($dueRows as $row) { + $payload = json_decode($row['payload'], true); + if (!is_array($payload)) { + // Corrupt row — can't retry something unparseable. Remove it rather + // than retrying forever against a row that will never succeed. + $del = $conn->prepare("DELETE FROM notification_retry_queue WHERE retry_id = ?"); + $del->bind_param('i', $row['retry_id']); + $del->execute(); + $del->close(); + logMessage("Discarded retry #{$row['retry_id']}: payload is not valid JSON"); + continue; + } + + $result = NotificationHelper::attemptDelivery($webhookUrl, $payload); + + if ($result['success']) { + $del = $conn->prepare("DELETE FROM notification_retry_queue WHERE retry_id = ?"); + $del->bind_param('i', $row['retry_id']); + $del->execute(); + $del->close(); + $succeeded++; + logMessage("Retry #{$row['retry_id']} succeeded (attempt " . ((int)$row['attempts'] + 1) . ')'); + continue; + } + + $newAttempts = (int)$row['attempts'] + 1; + if ($newAttempts >= (int)$row['max_attempts']) { + // Exhausted: leave the row (attempts is now == max_attempts, so the + // WHERE clause above naturally excludes it from future runs) rather + // than deleting it, so it stays visible for manual investigation. + $upd = $conn->prepare( + "UPDATE notification_retry_queue SET attempts = ?, last_error = ? WHERE retry_id = ?" + ); + $upd->bind_param('isi', $newAttempts, $result['error'], $row['retry_id']); + $upd->execute(); + $upd->close(); + $exhausted++; + logMessage("Retry #{$row['retry_id']} exhausted after {$newAttempts} attempts: {$result['error']}"); + continue; + } + + $delayMinutes = nextAttemptDelayMinutes($newAttempts); + $upd = $conn->prepare( + "UPDATE notification_retry_queue + SET attempts = ?, last_error = ?, next_attempt_at = DATE_ADD(NOW(), INTERVAL ? MINUTE) + WHERE retry_id = ?" + ); + $upd->bind_param('isii', $newAttempts, $result['error'], $delayMinutes, $row['retry_id']); + $upd->execute(); + $upd->close(); + $failed++; + logMessage("Retry #{$row['retry_id']} failed (attempt {$newAttempts}), next attempt in {$delayMinutes}m: {$result['error']}"); +} + +logMessage("Done: {$succeeded} succeeded, {$failed} rescheduled, {$exhausted} exhausted (of " . count($dueRows) . ' processed)'); diff --git a/helpers/NotificationHelper.php b/helpers/NotificationHelper.php index f4590e5..6535f2c 100644 --- a/helpers/NotificationHelper.php +++ b/helpers/NotificationHelper.php @@ -7,13 +7,16 @@ class NotificationHelper { // ─── Internal: fire a webhook ───────────────────────────────────────────── - private static function fire(array $payload): void + /** + * POST a payload to the configured Matrix webhook and report the raw + * result. Shared by fire() (best-effort, queues on failure) and + * cron/retry_failed_notifications.php (retries a previously-queued + * payload) so both use identical request handling. + * + * @return array{success: bool, http_code: ?int, error: ?string} + */ + public static function attemptDelivery(string $webhookUrl, array $payload): array { - $webhookUrl = $GLOBALS['config']['MATRIX_WEBHOOK_URL'] ?? null; - if (empty($webhookUrl)) { - return; - } - $ch = curl_init($webhookUrl); curl_setopt($ch, CURLOPT_HTTPHEADER, ['Content-Type: application/json']); curl_setopt($ch, CURLOPT_POST, 1); @@ -21,10 +24,11 @@ class NotificationHelper curl_setopt($ch, CURLOPT_RETURNTRANSFER, true); curl_setopt($ch, CURLOPT_TIMEOUT, 10); // A slow-but-not-fully-hung hookshot endpoint could otherwise add up - // to the full CURLOPT_TIMEOUT per fire() call, and a single request - // can call fire() (via notifyWatchers/sendCommentNotification/etc.) - // more than once sequentially — capping just the connect phase keeps - // that from stacking into tens of seconds of added latency. + // to the full CURLOPT_TIMEOUT per call, and a single request can + // trigger more than one notification sequentially (via + // notifyWatchers/sendCommentNotification/etc.) — capping just the + // connect phase keeps that from stacking into tens of seconds of + // added latency. curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, 3); $response = curl_exec($ch); @@ -32,12 +36,54 @@ class NotificationHelper $curlError = curl_error($ch); curl_close($ch); - $id = $payload['ticket_id'] ?? '?'; if ($curlError) { - error_log("Matrix webhook cURL error for ticket #{$id}: {$curlError}"); - } elseif ($httpCode < 200 || $httpCode >= 300) { - error_log("Matrix webhook failed for ticket #{$id}. HTTP {$httpCode}: {$response}"); + return ['success' => false, 'http_code' => null, 'error' => $curlError]; } + if ($httpCode < 200 || $httpCode >= 300) { + return ['success' => false, 'http_code' => $httpCode, 'error' => "HTTP {$httpCode}: {$response}"]; + } + return ['success' => true, 'http_code' => $httpCode, 'error' => null]; + } + + /** + * Persist a failed payload for later retry by + * cron/retry_failed_notifications.php. Best-effort: a DB failure here + * must not throw back into the original (already-failed) notification + * attempt — it just means this particular failure isn't retried, no + * worse than the pre-existing behavior. + */ + private static function queueForRetry(array $payload, string $error): void + { + try { + require_once dirname(__DIR__) . '/helpers/Database.php'; + $conn = Database::getConnection(); + $stmt = $conn->prepare( + "INSERT INTO notification_retry_queue (payload, last_error) VALUES (?, ?)" + ); + $payloadJson = json_encode($payload); + $stmt->bind_param("ss", $payloadJson, $error); + $stmt->execute(); + $stmt->close(); + } catch (Throwable $e) { + error_log('NotificationHelper: failed to queue notification for retry: ' . $e->getMessage()); + } + } + + private static function fire(array $payload): void + { + $webhookUrl = $GLOBALS['config']['MATRIX_WEBHOOK_URL'] ?? null; + if (empty($webhookUrl)) { + return; + } + + $result = self::attemptDelivery($webhookUrl, $payload); + if ($result['success']) { + return; + } + + $id = $payload['ticket_id'] ?? '?'; + error_log("Matrix webhook failed for ticket #{$id}: {$result['error']}"); + self::queueForRetry($payload, $result['error']); } private static function notifyUsers(): array diff --git a/middleware/ApiKeyAuth.php b/middleware/ApiKeyAuth.php index cf37670..cbf900e 100644 --- a/middleware/ApiKeyAuth.php +++ b/middleware/ApiKeyAuth.php @@ -37,6 +37,7 @@ class ApiKeyAuth { $this->keyContext = [ 'scope' => $keyData['scope'] ?? 'read_write', + 'see_all_visibility' => !empty($keyData['see_all_visibility']), 'key_name' => $keyData['key_name'] ?? null, 'created_by' => $keyData['created_by'] ?? null, 'api_key_id' => $keyData['api_key_id'] ?? null, @@ -46,7 +47,7 @@ class ApiKeyAuth /** * Get the context of the authenticated API key. * - * @return array|null ['scope', 'key_name', 'created_by', 'api_key_id'] or null + * @return array|null ['scope', 'see_all_visibility', 'key_name', 'created_by', 'api_key_id'] or null */ public function getKeyContext(): ?array { diff --git a/migrations/006_api_key_visibility_scope.sql b/migrations/006_api_key_visibility_scope.sql new file mode 100644 index 0000000..feb1797 --- /dev/null +++ b/migrations/006_api_key_visibility_scope.sql @@ -0,0 +1,13 @@ +-- Restrict Bearer API keys to public-visibility tickets by default (#70). +-- +-- Previously any 'read'-scope key bypassed ticket visibility entirely — +-- Confidential and Internal tickets were readable by any key, regardless +-- of who it was issued to. see_all_visibility is an explicit opt-in an +-- admin sets per-key when a key genuinely needs to see non-public tickets; +-- it defaults to 0 (public-only) for both new and existing keys, since the +-- prior blanket-access behavior is the thing being restricted. +-- +-- Safe to re-run. + +ALTER TABLE `api_keys` + ADD COLUMN IF NOT EXISTS `see_all_visibility` tinyint(1) NOT NULL DEFAULT 0 AFTER `scope`; diff --git a/migrations/007_notification_retry_queue.sql b/migrations/007_notification_retry_queue.sql new file mode 100644 index 0000000..fa995b8 --- /dev/null +++ b/migrations/007_notification_retry_queue.sql @@ -0,0 +1,20 @@ +-- Queue for Matrix webhook notifications that failed to send (#78). +-- +-- NotificationHelper::fire() previously logged a failed webhook post via +-- error_log() only, with no retry and no persistent record — once a +-- notification failed, it was gone. Failed payloads are now queued here and +-- retried by cron/retry_failed_notifications.php with exponential backoff. +-- +-- Safe to re-run. + +CREATE TABLE IF NOT EXISTS `notification_retry_queue` ( + `retry_id` int(11) NOT NULL AUTO_INCREMENT, + `payload` longtext CHARACTER SET utf8mb4 COLLATE utf8mb4_bin NOT NULL CHECK (json_valid(`payload`)), + `attempts` int(11) NOT NULL DEFAULT 0, + `max_attempts` int(11) NOT NULL DEFAULT 6, + `next_attempt_at` timestamp NULL DEFAULT current_timestamp(), + `last_error` varchar(500) DEFAULT NULL, + `created_at` timestamp NULL DEFAULT current_timestamp(), + PRIMARY KEY (`retry_id`), + KEY `idx_next_attempt` (`next_attempt_at`) +) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_general_ci; diff --git a/models/ApiKeyModel.php b/models/ApiKeyModel.php index 9213030..1af924a 100644 --- a/models/ApiKeyModel.php +++ b/models/ApiKeyModel.php @@ -19,9 +19,11 @@ class ApiKeyModel * @param int $createdBy User ID who created the key * @param int|null $expiresInDays Number of days until expiration (null for no expiration) * @param string $scope Access scope: 'read' or 'read_write' (default 'read_write') + * @param bool $seeAllVisibility If true, the key bypasses ticket visibility (Confidential/ + * Internal included); defaults to false (public tickets only) * @return array Array with 'success', 'api_key' (plaintext), 'key_prefix', 'scope', 'error' */ - public function createKey($keyName, $createdBy, $expiresInDays = null, $scope = 'read_write') + public function createKey($keyName, $createdBy, $expiresInDays = null, $scope = 'read_write', $seeAllVisibility = false) { // Validate the requested scope — only the two known values are allowed if (!in_array($scope, ['read', 'read_write'], true)) { @@ -47,11 +49,12 @@ class ApiKeyModel } // Insert API key into database + $seeAllVisibilityInt = $seeAllVisibility ? 1 : 0; $stmt = $this->conn->prepare( - "INSERT INTO api_keys (key_name, key_hash, key_prefix, scope, created_by, expires_at) " - . "VALUES (?, ?, ?, ?, ?, ?)" + "INSERT INTO api_keys (key_name, key_hash, key_prefix, scope, see_all_visibility, created_by, expires_at) " + . "VALUES (?, ?, ?, ?, ?, ?, ?)" ); - $stmt->bind_param("ssssis", $keyName, $keyHash, $keyPrefix, $scope, $createdBy, $expiresAt); + $stmt->bind_param("ssssiis", $keyName, $keyHash, $keyPrefix, $scope, $seeAllVisibilityInt, $createdBy, $expiresAt); if ($stmt->execute()) { $keyId = $this->conn->insert_id; @@ -63,6 +66,7 @@ class ApiKeyModel 'key_prefix' => $keyPrefix, 'key_id' => $keyId, 'scope' => $scope, + 'see_all_visibility' => $seeAllVisibility, 'expires_at' => $expiresAt ]; } else { @@ -114,6 +118,13 @@ class ApiKeyModel $keyData['scope'] = 'read_write'; } + // Unlike scope's backward-compatible fallback above, an un-migrated or + // null see_all_visibility defaults to the RESTRICTIVE value (public + // tickets only) — this column exists specifically to lock down a + // previously-unrestricted default, so a missing value must not fall + // back to the permissive behavior it's replacing. + $keyData['see_all_visibility'] = !empty($keyData['see_all_visibility']); + // Check expiration if ($keyData['expires_at'] !== null) { $expiresAt = strtotime($keyData['expires_at']); diff --git a/models/AuditLogModel.php b/models/AuditLogModel.php index dd691cc..001abcb 100644 --- a/models/AuditLogModel.php +++ b/models/AuditLogModel.php @@ -20,7 +20,8 @@ class AuditLogModel private const VALID_ACTION_TYPES = [ 'create', 'update', 'delete', 'view', 'security_event', 'login', 'logout', 'assign', 'unassign', 'comment', 'mention', - 'revoke', 'attachment_upload', 'attachment_delete', 'bulk_update' + 'revoke', 'attachment_upload', 'attachment_delete', 'bulk_update', + 'watch', 'unwatch' ]; /** @var array Allowed entity types for filtering */ diff --git a/views/TicketView.php b/views/TicketView.php index f84c4fd..3c65875 100644 --- a/views/TicketView.php +++ b/views/TicketView.php @@ -32,6 +32,8 @@ function getEventIcon(string $actionType): string 'status_change' => '[!]', 'attachment' => '[^]', 'delete' => '[x]', + 'watch' => '[o]', + 'unwatch' => '[o]', default => '[*]', }; } @@ -53,6 +55,10 @@ function formatAction(array $event): string return 'uploaded a file'; case 'delete': return 'deleted a comment'; + case 'watch': + return 'started watching this ticket'; + case 'unwatch': + return 'stopped watching this ticket'; case 'assign': if (is_array($det) && isset($det['assigned_to']['to'])) { $to = $det['assigned_to']['to'] ?: 'Unassigned'; diff --git a/views/admin/ApiKeysView.php b/views/admin/ApiKeysView.php index 24b8e6e..9502d90 100644 --- a/views/admin/ApiKeysView.php +++ b/views/admin/ApiKeysView.php @@ -45,10 +45,18 @@ include __DIR__ . '/../../views/layout_header.php'; +
Scope: read = GET only; read_write = create/comment/close. + By default a key only sees public-visibility tickets — check + See all visibility only if this key genuinely needs Confidential/Internal tickets too.
@@ -74,6 +82,7 @@ include __DIR__ . '/../../views/layout_header.php';