feat: introduce an authentication strategy interface

[emonddr]

Introduce an authentication strategy interface

Implements #2466

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 👈

[emonddr]

Implemented the function signature as

authenticate(request: Request, options: O): Promise<UserProfile>;

instead of

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

because this function should throw an error if authentication fails and a UserProfile cannot be returned.

[emonddr]

Didn't implement the optional function:

private extractCredentials?(request: Request): Promise<Credentials>;

mentioned in the design document since:

• it is not possible to have a private function in an interface
• we simply wanted to mention that the user could implement the extractCredentials function in the strategy implementation itself instead of using a separate service.

[jannyHou]

nitpick: export interface AuthenticationStrategy<O> {}

[raymondfeng]

1. I'm not keen on using O as a generic type name.
2. The options is passed in from authentication action. I don't think it's a good idea to type authenticate with specific options.

[emonddr]

@raymondfeng , yes...Janny and I spoke about this yesterday. Thanks for confirming that it isn't a good idea. There is no way to test that the interface fits in with all the providers, resolvers, actions etc except to go ahead and adjust all the surrounding files needed for a representative PoC. I am doing that at the moment...
This is why I placed [WIP] in the title... because I wasn't done yet.

[emonddr]

Removed options in the interface since the implementation classes of the interface can inject global strategy options, and inject the authentication metadata into the class constructor ( thereby obtaining the overriding options from the metadata provided by the optional options parameter from the @authenticate(type,options) decorator on the controller method). In short, the implementation strategy classes can obtain the global and overriding options via injection, and so the interface doesn't need to know about them.

[emonddr]

Introduced the name property in the interface to help a strategy resolver find a specific strategy registered via the extension point mechanism that will be introduced to register various authentication strategies.

[emonddr]

Not really possible to test this interface in a mocha test until we implement many other pieces in the authorization package. Instead I refactored the shopping cart example to use this interface, used 2 authentication strategies that implemented this interface, registered them as extensions of an authentication strategy extension point as a POC, and everything worked together. So I am confident with this interface at point in time.

[bajtos]

Instead I refactored the shopping cart example to use this interface, used 2 authentication strategies that implemented this interface, registered them as extensions of an authentication strategy extension point as a POC, and everything worked together.

Please include a link to the pull request making these changes, so that we can review the outcome.

[emonddr]

@bajtos , the shopping cart example modifications I made are not production yet ready. I only used it as a personal test for myself to play around with the extensionpoint/extension decorators Raymond created.

This interface we are introducing here identical to the strategy interface already working in the shopping example: https://github.com/strongloop/loopback4-example-shopping/blob/master/src/authentication-strategies/authentication.strategy.ts with the exception that there is a name:string field to help find a strategy by name in the forthcoming PR for registering different authentication strategies as extension to an extension point.

Since the interface is the same as the working one in the shopping cart example, I see no point in providing a PR of the same shopping cart example.

We will be revising the entire shopping cart with all the new authentication pieces that Janny and I are working on, in a separate task : loopbackio/loopback4-example-shopping#79

[bajtos]

In general, it's difficult to review changes in APIs and interfaces without seeing how the new version is going to be used. We usually write unit-tests to demonstrate the usage and verify the implementation. I understand this may be not practical here and that's why I am asking to see code using the new interface.

This interface we are introducing here identical to the strategy interface already working in the shopping example: https://github.com/strongloop/loopback4-example-shopping/blob/master/src/authentication-strategies/authentication.strategy.ts wiith the exception that there is a name:string field to help find a strategy by name in the forthcoming PR for registering different authentication strategies as extension to an extension point.

Great! This is exactly what I was looking for.

However! In the example, the authenticate method can return undefined, while your code is requiring the method to always return a user profile.

export interface AuthenticationStrategy {
  authenticate(request: Request): Promise<UserProfile | undefined>;
}

I have already pointed out this aspect, let's continue this discussion in #2688 (review) please.

[b-admike]

LGTM FWIW, glad that you've tested it with the shopping cart example app.

[jannyHou]

Cool! And I just created a draft PR as a prove of your interface works, see #2746

[nabdelgadir]

💯

[bajtos]

