From 0b6f531653a7a24f82ad65564479a70a9326301a Mon Sep 17 00:00:00 2001 From: Chocobozzz Date: Wed, 3 Aug 2022 10:10:26 +0200 Subject: [PATCH] Suffix external auth username on conflict --- .../custom-validators/user-notifications.ts | 2 +- server/lib/auth/oauth-model.ts | 9 ++--- server/lib/local-actor.ts | 18 +++++++++ server/lib/user.ts | 17 ++++---- .../main.js | 16 ++++++++ server/tests/plugins/external-auth.ts | 39 ++++++++++++++++--- 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({ 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