diff --git a/app/config/collections/platform.php b/app/config/collections/platform.php index c9f03e180b..d1a5a9b518 100644 --- a/app/config/collections/platform.php +++ b/app/config/collections/platform.php @@ -1048,6 +1048,17 @@ $platformCollections = [ 'array' => false, 'filters' => [], ], + [ + '$id' => ID::custom('recipientHash'), + 'type' => Database::VAR_STRING, + 'format' => '', + 'size' => 64, + 'signed' => true, + 'required' => true, + 'default' => null, + 'array' => false, + 'filters' => [], + ], [ '$id' => ID::custom('type'), 'type' => Database::VAR_STRING, @@ -1148,9 +1159,9 @@ $platformCollections = [ [ '$id' => ID::custom('_key_recipient'), 'type' => Database::INDEX_UNIQUE, - 'attributes' => ['messageId', 'channel', 'userId', 'teamId'], - 'lengths' => [Database::LENGTH_KEY, 64, Database::LENGTH_KEY, Database::LENGTH_KEY], - 'orders' => [Database::ORDER_ASC, Database::ORDER_ASC, Database::ORDER_ASC, Database::ORDER_ASC], + 'attributes' => ['messageId', 'channel', 'recipientHash'], + 'lengths' => [Database::LENGTH_KEY, 64, 64], + 'orders' => [Database::ORDER_ASC, Database::ORDER_ASC, Database::ORDER_ASC], ], [ '$id' => ID::custom('_key_userId_read'), @@ -1173,6 +1184,34 @@ $platformCollections = [ 'lengths' => [Database::LENGTH_KEY], 'orders' => [Database::ORDER_ASC], ], + [ + '$id' => ID::custom('_key_userId_type'), + 'type' => Database::INDEX_KEY, + 'attributes' => ['userId', 'type'], + 'lengths' => [Database::LENGTH_KEY, 64], + 'orders' => [Database::ORDER_ASC, Database::ORDER_ASC], + ], + [ + '$id' => ID::custom('_key_userId_channel'), + 'type' => Database::INDEX_KEY, + 'attributes' => ['userId', 'channel'], + 'lengths' => [Database::LENGTH_KEY, 64], + 'orders' => [Database::ORDER_ASC, Database::ORDER_ASC], + ], + [ + '$id' => ID::custom('_key_userId_messageId'), + 'type' => Database::INDEX_KEY, + 'attributes' => ['userId', 'messageId'], + 'lengths' => [Database::LENGTH_KEY, Database::LENGTH_KEY], + 'orders' => [Database::ORDER_ASC, Database::ORDER_ASC], + ], + [ + '$id' => ID::custom('_key_userId_projectId'), + 'type' => Database::INDEX_KEY, + 'attributes' => ['userId', 'projectId'], + 'lengths' => [Database::LENGTH_KEY, Database::LENGTH_KEY], + 'orders' => [Database::ORDER_ASC, Database::ORDER_ASC], + ], ], ], diff --git a/app/init/constants.php b/app/init/constants.php index 87c897ecb4..a8696a8fbf 100644 --- a/app/init/constants.php +++ b/app/init/constants.php @@ -263,12 +263,12 @@ const MESSAGE_TYPE_EMAIL = 'email'; const MESSAGE_TYPE_SMS = 'sms'; const MESSAGE_TYPE_PUSH = 'push'; // Notification types -const NOTIFICATION_TYPE_EMAIL = 'email'; -const NOTIFICATION_TYPE_SMS = 'sms'; -const NOTIFICATION_TYPE_PUSH = 'push'; +const NOTIFICATION_TYPE_EMAIL = MESSAGE_TYPE_EMAIL; +const NOTIFICATION_TYPE_SMS = MESSAGE_TYPE_SMS; +const NOTIFICATION_TYPE_PUSH = MESSAGE_TYPE_PUSH; const NOTIFICATION_TYPE_CONSOLE = 'console'; const NOTIFICATION_TYPE_WEBHOOK = 'webhook'; -const RESOURCE_TYPE_ALERTS = 'alerts'; +const ALERT_TRACKING_JWT_TTL = 60 * 60 * 24 * 7; // API key types const API_KEY_STANDARD = 'standard'; const API_KEY_EPHEMERAL = 'ephemeral'; diff --git a/src/Appwrite/Event/Notification.php b/src/Appwrite/Event/Notification.php index b7a31755ba..3c2c9d6eaa 100644 --- a/src/Appwrite/Event/Notification.php +++ b/src/Appwrite/Event/Notification.php @@ -20,7 +20,7 @@ class Notification extends Event /** * Recipients to deliver the notification to. * - * Each entry has an `address` (channel-specific identifier — email, + * Each entry has an `address` (channel-specific identifier: email, * userId, or webhook URL) and a `channel`. Webhook recipients may * additionally carry an optional `signatureKey`; when set, the * webhook adapter signs the request body with HMAC-SHA256 and adds @@ -267,7 +267,13 @@ class Notification extends Event public function reset(): self { + parent::reset(); + $this->project = null; + $this->context = []; + $this->platform = []; + $this->user = null; + $this->userId = null; $this->recipient = ''; $this->name = ''; $this->subject = ''; diff --git a/src/Appwrite/Platform/Modules/Account/Http/Alerts/Track/Get.php b/src/Appwrite/Platform/Modules/Account/Http/Alerts/Track/Get.php index 9a9beab0ff..73cb857cda 100644 --- a/src/Appwrite/Platform/Modules/Account/Http/Alerts/Track/Get.php +++ b/src/Appwrite/Platform/Modules/Account/Http/Alerts/Track/Get.php @@ -66,7 +66,7 @@ class Get extends Action if ($secret !== '' && $jwt !== '') { try { - $decoder = new JWT($secret, 'HS256', 2592000, 0); + $decoder = new JWT($secret, 'HS256', ALERT_TRACKING_JWT_TTL, 0); $decoded = $decoder->decode($jwt); if ( diff --git a/src/Appwrite/Platform/Workers/Notifications.php b/src/Appwrite/Platform/Workers/Notifications.php index d9ac17e592..fc3225d256 100644 --- a/src/Appwrite/Platform/Workers/Notifications.php +++ b/src/Appwrite/Platform/Workers/Notifications.php @@ -16,7 +16,7 @@ use Utopia\Database\Document; use Utopia\Database\Exception\Duplicate as DuplicateException; use Utopia\Database\Helpers\Permission; use Utopia\Database\Helpers\Role; -use Utopia\Database\Query; +use Utopia\Database\Validator\UID; use Utopia\Logger\Log; use Utopia\Messaging\Adapter\Email as EmailAdapter; use Utopia\Messaging\Adapter\Email\SMTP; @@ -32,11 +32,6 @@ class Notifications extends Action protected int $previewMaxLen = 150; protected string $whitespaceCodes = ' ‌​‍‎‏'; - /** - * Tracking pixel JWT lifetime: 30 days. - */ - private const TRACKING_JWT_TTL = 2592000; - /** * @var array */ @@ -76,18 +71,21 @@ class Notifications extends Action $deduplicationKey = $payload['deduplicationKey'] ?? ''; $messageId = $deduplicationKey !== '' ? \md5($deduplicationKey) : ''; - if ($messageId !== '' && $this->alreadyDelivered($dbForPlatform, $messageId)) { - $log->addTag('dedup', 'hit'); - return; - } - $recipients = $this->resolveRecipients($payload); if (empty($recipients)) { throw new Exception('No recipients in payload'); } foreach ($recipients as $recipient) { + $recipient = $this->normalizeRecipient($recipient); $channel = $recipient['channel']; + + if ($messageId !== '' && $this->alreadyDelivered($dbForPlatform, self::buildAlertId($messageId, $recipient))) { + $log->addTag('dedup', 'hit'); + $log->addTag('channel', $channel); + continue; + } + try { $alertId = $this->dispatch($recipient, $messageId, $payload, $project, $register, $dbForPlatform, $log); if ($messageId !== '' && $channel === NOTIFICATION_TYPE_WEBHOOK && $alertId === null) { @@ -120,23 +118,28 @@ class Notifications extends Action } /** - * Look up an existing alert by the indexed `messageId` attribute. - * - * Greptile P1 #1: persistAlert and the Console adapter both write - * compound `$id`s (messageId + recipient hash), so a direct - * `getDocument($messageId)` would always miss. Query the attribute. + * @param array{address: string, channel: string, signatureKey?: string, userId?: string, teamId?: string} $recipient + * @return array{address: string, channel: string, signatureKey?: string, userId?: string, teamId?: string} */ - private function alreadyDelivered(Database $dbForPlatform, string $messageId): bool + private function normalizeRecipient(array $recipient): array { - try { - $matches = $dbForPlatform->find('alerts', [ - Query::equal('messageId', [$messageId]), - Query::limit(1), - ]); - return !empty($matches); - } catch (Throwable) { - return false; + $recipient['userId'] = $recipient['userId'] ?? ''; + $recipient['teamId'] = $recipient['teamId'] ?? ''; + + if ( + $recipient['channel'] === NOTIFICATION_TYPE_CONSOLE + && $recipient['userId'] === '' + && $recipient['teamId'] === '' + ) { + $recipient['userId'] = $recipient['address']; } + + return $recipient; + } + + private function alreadyDelivered(Database $dbForPlatform, string $alertId): bool + { + return !$dbForPlatform->getDocument('alerts', $alertId)->isEmpty(); } /** @@ -183,7 +186,8 @@ class Notifications extends Action $smtp = $this->resolveSmtpConfig($project); if (empty($smtp) && empty(System::getEnv('_APP_SMTP_HOST'))) { - throw new Exception('Skipped email notification. No SMTP configuration has been set.'); + $log->addTag('email_skipped', 'no_smtp'); + return null; } $type = empty($smtp) ? 'cloud' : 'smtp'; @@ -247,16 +251,10 @@ class Notifications extends Action } $subject = \strip_tags($subjectTemplate->render()); - // Pre-compute the alertId WITHOUT persisting so the tracking pixel - // URL is stable, but defer the database write until after a - // successful adapter send. Persisting first risks a dedup-by-attribute - // hit on retry if SMTP throws — the email would be permanently lost. $deterministicAlertId = $messageId !== '' ? self::buildAlertId($messageId, $recipient) : null; - // Tracking pixel: only injectable when we have a userId AND a - // deterministic alertId AND a signing key. $userId = $recipient['userId'] ?? ''; $opensslKey = System::getEnv('_APP_OPENSSL_KEY_V1'); if ($deterministicAlertId !== null && $userId !== '' && !empty($opensslKey)) { @@ -320,16 +318,9 @@ class Notifications extends Action try { $adapter->send($emailMessage); } catch (Throwable $error) { - // Use 500 for any SMTP delivery failure: timeouts, DNS, refused - // connections, and authentication errors are all infrastructure - // problems, not HTTP-style 401s. The queue infrastructure handles - // retries based on the throw, not the code. throw new Exception('Error sending notification: ' . $error->getMessage(), 500); } - // Persist the alert ONLY after a successful send. The Track endpoint - // tolerates a missing alert row, so a tracking pixel fetched between - // send and persistence is harmless. if ($messageId !== '') { return $this->persistAlert($dbForPlatform, $messageId, $recipient, $payload); } @@ -342,22 +333,22 @@ class Notifications extends Action */ protected function dispatchConsole(array $recipient, string $messageId, array $payload, Database $dbForPlatform): ?string { + $this->validateConsoleRecipient($recipient); + $project = $payload['project'] ?? null; $projectId = \is_array($project) ? ($project['$id'] ?? null) : null; - $title = $payload['subject'] ?? ''; - $body = $payload['body'] ?? ''; $params = $payload['templateParams'] ?? ($payload['variables'] ?? []); - if ($title !== '' && !empty($params)) { - $rendered = Template::fromString($title); - foreach ($params as $key => $value) { - $rendered->setParam('{{' . $key . '}}', (string) $value); - } - $title = \strip_tags($rendered->render()); + $title = self::renderText($payload['subject'] ?? '', $params); + $body = self::renderText($payload['preview'] ?? '', $params); + if ($body === '') { + $body = self::renderText($payload['body'] ?? '', $params); } $userId = $recipient['userId'] ?? $recipient['address']; $teamId = $recipient['teamId'] ?? ''; + $alertId = $messageId !== '' ? self::buildAlertId($messageId, $recipient) : null; + $recipientHash = $messageId !== '' ? self::buildRecipientHash($recipient) : null; $consoleRecipient = []; if ($userId !== '') { @@ -366,6 +357,10 @@ class Notifications extends Action if ($teamId !== '') { $consoleRecipient['teamId'] = $teamId; } + if ($alertId !== null) { + $consoleRecipient['alertId'] = $alertId; + $consoleRecipient['recipientHash'] = $recipientHash; + } $consoleMessage = new ConsoleMessage( recipients: [$consoleRecipient], @@ -379,18 +374,12 @@ class Notifications extends Action $adapter = new ConsoleAdapter($dbForPlatform); $result = $adapter->send($consoleMessage); - // Greptile P1 #4: surface adapter failures. The Console adapter - // catches per-recipient exceptions and reports zero deliveries via - // `deliveredTo`; without this throw the worker would silently - // succeed on a hard write failure. if (($result['deliveredTo'] ?? 0) === 0) { $error = $result['results'][0]['error'] ?? 'unknown error'; throw new Exception('Console alert delivery failed: ' . $error); } - // Adapter persisted the alert, so the action loop must NOT - // call persistAlert again (Greptile P1 #3). - return null; + return $alertId; } /** @@ -444,39 +433,41 @@ class Notifications extends Action */ protected function persistAlert(Database $dbForPlatform, string $messageId, array $recipient, array $payload): string { + $recipient = $this->normalizeRecipient($recipient); + $project = $payload['project'] ?? null; $projectId = \is_array($project) ? ($project['$id'] ?? null) : null; $channel = $recipient['channel']; - $address = $recipient['address']; $userId = $recipient['userId'] ?? ''; $teamId = $recipient['teamId'] ?? ''; - // Console alerts derive userId from address when no explicit - // userId is supplied (matches Console adapter's own bookkeeping). - if ($channel === NOTIFICATION_TYPE_CONSOLE && $userId === '' && $teamId === '') { - $userId = $address; - $recipient['userId'] = $userId; - } - $alertId = self::buildAlertId($messageId, $recipient); + $recipientHash = self::buildRecipientHash($recipient); $permissions = $this->buildAlertPermissions($userId, $teamId); if (empty($permissions)) { $permissions = $payload['permissions'] ?? []; } + $params = $payload['templateParams'] ?? ($payload['variables'] ?? []); + $body = self::renderText($payload['preview'] ?? '', $params); + if ($body === '') { + $body = self::renderText($payload['body'] ?? '', $params); + } + $document = new Document([ '$id' => $alertId, '$permissions' => $permissions, 'messageId' => $messageId, + 'recipientHash' => $recipientHash, 'type' => $payload['type'] ?? 'info', 'channel' => $channel, - 'userId' => $userId !== '' ? $userId : null, - 'teamId' => $teamId !== '' ? $teamId : null, + 'userId' => $userId, + 'teamId' => $teamId, 'projectId' => $projectId, - 'title' => $payload['subject'] ?? '', - 'body' => $payload['body'] ?? '', + 'title' => self::renderText($payload['subject'] ?? '', $params), + 'body' => $body, 'read' => false, ]); @@ -490,21 +481,29 @@ class Notifications extends Action } /** - * Build the deterministic alertId composed of the messageId plus an - * 8-char hash over the recipient identity. Single source of truth used + * Build the deterministic alertId composed of the messageId plus a + * recipient hash. Single source of truth used * by both `persistAlert()` and `dispatchEmail()` so the tracking pixel * URL matches the eventually-persisted row exactly. * * @param array{address: string, channel: string, signatureKey?: string, userId?: string, teamId?: string} $recipient */ private static function buildAlertId(string $messageId, array $recipient): string + { + return \substr($messageId, 0, 19) . '_' . self::buildRecipientHash($recipient); + } + + /** + * @param array{address: string, channel: string, signatureKey?: string, userId?: string, teamId?: string} $recipient + */ + private static function buildRecipientHash(array $recipient): string { $channel = $recipient['channel']; $address = $recipient['address']; $userId = $recipient['userId'] ?? ''; $teamId = $recipient['teamId'] ?? ''; - return $messageId . '_' . \substr(\md5($channel . ':' . $address . ':' . $userId . ':' . $teamId), 0, 8); + return \substr(\md5($channel . ':' . $address . ':' . $userId . ':' . $teamId), 0, 16); } /** @@ -555,16 +554,41 @@ class Notifications extends Action ]; } + private function validateConsoleRecipient(array $recipient): void + { + $validator = new UID(); + + foreach (['userId', 'teamId'] as $key) { + $value = $recipient[$key] ?? ''; + if ($value !== '' && !$validator->isValid($value)) { + throw new Exception('Invalid console alert ' . $key . ': ' . $validator->getDescription()); + } + } + } + + private static function renderText(string $value, array $params): string + { + if ($value !== '' && !empty($params)) { + $template = Template::fromString($value); + foreach ($params as $key => $param) { + $template->setParam('{{' . $key . '}}', (string) $param); + } + $value = $template->render(); + } + + return \trim(\strip_tags($value)); + } + /** * Splice a 1x1 tracking pixel before the last `` tag (or * append at the end if the body has no closing tag). The pixel - * carries a 30-day JWT identifying the alert and user, which the + * carries a signed JWT identifying the alert and user, which the * `/v1/account/alerts/:alertId/track` endpoint verifies before * marking the alert as read. */ private function injectTrackingPixel(string $body, string $alertId, string $userId, string $opensslKey): string { - $jwt = (new JWT($opensslKey, 'HS256', self::TRACKING_JWT_TTL, 0)) + $jwt = (new JWT($opensslKey, 'HS256', ALERT_TRACKING_JWT_TTL, 0)) ->encode([ 'alertId' => $alertId, 'userId' => $userId, diff --git a/src/Appwrite/Platform/Workers/Webhooks.php b/src/Appwrite/Platform/Workers/Webhooks.php index 91485c2906..b46de5a49a 100644 --- a/src/Appwrite/Platform/Workers/Webhooks.php +++ b/src/Appwrite/Platform/Workers/Webhooks.php @@ -219,10 +219,9 @@ class Webhooks extends Action Query::limit(APP_LIMIT_SUBQUERY) ]); - // Webhook-paused alerts go only to project owners — non-owner team members do not receive them. $ownerMemberships = \array_filter( $memberships, - fn (Document $membership) => \in_array('owner', $membership->getAttribute('roles', []), true) + fn (Document $membership) => self::hasOwnerRole($membership) ); if (empty($ownerMemberships)) { @@ -310,4 +309,23 @@ class Webhooks extends Action $queueForNotifications->trigger(); } + + private static function hasOwnerRole(Document $membership): bool + { + $roles = $membership->getAttribute('roles', []); + if (\is_string($roles)) { + $roles = \array_map('trim', \explode(',', $roles)); + } + if (!\is_array($roles)) { + return false; + } + + foreach ($roles as $role) { + if (\is_string($role) && \strtolower($role) === 'owner') { + return true; + } + } + + return false; + } } diff --git a/src/Appwrite/Utopia/Messaging/Adapter/Console.php b/src/Appwrite/Utopia/Messaging/Adapter/Console.php index bb705e59e4..8e93f6557e 100644 --- a/src/Appwrite/Utopia/Messaging/Adapter/Console.php +++ b/src/Appwrite/Utopia/Messaging/Adapter/Console.php @@ -64,8 +64,9 @@ class Console extends Adapter $messageId = $message->getMessageId(); $recipientKey = $userId !== '' ? 'user:' . $userId : 'team:' . $teamId; + $recipientHash = $recipient['recipientHash'] ?? \substr(\md5($recipientKey), 0, 16); $documentId = $messageId !== null - ? $messageId . '_' . \substr(\md5($recipientKey), 0, 8) + ? ($recipient['alertId'] ?? \substr($messageId, 0, 19) . '_' . $recipientHash) : ID::unique(); try { @@ -73,10 +74,11 @@ class Console extends Adapter '$id' => $documentId, '$permissions' => $this->buildPermissions($userId, $teamId), 'messageId' => $messageId, + 'recipientHash' => $recipientHash, 'type' => $message->getType(), 'channel' => self::TYPE, - 'userId' => $userId !== '' ? $userId : null, - 'teamId' => $teamId !== '' ? $teamId : null, + 'userId' => $userId, + 'teamId' => $teamId, 'projectId' => $message->getProjectId(), 'title' => $message->getTitle(), 'body' => $message->getBody(), diff --git a/src/Appwrite/Utopia/Messaging/Messages/Console.php b/src/Appwrite/Utopia/Messaging/Messages/Console.php index 2226927b10..23fc791233 100644 --- a/src/Appwrite/Utopia/Messaging/Messages/Console.php +++ b/src/Appwrite/Utopia/Messaging/Messages/Console.php @@ -7,7 +7,7 @@ use Utopia\Messaging\Message; class Console implements Message { /** - * @param array $recipients + * @param array $recipients */ public function __construct( protected array $recipients, @@ -20,7 +20,7 @@ class Console implements Message } /** - * @return array + * @return array */ public function getRecipients(): array { @@ -28,7 +28,7 @@ class Console implements Message } /** - * @return array + * @return array */ public function getTo(): array { diff --git a/tests/e2e/Services/Notifications/NotificationsBase.php b/tests/e2e/Services/Notifications/NotificationsBase.php index 5423e4de3d..ba4ee6fb26 100644 --- a/tests/e2e/Services/Notifications/NotificationsBase.php +++ b/tests/e2e/Services/Notifications/NotificationsBase.php @@ -12,7 +12,7 @@ use Utopia\System\System; * account-alerts user-facing API. * * The notification worker itself is exercised in unit tests with a pinned - * queue payload — the server side cannot deterministically inject a + * queue payload; the server side cannot deterministically inject a * Notification onto the live queue without an admin endpoint, so the health * portion validates the public contract that ops and KEDA scale on: * @@ -70,7 +70,7 @@ trait NotificationsBase { // Always read alerts as the console-authenticated owner of the team. // The /v1/account/alerts endpoint is platform-scoped (dbForPlatform) and - // requires a session — server-mode API keys do not satisfy it. + // requires a session; server-mode API keys do not satisfy it. $response = $this->client->call(Client::METHOD_GET, '/account/alerts', $this->getConsoleAlertHeaders()); $this->assertSame(200, $response['headers']['status-code']); @@ -197,7 +197,7 @@ trait NotificationsBase // Track endpoint requires `purpose: 'alert_track'` — see C/M7 in // PR #12195 review. Other claim purposes are silently ignored (which // testTrackingPixelRejectsJwtWithoutPurposeClaim covers). - $jwt = (new JWT($secret, 'HS256', 2592000, 0))->encode([ + $jwt = (new JWT($secret, 'HS256', ALERT_TRACKING_JWT_TTL, 0))->encode([ 'alertId' => $alertId, 'userId' => $userId, 'purpose' => 'alert_track', @@ -252,7 +252,7 @@ trait NotificationsBase $userId = $this->getRoot()['$id']; // Mint a JWT with valid alertId/userId but NO purpose claim. - $jwtNoPurpose = (new JWT($secret, 'HS256', 2592000, 0))->encode([ + $jwtNoPurpose = (new JWT($secret, 'HS256', ALERT_TRACKING_JWT_TTL, 0))->encode([ 'alertId' => $alertId, 'userId' => $userId, ]); @@ -278,8 +278,8 @@ trait NotificationsBase $this->assertNotNull($found); $this->assertFalse($found['read'], 'JWT without purpose claim must not flip the read flag'); - // Mint a JWT with a wrong purpose value — same expectation: silently rejected. - $jwtWrongPurpose = (new JWT($secret, 'HS256', 2592000, 0))->encode([ + // Mint a JWT with a wrong purpose value: same expectation, silently rejected. + $jwtWrongPurpose = (new JWT($secret, 'HS256', ALERT_TRACKING_JWT_TTL, 0))->encode([ 'alertId' => $alertId, 'userId' => $userId, 'purpose' => 'something_else', diff --git a/tests/unit/Platform/Workers/NotificationsTest.php b/tests/unit/Platform/Workers/NotificationsTest.php index f5cc60c04c..5f147f3de5 100644 --- a/tests/unit/Platform/Workers/NotificationsTest.php +++ b/tests/unit/Platform/Workers/NotificationsTest.php @@ -177,6 +177,7 @@ class NotificationsTest extends TestCase false, ); $this->database->createAttribute('alerts', 'messageId', Database::VAR_STRING, 255, false); + $this->database->createAttribute('alerts', 'recipientHash', Database::VAR_STRING, 64, true); $this->database->createAttribute('alerts', 'type', Database::VAR_STRING, 64, false, 'info'); $this->database->createAttribute('alerts', 'channel', Database::VAR_STRING, 64, true); $this->database->createAttribute('alerts', 'userId', Database::VAR_STRING, 255, false); @@ -187,15 +188,15 @@ class NotificationsTest extends TestCase $this->database->createAttribute('alerts', 'read', Database::VAR_BOOLEAN, 0, false, false); // Mirror the production `_key_recipient` UNIQUE composite index so the - // duplicate-handling branch in persistAlert (catch DuplicateException → + // duplicate-handling branch in persistAlert (catch DuplicateException -> // return existing alertId) is actually exercised by tests. $this->database->createIndex( 'alerts', '_key_recipient', Database::INDEX_UNIQUE, - ['messageId', 'channel', 'userId', 'teamId'], - [Database::LENGTH_KEY, 64, Database::LENGTH_KEY, Database::LENGTH_KEY], - [Database::ORDER_ASC, Database::ORDER_ASC, Database::ORDER_ASC, Database::ORDER_ASC], + ['messageId', 'channel', 'recipientHash'], + [Database::LENGTH_KEY, 64, 64], + [Database::ORDER_ASC, Database::ORDER_ASC, Database::ORDER_ASC], ); $this->registry = new Registry(); @@ -219,6 +220,16 @@ class NotificationsTest extends TestCase ]); } + private function recipientHash(string $channel, string $address, string $userId = '', string $teamId = ''): string + { + return \substr(\md5($channel . ':' . $address . ':' . $userId . ':' . $teamId), 0, 16); + } + + private function alertId(string $messageId, string $channel, string $address, string $userId = '', string $teamId = ''): string + { + return \substr($messageId, 0, 19) . '_' . $this->recipientHash($channel, $address, $userId, $teamId); + } + public function testDispatchesPerChannelToCorrectAdapter(): void { $worker = new SpyNotifications(); @@ -284,21 +295,9 @@ class NotificationsTest extends TestCase 'deduplicationKey' => 'dup-key', ]; - // First run delivers and persists. $worker->action($this->buildMessage($payload), $this->project, $this->registry, $this->database, $this->log); $this->assertCount(1, $worker->dispatched); - // Manually insert a row with the dedup messageId so alreadyDelivered() returns true. - $messageId = \md5('dup-key'); - $this->database->createDocument('alerts', new Document([ - '$id' => $messageId, - '$permissions' => [Permission::read(Role::any())], - 'messageId' => $messageId, - 'channel' => 'console', - 'title' => 'x', - 'body' => 'y', - ])); - $worker->dispatched = []; $worker->action($this->buildMessage($payload), $this->project, $this->registry, $this->database, $this->log); $this->assertCount(0, $worker->dispatched, 'second invocation must short-circuit on dedup hit'); @@ -391,37 +390,46 @@ class NotificationsTest extends TestCase $this->assertCount(0, $rows, 'failed dispatch must not persist alert'); } - public function testDedupQueriesByAttributeNotById(): void + public function testDedupSkipsOnlyDeliveredRecipientAfterPartialFanoutFailure(): void { $worker = new SpyNotifications(); + $worker->throwOn[NOTIFICATION_TYPE_WEBHOOK] = new \RuntimeException('webhook down'); - // Seed an alert row with an arbitrary $id but the matching dedup - // messageId attribute. If alreadyDelivered() short-circuits via - // getDocument($messageId) it will miss this seed and dispatch - // anyway. Querying by the `messageId` attribute is the only way to - // see the seed. - $messageId = \md5('dup-key'); - $this->database->createDocument('alerts', new Document([ - '$id' => 'random-id-123', - '$permissions' => [Permission::read(Role::any())], - 'messageId' => $messageId, - 'channel' => 'console', - 'title' => 'seed', - 'body' => 'seed', - ])); - + $messageId = \md5('partial-key'); $payload = [ 'project' => ['$id' => 'project-x'], - 'recipients' => [['address' => 'user-1', 'channel' => NOTIFICATION_TYPE_CONSOLE]], + 'recipients' => [ + ['address' => 'user-1', 'channel' => NOTIFICATION_TYPE_CONSOLE, 'userId' => 'user-1'], + ['address' => 'https://hooks.example.test/in', 'channel' => NOTIFICATION_TYPE_WEBHOOK], + ], 'subject' => 'Sub', 'body' => 'B', - 'deduplicationKey' => 'dup-key', + 'deduplicationKey' => 'partial-key', ]; - $worker->action($this->buildMessage($payload), $this->project, $this->registry, $this->database, $this->log); + try { + $worker->action($this->buildMessage($payload), $this->project, $this->registry, $this->database, $this->log); + $this->fail('expected webhook failure to propagate'); + } catch (\RuntimeException $error) { + $this->assertSame('webhook down', $error->getMessage()); + } - $this->assertCount(0, $worker->dispatched, 'attribute-keyed seed must trigger dedup short-circuit'); - $this->assertSame('hit', $this->log->getTags()['dedup'] ?? null); + $rows = $this->database->find('alerts', [ + \Utopia\Database\Query::equal('messageId', [$messageId]), + ]); + $this->assertCount(1, $rows, 'first attempt should persist only the successful console recipient'); + $this->assertSame(NOTIFICATION_TYPE_CONSOLE, $rows[0]->getAttribute('channel')); + + $retry = new SpyNotifications(); + $retry->action($this->buildMessage($payload), $this->project, $this->registry, $this->database, $this->log); + + $this->assertCount(1, $retry->dispatched, 'retry should dispatch only the previously undelivered webhook'); + $this->assertSame(NOTIFICATION_TYPE_WEBHOOK, $retry->dispatched[0]['channel']); + + $rows = $this->database->find('alerts', [ + \Utopia\Database\Query::equal('messageId', [$messageId]), + ]); + $this->assertCount(2, $rows, 'retry must complete the missing recipient without duplicating console'); } public function testConsoleChannelSkipsPersistAlert(): void @@ -539,8 +547,12 @@ class NotificationsTest extends TestCase // provide an OpenSSL key so injectTrackingPixel actually runs. $previousSmtpHost = \getenv('_APP_SMTP_HOST'); $previousOpensslKey = \getenv('_APP_OPENSSL_KEY_V1'); + $previousDomain = \getenv('_APP_DOMAIN'); + $previousConsoleDomain = \getenv('_APP_CONSOLE_DOMAIN'); \putenv('_APP_SMTP_HOST=spy.smtp.test'); \putenv('_APP_OPENSSL_KEY_V1=test-key-32bytes-min-aaaaaaaaaaaaaa'); + \putenv('_APP_DOMAIN=api.example.test'); + \putenv('_APP_CONSOLE_DOMAIN=console.example.test'); try { $worker = new Notifications(); @@ -563,6 +575,8 @@ class NotificationsTest extends TestCase } finally { \putenv($previousSmtpHost === false ? '_APP_SMTP_HOST' : '_APP_SMTP_HOST=' . $previousSmtpHost); \putenv($previousOpensslKey === false ? '_APP_OPENSSL_KEY_V1' : '_APP_OPENSSL_KEY_V1=' . $previousOpensslKey); + \putenv($previousDomain === false ? '_APP_DOMAIN' : '_APP_DOMAIN=' . $previousDomain); + \putenv($previousConsoleDomain === false ? '_APP_CONSOLE_DOMAIN' : '_APP_CONSOLE_DOMAIN=' . $previousConsoleDomain); } $this->assertNotNull($spy->captured, 'SpyEmailAdapter must capture exactly one EmailMessage'); @@ -571,6 +585,8 @@ class NotificationsTest extends TestCase $this->assertStringContainsString(' must be present'); $this->assertStringContainsString('/v1/account/alerts/', $body); $this->assertStringContainsString('/track?jwt=', $body); + $this->assertStringContainsString('http://api.example.test/v1/account/alerts/', \html_entity_decode($body)); + $this->assertStringNotContainsString('console.example.test/v1/account/alerts/', \html_entity_decode($body)); // The pixel must sit BEFORE the last . $lastBodyClose = \strripos($body, ''); @@ -593,7 +609,7 @@ class NotificationsTest extends TestCase // All four unique-index fields populated so the // `_key_recipient` UNIQUE composite (messageId, channel, - // userId, teamId) actually fires — SQL UNIQUE semantics treat + // userId, teamId) actually fires. SQL UNIQUE semantics treat // NULL as not-equal, so any null in the tuple disables it. $payload = [ 'project' => ['$id' => 'project-x'], @@ -629,7 +645,7 @@ class NotificationsTest extends TestCase // action loop's alreadyDelivered() check short-circuits before // persistAlert, so call persistAlert directly to actually hit // the duplicate branch. The deterministic $id collides on the - // primary key → DuplicateException → branch returns the + // primary key -> DuplicateException -> branch returns the // existing alertId without throwing. $reflection = new \ReflectionMethod($worker, 'persistAlert'); $secondAlertId = $reflection->invoke($worker, $this->database, $messageId, $recipient, $payload); @@ -638,13 +654,14 @@ class NotificationsTest extends TestCase // Third write: bypass the deterministic $id path and use a // distinct $id with the same recipient tuple. The - // `_key_recipient` UNIQUE composite must reject it — proving + // `_key_recipient` UNIQUE composite must reject it, proving // the unique-index (not just primary-key) is what backstops the // duplicate-handling branch. $sameTupleDoc = new Document([ '$id' => 'sibling-id-' . \uniqid(), '$permissions' => [Permission::read(Role::any())], 'messageId' => $messageId, + 'recipientHash' => $this->recipientHash(NOTIFICATION_TYPE_EMAIL, 'user@example.test', 'user-7', 'team-7'), 'channel' => NOTIFICATION_TYPE_EMAIL, 'userId' => 'user-7', 'teamId' => 'team-7', @@ -814,7 +831,14 @@ class NotificationsTest extends TestCase ->setTemplate('template-id') ->setTemplateParams(['x' => 1]) ->setDeduplicationKey('dedup') - ->setPermissions([Permission::read(Role::any())]); + ->setPermissions([Permission::read(Role::any())]) + ->setEvent('databases.*.documents.*.create') + ->setParam('databaseId', 'database-1') + ->setPayload(['before' => 'reset'], ['before']) + ->setUser(new Document(['$id' => 'user-before'])) + ->setUserId('user-before') + ->setPlatform(['name' => 'platform-before']) + ->setContext('actor', new Document(['$id' => 'actor-before'])); $event->reset(); @@ -833,6 +857,13 @@ class NotificationsTest extends TestCase $this->assertSame('', $event->getDeduplicationKey(), 'deduplicationKey must reset to empty string'); $this->assertSame([], $event->getPermissions(), 'permissions must reset to empty array'); $this->assertNull($event->getProject(), 'project must reset to null'); + $this->assertSame('', $event->getEvent(), 'inherited event must reset to empty string'); + $this->assertSame([], $event->getParams(), 'inherited params must reset to empty array'); + $this->assertSame([], $event->getPayload(), 'inherited payload must reset to empty array'); + $this->assertNull($event->getUser(), 'inherited user must reset to null'); + $this->assertNull($event->getUserId(), 'inherited userId must reset to null'); + $this->assertSame([], $event->getPlatform(), 'inherited platform must reset to empty array'); + $this->assertNull($event->getContext('actor'), 'inherited context must reset'); } /** @@ -976,6 +1007,7 @@ class NotificationsTest extends TestCase // dispatchEmail's returned alertId must match the row $id (used by the // tracking pixel URL). $this->assertSame($worker->persistedIds[0], $row->getId()); + $this->assertLessThanOrEqual(36, \strlen($row->getId()), 'alert ids must pass the UID route validator'); } /** @@ -1017,11 +1049,10 @@ class NotificationsTest extends TestCase $this->assertSame(NOTIFICATION_TYPE_CONSOLE, $row->getAttribute('channel')); $this->assertFalse($row->getAttribute('read')); - // Per-recipient suffix scheme used by the Console adapter is - // `messageId . '_' . substr(md5('user:' . userId), 0, 8)`. $messageId = \md5('happy-console'); - $expectedId = $messageId . '_' . \substr(\md5('user:u1'), 0, 8); + $expectedId = $this->alertId($messageId, NOTIFICATION_TYPE_CONSOLE, 'console-recipient', 'u1', 't1'); $this->assertSame($expectedId, $row->getId(), 'row $id must match adapter suffix scheme'); + $this->assertLessThanOrEqual(36, \strlen($row->getId()), 'alert ids must pass the UID route validator'); $permissions = $row->getPermissions(); $this->assertContains(Permission::read(Role::user('u1')), $permissions); @@ -1034,6 +1065,86 @@ class NotificationsTest extends TestCase $this->assertSame(0, $worker->persistAlertCalls, 'console channel must NOT trigger action-loop persistAlert'); } + public function testConsoleChannelUsesPreviewBodyInsteadOfRenderedEmailHtml(): void + { + $worker = new CountingPersistAlertNotifications(); + + $payload = [ + 'project' => ['$id' => 'project-x'], + 'recipients' => [ + [ + 'address' => 'user-preview', + 'channel' => NOTIFICATION_TYPE_CONSOLE, + 'userId' => 'user-preview', + ], + ], + 'subject' => 'Webhook {{name}} paused', + 'preview' => 'Plain alert for {{name}}.', + 'body' => 'Email-only HTML', + 'templateParams' => ['name' => 'orders'], + 'deduplicationKey' => 'console-preview', + ]; + + $worker->action($this->buildMessage($payload), $this->project, $this->registry, $this->database, $this->log); + + $rows = $this->database->find('alerts'); + $this->assertCount(1, $rows); + $this->assertSame('Webhook orders paused', $rows[0]->getAttribute('title')); + $this->assertSame('Plain alert for orders.', $rows[0]->getAttribute('body')); + $this->assertStringNotContainsString('', $rows[0]->getAttribute('body')); + } + + public function testEmailChannelSkipsWhenSmtpIsNotConfigured(): void + { + $previousSmtpHost = \getenv('_APP_SMTP_HOST'); + \putenv('_APP_SMTP_HOST='); + + try { + $worker = new CountingPersistAlertNotifications(); + $payload = [ + 'project' => ['$id' => 'project-x'], + 'recipients' => [ + [ + 'address' => 'missing-smtp@example.test', + 'channel' => NOTIFICATION_TYPE_EMAIL, + 'userId' => 'user-smtp', + ], + ], + 'subject' => 'No SMTP', + 'body' => 'Body', + 'deduplicationKey' => 'missing-smtp', + ]; + + $worker->action($this->buildMessage($payload), $this->project, $this->registry, $this->database, $this->log); + } finally { + \putenv($previousSmtpHost === false ? '_APP_SMTP_HOST' : '_APP_SMTP_HOST=' . $previousSmtpHost); + } + + $this->assertSame(0, $worker->persistAlertCalls); + $this->assertCount(0, $this->database->find('alerts')); + $this->assertSame('no_smtp', $this->log->getTags()['email_skipped'] ?? null); + } + + public function testConsoleChannelRejectsInvalidImplicitUserId(): void + { + $worker = new CountingPersistAlertNotifications(); + + $payload = [ + 'project' => ['$id' => 'project-x'], + 'recipients' => [ + ['address' => 'not an appwrite user id', 'channel' => NOTIFICATION_TYPE_CONSOLE], + ], + 'subject' => 'Invalid', + 'body' => 'Body', + 'deduplicationKey' => 'invalid-console-user', + ]; + + $this->expectException(\Exception::class); + $this->expectExceptionMessage('Invalid console alert userId'); + + $worker->action($this->buildMessage($payload), $this->project, $this->registry, $this->database, $this->log); + } + /** * Worker happy-path: webhook channel. * diff --git a/tests/unit/Platform/Workers/WebhooksTest.php b/tests/unit/Platform/Workers/WebhooksTest.php new file mode 100644 index 0000000000..00fbd7878e --- /dev/null +++ b/tests/unit/Platform/Workers/WebhooksTest.php @@ -0,0 +1,36 @@ + 'membership-1', + 'roles' => $roles, + ]); + + $this->assertSame($expected, $method->invoke(null, $membership)); + } + + public static function ownerRoleProvider(): array + { + return [ + 'array owner' => [['owner'], true], + 'array mixed case owner' => [['Owner'], true], + 'comma string owner' => ['developer, owner', true], + 'non owner' => [['developer'], false], + 'invalid roles' => [null, false], + ]; + } +} diff --git a/tests/unit/Utopia/Messaging/Adapter/ConsoleTest.php b/tests/unit/Utopia/Messaging/Adapter/ConsoleTest.php index 5ed51d8db1..fe21f2d261 100644 --- a/tests/unit/Utopia/Messaging/Adapter/ConsoleTest.php +++ b/tests/unit/Utopia/Messaging/Adapter/ConsoleTest.php @@ -33,6 +33,7 @@ class ConsoleTest extends TestCase $this->database->create(); $this->database->createCollection('alerts', [], [], [Permission::create(Role::any()), Permission::read(Role::any())], false); $this->database->createAttribute('alerts', 'messageId', Database::VAR_STRING, 255, false); + $this->database->createAttribute('alerts', 'recipientHash', Database::VAR_STRING, 64, true); $this->database->createAttribute('alerts', 'type', Database::VAR_STRING, 64, false, 'info'); $this->database->createAttribute('alerts', 'channel', Database::VAR_STRING, 64, true); $this->database->createAttribute('alerts', 'userId', Database::VAR_STRING, 255, false); @@ -55,7 +56,7 @@ class ConsoleTest extends TestCase private function alertId(string $messageId, string $userId = '', string $teamId = ''): string { $key = $userId !== '' ? 'user:' . $userId : 'team:' . $teamId; - return $messageId . '_' . \substr(\md5($key), 0, 8); + return \substr($messageId, 0, 19) . '_' . \substr(\md5($key), 0, 16); } public function testWritesAlertWithCorrectSchema(): void @@ -155,9 +156,8 @@ class ConsoleTest extends TestCase $this->assertSame('a', $rowA->getAttribute('userId')); $this->assertSame('b', $rowB->getAttribute('userId')); - // $id values must be `messageId_<8-hex>` and the suffixes differ. - $this->assertMatchesRegularExpression('/^same-msg_[0-9a-f]{8}$/', $idA); - $this->assertMatchesRegularExpression('/^same-msg_[0-9a-f]{8}$/', $idB); + $this->assertMatchesRegularExpression('/^same-msg_[0-9a-f]{16}$/', $idA); + $this->assertMatchesRegularExpression('/^same-msg_[0-9a-f]{16}$/', $idB); } public function testRejectsForeignMessageType(): void @@ -166,7 +166,7 @@ class ConsoleTest extends TestCase $this->expectException(\Exception::class); $this->expectExceptionMessage('Invalid message type.'); - // ConsoleMessage extends nothing — pass an unrelated Message implementation + // ConsoleMessage extends nothing; pass an unrelated Message implementation $adapter->send(new \Appwrite\Utopia\Messaging\Messages\Webhook(urls: ['https://example.test'], payload: [])); } @@ -174,7 +174,7 @@ class ConsoleTest extends TestCase * Reviewer C4: a `DuplicateException` thrown by `createDocument` must be * treated as a SUCCESSFUL delivery, not a failure. The adapter previously * lumped Duplicate into the generic Throwable catch, which surfaced as a - * per-recipient `error` and caused the worker to throw — re-queueing the + * per-recipient `error` and caused the worker to throw, re-queueing the * notification and never marking the duplicate as delivered. */ public function testConsoleAdapterTreatsDuplicateAsDelivered(): void @@ -190,6 +190,7 @@ class ConsoleTest extends TestCase '$id' => $documentId, '$permissions' => [Permission::read(Role::any())], 'messageId' => $messageId, + 'recipientHash' => \substr(\md5('user:' . $userId), 0, 16), 'channel' => 'console', 'userId' => $userId, 'title' => 'pre-existing', @@ -212,7 +213,7 @@ class ConsoleTest extends TestCase $this->assertSame('success', $result['results'][0]['status'] ?? '', 'duplicate must report success status'); $this->assertSame('', $result['results'][0]['error'] ?? 'unset', 'duplicate must not surface a per-recipient error'); - // Still exactly ONE row — the pre-existing one. The adapter must not + // Still exactly ONE row: the pre-existing one. The adapter must not // overwrite it nor create a sibling. $rows = $this->database->find('alerts'); $this->assertCount(1, $rows);