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

Auth doc component #5643

Merged
merged 1 commit into from
Jun 10, 2020
Merged

Auth doc component #5643

merged 1 commit into from
Jun 10, 2020

Conversation

jannyHou
Copy link
Contributor

@jannyHou jannyHou commented Jun 2, 2020

Sub PR for #5598

Update the authentication-component page with new layout.

Given the length of this page, I break it down into sub pages for each topic. I run doc preview to make sure links all work.

Screenshots:

New menu:
Screen Shot 2020-06-05 at 11 19 45 AM

Pages:

Overview
Screen Shot 2020-06-05 at 11 28 09 AM
Decorator
Screen Shot 2020-06-05 at 11 28 18 AM
Action
Screen Shot 2020-06-05 at 11 28 23 AM
Strategy
Screen Shot 2020-06-05 at 11 28 27 AM
Option
Screen Shot 2020-06-05 at 11 28 32 AM

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 👈

@jannyHou jannyHou changed the base branch from master to auth-doc-refactor June 2, 2020 21:56
@jannyHou jannyHou marked this pull request as draft June 2, 2020 21:57
@jannyHou jannyHou force-pushed the auth-doc-refactor branch from 055f2b7 to b9ae852 Compare June 5, 2020 15:44
@jannyHou jannyHou force-pushed the auth-doc-component branch from 4dc9fa5 to 49833ff Compare June 5, 2020 15:55
@jannyHou jannyHou marked this pull request as ready for review June 5, 2020 15:58
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.

I didn't expect such detailed writeups! Nice work 👍have some questions and nitpicks

docs/site/Authentication-component-action.md Outdated Show resolved Hide resolved
docs/site/Authentication-component-action.md Outdated Show resolved Hide resolved
docs/site/Authentication-component-action.md Outdated Show resolved Hide resolved
docs/site/Authentication-component-decorator.md Outdated Show resolved Hide resolved
docs/site/Authentication-component-options.md Outdated Show resolved Hide resolved
docs/site/Authentication-component-options.md Show resolved Hide resolved
docs/site/Authentication-component-strategy.md Outdated Show resolved Hide resolved
docs/site/Authentication-component-strategy.md Outdated Show resolved Hide resolved
@jannyHou jannyHou force-pushed the auth-doc-component branch 3 times, most recently from 12a0e1a to 564296d Compare June 5, 2020 19:36
Copy link
Member

@dhmlau dhmlau left a comment

Choose a reason for hiding this comment

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

only had the chance to review the first file.

docs/site/Authentication-component-action.md Outdated Show resolved Hide resolved
docs/site/Authentication-component-action.md Outdated Show resolved Hide resolved

</details>

By default, `authenticate` is **not** part of the sequence of actions, so you
Copy link
Member

Choose a reason for hiding this comment

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

authenticate == the authenticate action?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, seems the word is confusing, any better suggestion? maybe "authenticate"?

Copy link
Contributor

Choose a reason for hiding this comment

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

authenticate makes sense to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👌 changed to authenticate

An authentication action `AuthenticateFn` is provided by the
`AuthenticateActionProvider` class.

`AuthenticateActionProvider` is defined as follows:
Copy link
Member

Choose a reason for hiding this comment

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

When reading the original documentation (before the refactoring), I was wondering why we need to include the source code of the provider. If we want to describe what the AuthenticateActionProvider does, would it be better to list them out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel it aims to describe the provider with comments in code: there are 3 comments in the code snippet and the 1st one above strategy getter has many lines.

Here is an example of a custom sequence which utilizes the `authentication`
action.

```ts
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 todo-jwt example, we are using the default sequence that comes with the scaffolded LB app. Maybe we use that instead? Or explain when we want to create a separate sequence?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dhmlau not really :p the todo-jwt example still uses the custom sequence with "authenticate" injected and invoked.

See code.

Copy link
Member

Choose a reason for hiding this comment

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

What i meant is that the sequence.ts is created by default when we're scaffolding the app. In the snippet here seems to guide the users to create a new sequence?

Copy link
Contributor Author

@jannyHou jannyHou Jun 9, 2020

Choose a reason for hiding this comment

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

@dhmlau I rephrased it to be:

By default, "authenticate" is not part of the sequence of actions, so you
must modify the default sequence and add the authentication action.

And

Here is an example of a modified sequence which utilizes the authenticate
action.

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.

Almost LGTM, just have some suggestions :D


</details>

By default, `authenticate` is **not** part of the sequence of actions, so you
Copy link
Contributor

Choose a reason for hiding this comment

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

authenticate makes sense to me.

It is up to the developer to throw the appropriate HTTP error code from within a
custom authentications strategy or its custom services.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is L180 one of these options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@agnes512 Yep that's correct.

docs/site/Authentication-component-decorator.md Outdated Show resolved Hide resolved
Comment on lines +89 to +95
Parameters of the `@authenticate` decorator can be obtained via dependency
injection using the `AuthenticationBindings.METADATA` binding key. It returns
data of type `AuthenticationMetadata` provided by `AuthMetadataProvider`. The
`AuthenticationStrategyProvider`, discussed in a later section, makes use of
`AuthenticationMetadata` to figure out what **name** you specified as a
parameter in the `@authenticate` decorator of a specific controller endpoint.

Copy link
Contributor

@agnes512 agnes512 Jun 9, 2020

Choose a reason for hiding this comment

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

I feel like this paragraph contains too much information. Is it possible to just provide minimum information to explain the mechanism?

For example ( I might not understand it correctly):
With the binding key AuthenticationBindings.METADATA, the
AuthenticationStrategyProvider, which will be discussed in a later section, can figure out what strategy name is specified in the @authenticate decorator.

Cause I assume that at this point the reader should know how binding key works and @authenticate decorates the endpoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@agnes512 Not really, people don't always read all docs in sequence...personally I feel this paragraph is good and necessary, so I tend to keep it as is.
(but thank you for writing the draft 👍 )

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.

👏 :shipit:

@jannyHou jannyHou force-pushed the auth-doc-component branch from e7b9d8d to 0e09c6f Compare June 9, 2020 20:47
@jannyHou jannyHou merged commit b2d3ef6 into auth-doc-refactor Jun 10, 2020
@jannyHou jannyHou deleted the auth-doc-component branch June 10, 2020 00:05
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.

4 participants