fix(recording): Handle the problem gracefully when the recording can …

…not be uploaded

Signed-off-by: Joas Schilling <coding@schilljs.com>

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<Http::STATUS_OK, array<empty>, array{}>|DataResponse<Http::STATUS_BAD_REQUEST, array{error: string}, array{}>|DataResponse<Http::STATUS_UNAUTHORIZED, array{type: string, error: array{code: string, message: string}}, array{}>
 	 *
 	 * 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);

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();

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
                         }
                     },
                     {

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
                         }
                     },
                     {

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 */

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 */

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'][] = [

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)

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;
@@ -68,6 +69,8 @@ class RecordingServiceTest extends TestCase {
 	private $config;
 	/** @var IConfig|MockObject */
 	private $serverConfig;
+	/** @var IAppConfig|MockObject */
+	private $appConfig;
 	/** @var IManager|MockObject */
 	private $notificationManager;
 	/** @var Manager|MockObject */
@@ -99,6 +102,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 +119,7 @@ public function setUp(): void {
 			$this->timeFactory,
 			$this->config,
 			$this->serverConfig,
+			$this->appConfig,
 			$this->roomService,
 			$this->shareManager,
 			$this->chatManager,