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 4 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
47 changes: 46 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,28 @@ describe('Test users API validators', function () {
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: '[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 +138,18 @@ describe('Test users API validators', function () {
await makePostBodyRequest({ url: server.url, path, fields })
})

it('Should succeed with the correct params', async function () {
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
25 changes: 25 additions & 0 deletions packages/tests/src/api/server/email.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,16 @@ describe('Test emails', function () {
username: 'user_1',
password: 'super_password'
}
const similarUsers = [
{
username: 'lowercase_user_1',
email: '[email protected]'
},
{
username: 'lowercase_user__1',
email: '[email protected]'
}
]

before(async function () {
this.timeout(120000)
Expand All @@ -41,6 +51,10 @@ describe('Test emails', function () {
await setAccessTokensToServers([ server ])
await server.config.enableSignup(true)

for (const user of similarUsers) {
await server.users.create(user)
}

{
const created = await server.users.create({ username: user.username, password: user.password })
userId = created.id
Expand Down Expand Up @@ -101,6 +115,10 @@ describe('Test emails', function () {
})
})

it('Should fail with wrong capitalization when multiple users with similar email exists', async function () {
await server.users.askResetPassword({ email: similarUsers[0].username.toUpperCase(), expectedStatus: HttpStatusCode.BAD_REQUEST_400 })
})

it('Should reset the password', async function () {
await server.users.resetPassword({ userId, verificationString, password: 'super_password2' })
})
Expand Down Expand Up @@ -269,6 +287,13 @@ describe('Test emails', function () {

describe('When verifying a user email', function () {

it('Should fail with wrong capitalization when multiple users with similar email exists', async function () {
await server.users.askSendVerifyEmail({
email: similarUsers[0].username.toUpperCase(),
expectedStatus: HttpStatusCode.BAD_REQUEST_400
})
})

it('Should ask to send the verification email', async function () {
await server.users.askSendVerifyEmail({ email: '[email protected]' })

Expand Down
21 changes: 21 additions & 0 deletions 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 @@ -87,6 +89,9 @@ describe('Test oauth', function () {

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,22 @@ 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 () {
kontrollanten marked this conversation as resolved.
Show resolved Hide resolved
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 })
})

it('Should not be able to login with an insensitive email when similar emails exist', async function () {
const user = { username: '[email protected]', password: 'AdvancedPassword' }
await server.login.login({ user, expectedStatus: HttpStatusCode.BAD_REQUEST_400 })

const user2 = { username: '[email protected]', password: 'AdvancedPassword' }
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 if (users.length === 1) {
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
14 changes: 10 additions & 4 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,14 +11,19 @@ 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) {
const user = await UserModel.loadByUsernameOrEmail(username, email)
const existingUser = await UserModel.loadByUsernameOrEmailCaseInsensitive(username)
const existingEmail = await UserModel.loadByUsernameOrEmailCaseInsensitive(email)

if (user) {
if (existingUser.length > 0 || existingEmail.length > 0) {
res.fail({
status: HttpStatusCode.CONFLICT_409,
message: 'User with this username or email already exists.'
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'
import { Hooks } from '@server/lib/plugins/hooks.js'

const usersAskSendVerifyEmailValidator = [
Expand All @@ -19,8 +19,8 @@ const usersAskSendVerifyEmailValidator = [
}, 'filter:api.email-verification.ask-send-verify-email.body')

const [ userExists, registrationExists ] = await Promise.all([
checkUserEmailExist(email, res, false),
checkRegistrationEmailExist(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 @@ -339,7 +339,7 @@ export const usersAskResetPasswordValidator = [
email: req.body.email
}, 'filter:api.users.ask-reset-password.body')

const exists = await checkUserEmailExist(email, res, false)
const exists = await checkUserEmailExistPermissive(email, res, false)
if (!exists) {
logger.debug('User with email %s does not exist (asking reset password).', 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
Loading
Loading