feat: resolve authentication strategy registered via extension point #2763

emonddr · 2019-04-17T18:11:39Z

Resolve authentication strategies registered via extension point

Part of #2312

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

emonddr · 2019-04-17T18:14:11Z

First commit tackles mock Basic authentication strategy class which implements AuthenticationStrategy interface, and which registers itself as an extension of the authentication strategy extension point.

jannyHou

@emonddr 👍

As we discussed, I would still prefer you finish your PR regardless what changes in my PoC, just comment out those failed passport-based tests and add your new tests. I will deal with them(either recover or rewrite) in mine when I do the rebase.

I post a few comment about having another extension point for passport-based strategies, which is out of the scope of this PR, it's JUST a proposal of handling the passport strategies according to my PoC. We are free to explorer better approaches.

jannyHou · 2019-04-18T02:21:59Z

...on/src/__tests__/acceptance/authentication-action-provider/authentication-action.provider.ts

+// This file is licensed under the MIT License.
+// License text available at https://opensource.org/licenses/MIT
+
+import {Getter, inject, Provider, Setter} from '@loopback/context';


You can rewrite the provider directly.

See the same file in my PR, which contains exactly the same code as yours, and also proves this action is agnostic of the strategy type(passport or non-passport).

jannyHou · 2019-04-18T02:37:08Z

packages/authentication/src/__tests__/acceptance/strategy-resolver/strategy.resolver.ts

+    @inject(AuthenticationBindings.METADATA)
+    private metadata: AuthenticationMetadata,
+    @extensions() // Sugar for @inject.getter(filterByTag({extensionPoint: 'greeter'}))
+    private authenticationStrategies: Getter<AuthenticationStrategy[]>,


The getter here only captures an array of our authentication strategies, therefore I think we need another extension which has a getter for passport-strategies, like

