Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(API): permissive email check in login, reset & verification #6648

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
69 changes: 68 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: '[email protected]' })
await server.users.generate('user2')

await server.registrations.requestRegistration({
username: 'request1',
registrationReason: 'tt'
})
await server.registrations.requestRegistration({
username: 'request_1',
email: '[email protected]',
registrationReason: 'tt'
})
await server.registrations.requestRegistration({
username: 'request2',
registrationReason: 'tt'
})
})

describe('When asking a password reset', function () {
Expand All @@ -50,6 +62,39 @@ 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: '[email protected]' }

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: '[email protected]' }

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: '[email protected]' }

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

it('Should success with the correct params', async function () {
const fields = { email: '[email protected]' }

Expand Down Expand Up @@ -104,7 +149,29 @@ 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: '[email protected]' }

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: '[email protected]' }

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: '[email protected]' }

await makePostBodyRequest({
Expand Down
15 changes: 14 additions & 1 deletion packages/tests/src/api/users/oauth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ describe('Test oauth', function () {
})

await setAccessTokensToServers([ server ])
await server.users.create({ username: 'user1', email: '[email protected]' })
await server.users.create({ username: 'user2', email: '[email protected]', password: 'AdvancedPassword' })

sqlCommand = new SQLCommand(server)
})
Expand Down Expand Up @@ -79,14 +81,17 @@ describe('Test oauth', function () {
})

it('Should not login with an invalid password', async function () {
const user = { username: server.store.user.username, password: 'mew_three' }
const user = { username: '[email protected]', password: 'password' }
const body = await server.login.login({ user, expectedStatus: HttpStatusCode.BAD_REQUEST_400 })

expectInvalidCredentials(body)
})

it('Should be able to login', async function () {
await server.login.login({ expectedStatus: HttpStatusCode.OK_200 })

const user = { username: '[email protected]', password: 'AdvancedPassword' }
await server.login.login({ user, expectedStatus: HttpStatusCode.OK_200 })
})

it('Should be able to login with an insensitive username', async function () {
Expand All @@ -99,6 +104,14 @@ describe('Test oauth', function () {
const user3 = { username: 'ROOt', password: server.store.user.password }
await server.login.login({ user: user3, expectedStatus: HttpStatusCode.OK_200 })
})

it('Should be able to login with an insensitive email when no similar emails exist', async function () {
const user = { username: 'ADMIN' + server.internalServerNumber + '@example.com', password: server.store.user.password }
await server.login.login({ user, expectedStatus: HttpStatusCode.OK_200 })

const user2 = { username: 'admin' + server.internalServerNumber + '@example.com', password: server.store.user.password }
await server.login.login({ user: user2, expectedStatus: HttpStatusCode.OK_200 })
})
})

describe('Logout', function () {
Expand Down
13 changes: 10 additions & 3 deletions server/core/lib/auth/oauth-model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { OAuthClientModel } from '../../models/oauth/oauth-client.js'
import { OAuthTokenModel } from '../../models/oauth/oauth-token.js'
import { UserModel } from '../../models/user/user.js'
import { findAvailableLocalActorName } from '../local-actor.js'
import { buildUser, createUserAccountAndChannelAndPlaylist } from '../user.js'
import { buildUser, createUserAccountAndChannelAndPlaylist, getUserByEmailPermissive } from '../user.js'
import { ExternalUser } from './external-auth.js'
import { TokensCache } from './tokens-cache.js'

Expand Down Expand Up @@ -87,7 +87,7 @@ async function getUser (usernameOrEmail?: string, password?: string, bypassLogin
if (bypassLogin && bypassLogin.bypass === true) {
logger.info('Bypassing oauth login by plugin %s.', bypassLogin.pluginName)

let user = await UserModel.loadByEmail(bypassLogin.user.email)
let user = getUserByEmailPermissive(await UserModel.loadByEmailCaseInsensitive(bypassLogin.user.email), bypassLogin.user.email)

if (!user) {
user = await createUserFromExternal(bypassLogin.pluginName, bypassLogin.user)
Expand Down Expand Up @@ -119,7 +119,14 @@ async function getUser (usernameOrEmail?: string, password?: string, bypassLogin

logger.debug('Getting User (username/email: ' + usernameOrEmail + ', password: ******).')

const user = await UserModel.loadByUsernameOrEmail(usernameOrEmail)
const users = await UserModel.loadByUsernameOrEmailCaseInsensitive(usernameOrEmail)
let user: MUserDefault

if (usernameOrEmail.includes('@')) {
user = getUserByEmailPermissive(users, usernameOrEmail)
} else {
user = users[0]
}

// If we don't find the user, or if the user belongs to a plugin
if (!user || user.pluginAuth !== null || !password) return null
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
26 changes: 26 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 All @@ -689,6 +701,20 @@ export class UserModel extends SequelizeModel<UserModel> {
return UserModel.findOne(query)
}

static loadByUsernameOrEmailCaseInsensitive (usernameOrEmail: string): Promise<MUserDefault[]> {
const query = {
where: {
[Op.or]: [
where(fn('lower', col('username')), fn('lower', usernameOrEmail) as any),

where(fn('lower', col('email')), fn('lower', usernameOrEmail) as any)
]
}
}

return UserModel.findAll(query)
}

static loadByVideoId (videoId: number): Promise<MUserDefault> {
const query = {
include: [
Expand Down