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

Jump to bottom

feat: addition of current user authentication in the request to creat… #71

Closed
wants to merge 5 commits into from
Closed

feat: addition of current user authentication in the request to creat… #71

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d74ca34
feat: addition of current user authentication in the request to creat…
austin047 Mar 30, 2019
0d663c8
chore: upgrade all deps
raymondfeng Apr 5, 2019
6903c1c
refactor: Update user-order.controller.ts
austin047 Apr 9, 2019
a5bb9a3
chore: rebase code
jannyHou Apr 29, 2019
55854d1
feat: 🎸 Client can create an order without userId in the body
austin047 Jun 18, 2019
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
98 changes: 92 additions & 6 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

137 changes: 106 additions & 31 deletions packages/shopping/src/__tests__/acceptance/user-order.controller.acceptance.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@ import {OrderRepository, UserRepository} from '../../repositories';
import {MongoDataSource} from '../../datasources';
import {User, Order} from '../../models';
import {setupApplication} from './helper';
import {JWTAuthenticationService} from '../../src/services/JWT.authentication.service';
import {
PasswordHasherBindings,
JWTAuthenticationBindings,
} from '../../src/keys';

describe('UserOrderController acceptance tests', () => {
let app: ShoppingApplication;
Expand All @@ -26,42 +31,113 @@ describe('UserOrderController acceptance tests', () => {
await app.stop();
});

it('creates an order for a user with a given orderId', async () => {
const user = await givenAUser();
const userId = user.id.toString();
const order = givenAOrder({userId: userId, orderId: '1'});
describe('Creating new orders for authenticated users', () => {
let plainPassword: string;
let jwtAuthService: JWTAuthenticationService;

await client
.post(`/users/${userId}/orders`)
.send(order)
.expect(200, order);
});
const user = {
email: '[email protected]',
password: 'p4ssw0rd',
firstname: 'Example',
surname: 'User',
};

it('creates an order for a user without a given orderId', async () => {
const user = await givenAUser();
const userId = user.id.toString();
const order = givenAOrder({userId: userId});
before('create new user for user orders', async () => {
app.bind(PasswordHasherBindings.ROUNDS).to(4);

const res = await client
.post(`/users/${userId}/orders`)
.send(order)
.expect(200);
const passwordHasher = await app.get(
PasswordHasherBindings.PASSWORD_HASHER,
);
plainPassword = user.password;
user.password = await passwordHasher.hashPassword(user.password);
jwtAuthService = await app.get(JWTAuthenticationBindings.SERVICE);
});

expect(res.body.orderId).to.be.a.String();
delete res.body.orderId;
expect(res.body).to.deepEqual(order);
});
it('creates an order for a user with a given orderId', async () => {
const newUser = await userRepo.create(user);
const userId = newUser.id.toString();
const order = givenAOrder({userId: userId, orderId: '1'});
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: givenAnOrder


const token = await jwtAuthService.getAccessTokenForUser({
email: newUser.email,
password: plainPassword,
});

await client
.post(`/users/${userId}/orders`)
.send(order)
.set('Authorization', 'Bearer ' + token)
.expect(200, order);
});

it('creates an order for a user without a given orderId', async () => {
const newUser = await userRepo.create(user);
const userId = newUser.id.toString();

const token = await jwtAuthService.getAccessTokenForUser({
email: newUser.email,
password: plainPassword,
});

const order = givenAOrder({userId: userId});
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason not provide the orderId here?

Copy link
Member

Choose a reason for hiding this comment

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

Ideally, I'd like the server to fill userId so that the clients making HTTP requests to create new orders don't have to. I.e. this test should pass with const order = givenAnOrder() too.


const res = await client
.post(`/users/${userId}/orders`)
.send(order)
.set('Authorization', 'Bearer ' + token)
.expect(200);

it('throws an error when a userId in path does not match body', async () => {
const user = await givenAUser();
const userId = user.id.toString();
const order = givenAOrder({userId: 'hello'});
expect(res.body.orderId).to.be.a.String();
delete res.body.orderId;
expect(res.body).to.deepEqual(order);
});

it('throws an error when a userId in path does not match body', async () => {
const newUser = await userRepo.create(user);
const userId = newUser.id.toString();

const token = await jwtAuthService.getAccessTokenForUser({
email: newUser.email,
password: plainPassword,
});

const order = givenAOrder({userId: 'hello'});

await client
.post(`/users/${userId}/orders`)
.set('Content-Type', 'application/json')
.set('Authorization', 'Bearer ' + token)
.send(order)
.expect(400);
});

it('throws an error when a user is not authenticated', async () => {
const newUser = await userRepo.create(user);
const userId = newUser.id.toString();

const order = givenAOrder({userId: userId});

await client
.post(`/users/${userId}/orders`)
.set('Content-Type', 'application/json')
.send(order)
.expect(400);
await client
.post(`/users/${userId}/orders`)
.send(order)
.expect(401);
});

it('throws an error when a user with wrong token is provided', async () => {
const newUser = await userRepo.create(user);
const userId = newUser.id.toString();

const order = givenAOrder({userId: userId});

await client
.post(`/users/${userId}/orders`)
.send(order)
.set(
'Authorization',
'Bearer ' + 'Wrong token - IjoidGVzdEBsb29wYmFjay5p',
)
.expect(401);
});
});

// TODO(virkt25): Implement after issue below is fixed.
Expand Down Expand Up @@ -128,7 +204,6 @@ describe('UserOrderController acceptance tests', () => {
firstname: 'Example',
surname: 'User',
};

return await userRepo.create(user);
}

Expand Down
12 changes: 12 additions & 0 deletions packages/shopping/src/controllers/user-order.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import {
HttpErrors,
} from '@loopback/rest';
import {Order} from '../models';
import {authenticate, UserProfile} from '@loopback/authentication';
import {inject} from '@loopback/core';

/**
* Controller for User's Orders
Expand All @@ -35,10 +37,20 @@ export class UserOrderController {
},
},
})
@authenticate('jwt')
async createOrder(
@param.path.string('userId') userId: string,
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove the userId argument please, we should take the id from currentUser.

@inject('authentication.currentUser') currentUser: UserProfile,
@requestBody() order: Order,
): Promise<Order> {
if (currentUser.id !== order.userId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The rewrite here is different from the expected behaviour:

The expected requirement mentioned here is don't take in userId from request:

@authenticate('jwt')
async createOrder(
  @inject('authentication.currentUser') currentUser: UserProfile,
  @requestBody() order: Order,
) {
  if (!currentUser.id) {
    throw new HttpErrors.BadRequest(`No user logged in.`);
  }
  // The rest of code that creates orders for the logged in user.
}

But your logic which compares the logged in user id and the id from query also makes sense to me.

If aligning with the original requirements costs too much effort I am ok with the approach here.

Any thought @strongloop/loopback-next ?

Copy link

Choose a reason for hiding this comment

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

+1. Agree. No user id in request.

Copy link
Member

Choose a reason for hiding this comment

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

Yes please, we should really remove userId argument from this controller method.

In the implementation proposed by this pull request, there are too many places where user id can be specified:

  • userId as a path parameter
  • order.userId from request body
  • currentUser.id from the authentication layer

throw new HttpErrors.BadRequest(
`User id does not match looged in user: ${userId} !== ${
currentUser.id
}`,
);
}

if (userId !== order.userId) {
throw new HttpErrors.BadRequest(
`User id does not match: ${userId} !== ${order.userId}`,
Expand Down