-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Improve authorization docs #4405
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for improving the docs. I have some comments especially on the diagrams. Thanks.
packages/authorization/README.md
Outdated
@@ -28,26 +16,105 @@ npm install --save @loopback/authorization | |||
|
|||
## Basic use | |||
|
|||
Start by decorating your controller methods with `@authorize` to require the | |||
request to be authorized. | |||
The following example shows the basic use of authorize decorator, authorizer and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: maybe authorize decorator
-> @authorize
decorator ?
The following example shows the basic use of authorize decorator, authorizer and | ||
authorization component by authorizing a client according to its role: | ||
|
||
ASSUMING your app uses jwt as the authentication strategy, and the user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ASSUMING
-> Assuming
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, I was intended to emphasize the assumption, that's why I make them all upper case
|
||
In this example, we make the user profile available via dependency injection | ||
using a key available from `@loopback/authorization` package. | ||
First **define `role` as a property in your User model** so that after a user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would using a numbered list be better than "first, then, ... finally"?
|
||
Here is the use case and diagram for the example: | ||
|
||
![Use case](imgs/use-case.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put the "endpoint" and "controller method" as text, and added your diagram to demo the use case
packages/authorization/README.md
Outdated
|
||
![Use case](imgs/use-case.png) | ||
|
||
![Example diagram](imgs/example-diagram.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would certainly be helpful to include some explanation on what each box means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intended to keep the title in each box brief :) Added explanation: The diagram shows authorization artifacts' responsibilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good after addressing Diana's feedback.
93148b0
to
cdb6c13
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the code snippet under "Summary and Diagram" section, there's a special character between the 2 decorators. Could you please look into it? Other than that, LGTM.
@authenticate(‘jwt’)�@authorize({allowRoles: ['ADMIN']})
cdb6c13
to
829342c
Compare
829342c
to
e9fc52b
Compare
Clarify the readme file with a basic example
Base on the discussion in #4291, the basic use example in the readme file is too simple. This PR aims to improve it.
See my comment in #4291 (comment)
Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated👉 Check out how to submit a PR 👈