feat: add authorization to user/order route #231

jannyHou · 2019-08-08T01:10:08Z

implements loopback features in loopbackio/loopback-next#1205 and loopbackio/loopback-next#2900

The PR serves as a PoC and feature to verify implementation of @loopback/authorization in the shopping app.

items:

- Try apply the casbin based authorization in the shopping example
- Figure out the responsibilities of authentication, authorization, authorizeInterceptor, authorizor, fixtures(casbin)
- Extract the common layer for authentication and authorization
- implement one endpoint Users/{userId}/orders with casbin policies , connect with loopback feature Refactor authorization loopback-next#3529
- ~~To be improved: casbin policies set fixed roles instead of checking the order's ownId on the fly.~~ Solved by adding a voter.
- add acceptance and unit tests

jannyHou · 2019-08-08T17:26:22Z

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

  async createOrder(
    @param.path.string('userId') userId: string,
    @requestBody() order: Order,
  ): Promise<Order> {
+    // should be moved to an authorizor
    if (userId !== order.userId) {


or use voter to decide.

raymondfeng · 2019-11-05T21:21:36Z

packages/shopping/package.json

    "pretest:ci": "npm run build",
+    "pretest": "npm run clean && npm run build && npm run docker:start",


Please remove && npm run docker:start

raymondfeng · 2019-11-05T21:22:04Z

packages/shopping/package.json

    "bcryptjs": "2.4.3",
+    "casbin": "^3.0.3",


Please remove ^ from all versions.

dhmlau · 2019-11-05T22:50:49Z

packages/shopping/src/services/authorizor.ts

+  }
+
+  async authorize(
+    // @jannyHou: make sure it's universal for authorizers


is it a reminder for something? Either way, I think it's the best to remove the @jannyHou. :)

Good catch...I think it's some notes from meeting, should be removed.

agreed, removing note

jannyHou

👍 Great progress! Mostly good, I left a few comments.

jannyHou · 2019-11-06T20:33:49Z

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

@@ -35,8 +42,11 @@ describe('UserOrderController acceptance tests', () => {
    const userId = user.id;
    const order = givenAOrder({userId: userId, orderId: '1'});

+    const token = await authenticateUser(user);


I think we can just generate the token once, then use it in all the it tests.

actually every test iteration calls givenAUser() to create a new user for every test. so a token has to be created every time for the new user.

jannyHou · 2019-11-06T20:35:41Z

packages/shopping/src/services/enforcer.ts

+    __dirname,
+    '../../fixtures/casbin/rbac_policy.csv',
+  );
+  return casbin.newEnforcer(conf, policy);


good catch, and we can turn createEnforcer into a sync function since casbin.newEnforcer() is sync.

casbin.newEnforcer returns a promise, so to resolve it createEnforcer has to be async. The reason I removed the await is because of lint errors and await is added in a return from an async function internally.

jannyHou · 2019-11-06T20:38:19Z

packages/shopping/src/services/jwt-service.ts

@@ -54,7 +58,7 @@ export class JWTService implements TokenService {
      );
    }
    const userInfoForToken = {
-      id: userProfile[securityId],
+      id: userProfile.id,


I think it should still be userProfile[securityId]

userProfile.id and userProfile[securityId] have same values.

depends on what is returned by user service

convertToUserProfile(user: User): UserProfile { // since first name and lastName are optional, no error is thrown if not provided let userName = ''; if (user.firstName) userName = `${user.firstName}`; if (user.lastName) userName = user.firstName ? `${userName} ${user.lastName}` : `${user.lastName}`; return {[securityId]: user.id, name: userName, id: user.id}; }

true in this case theoretically [securityId] always equals to id, but conceptually we should retrieve the value from a user profile's unique field, which is [securityId]

ok, I will correct this.

jannyHou

@deepakrkris almost there, a few more comments.

jannyHou · 2019-11-08T20:00:37Z

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

@@ -82,6 +91,8 @@ export class UserOrderController {
      },
    },
  })
+  @authenticate('jwt')
+  @authorize({resource: 'order', scopes: ['patch']})


jannyHou · 2019-11-08T20:00:44Z

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

@@ -66,6 +73,8 @@ export class UserOrderController {
      },
    },
  })
+  @authenticate('jwt')
+  @authorize({resource: 'order', scopes: ['find']})


can we also add voters: [compareId] for patch and find methods?

jannyHou · 2019-11-08T20:08:07Z

packages/shopping/src/services/jwt-service.ts

