From 19d6c9265e46eb185c8600617134835985797082 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Thu, 14 Nov 2019 12:04:39 +0100 Subject: [PATCH] feat: extract local credentials into a new model MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduce `UserCredentials` models to hold hashed passwords, add has-one relation from `User` to `UserCredentials`. Rework authentication-related code to work with the new domain model. Signed-off-by: Miroslav Bajtoš --- .../acceptance/authentication.acceptance.ts | 36 ++++++------ .../acceptance/authorization.acceptance.ts | 22 ++++---- .../user-order.controller.acceptance.ts | 19 ++++--- .../acceptance/user.controller.acceptance.ts | 28 +++++----- .../src/controllers/user.controller.ts | 55 +++++++++++++++---- packages/shopping/src/models/index.ts | 1 + .../src/models/user-credentials.model.ts | 40 ++++++++++++++ packages/shopping/src/models/user.model.ts | 12 ++-- packages/shopping/src/repositories/index.ts | 1 + .../user-credentials.repository.ts | 14 +++++ .../src/repositories/user.repository.ts | 32 ++++++++++- .../shopping/src/services/user-service.ts | 11 +++- packages/shopping/src/services/validator.ts | 2 +- 13 files changed, 201 insertions(+), 72 deletions(-) create mode 100644 packages/shopping/src/models/user-credentials.model.ts create mode 100644 packages/shopping/src/repositories/user-credentials.repository.ts diff --git a/packages/shopping/src/__tests__/acceptance/authentication.acceptance.ts b/packages/shopping/src/__tests__/acceptance/authentication.acceptance.ts index 4234756a9..4c3c98c4e 100644 --- a/packages/shopping/src/__tests__/acceptance/authentication.acceptance.ts +++ b/packages/shopping/src/__tests__/acceptance/authentication.acceptance.ts @@ -18,17 +18,19 @@ import { import {setupApplication} from './helper'; import {TokenService, UserService} from '@loopback/authentication'; import {securityId} from '@loopback/security'; +import * as _ from 'lodash'; describe('authentication services', () => { let app: ShoppingApplication; - const user = { + const userData = { email: 'unittest@loopback.io', - password: 'p4ssw0rd', firstName: 'unit', lastName: 'test', }; + const userPassword = 'p4ssw0rd'; + let newUser: User; let jwtService: TokenService; let userService: UserService; @@ -70,19 +72,15 @@ describe('authentication services', () => { it('user service verifyCredentials() succeeds', async () => { const {email} = newUser; - const credentials = {email, password: user.password}; + const credentials = {email, password: userPassword}; const returnedUser = await userService.verifyCredentials(credentials); // create a copy of returned user without password field - const returnedUserWithOutPassword = Object.assign({}, returnedUser, { - password: user.password, - }); - delete returnedUserWithOutPassword.password; + const returnedUserWithOutPassword = _.omit(returnedUser, 'password'); // create a copy of expected user without password field - const expectedUserWithoutPassword = Object.assign({}, newUser); - delete expectedUserWithoutPassword.password; + const expectedUserWithoutPassword = _.omit(newUser, 'password'); expect(returnedUserWithOutPassword).to.deepEqual( expectedUserWithoutPassword, @@ -178,21 +176,21 @@ describe('authentication services', () => { }); it('password encrypter hashPassword() succeeds', async () => { - const encrypedPassword = await bcryptHasher.hashPassword(user.password); - expect(encrypedPassword).to.not.equal(user.password); + const encrypedPassword = await bcryptHasher.hashPassword(userPassword); + expect(encrypedPassword).to.not.equal(userPassword); }); it('password encrypter compare() succeeds', async () => { - const encrypedPassword = await bcryptHasher.hashPassword(user.password); + const encrypedPassword = await bcryptHasher.hashPassword(userPassword); const passwordsAreTheSame = await bcryptHasher.comparePassword( - user.password, + userPassword, encrypedPassword, ); expect(passwordsAreTheSame).to.be.True(); }); it('password encrypter compare() fails', async () => { - const encrypedPassword = await bcryptHasher.hashPassword(user.password); + const encrypedPassword = await bcryptHasher.hashPassword(userPassword); const passwordsAreTheSame = await bcryptHasher.comparePassword( 'someotherpassword', encrypedPassword, @@ -208,12 +206,14 @@ describe('authentication services', () => { async function createUser() { bcryptHasher = await app.get(PasswordHasherBindings.PASSWORD_HASHER); - const encryptedPassword = await bcryptHasher.hashPassword(user.password); - newUser = await userRepo.create( - Object.assign({}, user, {password: encryptedPassword}), - ); + const encryptedPassword = await bcryptHasher.hashPassword(userPassword); + newUser = await userRepo.create(userData); // MongoDB returns an id object we need to convert to string newUser.id = newUser.id.toString(); + + await userRepo.userCredentials(newUser.id).create({ + password: encryptedPassword, + }); } async function clearDatabase() { diff --git a/packages/shopping/src/__tests__/acceptance/authorization.acceptance.ts b/packages/shopping/src/__tests__/acceptance/authorization.acceptance.ts index 56adea34d..90ca69e44 100644 --- a/packages/shopping/src/__tests__/acceptance/authorization.acceptance.ts +++ b/packages/shopping/src/__tests__/acceptance/authorization.acceptance.ts @@ -16,12 +16,13 @@ describe('authorization', () => { let client: Client; let userRepo: UserRepository; - let user = { + let userData = { email: 'testAuthor@loopback.io', - password: 'p4ssw0rd', firstName: 'customer_service', }; + const userPassword = 'p4ssw0rd'; + let passwordHasher: PasswordHasher; let newUser: User; let token: string; @@ -54,7 +55,7 @@ describe('authorization', () => { let res = await client .post('/users/login') - .send({email: newUser.email, password: user.password}) + .send({email: newUser.email, password: userPassword}) .expect(200); token = res.body.token; @@ -82,9 +83,8 @@ describe('authorization', () => { describe('bob', () => { it('allows bob create orders', async () => { - user = { + userData = { email: 'test2@loopback.io', - password: 'p4ssw0rd', firstName: 'bob', }; newUser = await createAUser(); @@ -102,7 +102,7 @@ describe('authorization', () => { let res = await client .post('/users/login') - .send({email: newUser.email, password: user.password}) + .send({email: newUser.email, password: userPassword}) .expect(200); token = res.body.token; @@ -145,13 +145,15 @@ describe('authorization', () => { } async function createAUser() { - const encryptedPassword = await passwordHasher.hashPassword(user.password); - const aUser = await userRepo.create( - Object.assign({}, user, {password: encryptedPassword}), - ); + const encryptedPassword = await passwordHasher.hashPassword(userPassword); + const aUser = await userRepo.create(userData); + // MongoDB returns an id object we need to convert to string aUser.id = aUser.id.toString(); + await userRepo.userCredentials(aUser.id).create({ + password: encryptedPassword, + }); return aUser; } diff --git a/packages/shopping/src/__tests__/acceptance/user-order.controller.acceptance.ts b/packages/shopping/src/__tests__/acceptance/user-order.controller.acceptance.ts index b40737029..a1e50bc71 100644 --- a/packages/shopping/src/__tests__/acceptance/user-order.controller.acceptance.ts +++ b/packages/shopping/src/__tests__/acceptance/user-order.controller.acceptance.ts @@ -16,10 +16,11 @@ describe('UserOrderController acceptance tests', () => { const userData = { email: 'testUserCtrl@loopback.io', - password: 'p4ssw0rd', firstName: 'customer_service', }; + const userPassword = 'p4ssw0rd'; + before('setupApplication', async () => { ({app, client} = await setupApplication()); }); @@ -158,22 +159,24 @@ describe('UserOrderController acceptance tests', () => { const passwordHasher = await app.get( PasswordHasherBindings.PASSWORD_HASHER, ); - const encryptedPassword = await passwordHasher.hashPassword( - userData.password, - ); + const encryptedPassword = await passwordHasher.hashPassword(userPassword); + + const newUser = await userRepo.create(userData); - const newUser = await userRepo.create( - Object.assign({}, userData, {password: encryptedPassword}), - ); // MongoDB returns an id object we need to convert to string newUser.id = newUser.id.toString(); + + await userRepo.userCredentials(newUser.id).create({ + password: encryptedPassword, + }); + return newUser; } async function authenticateUser(user: User) { const res = await client .post('/users/login') - .send({email: user.email, password: userData.password}); + .send({email: user.email, password: userPassword}); const token = res.body.token; diff --git a/packages/shopping/src/__tests__/acceptance/user.controller.acceptance.ts b/packages/shopping/src/__tests__/acceptance/user.controller.acceptance.ts index 4092afd42..b0cff0ed9 100644 --- a/packages/shopping/src/__tests__/acceptance/user.controller.acceptance.ts +++ b/packages/shopping/src/__tests__/acceptance/user.controller.acceptance.ts @@ -27,13 +27,14 @@ describe('UserController', () => { let userRepo: UserRepository; - const user = { + const userData = { email: 'test@loopback.io', - password: 'p4ssw0rd', firstName: 'Example', lastName: 'User', }; + const userPassword = 'p4ssw0rd'; + let passwordHasher: PasswordHasher; let expiredToken: string; @@ -54,7 +55,7 @@ describe('UserController', () => { it('creates new user when POST /users is invoked', async () => { const res = await client .post('/users') - .send(user) + .send({...userData, password: userPassword}) .expect(200); // Assertions @@ -122,11 +123,11 @@ describe('UserController', () => { it('throws error for POST /users with an existing email', async () => { await client .post('/users') - .send(user) + .send({...userData, password: userPassword}) .expect(200); const res = await client .post('/users') - .send(user) + .send({...userData, password: userPassword}) .expect(409); expect(res.body.error.message).to.equal('Email value is already taken'); @@ -134,7 +135,6 @@ describe('UserController', () => { it('returns a user with given id when GET /users/{id} is invoked', async () => { const newUser = await createAUser(); - delete newUser.password; delete newUser.orders; await client.get(`/users/${newUser.id}`).expect(200, newUser.toJSON()); @@ -146,7 +146,7 @@ describe('UserController', () => { const res = await client .post('/users/login') - .send({email: newUser.email, password: user.password}) + .send({email: newUser.email, password: userPassword}) .expect(200); const token = res.body.token; @@ -158,7 +158,7 @@ describe('UserController', () => { const res = await client .post('/users/login') - .send({email: 'idontexist@example.com', password: user.password}) + .send({email: 'idontexist@example.com', password: userPassword}) .expect(401); expect(res.body.error.message).to.equal('Invalid email or password.'); @@ -180,7 +180,7 @@ describe('UserController', () => { let res = await client .post('/users/login') - .send({email: newUser.email, password: user.password}) + .send({email: newUser.email, password: userPassword}) .expect(200); const token = res.body.token; @@ -280,13 +280,15 @@ describe('UserController', () => { } async function createAUser() { - const encryptedPassword = await passwordHasher.hashPassword(user.password); - const newUser = await userRepo.create( - Object.assign({}, user, {password: encryptedPassword}), - ); + const encryptedPassword = await passwordHasher.hashPassword(userPassword); + const newUser = await userRepo.create(userData); // MongoDB returns an id object we need to convert to string newUser.id = newUser.id.toString(); + await userRepo.userCredentials(newUser.id).create({ + password: encryptedPassword, + }); + return newUser; } diff --git a/packages/shopping/src/controllers/user.controller.ts b/packages/shopping/src/controllers/user.controller.ts index 4a309e38e..92d8afe30 100644 --- a/packages/shopping/src/controllers/user.controller.ts +++ b/packages/shopping/src/controllers/user.controller.ts @@ -3,9 +3,16 @@ // This file is licensed under the MIT License. // License text available at https://opensource.org/licenses/MIT -import {repository} from '@loopback/repository'; +import {repository, model, property} from '@loopback/repository'; import {validateCredentials} from '../services/validator'; -import {post, param, get, requestBody, HttpErrors} from '@loopback/rest'; +import { + post, + param, + get, + requestBody, + HttpErrors, + getModelSchemaRef, +} from '@loopback/rest'; import {User, Product} from '../models'; import {UserRepository} from '../repositories'; import {RecommenderService} from '../services/recommender.service'; @@ -31,6 +38,15 @@ import { import * as _ from 'lodash'; import {OPERATION_SECURITY_SPEC} from '../utils/security-spec'; +@model() +export class NewUserRequest extends User { + @property({ + type: 'string', + required: true, + }) + password: string; +} + export class UserController { constructor( @repository(UserRepository) public userRepository: UserRepository, @@ -58,18 +74,37 @@ export class UserController { }, }, }) - async create(@requestBody() user: User): Promise { + async create( + @requestBody({ + content: { + 'application/json': { + schema: getModelSchemaRef(NewUserRequest, { + title: 'NewUser', + exclude: ['id'], + }), + }, + }, + }) + newUserRequest: Omit, + ): Promise { // ensure a valid email value and password value - validateCredentials(_.pick(user, ['email', 'password'])); + validateCredentials(_.pick(newUserRequest, ['email', 'password'])); // encrypt the password - // eslint-disable-next-line require-atomic-updates - user.password = await this.passwordHasher.hashPassword(user.password); + const password = await this.passwordHasher.hashPassword( + newUserRequest.password, + ); try { // create the new user - const savedUser = await this.userRepository.create(user); - delete savedUser.password; + const savedUser = await this.userRepository.create( + _.omit(newUserRequest, 'password'), + ); + + // set the password + await this.userRepository + .userCredentials(savedUser.id) + .create({password}); return savedUser; } catch (error) { @@ -97,9 +132,7 @@ export class UserController { }, }) async findById(@param.path.string('userId') userId: string): Promise { - return this.userRepository.findById(userId, { - fields: {password: false}, - }); + return this.userRepository.findById(userId); } @get('/users/me', { diff --git a/packages/shopping/src/models/index.ts b/packages/shopping/src/models/index.ts index ec549e5ca..b767b3c21 100644 --- a/packages/shopping/src/models/index.ts +++ b/packages/shopping/src/models/index.ts @@ -8,3 +8,4 @@ export * from './shopping-cart-item.model'; export * from './shopping-cart.model'; export * from './order.model'; export * from './product.model'; +export * from './user-credentials.model'; diff --git a/packages/shopping/src/models/user-credentials.model.ts b/packages/shopping/src/models/user-credentials.model.ts new file mode 100644 index 000000000..af87ead54 --- /dev/null +++ b/packages/shopping/src/models/user-credentials.model.ts @@ -0,0 +1,40 @@ +// Copyright IBM Corp. 2018. All Rights Reserved. +// Node module: loopback4-example-shopping +// This file is licensed under the MIT License. +// License text available at https://opensource.org/licenses/MIT + +import {Entity, model, property} from '@loopback/repository'; + +@model() +export class UserCredentials extends Entity { + @property({ + type: 'string', + id: true, + mongodb: {dataType: 'ObjectID'}, + }) + id: string; + + @property({ + type: 'string', + required: true, + }) + password: string; + + @property({ + type: 'string', + required: true, + mongodb: {dataType: 'ObjectID'}, + }) + userId: string; + + constructor(data?: Partial) { + super(data); + } +} + +export interface UserCredentialsRelations { + // describe navigational properties here +} + +export type UserCredentialsWithRelations = UserCredentials & + UserCredentialsRelations; diff --git a/packages/shopping/src/models/user.model.ts b/packages/shopping/src/models/user.model.ts index 2235a0cf6..44248e530 100644 --- a/packages/shopping/src/models/user.model.ts +++ b/packages/shopping/src/models/user.model.ts @@ -3,8 +3,9 @@ // This file is licensed under the MIT License. // License text available at https://opensource.org/licenses/MIT -import {Entity, model, property, hasMany} from '@loopback/repository'; +import {Entity, model, property, hasMany, hasOne} from '@loopback/repository'; import {Order} from './order.model'; +import {UserCredentials} from './user-credentials.model'; @model({ settings: { @@ -33,12 +34,6 @@ export class User extends Entity { }) email: string; - @property({ - type: 'string', - required: true, - }) - password: string; - @property({ type: 'string', }) @@ -52,6 +47,9 @@ export class User extends Entity { @hasMany(() => Order) orders: Order[]; + @hasOne(() => UserCredentials) + userCredentials: UserCredentials; + constructor(data?: Partial) { super(data); } diff --git a/packages/shopping/src/repositories/index.ts b/packages/shopping/src/repositories/index.ts index b110fe06a..93d4b7520 100644 --- a/packages/shopping/src/repositories/index.ts +++ b/packages/shopping/src/repositories/index.ts @@ -6,3 +6,4 @@ export * from './user.repository'; export * from './shopping-cart.repository'; export * from './order.repository'; +export * from './user-credentials.repository'; diff --git a/packages/shopping/src/repositories/user-credentials.repository.ts b/packages/shopping/src/repositories/user-credentials.repository.ts new file mode 100644 index 000000000..cc4237c8b --- /dev/null +++ b/packages/shopping/src/repositories/user-credentials.repository.ts @@ -0,0 +1,14 @@ +import {DefaultCrudRepository} from '@loopback/repository'; +import {UserCredentials, UserCredentialsRelations} from '../models'; +import {MongoDataSource} from '../datasources'; +import {inject} from '@loopback/core'; + +export class UserCredentialsRepository extends DefaultCrudRepository< + UserCredentials, + typeof UserCredentials.prototype.id, + UserCredentialsRelations +> { + constructor(@inject('datasources.mongo') dataSource: MongoDataSource) { + super(UserCredentials, dataSource); + } +} diff --git a/packages/shopping/src/repositories/user.repository.ts b/packages/shopping/src/repositories/user.repository.ts index 9f8215343..167d9235f 100644 --- a/packages/shopping/src/repositories/user.repository.ts +++ b/packages/shopping/src/repositories/user.repository.ts @@ -8,10 +8,12 @@ import { juggler, HasManyRepositoryFactory, repository, + HasOneRepositoryFactory, } from '@loopback/repository'; -import {User, Order} from '../models'; -import {inject} from '@loopback/core'; +import {User, Order, UserCredentials} from '../models'; +import {inject, Getter} from '@loopback/core'; import {OrderRepository} from './order.repository'; +import {UserCredentialsRepository} from './user-credentials.repository'; export type Credentials = { email: string; @@ -24,14 +26,40 @@ export class UserRepository extends DefaultCrudRepository< > { public orders: HasManyRepositoryFactory; + public readonly userCredentials: HasOneRepositoryFactory< + UserCredentials, + typeof User.prototype.id + >; + constructor( @inject('datasources.mongo') protected datasource: juggler.DataSource, @repository(OrderRepository) protected orderRepository: OrderRepository, + @repository.getter('UserCredentialsRepository') + protected userCredentialsRepositoryGetter: Getter< + UserCredentialsRepository + >, ) { super(User, datasource); + this.userCredentials = this.createHasOneRepositoryFactoryFor( + 'userCredentials', + userCredentialsRepositoryGetter, + ); this.orders = this.createHasManyRepositoryFactoryFor( 'orders', async () => orderRepository, ); } + + async findCredentials( + userId: typeof User.prototype.id, + ): Promise { + try { + return this.userCredentials(userId).get(); + } catch (err) { + if (err.code === 'ENTITY_NOT_FOUND') { + return undefined; + } + throw err; + } + } } diff --git a/packages/shopping/src/services/user-service.ts b/packages/shopping/src/services/user-service.ts index 8578cce12..b1995d2a1 100644 --- a/packages/shopping/src/services/user-service.ts +++ b/packages/shopping/src/services/user-service.ts @@ -25,13 +25,20 @@ export class MyUserService implements UserService { const foundUser = await this.userRepository.findOne({ where: {email: credentials.email}, }); - if (!foundUser) { throw new HttpErrors.Unauthorized(invalidCredentialsError); } + + const credentialsFound = await this.userRepository.findCredentials( + foundUser.id, + ); + if (!credentialsFound) { + throw new HttpErrors.Unauthorized(invalidCredentialsError); + } + const passwordMatched = await this.passwordHasher.comparePassword( credentials.password, - foundUser.password, + credentialsFound.password, ); if (!passwordMatched) { diff --git a/packages/shopping/src/services/validator.ts b/packages/shopping/src/services/validator.ts index e82885fe0..608449a6f 100644 --- a/packages/shopping/src/services/validator.ts +++ b/packages/shopping/src/services/validator.ts @@ -14,7 +14,7 @@ export function validateCredentials(credentials: Credentials) { } // Validate Password Length - if (credentials.password.length < 8) { + if (!credentials.password || credentials.password.length < 8) { throw new HttpErrors.UnprocessableEntity( 'password must be minimum 8 characters', );