Refactor Shopping Example: simulate ACLs with LB4 authorization

[raymondfeng]

Describe how to simulate ACLs in Shopping Example

Acceptance criteria

• Illustrate usage of @authorize to add role based access control
• Create an pseudo Authorizer that implements ACL enforcement similar to https://github.com/strongloop/loopback-next/blob/master/examples/access-control-migration
• Create an pseudo Authorizer/Voter that implements custom LB3 role resolver
• Refactor for changes in Extract the jwt authentication to a local component for the auth migration example app #4753
  Extract the jwt authentication to a local component

[deepakrkris]

changing the scope and acceptance criteria to fit Shopping example, because ALL of the action items were accomplished by #4706 and #4571

[dhmlau]

@deepakrkris, since this is originally a migration guide task, I'd like to understand more on the acceptance criteria change. Here are my thoughts:

• If all the criteria are met by PR docs: access control migration #4706 and feat: add access control migration app #4571, and because it's not a migration/migration guide related task, I'd defer to Q2.

• I thought we already have role based access control in the shopping example. Why do we need to refactor it? I understand after the jwt authentication component refactoring is done by @jannyHou, we'd need to make some changes accordingly in the shopping app.

• about the 3rd criteria:

  Create an pseudo Authorizer/Voter that implements custom LB3 role resolver

  Should this be covered in the migration guide or already done?

[deepakrkris]

• I thought we already have role based access control in the shopping example. Why do we need to refactor it? I understand after the jwt authentication component refactoring is done by @jannyHou, we'd need to make some changes accordingly in the shopping app.

@dhmlau the shopping app has only a basic authorizer , not a casbin based rbac authorizer
https://github.com/strongloop/loopback4-example-shopping/blob/master/packages/shopping/src/services/basic.authorizor.ts

[deepakrkris]

Create an pseudo Authorizer/Voter that implements custom LB3 role resolver
Should this be covered in the migration guide or already done?

@dhmlau this is already done . https://github.com/strongloop/loopback-next/blob/master/examples/access-control-migration/src/services/assign-project-instance-id.voter.ts

[deepakrkris]

• If all the criteria are met by PR docs: access control migration #4706 and feat: add access control migration app #4571, and because it's not a migration/migration guide related task, I'd defer to Q2.

@dhmlau , yes the shopping example tasks can be deferred, there were few follow ups discussed in the migration PR https://github.com/strongloop/loopback-next/pull/4571/files#r381350460 , https://github.com/strongloop/loopback-next/pull/4571/files#r383709072 by @emonddr and @bajtos .May be we can prioritize them ?

[dhmlau]

@deepakrkris, thanks for your info.

1. the shopping app has only a basic authorizer , not a casbin based rbac authorizer

Seems like we're going around circles about having casbin in the first place, then removing it, and we want to add it back again. :). Not your fault, just pointing out the fact. :)
Either way (basic or rbac authorizer) is fine with me, as long as we have docs covering the other one.
Would like to get some inputs from @strongloop/loopback-maintainers on showcasing basic vs rbac authorizer in shopping app.

2. If Create an pseudo Authorizer/Voter that implements custom LB3 role resolver is already done, we can check that off from the acceptance criteria?

3. yes the shopping example tasks can be deferred, there were few follow ups discussed in the migration PR https://github.com/strongloop/loopback-next/pull/4571/files#r381350460 , https://github.com/strongloop/loopback-next/pull/4571/files#r383709072 by @emonddr and @bajtos .May be we can prioritize them ?

The links only show the files, do you see particular comment(s) that we need to have follow up discussions? Could you please coordinate with @emonddr and @bajtos for those follow up questions (possibly opening a new GH issues)? thanks!

[dhmlau]

From what you described, @deepakrkris, let's continue the discussion here on the next steps, and for now, I'll defer this task from Q1, since it is no longer a migration guide related issue. Thanks!

[deepakrkris]

@emonddr from your comment in https://github.com/strongloop/loopback-next/pull/4571/files#r381350460

right now, the database id for a user is : 1,2,3, etc. But if I store stuff in Mongodb it will be a long uuid or guid, so you would need u0209b175-b39c-4dd4-a158-4dc81547e77e in your casbin file. And if ever users are deleted and added to a database again, these guid values will be different, and so you will need to change your casbin files accordingly.
...... 
and in real database these users would have ID values like a guid or long int.

So what I am saying is that we should have a function that takes the securityId and queries the database for the unique userid, and this userid is used by cabin policy decisions.

@emonddr @jannyHou , do you think we can prioritize this for the migration example app in Q1 ?

[deepakrkris]

@jannyHou from @bajtos comment https://github.com/strongloop/loopback-next/pull/4571/files#r383709072 👍

we can create a new section "Known limitations" where we can list all issues we are aware of

I see one limitation, the current way of casbin configuration in this example will explode with a growing number of APIs and more importantly with "data" (when additional projects are added). Usually API access is grouped with scopes and rbac checks are done against the scopes. Data level restrictions are
not configured, they are programmed.

so instead of,

p, p1_owner, project1, show-balance
p, p1_owner, project1, donate
p, p1_owner, project1, withdraw
g, u1, p1_owner
p, p2_owner, project2, show-balance
p, p2_owner, project2, donate
p, p2_owner, project2, withdraw
g, u2, p2_owner

we could have,

g, u1, owner
g, u2, owner
p, owner, project, view-private
p, owner, project, deposit
p, owner, project, withdraw

and check for project1 specific access from the database with a ""owner column.

[jannyHou]

@dhmlau @deepakrkris I think, since we already have an example app(access-control-migration) that ONLY focuses on and demos the authentication and authorization, I would suggest use it as much as possible in the auth migration docs.
Personally I would not make the shopping example's auth system more complicated, as long as its functions perform well, I tend to keep the shopping example as it is.

For this story, I think sth we can do is:

• explain the difference of ACL in LB3 and LB4
  • I have a comparison in comment, but I am not super expert with the LB3 auth system, so don't have too much info for ACL, it's sth @deepakrkris could take a more detailed look
  • LB3: has its own predefined fields to describe the access control of endpoints, which is understood by LB3 authorization system.
  • LB4: the descriptive rules are defined by developers according to their implementation of the authorizers, and provided as auth metadata in @authorize
• refactor the corresponding lb4 acl entries into a separate file, see code
• update the example's tutorial to reflect ^ change. should be a minor change.

To summarize, we can first explain the concept differences. And since we already have example defining the ACL based on a user implemented authorization system, let's use it whenever needs scenario/code explanation to support the concept.

WDYT? And if we agree to not touch the shopping example, maybe bring back the original description and update based on it?

[jannyHou]

@deepakrkris for #4522 (comment), sorry I haven't got a chance to create follow-up story for it. If you can add the limitations section in this story that would be great 👍

[stale]

This issue has been marked stale because it has not seen activity within six months. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository. This issue will be closed within 30 days of being stale.

[stale]

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.