From 74702c85971bdf408d674575b7ebffff8b3ae979 Mon Sep 17 00:00:00 2001 From: Rigel Kent Date: Mon, 22 Jun 2020 11:20:03 +0200 Subject: [PATCH] fix video-abuse tests and check timestamp coherence --- .../video/modals/video-report.component.ts | 4 +-- server/controllers/api/videos/abuse.ts | 4 +-- .../helpers/custom-validators/video-abuses.ts | 11 +++++-- .../lib/activitypub/process/process-flag.ts | 4 +-- .../validators/videos/video-abuses.ts | 8 +++-- server/models/video/video-abuse.ts | 12 ++++---- server/tests/api/check-params/video-abuses.ts | 30 +++++++++++++++++-- server/tests/api/videos/video-abuse.ts | 15 +++++----- .../videos/abuse/video-abuse-reason.model.ts | 2 +- support/doc/api/openapi.yaml | 6 ++++ 10 files changed, 67 insertions(+), 29 deletions(-) diff --git a/client/src/app/shared/video/modals/video-report.component.ts b/client/src/app/shared/video/modals/video-report.component.ts index 0a01a4a179a5..c2d441bba294 100644 --- a/client/src/app/shared/video/modals/video-report.component.ts +++ b/client/src/app/shared/video/modals/video-report.component.ts @@ -10,7 +10,7 @@ import { VideoAbuseService } from '@app/shared/video-abuse' import { Video } from '@app/shared/video/video.model' import { buildVideoEmbed, buildVideoLink } from 'src/assets/player/utils' import { DomSanitizer, SafeHtml } from '@angular/platform-browser' -import { VideoAbusePredefinedReasonsString, VideoAbusePredefinedReasonsMap } from '@shared/models/videos/abuse/video-abuse-reason.model' +import { VideoAbusePredefinedReasonsString, videoAbusePredefinedReasonsMap } from '@shared/models/videos/abuse/video-abuse-reason.model' import { mapValues, pickBy } from 'lodash-es' @Component({ @@ -72,7 +72,7 @@ export class VideoReportComponent extends FormReactive implements OnInit { ngOnInit () { this.buildForm({ reason: this.videoAbuseValidatorsService.VIDEO_ABUSE_REASON, - predefinedReasons: mapValues(VideoAbusePredefinedReasonsMap, r => null), + predefinedReasons: mapValues(videoAbusePredefinedReasonsMap, r => null), timestamp: { hasStart: null, startAt: null, diff --git a/server/controllers/api/videos/abuse.ts b/server/controllers/api/videos/abuse.ts index 90f0f16243b6..ab207445950d 100644 --- a/server/controllers/api/videos/abuse.ts +++ b/server/controllers/api/videos/abuse.ts @@ -1,5 +1,5 @@ import * as express from 'express' -import { UserRight, VideoAbuseCreate, VideoAbuseState, VideoAbuse, VideoAbusePredefinedReasonsMap } from '../../../../shared' +import { UserRight, VideoAbuseCreate, VideoAbuseState, VideoAbuse, videoAbusePredefinedReasonsMap } from '../../../../shared' import { logger } from '../../../helpers/logger' import { getFormattedObjects } from '../../../helpers/utils' import { sequelizeTypescript } from '../../../initializers/database' @@ -124,7 +124,7 @@ async function reportVideoAbuse (req: express.Request, res: express.Response) { const videoAbuseInstance = await sequelizeTypescript.transaction(async t => { reporterAccount = await AccountModel.load(res.locals.oauth.token.User.Account.id, t) - const predefinedReasons = body.predefinedReasons?.map(r => VideoAbusePredefinedReasonsMap[r]) + const predefinedReasons = body.predefinedReasons?.map(r => videoAbusePredefinedReasonsMap[r]) const abuseToCreate = { reporterAccountId: reporterAccount.id, diff --git a/server/helpers/custom-validators/video-abuses.ts b/server/helpers/custom-validators/video-abuses.ts index 061353b9ebd6..0c2c342681ea 100644 --- a/server/helpers/custom-validators/video-abuses.ts +++ b/server/helpers/custom-validators/video-abuses.ts @@ -3,7 +3,7 @@ import validator from 'validator' import { CONSTRAINTS_FIELDS, VIDEO_ABUSE_STATES } from '../../initializers/constants' import { exists, isArray } from './misc' import { VideoAbuseVideoIs } from '@shared/models/videos/abuse/video-abuse-video-is.type' -import { VideoAbusePredefinedReasonsString, VideoAbusePredefinedReasonsMap } from '@shared/models/videos/abuse/video-abuse-reason.model' +import { VideoAbusePredefinedReasonsString, videoAbusePredefinedReasonsMap } from '@shared/models/videos/abuse/video-abuse-reason.model' const VIDEO_ABUSES_CONSTRAINTS_FIELDS = CONSTRAINTS_FIELDS.VIDEO_ABUSES @@ -12,17 +12,21 @@ function isVideoAbuseReasonValid (value: string) { } function isVideoAbusePredefinedReasonValid (value: VideoAbusePredefinedReasonsString) { - return exists(value) && value in VideoAbusePredefinedReasonsMap + return exists(value) && value in videoAbusePredefinedReasonsMap } function isVideoAbusePredefinedReasonsValid (value: VideoAbusePredefinedReasonsString[]) { - return exists(value) && isArray(value) && value.every(v => v in VideoAbusePredefinedReasonsMap) + return exists(value) && isArray(value) && value.every(v => v in videoAbusePredefinedReasonsMap) } function isVideoAbuseTimestampValid (value: number) { return value === null || (exists(value) && validator.isInt('' + value, { min: 0 })) } +function isVideoAbuseTimestampCoherent (endAt: number, { req }) { + return exists(req.body.startAt) && endAt > req.body.startAt +} + function isVideoAbuseModerationCommentValid (value: string) { return exists(value) && validator.isLength(value, VIDEO_ABUSES_CONSTRAINTS_FIELDS.MODERATION_COMMENT) } @@ -45,6 +49,7 @@ export { isVideoAbusePredefinedReasonValid, isVideoAbusePredefinedReasonsValid, isVideoAbuseTimestampValid, + isVideoAbuseTimestampCoherent, isVideoAbuseModerationCommentValid, isVideoAbuseStateValid, isAbuseVideoIsValid diff --git a/server/lib/activitypub/process/process-flag.ts b/server/lib/activitypub/process/process-flag.ts index 42a86c120dbd..1d7132a3a701 100644 --- a/server/lib/activitypub/process/process-flag.ts +++ b/server/lib/activitypub/process/process-flag.ts @@ -2,7 +2,7 @@ import { ActivityCreate, ActivityFlag, VideoAbuseState, - VideoAbusePredefinedReasonsMap + videoAbusePredefinedReasonsMap } from '../../../../shared' import { VideoAbuseObject } from '../../../../shared/models/activitypub/objects' import { retryTransactionWrapper } from '../../../helpers/database-utils' @@ -44,7 +44,7 @@ async function processCreateVideoAbuse (activity: ActivityCreate | ActivityFlag, const { video } = await getOrCreateVideoAndAccountAndChannel({ videoObject: object }) const reporterAccount = await sequelizeTypescript.transaction(async t => AccountModel.load(account.id, t)) const tags = Array.isArray(flag.tag) ? flag.tag : [] - const predefinedReasons = tags.map(tag => VideoAbusePredefinedReasonsMap[tag.name]) + const predefinedReasons = tags.map(tag => videoAbusePredefinedReasonsMap[tag.name]) .filter(v => !isNaN(v)) const startAt = flag.startAt const endAt = flag.endAt diff --git a/server/middlewares/validators/videos/video-abuses.ts b/server/middlewares/validators/videos/video-abuses.ts index 33bd4a912398..5bbd1e3c60eb 100644 --- a/server/middlewares/validators/videos/video-abuses.ts +++ b/server/middlewares/validators/videos/video-abuses.ts @@ -8,7 +8,8 @@ import { isVideoAbuseStateValid, isVideoAbusePredefinedReasonsValid, isVideoAbusePredefinedReasonValid, - isVideoAbuseTimestampValid + isVideoAbuseTimestampValid, + isVideoAbuseTimestampCoherent } from '../../../helpers/custom-validators/video-abuses' import { logger } from '../../../helpers/logger' import { doesVideoAbuseExist, doesVideoExist } from '../../../helpers/middlewares' @@ -36,7 +37,10 @@ const videoAbuseReportValidator = [ .optional() .customSanitizer(toIntOrNull) .custom(isVideoAbuseTimestampValid) - .withMessage('Should have valid ending time value'), + .withMessage('Should have valid ending time value') + .bail() + .custom(isVideoAbuseTimestampCoherent) + .withMessage('Should have a startAt timestamp beginning before endAt'), async (req: express.Request, res: express.Response, next: express.NextFunction) => { logger.debug('Checking videoAbuseReport parameters', { parameters: req.body }) diff --git a/server/models/video/video-abuse.ts b/server/models/video/video-abuse.ts index 9c6e03175922..1319332f0738 100644 --- a/server/models/video/video-abuse.ts +++ b/server/models/video/video-abuse.ts @@ -20,7 +20,7 @@ import { VideoDetails, VideoAbusePredefinedReasons, VideoAbusePredefinedReasonsString, - VideoAbusePredefinedReasonsMap + videoAbusePredefinedReasonsMap } from '../../../shared' import { VideoAbuseObject } from '../../../shared/models/activitypub/objects' import { VideoAbuse } from '../../../shared/models/videos' @@ -370,7 +370,7 @@ export class VideoAbuseModel extends Model { } = parameters const userAccountId = user ? user.Account.id : undefined - const predefinedReasonId = predefinedReason ? VideoAbusePredefinedReasonsMap[predefinedReason] : undefined + const predefinedReasonId = predefinedReason ? videoAbusePredefinedReasonsMap[predefinedReason] : undefined const query = { offset: start, @@ -405,7 +405,7 @@ export class VideoAbuseModel extends Model { } toFormattedJSON (this: MVideoAbuseFormattable): VideoAbuse { - const predefinedReasons = VideoAbuseModel.getPredefinedReasonsStrings(this.predefinedReasons || []) + const predefinedReasons = VideoAbuseModel.getPredefinedReasonsStrings(this.predefinedReasons) const countReportsForVideo = this.get('countReportsForVideo') as number const nthReportForVideo = this.get('nthReportForVideo') as number const countReportsForReporterVideo = this.get('countReportsForReporter__video') as number @@ -449,7 +449,7 @@ export class VideoAbuseModel extends Model { } toActivityPubObject (this: MVideoAbuseVideo): VideoAbuseObject { - const predefinedReasons = VideoAbuseModel.getPredefinedReasonsStrings(this.predefinedReasons || []) + const predefinedReasons = VideoAbuseModel.getPredefinedReasonsStrings(this.predefinedReasons) const startAt = this.startAt const endAt = this.endAt @@ -472,8 +472,8 @@ export class VideoAbuseModel extends Model { } private static getPredefinedReasonsStrings (predefinedReasons: VideoAbusePredefinedReasons[]): VideoAbusePredefinedReasonsString[] { - return predefinedReasons + return (predefinedReasons || []) .filter(r => r in VideoAbusePredefinedReasons) - .map(r => invert(VideoAbusePredefinedReasonsMap)[r] as VideoAbusePredefinedReasonsString) + .map(r => invert(videoAbusePredefinedReasonsMap)[r] as VideoAbusePredefinedReasonsString) } } diff --git a/server/tests/api/check-params/video-abuses.ts b/server/tests/api/check-params/video-abuses.ts index a3fe00ffbdfa..557bf20eb41f 100644 --- a/server/tests/api/check-params/video-abuses.ts +++ b/server/tests/api/check-params/video-abuses.ts @@ -20,7 +20,7 @@ import { checkBadSortPagination, checkBadStartPagination } from '../../../../shared/extra-utils/requests/check-api-params' -import { VideoAbuseState } from '../../../../shared/models/videos' +import { VideoAbuseState, VideoAbuseCreate } from '../../../../shared/models/videos' describe('Test video abuses API validators', function () { let server: ServerInfo @@ -132,12 +132,36 @@ describe('Test video abuses API validators', function () { await makePostBodyRequest({ url: server.url, path, token: server.accessToken, fields }) }) - it('Should succeed with the correct parameters', async function () { - const fields = { reason: 'super reason' } + it('Should succeed with the correct parameters (basic)', async function () { + const fields = { reason: 'my super reason' } const res = await makePostBodyRequest({ url: server.url, path, token: server.accessToken, fields, statusCodeExpected: 200 }) videoAbuseId = res.body.videoAbuse.id }) + + it('Should fail with a wrong predefined reason', async function () { + const fields = { reason: 'my super reason', predefinedReasons: [ 'wrongPredefinedReason' ] } + + await makePostBodyRequest({ url: server.url, path, token: server.accessToken, fields }) + }) + + it('Should fail with negative timestamps', async function () { + const fields = { reason: 'my super reason', startAt: -1 } + + await makePostBodyRequest({ url: server.url, path, token: server.accessToken, fields }) + }) + + it('Should fail mith misordered startAt/endAt', async function () { + const fields = { reason: 'my super reason', startAt: 5, endAt: 1 } + + await makePostBodyRequest({ url: server.url, path, token: server.accessToken, fields }) + }) + + it('Should succeed with the corret parameters (advanced)', async function () { + const fields: VideoAbuseCreate = { reason: 'my super reason', predefinedReasons: [ 'serverRules' ], startAt: 1, endAt: 5 } + + await makePostBodyRequest({ url: server.url, path, token: server.accessToken, fields, statusCodeExpected: 200 }) + }) }) describe('When updating a video abuse', function () { diff --git a/server/tests/api/videos/video-abuse.ts b/server/tests/api/videos/video-abuse.ts index d8706502548a..b04265af327c 100644 --- a/server/tests/api/videos/video-abuse.ts +++ b/server/tests/api/videos/video-abuse.ts @@ -304,17 +304,16 @@ describe('Test video abuses', function () { predefinedReasons5, 1, 5 - )).body as VideoAbuse + )).body.videoAbuse as VideoAbuse const res = await getVideoAbusesList({ url: servers[0].url, token: servers[0].accessToken }) - for (const abuse of res.body.data as VideoAbuse[]) { - if (abuse.id === createdAbuse.id) { - expect(abuse.reason).to.equals(reason5) - expect(abuse.predefinedReasons).to.equals(predefinedReasons5, "predefined reasons do not match the one reported") - expect(abuse.startAt).to.equal(1, "starting timestamp doesn't match the one reported") - expect(abuse.endAt).to.equal(5, "ending timestamp doesn't match the one reported") - } + { + const abuse = (res.body.data as VideoAbuse[]).find(a => a.id == createdAbuse.id) + expect(abuse.reason).to.equals(reason5) + expect(abuse.predefinedReasons).to.deep.equals(predefinedReasons5, "predefined reasons do not match the one reported") + expect(abuse.startAt).to.equal(1, "starting timestamp doesn't match the one reported") + expect(abuse.endAt).to.equal(5, "ending timestamp doesn't match the one reported") } }) diff --git a/shared/models/videos/abuse/video-abuse-reason.model.ts b/shared/models/videos/abuse/video-abuse-reason.model.ts index e514471b9dce..9064f0c1ae16 100644 --- a/shared/models/videos/abuse/video-abuse-reason.model.ts +++ b/shared/models/videos/abuse/video-abuse-reason.model.ts @@ -19,7 +19,7 @@ export type VideoAbusePredefinedReasonsString = 'thumbnails' | 'captions' -export const VideoAbusePredefinedReasonsMap: { +export const videoAbusePredefinedReasonsMap: { [key in VideoAbusePredefinedReasonsString]: VideoAbusePredefinedReasons } = { violentOrRepulsive: VideoAbusePredefinedReasons.VIOLENT_OR_REPULSIVE, diff --git a/support/doc/api/openapi.yaml b/support/doc/api/openapi.yaml index 35891877d605..9434af904909 100644 --- a/support/doc/api/openapi.yaml +++ b/support/doc/api/openapi.yaml @@ -1268,6 +1268,12 @@ paths: - serverRules - thumbnails - captions + startAt: + type: number + description: Timestamp in the video that marks the beginning of the report + endAt: + type: number + description: Timestamp in the video that marks the ending of the report required: - reason responses: