From 927fa4b11f692174d6296aa096d7a74bacdeea8b Mon Sep 17 00:00:00 2001 From: Chocobozzz Date: Tue, 26 Jul 2022 14:46:15 +0200 Subject: [PATCH] Add rejected state to follows Prevent reprocessing already rejected follows --- server/controllers/api/server/follows.ts | 34 +++- server/helpers/custom-validators/follows.ts | 2 +- server/initializers/constants.ts | 3 +- .../lib/activitypub/process/process-follow.ts | 5 + .../lib/activitypub/process/process-reject.ts | 3 +- server/middlewares/validators/follows.ts | 30 ++- .../validators/user-subscriptions.ts | 7 +- server/models/actor/actor-follow.ts | 40 ++-- server/models/actor/actor.ts | 2 +- server/tests/api/server/follows-moderation.ts | 184 ++++++++++++++++-- shared/models/actors/follow.model.ts | 2 +- .../server-commands/server/follows-command.ts | 8 +- 12 files changed, 266 insertions(+), 54 deletions(-) diff --git a/server/controllers/api/server/follows.ts b/server/controllers/api/server/follows.ts index 37e8d88f7..60d36ed59 100644 --- a/server/controllers/api/server/follows.ts +++ b/server/controllers/api/server/follows.ts @@ -1,5 +1,6 @@ import express from 'express' import { getServerActor } from '@server/models/application/application' +import { ServerFollowCreate } from '@shared/models' import { HttpStatusCode } from '../../../../shared/models/http/http-error-codes' import { UserRight } from '../../../../shared/models/users' import { logger } from '../../../helpers/logger' @@ -20,16 +21,16 @@ import { setDefaultSort } from '../../../middlewares' import { - acceptOrRejectFollowerValidator, - instanceFollowersSortValidator, - instanceFollowingSortValidator, + acceptFollowerValidator, followValidator, getFollowerValidator, + instanceFollowersSortValidator, + instanceFollowingSortValidator, listFollowsValidator, + rejectFollowerValidator, removeFollowingValidator } from '../../../middlewares/validators' import { ActorFollowModel } from '../../../models/actor/actor-follow' -import { ServerFollowCreate } from '@shared/models' const serverFollowsRouter = express.Router() serverFollowsRouter.get('/following', @@ -69,22 +70,22 @@ serverFollowsRouter.delete('/followers/:nameWithHost', authenticate, ensureUserHasRight(UserRight.MANAGE_SERVER_FOLLOW), asyncMiddleware(getFollowerValidator), - asyncMiddleware(removeOrRejectFollower) + asyncMiddleware(removeFollower) ) serverFollowsRouter.post('/followers/:nameWithHost/reject', authenticate, ensureUserHasRight(UserRight.MANAGE_SERVER_FOLLOW), asyncMiddleware(getFollowerValidator), - acceptOrRejectFollowerValidator, - asyncMiddleware(removeOrRejectFollower) + rejectFollowerValidator, + asyncMiddleware(rejectFollower) ) serverFollowsRouter.post('/followers/:nameWithHost/accept', authenticate, ensureUserHasRight(UserRight.MANAGE_SERVER_FOLLOW), asyncMiddleware(getFollowerValidator), - acceptOrRejectFollowerValidator, + acceptFollowerValidator, asyncMiddleware(acceptFollower) ) @@ -176,10 +177,23 @@ async function removeFollowing (req: express.Request, res: express.Response) { return res.status(HttpStatusCode.NO_CONTENT_204).end() } -async function removeOrRejectFollower (req: express.Request, res: express.Response) { +async function rejectFollower (req: express.Request, res: express.Response) { const follow = res.locals.follow - await sendReject(follow.url, follow.ActorFollower, follow.ActorFollowing) + follow.state = 'rejected' + await follow.save() + + sendReject(follow.url, follow.ActorFollower, follow.ActorFollowing) + + return res.status(HttpStatusCode.NO_CONTENT_204).end() +} + +async function removeFollower (req: express.Request, res: express.Response) { + const follow = res.locals.follow + + if (follow.state === 'accepted' || follow.state === 'pending') { + sendReject(follow.url, follow.ActorFollower, follow.ActorFollowing) + } await follow.destroy() diff --git a/server/helpers/custom-validators/follows.ts b/server/helpers/custom-validators/follows.ts index 8f65552c3..0bec683c1 100644 --- a/server/helpers/custom-validators/follows.ts +++ b/server/helpers/custom-validators/follows.ts @@ -4,7 +4,7 @@ import { FollowState } from '@shared/models' function isFollowStateValid (value: FollowState) { if (!exists(value)) return false - return value === 'pending' || value === 'accepted' + return value === 'pending' || value === 'accepted' || value === 'rejected' } function isRemoteHandleValid (value: string) { diff --git a/server/initializers/constants.ts b/server/initializers/constants.ts index e3f7ceb4a..99ae64f8d 100644 --- a/server/initializers/constants.ts +++ b/server/initializers/constants.ts @@ -129,7 +129,8 @@ const ACTOR_FOLLOW_SCORE = { const FOLLOW_STATES: { [ id: string ]: FollowState } = { PENDING: 'pending', - ACCEPTED: 'accepted' + ACCEPTED: 'accepted', + REJECTED: 'rejected' } const REMOTE_SCHEME = { diff --git a/server/lib/activitypub/process/process-follow.ts b/server/lib/activitypub/process/process-follow.ts index 93df7e191..a1958f464 100644 --- a/server/lib/activitypub/process/process-follow.ts +++ b/server/lib/activitypub/process/process-follow.ts @@ -58,6 +58,11 @@ async function processFollow (byActor: MActorSignature, activityId: string, targ transaction: t }) + // Already rejected + if (actorFollow.state === 'rejected') { + return { actorFollow: undefined as MActorFollowActors } + } + // Set the follow as accepted if the remote actor follows a channel or account // Or if the instance automatically accepts followers if (actorFollow.state !== 'accepted' && (isFollowingInstance === false || CONFIG.FOLLOWERS.INSTANCE.MANUAL_APPROVAL === false)) { diff --git a/server/lib/activitypub/process/process-reject.ts b/server/lib/activitypub/process/process-reject.ts index 7f7ab305f..db7ff24d8 100644 --- a/server/lib/activitypub/process/process-reject.ts +++ b/server/lib/activitypub/process/process-reject.ts @@ -25,7 +25,8 @@ async function processReject (follower: MActor, targetActor: MActor) { if (!actorFollow) throw new Error(`'Unknown actor follow ${follower.id} -> ${targetActor.id}.`) - await actorFollow.destroy({ transaction: t }) + actorFollow.state = 'rejected' + await actorFollow.save({ transaction: t }) return undefined }) diff --git a/server/middlewares/validators/follows.ts b/server/middlewares/validators/follows.ts index 86d2d6228..023dba5b5 100644 --- a/server/middlewares/validators/follows.ts +++ b/server/middlewares/validators/follows.ts @@ -81,7 +81,11 @@ const removeFollowingValidator = [ const serverActor = await getServerActor() const { name, host } = getRemoteNameAndHost(req.params.hostOrHandle) - const follow = await ActorFollowModel.loadByActorAndTargetNameAndHostForAPI(serverActor.id, name, host) + const follow = await ActorFollowModel.loadByActorAndTargetNameAndHostForAPI({ + actorId: serverActor.id, + targetName: name, + targetHost: host + }) if (!follow) { return res.fail({ @@ -126,13 +130,26 @@ const getFollowerValidator = [ } ] -const acceptOrRejectFollowerValidator = [ +const acceptFollowerValidator = [ (req: express.Request, res: express.Response, next: express.NextFunction) => { - logger.debug('Checking accept/reject follower parameters', { parameters: req.params }) + logger.debug('Checking accept follower parameters', { parameters: req.params }) const follow = res.locals.follow - if (follow.state !== 'pending') { - return res.fail({ message: 'Follow is not in pending state.' }) + if (follow.state !== 'pending' && follow.state !== 'rejected') { + return res.fail({ message: 'Follow is not in pending/rejected state.' }) + } + + return next() + } +] + +const rejectFollowerValidator = [ + (req: express.Request, res: express.Response, next: express.NextFunction) => { + logger.debug('Checking reject follower parameters', { parameters: req.params }) + + const follow = res.locals.follow + if (follow.state !== 'pending' && follow.state !== 'accepted') { + return res.fail({ message: 'Follow is not in pending/accepted state.' }) } return next() @@ -145,6 +162,7 @@ export { followValidator, removeFollowingValidator, getFollowerValidator, - acceptOrRejectFollowerValidator, + acceptFollowerValidator, + rejectFollowerValidator, listFollowsValidator } diff --git a/server/middlewares/validators/user-subscriptions.ts b/server/middlewares/validators/user-subscriptions.ts index 48ce90d7b..73da3142a 100644 --- a/server/middlewares/validators/user-subscriptions.ts +++ b/server/middlewares/validators/user-subscriptions.ts @@ -58,7 +58,12 @@ const userSubscriptionGetValidator = [ if (host === WEBSERVER.HOST) host = null const user = res.locals.oauth.token.User - const subscription = await ActorFollowModel.loadByActorAndTargetNameAndHostForAPI(user.Account.Actor.id, name, host) + const subscription = await ActorFollowModel.loadByActorAndTargetNameAndHostForAPI({ + actorId: user.Account.Actor.id, + targetName: name, + targetHost: host, + state: 'accepted' + }) if (!subscription || !subscription.ActorFollowing.VideoChannel) { return res.fail({ diff --git a/server/models/actor/actor-follow.ts b/server/models/actor/actor-follow.ts index 8870bec05..566bb5f31 100644 --- a/server/models/actor/actor-follow.ts +++ b/server/models/actor/actor-follow.ts @@ -1,5 +1,5 @@ import { difference, values } from 'lodash' -import { Includeable, IncludeOptions, Op, QueryTypes, Transaction } from 'sequelize' +import { Attributes, FindOptions, Includeable, IncludeOptions, Op, QueryTypes, Transaction, WhereAttributeHash } from 'sequelize' import { AfterCreate, AfterDestroy, @@ -209,7 +209,9 @@ export class ActorFollowModel extends Model { + static loadByActorAndTargetNameAndHostForAPI (options: { + actorId: number + targetName: string + targetHost: string + state?: FollowState + transaction?: Transaction + }): Promise { + const { actorId, targetHost, targetName, state, transaction } = options + const actorFollowingPartInclude: IncludeOptions = { model: ActorModel, required: true, @@ -271,10 +276,11 @@ export class ActorFollowModel extends Model> = { actorId} + if (state) where.state = state + + const query: FindOptions> = { + where, include: [ actorFollowingPartInclude, { @@ -283,7 +289,7 @@ export class ActorFollowModel extends Model>> { } return ActorModel.update({ - [columnToUpdate]: literal(`(SELECT COUNT(*) FROM "actorFollow" WHERE "${columnOfCount}" = ${sanitizedOfId})`) + [columnToUpdate]: literal(`(SELECT COUNT(*) FROM "actorFollow" WHERE "${columnOfCount}" = ${sanitizedOfId} AND "state" = 'accepted')`) }, { where, transaction }) } diff --git a/server/tests/api/server/follows-moderation.ts b/server/tests/api/server/follows-moderation.ts index 120bd7f88..a0e94c10e 100644 --- a/server/tests/api/server/follows-moderation.ts +++ b/server/tests/api/server/follows-moderation.ts @@ -2,6 +2,8 @@ import 'mocha' import * as chai from 'chai' +import { expectStartWith } from '@server/tests/shared' +import { ActorFollow, FollowState } from '@shared/models' import { cleanupTests, createMultipleServers, @@ -25,8 +27,51 @@ async function checkServer1And2HasFollowers (servers: PeerTubeServer[], state = const follow = body.data[0] expect(follow.state).to.equal(state) - expect(follow.follower.url).to.equal('http://localhost:' + servers[0].port + '/accounts/peertube') - expect(follow.following.url).to.equal('http://localhost:' + servers[1].port + '/accounts/peertube') + expect(follow.follower.url).to.equal(servers[0].url + '/accounts/peertube') + expect(follow.following.url).to.equal(servers[1].url + '/accounts/peertube') + } +} + +async function checkFollows (options: { + follower: { + server: PeerTubeServer + state?: FollowState // if not provided, it means it does not exist + } + following: { + server: PeerTubeServer + state?: FollowState // if not provided, it means it does not exist + } +}) { + const { follower, following } = options + + const followerUrl = follower.server.url + '/accounts/peertube' + const followingUrl = following.server.url + '/accounts/peertube' + const finder = (d: ActorFollow) => d.follower.url === followerUrl && d.following.url === followingUrl + + { + const { data } = await follower.server.follows.getFollowings() + const follow = data.find(finder) + + if (!follower.state) { + expect(follow).to.not.exist + } else { + expect(follow.state).to.equal(follower.state) + expect(follow.follower.url).to.equal(followerUrl) + expect(follow.following.url).to.equal(followingUrl) + } + } + + { + const { data } = await following.server.follows.getFollowers() + const follow = data.find(finder) + + if (!following.state) { + expect(follow).to.not.exist + } else { + expect(follow.state).to.equal(following.state) + expect(follow.follower.url).to.equal(followerUrl) + expect(follow.following.url).to.equal(followingUrl) + } } } @@ -37,7 +82,7 @@ async function checkNoFollowers (servers: PeerTubeServer[]) { ] for (const fn of fns) { - const body = await fn({ start: 0, count: 5, sort: 'createdAt' }) + const body = await fn({ start: 0, count: 5, sort: 'createdAt', state: 'accepted' }) expect(body.total).to.equal(0) } } @@ -124,7 +169,7 @@ describe('Test follows moderation', function () { it('Should manually approve followers', async function () { this.timeout(20000) - await commands[1].removeFollower({ follower: servers[0] }) + await commands[0].unfollow({ target: servers[1] }) await waitJobs(servers) const subConfig = { @@ -148,7 +193,7 @@ describe('Test follows moderation', function () { it('Should accept a follower', async function () { this.timeout(10000) - await commands[1].acceptFollower({ follower: 'peertube@localhost:' + servers[0].port }) + await commands[1].acceptFollower({ follower: 'peertube@' + servers[0].host }) await waitJobs(servers) await checkServer1And2HasFollowers(servers) @@ -161,31 +206,144 @@ describe('Test follows moderation', function () { await waitJobs(servers) { - const body = await commands[0].getFollowings({ start: 0, count: 5, sort: 'createdAt' }) + const body = await commands[0].getFollowings() expect(body.total).to.equal(2) } { - const body = await commands[1].getFollowers({ start: 0, count: 5, sort: 'createdAt' }) + const body = await commands[1].getFollowers() expect(body.total).to.equal(1) } { - const body = await commands[2].getFollowers({ start: 0, count: 5, sort: 'createdAt' }) + const body = await commands[2].getFollowers() expect(body.total).to.equal(1) } - await commands[2].rejectFollower({ follower: 'peertube@localhost:' + servers[0].port }) + await commands[2].rejectFollower({ follower: 'peertube@' + servers[0].host }) await waitJobs(servers) - await checkServer1And2HasFollowers(servers) + { // server 1 + { + const { data } = await commands[0].getFollowings({ state: 'accepted' }) + expect(data).to.have.lengthOf(1) + } - { - const body = await commands[2].getFollowers({ start: 0, count: 5, sort: 'createdAt' }) - expect(body.total).to.equal(0) + { + const { data } = await commands[0].getFollowings({ state: 'rejected' }) + expect(data).to.have.lengthOf(1) + expectStartWith(data[0].following.url, servers[2].url) + } + } + + { // server 3 + { + const { data } = await commands[2].getFollowers({ state: 'accepted' }) + expect(data).to.have.lengthOf(0) + } + + { + const { data } = await commands[2].getFollowers({ state: 'rejected' }) + expect(data).to.have.lengthOf(1) + expectStartWith(data[0].follower.url, servers[0].url) + } } }) + it('Should not change the follow on refollow with and without auto accept', async function () { + const run = async () => { + await commands[0].follow({ hosts: [ servers[2].url ] }) + await waitJobs(servers) + + await checkFollows({ + follower: { + server: servers[0], + state: 'rejected' + }, + following: { + server: servers[2], + state: 'rejected' + } + }) + } + + await servers[2].config.updateExistingSubConfig({ newConfig: { followers: { instance: { manualApproval: false } } } }) + await run() + + await servers[2].config.updateExistingSubConfig({ newConfig: { followers: { instance: { manualApproval: true } } } }) + await run() + }) + + it('Should not change the rejected status on unfollow', async function () { + await commands[0].unfollow({ target: servers[2] }) + await waitJobs(servers) + + await checkFollows({ + follower: { + server: servers[0] + }, + following: { + server: servers[2], + state: 'rejected' + } + }) + }) + + it('Should delete the follower and add again the follower', async function () { + await commands[2].removeFollower({ follower: servers[0] }) + await waitJobs(servers) + + await commands[0].follow({ hosts: [ servers[2].url ] }) + await waitJobs(servers) + + await checkFollows({ + follower: { + server: servers[0], + state: 'pending' + }, + following: { + server: servers[2], + state: 'pending' + } + }) + }) + + it('Should be able to reject a previously accepted follower', async function () { + await commands[1].rejectFollower({ follower: 'peertube@' + servers[0].host }) + await waitJobs(servers) + + await checkFollows({ + follower: { + server: servers[0], + state: 'rejected' + }, + following: { + server: servers[1], + state: 'rejected' + } + }) + }) + + it('Should be able to re accept a previously rejected follower', async function () { + await commands[1].acceptFollower({ follower: 'peertube@' + servers[0].host }) + await waitJobs(servers) + + await checkFollows({ + follower: { + server: servers[0], + state: 'accepted' + }, + following: { + server: servers[1], + state: 'accepted' + } + }) + }) + + it('Should ignore follow requests of muted servers', async function () { + + }) + after(async function () { await cleanupTests(servers) }) diff --git a/shared/models/actors/follow.model.ts b/shared/models/actors/follow.model.ts index 7de638cba..244d6d97e 100644 --- a/shared/models/actors/follow.model.ts +++ b/shared/models/actors/follow.model.ts @@ -1,6 +1,6 @@ import { Actor } from './actor.model' -export type FollowState = 'pending' | 'accepted' +export type FollowState = 'pending' | 'accepted' | 'rejected' export interface ActorFollow { id: number diff --git a/shared/server-commands/server/follows-command.ts b/shared/server-commands/server/follows-command.ts index 01ef6f179..496e11df1 100644 --- a/shared/server-commands/server/follows-command.ts +++ b/shared/server-commands/server/follows-command.ts @@ -6,13 +6,13 @@ import { PeerTubeServer } from './server' export class FollowsCommand extends AbstractCommand { getFollowers (options: OverrideCommandOptions & { - start: number - count: number - sort: string + start?: number + count?: number + sort?: string search?: string actorType?: ActivityPubActorType state?: FollowState - }) { + } = {}) { const path = '/api/v1/server/followers' const query = pick(options, [ 'start', 'count', 'sort', 'search', 'state', 'actorType' ]) -- 2.41.0