From 020d3d3d79338148873cfd78ba59856f63260f2f Mon Sep 17 00:00:00 2001 From: Chocobozzz Date: Wed, 10 Nov 2021 09:42:37 +0100 Subject: [PATCH] Remove resumable cache after upload success --- server/controllers/api/videos/upload.ts | 26 ++++-- server/lib/redis.ts | 4 + .../middlewares/validators/videos/videos.ts | 19 ++++- server/tests/api/videos/resumable-upload.ts | 79 +++++++++++++++++-- shared/extra-utils/videos/videos-command.ts | 14 +++- 5 files changed, 124 insertions(+), 18 deletions(-) diff --git a/server/controllers/api/videos/upload.ts b/server/controllers/api/videos/upload.ts index 02aadd426..3e9979330 100644 --- a/server/controllers/api/videos/upload.ts +++ b/server/controllers/api/videos/upload.ts @@ -19,7 +19,7 @@ import { VideoPathManager } from '@server/lib/video-path-manager' import { buildNextVideoState } from '@server/lib/video-state' import { openapiOperationDoc } from '@server/middlewares/doc' import { MVideo, MVideoFile, MVideoFullLight } from '@server/types/models' -import { uploadx } from '@uploadx/core' +import { Uploadx } from '@uploadx/core' import { VideoCreate, VideoState } from '../../../../shared' import { HttpStatusCode } from '../../../../shared/models' import { auditLoggerFactory, getAuditIdFromRes, VideoAuditView } from '../../../helpers/audit-logger' @@ -41,6 +41,7 @@ import { authenticate, videosAddLegacyValidator, videosAddResumableInitValidator, + videosResumableUploadIdValidator, videosAddResumableValidator } from '../../../middlewares' import { ScheduleVideoUpdateModel } from '../../../models/video/schedule-video-update' @@ -50,7 +51,9 @@ import { VideoFileModel } from '../../../models/video/video-file' const lTags = loggerTagsFactory('api', 'video') const auditLogger = auditLoggerFactory('videos') const uploadRouter = express.Router() -const uploadxMiddleware = uploadx.upload({ directory: getResumableUploadPath() }) + +const uploadx = new Uploadx({ directory: getResumableUploadPath() }) +uploadx.getUserId = (_, res: express.Response) => res.locals.oauth?.token.user.id const reqVideoFileAdd = createReqFiles( [ 'videofile', 'thumbnailfile', 'previewfile' ], @@ -84,18 +87,21 @@ uploadRouter.post('/upload-resumable', authenticate, reqVideoFileAddResumable, asyncMiddleware(videosAddResumableInitValidator), - uploadxMiddleware + uploadx.upload ) uploadRouter.delete('/upload-resumable', authenticate, - uploadxMiddleware + videosResumableUploadIdValidator, + asyncMiddleware(deleteUploadResumableCache), + uploadx.upload ) uploadRouter.put('/upload-resumable', openapiOperationDoc({ operationId: 'uploadResumable' }), authenticate, - uploadxMiddleware, // uploadx doesn't next() before the file upload completes + videosResumableUploadIdValidator, + uploadx.upload, // uploadx doesn't next() before the file upload completes asyncMiddleware(videosAddResumableValidator), asyncMiddleware(addVideoResumable) ) @@ -108,7 +114,7 @@ export { // --------------------------------------------------------------------------- -export async function addVideoLegacy (req: express.Request, res: express.Response) { +async function addVideoLegacy (req: express.Request, res: express.Response) { // Uploading the video could be long // Set timeout to 10 minutes, as Express's default is 2 minutes req.setTimeout(1000 * 60 * 10, () => { @@ -128,7 +134,7 @@ export async function addVideoLegacy (req: express.Request, res: express.Respons return res.json(response) } -export async function addVideoResumable (req: express.Request, res: express.Response) { +async function addVideoResumable (req: express.Request, res: express.Response) { const videoPhysicalFile = res.locals.videoFileResumable const videoInfo = videoPhysicalFile.metadata const files = { previewfile: videoInfo.previewfile } @@ -291,3 +297,9 @@ function createTorrentFederate (video: MVideoFullLight, videoFile: MVideoFile) { }) .catch(err => logger.error('Cannot federate or notify video creation %s', video.url, { err, ...lTags(video.uuid) })) } + +async function deleteUploadResumableCache (req: express.Request, res: express.Response, next: express.NextFunction) { + await Redis.Instance.deleteUploadSession(req.query.upload_id) + + return next() +} diff --git a/server/lib/redis.ts b/server/lib/redis.ts index 76b7868e8..8aec4b793 100644 --- a/server/lib/redis.ts +++ b/server/lib/redis.ts @@ -271,6 +271,10 @@ class Redis { : '' } + deleteUploadSession (uploadId: string) { + return this.deleteKey('resumable-upload-' + uploadId) + } + /* ************ Keys generation ************ */ generateCachedRouteKey (req: express.Request) { diff --git a/server/middlewares/validators/videos/videos.ts b/server/middlewares/validators/videos/videos.ts index 5f1234379..53643635c 100644 --- a/server/middlewares/validators/videos/videos.ts +++ b/server/middlewares/validators/videos/videos.ts @@ -103,6 +103,22 @@ const videosAddLegacyValidator = getCommonVideoEditAttributes().concat([ } ]) +const videosResumableUploadIdValidator = [ + (req: express.Request, res: express.Response, next: express.NextFunction) => { + const user = res.locals.oauth.token.User + const uploadId = req.query.upload_id + + if (uploadId.startsWith(user.id + '-') !== true) { + return res.fail({ + status: HttpStatusCode.FORBIDDEN_403, + message: 'You cannot send chunks in another user upload' + }) + } + + return next() + } +] + /** * Gets called after the last PUT request */ @@ -110,7 +126,7 @@ const videosAddResumableValidator = [ async (req: express.Request, res: express.Response, next: express.NextFunction) => { const user = res.locals.oauth.token.User const body: express.CustomUploadXFile = req.body - const file = { ...body, duration: undefined, path: getResumableUploadPath(body.id), filename: body.metadata.filename } + const file = { ...body, duration: undefined, path: getResumableUploadPath(body.name), filename: body.metadata.filename } const cleanup = () => deleteFileAndCatch(file.path) const uploadId = req.query.upload_id @@ -552,6 +568,7 @@ export { videosAddLegacyValidator, videosAddResumableValidator, videosAddResumableInitValidator, + videosResumableUploadIdValidator, videosUpdateValidator, videosGetValidator, diff --git a/server/tests/api/videos/resumable-upload.ts b/server/tests/api/videos/resumable-upload.ts index 6b5e0c09d..1ba7cdbcc 100644 --- a/server/tests/api/videos/resumable-upload.ts +++ b/server/tests/api/videos/resumable-upload.ts @@ -22,6 +22,8 @@ describe('Test resumable upload', function () { const defaultFixture = 'video_short.mp4' let server: PeerTubeServer let rootId: number + let userAccessToken: string + let userChannelId: number async function buildSize (fixture: string, size?: number) { if (size !== undefined) return size @@ -30,24 +32,33 @@ describe('Test resumable upload', function () { return (await stat(baseFixture)).size } - async function prepareUpload (sizeArg?: number) { - const size = await buildSize(defaultFixture, sizeArg) + async function prepareUpload (options: { + channelId?: number + token?: string + size?: number + originalName?: string + lastModified?: number + } = {}) { + const { token, originalName, lastModified } = options + + const size = await buildSize(defaultFixture, options.size) const attributes = { name: 'video', - channelId: server.store.channel.id, + channelId: options.channelId ?? server.store.channel.id, privacy: VideoPrivacy.PUBLIC, fixture: defaultFixture } const mimetype = 'video/mp4' - const res = await server.videos.prepareResumableUpload({ attributes, size, mimetype }) + const res = await server.videos.prepareResumableUpload({ token, attributes, size, mimetype, originalName, lastModified }) return res.header['location'].split('?')[1] } async function sendChunks (options: { + token?: string pathUploadId: string size?: number expectedStatus?: HttpStatusCode @@ -55,12 +66,13 @@ describe('Test resumable upload', function () { contentRange?: string contentRangeBuilder?: (start: number, chunk: any) => string }) { - const { pathUploadId, expectedStatus, contentLength, contentRangeBuilder } = options + const { token, pathUploadId, expectedStatus, contentLength, contentRangeBuilder } = options const size = await buildSize(defaultFixture, options.size) const absoluteFilePath = buildAbsoluteFixturePath(defaultFixture) return server.videos.sendResumableChunks({ + token, pathUploadId, videoFilePath: absoluteFilePath, size, @@ -105,6 +117,12 @@ describe('Test resumable upload', function () { const body = await server.users.getMyInfo() rootId = body.id + { + userAccessToken = await server.users.generateUserAndToken('user1') + const { videoChannels } = await server.users.getMyInfo({ token: userAccessToken }) + userChannelId = videoChannels[0].id + } + await server.users.update({ userId: rootId, videoQuota: 10_000_000 }) }) @@ -147,14 +165,14 @@ describe('Test resumable upload', function () { }) it('Should not accept more chunks than expected', async function () { - const uploadId = await prepareUpload(100) + const uploadId = await prepareUpload({ size: 100 }) await sendChunks({ pathUploadId: uploadId, expectedStatus: HttpStatusCode.CONFLICT_409 }) await checkFileSize(uploadId, 0) }) it('Should not accept more chunks than expected with an invalid content length/content range', async function () { - const uploadId = await prepareUpload(1500) + const uploadId = await prepareUpload({ size: 1500 }) // Content length check seems to have changed in v16 if (process.version.startsWith('v16')) { @@ -167,7 +185,7 @@ describe('Test resumable upload', function () { }) it('Should not accept more chunks than expected with an invalid content length', async function () { - const uploadId = await prepareUpload(500) + const uploadId = await prepareUpload({ size: 500 }) const size = 1000 @@ -195,6 +213,51 @@ describe('Test resumable upload', function () { await checkFileSize(uploadId, null) }) + + it('Should not have the same upload id with 2 different users', async function () { + const originalName = 'toto.mp4' + const lastModified = new Date().getTime() + + const uploadId1 = await prepareUpload({ originalName, lastModified, token: server.accessToken }) + const uploadId2 = await prepareUpload({ originalName, lastModified, channelId: userChannelId, token: userAccessToken }) + + expect(uploadId1).to.not.equal(uploadId2) + }) + + it('Should have the same upload id with the same user', async function () { + const originalName = 'toto.mp4' + const lastModified = new Date().getTime() + + const uploadId1 = await prepareUpload({ originalName, lastModified }) + const uploadId2 = await prepareUpload({ originalName, lastModified }) + + expect(uploadId1).to.equal(uploadId2) + }) + + it('Should not cache a request with 2 different users', async function () { + const originalName = 'toto.mp4' + const lastModified = new Date().getTime() + + const uploadId = await prepareUpload({ originalName, lastModified, token: server.accessToken }) + + await sendChunks({ pathUploadId: uploadId, token: server.accessToken }) + await sendChunks({ pathUploadId: uploadId, token: userAccessToken, expectedStatus: HttpStatusCode.FORBIDDEN_403 }) + }) + + it('Should not cache a request after a delete', async function () { + const originalName = 'toto.mp4' + const lastModified = new Date().getTime() + const uploadId1 = await prepareUpload({ originalName, lastModified, token: server.accessToken }) + + await sendChunks({ pathUploadId: uploadId1 }) + await server.videos.endResumableUpload({ pathUploadId: uploadId1 }) + + const uploadId2 = await prepareUpload({ originalName, lastModified, token: server.accessToken }) + expect(uploadId1).to.equal(uploadId2) + + const result2 = await sendChunks({ pathUploadId: uploadId1 }) + expect(result2.headers['x-resumable-upload-cached']).to.not.exist + }) }) after(async function () { diff --git a/shared/extra-utils/videos/videos-command.ts b/shared/extra-utils/videos/videos-command.ts index 68241f062..167fae22d 100644 --- a/shared/extra-utils/videos/videos-command.ts +++ b/shared/extra-utils/videos/videos-command.ts @@ -469,8 +469,11 @@ export class VideosCommand extends AbstractCommand { attributes: VideoEdit size: number mimetype: string + + originalName?: string + lastModified?: number }) { - const { attributes, size, mimetype } = options + const { attributes, originalName, lastModified, size, mimetype } = options const path = '/api/v1/videos/upload-resumable' @@ -482,7 +485,14 @@ export class VideosCommand extends AbstractCommand { 'X-Upload-Content-Type': mimetype, 'X-Upload-Content-Length': size.toString() }, - fields: { filename: attributes.fixture, ...this.buildUploadFields(options.attributes) }, + fields: { + filename: attributes.fixture, + originalName, + lastModified, + + ...this.buildUploadFields(options.attributes) + }, + // Fixture will be sent later attaches: this.buildUploadAttaches(omit(options.attributes, 'fixture')), implicitToken: true,