Skip to content

Commit 6f19ce0

Browse files
feat: move create user validations to the input (#9189)
## About the changes Validations in the constructor were executed on the way out (i.e. when reading users). Instead we should validate when we insert the users. We're also relaxing the email validation to support top domain emails (e.g. `...@jp`)
1 parent b9aa554 commit 6f19ce0

File tree

3 files changed

+63
-79
lines changed

3 files changed

+63
-79
lines changed

src/lib/services/user-service.test.ts

+45-53
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
import { URL } from 'url';
22
import UserService from './user-service';
33
import UserStoreMock from '../../test/fixtures/fake-user-store';
4-
import EventStoreMock from '../../test/fixtures/fake-event-store';
54
import AccessServiceMock from '../../test/fixtures/access-service-mock';
65
import ResetTokenService from './reset-token-service';
76
import { EmailService } from './email-service';
@@ -21,50 +20,53 @@ const config: IUnleashConfig = createTestConfig();
2120

2221
const systemUser = new User({ id: -1, username: 'system' });
2322

24-
test('Should create new user', async () => {
25-
const userStore = new UserStoreMock();
26-
const eventStore = new EventStoreMock();
27-
const accessService = new AccessServiceMock();
28-
const resetTokenStore = new FakeResetTokenStore();
29-
const resetTokenService = new ResetTokenService(
30-
{ resetTokenStore },
31-
config,
32-
);
33-
const sessionStore = new FakeSessionStore();
34-
const sessionService = new SessionService({ sessionStore }, config);
35-
const emailService = new EmailService(config);
36-
const eventService = createFakeEventsService(config);
37-
const settingService = new SettingService(
38-
{
39-
settingStore: new FakeSettingStore(),
40-
},
41-
config,
42-
eventService,
43-
);
23+
test.each([undefined, '[email protected]', 'top-level-domain@jp'])(
24+
'Should create new user with email %s',
25+
async (email?: string) => {
26+
const userStore = new UserStoreMock();
27+
const accessService = new AccessServiceMock();
28+
const resetTokenStore = new FakeResetTokenStore();
29+
const resetTokenService = new ResetTokenService(
30+
{ resetTokenStore },
31+
config,
32+
);
33+
const sessionStore = new FakeSessionStore();
34+
const sessionService = new SessionService({ sessionStore }, config);
35+
const emailService = new EmailService(config);
36+
const eventService = createFakeEventsService(config);
37+
const settingService = new SettingService(
38+
{
39+
settingStore: new FakeSettingStore(),
40+
},
41+
config,
42+
eventService,
43+
);
4444

45-
const service = new UserService({ userStore }, config, {
46-
accessService,
47-
resetTokenService,
48-
emailService,
49-
eventService,
50-
sessionService,
51-
settingService,
52-
});
53-
const user = await service.createUser(
54-
{
55-
username: 'test',
56-
rootRole: 1,
57-
},
58-
extractAuditInfoFromUser(systemUser),
59-
);
60-
const storedUser = await userStore.get(user.id);
61-
const allUsers = await userStore.getAll();
45+
const service = new UserService({ userStore }, config, {
46+
accessService,
47+
resetTokenService,
48+
emailService,
49+
eventService,
50+
sessionService,
51+
settingService,
52+
});
53+
const user = await service.createUser(
54+
{
55+
username: 'test',
56+
rootRole: 1,
57+
email,
58+
},
59+
extractAuditInfoFromUser(systemUser),
60+
);
61+
const storedUser = await userStore.get(user.id);
62+
const allUsers = await userStore.getAll();
6263

63-
expect(user.id).toBeTruthy();
64-
expect(user.username).toBe('test');
65-
expect(allUsers.length).toBe(1);
66-
expect(storedUser.username).toBe('test');
67-
});
64+
expect(user.id).toBeTruthy();
65+
expect(user.username).toBe('test');
66+
expect(allUsers.length).toBe(1);
67+
expect(storedUser.username).toBe('test');
68+
},
69+
);
6870

6971
describe('Default admin initialization', () => {
7072
const DEFAULT_ADMIN_USERNAME = 'admin';
@@ -79,7 +81,6 @@ describe('Default admin initialization', () => {
7981
jest.clearAllMocks();
8082

8183
const userStore = new UserStoreMock();
82-
const eventStore = new EventStoreMock();
8384
const accessService = new AccessServiceMock();
8485
const resetTokenStore = new FakeResetTokenStore();
8586
const resetTokenService = new ResetTokenService(
@@ -149,7 +150,6 @@ describe('Default admin initialization', () => {
149150

150151
test('Should not create any default admin user if `createAdminUser` is not true and `initialAdminUser` is not set', async () => {
151152
const userStore = new UserStoreMock();
152-
const eventStore = new EventStoreMock();
153153
const accessService = new AccessServiceMock();
154154
const resetTokenStore = new FakeResetTokenStore();
155155
const resetTokenService = new ResetTokenService(
@@ -204,7 +204,6 @@ describe('Default admin initialization', () => {
204204

205205
test('Should be a valid password', async () => {
206206
const userStore = new UserStoreMock();
207-
const eventStore = new EventStoreMock();
208207
const accessService = new AccessServiceMock();
209208
const resetTokenStore = new FakeResetTokenStore();
210209
const resetTokenService = new ResetTokenService(
@@ -240,7 +239,6 @@ test('Should be a valid password', async () => {
240239

241240
test('Password must be at least 10 chars', async () => {
242241
const userStore = new UserStoreMock();
243-
const eventStore = new EventStoreMock();
244242
const accessService = new AccessServiceMock();
245243
const resetTokenStore = new FakeResetTokenStore();
246244
const resetTokenService = new ResetTokenService(
@@ -277,7 +275,6 @@ test('Password must be at least 10 chars', async () => {
277275

278276
test('The password must contain at least one uppercase letter.', async () => {
279277
const userStore = new UserStoreMock();
280-
const eventStore = new EventStoreMock();
281278
const accessService = new AccessServiceMock();
282279
const resetTokenStore = new FakeResetTokenStore();
283280
const resetTokenService = new ResetTokenService(
@@ -315,7 +312,6 @@ test('The password must contain at least one uppercase letter.', async () => {
315312

316313
test('The password must contain at least one number', async () => {
317314
const userStore = new UserStoreMock();
318-
const eventStore = new EventStoreMock();
319315
const accessService = new AccessServiceMock();
320316
const resetTokenStore = new FakeResetTokenStore();
321317
const resetTokenService = new ResetTokenService(
@@ -354,7 +350,6 @@ test('The password must contain at least one number', async () => {
354350

355351
test('The password must contain at least one special character', async () => {
356352
const userStore = new UserStoreMock();
357-
const eventStore = new EventStoreMock();
358353
const accessService = new AccessServiceMock();
359354
const resetTokenStore = new FakeResetTokenStore();
360355
const resetTokenService = new ResetTokenService(
@@ -392,7 +387,6 @@ test('The password must contain at least one special character', async () => {
392387

393388
test('Should be a valid password with special chars', async () => {
394389
const userStore = new UserStoreMock();
395-
const eventStore = new EventStoreMock();
396390
const accessService = new AccessServiceMock();
397391
const resetTokenStore = new FakeResetTokenStore();
398392
const resetTokenService = new ResetTokenService(
@@ -427,7 +421,6 @@ test('Should be a valid password with special chars', async () => {
427421

428422
test('Should send password reset email if user exists', async () => {
429423
const userStore = new UserStoreMock();
430-
const eventStore = new EventStoreMock();
431424
const accessService = new AccessServiceMock();
432425
const resetTokenStore = new FakeResetTokenStore();
433426
const resetTokenService = new ResetTokenService(
@@ -478,7 +471,6 @@ test('Should send password reset email if user exists', async () => {
478471

479472
test('Should throttle password reset email', async () => {
480473
const userStore = new UserStoreMock();
481-
const eventStore = new EventStoreMock();
482474
const accessService = new AccessServiceMock();
483475
const resetTokenStore = new FakeResetTokenStore();
484476
const resetTokenService = new ResetTokenService(

src/lib/services/user-service.ts

+17-14
Original file line numberDiff line numberDiff line change
@@ -240,6 +240,19 @@ class UserService {
240240
return this.store.getByQuery({ email });
241241
}
242242

243+
private validateEmail(email?: string): void {
244+
if (email) {
245+
Joi.assert(
246+
email,
247+
Joi.string().email({
248+
ignoreLength: true,
249+
minDomainSegments: 1,
250+
}),
251+
'Email',
252+
);
253+
}
254+
}
255+
243256
async createUser(
244257
{ username, email, name, password, rootRole }: ICreateUser,
245258
auditUser: IAuditUser = SYSTEM_USER_AUDIT,
@@ -248,13 +261,9 @@ class UserService {
248261
throw new BadDataError('You must specify username or email');
249262
}
250263

251-
if (email) {
252-
Joi.assert(
253-
email,
254-
Joi.string().email({ ignoreLength: true }),
255-
'Email',
256-
);
257-
}
264+
Joi.assert(name, Joi.string(), 'Name');
265+
266+
this.validateEmail(email);
258267

259268
const exists = await this.store.hasUser({ username, email });
260269
if (exists) {
@@ -348,13 +357,7 @@ class UserService {
348357
): Promise<IUserWithRootRole> {
349358
const preUser = await this.getUser(id);
350359

351-
if (email) {
352-
Joi.assert(
353-
email,
354-
Joi.string().email({ ignoreLength: true }),
355-
'Email',
356-
);
357-
}
360+
this.validateEmail(email);
358361

359362
if (rootRole) {
360363
await this.accessService.setUserRootRole(id, rootRole);

src/lib/types/user.ts

+1-12
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import Joi, { ValidationError } from 'joi';
1+
import { ValidationError } from 'joi';
22
import { generateImageUrl } from '../util/generateImageUrl';
33

44
export const AccountTypes = ['User', 'Service Account'] as const;
@@ -90,17 +90,6 @@ export default class User implements IUser {
9090
if (!id) {
9191
throw new ValidationError('Id is required', [], undefined);
9292
}
93-
try {
94-
Joi.assert(
95-
email,
96-
Joi.string().email({ ignoreLength: true }),
97-
'Email',
98-
);
99-
} catch (e) {
100-
console.error('Invalid email', email, e);
101-
}
102-
Joi.assert(username, Joi.string(), 'Username');
103-
Joi.assert(name, Joi.string(), 'Name');
10493

10594
this.id = id;
10695
this.name = name!;

0 commit comments

Comments
 (0)