diff --git a/api/src/services/users.test.ts b/api/src/services/users.test.ts index a6e8b2adcb..43843d6d6b 100644 --- a/api/src/services/users.test.ts +++ b/api/src/services/users.test.ts @@ -3,7 +3,7 @@ import knex from 'knex'; import type { Knex } from 'knex'; import { createTracker, MockClient, Tracker } from 'knex-mock-client'; import { afterEach, beforeAll, beforeEach, describe, expect, it, MockedFunction, SpyInstance, vi } from 'vitest'; -import { ItemsService, UsersService } from './index.js'; +import { ItemsService, MailService, UsersService } from './index.js'; import { ForbiddenException, InvalidPayloadException } from '../exceptions/index.js'; import { RecordNotUniqueException } from '../exceptions/database/record-not-unique.js'; @@ -12,6 +12,13 @@ vi.mock('../../src/database/index', () => ({ getDatabaseClient: vi.fn().mockReturnValue('postgres'), })); +vi.mock('./mail', () => { + const MailService = vi.fn(); + MailService.prototype.send = vi.fn(); + + return { MailService }; +}); + const testRoleId = '4ccdb196-14b3-4ed1-b9da-c1978be07ca2'; const testSchema = { @@ -66,6 +73,8 @@ describe('Integration Tests', () => { describe('Services / Users', () => { let service: UsersService; + let mailService: MailService; + let superCreateManySpy: SpyInstance; let superUpdateManySpy: SpyInstance; let checkUniqueEmailsSpy: SpyInstance; let checkPasswordPolicySpy: SpyInstance; @@ -106,7 +115,8 @@ describe('Integration Tests', () => { }, }); - superUpdateManySpy = vi.spyOn(ItemsService.prototype, 'updateMany'); + superCreateManySpy = vi.spyOn(ItemsService.prototype as any, 'createMany'); + superUpdateManySpy = vi.spyOn(ItemsService.prototype as any, 'updateMany'); // "as any" are needed since these are private methods checkUniqueEmailsSpy = vi @@ -121,13 +131,16 @@ describe('Integration Tests', () => { checkRemainingActiveAdminSpy = vi .spyOn(UsersService.prototype as any, 'checkRemainingActiveAdmin') .mockResolvedValue(() => vi.fn()); + + vi.spyOn(UsersService.prototype as any, 'inviteUrl').mockImplementation(() => vi.fn()); + + mailService = new MailService({ + schema: testSchema, + }); }); afterEach(() => { - checkUniqueEmailsSpy.mockClear(); - checkPasswordPolicySpy.mockClear(); - checkRemainingAdminExistenceSpy.mockClear(); - checkRemainingActiveAdminSpy.mockClear(); + vi.clearAllMocks(); }); describe('createOne', () => { @@ -603,5 +616,92 @@ describe('Integration Tests', () => { expect(checkRemainingAdminExistenceSpy).toBeCalledTimes(1); }); }); + + describe('invite', () => { + it('should invite new users', async () => { + const service = new UsersService({ + knex: db, + schema: testSchema, + accountability: { role: 'test', admin: true }, + }); + + const promise = service.inviteUser('user@example.com', 'invite-role', null); + + await expect(promise).resolves.not.toThrow(); + + expect(superCreateManySpy.mock.lastCall![0]).toEqual([ + expect.objectContaining({ + email: 'user@example.com', + status: 'invited', + role: 'invite-role', + }), + ]); + + expect(mailService.send).toBeCalledTimes(1); + }); + + it('should re-send invites for invited users', async () => { + const service = new UsersService({ + knex: db, + schema: testSchema, + accountability: { role: 'test', admin: true }, + }); + + // mock an invited user + vi.spyOn(UsersService.prototype as any, 'getUserByEmail').mockResolvedValueOnce({ + status: 'invited', + role: 'invite-role', + }); + + const promise = service.inviteUser('user@example.com', 'invite-role', null); + await expect(promise).resolves.not.toThrow(); + + expect(superCreateManySpy).not.toBeCalled(); + expect(mailService.send).toBeCalledTimes(1); + }); + + it('should not re-send invites for users in state other than invited', async () => { + const service = new UsersService({ + knex: db, + schema: testSchema, + accountability: { role: 'test', admin: true }, + }); + + // mock an active user + vi.spyOn(UsersService.prototype as any, 'getUserByEmail').mockResolvedValueOnce({ + status: 'active', + role: 'invite-role', + }); + + const promise = service.inviteUser('user@example.com', 'invite-role', null); + await expect(promise).resolves.not.toThrow(); + + expect(superCreateManySpy).not.toBeCalled(); + expect(mailService.send).not.toBeCalled(); + }); + + it('should update role when re-sent invite contains different role than user has', async () => { + const service = new UsersService({ + knex: db, + schema: testSchema, + accountability: { role: 'test', admin: true }, + }); + + tracker.on.select(/select "admin_access" from "directus_roles"/).response({ admin_access: true }); + + // mock an invited user with different role + vi.spyOn(UsersService.prototype as any, 'getUserByEmail').mockResolvedValueOnce({ + id: 1, + status: 'invited', + role: 'existing-role', + }); + + const promise = service.inviteUser('user@example.com', 'invite-role', null); + await expect(promise).resolves.not.toThrow(); + + expect(superUpdateManySpy.mock.lastCall![0]).toEqual([1]); + expect(superUpdateManySpy.mock.lastCall![1]).toContain({ role: 'invite-role' }); + }); + }); }); }); diff --git a/api/src/services/users.ts b/api/src/services/users.ts index faf3f7595d..0c52bd3b66 100644 --- a/api/src/services/users.ts +++ b/api/src/services/users.ts @@ -2,7 +2,7 @@ import { FailedValidationException } from '@directus/exceptions'; import type { Query } from '@directus/types'; import { getSimpleHash, toArray } from '@directus/utils'; import jwt from 'jsonwebtoken'; -import { cloneDeep } from 'lodash-es'; +import { cloneDeep, isEmpty } from 'lodash-es'; import { performance } from 'perf_hooks'; import getDatabase from '../database/index.js'; import env from '../env.js'; @@ -135,6 +135,30 @@ export class UsersService extends ItemsService { } } + /** + * Get basic information of user identified by email + */ + private async getUserByEmail(email: string): Promise<{ id: string; role: string; status: string; password: string }> { + return await this.knex + .select('id', 'role', 'status', 'password') + .from('directus_users') + .whereRaw(`LOWER(??) = ?`, ['email', email.toLowerCase()]) + .first(); + } + + /** + * Create url for inviting users + */ + private inviteUrl(email: string, url: string | null): string { + const payload = { email, scope: 'invite' }; + + const token = jwt.sign(payload, env['SECRET'] as string, { expiresIn: '7d', issuer: 'directus' }); + const inviteURL = url ? new Url(url) : new Url(env['PUBLIC_URL']).addPath('admin', 'accept-invite'); + inviteURL.setQuery('token', token); + + return inviteURL.toString(); + } + /** * Create a new user */ @@ -326,26 +350,34 @@ export class UsersService extends ItemsService { }); for (const email of emails) { - const payload = { email, scope: 'invite' }; - const token = jwt.sign(payload, env['SECRET'] as string, { expiresIn: '7d', issuer: 'directus' }); - const subjectLine = subject ?? "You've been invited"; - const inviteURL = url ? new Url(url) : new Url(env['PUBLIC_URL']).addPath('admin', 'accept-invite'); - inviteURL.setQuery('token', token); + // Check if user is known + const user = await this.getUserByEmail(email); - // Create user first to verify uniqueness - await this.createOne({ email, role, status: 'invited' }, opts); + // Create user first to verify uniqueness if unknown + if (isEmpty(user)) { + await this.createOne({ email, role, status: 'invited' }, opts); - await mailService.send({ - to: email, - subject: subjectLine, - template: { - name: 'user-invitation', - data: { - url: inviteURL.toString(), - email, + // For known users update role if changed + } else if (user.status === 'invited' && user.role !== role) { + await this.updateOne(user.id, { role }, opts); + } + + // Send invite for new and already invited users + if (isEmpty(user) || user.status === 'invited') { + const subjectLine = subject ?? "You've been invited"; + + await mailService.send({ + to: email, + subject: subjectLine, + template: { + name: 'user-invitation', + data: { + url: this.inviteUrl(email, url), + email, + }, }, - }, - }); + }); + } } } @@ -357,7 +389,7 @@ export class UsersService extends ItemsService { if (scope !== 'invite') throw new ForbiddenException(); - const user = await this.knex.select('id', 'status').from('directus_users').where({ email }).first(); + const user = await this.getUserByEmail(email); if (user?.status !== 'invited') { throw new InvalidPayloadException(`Email address ${email} hasn't been invited.`); @@ -376,11 +408,7 @@ export class UsersService extends ItemsService { const STALL_TIME = 500; const timeStart = performance.now(); - const user = await this.knex - .select('status', 'password') - .from('directus_users') - .whereRaw('LOWER(??) = ?', ['email', email.toLowerCase()]) - .first(); + const user = await this.getUserByEmail(email); if (user?.status !== 'active') { await stall(STALL_TIME, timeStart); @@ -436,7 +464,7 @@ export class UsersService extends ItemsService { opts.preMutationException = err; } - const user = await this.knex.select('id', 'status', 'password').from('directus_users').where({ email }).first(); + const user = await this.getUserByEmail(email); if (user?.status !== 'active' || hash !== getSimpleHash('' + user.password)) { throw new ForbiddenException(); diff --git a/contributors.yml b/contributors.yml index 86209c0cb0..47fd1eaa8c 100644 --- a/contributors.yml +++ b/contributors.yml @@ -15,5 +15,6 @@ - denkhaus - joselcvarela - knulpi +- david-zacharias - hanneskuettner - JoshTheDerf