Skip to content

Commit

Permalink
fix(recording): Handle the problem gracefully when the recording can …
Browse files Browse the repository at this point in the history
…not be uploaded

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

[skip ci]
  • Loading branch information
nickvergessen authored and backportbot[bot] committed May 23, 2024
1 parent e88b366 commit 17ef584
Show file tree
Hide file tree
Showing 9 changed files with 93 additions and 18 deletions.
14 changes: 12 additions & 2 deletions lib/Controller/RecordingController.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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([
Expand All @@ -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);
Expand Down
35 changes: 35 additions & 0 deletions lib/Service/RecordingService.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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'],
Expand All @@ -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,
Expand Down Expand Up @@ -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 {
Expand All @@ -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) {
Expand Down Expand Up @@ -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();
Expand Down
6 changes: 3 additions & 3 deletions openapi-backend-recording.json
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
},
{
Expand Down
6 changes: 3 additions & 3 deletions openapi-full.json
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
},
{
Expand Down
6 changes: 3 additions & 3 deletions src/types/openapi/openapi-backend-recording.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down
6 changes: 3 additions & 3 deletions src/types/openapi/openapi-full.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down
11 changes: 8 additions & 3 deletions tests/integration/features/bootstrap/FeatureContext.php
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand All @@ -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'][] = [
Expand Down
24 changes: 23 additions & 1 deletion tests/integration/features/callapi/recording.feature
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand All @@ -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)
Expand Down
3 changes: 3 additions & 0 deletions tests/php/Service/RecordingServiceTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -115,6 +117,7 @@ public function setUp(): void {
$this->timeFactory,
$this->config,
$this->serverConfig,
$this->appConfig,
$this->roomService,
$this->shareManager,
$this->chatManager,
Expand Down

0 comments on commit 17ef584

Please sign in to comment.