Merge pull request #9253 from mishamosher/recording-upload-memory

Avoid out-of-memory errors when uploading a recording

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 [