diff options
author | Daniel Calviño Sánchez <danxuliu@gmail.com> | 2023-04-26 21:40:31 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2023-04-26 21:40:31 +0200 |
commit | b1ed68c574d77f657b9ca04cfb590682cc4a8ea7 (patch) | |
tree | d47e233ffee0e9929d5d9ee2eb6b7acc18f4852d | |
parent | 9aed218ee1aead4c79a270fbd284d0dd4246ba91 (diff) | |
parent | 7e036e4ebfc7b128c60c181db88a18c79b9c4a24 (diff) |
Merge pull request #9253 from mishamosher/recording-upload-memory
Avoid out-of-memory errors when uploading a recording
-rw-r--r-- | lib/Service/RecordingService.php | 46 | ||||
-rw-r--r-- | recording/pyproject.toml | 2 | ||||
-rw-r--r-- | recording/src/nextcloud/talk/recording/BackendNotifier.py | 29 | ||||
-rw-r--r-- | tests/php/Service/RecordingServiceTest.php | 29 |
4 files changed, 64 insertions, 42 deletions
diff --git a/lib/Service/RecordingService.php b/lib/Service/RecordingService.php index 00dac2e26..a5c2defe4 100644 --- a/lib/Service/RecordingService.php +++ b/lib/Service/RecordingService.php @@ -2,7 +2,8 @@ declare(strict_types=1); /** - * @copyright Copyright (c) 2022 Vitor Mattos <vitor@php.rio> + * @copyright Copyright (c) 2022, Vitor Mattos <vitor@php.rio> + * @copyright Copyright (c) 2023, Elmer Miroslav Mosher Golovin (miroslav@mishamosher.com) * * @author Vitor Mattos <vitor@php.rio> * @@ -127,14 +128,16 @@ class RecordingService { throw new InvalidArgumentException('owner_participant'); } - $content = $this->getContentFromFileArray($file, $room, $participant); + $resource = $this->getResourceFromFileArray($file, $room, $participant); $fileName = basename($file['name']); - $this->validateFileFormat($fileName, $content); + $fileRealPath = realpath($file['tmp_name']); + + $this->validateFileFormat($fileName, $fileRealPath); try { $recordingFolder = $this->getRecordingFolder($owner, $room->getToken()); - $fileNode = $recordingFolder->newFile($fileName, $content); + $fileNode = $recordingFolder->newFile($fileName, $resource); $this->notifyStoredRecording($room, $participant, $fileNode); } catch (NoUserException $e) { throw new InvalidArgumentException('owner_invalid'); @@ -203,7 +206,15 @@ class RecordingService { $this->notificationManager->notify($notification); } - public function getContentFromFileArray(array $file, Room $room, Participant $participant): string { + /** + * Gets a resource that represents the file contents of the file array. + * + * @param array $file File array from which a resource will be returned + * @param Room $room The Talk room that requests the resource + * @param Participant $participant The Talk participant that requests the resource + * @return resource Resource representing the file contents of the file array + */ + public function getResourceFromFileArray(array $file, Room $room, Participant $participant) { if ($file['error'] !== 0) { $error = self::UPLOAD_ERRORS[$file['error']]; $this->logger->error($error); @@ -220,17 +231,30 @@ class RecordingService { throw new InvalidArgumentException('invalid_file'); } - $content = file_get_contents($file['tmp_name']); - unlink($file['tmp_name']); + $resource = fopen($file['tmp_name'], 'r'); + if ($resource === false) { + throw new InvalidArgumentException('fopen_failed'); + } - if (!$content) { + $resourceStat = fstat($resource); + if ($resourceStat === false) { + throw new InvalidArgumentException('fstat_failed'); + } + + if ($resourceStat['size'] === 0) { throw new InvalidArgumentException('empty_file'); } - return $content; + + return $resource; } - public function validateFileFormat(string $fileName, $content): void { - $mimeType = $this->mimeTypeDetector->detectString($content); + public function validateFileFormat(string $fileName, string $fileRealPath): void { + if (!is_file($fileRealPath)) { + $this->logger->warning("An invalid file path ($fileRealPath) was provided"); + throw new InvalidArgumentException('file_invalid_path'); + } + + $mimeType = $this->mimeTypeDetector->detectContent($fileRealPath); $allowed = self::DEFAULT_ALLOWED_RECORDING_FORMATS; if (!array_key_exists($mimeType, $allowed)) { $this->logger->warning("Uploaded file detected mime type ($mimeType) is not allowed"); diff --git a/recording/pyproject.toml b/recording/pyproject.toml index 35653c7e6..726022079 100644 --- a/recording/pyproject.toml +++ b/recording/pyproject.toml @@ -9,8 +9,8 @@ dependencies = [ "flask", "pulsectl", "pyvirtualdisplay>=2.0", + "requests-toolbelt", "selenium>=4.6.0", - "urllib3", "websocket-client", ] dynamic = ["version"] diff --git a/recording/src/nextcloud/talk/recording/BackendNotifier.py b/recording/src/nextcloud/talk/recording/BackendNotifier.py index 294e3fc63..eff58caea 100644 --- a/recording/src/nextcloud/talk/recording/BackendNotifier.py +++ b/recording/src/nextcloud/talk/recording/BackendNotifier.py @@ -27,11 +27,12 @@ import hmac import json import logging import os +import requests import ssl from nextcloud.talk import recording +from requests import Request, Session +from requests_toolbelt import MultipartEncoder from secrets import token_urlsafe -from urllib.request import Request, urlopen -from urllib3 import encode_multipart_formdata from .Config import config @@ -62,13 +63,12 @@ def doRequest(backend, request, retries=3): """ context = None - if config.getBackendSkipVerify(backend): - context = ssl.create_default_context() - context.check_hostname = False - context.verify_mode = ssl.CERT_NONE + backendSkipVerify = config.getBackendSkipVerify(backend) try: - urlopen(request, context=context) + session = Session() + preparedRequest = session.prepare_request(request) + session.send(preparedRequest, verify=not backendSkipVerify) except Exception as exception: if retries > 1: logger.exception(f"Failed to send message to backend, {retries} retries left!") @@ -102,7 +102,7 @@ def backendRequest(backend, data): 'User-Agent': recording.USER_AGENT, } - backendRequest = Request(url, data, headers) + backendRequest = Request('POST', url, headers, data=data) doRequest(backend, backendRequest) @@ -190,28 +190,25 @@ def uploadRecording(backend, token, fileName, owner): url = backend.rstrip('/') + '/ocs/v2.php/apps/spreed/api/v1/recording/' + token + '/store' - fileContents = None - with open(fileName, 'rb') as file: - fileContents = file.read() - # Plain values become arguments, while tuples become files; the body used to # calculate the checksum is empty. data = { 'owner': owner, - 'file': (os.path.basename(fileName), fileContents), + 'file': (os.path.basename(fileName), open(fileName, 'rb')), } - data, contentType = encode_multipart_formdata(data) + + multipartEncoder = MultipartEncoder(data) random, checksum = getRandomAndChecksum(backend, token.encode()) headers = { - 'Content-Type': contentType, + 'Content-Type': multipartEncoder.content_type, 'OCS-ApiRequest': 'true', 'Talk-Recording-Random': random, 'Talk-Recording-Checksum': checksum, 'User-Agent': recording.USER_AGENT, } - uploadRequest = Request(url, data, headers) + uploadRequest = Request('POST', url, headers, data=multipartEncoder) doRequest(backend, uploadRequest) diff --git a/tests/php/Service/RecordingServiceTest.php b/tests/php/Service/RecordingServiceTest.php index 1ff6fcea6..94e9ae27b 100644 --- a/tests/php/Service/RecordingServiceTest.php +++ b/tests/php/Service/RecordingServiceTest.php @@ -3,6 +3,7 @@ declare(strict_types=1); /** * @copyright Copyright (c) 2022, Vitor Mattos <vitor@php.rio> + * @copyright Copyright (c) 2023, Elmer Miroslav Mosher Golovin (miroslav@mishamosher.com) * * @author Vitor Mattos <vitor@php.rio> * @@ -119,34 +120,35 @@ class RecordingServiceTest extends TestCase { } /** @dataProvider dataValidateFileFormat */ - public function testValidateFileFormat(string $fileName, string $content, string $exceptionMessage):void { + public function testValidateFileFormat(string $fileName, string $fileRealPath, string $exceptionMessage): void { if ($exceptionMessage) { $this->expectExceptionMessage($exceptionMessage); } else { $this->expectNotToPerformAssertions(); } - $this->recordingService->validateFileFormat($fileName, $content); + $this->recordingService->validateFileFormat($fileName, $fileRealPath); } public function dataValidateFileFormat(): array { return [ + # file_invalid_path + ['', '', 'file_invalid_path'], # file_mimetype - ['', '', 'file_mimetype'], - ['', file_get_contents(__DIR__ . '/../../../img/app.svg'), 'file_mimetype'], - ['name.ogg', file_get_contents(__DIR__ . '/../../../img/app.svg'), 'file_mimetype'], + ['', realpath(__DIR__ . '/../../../img/app.svg'), 'file_mimetype'], + ['name.ogg', realpath(__DIR__ . '/../../../img/app.svg'), 'file_mimetype'], # file_extension - ['', file_get_contents(__DIR__ . '/../../../img/join_call.ogg'), 'file_extension'], - ['name', file_get_contents(__DIR__ . '/../../../img/join_call.ogg'), 'file_extension'], - ['name.mp3', file_get_contents(__DIR__ . '/../../../img/join_call.ogg'), 'file_extension'], + ['', realpath(__DIR__ . '/../../../img/join_call.ogg'), 'file_extension'], + ['name', realpath(__DIR__ . '/../../../img/join_call.ogg'), 'file_extension'], + ['name.mp3', realpath(__DIR__ . '/../../../img/join_call.ogg'), 'file_extension'], # Success - ['name.ogg', file_get_contents(__DIR__ . '/../../../img/join_call.ogg'), ''], + ['name.ogg', realpath(__DIR__ . '/../../../img/join_call.ogg'), ''], ]; } /** - * @dataProvider dataGetContentFromFileArray + * @dataProvider dataGetResourceFromFileArray */ - public function testGetContentFromFileArray(array $file, $expected, string $exceptionMessage): void { + public function testGetResourceFromFileArray(array $file, $expected, string $exceptionMessage): void { if ($exceptionMessage) { $this->expectExceptionMessage($exceptionMessage); } @@ -158,12 +160,11 @@ class RecordingServiceTest extends TestCase { ]); $participant = new Participant($room, $attendee, null); - $actual = $this->recordingService->getContentFromFileArray($file, $room, $participant); + $actual = stream_get_contents($this->recordingService->getResourceFromFileArray($file, $room, $participant)); $this->assertEquals($expected, $actual); - $this->assertFileDoesNotExist($file['tmp_name']); } - public function dataGetContentFromFileArray(): array { + public function dataGetResourceFromFileArray(): array { $fileWithContent = tempnam(sys_get_temp_dir(), 'txt'); file_put_contents($fileWithContent, 'bla'); return [ |