From 13285cefe6cbb931498cfc16b7e6ae960c169d1f Mon Sep 17 00:00:00 2001 From: MBergCap <111343628+MBergCap@users.noreply.github.com> Date: Tue, 1 Nov 2022 15:30:59 +0100 Subject: [PATCH] EW-167 Refactor infrastructure (#3685) Various replacements of directs access to account collection from feathers stack. Major changes include password recovery, password recovery link generation, JWT support. --- .../account/services/account.service.spec.ts | 15 +++++++ .../account/services/account.service.ts | 8 ++++ .../src/shared/domain/entity/system.entity.ts | 2 +- src/hooks/index.js | 8 ++-- src/services/account/hooks/index.js | 14 +++++-- .../account/services/SupportJWTService.js | 5 +-- .../activation/services/eMailAddress/index.js | 8 +--- src/services/authentication/index.js | 3 +- src/services/link/index.js | 2 +- src/services/passwordRecovery/index.js | 3 +- .../services/ChangePasswordService.js | 8 ++-- .../GenerateRecoveryPasswordTokenService.js | 42 ++++++++----------- test/services/passwordRecovery/index.test.js | 6 +-- test/services/teams/helper/helper.user.js | 12 +++--- test/services/teams/hooks/helpers.test.js | 8 +++- 15 files changed, 85 insertions(+), 59 deletions(-) diff --git a/apps/server/src/modules/account/services/account.service.spec.ts b/apps/server/src/modules/account/services/account.service.spec.ts index 0f2cf94c8b2..f2000757120 100644 --- a/apps/server/src/modules/account/services/account.service.spec.ts +++ b/apps/server/src/modules/account/services/account.service.spec.ts @@ -1,6 +1,7 @@ import { MikroORM } from '@mikro-orm/core'; import { ObjectId } from '@mikro-orm/mongodb'; import { Test, TestingModule } from '@nestjs/testing'; +import bcrypt from 'bcryptjs'; import { EntityNotFoundError } from '@shared/common'; import { Account, EntityId, Role, School, User, RoleName, Permission } from '@shared/domain'; import { AccountRepo } from '@shared/repo'; @@ -404,6 +405,20 @@ describe('AccountService', () => { }); }); + describe('updatePassword', () => { + it('should update password', async () => { + const newPassword = 'newPassword'; + const ret = await accountService.updatePassword(mockTeacherAccount.id, newPassword); + + expect(ret).toBeDefined(); + if (ret.password) { + await expect(bcrypt.compare(newPassword, ret.password)).resolves.toBe(true); + } else { + fail('return password is undefined'); + } + }); + }); + describe('delete', () => { it('should delete account via repo', async () => { await accountService.delete(mockTeacherAccount.id); diff --git a/apps/server/src/modules/account/services/account.service.ts b/apps/server/src/modules/account/services/account.service.ts index f125342b15b..45e0397084e 100644 --- a/apps/server/src/modules/account/services/account.service.ts +++ b/apps/server/src/modules/account/services/account.service.ts @@ -89,6 +89,14 @@ export class AccountService { return AccountEntityToDtoMapper.mapToDto(account); } + async updatePassword(accountId: EntityId, password: string): Promise { + const account = await this.accountRepo.findById(accountId); + account.password = await this.encryptPassword(password); + account.updatedAt = new Date(); + await this.accountRepo.save(account); + return AccountEntityToDtoMapper.mapToDto(account); + } + delete(id: EntityId): Promise { return this.accountRepo.deleteById(id); } diff --git a/apps/server/src/shared/domain/entity/system.entity.ts b/apps/server/src/shared/domain/entity/system.entity.ts index 37aeea046a0..bbd76d394ee 100644 --- a/apps/server/src/shared/domain/entity/system.entity.ts +++ b/apps/server/src/shared/domain/entity/system.entity.ts @@ -102,6 +102,6 @@ export class System extends BaseEntityWithTimestamps { @Property({ nullable: true }) ldapConfig?: Record; - @Property() + @Property({ nullable: true }) provisioningUrl?: string; } diff --git a/src/hooks/index.js b/src/hooks/index.js index 931cbed9a09..2024e49aaa9 100644 --- a/src/hooks/index.js +++ b/src/hooks/index.js @@ -1,6 +1,5 @@ /* eslint-disable no-param-reassign */ -const { Forbidden, GeneralError, NotFound, BadRequest, TypeError } = require('../errors'); const { authenticate } = require('@feathersjs/authentication'); const { v4: uuidv4 } = require('uuid'); @@ -8,6 +7,7 @@ const { v4: uuidv4 } = require('uuid'); const { Configuration } = require('@hpi-schul-cloud/commons'); const _ = require('lodash'); const mongoose = require('mongoose'); +const { Forbidden, GeneralError, NotFound, BadRequest, TypeError } = require('../errors'); const { equal: equalIds } = require('../helper/compare').ObjectId; const logger = require('../logger'); @@ -201,11 +201,13 @@ exports.hasPermissionNoHook = (context, userId, permissionName) => { exports.hasRoleNoHook = (context, userId, roleName, account = false) => { const userService = context.app.service('/users/'); - const accountService = context.app.service('/accounts/'); + const accountService = context.app.service('nest-account-service'); + // What is the account flag for? // if statement can be remove because it has no effect?! if (account) { - return accountService.get(userId).then((_account) => + return accountService.findByUserId(userId).then((_account) => + // eslint-disable-next-line promise/no-nesting userService.find({ query: { _id: _account.userId || '', $populate: 'roles' } }).then((user) => { user.data[0].roles = Array.from(user.data[0].roles); diff --git a/src/services/account/hooks/index.js b/src/services/account/hooks/index.js index 8f49e3d8569..8a3f8fa3bbd 100644 --- a/src/services/account/hooks/index.js +++ b/src/services/account/hooks/index.js @@ -272,7 +272,7 @@ const restrictToUsersSchool = async (context) => { const userService = context.app.service('users'); const { schoolId: usersSchoolId } = await userService.get(context.params.account.userId); - const targetAccount = await context.app.service('accountModel').get(context.id); + const targetAccount = await context.app.service('nest-account-service').findById(context.id); const { schoolId: targetSchoolId } = await userService.get(targetAccount.userId); if (!equalIds(usersSchoolId, targetSchoolId)) { throw new NotFound('this account doesnt exist'); @@ -281,8 +281,16 @@ const restrictToUsersSchool = async (context) => { }; const validateUserName = async (context) => { - const accountService = context.app.service('accounts'); - const { systemId } = context.method === 'create' ? context.data : await accountService.get(context.id); + let account; + + if (context.method === 'create') { + account = context.data; + } else { + account = await context.app.service('nest-account-service').findById(context.id); + } + + const { systemId } = account; + if (systemId) { return context; } diff --git a/src/services/account/services/SupportJWTService.js b/src/services/account/services/SupportJWTService.js index 37bbbe2321f..6bf7c886481 100644 --- a/src/services/account/services/SupportJWTService.js +++ b/src/services/account/services/SupportJWTService.js @@ -7,7 +7,6 @@ const { ObjectId } = require('mongoose').Types; const { BadRequest } = require('../../../errors'); const { hasPermission } = require('../../../hooks/index'); const { authenticationSecret, audience: audienceName } = require('../../authentication/logic'); -const accountModel = require('../model'); const logger = require('../../../logger'); const { addTokenToWhitelistWithIdAndJti } = require('../../authentication/logic/whitelist'); @@ -133,8 +132,8 @@ class SupportJWTService { const requestedUserId = userId.toString(); const currentUserId = params.account.userId.toString(); - const account = await accountModel.findOne({ userId }).select(['_id', 'systemId']).lean().exec(); - if (!account && !account._id) { + const account = await this.app.service('nest-account-service').findByUserId(userId); + if (!account && !account.id) { throw new Error(`Account for user with the id ${userId} does not exist.`); } diff --git a/src/services/activation/services/eMailAddress/index.js b/src/services/activation/services/eMailAddress/index.js index a09b28ec29e..9294bbb593a 100644 --- a/src/services/activation/services/eMailAddress/index.js +++ b/src/services/activation/services/eMailAddress/index.js @@ -101,11 +101,7 @@ class EMailAddressActivationService { async update(id, data, params) { const { entry, user } = data; - const [account] = await this.app.service('/accounts').find({ - query: { - userId: user._id, - }, - }); + const account = await this.app.service('nest-account-service').findByUserId(user._id); if (!account) throw new Forbidden(customErrorMessages.NOT_AUTHORIZED); const email = entry.quarantinedObject; @@ -119,7 +115,7 @@ class EMailAddressActivationService { // update user and account await this.app.service('users').patch(account.userId, { email }); - await this.app.service('/accounts').patch(account._id, { username: email }); + await this.app.service('nest-account-service').updateUsername(account._id, email); // set activation link as consumed await setEntryState(this, entry._id, STATE.SUCCESS); } catch (error) { diff --git a/src/services/authentication/index.js b/src/services/authentication/index.js index daff855611c..78715e02d21 100644 --- a/src/services/authentication/index.js +++ b/src/services/authentication/index.js @@ -10,13 +10,14 @@ const { authConfig } = require('./configuration'); class SCAuthenticationService extends AuthenticationService { async getUserData(user, account) { return { - accountId: account._id, + accountId: account._id ? account._id : account.id, systemId: account.systemId, userId: user._id, schoolId: user.schoolId, roles: user.roles, }; } + async getPayload(authResult, params) { let payload = await super.getPayload(authResult, params); const user = await this.app.service('usersModel').get(authResult.account.userId); diff --git a/src/services/link/index.js b/src/services/link/index.js index f9efd180a0d..3c65ed23177 100644 --- a/src/services/link/index.js +++ b/src/services/link/index.js @@ -78,7 +78,7 @@ module.exports = function setup() { if (user && user.importHash) linkData.hash = user.importHash; else { if (user) { - const account = ((await app.service('accounts').find({ query: { userId: user._id } })) || {})[0]; + const account = await app.service('nest-account-service').findByUserId(user._id); if (account && account.userId) { throw new BadRequest(`User already has an account.`); } diff --git a/src/services/passwordRecovery/index.js b/src/services/passwordRecovery/index.js index 38c0f5f5d46..6bd7a6e96b3 100644 --- a/src/services/passwordRecovery/index.js +++ b/src/services/passwordRecovery/index.js @@ -2,7 +2,6 @@ const { static: staticContent } = require('@feathersjs/express'); const path = require('path'); const passwordRecovery = require('./model'); -const AccountModel = require('../account/model'); const { ChangePasswordService, hooks: changePasswordServiceHooks } = require('./services/ChangePasswordService'); const { GenerateRecoveryPasswordTokenService, @@ -13,7 +12,7 @@ module.exports = function setup() { const app = this; app.use('/passwordRecovery/api', staticContent(path.join(__dirname, '/docs/openapi.yaml'))); - app.use('/passwordRecovery/reset', new ChangePasswordService(passwordRecovery, AccountModel)); + app.use('/passwordRecovery/reset', new ChangePasswordService(passwordRecovery, app)); app.use('/passwordRecovery', new GenerateRecoveryPasswordTokenService(passwordRecovery)); const passwordRecoveryService = app.service('/passwordRecovery'); diff --git a/src/services/passwordRecovery/services/ChangePasswordService.js b/src/services/passwordRecovery/services/ChangePasswordService.js index 851dd7494bf..370d04e1807 100644 --- a/src/services/passwordRecovery/services/ChangePasswordService.js +++ b/src/services/passwordRecovery/services/ChangePasswordService.js @@ -3,14 +3,14 @@ const local = require('@feathersjs/authentication-local'); const { BadRequest, GeneralError, SilentError } = require('../../../errors'); const logger = require('../../../logger/index'); const globalHooks = require('../../../hooks'); -const { ObjectId } = require('../../../helper/compare'); const MAX_LIVE_TIME = 6 * 60 * 60 * 1000; // 6 hours class ChangePasswordService { - constructor(passwordRecoveryModel, accountModel) { + constructor(passwordRecoveryModel, app) { this.passwordRecoveryModel = passwordRecoveryModel; - this.accountModel = accountModel; + this.app = app; + this.errors = { inputValidation: 'Malformed request body.', expired: 'Token expired!', @@ -39,7 +39,7 @@ class ChangePasswordService { } try { await Promise.all([ - this.accountModel.updateOne({ _id: pwrecover.account }, { $set: { password } }).lean().exec(), + this.app.service('nest-account-service').updatePassword(pwrecover.account, password), this.passwordRecoveryModel .updateOne({ token }, { $set: { changed: true } }) .lean() diff --git a/src/services/passwordRecovery/services/GenerateRecoveryPasswordTokenService.js b/src/services/passwordRecovery/services/GenerateRecoveryPasswordTokenService.js index 09fc32f5053..7a25f7def47 100644 --- a/src/services/passwordRecovery/services/GenerateRecoveryPasswordTokenService.js +++ b/src/services/passwordRecovery/services/GenerateRecoveryPasswordTokenService.js @@ -17,7 +17,7 @@ class GenerateRecoveryPasswordTokenService { } setup(app) { - this.accountsService = app.service('/accounts'); + this.accountsService = app.service('nest-account-service'); } async get(id) { @@ -29,16 +29,14 @@ class GenerateRecoveryPasswordTokenService { async create(data) { if (data && data.username) { const { username } = data; - const accounts = await this.accountsService.find({ - query: { - username, - }, - }); + const accountsResult = await this.accountsService.searchByUsernameExactMatch(username); + const { accounts, total } = accountsResult; - if (Array.isArray(accounts) && accounts.length !== 0 && accounts[0]._id) { - data.account = accounts[0]._id; + if (total !== 0 && Array.isArray(accounts) && accounts[0].id) { + data.account = accounts[0].id; + data.user = accounts[0].userId; const recoveryModel = await PasswordRecoveryModel.create({ - account: accounts[0]._id, + account: accounts[0].id, token: randomStringAsBase64Url(24), }); return recoveryModel; @@ -52,29 +50,25 @@ class GenerateRecoveryPasswordTokenService { const sendInfo = (context) => { if (context.path === 'passwordRecovery') { return context.app - .service('/accounts') - .get(context.data.account, { - query: { - $populate: ['userId'], - }, - }) - .then((account) => { + .service('/users') + .get(context.data.user) + .then((user) => { const recoveryLink = `${HOST}/pwrecovery/${context.result.token}`; - const mailContent = `Sehr geehrte/r ${account.userId.firstName} ${account.userId.lastName}, \n -Bitte setzen Sie Ihr Passwort unter folgendem Link zurück: -${recoveryLink}\n -Bitte beachten Sie das der Link nur für 6 Stunden gültig ist. Danach müssen sie ein neuen Link anfordern.\n -Mit Freundlichen Grüßen -Ihr ${SC_SHORT_TITLE} Team`; + const mailContent = `Sehr geehrte/r ${user.firstName} ${user.lastName}, \n + Bitte setzen Sie Ihr Passwort unter folgendem Link zurück: + ${recoveryLink}\n + Bitte beachten Sie das der Link nur für 6 Stunden gültig ist. Danach müssen sie ein neuen Link anfordern.\n + Mit Freundlichen Grüßen + Ihr ${SC_SHORT_TITLE} Team`; globalHooks.sendEmail(context, { subject: `Passwort zurücksetzen für die ${SC_SHORT_TITLE}`, - emails: [account.userId.email], + emails: [user.email], content: { text: mailContent, }, }); - logger.info(`send password recovery information to userId ${account.userId._id}`); + logger.info(`send password recovery information to userId ${user._id}`); return context; }) .catch((err) => { diff --git a/test/services/passwordRecovery/index.test.js b/test/services/passwordRecovery/index.test.js index 8672c220f2a..8def38ea07b 100644 --- a/test/services/passwordRecovery/index.test.js +++ b/test/services/passwordRecovery/index.test.js @@ -6,8 +6,6 @@ const testObjects = require('../helpers/testObjects')(appPromise()); const { setupNestServices, closeNestServices } = require('../../utils/setup.nest.services'); const passwordRecovery = require('../../../src/services/passwordRecovery/model'); -const PORT = 0; - describe('passwordRecovery service', () => { let app; let nestServices; @@ -27,9 +25,9 @@ describe('passwordRecovery service', () => { before(async () => { app = await appPromise(); - passwordRecoveryService = app.service('passwordRecovery'); - server = await app.listen(0); nestServices = await setupNestServices(app); + server = await app.listen(0); + passwordRecoveryService = app.service('passwordRecovery'); savedUser = await testObjects.createTestUser(); savedAccount = await testObjects.createTestAccount(newAccount, null, savedUser); await passwordRecoveryService.create({ username: recoveryUsername }); diff --git a/test/services/teams/helper/helper.user.js b/test/services/teams/helper/helper.user.js index 13d2b2081f8..e2b4ef09c8a 100644 --- a/test/services/teams/helper/helper.user.js +++ b/test/services/teams/helper/helper.user.js @@ -1,9 +1,8 @@ const { ObjectId } = require('mongoose').Types; const { BadRequest } = require('../../../../src/errors'); -const rolesModel = require('../../../../src/services/role/model.js'); +const rolesModel = require('../../../../src/services/role/model'); const { userModel } = require('../../../../src/services/user/model'); -const accountModel = require('../../../../src/services/account/model.js'); const appPromise = require('../../../../src/app'); const { TEST_PW, TEST_HASH } = require('../../../../config/globals'); @@ -57,13 +56,15 @@ const createUser = async (userId, roleName = 'student', schoolId = '5f2987e02083 }); }; -const createAccount = (userId) => - accountModel.create({ +const createAccount = async (userId) => { + const app = await appPromise(); + return app.service('nest-account-service').save({ username: userId + AT, password: TEST_HASH, userId, activated: true, }); +}; const setupUser = async (roleName, schoolId) => { const userId = new ObjectId(); @@ -81,9 +82,10 @@ const setupUser = async (roleName, schoolId) => { const deleteUser = async (userId) => { if (typeof userId === 'object' && userId.userId !== undefined) userId = userId.userId; + const app = await appPromise(); const email = userId + AT; await userModel.deleteOne({ email }); // todo: add error handling if not exist - await accountModel.deleteOne({ username: email }); + await app.service('nest-account-service').deleteByUserId(userId); }; module.exports = { diff --git a/test/services/teams/hooks/helpers.test.js b/test/services/teams/hooks/helpers.test.js index 9d62db2bd9b..3b8293d7cb5 100644 --- a/test/services/teams/hooks/helpers.test.js +++ b/test/services/teams/hooks/helpers.test.js @@ -13,18 +13,22 @@ const { } = require('../../../../src/services/teams/hooks/helpers.js'); const { teamUserModel } = require('../../../../src/services/teams/model'); +const { setupNestServices, closeNestServices } = require('../../../utils/setup.nest.services'); describe('hook helpers', () => { let app; let server; + let nestServices; before(async () => { app = await appPromise(); server = await app.listen(0); + nestServices = await setupNestServices(app); }); - after((done) => { - server.close(done); + after(async () => { + await server.close(); + await closeNestServices(nestServices); }); describe('ifSuperhero', () => {