feat: design auth system with user scenario #2576

jannyHou · 2019-03-12T06:41:02Z

Description

connect to #2491

(The markdown files should be part of task #2435 while we wrote them when creating the interface for login service, so connect this PR to #2491 instead)

This PR

explains/proposed a new design of auth module which could potentially support multiple auth strategies.
adds abstractions for ~~login~~ user service

When review PR, please start from the file authentication-system.md in the root dir of @loopback/authentication, the content on that page will mention about each new created .md file and take you there.

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

bajtos

The text seems to be based in the original ideas described in GH issues and does not seem to take into account my comments and concerns about shortcomings of that original proposal. Is that intentional?

packages/authentication/authentication-system.md

emonddr · 2019-03-12T14:30:38Z

packages/authentication/docs/strategies/jwt.md

+  // 1. Try to find current user
+  // 2. If found, return it
+  // 3. If not found, throw 401:
+  verify(request) {


Missing complete signature

emonddr · 2019-03-12T14:32:32Z

packages/authentication/docs/strategies/oauth2.md

+    return tokenAndUser.userProfile;
+  };
+
+  ensureLoggedIn(request) {


Missing complete signature

jannyHou · 2019-03-12T15:14:37Z

@bajtos

The text seems to be based in the original ideas described in GH issues and does not seem to take into account my comments and concerns about shortcomings of that original proposal. Is that intentional?

do you mean split the abstractions into "transport related" and "transport agnostic" ones?

If you look at the services folder, we split the login service into two:

login service: agnostic of the transport layer.
client service: related to the transport layer. (The service name may not be a proper one, we can change)

Or did I miss anything?

raymondfeng · 2019-03-13T17:36:18Z

packages/authentication/docs/authentication-action.md

+    // read the action metadata from endpoint's auth metadata like 
+    // `@authenticate('strategy_name', {actions: 'authenticate'})`
+    // type ActionType = 'authenticate' | 'ensureLoggedIn'
+    const action = await this.getAction();


Maybe we don't need to differentiate between login and verify actions. Please note Passport just uses different strategies, for example:

Local strategy for username/password based login

BearTokenStrategy for token verification

Both strategies implement authenticate method.

I see, I was coupling them into one strategy.

raymondfeng · 2019-03-13T18:03:27Z

packages/authentication/docs/controller-functions.md

+class UserController {
+  @post('/loginWithFB', ...APISpec)
+  @authenticate('oath2.fb', {action: 'login', session: false})
+  loginWithFB() {}


If we use login with facebook, the browser will be redirected to facebook login page to perform the oAuth 2.0 implicit or authorization code flow. Both of them require a callback URL. When the controller method is decorated as shown, what logic should be in loginWithFB method? Is it the callback function or just a place holder route to excise the FB login flow? I imagine that /loginWithFB will initiate the FB login.

Is it the callback function or just a place holder route to excise the FB login flow?

The second one "a place holder route to excise the FB login flow".

We find it pretty hard to illustrate the oauth+session strategies without implementing an example, so I created a story to explorer it then revisit the design here and update accordingly.

#2624

My proposal would be defining a separate callback endpoint, e.g. login/facebook and login/facebook/callback

raymondfeng · 2019-03-13T18:07:32Z

packages/authentication/authentication-system.md

+
+The diagram below illustrates the high level abstraction of such an extensible authentication system.
+
+<img src="./docs/imgs/multiple-auth-strategies-login.png" width="1000px" />


raymondfeng · 2019-03-13T20:46:58Z

@jannyHou Please run npm lint:fix to fix formatting issues reported by https://travis-ci.org/strongloop/loopback-next/jobs/505426718

bajtos

do you mean split the abstractions into "transport related" and "transport agnostic" ones?

If you look at the services folder, we split the login service into two:

login service: agnostic of the transport layer.

client service: related to the transport layer. (The service name may not be a proper one, we can change)

Thank you for the pointers! My comment was based on reading the first Markdown file only, I didn't read the TypeScript definitions.

I think this aspect is resolved now 👍

Or did I miss anything?

See the second comment below.

I think that by now, you have much better picture about the authentication architecture than I have. I am happy to leave this in your hands going forward.

bajtos · 2019-03-14T13:43:42Z

packages/authentication/src/services/login.service.ts

+   * @param request The incoming Request
+   */
+  // This function should probably moved to session service?
+  invalidateCredentials?(request: Request): Promise<boolean>


Since the login service is transport agnostic now, I think it should be accepting credentials: Credentials instead of request: Request?

👍 yep, we redesigned all the transport related logic into the strategy class or the controller method. Now the services are agnostic of the client.

bajtos · 2019-03-14T13:47:11Z

packages/authentication/src/services/client.service.ts

+  // Here I hardcoded the request type to be HTTP request,
+  // while a generic type R should be used here to support
+  // other clients like SOAP, GRPC
+  extractCredentials(request: Request): Promise<C>;


What are your thoughts on the following (re-posted from #2491 (comment))?

I find it problematic to implement transport specific behavior in a service.

How do we envision that such service would be used from a controller? It's important to describe the login endpoint via OpenAPI and include specification of input parameters. Are we expecting the controller method to be decorated with @param decorators and then pass the entire request to the login service? That would make it too easy to introduce bugs where the controller is describing different parameters than the service extracts from the request.

IMO, extract & invalidate credentials should not be provided by any service, but should be implemented as controller actions leveraging the services described in #2435 under the hood.

class UserController { constructor ( // inject response, AccessTokenService, TokenTransportStrategy and LoginService ) {} @post('/login') async login( @param() email: string, @param() password: string, ) { const user = await this.loginService.verifyCredentials({email, password}); const token = await this.tokenService.generateAccessToken(user); await this.transportStrategy.serializeAccessToken(this.response); // uh oh, what is the return value of this method? // if the token is returned in response body, how are we going to describe response schema? } @post('/logout') async logout( @inject('current access token') token: string ) { await this.tokenService.invalidateAccessToken(token); } }

@bajtos Yeah that's the twisted part...

How do we envision that such service would be used from a controller?

We decide to not create service for request/response related operations, like extracting credentials from request, writing into response, etc...

The login endpoint is verify special, in the shopping example, login doesn't even invokes the authentication action...

In the new design we come up with a new proposal which moves the credential verification back into the auth strategy. The controller function only generates the access token.

Because the auth strategy's responsibility is taking in a request and returning an authenticated user profile, I suggest let developers decide handling the authentication in the auth action OR in the controller:

Do authentication in strategy:

the strategy will extract and validate the user credentials from request

it also verifies the credentials by calling userService.verifyCredentials()

controller function generates the access token

pros

consistent with endpoints other than /login

cons

lose the convenience provided by REST decorator(e.g. describing parameters using maxLength)

Do authentication in controller:

describe the credentials from request using OpenAPI spec

action parseParam will parse+validate credentials

controller function verify credentials(call userService.verifyCredentials()) and generates the access token.

pros

describe the response and request parameters in a clear way.

cons

repeating the auth strategy logic(extract+verify credentials) using OpenAPI spec+controller code

usually the login endpoints is countable and less than 5 for most of applications, treat them specially wouldn't be a huge effort.

hacksparrow · 2019-03-25T14:32:38Z

packages/authentication/docs/authentication-action.md

+      // convert it to http error
+      if (err.code == '401') {
+        throw new HttpErrors.Unauthorized(err.message);
+      }


Shouldn't we throw for the else condition as well?

good point 👍 The code in the .md file is pseudo code so I didn't refine too much for the error catch, will handle all the errors in story #2467

hacksparrow · 2019-03-25T15:19:28Z

I wish the mechanics for enabling authentication was simpler. Like flip a switch, and @authenticate() just works, without having to create a provider and a custom sequence.

jannyHou · 2019-03-25T15:49:24Z

@hacksparrow Good feedback

I wish the mechanics for enabling authentication was simpler. Like flip a switch, and @authenticate() just works, without having to create a provider and a custom sequence.

The design in this PR covers all the details of the auth mechanics, to give extension providers/app developers the max flexibility to plugin/replace their implementation. That's the reason why "making a complete auth system" looks complicated. While we could provide a default auth system that saves people's effort.

E.g.

Add an option to enable authenticate in the sequence when scaffold the app. People don't need to customize the sequence.
Implement the default auth strategy extension point in Extension point for registering the authentication strategies #2312, then people can authenticate an endpoint by ONLY 2 steps: 1. register their auth strategy as an extension-->2.decorate the endpoint with strategy name. Then everything automatically works!

raymondfeng · 2019-03-26T17:40:57Z

packages/authentication/docs/authentication-action.md

+
+async action(request: Request): Promise<UserProfile | undefined> {
+    const authStrategy = await this.getAuthStrategy();
+    if (!authStrategy) {


How do we differentiate between the following two cases?

No authentication is required

No strategy is found to handle the authentication

@raymondfeng I would expect the resolver getAuthStrategy() to throw an error if the strategy is not found.

We haven't added the resolver, I plan to implement it in the extension story in #2312

raymondfeng · 2019-03-26T17:43:34Z

packages/authentication/docs/authentication-strategy.md

+  // The resolver will read the options object from metadata, call `strategy.setOptions`
+  options: object;
+  authenticate(request: Request): Promise<UserProfile | undefined>;
+  setOptions(options: object);


It's better to pass in the metadata via authenticate method, such as:

authenticate(request: Request, options?: ...): Promise<UserProfile | undefined>;

Your current proposal will mandate a strategy implementation to be bound in TRANSIENT scope.

Your current proposal will mandate a strategy implementation to be bound in TRANSIENT scope.

True....

raymondfeng · 2019-03-26T17:46:30Z

packages/authentication/docs/controller-functions.md

+[authentication-system](./authentication-system.md).
+
+Please note how they are decorated with `@authenticate()`, the syntax is:
+`@authenticate(<strategy_name>, {action: <action_name>, session: <enabled_or_not>})`


Is action still needed?

good catch, not really

raymondfeng · 2019-03-26T17:48:43Z

packages/authentication/docs/controller-functions.md

+
+  // Describe the response using OpenAPI spec
+  @post('/loginOAI/local', RESPONSE_SPEC_FOR_JWT_LOGIN)
+  @authenticate('basicAuth')


Do we allow a method to be authenticated by one of the strategies?

@authenticate(['basicAuth', 'localAuth']) // We allow either basic or local auth

I thought "local" and "basic" auth are same, which is wrong. So the endpoint here meant to be decorated with only one strategy.

We leave it for future decisions. It would be nice to support one of the many auths on the same method.

We leave it for future decisions. It would be nice to support one of the many auths on the same method.

My understanding is:

For "login" endpoints:

the number of endpoints will be small, e.g. one system supports 5 login ways

usually there is only one strategy applied per endpoint

For "verify" endpoints:

the number of endpoints will be large

it makes sense to specify a default auth strategy for them instead of repeating the decoration thousands of times, OR, to be more practical, resolve the auth strategy based on the login approach.

There is an issue discussing about resolving the strategy after login:
Shall we continue the discussion in #2139?

raymondfeng · 2019-03-26T17:50:12Z

packages/authentication/docs/strategies/basic-auth.md

+class BasicAuthenticationStrategy implements AuthenticationStrategy {
+  options: object;
+  constructor(
+    @inject(AUTHENTICATION_BINDINGS.SERVICES.USER) userService: UserService,


Maybe AUTHENTICATION_BINDINGS.USER_SERVICE? Let's constrain the namespace to be at most one level.

makes sense

raymondfeng · 2019-03-26T20:56:19Z

packages/authentication/docs/controller-functions.md

@@ -5,7 +5,7 @@ the beginning of markdown file
 [authentication-system](./authentication-system.md).

 Please note how they are decorated with `@authenticate()`, the syntax is:
-`@authenticate(<strategy_name>, {action: <action_name>, session: <enabled_or_not>})`
+`@authenticate(strategy_name, options)`


In the future, we can allow class-level decoration of @authenticate.

Agree. sounds like an improvement could be applied with #2576 (comment)

raymondfeng · 2019-03-26T20:57:53Z

packages/authentication/docs/controller-functions.md

  // Describe the response using OpenAPI spec
-  @post('/loginOAI/local', RESPONSE_SPEC_FOR_JWT_LOGIN)
+  @post('/loginOAI/basicAuth', RESPONSE_SPEC_FOR_JWT_LOGIN)


Do we want to clarify that the login method implementation can have two choices:

@authenticate + user code

plain user code

Sounds good, would like to add #2576 (comment) here.

Co-authored-by: emonddr <[email protected]>

emonddr · 2019-03-27T18:37:03Z

rebased and squashed commits

jannyHou requested review from bajtos and raymondfeng as code owners March 12, 2019 06:41

jannyHou force-pushed the login-service branch from 77282e7 to e32902e Compare March 12, 2019 07:13

bajtos reviewed Mar 12, 2019

View reviewed changes

packages/authentication/authentication-system.md Outdated Show resolved Hide resolved

bajtos added Docs Authentication labels Mar 12, 2019

emonddr reviewed Mar 12, 2019

View reviewed changes

jannyHou force-pushed the login-service branch from bb3d51c to 62114fb Compare March 12, 2019 20:33

jannyHou assigned jannyHou and emonddr Mar 13, 2019

raymondfeng reviewed Mar 13, 2019

View reviewed changes

bajtos reviewed Mar 14, 2019

View reviewed changes

jannyHou force-pushed the login-service branch from 516a829 to 38841ba Compare March 21, 2019 17:15

jannyHou mentioned this pull request Mar 21, 2019

[Spike] Explore/research the oauth2+session based authentication strategy #2624

Closed

5 tasks

hacksparrow reviewed Mar 25, 2019

View reviewed changes

raymondfeng reviewed Mar 26, 2019

View reviewed changes

raymondfeng approved these changes Mar 26, 2019

View reviewed changes

emonddr force-pushed the login-service branch from 9bb5241 to 6b8e236 Compare March 27, 2019 18:17

feat: design auth system with user scenario

99e3090

Co-authored-by: emonddr <[email protected]>

emonddr force-pushed the login-service branch from 6b8e236 to 99e3090 Compare March 27, 2019 18:33

jannyHou merged commit 124c078 into master Mar 28, 2019

bajtos deleted the login-service branch March 28, 2019 09:59

emonddr mentioned this pull request May 12, 2019

refactor: include user and token service interfaces in index.ts #2875

Merged

7 tasks


		The diagram below illustrates the high level abstraction of such an extensible authentication system.

		<img src="./docs/imgs/multiple-auth-strategies-login.png" width="1000px" />

feat: design auth system with user scenario #2576

feat: design auth system with user scenario #2576

Conversation

jannyHou commented Mar 12, 2019 • edited Loading

Description

Checklist

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou commented Mar 12, 2019

raymondfeng Mar 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng commented Mar 13, 2019

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou Mar 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hacksparrow commented Mar 25, 2019

jannyHou commented Mar 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng Mar 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emonddr commented Mar 27, 2019

jannyHou commented Mar 12, 2019 •

edited

Loading

raymondfeng Mar 13, 2019 •

edited

Loading

jannyHou Mar 21, 2019 •

edited

Loading

raymondfeng Mar 26, 2019 •

edited

Loading