@@ -54,7 +58,7 @@ export class JWTService implements TokenService {
      );
    }
    const userInfoForToken = {
-      id: userProfile[securityId],
+      id: userProfile.id,


true in this case theoretically [securityId] always equals to id, but conceptually we should retrieve the value from a user profile's unique field, which is [securityId]

raymondfeng · 2019-11-09T06:15:59Z

packages/shopping/package.json

+    "@loopback/repository": "1.10.1",
+    "@loopback/rest": "1.16.6",
+    "@loopback/rest-explorer": "1.3.1",
+    "@loopback/security": "^0.1.6",


Please remove all leading ^ for version ranges.

@deepakrkris ^ +1 and please also resolve the conflicts for 2 package-lock.json files. LGTM after cleaning up the package.json file

jannyHou

@deepakrkris LGTM after cleaning up the package.json, package-lock.json files. Thank you!

jannyHou · 2019-11-11T20:16:43Z

packages/shopping/package.json

+    "@loopback/repository": "1.10.1",
+    "@loopback/rest": "1.16.6",
+    "@loopback/rest-explorer": "1.3.1",
+    "@loopback/security": "^0.1.6",


@deepakrkris ^ +1 and please also resolve the conflicts for 2 package-lock.json files. LGTM after cleaning up the package.json file

Signed-off-by: DEEPAK RAJAMOHAN <[email protected]>

jannyHou commented Aug 8, 2019

View reviewed changes

jannyHou changed the title ~~[WIP, early feedback are welcomed] feat: try authorization~~ [PoC] feat: try authorization Aug 9, 2019

This was referenced Aug 9, 2019

Refactor authorization loopbackio/loopback-next#3529

Merged

feat: add authorization component loopbackio/loopback-next#1205

Merged

jannyHou mentioned this pull request Sep 10, 2019

Apply authorization in the shopping example loopbackio/loopback-next#3695

Closed

4 tasks

deepakrkris force-pushed the test/authorization branch 6 times, most recently from a4f9c7e to 1d6cce9 Compare November 5, 2019 02:18

deepakrkris marked this pull request as ready for review November 5, 2019 02:20

deepakrkris requested review from emonddr and raymondfeng as code owners November 5, 2019 02:20

deepakrkris force-pushed the test/authorization branch 4 times, most recently from 7f27197 to 1bfb2ac Compare November 5, 2019 16:58

deepakrkris changed the title ~~[PoC] feat: try authorization~~ feat: add authorization to user order model Nov 5, 2019

deepakrkris force-pushed the test/authorization branch 2 times, most recently from 45d113e to 53604a9 Compare November 5, 2019 17:18

deepakrkris changed the title ~~feat: add authorization to user order model~~ feat: add authorization to user/order route Nov 5, 2019

deepakrkris requested review from dhmlau, bajtos, agnes512 and hacksparrow November 5, 2019 17:31

raymondfeng reviewed Nov 5, 2019

View reviewed changes

dhmlau reviewed Nov 5, 2019

View reviewed changes

jannyHou commented Nov 6, 2019

View reviewed changes

deepakrkris force-pushed the test/authorization branch 2 times, most recently from 2432c69 to 33451b1 Compare November 8, 2019 17:25

dhmlau added the Authorization label Nov 8, 2019

deepakrkris force-pushed the test/authorization branch from 33451b1 to 7a8cfce Compare November 8, 2019 18:53

jannyHou commented Nov 8, 2019

View reviewed changes

deepakrkris force-pushed the test/authorization branch 2 times, most recently from 0a27666 to f1136b7 Compare November 8, 2019 22:31

raymondfeng reviewed Nov 9, 2019

View reviewed changes

jannyHou commented Nov 11, 2019

View reviewed changes

deepakrkris force-pushed the test/authorization branch from f1136b7 to 0c12c45 Compare November 11, 2019 20:24

jannyHou and others added 5 commits November 11, 2019 12:24

feat: try authorization

5c9e197

Signed-off-by: DEEPAK RAJAMOHAN <[email protected]>

test: add tests for user

26078c4

Signed-off-by: DEEPAK RAJAMOHAN <[email protected]>

fix: update with the refactor pr

0356439

Signed-off-by: DEEPAK RAJAMOHAN <[email protected]>

fix: upgrade loopback packages

54ca74c

Signed-off-by: DEEPAK RAJAMOHAN <[email protected]>

fix: clear test cases

e3846bd

Signed-off-by: DEEPAK RAJAMOHAN <[email protected]>

deepakrkris force-pushed the test/authorization branch from 0c12c45 to e3846bd Compare November 11, 2019 20:29

raymondfeng approved these changes Nov 11, 2019

View reviewed changes

deepakrkris merged commit 11c48ef into master Nov 11, 2019

deepakrkris deleted the test/authorization branch November 11, 2019 22:24

raymondfeng mentioned this pull request Dec 4, 2019

docs: update authorization documentation loopbackio/loopback-next#4185

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add authorization to user/order route #231

feat: add authorization to user/order route #231

jannyHou commented Aug 8, 2019 •

edited by deepakrkris

Loading

jannyHou Aug 8, 2019

raymondfeng Nov 5, 2019

deepakrkris Nov 7, 2019

raymondfeng Nov 5, 2019

deepakrkris Nov 7, 2019

dhmlau Nov 5, 2019

jannyHou Nov 6, 2019

deepakrkris Nov 7, 2019

jannyHou left a comment

jannyHou Nov 6, 2019

deepakrkris Nov 7, 2019

jannyHou Nov 6, 2019

deepakrkris Nov 7, 2019

jannyHou Nov 6, 2019

deepakrkris Nov 7, 2019

deepakrkris Nov 7, 2019

jannyHou Nov 8, 2019

deepakrkris Nov 8, 2019

jannyHou left a comment

jannyHou Nov 8, 2019

deepakrkris Nov 8, 2019

jannyHou Nov 8, 2019

deepakrkris Nov 8, 2019

jannyHou Nov 8, 2019

raymondfeng Nov 9, 2019

jannyHou Nov 11, 2019

deepakrkris Nov 11, 2019

jannyHou left a comment

jannyHou Nov 11, 2019

		"pretest:ci": "npm run build",
		"pretest": "npm run clean && npm run build && npm run docker:start",

feat: add authorization to user/order route #231

feat: add authorization to user/order route #231

Conversation

jannyHou commented Aug 8, 2019 • edited by deepakrkris Loading

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

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

Choose a reason for hiding this comment

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

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

jannyHou commented Aug 8, 2019 •

edited by deepakrkris

Loading