At the moment, authenticate can return either a user profile or undefined. I presume undefined is indicating there were no credentials provided by the client and thus the request is anonymous. (It's important to distinguish this case from the situation where the client provided some credentials but they were not valid - an error should be thrown in such case.)

In your new proposal, authentication strategies have to either return a user profile or throw an error.

How do you envision to handle anonymous requests with no credentials provided by the client?

I am not familiar with all details of the proposed authentication design. If anonymous requests are meant to be handled elsewhere in the new authentication solution, then please ignore my comment.

[emonddr]

@bajtos , what is meant by anonymous request? In the controller classes, there are endpoints that won't be decorated with @authenticate( '{authentication strategy name}' ), and some endpoints that will be decorated with it. The authenticate action in the sequence will not attempt any authentication on an endpoint which doesn't have an authentication strategy specified. If a strategy is provided, that strategy will return a user if and only if the user credentials are valid, otherwise the strategy will throw an http error.

[bajtos]

By anonymous request I mean a request that does not include any credentials. For example, by opening http://localhost:3000/orders in your browser.

Let's say this that the controller method implementing /orders is decorated with @authenticate(/*some strategy*/).

In the code we have so far, the authentication strategy returns undefined, meaning there are no credentials provided for the request, and it's up to authenticate() sequence action or even the sequence itself to decide how to handle the situation when an anonymous request is received for a resource that requires authentication.

IIUC your proposal correctly, you are pushing the responsibility of handling this situation to individual authentication strategies.

I have two concerns about that approach:

• Different strategy can throw different HTTP error. Applications using multiple strategies can end up with inconsistent error responses depending on which strategy was applied.
• It's more difficult to implement fallbacks, i.e. configure the app so that if one strategy did not find any credentials in the request, then we can try a different strategy to see if it's more successful.

Anyhow, I am not familiar with all details of the solution you are aiming for. If the change of authenticate signature is in line with your new overall design, then I am fine with that. As long as it's a conscious decision made after considering pros and cons of different approaches.

[b-admike]

@emonddr Can we keep the undefined in the return signature of the authenticate method given the above discussion and the points made by @bajtos? I am also leaning towards letting the sequence action or sequence handle anonymous requests in a unified way as opposed to delegating it to authentication strategies.

[emonddr]

I mention the shopping cart here because the sequence and authentication action it in will look similar in the authentication package...

Currently the shopping cart sequence looks like this:

export class MySequence implements SequenceHandler {
  constructor(
    @inject(SequenceActions.FIND_ROUTE) protected findRoute: FindRoute,
    @inject(SequenceActions.PARSE_PARAMS) protected parseParams: ParseParams,
    @inject(SequenceActions.INVOKE_METHOD) protected invoke: InvokeMethod,
    @inject(SequenceActions.SEND) public send: Send,
    @inject(SequenceActions.REJECT) public reject: Reject,
    @inject(AuthenticationBindings.AUTH_ACTION)
    protected authenticateRequest: AuthenticateFn,
  ) {}

  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 (err) {
      this.reject(context, err);
    }
  }
}

Currently the shopping cart authentication action in the sequence looks like this:

export class AuthenticateActionProvider implements Provider<AuthenticateFn> {
  constructor(
    // The provider is instantiated for Sequence constructor,
    // at which time we don't have information about the current
    // route yet. This information is needed to determine
    // what auth strategy should be used.
    // To solve this, we are injecting a getter function that will
    // defer resolution of the strategy until authenticate() action
    // is executed.
    @inject.getter(AuthenticationBindings.STRATEGY)
    readonly getStrategy: Getter<AuthenticationStrategy>,
    @inject.setter(AuthenticationBindings.CURRENT_USER)
    readonly setCurrentUser: Setter<UserProfile>,
  ) {}

  /**
   * @returns authenticateFn
   */
  value(): AuthenticateFn {
    return request => this.action(request);
  }

  /**
   * The implementation of authenticate() sequence action.
   * @param request The incoming request provided by the REST layer
   */
  async action(request: Request): Promise<UserProfile | undefined> {
    const strategy = await this.getStrategy();
    if (!strategy) {
      // The invoked operation does not require authentication.
      return undefined;
    }
    if (!strategy.authenticate) {
      throw new Error('invalid strategy parameter');
    }
    const user = await strategy.authenticate(request);
    if (user) this.setCurrentUser(user);
    return user;
  }
}

