huanld/speckle-server
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

fix(server): fixed various email lookups/updates being case sensitive (#2595)

Browse Source 
* fix(server): case insensitivity in userEmails repo

* minor fix

* more test improvements + user repo tests

* more tests

* test fixes
This commit is contained in:
Kristaps Fabians Geikins
2024-08-07 17:48:01 +03:00
committed by GitHub
parent b03e79ae44
commit 0085bab1db
12 changed files with 290 additions and 79 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/server/modules/auth/tests/integration/registration.spec.ts
+1 -1
View File
@@ -101,7 +101,7 @@ describe('Server registration', () => {
itEach(
<const>[
{ key: 'email', msg: 'E-mail address is empty' },
{ key: 'email', msg: 'E-mail address is required' },
{ key: 'name', msg: 'User name is required' },
{ key: 'password', msg: 'Password missing' }
],
packages/server/modules/core/domain/userEmails/operations.ts
+6 -5
View File
@@ -1,11 +1,12 @@
import { UserEmail } from '@/modules/core/domain/userEmails/types'
import { Optional } from '@speckle/shared'
import { Knex } from 'knex'
export type CreateUserEmail = ({
userEmail
}: {
userEmail: Pick<UserEmail, 'email' | 'userId'> & { primary?: boolean }
}) => Promise<string>
}) => Promise<UserEmail>
export type UpdateUserEmail = (
{
@@ -19,7 +20,7 @@ export type UpdateUserEmail = (
update: Pick<Partial<UserEmail>, 'email' | 'primary' | 'verified'>
},
options?: { trx: Knex.Transaction }
) => Promise<UserEmail>
) => Promise<Optional<UserEmail>>
export type DeleteUserEmail = (
userEmail: Pick<UserEmail, 'id' | 'userId'>
@@ -29,7 +30,7 @@ export type MarkUserEmailAsVerified = ({
email
}: {
email: string
}) => Promise<UserEmail>
}) => Promise<Optional<UserEmail>>
export type FindPrimaryEmailForUser = (
query:
@@ -37,9 +38,9 @@ export type FindPrimaryEmailForUser = (
userId: string
}
| { email: string }
) => Promise<UserEmail>
) => Promise<Optional<UserEmail>>
export type FindEmail = (query: Partial<UserEmail>) => Promise<UserEmail>
export type FindEmail = (query: Partial<UserEmail>) => Promise<Optional<UserEmail>>
export type FindEmailsByUserId = ({
userId
packages/server/modules/core/helpers/testHelpers.ts
+7 -1
View File
@@ -1,9 +1,15 @@
import crs from 'crypto-random-string'
export function createRandomEmail() {
return `${crs({ length: 6 })}@example.org`
return randomizeCase(`${crs({ length: 6 })}@example.org`)
}
export function createRandomPassword(length?: number) {
return crs({ length: length ?? 10 })
}
export const randomizeCase = (str: string) =>
str
.split('')
.map((char) => (Math.random() > 0.5 ? char.toUpperCase() : char.toLowerCase()))
.join('')
packages/server/modules/core/repositories/userEmails.ts
+49 -16
View File
@@ -16,6 +16,11 @@ import {
UserEmailDeleteError,
UserEmailPrimaryAlreadyExistsError
} from '@/modules/core/errors/userEmails'
import { get, omit } from 'lodash'
const whereEmailIs = (query: Knex.QueryBuilder, email: string) => {
query.whereRaw('lower("email") = lower(?)', [email])
}
const checkPrimaryEmail =
({ db }: { db: Knex }) =>
@@ -36,14 +41,17 @@ export const createUserEmailFactory =
await checkPrimaryEmail({ db })(rest)
}
await db(UserEmails.name).insert({
id,
primary: true,
email: email.toLowerCase().trim(),
...rest
})
const [row] = await db<UserEmail>(UserEmails.name).insert(
{
id,
primary: true,
email: email.toLowerCase().trim(),
...rest
},
'*'
)
return id
return row
}
export const updateUserEmailFactory =
@@ -53,10 +61,21 @@ export const updateUserEmailFactory =
if (queryWithUserId.userId && update.primary) {
await checkPrimaryEmail({ db })(queryWithUserId)
}
const [updated] = await db<UserEmail>(UserEmails.name)
.where(query)
.update(update, '*')
const q = db<UserEmail>(UserEmails.name)
.where(omit(query, ['email']))
.update(
{
...update,
...(update.email?.length ? { email: update.email.toLowerCase() } : {})
},
'*'
)
if ('email' in query && query.email?.length) {
whereEmailIs(q, query.email)
}
const [updated] = await q
return updated
}
@@ -91,22 +110,36 @@ export const deleteUserEmailFactory =
export const findPrimaryEmailForUserFactory =
({ db }: { db: Knex }): FindPrimaryEmailForUser =>
async (query) => {
return db(UserEmails.name)
if (!get(query, 'userId') && !get(query, 'email')) return undefined
const q = db<UserEmail>(UserEmails.name)
.where({
...query,
...omit(query, ['email']),
primary: true
})
.first()
if ('email' in query && query.email?.length) {
whereEmailIs(q, query.email)
}
return await q
}
export const findEmailFactory =
({ db }: { db: Knex }): FindEmail =>
async (query) => {
return db(UserEmails.name)
.where({
...query
})
if (!Object.values(query).length) return undefined
const q = db<UserEmail>(UserEmails.name)
.where(omit(query, ['email']))
.first()
if (query.email?.length) {
whereEmailIs(q, query.email)
}
return await q
}
export const findEmailsByUserIdFactory =
packages/server/modules/core/repositories/users.ts
+9 -6
View File
@@ -44,7 +44,7 @@ function sanitizeUserRecord<T extends Nullable<UserRecord>>(user: T): T {
// Once we will refactor services this one should not be exported
export const getUsersBaseQuery = (
query: Knex.QueryBuilder,
{ searchQuery, role }: { searchQuery: string | null; role: string | null }
{ searchQuery, role }: { searchQuery: string | null; role?: string | null }
) => {
if (searchQuery) {
query.where((queryBuilder) => {
@@ -96,7 +96,7 @@ export async function getUsers(
type UserQuery = {
query: string | null
role: ServerRoles | null
role?: ServerRoles | null
}
/**
@@ -109,7 +109,7 @@ export async function listUsers({
role
}: {
limit: number
cursor: Date | null
cursor?: Date | null
} & UserQuery): Promise<UserWithRole[]> {
const sanitizedLimit = clamp(limit, 1, 200)
@@ -136,7 +136,9 @@ export async function listUsers({
export async function countUsers(args: UserQuery): Promise<number> {
const q = Users.knex()
.leftJoin(ServerAcl.name, ServerAcl.col.userId, Users.col.id)
.leftJoin(UserEmails.name, UserEmails.col.userId, Users.col.id)
.countDistinct(Users.col.id)
const result = await getUsersBaseQuery(q, {
searchQuery: args.query,
role: args.role
@@ -187,17 +189,18 @@ export async function getUserByEmail(
*/
export async function markUserAsVerified(email: string) {
const UserCols = Users.with({ withoutTablePrefix: true }).col
const q = Users.knex()
const usersUpdate = await Users.knex()
.whereRaw('lower(email) = lower(?)', [email])
.update({
[UserCols.verified]: true
})
await markUserEmailAsVerifiedFactory({
const userEmailsUpdate = await markUserEmailAsVerifiedFactory({
updateUserEmail: updateUserEmailFactory({ db })
})({ email: email.toLowerCase().trim() })
return !!(await q)
return !!(usersUpdate || userEmailsUpdate)
}
export async function markOnboardingComplete(userId: string) {
packages/server/modules/core/services/users.js
+10 -1
View File
@@ -69,6 +69,8 @@ module.exports = {
// ONLY ALLOW SKIPPING WHEN CREATING USERS FOR TESTS, IT'S UNSAFE OTHERWISE
const { skipPropertyValidation = false } = options || {}
if (!user.email?.length) throw new UserInputError('E-mail address is required')
let expectedRole = null
if (user.role) {
const isValidRole = Object.values(Roles.Server).includes(user.role)
@@ -323,6 +325,9 @@ module.exports = {
* TODO: this should be moved to repositories
* Get all users or filter them with the specified searchQuery. This is meant for
* server admins, because it exposes the User object (& thus the email).
* @param {number} limit
* @param {number} offset
* @param {string | null} searchQuery
* @returns {Promise<import('@/modules/core/helpers/userHelper').UserRecord[]>}
*/
async getUsers(limit = 10, offset = 0, searchQuery = null) {
@@ -347,7 +352,11 @@ module.exports = {
.offset(offset)
},
// TODO: this should be moved to repositories
/**
* TODO: this should be moved to repositories
* @param {string|null} searchQuery
* @returns
*/
async countUsers(searchQuery = null) {
const query = Users().leftJoin(
UserEmails.name,
packages/server/modules/core/tests/integration/createUser.spec.ts
+3 -3
View File
@@ -2,7 +2,6 @@ import { expect } from 'chai'
import { createUser, getUser, getUserById } from '@/modules/core/services/users'
import { beforeEach, describe, it } from 'mocha'
import { beforeEachContext } from '@/test/hooks'
import { UserRecord } from '@/modules/core/helpers/types'
import { db } from '@/db/knex'
import {
createRandomEmail,
@@ -75,8 +74,9 @@ describe('Users @core-users', () => {
password: createRandomPassword()
})
const user = (await getUserById({ userId })) as UserRecord
expect(user.email).to.eq(email)
const user = await getUserById({ userId })
expect(user).to.be.ok
expect(user!.email.toLowerCase()).to.eq(email.toLowerCase())
const userEmail = await findPrimaryEmailForUserFactory({ db })({ userId })
packages/server/modules/core/tests/integration/emailVerification.spec.ts
+7 -6
View File
@@ -1,5 +1,8 @@
import { describe } from 'mocha'
import { updateUserEmailFactory } from '@/modules/core/repositories/userEmails'
import {
findEmailFactory,
updateUserEmailFactory
} from '@/modules/core/repositories/userEmails'
import { db } from '@/db/knex'
import { createUser } from '@/modules/core/services/users'
import {
@@ -8,9 +11,6 @@ import {
} from '@/modules/core/helpers/testHelpers'
import { markUserEmailAsVerifiedFactory } from '@/modules/core/services/users/emailVerification'
import { expect } from 'chai'
import { UserEmails } from '@/modules/core/dbSchema'
const userEmailTable = db(UserEmails.name)
describe('Verification @user-emails', () => {
it('should mark user email as verified', async () => {
@@ -26,7 +26,8 @@ describe('Verification @user-emails', () => {
updateUserEmail: updateUserEmailFactory({ db })
})({ email })
const userEmail = await userEmailTable.where({ email }).first()
expect(userEmail.verified).to.be.true
const userEmail = await findEmailFactory({ db })({ email })
expect(userEmail).to.be.ok
expect(userEmail!.verified).to.be.true
})
})
packages/server/modules/core/tests/integration/findUsers.spec.ts
+4 -4
View File
@@ -31,7 +31,7 @@ describe('Find users @core', () => {
const user = (await getUsers([userId]))[0]
expect(user.id).to.eq(userId)
expect(user.email).to.eq(newEmail)
expect(user.email.toLowerCase()).to.eq(newEmail.toLowerCase())
expect(user.verified).to.eq(true)
})
@@ -55,7 +55,7 @@ describe('Find users @core', () => {
const user = (await getUsers([userId], { withRole: true }))[0]
expect(user.id).to.eq(userId)
expect(user.email).to.eq(newEmail)
expect(user.email.toLowerCase()).to.eq(newEmail.toLowerCase())
expect(user.verified).to.eq(true)
})
})
@@ -83,7 +83,7 @@ describe('Find users @core', () => {
await listUsers({ query: '', limit: 10, cursor: null, role: null })
)[0]
expect(user.id).to.eq(userId)
expect(user.email).to.eq(newEmail)
expect(user.email.toLowerCase()).to.eq(newEmail.toLowerCase())
expect(user.verified).to.eq(true)
})
})
@@ -97,7 +97,7 @@ describe('Find users @core', () => {
email
})
const user = await getUserByEmail(email)
expect(user!.email).to.equal('test@example.org')
expect(user!.email).to.equal(email.toLowerCase())
})
})
})
packages/server/modules/core/tests/integration/updateUser.spec.ts
+2 -2
View File
@@ -47,7 +47,7 @@ describe('Users @core-users', () => {
const updated = await getUser(userId)
const updatedUserEmail = await userEmailsDB.where({ userId, primary: true }).first()
expect(updated.email).eq(newEmail)
expect(updatedUserEmail.email).eq(newEmail)
expect(updated.email.toLowerCase()).eq(newEmail.toLowerCase())
expect(updatedUserEmail.email.toLowerCase()).eq(newEmail.toLowerCase())
})
})
packages/server/modules/core/tests/integration/userEmails.graph.spec.ts
+19 -20
View File
@@ -6,16 +6,18 @@ import {
createRandomEmail,
createRandomPassword
} from '@/modules/core/helpers/testHelpers'
import { createUserEmailFactory } from '@/modules/core/repositories/userEmails'
import {
createUserEmailFactory,
findEmailFactory
} from '@/modules/core/repositories/userEmails'
import { db } from '@/db/knex'
import { UserEmails, Users } from '@/modules/core/dbSchema'
import { UserEmail } from '@/modules/core/domain/userEmails/types'
import { testApolloServer } from '@/test/graphqlHelper'
import {
CreateUserEmailDocument,
DeleteUserEmailDocument,
SetPrimaryUserEmailDocument
} from '@/test/graphql/generated/graphql'
import { UserEmails, Users } from '@/modules/core/dbSchema'
describe('User emails graphql @core', () => {
before(async () => {
@@ -41,23 +43,18 @@ describe('User emails graphql @core', () => {
})
expect(res).to.not.haveGraphQLErrors()
const userEmail = await db<UserEmail>(UserEmails.name)
.where({
userId,
email: secondEmail
})
.first()
const userEmail = await findEmailFactory({ db })({ email: secondEmail, userId })
expect(userEmail).to.be.ok
expect(userEmail!.email).to.eq(secondEmail)
expect(userEmail!.email.toLowerCase()).to.eq(secondEmail.toLowerCase())
expect(userEmail!.userId).to.eq(userId)
const createRes = res.data?.activeUserMutations.emailMutations.create
expect(createRes).to.be.ok
expect(createRes?.emails.length).to.eq(2)
expect((createRes?.emails || []).map((e) => e.email)).to.deep.equalInAnyOrder([
firstEmail,
secondEmail
firstEmail.toLowerCase(),
secondEmail.toLowerCase()
])
})
})
@@ -72,7 +69,7 @@ describe('User emails graphql @core', () => {
})
const email = createRandomEmail()
const id = await createUserEmailFactory({ db })({
const { id } = await createUserEmailFactory({ db })({
userEmail: {
email,
userId,
@@ -86,8 +83,10 @@ describe('User emails graphql @core', () => {
expect(res).to.not.haveGraphQLErrors()
expect(res.data?.activeUserMutations.emailMutations.delete.id).to.be.ok
expect(
res.data?.activeUserMutations.emailMutations.delete.emails.map((e) => e.email)
).deep.equal([firstEmail])
res.data?.activeUserMutations.emailMutations.delete.emails.map((e) =>
e.email.toLowerCase()
)
).deep.equal([firstEmail.toLowerCase()])
})
})
@@ -100,7 +99,7 @@ describe('User emails graphql @core', () => {
})
const email = createRandomEmail()
const id = await createUserEmailFactory({ db })({
const { id } = await createUserEmailFactory({ db })({
userEmail: {
email,
userId,
@@ -114,10 +113,10 @@ describe('User emails graphql @core', () => {
expect(res).to.not.haveGraphQLErrors()
expect(res.data?.activeUserMutations.emailMutations.setPrimary.id).to.be.ok
expect(
res.data?.activeUserMutations.emailMutations.setPrimary.emails.find(
(e) => !!e.primary
)?.email
).to.eq(email)
res.data?.activeUserMutations.emailMutations.setPrimary.emails
.find((e) => !!e.primary)
?.email.toLowerCase()
).to.eq(email.toLowerCase())
})
})
})
packages/server/modules/core/tests/integration/userEmails.spec.ts
+173 -14
View File
@@ -2,23 +2,31 @@ import { before } from 'mocha'
import { createUser } from '@/modules/core/services/users'
import { beforeEachContext } from '@/test/hooks'
import { expect } from 'chai'
import { getUserByEmail, markUserAsVerified } from '@/modules/core/repositories/users'
import {
countUsers,
getUserByEmail,
listUsers,
markUserAsVerified
} from '@/modules/core/repositories/users'
import * as UsersService from '@/modules/core/services/users'
import { db } from '@/db/knex'
import {
createRandomEmail,
createRandomPassword
createRandomPassword,
randomizeCase
} from '@/modules/core/helpers/testHelpers'
import { UserEmails } from '@/modules/core/dbSchema'
import {
createUserEmailFactory,
deleteUserEmailFactory,
findEmailFactory,
findPrimaryEmailForUserFactory,
setPrimaryUserEmailFactory,
updateUserEmailFactory
} from '@/modules/core/repositories/userEmails'
import { expectToThrow } from '@/test/assertionHelper'
const userEmailTable = db(UserEmails.name)
import { MaybeNullOrUndefined } from '@speckle/shared'
import { BasicTestUser, createTestUsers } from '@/test/authHelper'
import { UserEmails, Users } from '@/modules/core/dbSchema'
describe('Core @user-emails', () => {
before(async () => {
@@ -41,8 +49,8 @@ describe('Core @user-emails', () => {
await markUserAsVerified(email)
const userEmail = await userEmailTable.where({ email }).first()
expect(userEmail.verified).to.be.true
const userEmail = await findEmailFactory({ db })({ email })
expect(userEmail?.verified).to.be.true
})
})
@@ -56,9 +64,10 @@ describe('Core @user-emails', () => {
})
const userEmail = await findEmailFactory({ db })({ userId, email })
expect(userEmail).to.be.ok
const err = await expectToThrow(() =>
deleteUserEmailFactory({ db })({ id: userEmail.id, userId })
deleteUserEmailFactory({ db })({ id: userEmail!.id, userId })
)
expect(err.message).to.eq('Cannot delete last user email')
})
@@ -79,9 +88,10 @@ describe('Core @user-emails', () => {
}
})
const userEmail = await findEmailFactory({ db })({ userId, email, primary: true })
expect(userEmail).to.be.ok
const err = await expectToThrow(() =>
deleteUserEmailFactory({ db })({ id: userEmail.id, userId })
deleteUserEmailFactory({ db })({ id: userEmail!.id, userId })
)
expect(err.message).to.eq('Cannot delete primary email')
})
@@ -106,8 +116,12 @@ describe('Core @user-emails', () => {
email,
primary: false
})
expect(userEmail).to.be.ok
const deleted = await deleteUserEmailFactory({ db })({ id: userEmail.id, userId })
const deleted = await deleteUserEmailFactory({ db })({
id: userEmail!.id,
userId
})
expect(deleted).to.be.true
@@ -142,9 +156,10 @@ describe('Core @user-emails', () => {
email,
primary: false
})
expect(userEmail).to.be.ok
const updated = await setPrimaryUserEmailFactory({ db })({
id: userEmail.id,
id: userEmail!.id,
userId
})
@@ -159,8 +174,8 @@ describe('Core @user-emails', () => {
primary: true
})
expect(previousPrimary.primary).to.be.false
expect(newPrimary.primary).to.be.true
expect(previousPrimary?.primary).to.be.false
expect(newPrimary?.primary).to.be.true
})
})
@@ -208,11 +223,12 @@ describe('Core @user-emails', () => {
email,
primary: false
})
expect(userEmail).to.be.ok
const err = await expectToThrow(() =>
updateUserEmailFactory({ db })({
query: {
id: userEmail.id,
id: userEmail!.id,
userId
},
update: { primary: true }
@@ -222,4 +238,147 @@ describe('Core @user-emails', () => {
expect(err.message).to.eq('A primary email already exists for this user')
})
})
describe('supports case insensitive email lookup/mutation', () => {
const randomCaseGuy: BasicTestUser = {
name: 'RAnDoM cAsE gUY',
email: createRandomEmail(),
password: createRandomPassword(),
id: ''
}
const updateEmailDirectly = async (newEmail: string) => {
// Intentionally putting case-sensitive email in DB, avoiding any code protection
// to ensure that the lookups still work
const [emailsRow] = await UserEmails.knex()
.where({ userId: randomCaseGuy.id })
.update({ email: newEmail }, '*')
expect(emailsRow.email).to.eq(newEmail)
const [usersRow] = await Users.knex()
.where({ id: randomCaseGuy.id })
.update({ email: newEmail }, '*')
expect(usersRow.email).to.eq(newEmail)
}
const assertLowercaseEquality = (
val1: MaybeNullOrUndefined<string>,
val2: string
) => {
expect(val1?.toLowerCase()).to.eq(val2.toLowerCase())
}
const assertLowercase = (val1: MaybeNullOrUndefined<string>) => {
expect(val1).to.be.ok
expect(val1).to.eq(val1!.toLowerCase())
}
before(async () => {
await createTestUsers([randomCaseGuy])
await updateEmailDirectly(randomCaseGuy.email)
})
it('with findEmailFactory()', async () => {
const { email, id: userId } = randomCaseGuy
const foundEmail = (
await findEmailFactory({ db })({ email: randomizeCase(email), userId })
)?.email
assertLowercaseEquality(foundEmail, email)
})
it('with updateUserEmailFactory()', async () => {
const { email, id: userId } = randomCaseGuy
const newEmail = createRandomEmail()
const updatedEmail = (
await updateUserEmailFactory({ db })({
query: { email: randomizeCase(email), userId },
update: { email: newEmail }
})
)?.email
assertLowercaseEquality(updatedEmail, newEmail)
assertLowercase(updatedEmail)
randomCaseGuy.email = newEmail
updateEmailDirectly(newEmail)
})
it('with createUserEmailFactory()', async () => {
const { id: userId } = randomCaseGuy
const newEmail = createRandomEmail()
const createdEmail = (
await createUserEmailFactory({ db })({
userEmail: { email: newEmail, userId }
})
)?.email
assertLowercaseEquality(createdEmail, newEmail)
assertLowercase(createdEmail)
})
it('with findPrimaryEmailForUserFactory()', async () => {
const { email } = randomCaseGuy
const primaryEmail = (
await findPrimaryEmailForUserFactory({ db })({
email: randomizeCase(email),
userId: randomCaseGuy.id
})
)?.email
assertLowercaseEquality(primaryEmail, email)
})
it('with listUsers()', async () => {
const [user] = await listUsers({
query: randomizeCase(randomCaseGuy.email),
limit: 1
})
expect(user).to.be.ok
assertLowercaseEquality(user.email, randomCaseGuy.email)
})
it('with countUsers()', async () => {
const count = await countUsers({ query: randomizeCase(randomCaseGuy.email) })
expect(count).to.eq(1)
})
it('with getUserByEmail()', async () => {
const user = await getUserByEmail(randomizeCase(randomCaseGuy.email))
expect(user).to.be.ok
assertLowercaseEquality(user?.email, randomCaseGuy.email)
})
it('with markUserAsVerified()', async () => {
const res = await markUserAsVerified(randomizeCase(randomCaseGuy.email))
expect(res).to.be.ok
const user = await getUserByEmail(randomCaseGuy.email)
expect(user?.verified).to.be
})
it('with UsersService.getUserByEmail()', async () => {
const user = await UsersService.getUserByEmail({
email: randomizeCase(randomCaseGuy.email)
})
expect(user).to.be.ok
assertLowercaseEquality(user?.email, randomCaseGuy.email)
})
it('with UsersService.getUsers()', async () => {
const users = await UsersService.getUsers(
10,
0,
randomizeCase(randomCaseGuy.email)
)
expect(users).to.be.ok
expect(users).to.have.length(1)
assertLowercaseEquality(users[0].email, randomCaseGuy.email)
})
it('with UsersService.countUsers()', async () => {
const count = await UsersService.countUsers(randomizeCase(randomCaseGuy.email))
expect(count).to.eq(1)
})
})
})