Skip to content

Commit

Permalink
feat(API): permissive email check in reset & verification
Browse files Browse the repository at this point in the history
In order to not force users to be case sensitive when asking for
password reset or resend email verification. When there's multiple
emails where the only difference in the local is the capitalized
letters, in those cases the users has to be case sensitive.

closes Chocobozzz#6570
  • Loading branch information
kontrollanten committed Oct 2, 2024
1 parent 639feb2 commit 697c039
Show file tree
Hide file tree
Showing 9 changed files with 111 additions and 19 deletions.
6 changes: 4 additions & 2 deletions packages/server-commands/src/users/users-command.ts
Original file line number Diff line number Diff line change
Expand Up @@ -161,14 +161,16 @@ export class UsersCommand extends AbstractCommand {
videoQuotaDaily?: number
role?: UserRoleType
adminFlags?: UserAdminFlagType
email?: string
}) {
const {
username,
adminFlags,
password = 'password',
videoQuota,
videoQuotaDaily,
role = UserRole.USER
role = UserRole.USER,
email = username + '@example.com'
} = options

const path = '/api/v1/users'
Expand All @@ -182,7 +184,7 @@ export class UsersCommand extends AbstractCommand {
password,
role,
adminFlags,
email: username + '@example.com',
email,
videoQuota,
videoQuotaDaily
},
Expand Down
59 changes: 58 additions & 1 deletion packages/tests/src/api/check-params/users-emails.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,23 @@ describe('Test users API validators', function () {
await server.config.enableSignup(true)

await server.users.generate('moderator2', UserRole.MODERATOR)
await server.users.create({ username: 'user' })
await server.users.create({ username: 'user_similar', email: 'User@example.com' })
await server.users.generate('user2')

await server.registrations.requestRegistration({
username: 'request1',
registrationReason: 'tt'
})
await server.registrations.requestRegistration({
username: 'request_1',
email: 'Request1@example.com',
registrationReason: 'tt'
})
await server.registrations.requestRegistration({
username: 'request2',
registrationReason: 'tt'
})
})

describe('When asking a password reset', function () {
Expand All @@ -50,6 +62,34 @@ describe('Test users API validators', function () {
await makePostBodyRequest({ url: server.url, path, fields })
})

it('Should fail with wrong capitalization when multiple users with similar email exists', async function () {
const fields = { email: 'USER@example.com' }

await makePostBodyRequest({ url: server.url, path, fields })
})

it('Should success with correct capitalization when multiple users with similar email exists', async function () {
const fields = { email: 'User@example.com' }

await makePostBodyRequest({
url: server.url,
path,
fields,
expectedStatus: HttpStatusCode.NO_CONTENT_204
})
})

it('Should success with wrong capitalization when no similar emails exists', async function () {
const fields = { email: 'USER2@example.com' }

await makePostBodyRequest({
url: server.url,
path,
fields,
expectedStatus: HttpStatusCode.NO_CONTENT_204
})
})

it('Should success with the correct params', async function () {
const fields = { email: 'admin@example.com' }

Expand Down Expand Up @@ -104,7 +144,24 @@ describe('Test users API validators', function () {
await makePostBodyRequest({ url: server.url, path, fields })
})

it('Should succeed with the correct params', async function () {
it('Should fail with wrong capitalization when multiple users with similar email exists', async function () {
const fields = { email: 'REQUEST1@example.com' }

await makePostBodyRequest({ url: server.url, path, fields })
})

it('Should success with wrong capitalization when no similar emails exists', async function () {
const fields = { email: 'REQUEST2@example.com' }

await makePostBodyRequest({
url: server.url,
path,
fields,
expectedStatus: HttpStatusCode.NO_CONTENT_204
})
})

it('Should success with correct capitalization when multiple users with similar email exists', async function () {
const fields = { email: 'request1@example.com' }

await makePostBodyRequest({
Expand Down
9 changes: 8 additions & 1 deletion server/core/lib/user.ts
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,12 @@ async function isUserQuotaValid (options: {
return true
}

function getUserByEmailPermissive <T extends { email: string }> (users: T[], email: string): T {
if (users.length === 1) return users[0]

return users.find(r => r.email === email)
}

// ---------------------------------------------------------------------------

export {
Expand All @@ -250,7 +256,8 @@ export {
sendVerifyRegistrationEmail,

isUserQuotaValid,
buildUser
buildUser,
getUserByEmailPermissive
}

// ---------------------------------------------------------------------------
Expand Down
9 changes: 7 additions & 2 deletions server/core/middlewares/validators/shared/users.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { forceNumber } from '@peertube/peertube-core-utils'
import { HttpStatusCode, UserRightType } from '@peertube/peertube-models'
import { getUserByEmailPermissive } from '@server/lib/user.js'
import { ActorModel } from '@server/models/actor/actor.js'
import { UserModel } from '@server/models/user/user.js'
import { MAccountId, MUserAccountId, MUserDefault } from '@server/types/models/index.js'
Expand All @@ -10,8 +11,12 @@ export function checkUserIdExist (idArg: number | string, res: express.Response,
return checkUserExist(() => UserModel.loadByIdWithChannels(id, withStats), res)
}

export function checkUserEmailExist (email: string, res: express.Response, abortResponse = true) {
return checkUserExist(() => UserModel.loadByEmail(email), res, abortResponse)
export function checkUserEmailExistPermissive (email: string, res: express.Response, abortResponse = true) {
return checkUserExist(async () => {
const users = await UserModel.loadByEmailCaseInsensitive(email)

return getUserByEmailPermissive(users, email)
}, res, abortResponse)
}

export async function checkUserNameOrEmailDoNotAlreadyExist (username: string, email: string, res: express.Response) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,19 @@ import { UserRegistrationModel } from '@server/models/user/user-registration.js'
import { MRegistration } from '@server/types/models/index.js'
import { forceNumber, pick } from '@peertube/peertube-core-utils'
import { HttpStatusCode } from '@peertube/peertube-models'
import { getUserByEmailPermissive } from '@server/lib/user.js'

function checkRegistrationIdExist (idArg: number | string, res: express.Response) {
const id = forceNumber(idArg)
return checkRegistrationExist(() => UserRegistrationModel.load(id), res)
}

function checkRegistrationEmailExist (email: string, res: express.Response, abortResponse = true) {
return checkRegistrationExist(() => UserRegistrationModel.loadByEmail(email), res, abortResponse)
function checkRegistrationEmailExistPermissive (email: string, res: express.Response, abortResponse = true) {
return checkRegistrationExist(async () => {
const registrations = await UserRegistrationModel.loadByEmailCaseInsensitive(email)

return getUserByEmailPermissive(registrations, email)
}, res, abortResponse)
}

async function checkRegistrationHandlesDoNotAlreadyExist (options: {
Expand Down Expand Up @@ -54,7 +59,7 @@ async function checkRegistrationExist (finder: () => Promise<MRegistration>, res

export {
checkRegistrationIdExist,
checkRegistrationEmailExist,
checkRegistrationEmailExistPermissive,
checkRegistrationHandlesDoNotAlreadyExist,
checkRegistrationExist
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ import { toBooleanOrNull } from '@server/helpers/custom-validators/misc.js'
import { HttpStatusCode } from '@peertube/peertube-models'
import { logger } from '../../../helpers/logger.js'
import { Redis } from '../../../lib/redis.js'
import { areValidationErrors, checkUserEmailExist, checkUserIdExist } from '../shared/index.js'
import { checkRegistrationEmailExist, checkRegistrationIdExist } from './shared/user-registrations.js'
import { areValidationErrors, checkUserEmailExistPermissive, checkUserIdExist } from '../shared/index.js'
import { checkRegistrationEmailExistPermissive, checkRegistrationIdExist } from './shared/user-registrations.js'

const usersAskSendVerifyEmailValidator = [
body('email').isEmail().not().isEmpty().withMessage('Should have a valid email'),
Expand All @@ -14,8 +14,8 @@ const usersAskSendVerifyEmailValidator = [
if (areValidationErrors(req, res)) return

const [ userExists, registrationExists ] = await Promise.all([
checkUserEmailExist(req.body.email, res, false),
checkRegistrationEmailExist(req.body.email, res, false)
checkUserEmailExistPermissive(req.body.email, res, false),
checkRegistrationEmailExistPermissive(req.body.email, res, false)
])

if (!userExists && !registrationExists) {
Expand Down
4 changes: 2 additions & 2 deletions server/core/middlewares/validators/users/users.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import { ActorModel } from '../../../models/actor/actor.js'
import {
areValidationErrors,
checkUserCanManageAccount,
checkUserEmailExist,
checkUserEmailExistPermissive,
checkUserIdExist,
checkUserNameOrEmailDoNotAlreadyExist,
doesVideoChannelIdExist,
Expand Down Expand Up @@ -334,7 +334,7 @@ export const usersAskResetPasswordValidator = [
async (req: express.Request, res: express.Response, next: express.NextFunction) => {
if (areValidationErrors(req, res)) return

const exists = await checkUserEmailExist(req.body.email, res, false)
const exists = await checkUserEmailExistPermissive(req.body.email, res, false)
if (!exists) {
logger.debug('User with email %s does not exist (asking reset password).', req.body.email)
// Do not leak our emails
Expand Down
12 changes: 8 additions & 4 deletions server/core/models/user/user-registration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { isVideoChannelDisplayNameValid } from '@server/helpers/custom-validator
import { cryptPassword } from '@server/helpers/peertube-crypto.js'
import { USER_REGISTRATION_STATES } from '@server/initializers/constants.js'
import { MRegistration, MRegistrationFormattable } from '@server/types/models/index.js'
import { FindOptions, Op, QueryTypes, WhereOptions } from 'sequelize'
import { col, FindOptions, fn, Op, QueryTypes, where, WhereOptions } from 'sequelize'
import {
AllowNull,
BeforeCreate,
Expand Down Expand Up @@ -129,12 +129,16 @@ export class UserRegistrationModel extends SequelizeModel<UserRegistrationModel>
return UserRegistrationModel.findByPk(id)
}

static loadByEmail (email: string): Promise<MRegistration> {
static loadByEmailCaseInsensitive (email: string): Promise<MRegistration[]> {
const query = {
where: { email }
where: where(
fn('LOWER', col('email')),
'=',
email.toLowerCase()
)
}

return UserRegistrationModel.findOne(query)
return UserRegistrationModel.findAll(query)
}

static loadByEmailOrUsername (emailOrUsername: string): Promise<MRegistration> {
Expand Down
12 changes: 12 additions & 0 deletions server/core/models/user/user.ts
Original file line number Diff line number Diff line change
Expand Up @@ -673,6 +673,18 @@ export class UserModel extends SequelizeModel<UserModel> {
return UserModel.findOne(query)
}

static loadByEmailCaseInsensitive (email: string): Promise<MUserDefault[]> {
const query = {
where: where(
fn('LOWER', col('email')),
'=',
email.toLowerCase()
)
}

return UserModel.findAll(query)
}

static loadByUsernameOrEmail (username: string, email?: string): Promise<MUserDefault> {
if (!email) email = username

Expand Down

0 comments on commit 697c039

Please sign in to comment.