diff --git a/lib/Controller/RecordingController.php b/lib/Controller/RecordingController.php index d606ff835146..6a5c11f0c4c0 100644 --- a/lib/Controller/RecordingController.php +++ b/lib/Controller/RecordingController.php @@ -375,7 +375,7 @@ public function stop(): DataResponse { /** * Store the recording * - * @param string $owner User that will own the recording file + * @param ?string $owner User that will own the recording file. `null` is actually not allowed and will always result in a "400 Bad Request". It's only allowed code-wise to handle requests where the post data exceeded the limits, so we can return a proper error instead of "500 Internal Server Error". * @return DataResponse, array{}>|DataResponse|DataResponse * * 200: Recording stored successfully @@ -386,7 +386,7 @@ public function stop(): DataResponse { #[BruteForceProtection(action: 'talkRecordingSecret')] #[OpenAPI(scope: 'backend-recording')] #[RequireRoom] - public function store(string $owner): DataResponse { + public function store(?string $owner): DataResponse { $data = $this->room->getToken(); if (!$this->validateBackendRequest($data)) { $response = new DataResponse([ @@ -400,6 +400,16 @@ public function store(string $owner): DataResponse { return $response; } + if ($owner === null) { + $this->logger->error('Recording backend failed to provide the owner when uploading a recording [ conversation: "' . $this->room->getToken() . '" ]. Most likely the post_max_size or upload_max_filesize were exceeded.'); + try { + $this->recordingService->notifyAboutFailedStore($this->room); + } catch (InvalidArgumentException) { + // Ignoring, we logged an error already + } + return new DataResponse(['error' => 'size'], Http::STATUS_BAD_REQUEST); + } + try { $file = $this->request->getUploadedFile('file'); $this->recordingService->store($this->getRoom(), $owner, $file); diff --git a/lib/Service/RecordingService.php b/lib/Service/RecordingService.php index 886bd0e1088d..7b9ec02bccb3 100644 --- a/lib/Service/RecordingService.php +++ b/lib/Service/RecordingService.php @@ -37,6 +37,7 @@ use OCA\Talk\Participant; use OCA\Talk\Recording\BackendNotifier; use OCA\Talk\Room; +use OCP\AppFramework\Services\IAppConfig; use OCP\AppFramework\Utility\ITimeFactory; use OCP\Files\File; use OCP\Files\Folder; @@ -57,6 +58,8 @@ class RecordingService { public const CONSENT_REQUIRED_YES = 1; public const CONSENT_REQUIRED_OPTIONAL = 2; + public const APPCONFIG_PREFIX = 'recording/'; + public const DEFAULT_ALLOWED_RECORDING_FORMATS = [ 'audio/ogg' => ['ogg'], 'video/ogg' => ['ogv'], @@ -82,6 +85,7 @@ public function __construct( protected ITimeFactory $timeFactory, protected Config $config, protected IConfig $serverConfig, + protected IAppConfig $appConfig, protected RoomService $roomService, protected ShareManager $shareManager, protected ChatManager $chatManager, @@ -110,6 +114,7 @@ public function start(Room $room, int $status, string $owner, Participant $parti $startingStatus = $status === Room::RECORDING_VIDEO ? Room::RECORDING_VIDEO_STARTING : Room::RECORDING_AUDIO_STARTING; $this->roomService->setCallRecording($room, $startingStatus); + $this->appConfig->setAppValueString(self::APPCONFIG_PREFIX . $room->getToken(), $owner, true, true); } public function stop(Room $room, ?Participant $participant = null): void { @@ -128,6 +133,7 @@ public function stop(Room $room, ?Participant $participant = null): void { } public function store(Room $room, string $owner, array $file): void { + $this->appConfig->deleteAppValue(self::APPCONFIG_PREFIX . $room->getToken()); try { $participant = $this->participantService->getParticipant($room, $owner); } catch (ParticipantNotFoundException $e) { @@ -190,6 +196,35 @@ public function storeTranscript(string $owner, File $recording, string $transcri } } + /** + * @throws InvalidArgumentException + */ + public function notifyAboutFailedStore(Room $room): void { + $owner = $this->appConfig->getAppValueString(self::APPCONFIG_PREFIX . $room->getToken(), lazy: true); + if ($owner === '') { + return; + } + + try { + $participant = $this->participantService->getParticipant($room, $owner); + } catch (ParticipantNotFoundException) { + $this->logger->warning('Could not determinate conversation when trying to notify about failed upload of call recording'); + throw new InvalidArgumentException('owner_participant'); + } + + $attendee = $participant->getAttendee(); + + $notification = $this->notificationManager->createNotification(); + + $notification + ->setApp('spreed') + ->setDateTime($this->timeFactory->getDateTime()) + ->setObject('recording_information', $room->getToken()) + ->setUser($attendee->getActorId()) + ->setSubject('record_file_store_fail'); + $this->notificationManager->notify($notification); + } + public function notifyAboutFailedTranscript(string $owner, File $recording): void { $recordingFolder = $recording->getParent(); $roomToken = $recordingFolder->getName(); diff --git a/openapi-backend-recording.json b/openapi-backend-recording.json index 9555d8bf6323..636ae832288a 100644 --- a/openapi-backend-recording.json +++ b/openapi-backend-recording.json @@ -525,10 +525,10 @@ { "name": "owner", "in": "query", - "description": "User that will own the recording file", - "required": true, + "description": "User that will own the recording file. `null` is actually not allowed and will always result in a \"400 Bad Request\". It's only allowed code-wise to handle requests where the post data exceeded the limits, so we can return a proper error instead of \"500 Internal Server Error\".", "schema": { - "type": "string" + "type": "string", + "nullable": true } }, { diff --git a/openapi-full.json b/openapi-full.json index 8489fc96b4f2..ef189714160a 100644 --- a/openapi-full.json +++ b/openapi-full.json @@ -18084,10 +18084,10 @@ { "name": "owner", "in": "query", - "description": "User that will own the recording file", - "required": true, + "description": "User that will own the recording file. `null` is actually not allowed and will always result in a \"400 Bad Request\". It's only allowed code-wise to handle requests where the post data exceeded the limits, so we can return a proper error instead of \"500 Internal Server Error\".", "schema": { - "type": "string" + "type": "string", + "nullable": true } }, { diff --git a/src/types/openapi/openapi-backend-recording.ts b/src/types/openapi/openapi-backend-recording.ts index 6bb66d5358bd..2ec763b87b36 100644 --- a/src/types/openapi/openapi-backend-recording.ts +++ b/src/types/openapi/openapi-backend-recording.ts @@ -181,9 +181,9 @@ export type operations = { /** Store the recording */ "recording-store": { parameters: { - query: { - /** @description User that will own the recording file */ - owner: string; + query?: { + /** @description User that will own the recording file. `null` is actually not allowed and will always result in a "400 Bad Request". It's only allowed code-wise to handle requests where the post data exceeded the limits, so we can return a proper error instead of "500 Internal Server Error". */ + owner?: string | null; }; header: { /** @description Required to be true for the API request to pass */ diff --git a/src/types/openapi/openapi-full.ts b/src/types/openapi/openapi-full.ts index 69bb82418f93..65a818bf8d53 100644 --- a/src/types/openapi/openapi-full.ts +++ b/src/types/openapi/openapi-full.ts @@ -6558,9 +6558,9 @@ export type operations = { /** Store the recording */ "recording-store": { parameters: { - query: { - /** @description User that will own the recording file */ - owner: string; + query?: { + /** @description User that will own the recording file. `null` is actually not allowed and will always result in a "400 Bad Request". It's only allowed code-wise to handle requests where the post data exceeded the limits, so we can return a proper error instead of "500 Internal Server Error". */ + owner?: string | null; }; header: { /** @description Required to be true for the API request to pass */ diff --git a/tests/integration/features/bootstrap/FeatureContext.php b/tests/integration/features/bootstrap/FeatureContext.php index 03b79c846b05..4cc749325e6c 100644 --- a/tests/integration/features/bootstrap/FeatureContext.php +++ b/tests/integration/features/bootstrap/FeatureContext.php @@ -4115,8 +4115,6 @@ public function userStopRecordingInRoom(string $user, string $identifier, int $s * @When /^user "([^"]*)" store recording file "([^"]*)" in room "([^"]*)" with (\d+)(?: \((v1)\))?$/ */ public function userStoreRecordingFileInRoom(string $user, string $file, string $identifier, int $statusCode, string $apiVersion = 'v1'): void { - $this->setCurrentUser($user); - $recordingServerSharedSecret = 'the secret'; $this->setAppConfig('spreed', new TableNode([['recording_servers', json_encode(['secret' => $recordingServerSharedSecret])]])); $validRandom = md5((string) rand()); @@ -4125,7 +4123,14 @@ public function userStoreRecordingFileInRoom(string $user, string $file, string 'TALK_RECORDING_RANDOM' => $validRandom, 'TALK_RECORDING_CHECKSUM' => $validChecksum, ]; - $options = ['multipart' => [['name' => 'owner', 'contents' => $user]]]; + + $options = ['multipart' => []]; + if ($user !== 'NULL') { + // When exceeding post_max_size, the owner parameter is not sent: + // RecordingController::store(): Argument #1 ($owner) must be of type string, null given + $options['multipart'][] = ['name' => 'owner', 'contents' => $user]; + } + if ($file === 'invalid') { // Create invalid content $options['multipart'][] = [ diff --git a/tests/integration/features/callapi/recording.feature b/tests/integration/features/callapi/recording.feature index cef1d14b6c2c..be68e6bfca84 100644 --- a/tests/integration/features/callapi/recording.feature +++ b/tests/integration/features/callapi/recording.feature @@ -508,7 +508,7 @@ Feature: callapi/recording | spreed | recording | room1 | Failed to transcript call recording | The server failed to transcript the recording at /Talk/Recording/ROOM(room1)/leave_call.ogg for the call in room1. Please reach out to the administration. | | spreed | recording | room1 | Call recording now available | The recording for the call in room1 was uploaded to /Talk/Recording/ROOM(room1)/leave_call.ogg. | - Scenario: Store recording with failure + Scenario: Store recording with failure exceeding the upload_max_filesize Given user "participant1" creates room "room1" (v4) | roomType | 2 | | roomName | room1 | @@ -521,6 +521,28 @@ Feature: callapi/recording | type | name | callRecording | | 2 | room1 | 0 | + Scenario: Store recording with failure exceeding the post_max_size + Given recording server is started + Given user "participant1" creates room "room1" (v4) + | roomType | 2 | + | roomName | room1 | + And user "participant1" joins room "room1" with 200 (v4) + And user "participant1" joins call "room1" with 200 (v4) + And user "participant1" starts "audio" recording in room "room1" with 200 (v1) + And recording server received the following requests + | token | data | + | room1 | {"type":"start","start":{"status":2,"owner":"participant1","actor":{"type":"users","id":"participant1"}}} | + And recording server sent started request for "audio" recording in room "room1" as "participant1" with 200 + When user "participant1" ends call "room1" with 200 (v4) + Then recording server received the following requests + | token | data | + | room1 | {"type":"stop","stop":[]} | + And recording server sent stopped request for recording in room "room1" as "participant1" with 200 + When user "NULL" store recording file "big" in room "room1" with 400 (v1) + Then user "participant1" has the following notifications + | app | object_type | object_id | subject | + | spreed | recording_information | room1 | Failed to upload call recording | + Scenario: Stop recording automatically when end the call Given recording server is started And user "participant1" creates room "room1" (v4) diff --git a/tests/php/Service/RecordingServiceTest.php b/tests/php/Service/RecordingServiceTest.php index 42d4383b5efa..522125e17d94 100644 --- a/tests/php/Service/RecordingServiceTest.php +++ b/tests/php/Service/RecordingServiceTest.php @@ -46,6 +46,7 @@ function is_uploaded_file($filename) { use OCA\Talk\Service\ParticipantService; use OCA\Talk\Service\RecordingService; use OCA\Talk\Service\RoomService; +use OCP\AppFramework\Services\IAppConfig; use OCP\AppFramework\Utility\ITimeFactory; use OCP\Files\IMimeTypeDetector; use OCP\Files\IRootFolder; @@ -99,6 +100,7 @@ public function setUp(): void { $this->timeFactory = $this->createMock(ITimeFactory::class); $this->config = $this->createMock(Config::class); $this->serverConfig = $this->createMock(IConfig::class); + $this->appConfig = $this->createMock(IAppConfig::class); $this->roomService = $this->createMock(RoomService::class); $this->shareManager = $this->createMock(ShareManager::class); $this->chatManager = $this->createMock(ChatManager::class); @@ -115,6 +117,7 @@ public function setUp(): void { $this->timeFactory, $this->config, $this->serverConfig, + $this->appConfig, $this->roomService, $this->shareManager, $this->chatManager,