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

austin047 · 2019-03-30T02:48:05Z

…e a new order

Updated the create order method that handles post request to create a user order to authenticate
against the current user before an order can be created, the client will have to pass the
authentication token in the request header which will be authenticated before the order is created

BREAKING CHANGE: For an order to be created the client must pass the authentication token in the
request header, otherwise a 401 Unauthorized error will be generated

feat #43

emonddr · 2019-04-01T13:14:23Z

src/controllers/user-order.controller.ts

    @requestBody() order: Order,
  ): Promise<Order> {
+    if (currentUser.id !== userId) {
+      throw new HttpErrors.BadRequest(
+        `User id does not match looged in user: ${userId} !== ${


User id does not match looged in user:

should be

User id does not match logged in user:

I have done the changes.

…e a new order Updated the create order method that handles post request to create a user order to authenticate against the current user before an order can be created, the client will have to pass the authentication token in the request header which will be authenticated before the order is created BREAKING CHANGE: For an order to be created the client must pass the authentication token in the request header, otherwise a 401 Unauthorized error will be generated feat loopbackio#43 Signed-off-by: austin047 <fuhaustin@gmail.com>

Signed-off-by: Raymond Feng <enjoyjava@gmail.com>

Signed-off-by: austin047 <fuhaustin@gmail.com>

Signed-off-by: jannyHou <juehou@ca.ibm.com>

jannyHou

@austin047 Thank you so much for adding the authentication for creating the orders of the logged in user.

I left a few comments and rebased your branch(including some trivial change in the last commit to get tests pass). Let me know if you have any questions.

And would you like to rewrite endpoint GET /orders (returns all orders of the current user) as well?
It's ok to leave it out of the scope of this PR, depends on you 🙂

jannyHou · 2019-04-29T20:20:33Z

packages/shopping/src/controllers/user-order.controller.ts

    @requestBody() order: Order,
  ): Promise<Order> {
+    if (currentUser.id !== order.userId) {


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 ?

+1. Agree. No user id in request.

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

jannyHou · 2019-04-29T20:30:19Z

packages/shopping/src/__tests__/acceptance/user-order.controller.acceptance.ts

+    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'});


nitpick: givenAnOrder

jannyHou · 2019-04-29T20:34:10Z

packages/shopping/src/__tests__/acceptance/user-order.controller.acceptance.ts

+        password: plainPassword,
+      });
+
+      const order = givenAOrder({userId: userId});


any reason not provide the orderId here?

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.

dhmlau · 2019-06-10T20:06:50Z

@austin047, there are a few review comments from the maintainers. Are you interested to continue to work on this PR? Thanks!

austin047 · 2019-06-16T12:25:40Z

Yes, I will work on it as soon as possible.

Client can create an order without passing the userId in the body of the request, the serve collects the userId from the url and compares against the authenticated user(Current User) and the creates the order for that user. ✅ Closes: loopbackio#43 refactor: 💡 remove debug console logs Remove console logs created for debug purposes Signed-off-by: Fuh Austin <fuhaustin@gmail.com>

bajtos · 2019-06-20T11:35:37Z

packages/shopping/src/controllers/user-order.controller.ts

@@ -35,13 +37,29 @@ export class UserOrderController {
      },
    },
  })
+  @authenticate('jwt')
  async createOrder(
    @param.path.string('userId') userId: string,


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

dougal83 · 2020-02-22T21:51:08Z

Hey @austin047 Are you interested in continuing work on this PR? I'll happily complete the work if you would prefer. Thanks.

dougal83 · 2020-03-29T14:27:44Z

@jannyHou @hacksparrow should this PR be closed as inactive?

jannyHou · 2020-03-31T16:17:56Z

@dougal83 Thank you for checking it, yes I think after refactoring the shopping app, all orders endpoints are secured in https://github.com/strongloop/loopback4-example-shopping/blob/master/packages/shopping/src/controllers/user-order.controller.ts

To continue(or open a new PR), are you planning to remove the /{userId} from all paths?

And really appreciate @austin047 's effort on updating it after auth was first added in this example. There are several breaking changes made since then.

dougal83 · 2020-03-31T16:23:39Z

@jannyHou Probably easier to close this PR and start afresh from the current master. If you could create the story/issue with help wanted tag I'll pick it up when I have a moment unless someone beats me to it. Thank you.

jannyHou · 2020-04-02T16:27:34Z

@dougal83 Thank you created story in #629
Based on the chat I am going to close this PR. Let's keep discussion in #629.
And apologize to @austin047 again for unable to merge his code 😞 we appreciate your effort spending on kicking off this refactor 💪

austin047 force-pushed the user-order-authentication-with-jwt branch from 32bfd3d to 71b5f5f Compare March 30, 2019 02:49

emonddr reviewed Apr 1, 2019

View reviewed changes

dhmlau mentioned this pull request Apr 9, 2019

The First Scenario: Authenticated orders (a minimal authentication) loopbackio/loopback-next#1998

Closed

austin047 force-pushed the user-order-authentication-with-jwt branch 9 times, most recently from b26c70e to 7da2091 Compare April 10, 2019 00:52

dhmlau added community-contribution Authentication labels Apr 16, 2019

dhmlau mentioned this pull request Apr 18, 2019

Refactor shopping app to use the updated authentication package #79

Closed

2 tasks

austin047 and others added 3 commits April 29, 2019 15:17

chore: upgrade all deps

0d663c8

Signed-off-by: Raymond Feng <enjoyjava@gmail.com>

refactor: Update user-order.controller.ts

6903c1c

Signed-off-by: austin047 <fuhaustin@gmail.com>

jannyHou force-pushed the user-order-authentication-with-jwt branch from 7da2091 to 6903c1c Compare April 29, 2019 19:27

chore: rebase code

a5bb9a3

Signed-off-by: jannyHou <juehou@ca.ibm.com>

jannyHou reviewed Apr 29, 2019

View reviewed changes

austin047 requested a review from raymondfeng as a code owner June 19, 2019 14:17

austin047 force-pushed the user-order-authentication-with-jwt branch 5 times, most recently from da2c61e to 4020dc2 Compare June 19, 2019 14:37

austin047 force-pushed the user-order-authentication-with-jwt branch from 4020dc2 to 55854d1 Compare June 19, 2019 20:56

bajtos reviewed Jun 20, 2019

View reviewed changes

jannyHou mentioned this pull request Apr 2, 2020

Remove path parameter {userid} from order endpoints #629

Closed

2 tasks

jannyHou closed this Apr 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

austin047 commented Mar 30, 2019

emonddr Apr 1, 2019

austin047 Apr 9, 2019

jannyHou left a comment

jannyHou Apr 29, 2019

samarpanB Apr 30, 2019

bajtos May 6, 2019

jannyHou Apr 29, 2019

jannyHou Apr 29, 2019

bajtos May 6, 2019

dhmlau commented Jun 10, 2019

austin047 commented Jun 16, 2019

bajtos Jun 20, 2019

dougal83 commented Feb 22, 2020

dougal83 commented Mar 29, 2020

jannyHou commented Mar 31, 2020

dougal83 commented Mar 31, 2020

jannyHou commented Apr 2, 2020

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

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

Conversation

austin047 commented Mar 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhmlau commented Jun 10, 2019

austin047 commented Jun 16, 2019

Choose a reason for hiding this comment

dougal83 commented Feb 22, 2020

dougal83 commented Mar 29, 2020

jannyHou commented Mar 31, 2020

dougal83 commented Mar 31, 2020

jannyHou commented Apr 2, 2020