Suffix external auth username on conflict

6 files changed, 81 insertions, 20 deletions

diff --git a/server/helpers/custom-validators/user-notifications.ts b/server/helpers/custom-validators/user-notifications.ts
index 252c107db..2de13ca09 100644
--- a/server/helpers/custom-validators/user-notifications.ts
+++ b/server/helpers/custom-validators/user-notifications.ts
@@ -1,5 +1,5 @@
 import validator from 'validator'
-import { UserNotificationSettingValue } from '../../../shared/models/users/user-notification-setting.model'
+import { UserNotificationSettingValue } from '@shared/models'
 import { exists } from './misc'
 
 function isUserNotificationTypeValid (value: any) {
diff --git a/server/lib/auth/oauth-model.ts b/server/lib/auth/oauth-model.ts
index d9cf32827..322b69e3a 100644
--- a/server/lib/auth/oauth-model.ts
+++ b/server/lib/auth/oauth-model.ts
@@ -1,7 +1,6 @@
 import express from 'express'
 import { AccessDeniedError } from '@node-oauth/oauth2-server'
 import { PluginManager } from '@server/lib/plugins/plugin-manager'
-import { ActorModel } from '@server/models/actor/actor'
 import { MOAuthClient } from '@server/types/models'
 import { MOAuthTokenUser } from '@server/types/models/oauth/oauth-token'
 import { MUser } from '@server/types/models/user/user'
@@ -12,6 +11,7 @@ import { CONFIG } from '../../initializers/config'
 import { OAuthClientModel } from '../../models/oauth/oauth-client'
 import { OAuthTokenModel } from '../../models/oauth/oauth-token'
 import { UserModel } from '../../models/user/user'
+import { findAvailableLocalActorName } from '../local-actor'
 import { buildUser, createUserAccountAndChannelAndPlaylist } from '../user'
 import { TokensCache } from './tokens-cache'
 
@@ -225,13 +225,12 @@ async function createUserFromExternal (pluginAuth: string, options: {
   role: UserRole
   displayName: string
 }) {
-  // Check an actor does not already exists with that name (removed user)
-  const actor = await ActorModel.loadLocalByName(options.username)
-  if (actor) return null
+  const username = await findAvailableLocalActorName(options.username)
 
   const userToCreate = buildUser({
-    ...pick(options, [ 'username', 'email', 'role' ]),
+    ...pick(options, [ 'email', 'role' ]),
 
+    username,
     emailVerified: null,
     password: null,
     pluginAuth
diff --git a/server/lib/local-actor.ts b/server/lib/local-actor.ts
index 1d9be76e2..8c10ed700 100644
--- a/server/lib/local-actor.ts
+++ b/server/lib/local-actor.ts
@@ -1,6 +1,7 @@
 import { remove } from 'fs-extra'
 import LRUCache from 'lru-cache'
 import { join } from 'path'
+import { Transaction } from 'sequelize/types'
 import { ActorModel } from '@server/models/actor/actor'
 import { getLowercaseExtension } from '@shared/core-utils'
 import { buildUUID } from '@shared/extra-utils'
@@ -87,6 +88,22 @@ async function deleteLocalActorImageFile (accountOrChannel: MAccountDefault | MC
 
 // ---------------------------------------------------------------------------
 
+async function findAvailableLocalActorName (baseActorName: string, transaction?: Transaction) {
+  let actor = await ActorModel.loadLocalByName(baseActorName, transaction)
+  if (!actor) return baseActorName
+
+  for (let i = 1; i < 30; i++) {
+    const name = `${baseActorName}-${i}`
+
+    actor = await ActorModel.loadLocalByName(name, transaction)
+    if (!actor) return name
+  }
+
+  throw new Error('Cannot find available actor local name (too much iterations).')
+}
+
+// ---------------------------------------------------------------------------
+
 function downloadActorImageFromWorker (options: {
   fileUrl: string
   filename: string
@@ -109,6 +126,7 @@ const actorImagePathUnsafeCache = new LRUCache<string, string>({ max: LRU_CACHE.
 export {
   actorImagePathUnsafeCache,
   updateLocalActorImageFiles,
+  findAvailableLocalActorName,
   downloadActorImageFromWorker,
   deleteLocalActorImageFile,
   downloadImageFromWorker,
diff --git a/server/lib/user.ts b/server/lib/user.ts
index f4ffae0e4..2e433da04 100644
--- a/server/lib/user.ts
+++ b/server/lib/user.ts
@@ -3,13 +3,11 @@ import { logger } from '@server/helpers/logger'
 import { CONFIG } from '@server/initializers/config'
 import { UserModel } from '@server/models/user/user'
 import { MActorDefault } from '@server/types/models/actor'
-import { buildUUID } from '@shared/extra-utils'
 import { ActivityPubActorType } from '../../shared/models/activitypub'
 import { UserAdminFlag, UserNotificationSetting, UserNotificationSettingValue, UserRole } from '../../shared/models/users'
 import { SERVER_ACTOR_NAME, WEBSERVER } from '../initializers/constants'
 import { sequelizeTypescript } from '../initializers/database'
 import { AccountModel } from '../models/account/account'
-import { ActorModel } from '../models/actor/actor'
 import { UserNotificationSettingModel } from '../models/user/user-notification-setting'
 import { MAccountDefault, MChannelActor } from '../types/models'
 import { MUser, MUserDefault, MUserId } from '../types/models/user'
@@ -17,7 +15,7 @@ import { generateAndSaveActorKeys } from './activitypub/actors'
 import { getLocalAccountActivityPubUrl } from './activitypub/url'
 import { Emailer } from './emailer'
 import { LiveQuotaStore } from './live/live-quota-store'
-import { buildActorInstance } from './local-actor'
+import { buildActorInstance, findAvailableLocalActorName } from './local-actor'
 import { Redis } from './redis'
 import { createLocalVideoChannel } from './video-channel'
 import { createWatchLaterPlaylist } from './video-playlist'
@@ -71,6 +69,8 @@ function buildUser (options: {
   })
 }
 
+// ---------------------------------------------------------------------------
+
 async function createUserAccountAndChannelAndPlaylist (parameters: {
   userToCreate: MUser
   userDisplayName?: string
@@ -157,6 +157,8 @@ async function createApplicationActor (applicationId: number) {
   return accountCreated
 }
 
+// ---------------------------------------------------------------------------
+
 async function sendVerifyUserEmail (user: MUser, isPendingEmail = false) {
   const verificationString = await Redis.Instance.setVerifyEmailVerificationString(user.id)
   let url = WEBSERVER.URL + '/verify-account/email?userId=' + user.id + '&verificationString=' + verificationString
@@ -169,6 +171,8 @@ async function sendVerifyUserEmail (user: MUser, isPendingEmail = false) {
   Emailer.Instance.addVerifyEmailJob(username, email, url)
 }
 
+// ---------------------------------------------------------------------------
+
 async function getOriginalVideoFileTotalFromUser (user: MUserId) {
   // Don't use sequelize because we need to use a sub query
   const query = UserModel.generateUserQuotaBaseSQL({
@@ -263,12 +267,7 @@ function createDefaultUserNotificationSettings (user: MUserId, t: Transaction |
 async function buildChannelAttributes (user: MUser, transaction?: Transaction, channelNames?: ChannelNames) {
   if (channelNames) return channelNames
 
-  let channelName = user.username + '_channel'
-
-  // Conflict, generate uuid instead
-  const actor = await ActorModel.loadLocalByName(channelName, transaction)
-  if (actor) channelName = buildUUID()
-
+  const channelName = await findAvailableLocalActorName(user.username + '_channel', transaction)
   const videoChannelDisplayName = `Main ${user.username} channel`
 
   return {
diff --git a/server/tests/fixtures/peertube-plugin-test-external-auth-two/main.js b/server/tests/fixtures/peertube-plugin-test-external-auth-two/main.js
index 1604a7c41..755dbb62b 100644
--- a/server/tests/fixtures/peertube-plugin-test-external-auth-two/main.js
+++ b/server/tests/fixtures/peertube-plugin-test-external-auth-two/main.js
@@ -65,6 +65,22 @@ async function register ({
       }
     })
   }
+
+  {
+    const result = registerExternalAuth({
+      authName: 'external-auth-7',
+      authDisplayName: () => 'External Auth 7',
+      onAuthRequest: (req, res) => {
+        result.userAuthenticated({
+          req,
+          res,
+          username: 'existing_user2',
+          email: 'custom_email_existing_user2@example.com',
+          displayName: 'Existing user 2'
+        })
+      }
+    })
+  }
 }
 
 async function unregister () {
diff --git a/server/tests/plugins/external-auth.ts b/server/tests/plugins/external-auth.ts
index 583100671..042681dbe 100644
--- a/server/tests/plugins/external-auth.ts
+++ b/server/tests/plugins/external-auth.ts
@@ -58,7 +58,14 @@ describe('Test external auth plugins', function () {
   before(async function () {
     this.timeout(30000)
 
-    server = await createSingleServer(1)
+    server = await createSingleServer(1, {
+      rates_limit: {
+        login: {
+          max: 30
+        }
+      }
+    })
+
     await setAccessTokensToServers([ server ])
 
     for (const suffix of [ 'one', 'two', 'three' ]) {
@@ -70,7 +77,7 @@ describe('Test external auth plugins', function () {
     const config = await server.config.getConfig()
 
     const auths = config.plugin.registeredExternalAuths
-    expect(auths).to.have.lengthOf(8)
+    expect(auths).to.have.lengthOf(9)
 
     const auth2 = auths.find((a) => a.authName === 'external-auth-2')
     expect(auth2).to.exist
@@ -275,7 +282,7 @@ describe('Test external auth plugins', function () {
     const config = await server.config.getConfig()
 
     const auths = config.plugin.registeredExternalAuths
-    expect(auths).to.have.lengthOf(7)
+    expect(auths).to.have.lengthOf(8)
 
     const auth1 = auths.find(a => a.authName === 'external-auth-2')
     expect(auth1).to.not.exist
@@ -318,7 +325,7 @@ describe('Test external auth plugins', function () {
     })
   })
 
-  it('Should not login an existing user', async function () {
+  it('Should not login an existing user email', async function () {
     await server.users.create({ username: 'existing_user', password: 'super_password' })
 
     await loginExternal({
@@ -330,11 +337,33 @@ describe('Test external auth plugins', function () {
     })
   })
 
+  it('Should be able to login an existing user username and channel', async function () {
+    await server.users.create({ username: 'existing_user2' })
+    await server.users.create({ username: 'existing_user2-1_channel' })
+
+    // Test twice to ensure we don't generate a username on every login
+    for (let i = 0; i < 2; i++) {
+      const res = await loginExternal({
+        server,
+        npmName: 'test-external-auth-two',
+        authName: 'external-auth-7',
+        username: 'existing_user2'
+      })
+
+      const token = res.access_token
+
+      const myInfo = await server.users.getMyInfo({ token })
+      expect(myInfo.username).to.equal('existing_user2-1')
+
+      expect(myInfo.videoChannels[0].name).to.equal('existing_user2-1_channel-1')
+    }
+  })
+
   it('Should display the correct configuration', async function () {
     const config = await server.config.getConfig()
 
     const auths = config.plugin.registeredExternalAuths
-    expect(auths).to.have.lengthOf(6)
+    expect(auths).to.have.lengthOf(7)
 
     const auth2 = auths.find((a) => a.authName === 'external-auth-2')
     expect(auth2).to.not.exist