From bd9f3de5af728adf7f933822e40aa38a9407f280 Mon Sep 17 00:00:00 2001 From: kontrollanten <6680299+kontrollanten@users.noreply.github.com> Date: Mon, 20 Jan 2025 19:25:59 +0100 Subject: [PATCH] code review changes --- .../src/api/check-params/users-emails.ts | 22 ---------------- packages/tests/src/api/server/email.ts | 25 ++++++++++++++++++ packages/tests/src/api/users/oauth.ts | 10 ++++++- server/core/lib/auth/oauth-model.ts | 2 +- .../middlewares/validators/shared/users.ts | 5 ++-- server/core/models/user/user.ts | 26 ------------------- 6 files changed, 38 insertions(+), 52 deletions(-) diff --git a/packages/tests/src/api/check-params/users-emails.ts b/packages/tests/src/api/check-params/users-emails.ts index 2796dda5f..9cc305658 100644 --- a/packages/tests/src/api/check-params/users-emails.ts +++ b/packages/tests/src/api/check-params/users-emails.ts @@ -62,17 +62,6 @@ 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, - expectedStatus: HttpStatusCode.NO_CONTENT_204 - }) - }) - it('Should success with correct capitalization when multiple users with similar email exists', async function () { const fields = { email: 'User@example.com' } @@ -149,17 +138,6 @@ 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: 'REQUEST1@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: 'REQUEST2@example.com' } diff --git a/packages/tests/src/api/server/email.ts b/packages/tests/src/api/server/email.ts index 6d3f3f3bb..74c68dfb9 100644 --- a/packages/tests/src/api/server/email.ts +++ b/packages/tests/src/api/server/email.ts @@ -31,6 +31,16 @@ describe('Test emails', function () { username: 'user_1', password: 'super_password' } + const similarUsers = [ + { + username: 'lowercase_user_1', + email: 'lowercase_user_1@example.com' + }, + { + username: 'lowercase_user__1', + email: 'Lowercase_user_1@example.com' + } + ] before(async function () { this.timeout(120000) @@ -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 @@ -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' }) }) @@ -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: 'user_1@example.com' }) diff --git a/packages/tests/src/api/users/oauth.ts b/packages/tests/src/api/users/oauth.ts index a69f51001..2dd79dbf7 100644 --- a/packages/tests/src/api/users/oauth.ts +++ b/packages/tests/src/api/users/oauth.ts @@ -81,7 +81,7 @@ describe('Test oauth', function () { }) it('Should not login with an invalid password', async function () { - const user = { username: 'User@example.com', password: 'password' } + const user = { username: server.store.user.username, password: 'mew_three' } const body = await server.login.login({ user, expectedStatus: HttpStatusCode.BAD_REQUEST_400 }) expectInvalidCredentials(body) @@ -112,6 +112,14 @@ describe('Test oauth', function () { 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: 'uSer@example.com', password: 'AdvancedPassword' } + await server.login.login({ user, expectedStatus: HttpStatusCode.BAD_REQUEST_400 }) + + const user2 = { username: 'User@example.com', password: 'AdvancedPassword' } + await server.login.login({ user: user2, expectedStatus: HttpStatusCode.OK_200 }) + }) }) describe('Logout', function () { diff --git a/server/core/lib/auth/oauth-model.ts b/server/core/lib/auth/oauth-model.ts index ce1ba6fb4..a1236764c 100644 --- a/server/core/lib/auth/oauth-model.ts +++ b/server/core/lib/auth/oauth-model.ts @@ -124,7 +124,7 @@ async function getUser (usernameOrEmail?: string, password?: string, bypassLogin if (usernameOrEmail.includes('@')) { user = getUserByEmailPermissive(users, usernameOrEmail) - } else { + } else if (users.length === 1) { user = users[0] } diff --git a/server/core/middlewares/validators/shared/users.ts b/server/core/middlewares/validators/shared/users.ts index d8f9dd945..760b7c701 100644 --- a/server/core/middlewares/validators/shared/users.ts +++ b/server/core/middlewares/validators/shared/users.ts @@ -20,9 +20,10 @@ export function checkUserEmailExistPermissive (email: string, res: express.Respo } 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.' diff --git a/server/core/models/user/user.ts b/server/core/models/user/user.ts index 4d23f800e..40e668662 100644 --- a/server/core/models/user/user.ts +++ b/server/core/models/user/user.ts @@ -663,16 +663,6 @@ export class UserModel extends SequelizeModel { return UserModel.scope(ScopeNames.FOR_ME_API).findOne(query) } - static loadByEmail (email: string): Promise { - const query = { - where: { - email - } - } - - return UserModel.findOne(query) - } - static loadByEmailCaseInsensitive (email: string): Promise { const query = { where: where( @@ -685,22 +675,6 @@ export class UserModel extends SequelizeModel { return UserModel.findAll(query) } - static loadByUsernameOrEmail (username: string, email?: string): Promise { - if (!email) email = username - - const query = { - where: { - [Op.or]: [ - where(fn('lower', col('username')), fn('lower', username) as any), - - { email } - ] - } - } - - return UserModel.findOne(query) - } - static loadByUsernameOrEmailCaseInsensitive (usernameOrEmail: string): Promise { const query = { where: {