Add rejected state to follows

Prevent reprocessing already rejected follows

10 files changed, 261 insertions, 49 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<Partial<AttributesOnly<ActorFollowMo
   }
 
   static isFollowedBy (actorId: number, followerActorId: number) {
-    const query = 'SELECT 1 FROM "actorFollow" WHERE "actorId" = $followerActorId AND "targetActorId" = $actorId LIMIT 1'
+    const query = `SELECT 1 FROM "actorFollow" ` +
+      `WHERE "actorId" = $followerActorId AND "targetActorId" = $actorId AND "state" = 'accepted' ` +
+      `LIMIT 1`
 
     return doesExist(query, { actorId, followerActorId })
   }
@@ -238,12 +240,15 @@ export class ActorFollowModel extends Model<Partial<AttributesOnly<ActorFollowMo
     return ActorFollowModel.findOne(query)
   }
 
-  static loadByActorAndTargetNameAndHostForAPI (
-    actorId: number,
-    targetName: string,
-    targetHost: string,
-    t?: Transaction
-  ): Promise<MActorFollowActorsDefaultSubscription> {
+  static loadByActorAndTargetNameAndHostForAPI (options: {
+    actorId: number
+    targetName: string
+    targetHost: string
+    state?: FollowState
+    transaction?: Transaction
+  }): Promise<MActorFollowActorsDefaultSubscription> {
+    const { actorId, targetHost, targetName, state, transaction } = options
+
     const actorFollowingPartInclude: IncludeOptions = {
       model: ActorModel,
       required: true,
@@ -271,10 +276,11 @@ export class ActorFollowModel extends Model<Partial<AttributesOnly<ActorFollowMo
       })
     }
 
-    const query = {
-      where: {
-        actorId
-      },
+    const where: WhereAttributeHash<Attributes<ActorFollowModel>> = { actorId}
+    if (state) where.state = state
+
+    const query: FindOptions<Attributes<ActorFollowModel>> = {
+      where,
       include: [
         actorFollowingPartInclude,
         {
@@ -283,7 +289,7 @@ export class ActorFollowModel extends Model<Partial<AttributesOnly<ActorFollowMo
           as: 'ActorFollower'
         }
       ],
-      transaction: t
+      transaction
     }
 
     return ActorFollowModel.findOne(query)
@@ -325,6 +331,7 @@ export class ActorFollowModel extends Model<Partial<AttributesOnly<ActorFollowMo
             [Op.or]: whereTab
           },
           {
+            state: 'accepted',
             actorId
           }
         ]
@@ -372,6 +379,7 @@ export class ActorFollowModel extends Model<Partial<AttributesOnly<ActorFollowMo
   }) {
     const { actorId, start, count, sort } = options
     const where = {
+      state: 'accepted',
       actorId
     }
 
@@ -512,13 +520,15 @@ export class ActorFollowModel extends Model<Partial<AttributesOnly<ActorFollowMo
 
     const totalInstanceFollowing = await ActorFollowModel.count({
       where: {
-        actorId: serverActor.id
+        actorId: serverActor.id,
+        state: 'accepted'
       }
     })
 
     const totalInstanceFollowers = await ActorFollowModel.count({
       where: {
-        targetActorId: serverActor.id
+        targetActorId: serverActor.id,
+        state: 'accepted'
       }
     })
 
diff --git a/server/models/actor/actor.ts b/server/models/actor/actor.ts
index 943b7364f..7be5a140c 100644
--- a/server/models/actor/actor.ts
+++ b/server/models/actor/actor.ts
@@ -462,7 +462,7 @@ export class ActorModel extends Model<Partial<AttributesOnly<ActorModel>>> {
     }
 
     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)
   })