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.

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

docs: access control migration #4706

Merged
merged 1 commit into from
Mar 2, 2020
Merged

Conversation

jannyHou
Copy link
Contributor

@jannyHou jannyHou commented Feb 21, 2020

This is the doc for #4520, it helps review feat PR #4571

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

Copy link
Contributor

@agnes512 agnes512 left a comment

Choose a reason for hiding this comment

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

Thanks for the writeup ❤️! I think the structure is good. Just have some nitpicks.

examples/access-control-migration/README.md Outdated Show resolved Hide resolved
examples/access-control-migration/README.md Outdated Show resolved Hide resolved
examples/access-control-migration/README.md Outdated Show resolved Hide resolved
examples/access-control-migration/README.md Outdated Show resolved Hide resolved
examples/access-control-migration/README.md Outdated Show resolved Hide resolved
examples/access-control-migration/README.md Outdated Show resolved Hide resolved
examples/access-control-migration/README.md Outdated Show resolved Hide resolved
examples/access-control-migration/README.md Outdated Show resolved Hide resolved
examples/access-control-migration/README.md Outdated Show resolved Hide resolved

![casbin-model-policy](./imgs/casbin-model-policy.png)

Based on the screenshot, suppose u2(user with id 2) makes a request to
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe wrap u2, p1_team, etc. here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, same as #4706 (comment)

