diff --git a/lib/Db/TalkBot.php b/lib/Db/TalkBot.php new file mode 100644 index 00000000..20bdf8d3 --- /dev/null +++ b/lib/Db/TalkBot.php @@ -0,0 +1,50 @@ +addType('appid', 'string'); + $this->addType('route', 'string'); + $this->addType('secret', 'string'); + $this->addType('createdTime', 'integer'); + } + + public function jsonSerialize(): array { + return [ + 'id' => $this->getId(), + 'appid' => $this->getAppid(), + 'route' => $this->getRoute(), + 'created_time' => $this->getCreatedTime(), + ]; + } +} diff --git a/lib/Db/TalkBotMapper.php b/lib/Db/TalkBotMapper.php new file mode 100644 index 00000000..7960cdd3 --- /dev/null +++ b/lib/Db/TalkBotMapper.php @@ -0,0 +1,55 @@ + + */ +class TalkBotMapper extends QBMapper { + public function __construct(IDBConnection $db) { + parent::__construct($db, 'ex_apps_talk_bots'); + } + + /** + * @throws DoesNotExistException if not found + * @throws MultipleObjectsReturnedException if more than one row matched (shouldn't, UNIQUE index) + * @throws Exception + */ + public function findByAppidAndRoute(string $appId, string $route): TalkBot { + $qb = $this->db->getQueryBuilder(); + $qb->select('*') + ->from($this->tableName) + ->where( + $qb->expr()->eq('appid', $qb->createNamedParameter($appId, IQueryBuilder::PARAM_STR)), + $qb->expr()->eq('route', $qb->createNamedParameter($route, IQueryBuilder::PARAM_STR)), + ); + return $this->findEntity($qb); + } + + /** + * @throws Exception + * + * @return TalkBot[] + */ + public function findAllByAppid(string $appId): array { + $qb = $this->db->getQueryBuilder(); + $qb->select('*') + ->from($this->tableName) + ->where($qb->expr()->eq('appid', $qb->createNamedParameter($appId, IQueryBuilder::PARAM_STR))); + return $this->findEntities($qb); + } +} diff --git a/lib/Migration/Version034000Date20260428144801.php b/lib/Migration/Version034000Date20260428144801.php new file mode 100644 index 00000000..bfae7356 --- /dev/null +++ b/lib/Migration/Version034000Date20260428144801.php @@ -0,0 +1,230 @@ +hasTable('ex_apps_talk_bots')) { + $table = $schema->createTable('ex_apps_talk_bots'); + + $table->addColumn('id', Types::BIGINT, [ + 'notnull' => true, + 'autoincrement' => true, + ]); + $table->addColumn('appid', Types::STRING, [ + 'notnull' => true, + 'length' => 32, + ]); + $table->addColumn('route', Types::STRING, [ + 'notnull' => true, + 'length' => 128, + ]); + // ICrypto envelope output is variable-length; TEXT keeps headroom across DB engines. + $table->addColumn('secret', Types::TEXT, [ + 'notnull' => true, + ]); + // BIGINT for consistency with oc_ex_apps.created_time and to avoid the year-2038 truncation + // that affects INT-typed Unix timestamps. + $table->addColumn('created_time', Types::BIGINT, [ + 'notnull' => true, + 'default' => 0, + ]); + + $table->setPrimaryKey(['id']); + $table->addUniqueIndex(['appid', 'route'], 'ex_apps_talk_bots__app_route'); + $table->addIndex(['appid'], 'ex_apps_talk_bots__appid'); + } + + return $schema; + } + + public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper { + /** @var ISchemaWrapper $schema */ + $schema = $schemaClosure(); + if (!$schema->hasTable('appconfig_ex') || !$schema->hasTable('ex_apps_talk_bots')) { + return null; + } + + // Materialize the cursor up front. Iterating a forward-only cursor while issuing DML + // against the same table on the same connection is undefined across drivers, and we + // also want each per-bot insert+deletes to commit (or roll back) as a single unit. + $rows = $this->fetchRouteIndexRows(); + + $migrated = 0; + $skipped = 0; + + foreach ($rows as $row) { + $appId = (string)$row['appid']; + $route = (string)$row['configvalue']; + $expectedHash = sha1($appId . '_' . $route); + $keySuffix = substr((string)$row['configkey'], strlen(self::TALK_BOT_ROUTE_PREFIX)); + + if ($keySuffix !== $expectedHash) { + $output->warning(sprintf( + 'TalkBot migration: malformed talk_bot_route row id=%d (appid=%s) — key suffix does not match sha1(appid_route), skipping', + (int)$row['id'], $appId, + )); + $skipped++; + continue; + } + + $secretRow = $this->fetchSecretRow($appId, $expectedHash); + if ($secretRow === null) { + $output->warning(sprintf( + 'TalkBot migration: orphan talk_bot_route_%s for app %s (no matching secret row), skipping', + $expectedHash, $appId, + )); + $skipped++; + continue; + } + + // Pre-migration TalkBotsService stored bot secrets via ExAppConfigService::setAppConfigValue() + // WITHOUT passing $sensitive=true, so legacy rows are plaintext (sensitive=0). Honor the column + // instead of unconditionally decrypting — Version032002Date20250527174907 retroactively encrypted + // any sensitive=1 rows already, so if someone manually marked a bot secret sensitive after the fact + // we still handle it correctly. + $rawSecret = (string)$secretRow['configvalue']; + if ((int)($secretRow['sensitive'] ?? 0) === 1 && $rawSecret !== '') { + try { + $plaintextSecret = $this->crypto->decrypt($rawSecret); + } catch (Throwable $e) { + $output->warning(sprintf( + 'TalkBot migration: failed to decrypt sensitive secret for app %s route %s: %s — skipping', + $appId, $route, $e->getMessage(), + )); + $skipped++; + continue; + } + } else { + $plaintextSecret = $rawSecret; + } + + $this->connection->beginTransaction(); + try { + if (!$this->talkBotExists($appId, $route)) { + $this->insertTalkBot($appId, $route, $this->crypto->encrypt($plaintextSecret)); + } + $this->deleteAppconfigRow((int)$row['id']); + $this->deleteAppconfigRow((int)$secretRow['id']); + $this->connection->commit(); + $migrated++; + } catch (Throwable $e) { + try { + $this->connection->rollBack(); + } catch (Throwable) { + // rollBack failures on an already-aborted transaction are not actionable here. + } + $output->warning(sprintf( + 'TalkBot migration: per-bot transaction failed for app %s route %s: %s — old rows preserved, retry by re-running the migration', + $appId, $route, $e->getMessage(), + )); + $skipped++; + } + } + + $output->info(sprintf('TalkBot migration: %d migrated, %d skipped', $migrated, $skipped)); + return null; + } + + /** + * @return array + */ + private function fetchRouteIndexRows(): array { + $qb = $this->connection->getQueryBuilder(); + $qb->select('id', 'appid', 'configkey', 'configvalue') + ->from('appconfig_ex') + ->where($qb->expr()->like('configkey', $qb->createNamedParameter(self::TALK_BOT_ROUTE_PREFIX . '%'))); + $cursor = $qb->executeQuery(); + $rows = $cursor->fetchAll(); + $cursor->closeCursor(); + return $rows; + } + + private function fetchSecretRow(string $appId, string $hash): ?array { + $qb = $this->connection->getQueryBuilder(); + $qb->select('id', 'configvalue', 'sensitive') + ->from('appconfig_ex') + ->where( + $qb->expr()->eq('appid', $qb->createNamedParameter($appId)), + $qb->expr()->eq('configkey', $qb->createNamedParameter($hash)), + ) + ->setMaxResults(1); + $res = $qb->executeQuery(); + $row = $res->fetch(); + $res->closeCursor(); + return $row === false ? null : $row; + } + + private function talkBotExists(string $appId, string $route): bool { + $qb = $this->connection->getQueryBuilder(); + $qb->select('id') + ->from('ex_apps_talk_bots') + ->where( + $qb->expr()->eq('appid', $qb->createNamedParameter($appId)), + $qb->expr()->eq('route', $qb->createNamedParameter($route)), + ) + ->setMaxResults(1); + $res = $qb->executeQuery(); + $exists = $res->fetch() !== false; + $res->closeCursor(); + return $exists; + } + + private function insertTalkBot(string $appId, string $route, string $encryptedSecret): void { + $qb = $this->connection->getQueryBuilder(); + $qb->insert('ex_apps_talk_bots') + ->values([ + 'appid' => $qb->createNamedParameter($appId), + 'route' => $qb->createNamedParameter($route), + 'secret' => $qb->createNamedParameter($encryptedSecret), + 'created_time' => $qb->createNamedParameter(time(), Types::BIGINT), + ]); + $qb->executeStatement(); + } + + private function deleteAppconfigRow(int $id): void { + $qb = $this->connection->getQueryBuilder(); + $qb->delete('appconfig_ex') + ->where($qb->expr()->eq('id', $qb->createNamedParameter($id, Types::INTEGER))); + $qb->executeStatement(); + } +} diff --git a/lib/Service/TalkBotsService.php b/lib/Service/TalkBotsService.php index 627f7ed6..0cb2208b 100644 --- a/lib/Service/TalkBotsService.php +++ b/lib/Service/TalkBotsService.php @@ -10,42 +10,84 @@ namespace OCA\AppAPI\Service; use OCA\AppAPI\Db\ExApp; +use OCA\AppAPI\Db\TalkBot; +use OCA\AppAPI\Db\TalkBotMapper; use OCA\Talk\Events\BotInstallEvent; use OCA\Talk\Events\BotUninstallEvent; +use OCP\AppFramework\Db\DoesNotExistException; use OCP\EventDispatcher\IEventDispatcher; use OCP\IURLGenerator; +use OCP\Security\ICrypto; use OCP\Security\ISecureRandom; +use Psr\Log\LoggerInterface; +use Throwable; readonly class TalkBotsService { public function __construct( - private ExAppConfigService $exAppConfigService, + private TalkBotMapper $mapper, private IEventDispatcher $dispatcher, private ISecureRandom $random, private IURLGenerator $urlGenerator, + private ICrypto $crypto, + private LoggerInterface $logger, ) { } public function registerExAppBot(ExApp $exApp, string $name, string $route, string $description): ?array { - if (!class_exists(BotInstallEvent::class)) { + // Require both events up front — the compensation path on mapper-insert failure dispatches + // BotUninstallEvent, so we must not enter the function unless both Talk classes are loadable. + if (!class_exists(BotInstallEvent::class) || !class_exists(BotUninstallEvent::class)) { return null; } - [$id, $url, $secret] = $this->getExAppTalkBotConfig($exApp, $route); - - $event = new BotInstallEvent( - $name, - $secret, - $url, - $description, - ); - $this->dispatcher->dispatchTyped($event); + $appId = $exApp->getAppid(); + $url = $this->buildProxyUrl($appId, $route); + + $bot = $this->findBot($appId, $route); + if ($bot !== null) { + $secret = $this->decryptSecret($bot); + if ($secret === null) { + // Auto-recovery would mint a new secret against the same URL, but Talk's BotListener + // rejects an install event whose URL already exists with a different secret. Surface + // the failure so an operator can clear the bad row (and Talk's matching one) by hand. + return null; + } + } else { + $secret = $this->random->generate(64, ISecureRandom::CHAR_HUMAN_READABLE); + } - $this->exAppConfigService->setAppConfigValue($exApp->getAppid(), $id, $secret); - $this->exAppConfigService->setAppConfigValue($exApp->getAppid(), 'talk_bot_route_' . $id, $route); + $this->dispatcher->dispatchTyped(new BotInstallEvent($name, $secret, $url, $description)); + + // Re-register is a no-op on our side: name+description live in spreed's talk_bots_server and + // Talk's listener reconciles its row from the BotInstallEvent above. We only insert when + // minting a fresh bot — the secret and the (appid, route) pair are all we own. + if ($bot === null) { + $bot = new TalkBot(); + $bot->setAppid($appId); + $bot->setRoute($route); + $bot->setSecret($this->crypto->encrypt($secret)); + $bot->setCreatedTime(time()); + + try { + $this->mapper->insert($bot); + } catch (Throwable $e) { + // Talk already persisted the bot via the install event above; without compensation + // it would dangle as a row pointing at our proxy URL with a secret we never stored. + try { + $this->dispatcher->dispatchTyped(new BotUninstallEvent($secret, $url)); + } catch (Throwable $compensationError) { + $this->logger->error(sprintf( + 'TalkBot register: failed to compensate Talk install for app=%s route=%s after mapper insert error: %s', + $appId, $route, $compensationError->getMessage(), + )); + } + throw $e; + } + } return [ - 'id' => $id, + 'id' => self::getExAppTalkBotHash($appId, $route), 'secret' => $secret, ]; } @@ -55,53 +97,79 @@ public function unregisterExAppBot(ExApp $exApp, string $route): ?bool { return null; } - [$id, $url, $secret] = $this->getExAppTalkBotConfig($exApp, $route); - - $event = new BotUninstallEvent($secret, $url); - $this->dispatcher->dispatchTyped($event); - - if ($this->exAppConfigService->deleteAppConfigValues([$id], $exApp->getAppid()) !== 1) { + $bot = $this->findBot($exApp->getAppid(), $route); + if ($bot === null) { return null; } + + $secret = $this->decryptSecret($bot); + if ($secret !== null) { + $url = $this->buildProxyUrl($exApp->getAppid(), $route); + $this->dispatcher->dispatchTyped(new BotUninstallEvent($secret, $url)); + } + + $this->mapper->delete($bot); return true; } - private function getExAppTalkBotConfig(ExApp $exApp, string $route): array { - $url = $this->urlGenerator->linkToOCSRouteAbsolute( - 'app_api.TalkBot.proxyTalkMessage', ['appId' => $exApp->getAppid(), 'route' => $route] - ); - $id = $this->getExAppTalkBotHash($exApp->getAppid(), $route); - - $exAppConfig = $this->exAppConfigService->getAppConfig($exApp->getAppid(), $id); - if ($exAppConfig === null) { - $secret = $this->random->generate(64, ISecureRandom::CHAR_HUMAN_READABLE); - } else { - $secret = $exAppConfig->getConfigvalue(); // Do not regenerate already registered bot secret + public function unregisterExAppTalkBots(ExApp $exApp): void { + try { + $bots = $this->mapper->findAllByAppid($exApp->getAppid()); + } catch (Throwable $e) { + $this->logger->error(sprintf('Failed to enumerate TalkBots for ExApp %s: %s', $exApp->getAppid(), $e->getMessage()), ['exception' => $e]); + return; + } + // Isolate per-bot failures so a single deadlock / DB blip / dispatcher throw doesn't leave + // remaining bots stranded in our table (with Talk's matching row still live) once the + // owning ExApp row is gone. + foreach ($bots as $bot) { + try { + $this->unregisterExAppBot($exApp, $bot->getRoute()); + } catch (Throwable $e) { + $this->logger->error(sprintf( + 'Failed to unregister TalkBot for ExApp %s route %s: %s', + $exApp->getAppid(), $bot->getRoute(), $e->getMessage(), + ), ['exception' => $e]); + } } - return [$id, $url, $secret]; } public function getTalkBotSecret(string $appId, string $route): ?string { - $exAppConfig = $this->exAppConfigService->getAppConfig($appId, $this->getExAppTalkBotHash($appId, $route)); - return $exAppConfig?->getConfigvalue(); + $bot = $this->findBot($appId, $route); + return $bot === null ? null : $this->decryptSecret($bot); } - public function unregisterExAppTalkBots(ExApp $exApp): void { - $exAppConfigs = $this->exAppConfigService->getAllAppConfig($exApp->getAppid()); - foreach ($exAppConfigs as $exAppConfig) { - if (str_starts_with($exAppConfig->getConfigkey(), 'talk_bot_route_')) { - $route = $exAppConfig->getConfigvalue(); - $id = $this->getExAppTalkBotHash($exApp->getAppid(), $route); - $configHash = substr($exAppConfig->getConfigkey(), 15); - if ($id === $configHash) { - $this->unregisterExAppBot($exApp, $route); - $this->exAppConfigService->deleteAppConfig($exAppConfig); - } - } + /** + * Stable opaque ID exposed in the OCS register response. Kept on the sha1 + * scheme that nc_py_api and existing ExApps already persist as `bot_id`. + */ + private static function getExAppTalkBotHash(string $appId, string $route): string { + return sha1($appId . '_' . $route); + } + + private function findBot(string $appId, string $route): ?TalkBot { + try { + return $this->mapper->findByAppidAndRoute($appId, $route); + } catch (DoesNotExistException) { + return null; } } - private function getExAppTalkBotHash(string $appId, string $route): string { - return sha1($appId . '_' . $route); + private function decryptSecret(TalkBot $bot): ?string { + try { + return $this->crypto->decrypt($bot->getSecret()); + } catch (Throwable $e) { + $this->logger->error(sprintf( + 'Failed to decrypt TalkBot secret for app=%s route=%s: %s', + $bot->getAppid(), $bot->getRoute(), $e->getMessage(), + )); + return null; + } + } + + private function buildProxyUrl(string $appId, string $route): string { + return $this->urlGenerator->linkToOCSRouteAbsolute( + 'app_api.TalkBot.proxyTalkMessage', ['appId' => $appId, 'route' => $route], + ); } } diff --git a/psalm.xml b/psalm.xml index 7f6c1930..34aef173 100644 --- a/psalm.xml +++ b/psalm.xml @@ -59,5 +59,6 @@ + diff --git a/tests/php/Controller/TalkBotControllerTest.php b/tests/php/Controller/TalkBotControllerTest.php index 7b119e77..a5c1c670 100644 --- a/tests/php/Controller/TalkBotControllerTest.php +++ b/tests/php/Controller/TalkBotControllerTest.php @@ -10,10 +10,14 @@ namespace OCA\AppAPI\Tests\php\Controller; use OCA\AppAPI\Controller\TalkBotController; +use OCA\AppAPI\Db\TalkBot; +use OCA\AppAPI\Db\TalkBotMapper; use OCA\AppAPI\Service\AppAPIService; use OCA\AppAPI\Service\ExAppService; use OCA\AppAPI\Service\TalkBotsService; +use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Http; +use OCP\AppFramework\OCS\OCSBadRequestException; use OCP\AppFramework\OCS\OCSNotFoundException; use OCP\IRequest; use OCP\Security\Bruteforce\IThrottler; @@ -130,12 +134,52 @@ public function testUnregisterMissingThrowsNotFound(): void { } public function testRegisterTwiceReusesSecret(): void { - // TalkBotsService caches the secret keyed by the (appid, route) hash and reuses it on re-register - // (see getExAppTalkBotConfig: "Do not regenerate already registered bot secret"). Both register calls - // must succeed and return the SAME id/secret pair. + // TalkBotsService keeps one row per (appid, route) and reuses its stored secret on re-register + // so existing Talk-side bots stay valid. Both register calls must return the SAME id/secret. $first = $this->controller->registerExAppTalkBot('PHPUnit Bot', self::ROUTE, 'desc')->getData(); $second = $this->controller->registerExAppTalkBot('PHPUnit Bot', self::ROUTE, 'desc')->getData(); self::assertSame($first['id'], $second['id']); self::assertSame($first['secret'], $second['secret']); } + + public function testRegisterReturnsBadRequestWhenStoredSecretIsCorrupted(): void { + // Insert a TalkBot row directly with bogus secret data that ICrypto::decrypt cannot parse. + // The service must refuse to auto-recover (re-minting against the same URL would leave Talk + // wedged with a "different secret" rejection) and the controller surfaces a 400 to the caller. + $mapper = Server::get(TalkBotMapper::class); + $bot = new TalkBot(); + $bot->setAppid(self::TEST_APP_ID); + $bot->setRoute(ltrim(self::ROUTE, '/')); + $bot->setSecret('NOT_A_VALID_CRYPTO_PAYLOAD'); + $bot->setCreatedTime(time()); + $mapper->insert($bot); + + try { + $this->expectException(OCSBadRequestException::class); + $this->controller->registerExAppTalkBot('PHPUnit Bot', self::ROUTE, 'desc'); + } finally { + // Clean up the manually-inserted corrupted row so tearDown's safeUnregister works. + try { + $mapper->delete($mapper->findByAppidAndRoute(self::TEST_APP_ID, ltrim(self::ROUTE, '/'))); + } catch (DoesNotExistException) { + } + } + } + + public function testFanOutUnregisterClearsAllAppBots(): void { + // unregisterExAppTalkBots is invoked by ExAppService::unregisterExApp. It must enumerate via + // TalkBotMapper::findAllByAppid and dispatch a BotUninstallEvent per bot. + $exApp = $this->exAppService->getExApp(self::TEST_APP_ID); + self::assertNotNull($exApp); + + $this->talkBotsService->registerExAppBot($exApp, 'PHPUnit Bot 1', 'fanout_route_one', 'desc'); + $this->talkBotsService->registerExAppBot($exApp, 'PHPUnit Bot 2', 'fanout_route_two', 'desc'); + self::assertNotNull($this->talkBotsService->getTalkBotSecret(self::TEST_APP_ID, 'fanout_route_one')); + self::assertNotNull($this->talkBotsService->getTalkBotSecret(self::TEST_APP_ID, 'fanout_route_two')); + + $this->talkBotsService->unregisterExAppTalkBots($exApp); + + self::assertNull($this->talkBotsService->getTalkBotSecret(self::TEST_APP_ID, 'fanout_route_one')); + self::assertNull($this->talkBotsService->getTalkBotSecret(self::TEST_APP_ID, 'fanout_route_two')); + } } diff --git a/tests/php/Migration/Version034000Date20260428144801Test.php b/tests/php/Migration/Version034000Date20260428144801Test.php new file mode 100644 index 00000000..8661b8fc --- /dev/null +++ b/tests/php/Migration/Version034000Date20260428144801Test.php @@ -0,0 +1,229 @@ +db = Server::get(IDBConnection::class); + $this->crypto = Server::get(ICrypto::class); + $this->migration = new Version034000Date20260428144801( + $this->db, + $this->crypto, + ); + $this->cleanupAll(); + } + + protected function tearDown(): void { + $this->cleanupAll(); + parent::tearDown(); + } + + public function testPlaintextSecretMigratesAndRoundTrips(): void { + // Legacy reality: TalkBotsService::setAppConfigValue() was called without $sensitive=true, + // so bot secrets sit in appconfig_ex as plaintext. The migration must accept this and + // re-encrypt on insert into the new table. + $plaintext = str_repeat('A', 64); + $this->seedLegacyBot(self::TEST_APP_ONE, 'plain_route', $plaintext, sensitive: 0); + + $this->migration->postSchemaChange(new \OC\Migration\NullOutput(), fn () => $this->schema(), []); + + $row = $this->fetchExAppsTalkBotsRow(self::TEST_APP_ONE, 'plain_route'); + self::assertNotNull($row, 'bot row must exist post-migration'); + self::assertSame($plaintext, $this->crypto->decrypt($row['secret'])); + self::assertSame(0, $this->countLegacyRowsFor(self::TEST_APP_ONE), 'legacy rows must be purged'); + } + + public function testSensitiveEncryptedSecretDecryptsThenReEncrypts(): void { + // Hypothetical scenario: an operator manually flipped sensitive=1 and let Version032002 + // re-encrypt the row in place. The migration's `if (sensitive == 1)` branch must decrypt + // with the same key and re-encrypt for the new table. + $plaintext = str_repeat('B', 64); + $encryptedLegacy = $this->crypto->encrypt($plaintext); + $this->seedLegacyBot(self::TEST_APP_ONE, 'sensitive_route', $encryptedLegacy, sensitive: 1); + + $this->migration->postSchemaChange(new \OC\Migration\NullOutput(), fn () => $this->schema(), []); + + $row = $this->fetchExAppsTalkBotsRow(self::TEST_APP_ONE, 'sensitive_route'); + self::assertNotNull($row); + self::assertSame($plaintext, $this->crypto->decrypt($row['secret'])); + } + + public function testOrphanRouteRowSkippedAndPreserved(): void { + // A route row without its matching secret row: defensive skip, no insert, no delete — + // re-running the migration after the operator fixes the data must still succeed. + $route = 'orphan_route'; + $hash = sha1(self::TEST_APP_ONE . '_' . $route); + $this->insertAppConfigEx(self::TEST_APP_ONE, self::TALK_BOT_ROUTE_PREFIX . $hash, $route, 0); + + $this->migration->postSchemaChange(new \OC\Migration\NullOutput(), fn () => $this->schema(), []); + + self::assertNull($this->fetchExAppsTalkBotsRow(self::TEST_APP_ONE, $route)); + self::assertSame(1, $this->countLegacyRowsFor(self::TEST_APP_ONE), 'orphan route must NOT be deleted'); + } + + public function testMalformedKeySuffixSkipped(): void { + // A talk_bot_route_* row whose suffix does not match sha1(appid_route) is either corrupt + // or planted by an external writer. Don't trust the configvalue; skip and preserve. + $route = 'mismatched_route'; + $this->insertAppConfigEx(self::TEST_APP_ONE, self::TALK_BOT_ROUTE_PREFIX . str_repeat('0', 40), $route, 0); + // Add a (different-key) secret row so we can prove the migration didn't consume it either. + $this->insertAppConfigEx(self::TEST_APP_ONE, str_repeat('0', 40), str_repeat('X', 64), 0); + + $this->migration->postSchemaChange(new \OC\Migration\NullOutput(), fn () => $this->schema(), []); + + self::assertNull($this->fetchExAppsTalkBotsRow(self::TEST_APP_ONE, $route)); + self::assertSame(2, $this->countLegacyRowsFor(self::TEST_APP_ONE), 'malformed pair must be preserved as-is'); + } + + public function testMultiBotMultiApp(): void { + // Three bots across two ExApps in a single migration pass. The unique (appid, route) + // constraint means each pair lands in its own row, even when routes collide across apps. + $this->seedLegacyBot(self::TEST_APP_ONE, 'shared_route', str_repeat('A', 64), 0); + $this->seedLegacyBot(self::TEST_APP_ONE, 'second_route', str_repeat('B', 64), 0); + $this->seedLegacyBot(self::TEST_APP_TWO, 'shared_route', str_repeat('C', 64), 0); + + $this->migration->postSchemaChange(new \OC\Migration\NullOutput(), fn () => $this->schema(), []); + + self::assertNotNull($this->fetchExAppsTalkBotsRow(self::TEST_APP_ONE, 'shared_route')); + self::assertNotNull($this->fetchExAppsTalkBotsRow(self::TEST_APP_ONE, 'second_route')); + self::assertNotNull($this->fetchExAppsTalkBotsRow(self::TEST_APP_TWO, 'shared_route')); + self::assertSame(0, $this->countLegacyRowsFor(self::TEST_APP_ONE)); + self::assertSame(0, $this->countLegacyRowsFor(self::TEST_APP_TWO)); + } + + public function testIdempotentRerun(): void { + // `occ migrations:execute` is the operator's escape hatch when a previous run was + // interrupted. Running the migration twice on the same data must not duplicate rows + // or fail; the second pass should see 0 candidate route rows. + $this->seedLegacyBot(self::TEST_APP_ONE, 'idempotent_route', str_repeat('D', 64), 0); + $this->migration->postSchemaChange(new \OC\Migration\NullOutput(), fn () => $this->schema(), []); + $first = $this->fetchExAppsTalkBotsRow(self::TEST_APP_ONE, 'idempotent_route'); + + $this->migration->postSchemaChange(new \OC\Migration\NullOutput(), fn () => $this->schema(), []); + $second = $this->fetchExAppsTalkBotsRow(self::TEST_APP_ONE, 'idempotent_route'); + + self::assertNotNull($first); + self::assertNotNull($second); + self::assertSame((int)$first['id'], (int)$second['id'], 'idempotent re-run must NOT mint a new row'); + } + + public function testRerunAfterPreviousMigratedRow(): void { + // Simulates: operator re-runs after fixing the data — the previously-migrated bot is + // still in ex_apps_talk_bots, AND a fresh legacy pair shows up for the SAME route. + // The migration must skip the insert (UNIQUE constraint would fire), still clean the + // stale legacy pair, and not corrupt the existing row. + $this->seedLegacyBot(self::TEST_APP_ONE, 'preexist_route', str_repeat('E', 64), 0); + $this->migration->postSchemaChange(new \OC\Migration\NullOutput(), fn () => $this->schema(), []); + $beforeId = (int)$this->fetchExAppsTalkBotsRow(self::TEST_APP_ONE, 'preexist_route')['id']; + + // Re-seed the legacy pair (e.g. someone restored from a partial backup). + $this->seedLegacyBot(self::TEST_APP_ONE, 'preexist_route', str_repeat('F', 64), 0); + $this->migration->postSchemaChange(new \OC\Migration\NullOutput(), fn () => $this->schema(), []); + + $row = $this->fetchExAppsTalkBotsRow(self::TEST_APP_ONE, 'preexist_route'); + self::assertSame($beforeId, (int)$row['id'], 'pre-existing row must be untouched'); + self::assertSame(0, $this->countLegacyRowsFor(self::TEST_APP_ONE), 'stale legacy pair must still be cleaned'); + } + + private function seedLegacyBot(string $appId, string $route, string $secretValue, int $sensitive): void { + $hash = sha1($appId . '_' . $route); + $this->insertAppConfigEx($appId, $hash, $secretValue, $sensitive); + $this->insertAppConfigEx($appId, self::TALK_BOT_ROUTE_PREFIX . $hash, $route, 0); + } + + private function insertAppConfigEx(string $appId, string $configKey, string $configValue, int $sensitive): void { + $qb = $this->db->getQueryBuilder(); + $qb->insert('appconfig_ex') + ->values([ + 'appid' => $qb->createNamedParameter($appId), + 'configkey' => $qb->createNamedParameter($configKey), + 'configvalue' => $qb->createNamedParameter($configValue), + 'sensitive' => $qb->createNamedParameter($sensitive, \OCP\DB\QueryBuilder\IQueryBuilder::PARAM_INT), + ]); + $qb->executeStatement(); + } + + private function fetchExAppsTalkBotsRow(string $appId, string $route): ?array { + $qb = $this->db->getQueryBuilder(); + $qb->select('*') + ->from('ex_apps_talk_bots') + ->where( + $qb->expr()->eq('appid', $qb->createNamedParameter($appId)), + $qb->expr()->eq('route', $qb->createNamedParameter($route)), + ) + ->setMaxResults(1); + $res = $qb->executeQuery(); + $row = $res->fetch(); + $res->closeCursor(); + return $row === false ? null : $row; + } + + private function countLegacyRowsFor(string $appId): int { + $qb = $this->db->getQueryBuilder(); + $qb->select($qb->func()->count('*', 'cnt')) + ->from('appconfig_ex') + ->where($qb->expr()->eq('appid', $qb->createNamedParameter($appId))); + $res = $qb->executeQuery(); + $row = $res->fetch(); + $res->closeCursor(); + return (int)$row['cnt']; + } + + private function cleanupAll(): void { + foreach ([self::TEST_APP_ONE, self::TEST_APP_TWO] as $appId) { + $qb = $this->db->getQueryBuilder(); + $qb->delete('appconfig_ex') + ->where($qb->expr()->eq('appid', $qb->createNamedParameter($appId))); + $qb->executeStatement(); + + $qb = $this->db->getQueryBuilder(); + $qb->delete('ex_apps_talk_bots') + ->where($qb->expr()->eq('appid', $qb->createNamedParameter($appId))); + $qb->executeStatement(); + } + } + + private function schema(): ISchemaWrapper { + // Server::get(IDBConnection::class) returns the public ConnectionAdapter, but SchemaWrapper + // needs the internal OC\DB\Connection it wraps. Reflect once at test setup time — the + // `inner` field on ConnectionAdapter is declared private but stable across NC versions. + $inner = (new \ReflectionProperty(\OC\DB\ConnectionAdapter::class, 'inner'))->getValue($this->db); + return new \OC\DB\SchemaWrapper($inner); + } +} diff --git a/tests/stubs/oca_talk_events.php b/tests/stubs/oca_talk_events.php new file mode 100644 index 00000000..518252d3 --- /dev/null +++ b/tests/stubs/oca_talk_events.php @@ -0,0 +1,36 @@ +