Multiple JWT auth strategies clash

[DaddyWarbucks]

Steps to reproduce

Create an additional JWT strategy by extending the JWTStrategy. Register the new strategy (alongside the default jwt strat) and try to use the new strategy. In this example the new strategy is called "preview"

Please see this repo: https://github.com/DaddyWarbucks/test-feathers-auth
for more detailed explanation, comments, and debugging.

I have also noticed that things act differently between a socket provider and rest provider. Checkout the index.html to switch between setup(socketApp) and setup(restApp) to easily switch between the two.

I believe that the problem exists in the connection hook for sockets and potentially in the express-auth lib for rest.

Expected behavior

When making a request to a service that has the authenticate('preview') hook, the request is authenticated with the "preview" strategy.

Actual behavior

The request attempts (and fails) to authenticate with the "jwt" strategy.

[DaddyWarbucks]

So I have made some headway on this. The root of the problem is in the core AuthenticationBaseService and it's handleConnection method. Which can be seen here:

feathers/packages/authentication/src/core.ts

Line 234 in 859c601

for (const strategy of strategies) {

The method loops over the strategies and calls each strategy's handleConnection method. For the JWTStrategy (and the strategy I made that extends the JWTStrategy) that handleConnection method sets the connection.authentication. This means that whatever strategy (that has a handleConnection method) that is registered last will be the one set to connection.authentication. So even if the user authenticated with the jwt strategy, if preview is registered later then the connection.authentication.strategy is set to preview.

I was able to solve this with

class Service extends AuthenticationService {
  async handleConnection(event, connection, authResult) {
    // The update to the filter function below means that only one
    // strategy is being set to the connection.authenticatoin. Previously
    // when all strategies (which was really just the one jwt strat),
    // the handleConnection basically defaulted to jwt because it was the
    // only strat with a handleConnection method. So when authing
    // with local, handleConnection just set the one jwt strategy it
    // knew about and everything was fine. Since there are multiple
    // handleConnections now...we have to manually default local to somehting
    if (authResult && authResult.authentication.strategy === 'local') {
      authResult.authentication.strategy = 'jwt';
    }

    const strategies = this.getStrategies(
      ...Object.keys(this.strategies)
    ).filter(
      current =>
        typeof current.handleConnection === 'function' &&
        // Only set the connection if this was the strategy that
        // was actually used in the authentication
        authResult &&
        authResult.authentication.strategy === current.name
    );

    for (const strategy of strategies) {
      await strategy.handleConnection(event, connection, authResult);
    }
  }
}

I am not a fan of the part where I reassign the authResult.authentication.strategy from "local" to "jwt". It seems like the LocalStrategy should also have a handleConnection method. But, obviously we wouldn't want to set the connection.authentication to

{
  strategy: 'local',
  username: '...',
  password: '...'
}

But that is actually correct...because the authenticate('local') hook is going to try to authenticate with the local strategy, username, and password on the connection.

Instead the local strat's handleConnection method should probably set the connection.authentication to

{
  strategy: 'jwt',  //or how do we specify this default?
  accessToken: '...'
}

[daffl]

Thanks for digging into this! Looks like that part of the code is causing a couple of problems (like the one I've been trying to fix in #1889). What the handleConnection was really supposed to accomplish is emulating the flow of a REST request where you get the access token and then all subsequent request use the token.

I think the best way to solve this would be to use the same logic as for header parsing via .parse (where the first authStrategies or parseStrategies that returns will be used) instead of running through all strategies.

[DaddyWarbucks]

I don't think that will solve the problem. That still relies on the order in which the strategies are registered. Right now the developer can set schemes per strategy and that would work, but cause them to deviate from the standard Authorization: Bearer .... And I believe that is only going to work for REST because socket does not send "headers" on every emit.

I am not sure what the appropriate way for the client to set a "strategy" header would be? I know that is also probably not standard either. But I feel like that would solve a lot of problems.

[daffl]

With the authStrategies (or if they are different parseStrategies) you set, the first one that applies will be used. E.g. if you have [ 'jwt', 'preview' ] for a REST request, the jwt one will parse the Authorization header and return successfully. What I'm saying is that the same should happen for connection handling. The first strategy that can handle the connection will be used as the subsequent auth mechanism for the connection.

You're right, that it may also make sense for e.g. the client to indicate what they want to use but that change would at least align connection handling with Authorization header parsing behaviour.

[DaddyWarbucks]

In my case the jwt and the preview authentication strategies expect different token payloads and handle those differently. jwt is a standard JWTStrategy class that looks up a user from the token payload user id. PreviewStrategy token payload has a shipment_id that looks up a shipment. Provided the jwt and preview are both using the same header scheme, then when the jwt strategy is selected (because it is registered first) and the token is really a "preview" token is where the problem I am describing is happening.

I figured this couldn't be the first time this problem has happened. I found this with some quick Googling: hapijs/hapi#3444. That makes sense and kinda what I was thinking anyway. At the end of the day, if you are using the same header scheme for multiple auth strategies you've got to have a switch somewhere that determines what kind of strategy that token is for. That kind of logic probably shouldn't be baked in feathers-auth by default.

So on REST I think things work as is. The user can either use a different header scheme per strategy (breaks "standards" but just works), or update the service authenticate method to handle the different token types. In regards to my previous comments about clients sending strategy headers, I don't think we want to have REST clients sending additional headers. As helpful as it would be, it's just not standard and will make feathers auth funky to clients. The developer needs to solve this in user land. I have an idea about adding the strategy in the jwt payload itself...but meh.

On to the handleConnection method, If we just accept the first strategy (instead of all) that can handle the connection we still don't know how to get the "right" one. The user can still extend the authenticate method, but we would still set the connection to the wrong strategy I think.

But, socket connections don't really have a standard in their payload. We are already specifying some DSL in the payload such as get::users (I know thats been updated but don't know the syntax) and data, query, etc. If we where to add headers to the payload, we could make the socket connection more stateless and analogous to the REST flow. This could be done largely unnoticed by anyone using feathers socket clients and would only affect those constructing their own socket clients. Primus too I suppose.

On a REST client we already parse the headers into the underlying rest provider (fetch, axios, etc). And on the Express side, feathers-express parses those back onto and puts them onto the params for the app to consume. We could simulate this on the socket side. The socket-client could pass the additional { headers: { 'Authorization': 'Bearer 231123' } } under the hood just like REST, and the server side socket handler could pass those along for the app to consume. Then we wouldn't need the actual connection to know anything about authentication. Each request is validated against the "headers" it sends.

I am thinking you have probably thought of this before and potentially turned away from it? I could see an argument that its "too far from the transport layer". For example because it doesn't happen in socket.io middleware and happens at the service parsing level that it is happening in the wrong place? Not sure if I explained that well.

Let me know your thoughts on sending headers in sockets. This is something I would be willing to spearhead. I have run into a number of issues around the connection being stateful as I think back on some of the previous issues I have opened around auth.

[deskoh]

I'm facing similar issue for parsing HTTP headers when using multiple JWT strategies. What I did was to check the issuer (JWT strategies should have unique issuer(s)) and return null if the issuer is not 'whitelisted'. The behaviour for parse is documented here.

This still relies on the ordering of authStrategies or parseStrategies, unless the built-in JwtStrategy adopts this approach as well.

See this snippet for example implementation.

The minor downside is that each JwtStrategy is unable to throw errors if the issuers is invalid.

[daffl]

@DaddyWarbucks is this completed? I was going to see if there is something more that could be done which is why I left it open.

[DaddyWarbucks]

I suppose not. I was just cleaning up old, open issues I had created. I probably should not have closed this one because it had so many comments. I'll reopen it. Sorry about that!

[juarezfranco]

Hi everyone,

I had a lot of trouble getting multiple JWT strategies working that mixed sockets and REST. The custom service by @DaddyWarbucks helped a lot to understand the main problem, and after spending long hours going deep into the code, I was able to understand the architecture and the problems involved not only with the service but also with the "authenticate" hook. I found the way it was designed to be a bit strange.

Well, as I said, I spent a few hours going deep into the code, and I'm going to share with you the strategy I took, which could maybe be considered for inclusion in the future.

Let's get started.

First, I created my service like @DaddyWarbucks did, with a small difference: I will store the strategy in the token, so that I can use multiple strategies per route in my custom hook.

class CustomAuthenticationService extends AuthenticationService {
  constructor(app: Application) {
    super(app)
  }

  // Unfortunately, super.getPayload completely ignores _authResult, so we need to rewrite it. 
  // Otherwise, I could have done this in my own token strategy.
  async getPayload(_authResult: AuthenticationResult, params: AuthenticationParams) {
    return {
      ...await super.getPayload(_authResult, params),
      // add strategy inside jwttoken, to access after in customAuthenticate
      strategy: _authResult.authentication.strategy,
    }
  }

  async handleConnection(event: ConnectionEvent, connection: any, authResult?: AuthenticationResult) {
    const strategies = this.getStrategies(...Object.keys(this.strategies)).filter(
      (current) =>
        typeof current.handleConnection === 'function' &&
        authResult?.authentication?.strategy === (current as any).name
    )

    for (const strategy of strategies) {
      await strategy.handleConnection!(event, connection, authResult)
    }
  }
}

Here is an example of my custom strategy for another entity:

export class AdminLocalStrategy extends LocalStrategy {

  async authenticate(data: any, params: any): Promise<any> {
    const result : any = await super.authenticate(data, params);
    // this is the authResult, I would like to add the payload that goes to jwt here, but as I said in the comment above, getPayload disregards this guy, although it passes as a parameter
    return {
      authentication: {
        // rewrite, default is always 'jwt' when use LocalStrategy
        strategy: 'admin-jwt',
      },
      user: {
        _id: result.user._id,
        username: result.user.username
      }
    }
  }

  get configuration() {
    const config = super.configuration;
    return {
      ...config,
      service: adminPath,
      usernameField: 'username',
      passwordField: 'password',
      entityUsernameField: 'username',
      entityPasswordField: 'password',
    };
  }
}

And finally my hook customAuthenticate

export default (...strategies: string[]) =>
  async (context: HookContext, next: NextFunction) => {
    const params = context.params
    const authService = context.app.service('authentication') as any

    // check strategy if socket
    if (strategies.includes(params.connection?.authentication?.strategy)) {
      const accessToken = params.connection?.authentication?.accessToken
      await authService.verifyAccessToken(accessToken, params.jwt)
      return next()
    }

    // check access token from header
    const authorization = (params.headers?.authorization || '').split(' ')
    if (authorization.length < 2) {
      throw new NotAuthenticated()
    }
    const accessToken = authorization[1]
    if (!accessToken) {
      throw new NotAuthenticated()
    }

    // check token strategy (included in CustomAuthenticationService)
    const payload = await authService.verifyAccessToken(accessToken, params.jwt)
    if (!strategies.includes(payload.strategy)) {
      throw new NotAuthenticated()
    }

    // authenticate to check user
    try {
      await authService.strategies[payload.strategy].authenticate({ accessToken }, params)
    } catch (e) {
      throw new NotAuthenticated()
    }

    return next()
  }

Now in my services, I use the customAuthenticate

import customAuthenticate from "../../hooks/custom-authenticate";
// ...
 app.service(zonePath).hooks({
    around: {
      all: [customAuthenticate('jwt')],
      }
})
// or multiple strategy by method
app.service(zonePath).hooks({
    around: {
      all: [customAuthenticate('jwt', 'admin-jwt')], // jwt or admin-jwt
      }
})

It worked great for me, I hope it helps someone