From 9b8a982a0a1a0c76be9f59a0be83810f02612d1d Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 27 Jan 2026 23:02:27 +0100 Subject: [PATCH] fix(service): Use validation service to deduplicate code Signed-off-by: Joas Schilling --- lib/Controller/RecordingController.php | 15 ++++--- lib/Controller/SignalingController.php | 35 ++++++---------- lib/Service/ChecksumVerificationService.php | 2 +- .../Controller/SignalingControllerTest.php | 41 ++++++++++++------- .../ChecksumVerificationServiceTest.php | 2 +- 5 files changed, 49 insertions(+), 46 deletions(-) diff --git a/lib/Controller/RecordingController.php b/lib/Controller/RecordingController.php index f0d2313e6ff..dee7db5dd81 100644 --- a/lib/Controller/RecordingController.php +++ b/lib/Controller/RecordingController.php @@ -13,6 +13,7 @@ use OCA\Talk\Config; use OCA\Talk\Exceptions\ParticipantNotFoundException; use OCA\Talk\Exceptions\RoomNotFoundException; +use OCA\Talk\Exceptions\UnauthorizedException; use OCA\Talk\Manager; use OCA\Talk\Middleware\Attribute\RequireLoggedInModeratorParticipant; use OCA\Talk\Middleware\Attribute\RequireModeratorParticipant; @@ -23,6 +24,7 @@ use OCA\Talk\Recording\RecordingStoppedRequest; use OCA\Talk\Room; use OCA\Talk\Service\CertificateService; +use OCA\Talk\Service\ChecksumVerificationService; use OCA\Talk\Service\ParticipantService; use OCA\Talk\Service\RecordingService; use OCA\Talk\Service\RoomService; @@ -55,6 +57,7 @@ public function __construct( private RecordingService $recordingService, private RoomService $roomService, private ITimeFactory $timeFactory, + private ChecksumVerificationService $checksumVerificationService, private LoggerInterface $logger, ) { parent::__construct($appName, $request); @@ -150,17 +153,13 @@ public function getWelcomeMessage(int $serverId): DataResponse { */ private function validateBackendRequest(string $data): bool { $random = $this->request->getHeader('talk-recording-random'); - if (empty($random) || strlen($random) < 32) { - $this->logger->debug('Missing random'); - return false; - } $checksum = $this->request->getHeader('talk-recording-checksum'); - if (empty($checksum)) { - $this->logger->debug('Missing checksum'); + $secret = $this->talkConfig->getRecordingSecret(); + try { + return $this->checksumVerificationService->validateRequest($random, $checksum, $secret, $data); + } catch (UnauthorizedException) { return false; } - $hash = hash_hmac('sha256', $random . $data, $this->talkConfig->getRecordingSecret()); - return hash_equals($hash, strtolower($checksum)); } /** diff --git a/lib/Controller/SignalingController.php b/lib/Controller/SignalingController.php index a18912c41e3..adfa052bafc 100644 --- a/lib/Controller/SignalingController.php +++ b/lib/Controller/SignalingController.php @@ -13,6 +13,7 @@ use OCA\Talk\Exceptions\ForbiddenException; use OCA\Talk\Exceptions\ParticipantNotFoundException; use OCA\Talk\Exceptions\RoomNotFoundException; +use OCA\Talk\Exceptions\UnauthorizedException; use OCA\Talk\Federation\Authenticator; use OCA\Talk\Manager; use OCA\Talk\Model\Attendee; @@ -21,6 +22,7 @@ use OCA\Talk\ResponseDefinitions; use OCA\Talk\Room; use OCA\Talk\Service\BanService; +use OCA\Talk\Service\ChecksumVerificationService; use OCA\Talk\Service\ParticipantService; use OCA\Talk\Service\RoomService; use OCA\Talk\Service\SessionService; @@ -37,7 +39,6 @@ use OCP\AppFramework\Utility\ITimeFactory; use OCP\DB\Exception; use OCP\EventDispatcher\IEventDispatcher; -use OCP\Http\Client\IClientService; use OCP\IDBConnection; use OCP\IRequest; use OCP\IUser; @@ -68,7 +69,7 @@ public function __construct( private IUserManager $userManager, private IEventDispatcher $dispatcher, private ITimeFactory $timeFactory, - private IClientService $clientService, + private ChecksumVerificationService $checksumVerificationService, private BanService $banService, private LoggerInterface $logger, protected Authenticator $federationAuthenticator, @@ -91,17 +92,13 @@ public function __construct( */ private function validateRecordingBackendRequest(string $data): bool { $random = $this->request->getHeader('talk-recording-random'); - if (empty($random) || strlen($random) < 32) { - $this->logger->debug('Missing random'); - return false; - } $checksum = $this->request->getHeader('talk-recording-checksum'); - if (empty($checksum)) { - $this->logger->debug('Missing checksum'); + $secret = $this->talkConfig->getRecordingSecret(); + try { + return $this->checksumVerificationService->validateRequest($random, $checksum, $secret, $data); + } catch (UnauthorizedException) { return false; } - $hash = hash_hmac('sha256', $random . $data, $this->talkConfig->getRecordingSecret()); - return hash_equals($hash, strtolower($checksum)); } /** @@ -685,20 +682,14 @@ protected function banIpIfGuestGotBanned(string $token): void { * @return bool */ private function validateBackendRequest(string $data): bool { - if (!isset($_SERVER['HTTP_SPREED_SIGNALING_RANDOM'], - $_SERVER['HTTP_SPREED_SIGNALING_CHECKSUM'])) { - return false; - } - $random = $_SERVER['HTTP_SPREED_SIGNALING_RANDOM']; - if (empty($random) || strlen($random) < 32) { - return false; - } - $checksum = $_SERVER['HTTP_SPREED_SIGNALING_CHECKSUM']; - if (empty($checksum)) { + $random = $this->request->getHeader('spreed-signaling-random'); + $checksum = $this->request->getHeader('spreed-signaling-checksum'); + $secret = $this->talkConfig->getSignalingSecret(); + try { + return $this->checksumVerificationService->validateRequest($random, $checksum, $secret, $data); + } catch (UnauthorizedException) { return false; } - $hash = hash_hmac('sha256', $random . $data, $this->talkConfig->getSignalingSecret()); - return hash_equals($hash, strtolower($checksum)); } /** diff --git a/lib/Service/ChecksumVerificationService.php b/lib/Service/ChecksumVerificationService.php index c090ee2a417..99fe7de37f3 100644 --- a/lib/Service/ChecksumVerificationService.php +++ b/lib/Service/ChecksumVerificationService.php @@ -41,7 +41,7 @@ public function validateRequest(string $random, string $checksum, string $secret } if ($secret === '') { - throw new UnauthorizedException('No shared SIP secret provided'); + throw new UnauthorizedException('No secret provided'); } $hash = hash_hmac('sha256', $random . $data, $secret); diff --git a/tests/php/Controller/SignalingControllerTest.php b/tests/php/Controller/SignalingControllerTest.php index 536b92d519e..154824cce36 100644 --- a/tests/php/Controller/SignalingControllerTest.php +++ b/tests/php/Controller/SignalingControllerTest.php @@ -22,6 +22,7 @@ use OCA\Talk\Participant; use OCA\Talk\Room; use OCA\Talk\Service\BanService; +use OCA\Talk\Service\ChecksumVerificationService; use OCA\Talk\Service\ParticipantService; use OCA\Talk\Service\RoomService; use OCA\Talk\Service\SessionService; @@ -31,7 +32,6 @@ use OCP\AppFramework\Services\IAppConfig; use OCP\AppFramework\Utility\ITimeFactory; use OCP\EventDispatcher\IEventDispatcher; -use OCP\Http\Client\IClientService; use OCP\IConfig; use OCP\IDBConnection; use OCP\IGroupManager; @@ -65,6 +65,7 @@ protected function getInputStream(): string { class SignalingControllerTest extends TestCase { protected TalkSession&MockObject $session; protected \OCA\Talk\Signaling\Manager&MockObject $signalingManager; + protected IRequest&MockObject $request; protected Manager|MockObject $manager; protected ParticipantService&MockObject $participantService; protected RoomService&MockObject $roomService; @@ -72,7 +73,7 @@ class SignalingControllerTest extends TestCase { protected Messages&MockObject $messages; protected IUserManager&MockObject $userManager; protected ITimeFactory&MockObject $timeFactory; - protected IClientService&MockObject $clientService; + protected ChecksumVerificationService $checksumVerificationService; protected IThrottler&MockObject $throttler; protected BanService&MockObject $banService; protected LoggerInterface&MockObject $logger; @@ -91,13 +92,14 @@ public function setUp(): void { $this->userId = 'testUser'; $this->secureRandom = \OCP\Server::get(ISecureRandom::class); + $this->request = $this->createMock(IRequest::class); /** @var MockObject|IAppConfig $appConfig */ $appConfig = $this->createMock(IAppConfig::class); $timeFactory = $this->createMock(ITimeFactory::class); $groupManager = $this->createMock(IGroupManager::class); $this->serverConfig = \OCP\Server::get(IConfig::class); $this->serverConfig->setAppValue('spreed', 'signaling_servers', json_encode([ - 'secret' => 'MySecretValue', + 'secret' => 'MySecretValueMySecretValue1234567890', ])); $this->serverConfig->setAppValue('spreed', 'signaling_ticket_secret', 'the-app-ticket-secret'); $this->serverConfig->setUserValue($this->userId, 'spreed', 'signaling_ticket_secret', 'the-user-ticket-secret'); @@ -114,7 +116,7 @@ public function setUp(): void { $this->sessionService = $this->createMock(SessionService::class); $this->messages = $this->createMock(Messages::class); $this->timeFactory = $this->createMock(ITimeFactory::class); - $this->clientService = $this->createMock(IClientService::class); + $this->checksumVerificationService = new ChecksumVerificationService(); $this->banService = $this->createMock(BanService::class); $this->logger = $this->createMock(LoggerInterface::class); $this->authenticator = $this->createMock(Authenticator::class); @@ -124,7 +126,7 @@ public function setUp(): void { private function recreateSignalingController() { $this->controller = new CustomInputSignalingController( 'spreed', - $this->createMock(IRequest::class), + $this->request, $this->config, $this->signalingManager, $this->session, @@ -137,7 +139,7 @@ private function recreateSignalingController() { $this->userManager, $this->dispatcher, $this->timeFactory, - $this->clientService, + $this->checksumVerificationService, $this->banService, $this->logger, $this->authenticator, @@ -555,19 +557,24 @@ public function testBackendChecksums(): void { // Test checksum generation / validation with the example from the API documentation. $data = '{"type":"auth","auth":{"version":"1.0","params":{"hello":"world"}}}'; $random = 'afb6b872ab03e3376b31bf0af601067222ff7990335ca02d327071b73c0119c6'; - $checksum = '3c4a69ff328299803ac2879614b707c807b4758cf19450755c60656cac46e3bc'; + $checksum = '37455eb661e5a4bc81cc7e1b10beb2cf09c30d0161f82ce12afcc108dd76e4f3'; $this->assertEquals($checksum, $this->calculateBackendChecksum($data, $random)); $this->assertTrue($this->validateBackendRandom($data, $random, $checksum)); } + protected string $checksum = ''; + private function performBackendRequest($data) { if (!is_string($data)) { $data = json_encode($data); } $random = 'afb6b872ab03e3376b31bf0af601067222ff7990335ca02d327071b73c0119c6'; - $checksum = $this->calculateBackendChecksum($data, $random); - $_SERVER['HTTP_SPREED_SIGNALING_RANDOM'] = $random; - $_SERVER['HTTP_SPREED_SIGNALING_CHECKSUM'] = $checksum; + $this->checksum = $this->calculateBackendChecksum($data, $random); + $this->request->method('getHeader') + ->willReturnCallback(fn ($header): string => match ($header) { + 'spreed-signaling-random' => $random, + 'spreed-signaling-checksum' => $this->checksum, + }); $this->controller->setInputStream($data); return $this->controller->backend(); } @@ -590,8 +597,11 @@ public function testBackendChecksumValidation(): void { $this->controller->setInputStream($data); $random = 'afb6b872ab03e3376b31bf0af601067222ff7990335ca02d327071b73c0119c6'; $checksum = $this->calculateBackendChecksum('{"foo": "bar"}', $random); - $_SERVER['HTTP_SPREED_SIGNALING_RANDOM'] = $random; - $_SERVER['HTTP_SPREED_SIGNALING_CHECKSUM'] = $checksum; + $this->request->method('getHeader') + ->willReturnMap([ + ['spreed-signaling-random', $random], + ['spreed-signaling-checksum', $checksum], + ]); $result = $this->controller->backend(); $this->assertSame([ 'type' => 'error', @@ -605,8 +615,11 @@ public function testBackendChecksumValidation(): void { $this->controller->setInputStream($data); $random = '12345'; $checksum = $this->calculateBackendChecksum($data, $random); - $_SERVER['HTTP_SPREED_SIGNALING_RANDOM'] = $random; - $_SERVER['HTTP_SPREED_SIGNALING_CHECKSUM'] = $checksum; + $this->request->method('getHeader') + ->willReturnMap([ + ['spreed-signaling-random', $random], + ['spreed-signaling-checksum', $checksum], + ]); $result = $this->controller->backend(); $this->assertSame([ 'type' => 'error', diff --git a/tests/php/Service/ChecksumVerificationServiceTest.php b/tests/php/Service/ChecksumVerificationServiceTest.php index aaf23c7fa11..f670ccd50a2 100644 --- a/tests/php/Service/ChecksumVerificationServiceTest.php +++ b/tests/php/Service/ChecksumVerificationServiceTest.php @@ -31,7 +31,7 @@ public static function dataValidateRequest(): array { ['', '', '', '', '', false], ['1234', '', '', '', 'Invalid random provided', false], [str_repeat('1', 32), '', '', '', 'Invalid checksum provided', false], - [str_repeat('1', 32), 'fake', '', '', 'No shared SIP secret provided', false], + [str_repeat('1', 32), 'fake', '', '', 'No secret provided', false], [str_repeat('1', 32), 'fake', 'invalid', '', 'Invalid HMAC provided', false], [$validRandom, $validChecksum, $validSecret, $fakeData, '', true], ];