Fix local auth strategy, pass authed entity for hooks to use

[KidkArolis]

Should it be

{ ...params, [entity]: result }

or

{ ...params, [entity]: result, authenticated: true }

?

[KidkArolis]

Because this is making 2 calls now, one internal to resolve entity and check credentials and 1 on behalf of the external call, I had to whitelist the email query param from external providers. I didn't have to do this before. So this was a bit of an unexpected thing to fix.

What if instead of using findEntity() the 2nd time, we instead use service.get(result.id, { ...params [entity]: result)? This should be a bit more universal.

Specifically, because in my case, it wasn't a straight up failure, because I wasn't allowing email in the query params from external calls, the findEntity call was essentially fetching 12 users and using the first matching one, which would then lead to incorrect user being returned to the clientside! Essentially my credentials were logging me in as a different user. I only noticed an issue because it was not matching the generated jwt, which is generated based on the original user id. Or something like that.

[KidkArolis]

I'm running into the same issue with the jwt strategy. It calls entityService.get(id, params), but I can't trust this call without knowing either that this is coming from auth strategy or without the user being passed in. So we should possibly turn this into a double call, one with omit(params, 'provider') and one with get, like in the local strategy. Shall I open yet another PR? 😬

[KidkArolis]

Had to use this locally, double get:

class CustomJWTStrategy extends JWTStrategy {
  async getEntity(id, params) {
    const { entity } = this.configuration
    const entityService = this.entityService

    if (entityService === null) {
      throw new errors.NotAuthenticated(`Could not find entity service`)
    }

    const result = await entityService.get(id, omit(params, 'provider'))
    return entityService.get(id, { ...params, [entity]: result, authenticated: true, [AUTHENTICATE]: false })
  }
}

[KidkArolis]

One suggestion, in addition to double fetching as appropriate in both local and jwt strategies, what about putting the second fetch into a dedicated method, something like resolveEntity, by default it would fetch the user with external params. But this way you could override it to return nothing or tu return { id } only, to save the extra fetch if it's not needed for your application.

[KidkArolis]

Not 100% sure yet, but I think there's another issue here with passing all params from client as is. The query could be meant for another resource, e.g.:

["find","api/emergency-contacts",{"userId":"8PTGGhJkIcJAoGOfs5zYT3q8","$limit":2}]

THis is query for emergency-contacts service. But the authentication strategy passes that as query to the users service when resolving the user. But not 100% sure that's the issue. Haven't looked more into it. Gotta run now.

[KidkArolis]

I have pushed another commit similarly fixing the jwt strategy. There are 2 issues in jwt authenticate/getEntity implementation the way I see it:

1. When authenticate() is called from within a service authenticate hook, we take all params that were intended for that service and forward them to the getEntity call. That's not gonna work, because those params are meant for the service in question, not the user service. I modified the code to omit query, but I'm wondering if the best course of action is to completely omit all params. I.e. when making an internal call to resolve the user for the hook context - I think the call should be entityService.get(id) - no params!
2. When authenticate() is called from the /authentication endpoint, we have to first resolve the user internally, and then resolve it for external usage (the way it's done in local strategy) so that correct permissioning and data filtering is applied.

My modification checks if the call is internal (coming from authenticate hook) or external (coming from /authentication endpoint) and either does the double entity resolution or does not.

As before, I suggest to add resolveEntity method to the base strategy class so that this behaviour could be removed if such an optimisation is relevant.