-
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
docs: update docs on authentication package #2977
Conversation
This is a work on progress and is not ready for review. Just saving my work for today. |
bfda07d
to
5a283fb
Compare
I've taken a first pass at explaining the authentication component's main pieces, and what a developer has to do for authentication. I haven't |
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.
Looking good!
My review comments are more from the new user's perspective because I'm not familiar with what had been implemented. :)
Besides the review comments, I think it might be useful to have a diagram to show the relationship among the authentication component/strategy(and interface)/provider.
constructor(options?: ApplicationConfig) { | ||
super(options); | ||
|
||
... |
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.
@bajtos suggested, at some point, that we should make the ts
code snippet a valid one. so, my suggested change would be:
// ...
instead of simply
...
## Authentication Component | ||
|
||
To utilize `authentication` in an application `application.ts`, you must load | ||
the authentication component named `AuthenticationComponent`. |
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.
Apologize that I'm not familiar with this feature. So the name has to be AuthenticationComponent
? That means we cannot have more than one Authentication component?
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.
Apologize that I'm not familiar with this feature. So the name has to be
AuthenticationComponent
? That means we cannot have more than one Authentication component?
So most of the logic of @loopback/authentication ( the authentication metadata provider, the authentication action provider, and the authentication strategy provider ) is available in a component named AuthenticationComponent
, and must be loaded by an application.
The user needs to create and register their own custom authentication strategies (user can have many of these), and implement their own token service or user service etc, and create a custom sequence that places the authentication action in a specific place.
But the core authentication framework logic
is stored in 1 component.
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.
@dhmlau ^^ :)
|
||
As you can see in the example, an authentication strategy can inject custom | ||
services to help it accomplish certain tasks. A complete example of a **basic** | ||
authentication strategy can be found |
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.
Suggested change (see if it makes sense to you):
See the complete example for basic authentication strategy and jwt authentication strategy
[Authentication Decorator](https://loopback.io/doc/en/lb4/Decorators_authenticate.html) | ||
on your controller endpoints. | ||
|
||
They decorator's syntax is : |
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.
typo: They -> The
@authenticate(strategyName: string, options?: Object) | ||
``` | ||
|
||
The strategy `name` is **mandatory** and the `options` object is **optional**. |
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 think it's kind of understood and no need for this paragraph?
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 think it's kind of understood and no need for this paragraph?
You are right. Sometimes I like writing for the lowest common denominator (somebody new to TypeScript). I will remove it.
An example of the decorator when options **are** specified looks like this: | ||
|
||
```ts | ||
@authenticate('basic', { propertyOne : "foo", propertyTwo:"bar"}) |
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: do you have some more realistic values that our users might want to use the options for?
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: do you have some more realistic values that our users might want to use the options for?
Not really because we don't ship any authentication strategies at all ( just the interface). I could put a made up options object { useCookie: true}
or {session:true}
or { enforceLongPassword: true}
but this would give them the wrong impression that our interface supports this kind of option. @jannyHou @raymondfeng , what are your thoughts on this?
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 was also searching for some meaningful options for the basic auth in passport-http
, but unfortunately didn't find any...like what Dom said, they are all related to the passport
middleware instead of the strategy itself.
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.
got it. maybe at least mention those options are specific to the strategy the user select?
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 was also searching for some meaningful options for the basic auth in passport-http, but unfortunately didn't find any...like what Dom said, they are all related to the passport middleware instead of the strategy itself.
Actually, the focus of the discussion is not on passport stuff in this discussion. The @authenticate(strategyName, options?)
decorator allows for passing in options
. This gives the developer of custom authentication strategies a way to pass options in...if needed.
(Buy Janny is correct that the passport-based api have options they can pass in too...)
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.
We can use something as follows:
@authenticate('basic', { /* some options for the strategy */})
|
||
## Adding an Authentication Action to a Sequence | ||
|
||
In a LoopBack 4 REST application, each request passes through a stateless |
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.
does it only applicable to LB4 application with REST server? I find it a bit strange to refer it as "LoopBack 4 REST application".
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.
@dhmlau , I see what you mean. A user can create a gRPC server I guess, and not a typical REST API server which uses HTTP requests. Are people using LoopBack for more than REST servers at the moment?
I will remove the word 'REST' in this instance.
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 think they can, right? @raymondfeng
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.
@dhmlau The default sequence mentioned here is for REST server only.
And please note that in the AuthenticationStrategy
interface, the Request
type is imported from @loopback/rest
, see https://github.com/strongloop/loopback-next/blob/master/packages/authentication/src/types.ts#L7
So...just speak for myself :-p mentioning REST application
is ok to me here.
Those services like UserService
and TokenService
are reusable for other clients, but the authentication component does target on the REST requests.
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.
The sequence so far only applies to RestServer. Maybe we can say In a LoopBack 4 application with REST API endpoints
.
2e31957
to
60e06bb
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.
The documentation part looks perfect to me 🙂 I don't have anything else to add, just 2 tiny comments.
And for the diagram, the content looks good, a few suggestion:
- A more accurate type would be
Promise<UserProfile>
:
- nitpick: 'some' --> 'amodel' or 'somemodel'. Just a suggestion based on personally preferred naming conversion.
- Maybe re-organize the rectangles in a more sorted way?
The decorator's syntax is : | ||
|
||
```ts | ||
@authenticate(strategyName: string, options?: Object) |
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.
Object
--> object
:)
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.
@jannyHou thanks for your feedback.
A more accurate type would be Promise
When creating this diagram, space is very limited, so I chose to avoid using stuff like:
this.
and Promise< >
to save space, and give a general idea of the logic. The documentation text in the sections below the diagram will show the correct code.
nitpick: 'some' --> 'amodel' or 'somemodel'. Just a suggestion based on personally preferred naming conversion.
The lb4 controller
CLI creates controller files is this format: someName.controller.ts
. And all the artifacts follow the same pattern. I think my filename is ok.
Maybe re-organize the rectangles in a more sorted way?
It took a long time to create the diagram and all its pieces and labels and move them into place, so I am reluctant to iteratively move all the PowerPoint shapes/labels/arrows around unless
someone sends me a pencil and paper picture
of a re-design.
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.
When creating this diagram, space is very limited, so I chose to avoid using stuff like:
this. and Promise< > to save space
👍 Fair enough.
And the naming and diagram comments are just nitpicks 🙂 feel free to ignore.
``` | ||
|
||
{% include tip.html content=" | ||
To avoid repeating the same options in the <b>@authenticate</b> decorator for many endpoints in a controller, you can instead define global options which can be injected into an authentication strategy thereby allowing you to avoid specifying the options object in the decorator itself. For controller endpoints that need to override a global option, you can specify it in an options object passed into the decorator. Your authentication strategy would need to handle the option overrides. |
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 be good if we can show a code example here. While I also realized we don't have an auth option sample yet, maybe use the greeter extension https://github.com/strongloop/loopback-next/tree/master/examples/greeter-extension#implement-an-extension ?
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.
@jannyHou ^^ ,
Can we point to a unit test
that passes in options? https://github.com/strongloop/loopback-next/blob/master/packages/authentication/src/__tests__/unit/providers/auth-metadata.provider.unit.ts#L16
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.
@emonddr Cool the unit test is much more relevant. Looks good to me.
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.
@jannyHou , I've changed my mind about pointing to the unit test. It doesn't really show global options being overridden by request
options from the decorator. Perhaps after this PR lands, we can have a follow up PR to add a section about injecting global options into the strategy, and then the strategy checking for overriding options from the decorator and then overriding the global options. I have an example of this I can dig up from one of my workspaces. But I won't be able to prepare it nicely today for @raymondfeng to review.
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.
@emonddr Sure it would be good to have an example and point to it. +1 for improving it in another PR.
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.
Just some nitpicks but 👍. When you're linking to another docs on the website, if you use {file-name}.md
, then, in vscode, it goes to the file instead of the website so that's why I recommend changing those (there are more of them in the file that I didn't comment on).
This document describes the details of the LoopBack 4 `Authentication` | ||
component. | ||
|
||
![authentication_overview](./imgs/authentication_overview.png) Basically, to |
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: can you add a new line between the image and paragraph?
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.
Perhaps we should have a whiteboard session
and see how we can move between highlevel and lowlevel diagrams and when...
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's just a suggestion. Since I don't have much background on the detailed design, so a higher level diagram helps me better to understand. Maybe it's just personal preference.
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.
The action provider isn't a composite part of the strategy provider, and the metadata provider can be used from anywhere (actually all of them are set up like this), so it is more accurate to have them separate from each other than contained in each other as in my diagram.
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.
- wondering if I should show custom authentication strategies at all in this diagram and just leave it as
extensions
- wondering if I should specify the parameters of the decorator: strategyName:string, options?:object .
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.
secure your API endpoints, you need to: create/register a custom authentication | ||
strategy with a unique **name**, insert the authentication action in a custom | ||
sequence, and then decorate the endpoints of a controller with the | ||
`@authenticate(strategyName, options?)` authentication decorator. The |
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.
Let's use a bullet list.
* If the user credentials are missing, this method should throw an error, or return 'undefined' | ||
* and let the authentication action deal with it. | ||
* | ||
* @param request |
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.
Add some description for request
, such as:
@param request - Express request object
975741c
to
93496eb
Compare
- Authorization | ||
|
||
`Authentication` is a process of verifying someone's identity before a protected | ||
endpoint is accessed. |
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.
endpoint
-> resource
?
`Authentication` is a process of verifying someone's identity before a protected | ||
endpoint is accessed. | ||
|
||
`Authorization` is a process of verifying the actions that an authenticated 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.
verifying ...
-> deciding if a user can perform an action on a protected resource
?
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.
Maybe add links such as:
|
||
Here is a **high level** overview of the authentication component. | ||
|
||
![authentication_overview_highlevel](./imgs/authentication_overview_highlevel.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.
![authentication_overview_highlevel](./imgs/authentication_overview_highlevel.png) | ||
|
||
It is made up of a decorator, a few providers, and an extension point to which | ||
authentication strategies must be registered. |
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.
Terms like decorator/provider/extension point seem to be too low level here. We can say:
- Decorators to express authentication requirement on controller methods
- A provider to access method-level authentication metadata
- An action in the REST sequence to enforce authentication
- An extension point to discover all authentication strategies and handle the delegation
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.
Decorators
A decorator
|
||
Here is a **detailed** overview of the authentication component. | ||
|
||
![authentication_overview_detailed](./imgs/authentication_overview_detailed.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.
👍
- insert the authentication action in a custom sequence | ||
- decorate the endpoints of a controller with the | ||
`@authenticate(strategyName, options?)` authentication decorator | ||
|
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.
Maybe change the order as follows:
- decorate (app developer)
- add authentication action (app developer/cli)
- create custom authentication strategy (extension developer)
- register a custom authentication strategy (app developer)
} | ||
``` | ||
|
||
Authentication is **not** part of the default sequence of actions, so you must |
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.
By default, ...
b5aa8d6
to
8204304
Compare
} | ||
``` | ||
|
||
You can find a working **example** of a LoopBack 4 shopping cart application |
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: I'm not quite sure if we need to bold the "example" word.. similar to a few incidents towards the end of the last section. It could be just personal preference.
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.
LGTM. Thanks for addressing my comments. Please at least get approvals from @raymondfeng and ideally @jannyHou too on this. Thanks.
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.
Great details!
8204304
to
93f27bf
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.
Overall LGTM; I like how detailed the documentation is!
point in an application `application.ts` is as simple as: | ||
|
||
```ts | ||
registerAuthenticationStrategy(this, BasicAuthenticationStrategy); |
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.
can we show the import for registerAuthenticationStrategy
here?
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.
💪 LGTM
5e1bde4
to
c681657
Compare
add new section in documentation about using the authentication component
c681657
to
c4b6559
Compare
Add a new section on loopback.io about AuthenticationComponent
Associated with : #2940
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 👈