[emonddr]

@jannyHou 's new authentication action for authentication package will look similar. See : #2467 .

In the authentication action,
If a request comes through from an endpoint that is not decorated with a strategy(in controller via @authenticate('jwt') for example), then the action won't find a strategy and returns.

If a request comes throw that does have a strategy associated with it, that strategy's authenticate method is called and returns a user profile which is placed on the context. (Or it throws an error, and that error percolates up the try/catch in the sequence which has an error handler. I don't see the point for an authentication strategy to return undefined.

@bajtos @b-admike

[emonddr]

Are you saying that you want the authenticate method of a strategy to return { user: {value or undefined}, error: {value or undefined}}, and never throws an error, and it is up to the action to standardize the error handling in its block of code (regardless of strategy)? In this case, still... the strategy.authenticate will always return an object (never undefined), but it is the action that will handle throwing errors.

[bajtos]

I am asking to preserve the signature used by the current strategy interface in the shopping cart.

export interface AuthenticationStrategy {
  name: string;
  authenticate(request: Request): Promise<UserProfile | undefined>;
}

Take a look at the implementation of AuthenticateFn you posted above - it's already prepared to handle the case when the authentication strategy did not find any user credentials.

    const user = await strategy.authenticate(request);
    if (user) this.setCurrentUser(user);
    return user;

[emonddr]

Instead I refactored the shopping cart example to use this interface, used 2 authentication strategies that implemented this interface, registered them as extensions of an authentication strategy extension point as a POC, and everything worked together.

@emonddr Please include a link to the pull request making these changes, so that we can review the outcome.

Please see my comment above. thx.

[raymondfeng]

Please squash the commits before landing.

[raymondfeng]

@emonddr Please fix the CI failure - https://travis-ci.org/strongloop/loopback-next/jobs/520443467

[bajtos]

Please explain what is the expected strategy behavior when no credentials were found in the request.

For example:

If no user credentials were found or the user credentials are invalid,
this method will throw an error.

[emonddr]

Please explain what is the expected strategy behavior when no credentials were found in the request.

Hi @bajtos , the authenticate method on a strategy will throw an error like: https://github.com/strongloop/loopback4-example-shopping/blob/master/src/authentication-strategies/JWT.strategy.ts#L34.

For the authentication strategies that implement the interface:

An error should be thrown by the strategy if the controller method is decorated with an authentication strategy BUT the user has not passed in any credentials. (What you refer to as anonymous request)

An error should be thrown by the strategy if the controller method is decorated with an authentication strategy BUT the user from the credentials is not registered in the database (basic or local strategies)

An error should be thrown by the strategy if the controller method is decorated with an authentication strategy BUT the credentials are invalid ( invalid password or expired/invalid token)

A user profile will be returned by the strategy if the controller method is decorated with an authentication strategy AND the credentials passed in are valid.

If the controller method is NOT decorated with an authentication strategy, the authentication action in the sequence returns without performing any work, and the other actions in the sequence continue.

The authenticate method of a strategy will never return undefined...it will throw an error instead.

[bajtos]

Please explain what is the expected strategy behavior when no credentials were found in the request.

Hi @bajtos , the authenticate method on a strategy will throw an error like: https://github.com/strongloop/loopback4-example-shopping/blob/master/src/authentication-strategies/JWT.strategy.ts#L34.

As I understand that code, a different error will be thrown, see here:
https://github.com/strongloop/loopback4-example-shopping/blob/91805dd8740dce8b19bd7af613c485f7935cc8b5/src/authentication-strategies/JWT.strategy.ts#L21-L22

    let token = request.query.access_token || request.headers['authorization'];
    if (!token) throw new HttpErrors.Unauthorized('No access token found!');

An error should be thrown by the strategy if the controller method is decorated with an authentication strategy BUT the user has not passed in any credentials

Makes sense (if that's the indented design). I am asking you to capture this information in the tsdoc comment for the new interface.

[emonddr]

This code is from shopping cart.. it is old code. Going forward the code may be different. The point is that some error is thrown for a given strategy.authenticate().
This PR is about an interface not implementation.

[b-admike]

👍