@extensionPoint('passport-strategy') export class PassportStrategyResolverProvider implements Provider<Strategy | undefined> { constructor( @inject(AuthenticationBindings.METADATA) private metadata: AuthenticationMetadata, @extensions() private passportStrategies: Getter<Strategy[]>, ) {} value(): ValueOrPromise<Strategy | undefined> { if (!this.metadata) { return; } // make sure the endpoint is decorated with a passport based strategy const isPassportStrategy = this.metadata.options && this.metadata.options.isPassportStrategy; if (!isPassportStrategy) return; const name = this.metadata.strategy; return this.findPassportStrategy(name).then(function(strategy) { if (strategy) { // Please note we wrap it with the adapter to return a Loopback auth strategy return new StrategyAdapter(strategy); } else { throw new Error(`The strategy '${name}' is not available.`); } }); } async findPassportStrategy(name: string) { const strategies = await this.passportStrategies(); const matchingAuthStrategy = strategies.find(a => a.name === name); return matchingAuthStrategy; } }

Then the StrategyResolverProvider invokes the PassportStrategyResolverProvider when it finds the endpoint is decorated with a passport strategy.

You can find the prototype implementation in My PR.
There are 2 provider classes and each should be decorated with extensionPoint().

I will take a look. My strategy resolver in my PR will not deal with passport strategies at the moment. This will be handled by your PR. You can rebase your PR branch off of my branch as discussed.

raymondfeng · 2019-04-18T17:47:05Z

Pls fix lint issue - https://travis-ci.org/strongloop/loopback-next/jobs/521377177

jannyHou · 2019-04-22T19:08:49Z

packages/authentication/src/__tests__/acceptance/keys.ts

@@ -0,0 +1,25 @@
+import {BindingKey} from '@loopback/context';


Nitpick: I would suggest moving keys.ts to a folder called fixtures, same for services and strategies.

jannyHou · 2019-04-22T19:19:01Z

packages/authentication/src/__tests__/acceptance/users/user.ts

+// This file is licensed under the MIT License.
+// License text available at https://opensource.org/licenses/MIT
+
+export interface User {


Usually the User would be a model, can we define a model instead?

The user repository used in similar, existing tests was a hard-coded list of JSON objects and didn't involve datasources and models...so I figured there is no need to create a model for my acceptance tests.

jannyHou · 2019-04-22T19:27:04Z

packages/authentication/src/types.ts

@@ -51,3 +51,54 @@ export interface AuthenticationStrategy {
   */
  authenticate(request: Request): Promise<UserProfile | undefined>;
 }
+
+export interface UserService<U, C> {


We created the interface for services in https://github.com/strongloop/loopback-next/blob/master/packages/authentication/src/services/user.service.ts
and
https://github.com/strongloop/loopback-next/blob/master/packages/authentication/src/services/token.service.ts
No need to duplicate the definition here :-p

emonddr · 2019-04-23T17:52:29Z

packages/authentication/src/__tests__/acceptance/basic-auth.acceptance.ts

-    cb(null, userList[found].profile);
-  }
-}
+// import {inject, Provider, ValueOrPromise} from '@loopback/context';


@jannyHou stated I should comment out these tests for the meantime. Her PR will re-write the test.

@emonddr Could you use describe.skip() instead of commenting out the code?
It will be much easier for reviewers to compare the difference in the PR recovering the tests. Thanks!

See the current code change comparison: 92b10a2#diff-a5eff729d80d870cc338b162f09467be, all the code are changed, it's hard to see the real difference.

emonddr · 2019-04-23T17:53:05Z

packages/authentication/src/__tests__/unit/providers/authentication.provider.unit.ts

-import {AuthenticateFn, AuthenticationBindings, UserProfile} from '../../..';
-import {AuthenticateActionProvider} from '../../../providers';
-import {MockStrategy} from '../fixtures/mock-strategy';
+// import {Context, instantiateClass} from '@loopback/context';


@jannyHou stated I should comment out these tests for the meantime. Her PR will re-write the test.

emonddr · 2019-04-24T20:37:30Z

#2311 will have changes which will affect the AuthenticationStrategyProvider presented in this work item. It will involve resolving passport related authentication strategies, and will also affect some mocha tests that I have commented out for the moment.

hacksparrow · 2019-04-25T12:49:55Z

packages/authentication/docs/authentication-system.md

+`src/providers/auth-strategy.provider.ts` declares an `extension point` named
+`AuthenticationBindings.AUTHENTICATION_STRATEGY_EXTENSION_POINT_NAME` via the
+`@extensionPoint` decorator. `AuthenticationStrategyProvider` is responsible for
+returning an authentication strategy which has a `specific name` and has been


Is specific name a programming construct? If not, we should omit the backticks. Likewise, other backticked words in the doc.

jannyHou · 2019-04-25T15:19:37Z

packages/authentication/src/__tests__/fixtures/keys.ts

+);
+
+export namespace BasicAuthenticationStrategyBindings {
+  export const USER_SERVICE = BindingKey.create<BasicAuthenticationUserService>(


The type of user service here should be interface UserService.

export const USER_SERVICE = BindingKey.create<BasicAuthenticationUserService>('services.authentication.user.service')

Then we bind the BasicAuthenticationUserService class to the key

@jannyHou :

export const USER_SERVICE = BindingKey.create<UserService>( 'services.authentication.basic.user.service', );

produces a typescript error most likely because UserService is defined like:

interface UserService<U, C>

In my test cases, I only use the BasicAuthenticationUserService for my basic authentication strategy mock implementation. I don't see a point in using UserService when creating a binding key.

jannyHou · 2019-04-25T15:20:33Z

packages/authentication/src/__tests__/fixtures/keys.ts

+  export const TOKEN_EXPIRES_IN = BindingKey.create<string>(
+    'authentication.jwt.expires.in.seconds',
+  );
+  export const TOKEN_SERVICE = BindingKey.create<JWTService>(


Same here.

export const TOKEN_SERVICE = BindingKey.create<TokenService>('services.authentication.tokenservice')

@jannyHou :
Because I am not going to create a binding key with generic interface UserService, to be consistent, I won't create a binding key using the generic interface TokenService.

jannyHou · 2019-04-25T15:24:15Z

packages/authentication/src/__tests__/fixtures/services/basic-auth-user-service.ts

+    credentials: BasicAuthenticationStrategyCredentials,
+  ): Promise<User> {
+    if (!credentials) {
+      throw new HttpErrors.Unauthorized(`'credentials' is null`);


As we discussed yesterday, it would be good to throw a JS error object here, and your sequence turns it to an HTTP error. (I remember you have that code in the sequence file)

@jannyHou , we discussed that the strategy resolver would return a JS error object since, in the future, we may use it to resolve strategies for authenticating protocol requests other than http rest requests, and that the strategy resolver that ships in the main code of the authentication package shouldn't be returning any http specific errors.

However my mock implementations of token service and user service and strategies don't ship in the main code of the authentication package and only exist in the src/tests folder, and so I have chosen to have then throw http errors since I am testing them with a rest server.

@emonddr Fair enough 👍

jannyHou · 2019-04-25T15:25:35Z

packages/authentication/src/__tests__/fixtures/services/basic-auth-user-service.ts

+      throw new HttpErrors.Unauthorized(`'user.email' is null`);
+    } //if
+
+    let userProfile: UserProfile = {


const is better.

jannyHou · 2019-04-25T15:30:03Z

packages/authentication/src/__tests__/fixtures/services/basic-auth-user-service.ts

+
+  convertToUserProfile(user: User): UserProfile {
+    if (!user) {
+      throw new HttpErrors.Unauthorized(`'user' is null`);


hmm, are the comments //if still needed?

@jannyHou :
I personally like adding //if and //endif and //try and //catch next to the } in complex nested blocks in order to make my code easier to read...but I will remove the //if comments in this piece of code because the code isn't complex.

Oh ok I am ok with either using the comment or not.

Modern IDE such as VSCode can help find matching {} or (). Adding such comments is not helpful for other developers as if can be nested too.

jannyHou · 2019-04-25T16:12:54Z

packages/authentication/src/__tests__/fixtures/services/jwt-service.ts

+      );
+    } //if
+
+    if (!userProfile.email) {


I remember the email and name are optional, see interface UserProfile

@jannyHou:

I realize that email and name can be optional in the UserProfile interface we have defined in the authentication package, but this is mainly so we don't force the community to use the two fields we are providing as suggestions.

In my acceptance tests in the src/__tests__folder, I create mock implementation classes for two strategies, for a token service, and for a user service, and I personally decided that each field in UserProfile (optional or not) is required for creating a JWT Token. The purpose of the mocha tests are to test the authentication strategy interface, and registering authentication strategies via the extensionPoint/extensions pattern.

and I personally decided that each field in UserProfile (optional or not) is required for creating a JWT Token.

Fair enough 👍

jannyHou · 2019-04-25T16:27:36Z

packages/authentication/src/__tests__/fixtures/strategies/jwt-strategy.ts

+  }
+
+  extractCredentals(request: Request): string {
+    if (!request.headers.access_token) {


From what I've read online, the typical way is checking the authorization header like request.header.authorization.

See "How do JSON Web Tokens work?" in https://jwt.io/introduction/

The access_token field is usually from the query, see our implementation in the shopping cart example.

@jannyHou , I changed it to use the Authorization header with 'Bearer aaa.bbb.ccc' (where aaa.bbb.ccc is the JWT token ) instead.

jannyHou

@emonddr Great effort!

I left some comments. Your implementation of the extension point LGTM. And the doc explains how to register extensions clearly.

In one of my comment I suggested you to rewrite the auth action provider directly and I will make it compatible with the passport strategies in the other PR, while on a second thought, I am afraid landing this PR will cause breaking change for the @loopback/authentication users.
Maybe still keep the new action provider in a separate file so that existing users can bind the old action provider easily.
Sorry for my changing mind again :( we can discuss more details tomorrow regarding this.

jannyHou · 2019-04-26T01:57:00Z

packages/authentication/src/providers/auth-strategy.provider.ts

+    const name = this.metadata.strategy;
+
+    return this.findAuthenticationStrategy(name).then(function(strategy) {
+      if (strategy) {


nitpick:

then((strategy) => {}) { }

jannyHou · 2019-04-26T02:10:13Z

packages/authentication/src/types.ts

+ * cannot find the specified authentication strategy by name.
+ */
+export class AuthenticationStrategyNotFoundError extends Error {
+  constructor(error: string) {


You may want to add a code property as the machine-readable error code that could be understood by any client(HTTP/GRPC/SOAP...)

You may want to add a code property as the machine-readable error code that could be understood by any client(HTTP/GRPC/SOAP...)
@jannyHou , not sure what you mean here.

@emonddr The same topic as what we discussed here #2763 (comment) :)

export class AuthenticationStrategyNotFoundError extends Error { // the `code` would be a string that could be understood by different clients // like 'INVALID_ACCESS_TOKEN' // see https://github.com/strongloop/loopback4-example-shopping/blob/master/packages/shopping/src/authentication-strategies/JWT.strategy.ts#L35 code: string; constructor(error: string) { super(error); } }

Also see my example 9bda8b7#diff-440925ebccbc865e10eb2da8810e08eaR9

@jannyHou , I removed the AuthenticationStrategyNotFoundError class, and now I just create an instance of Error, and augment it (with Object.assign) with a code field which contains a specific error.

return this.findAuthenticationStrategy(name).then(strategy => { if (strategy) { return strategy; } else { // important not to throw a non-protocol-specific error here let error = new Error(`The strategy '${name}' is not available.`); Object.assign(error, { code: AUTHENTICATION_STRATEGY_NOT_FOUND, }); throw error; } });

and the constant literal string is defined in types.ts:

export const AUTHENTICATION_STRATEGY_NOT_FOUND = 'AUTHENTICATION_STRATEGY_NOT_FOUND';

and my custom sequences in packages/authentication/src/__tests__/acceptance/basic-auth-extension.acceptance.tsand packages/authentication/src/__tests__/acceptance/jwt-auth-extension.acceptance.ts
catch the error:

... try { //call authentication action await this.authenticateRequest(request); } catch (e) { // strategy not found error if (e.code === AUTHENTICATION_STRATEGY_NOT_FOUND) { throw new HttpErrors.Unauthorized(e.message); } //if else { // strategy error throw e; } //endif } //catch ...

@emonddr thank you, that refactor looks perfect to me 👍

emonddr · 2019-04-28T06:22:03Z

@jannyHou , I have moved my revised authentication action provider from the main code to the src/tests/fixtures/providers folder since you stated you changed your mind about me replacing the original...that it might cause existing users problems. We will need a new authentication action provider going forward, and the authentication component needs to specify an authentication action provider...so which one will we set?

I am assuming that your PR will set the appropriate one for the component then.

jannyHou · 2019-04-29T15:49:46Z

Was discussing with @emonddr , the next step would be:

Move the action provider from __test__/fixtures to src/providers and replace the current authentication-action.provider.ts
This PR contains breaking change, let's bump up the version of @loopback/authentication to 2.0.0

emonddr · 2019-04-29T19:16:20Z

packages/authentication/src/__tests__/acceptance/basic-auth.acceptance.ts


 const SequenceActions = RestBindings.SequenceActions;

-describe('Basic Authentication', () => {
+describe.skip('Basic Authentication', () => {


I only added '.skip(' to the test file, but it also resorted the imports. Not sure how to prevent that.
It shouldn't matter, @jannyHou's PR, #2746, needs to rewrite this test file.

It's ok, should be some auto fix by prettier/tslint

emonddr · 2019-04-29T19:17:16Z

packages/authentication/src/__tests__/unit/providers/authentication.provider.unit.ts

-        () => Promise.resolve(strategy),
-        u => (currentUser = u),
-      );
+      // provider = new AuthenticateActionProvider(


Needed to comment out code here. @jannyHou's PR, #2746, needs to rewrite this test file.

raymondfeng · 2019-04-29T20:39:17Z

packages/authentication/docs/authentication-system.md

+    }
+    const name = this.metadata.strategy;
+
+    return this.findAuthenticationStrategy(name).then(function(strategy) {


Let's consistently use async-await instead of then.

raymondfeng · 2019-04-29T20:43:25Z

packages/authentication/src/providers/auth-strategy.provider.ts

+    }
+    const name = this.metadata.strategy;
+
+    return this.findAuthenticationStrategy(name).then(strategy => {


Let's use async-await instead of then.

I didn't realize I was allowed to : add async to the value() function of the Provider interface, and change its return type from ValueOrPromise to Promise. Thanks @b-admike

raymondfeng · 2019-04-29T20:45:30Z

packages/authentication/docs/authentication-system.md

+
+    this.sequence(SequenceIncludingAuthentication);
+
+    addExtension(


Maybe we should add a sugar method such as app.extension().

@raymondfeng is this something you will add?

raymondfeng · 2019-05-01T15:03:24Z

packages/authentication/src/__tests__/fixtures/users/user.repository.ts

+    const found = Object.keys(userList).find(search);
+    if (found) {
+      return userList[found].user;
+    } //if


No need to have // if.

raymondfeng · 2019-05-01T15:05:45Z

packages/authentication/src/__tests__/fixtures/users/user.repository.ts

+    const userList = this.list;
+    function search(key: string) {
+      return userList[key].user.email === email;
+    }


Use an arrow function instead:

Object.keys(userList).find(k => this.list[k].email === email);

b-admike

Great job @emonddr. Overall LGTM, I've left some high level comments on some test/doc/code refactoring.

b-admike · 2019-05-01T17:34:50Z

packages/authentication/docs/authentication-system.md

@@ -163,3 +163,192 @@ And the abstractions for:
    - return user
  - controller function:
    - process the injected user
+
+## Registering an authentication strategy via an extension point


I hope in follow-up PRs we can move this awesome doc to the README.

@b-admike , yes. @jannyHou 's PR : #2822 overlaps with mine, and we have discussed that we will be moving all separate .md files in the authentication package into a comprehensive readme.md file. And this readme file will also be the source of the loopback.io docs that are generated by our doc build process.

b-admike · 2019-05-01T17:38:45Z

packages/authentication/src/__tests__/acceptance/basic-auth-extension.acceptance.ts

+      .expect(users.list['joe@example.com'].user.email);
+  });
+
+  it(`authenticates successfully for correct credentials for user 'jill'`, async () => {


Feel free to ignore, but I think it'd be good to re-assign credential to user jill in the test above and assert that auth works for both users (have it in one test)

After speaking with @b-admike , we decided to remove the 2nd test involving the user Jill.

b-admike · 2019-05-01T17:42:10Z

packages/authentication/src/__tests__/acceptance/jwt-auth-extension.acceptance.ts

+  });
+
+  it(`returns error for invalid 'Bearer ' portion of Authorization header value`, async () => {
+    class InfoController {


Consider moving InfoController to the bottom of the test as a helper if it can be re-used throughout these tests. Feel free to do it in a follow up PR.

@b-admike , The Info controllers are different for pretty much all tests, so I will leave as is.

b-admike · 2019-05-01T17:44:06Z

packages/authentication/src/__tests__/acceptance/jwt-auth-extension.acceptance.ts

+
+    await whenIMakeRequestTo(server)
+      .get('/createtoken')
+      .expect(401);


For a lot of these tests we are asserting the error code, but is it possible to also assert error messages sent back? Not a deal breaker, but it'll make the tests more robust.

@b-admike , I have addressed your comment. I check error code and message in my tests.

b-admike · 2019-05-01T17:46:13Z

packages/authentication/src/__tests__/fixtures/strategies/jwt-strategy.ts

+    } //if
+
+    //split the string into 2 parts : 'Bearer ' and the `xxx.yyy.zzz`
+    let parts = auth_header_value.split(' ');


Do we want to assert that length of parts does not exceed 2 here?

I will add a check, but we must remember these are mock implementations in src/tests and not production code. We don't need to test each line of code for these items. This PR has mocha tests that focus on verifying the authentication strategy interface , registering and finding of authentication strategies via extensionPoints, the authentication action, and the authentication strategy provider. The mock implementations for token service and user service and basic strategy and jwt strategy only serve as fixture helpers and don't need to be tested so heavily.

b-admike · 2019-05-01T17:47:20Z

packages/authentication/src/providers/auth-strategy.provider.ts

+  AuthenticationBindings.AUTHENTICATION_STRATEGY_EXTENSION_POINT_NAME,
+  {scope: BindingScope.TRANSIENT},
+)
+export class AuthenticationStrategyProvider


Since this is part of the source code, it'd be great to add TSDocs to here for our API docs.

emonddr · 2019-05-01T19:35:12Z

Please either remove https://github.com/strongloop/loopback-next/blob/dremond_authentication_2312/packages/authentication/src/services/session.service.ts or make it an empty interface with copyright headers for now.

I will make it an empty interface with copyright headers for now.

dhmlau · 2019-05-01T19:57:23Z

@emonddr , FYI, you can install strong-tools globally and run slt copyright to add the copyright headers.

emonddr · 2019-05-02T14:51:58Z

added two more test cases for auth-action.provider.ts since code coverage decreased after I added an error for undefined userProfile return from a strategy.

emonddr · 2019-05-02T18:41:12Z

rebased...

raymondfeng · 2019-05-02T19:50:14Z

packages/authentication/src/__tests__/fixtures/helper.ts

+  // extraPart is '' (blank), unless another is specified
+  const extraPart =
+    options && options.extraSegment ? separator + options.extraSegment : '';
+  return (


This can be simplified as:

options = Object.assign({ alternativePrefix: 'Basic ', alternativeSeparator: ':', extraSegment: '' }, options);

That's a great replacement for the ternary operator in this case. Thanks. :)

Thanks for the update. Next time, please click on Resolve conversation if it's fixed.

raymondfeng · 2019-05-02T19:51:53Z

packages/authentication/src/__tests__/fixtures/sequences/authentication.sequence.ts

+          e.code === USER_PROFILE_NOT_RETURNED
+        ) {
+          throw new HttpErrors.Unauthorized(e.message);
+        } //if


Please remove //if.

removed all //if occurrences.

raymondfeng · 2019-05-02T19:52:12Z

packages/authentication/src/__tests__/fixtures/sequences/authentication.sequence.ts

+          // strategy error
+          throw e;
+        } //endif
+      } //catch


Please remove //endif and //catch.

raymondfeng · 2019-05-02T19:53:41Z

packages/authentication/src/__tests__/fixtures/services/basic-auth-user-service.ts

+      throw new HttpErrors.Unauthorized(`'credentials' is null`);
+    }
+
+    if (!credentials.email) {


Basic auth allows username. Do we want generalize it as username instead of email?

can't people log in with email and password?

email can be a person's username can it not?

I can have two gmail addresses associated with my name and surname. So an email can serve as a username.

I am trying to make use of

export interface UserProfile { id: string; name?: string; email?: string; }

and in our user service interface, we have

convertToUserProfile(user: U): UserProfile;

where we have a user object (we obtained by searching a database with user credentials) and then we convert the larger user object into a slimmer user profile we place on the context.

Why would we have the email field in UserProfile if we cannot use it?
@jannyHou , what are your thoughts? thx

Not sure how relevant it is, just wanna point out that there is another discussion thread talking about making the UserProfile more flexible: #2246.

I'm proposing to be consistent with http://www.passportjs.org/docs/basic-digest/. Please note email id can be used as username.

raymondfeng · 2019-05-02T19:55:58Z

packages/authentication/src/__tests__/fixtures/services/basic-auth-user-service.ts

+    if (!user.email) {
+      throw new HttpErrors.Unauthorized(`'user.email' is null`);
+    }
+


Do we really want to make firstname, surname, email required? It is desired sometimes to hide fields such as email due to privacy.

I'm also not sure if these errors should be mapped to HttpErrors.Unauthorized.

The token service and user service and strategies in src/tests/fixtures are mock implementations not production implementations in 'src'. Is it absolutely necessary for them to be perfect? The users must create their own production ready artifacts. (Just wondering)

The code block above is called when the basic strategy is trying to authenticate the user for a secured endpoint.

async authenticate(request: Request): Promise<UserProfile | undefined> { const credentials: BasicAuthenticationStrategyCredentials = this.extractCredentals( request, ); const user = await this.userService.verifyCredentials(credentials); const userProfile = this.userService.convertToUserProfile(user); return userProfile; }

In my opinion, if there is something wrong with the credentials, then a good default is not authorized (for the endpoint).

In this case, some of the user profile fields are not populated. It's not something wrong with the credentials. For basic auth, credentials are username/password.

raymondfeng · 2019-05-02T19:56:31Z

packages/authentication/src/__tests__/fixtures/services/jwt-service.ts

+      // decode user profile from token
+      userProfile = await verifyAsync(token, this.jwt_secret);
+    } catch (error) {
+      throw new HttpErrors['Unauthorized'](


Let's use new HttpErrors.Unauthorized instead.

raymondfeng · 2019-05-02T19:58:07Z

packages/authentication/src/__tests__/fixtures/strategies/jwt-strategy.ts

+
+  constructor(
+    @inject(JWTAuthenticationStrategyBindings.TOKEN_SERVICE)
+    public token_service: JWTService,


Please use camelCase tokenService.

raymondfeng · 2019-05-02T19:58:29Z

packages/authentication/src/__tests__/fixtures/strategies/jwt-strategy.ts

+    if (!request.headers.authorization) {
+      //throw an error
+      throw new HttpErrors.Unauthorized(`Authorization header not found.`);
+    } //if


Please remove //if

raymondfeng · 2019-05-02T19:59:37Z

packages/authentication/src/__tests__/fixtures/users/user.repository.ts

+    if (found) {
+      return this.list[found];
+    }
+    return undefined;


It can be simplified as return found ? this.list[found]: undefined;.

raymondfeng · 2019-05-03T04:49:03Z

packages/authentication/src/__tests__/fixtures/strategies/basic-strategy.ts

@@ -36,20 +36,20 @@ export class BasicAuthenticationStrategy implements AuthenticationStrategy {
    if (!request.headers.authorization) {
      //throw an error


This comment does not add any value here.

bajtos · 2019-05-03T06:15:19Z

packages/authentication/docs/authentication-system.md

+          e.code === AUTHENTICATION_STRATEGY_NOT_FOUND ||
+          e.code === USER_PROFILE_NOT_RETURNED
+        ) {
+          throw new HttpErrors.Unauthorized(e.message);


I feel this is adding too much friction for application developers. The current proposal is is good enough for now (incremental baby steps FTW!), but I'd like you to create a follow-up SPIKE issue to look into ways how to allow @loopback/authentication (and any other 3rd party extensions) to contribute custom error-code to status-code mapping. See the current hard-coded map here:
https://github.com/strongloop/loopback-next/blob/22400fe9a40b17042a5df684300f8a007dd50edd/packages/rest/src/providers/reject.provider.ts#L12-L16

@bajtos :

I'd like you to create a follow-up SPIKE issue to look into ways how to allow @loopback/authentication (and any other 3rd party extensions)

@jannyHou mentioned there was a task to work on something like this.

Do you mean Enable custom reject implementation to leverage the built-in "code to statusCode" mapping ?

@bajtos

I feel this is adding too much friction for application developers

I am not sure what you mean. The code you mention is inside a custom sequence that user must write... we are not shipping the sequence which includes the authentication action. The two agnostic errors I am specifying here are thrown from code that cannot throw protocol specific errors since that code may support protocols other than http in the future. My mock (not production) implementations for strategies, token service and user service need to throw some kind of errors. I decided for my test bucket to have them throw http errors because my test bucket runs a rest server that handles http requests. The custom sequence rethrows these http exceptions as is, and throws the captured agnostic errors as http errors .

Since users will be writing their own production-ready implementations for sequence, token service, user service, and strategies , I am having a hard time understanding how the code above is painting the user into a corner. If you could clarify. thanks :)

@emonddr

Do you mean Enable custom reject implementation to leverage the built-in "code to statusCode" mapping ?

Not really. The goal of #1942 is to allow custom implementations of reject to read data from the mapping table. I.e. if the application developer decides to write a different error handling routine, they should have a way how to determine the HTTP status code to return based on error code.

In this discussion, I am asking for a mechanism that will allow extension developers to contribute new mapping entries, so that reject implementations can pick them up.

I am not sure what you mean. The code you mention is inside a custom sequence that user must write...

I find the custom sequence you are proposing as too complex. I would like the custom sequence to look like this:

class SequenceIncludingAuthentication implements SequenceHandler { // ... async handle(context: RequestContext) { try { const {request, response} = context; const route = this.findRoute(request); await this.authenticateRequest(request); const args = await this.parseParams(request, route); const result = await this.invoke(route, args); this.send(response, result); } catch (error) { this.reject(context, error); return; } } }

Under the hood:

When @loopback/authentication is registered as a component via app.component, it contributes new mappings to codeToStatusCodeMap.

The built-in reject action picks up the mapping contributed by the authentication extension and converts AUTHENTICATION_STRATEGY_NOT_FOUND to 401 Unauthorized.

BTW the current implementation is problematic because it discards the original error stack trace pointing to the place where the error was triggered (e.g. in the authentication strategy) and replaces it with a stack trace pointing to the custom sequence. This makes troubleshooting rather difficult.

Here is a better solution:

try { await this.authenticateRequest(request); } catch (e) { if ( e.code === AUTHENTICATION_STRATEGY_NOT_FOUND || e.code === USER_PROFILE_NOT_FOUND ) { err.statusCode = 401; } throw err; }

Good point about the stack trace. I will look into it. :)

bajtos · 2019-05-03T06:17:41Z

packages/authentication/docs/authentication-system.md

+        // strategy not found error, or user profile undefined
+        if (
+          e.code === AUTHENTICATION_STRATEGY_NOT_FOUND ||
+          e.code === USER_PROFILE_NOT_RETURNED


I find the code USER_PROFILE_NOT_RETURNED not very clear - what does it mean that user profile was not returned? Did the code looking it up freeze and never return back to the caller?

I am proposing to use USER_PROFILE_NOT_FOUND code instead, following the convention used by AUTHENTICATION_STRATEGY_NOT_FOUND and ENTITY_NOT_FOUND (from @loopback/repository).

raymondfeng

@emonddr Thank you all the efforts to improve the PR over multiple iterations.

Resolve authentication strategies registered via extension point BREAKING CHANGE: the new interface and authentication action in 2.0 will require users to adjust existing code

emonddr requested review from bajtos and raymondfeng as code owners April 17, 2019 18:11

jannyHou reviewed Apr 18, 2019

View reviewed changes

jannyHou referenced this pull request Apr 18, 2019

feat: refactor auth action

53cfeeb

jannyHou reviewed Apr 22, 2019

View reviewed changes

emonddr commented Apr 23, 2019

View reviewed changes

emonddr force-pushed the dremond_authentication_2312 branch 3 times, most recently from 53f74bd to 0c022bb Compare April 24, 2019 20:28

emonddr changed the title ~~[WIP] feat: resolve authentication strategy registered via extension point~~ feat: resolve authentication strategy registered via extension point Apr 24, 2019

hacksparrow reviewed Apr 25, 2019

View reviewed changes

jannyHou reviewed Apr 25, 2019

View reviewed changes

jannyHou reviewed Apr 26, 2019

View reviewed changes

emonddr force-pushed the dremond_authentication_2312 branch 3 times, most recently from 45f1fd4 to bcf041e Compare April 28, 2019 06:05

jannyHou mentioned this pull request Apr 29, 2019

Authentication and Authorization extension points #1035

Closed

emonddr force-pushed the dremond_authentication_2312 branch from 6096b8b to f016ff5 Compare April 29, 2019 19:12

emonddr commented Apr 29, 2019

View reviewed changes

emonddr force-pushed the dremond_authentication_2312 branch from f016ff5 to 694c068 Compare April 29, 2019 19:28

raymondfeng reviewed Apr 29, 2019

View reviewed changes

raymondfeng reviewed May 1, 2019

View reviewed changes

b-admike reviewed May 1, 2019

View reviewed changes

emonddr force-pushed the dremond_authentication_2312 branch from 6427f68 to a724eec Compare May 2, 2019 18:40

raymondfeng reviewed May 2, 2019

View reviewed changes

raymondfeng reviewed May 3, 2019

View reviewed changes

bajtos reviewed May 3, 2019

View reviewed changes

raymondfeng approved these changes May 3, 2019

View reviewed changes

feat: resolve authentication strategy registered via extension point

146d9ca

Resolve authentication strategies registered via extension point BREAKING CHANGE: the new interface and authentication action in 2.0 will require users to adjust existing code

emonddr force-pushed the dremond_authentication_2312 branch from 7edb7ee to 146d9ca Compare May 3, 2019 18:57

emonddr merged commit e8b8e8b into master May 3, 2019

dhmlau mentioned this pull request May 4, 2019

Extension point for registering the authentication strategies #2312

Closed

6 tasks

This was referenced May 12, 2019

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

Merged

Authentication example is not working #2886

Closed

dhmlau mentioned this pull request May 14, 2019

[Epic] MVP: authentication extension points #2892

Closed

6 tasks

emonddr mentioned this pull request May 15, 2019

LB4 - Authorization Implementation - Cannot find name AuthResponse - Authorization sample #2896

Closed

emonddr mentioned this pull request Jun 3, 2019

loopback/authentication this.fail is not a function #3032

Closed

emonddr deleted the dremond_authentication_2312 branch June 13, 2019 17:59

		@@ -0,0 +1,25 @@
		import {BindingKey} from '@loopback/context';


		this.sequence(SequenceIncludingAuthentication);

		addExtension(

		@@ -36,20 +36,20 @@ export class BasicAuthenticationStrategy implements AuthenticationStrategy {
		if (!request.headers.authorization) {
		//throw an error

feat: resolve authentication strategy registered via extension point #2763

feat: resolve authentication strategy registered via extension point #2763

Conversation

emonddr commented Apr 17, 2019 • edited

Checklist

emonddr commented Apr 17, 2019

jannyHou 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

Choose a reason for hiding this comment

raymondfeng commented Apr 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emonddr Apr 24, 2019 • edited

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 Apr 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emonddr Apr 28, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emonddr Apr 28, 2019 • edited

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 Apr 28, 2019 • edited

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

jannyHou 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

Choose a reason for hiding this comment

emonddr Apr 29, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emonddr commented Apr 28, 2019

jannyHou commented Apr 29, 2019 • edited

emonddr Apr 29, 2019 • edited

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 Apr 30, 2019 • edited

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

b-admike 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emonddr May 1, 2019 • edited

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 Apr 17, 2019 •

edited

emonddr Apr 24, 2019 •

edited

emonddr Apr 28, 2019 •

edited

emonddr Apr 28, 2019 •

edited

emonddr Apr 28, 2019 •

edited

emonddr Apr 29, 2019 •

edited

jannyHou commented Apr 29, 2019 •

edited

emonddr Apr 29, 2019 •

edited

emonddr Apr 30, 2019 •

edited

emonddr May 1, 2019 •

edited