@jannyHou jannyHou force-pushed the doc/example/acl-migration branch 2 times, most recently from c423a4e to 33b401b Compare February 24, 2020 23:12
@jannyHou jannyHou changed the title [WIP, help review #4571]docs: access control migration docs: access control migration Feb 24, 2020
@jannyHou
Copy link
Contributor Author

@agnes512 Thank you for providing the feedback so fast! I added all the reference and the doc is now complete 📝 (some content is waiting for final decision/approve of the feat PR). I will address your comments asap.


# App Scenario

This example is a donation system. Users can view or make donations to projects
Copy link
Member

Choose a reason for hiding this comment

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

In the original loopback-example-access control README, it has a pretty good description of what the scenario is. Do you want to reuse that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reused it

(Project, Team, User). And we add one more model UserCredentials to separate the
sensitive information from the User model.

You can run `lb4 model` to create the 4 models, and run `lb4 repository` to
Copy link
Member

Choose a reason for hiding this comment

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

Instead of creating the models from scratch, is it possible to run lb4 import-lb3-models to import the LB3 models?
https://loopback.io/doc/en/lb4/Importing-LB3-models.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The User model is simplified in the migrated application, and the credentials are also extracted into a separate module.

Only Project and Team can be migrated.
I haven't tried that command and my example app is not created using it, can I keep the content as it is now then open a new PR to try the model migration? If I need to update the example code, will also do it together.

Copy link
Member

Choose a reason for hiding this comment

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

open a new PR to make use the lb4 import-lb3-models sounds good to me. 👍 I'd prefer to do it right away though, because it will make the migration process cleaner. Thanks.


## Try Out

(a question for reviewers: do you prefer to see all the screenshots in this
Copy link
Member

Choose a reason for hiding this comment

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

IMHO, it would be good to have a screenshot for the first one to show where the token can be found and also the Authorize button. No need to show screenshots for all roles though.


Try role 'admin':

- Login as admin first (user Bob)
Copy link
Member

Choose a reason for hiding this comment

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

The screenshot(s) i have in mind:

  • under the users/login endpoint, it shows:
    • the request body json
    • the token as the response
  • Authorize button
    • (optional) shows what it looks like after clicking the Authorize button

Suggestion: We could possibly highlight the portion of the screenshot with red rectangle, annotated with numbers that shows the sequence of actions that you're describing in text.


## References

### Model Creation
Copy link
Member

Choose a reason for hiding this comment

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

Ideally, we can use the lb4 import-lb3-models.


## Set up authentication

This demo uses token based authentication, and uses the jwt authentication
Copy link
Member

Choose a reason for hiding this comment

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

perhaps I don't know enough of LB3 or the access-control example... Where in https://github.com/strongloop/loopback-example-access-control indicates it's using JWT authentication or is it by default that JWT authentication is used when authentication is enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

created story in #4753

example seeds data in the boot script, now they are migrated to an observer file
called `sample.observer.ts`.

_Since the data are already generated in `db.json`, that observer file is
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 for #4571 (comment), see the Seed Data section here. The observer is automatically skipped.

Copy link
Contributor

@emonddr emonddr left a comment

Choose a reason for hiding this comment

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

Great job, Janny. Very detailed migration guide.
Just several tiny typos to fix.


This example is a donation system. Users can view or make donations to projects
based on their roles. A project is owned by a user, and a user can create a team
to involve other users as team members. The system has an administration.
Copy link
Contributor

Choose a reason for hiding this comment

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

has an administration.

has an administrator ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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


The diagram for models also include the pre-created data in this application.

For example, u1 owns project1, it also creates a team with u1 and u2 as members.
Copy link
Contributor

Choose a reason for hiding this comment

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

For example, u1 owns project1, it also creates a team with u1 and u2 as members.

In this example, u1 owns project1, and u1 created a team with u1 and u2 as members.

# Difference

LoopBack 3 has several built-in models that consists of a RBAC system. The
models are User, Role, RoleMapping, ACL, AccessToken. You can learn how they
Copy link
Contributor

Choose a reason for hiding this comment

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

ACL, AccessToken.

ACL, and AccessToken.

Last item in a list should get an 'and'.

work together in tutorial
[Controlling data access](https://loopback.io/doc/en/lb3/Controlling-data-access.html).

LoopBack 4 authorization system gives developers the flexibility to implement
Copy link
Contributor

Choose a reason for hiding this comment

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

LoopBack 4 authorization system

The LoopBack 4 authorization system


# Steps with Casbin

Next let's migrate the LoopBack 3 access control example to LoopBack 4. Here is
Copy link
Contributor

Choose a reason for hiding this comment

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

Next let's migrate
Let's migrate

Copy link
Contributor

Choose a reason for hiding this comment

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

Place Here is an overview of the steps: on its own line. It seems far away from bullet list

- add a new user to a team
- find the projects owned by the team owner, then create role inherit rules
g, u${id}, project${id}\_team
- add a new endpoint(operation)
Copy link
Contributor

Choose a reason for hiding this comment

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

add a new endpoint(operation)

adding a new endpoint(operation)

_Since the data are already generated in `db.json`, that observer file is
skipped by default._

## Try Out
Copy link
Contributor

Choose a reason for hiding this comment

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

Try Out

Try It Out

- Run `npm start` to start the application.
- Open the explorer

Try role 'admin':
Copy link
Contributor

Choose a reason for hiding this comment

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

Try the 'admin' role:

- Try the 5 endpoints, 'show-balance' and 'withdraw' will return 401, others
succeed

Try role 'owner':
Copy link
Contributor

Choose a reason for hiding this comment

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

Try the 'owner' role:

- Click authorize button and paste the token
- Try the 5 endpoints, 'view-all' will return 401, others succeed

Try role 'team-member':
Copy link
Contributor

Choose a reason for hiding this comment

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

Try the 'team-member' role:

@dhmlau dhmlau added this to the Feb 2020 milestone Feb 26, 2020
@jannyHou jannyHou force-pushed the example/acl-migration branch 2 times, most recently from bd5ee68 to 76bb1e6 Compare February 26, 2020 23:19
Copy link
Contributor

@nabdelgadir nabdelgadir left a comment

Choose a reason for hiding this comment

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

Can you add the new docs file to the sidebar?

docs/site/migration/auth/example.md Show resolved Hide resolved
Comment on lines 32 to 33
Which means u1 is the **owner** of project1, and u1 and u2 are the **team
members** of project1. And u3 is the **admin**.
Copy link
Contributor

Choose a reason for hiding this comment

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

Which means u1 is the **owner** of project1, and u1 and u2 are the **team members** of project1. And u3 is the **admin**. -> This means u1 is the **owner** of project1, u1 and u2 are the **team members** of project1, and u3 is the **admin**.

Comment on lines 39 to 40
work together in tutorial
[Controlling data access](https://loopback.io/doc/en/lb3/Controlling-data-access.html).
Copy link
Contributor

Choose a reason for hiding this comment

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

in tutorial [Controlling data access](https://loopback.io/doc/en/lb3/Controlling-data-access.html) -> in the [Controlling data access](https://loopback.io/doc/en/lb3/Controlling-data-access.html) tutorial

the role mapping.

This guide includes a demo for using [casbin](#steps-with-casbin) and using
[oauth0](#steps-with-oauth0)(TBD)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the (TBD) here for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This example uses casbin as the 3rd party lib for access control, in the future we will explorer using oauth0 as the lib.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is #steps-with-casbin supposed to link to Migrating Example to LoopBack 4 with Casbin? And since #steps-with-oauth0 doesn't exist (in this PR), can you remove the link to it?

docs/site/migration/auth/example.md Outdated Show resolved Hide resolved
and uses the authentication and authorization system in LoopBack 4 to implement
the access control.

# App Scenario
Copy link
Contributor

Choose a reason for hiding this comment

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

The headings should start with the second-level ones # -> ## (so shift all of them by one)

and `teamMember` is defined and registered in
[role-resolver](https://github.com/strongloop/loopback-example-access-control/blob/master/server/boot/role-resolver.js).

In the migrated application, we use a 3rd-party library
Copy link
Contributor

Choose a reason for hiding this comment

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

3rd-party -> third-party

docs/site/migration/auth/example.md Show resolved Hide resolved
@jannyHou jannyHou changed the base branch from example/acl-migration to master February 27, 2020 16:14
@jannyHou jannyHou force-pushed the doc/example/acl-migration branch 2 times, most recently from a76154f to 1fc0251 Compare February 28, 2020 18:57

The LoopBack 4 authorization system gives developers the flexibility to
implement the RBAC on their own. You can leverage popular third-party libraries
like [casbin](https://github.com/casbin/casbin) or [oauth0](https://auth0.com/)
Copy link
Contributor

Choose a reason for hiding this comment

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

oauth0 -> oAuth0.

Copy link
Member

Choose a reason for hiding this comment

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

From that web site, it refers as Auth0. Should we use that instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah double checked, I think Auth0 is correct. oAuth is used for 2.0

for the role mapping.

This guide includes a demo for using [casbin](#steps-with-casbin) and using
[oauth0](#steps-with-oauth0)(TBD)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

@jannyHou jannyHou force-pushed the doc/example/acl-migration branch 2 times, most recently from c02c77b to 674239f Compare February 28, 2020 21:23
Copy link
Contributor

@nabdelgadir nabdelgadir left a comment

Choose a reason for hiding this comment

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

The rest of my comments were just stylistic so this LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants