Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: hash the password #37

Merged
merged 1 commit into from
Jan 31, 2019
Merged

fix: hash the password #37

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions src/utils/user.authentication.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import {promisify} from 'util';
import * as isemail from 'isemail';
import {HttpErrors} from '@loopback/rest';
import {UserProfile} from '@loopback/authentication';
import {compare} from 'bcryptjs';
const compareAsync = promisify(compare);
const jwt = require('jsonwebtoken');
const signAsync = promisify(jwt.sign);
const verifyAsync = promisify(jwt.verify);
Expand All @@ -19,10 +21,19 @@ export async function getAccessTokenForUser(
credentials: Credentials,
): Promise<string> {
const foundUser = await userRepository.findOne({
where: {email: credentials.email, password: credentials.password},
where: {email: credentials.email},
});
if (!foundUser) {
throw new HttpErrors.Unauthorized('Wrong credentials!');
throw new HttpErrors['NotFound'](
`User with email ${credentials.email} not found.`,
);
}
const passwordMatched = await compareAsync(
credentials.password,
foundUser.password,
);
if (!passwordMatched) {
throw new HttpErrors.Unauthorized('The credentials are not correct.');
}

const currentUser = _.pick(toJSON(foundUser), ['id', 'email', 'firstName']);
Expand All @@ -49,6 +60,8 @@ export function validateCredentials(credentials: Credentials) {
}

// secret should be injected
// the refactor PR is in progress
// https://github.com/strongloop/loopback4-example-shopping/pull/33
export async function decodeAccessToken(
token: string,
secret: string,
Expand Down
27 changes: 18 additions & 9 deletions test/acceptance/user.controller.acceptance.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,11 @@ import {setupApplication} from './helper';
import {createRecommendationServer} from '../../recommender';
import {Server} from 'http';
import * as _ from 'lodash';
import {promisify} from 'util';
import {hash} from 'bcryptjs';
import {getAccessTokenForUser} from '../../src/utils/user.authentication';
const recommendations = require('../../recommender/recommendations.json');
const hashAsync = promisify(hash);

describe('UserController', () => {
let app: ShoppingApplication;
Expand Down Expand Up @@ -135,36 +138,42 @@ describe('UserController', () => {
});

describe('authentication functions', () => {
// TODO: fix storing the plain password in the following issue:
// https://github.com/strongloop/loopback-next/issues/1996
let plainPassword: string;

before('create new user', async () => {
plainPassword = user.password;
// Salt + Hash Password
user.password = await hashAsync(user.password, 10);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be the responsibility of the controller/repository to hash passwords?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jannyHou and I were talking about a hash utility function which can be called from the user repo or controller. I think it makes sense for the repository to call it when storing user passwords in the backend, but I see how that can be delegated to a controller too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@raymondfeng @b-admike My opinion is unless we define a specific repository interface for model User and add a function for the hashing purpose, I tend to put the logic in controller. And as @b-admike said, we can extract it into utility/service that can be called from the controller.
Like what we have in authentication utils.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I plan to refactor them into the util in the next PR, this one focus on fixing the plain password.

Explained in the PR description :)

A follow up PR for #26
And connect to the first acceptance criteria in #35 (there will be another PR after it to refactor the hash utils into a proper place.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jannyHou Please do extract this code into utility/service that can be called from multiple places. For example tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bajtos See the refactor PR #41

});

it('login returns a valid token', async () => {
const newUser = await userRepo.create(user);
await client
.post('/users/login')
.send({email: newUser.email, password: newUser.password})
.send({email: newUser.email, password: plainPassword})
.expect(200)
.then(getToken);
.then(verifyToken);

function getToken(res: Response) {
function verifyToken(res: Response) {
const token = res.body.token;
expect(token).to.not.be.empty();
}
});

it('login returns an error when invalid credentials are used', async () => {
// tslint:disable-next-line:no-unused
const newUser = await userRepo.create(user);
newUser.password = 'wrong password';
await client
.post('/users/login')
.send({email: newUser.email, password: newUser.password})
.send({email: user.email, password: 'wrongpassword'})
.expect(401);
});

it('/me returns the current user', async () => {
const newUser = await userRepo.create(user);
const token = await getAccessTokenForUser(userRepo, {
email: newUser.email,
password: newUser.password,
password: plainPassword,
});

newUser.id = newUser.id.toString();
Expand All @@ -180,7 +189,7 @@ describe('UserController', () => {
const newUser = await userRepo.create(user);
await getAccessTokenForUser(userRepo, {
email: newUser.email,
password: newUser.password,
password: plainPassword,
});

await client.get('/users/me').expect(401);
Expand Down
62 changes: 51 additions & 11 deletions test/unit/utils.authentication.unit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,14 @@ import {
import {UserRepository, OrderRepository} from '../../src/repositories';
import {User} from '../../src/models';
import * as _ from 'lodash';
import {promisify} from 'util';
import {hash} from 'bcryptjs';
import {JsonWebTokenError} from 'jsonwebtoken';
import {HttpErrors} from '@loopback/rest';
const hashAsync = promisify(hash);
const SECRET = 'secretforjwt';

describe('authentication', () => {
describe('authentication utilities', () => {
const mongodbDS = new MongoDataSource();
const orderRepo = new OrderRepository(mongodbDS);
const userRepo = new UserRepository(mongodbDS, orderRepo);
Expand All @@ -26,11 +31,42 @@ describe('authentication', () => {
};
let newUser: User;

before('create user', async () => {
newUser = await userRepo.create(user);
before(clearDatabase);
before(createUser);

it('getAccessTokenForUser creates valid jwt access token', async () => {
const token = await getAccessTokenForUser(userRepo, {
email: '[email protected]',
password: 'p4ssw0rd',
});
expect(token).to.not.be.empty();
});

it('decodes valid access token', async () => {
it('getAccessTokenForUser rejects non-existing user with error Not Found', async () => {
const expectedError = new HttpErrors['NotFound'](
`User with email [email protected] not found.`,
);
return expect(
getAccessTokenForUser(userRepo, {
email: '[email protected]',
password: 'fake',
}),
).to.be.rejectedWith(expectedError);
});

it('getAccessTokenForUser rejects wrong credential with error Unauthorized', async () => {
const expectedError = new HttpErrors.Unauthorized(
'The credentials are not correct.',
);
return expect(
getAccessTokenForUser(userRepo, {
email: '[email protected]',
password: 'fake',
}),
).to.be.rejectedWith(expectedError);
});

it('decodeAccessToken decodes valid access token', async () => {
const token = await getAccessTokenForUser(userRepo, {
email: '[email protected]',
password: 'p4ssw0rd',
Expand All @@ -40,15 +76,19 @@ describe('authentication', () => {
expect(currentUser).to.deepEqual(expectedUser);
});

it('throws error for invalid accesstoken', async () => {
it('decodeAccessToken throws error for invalid accesstoken', async () => {
const token = 'fake';
try {
await decodeAccessToken(token, SECRET);
expect('throws error').to.be.true();
} catch (err) {
expect(err.message).to.equal('jwt malformed');
}
const error = new JsonWebTokenError('jwt malformed');
return expect(decodeAccessToken(token, SECRET)).to.be.rejectedWith(error);
});

async function createUser() {
user.password = await hashAsync(user.password, 10);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jannyHou
In the test suite, please use the smallest number that's still supported by bcrypt - we use 4 in LoopBack 3.x

The higher the number is, the longer it takes to compute the hash. Slow hash computation is good in production as it makes brute-force attacks difficult, but also impractical in tests because it can significantly slow down the test suite. We have been through this in LoopBack 3.x, let's not repeat that mistake again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bajtos Thanks for sharing the experience and explain the reason behind using the smallest number!

In the test suite, please use the smallest number that's still supported by bcrypt

Will do it in the next PR that extracts hash functions into utils.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bajtos fixed in https://github.com/strongloop/loopback4-example-shopping/pull/41/files#diff-40bd51c7c6fe0f144ef2295401976579R20

newUser = await userRepo.create(user);
}
async function clearDatabase() {
await userRepo.deleteAll();
}
});

function getExpectedUser(originalUser: User) {
Expand Down