v4 Authentication passes params to user entity

DaddyWarbucks commented 5 years ago

Steps to reproduce

The basic LocalStrategy passes params to the getEntity method but not to the findEntity method, specifically the provider. I have some hooks that disallow external calls to services, including the users service. This means that auth fails even though it is actually an "internal" call.

Expected behavior

The call to the underlying user's service should return the user as if it were made "internally".

Actual behavior

The provider is passed to the users service call as if were being made "externally"

Repo

Please see this repo: https://github.com/DaddyWarbucks/feathers-auth-test Open your browser to http://localhost:3030/ and open the dev console.

This is a new feathers app generate with all @pre tags. Things to note:

A new user is created in a memory service on each app startup
src/services/users/users.hooks.js. I have added a simple hook that throws if context.prams.povider is set.
public/index.html Tries to authenticate and throws the error.

This is probably not a "bug" and I realize that I can create my own Auth strategies. But, there have been a number of conversations about how passing params can be troublesome and I think this is an example of that. I assumed this would "just work" with my setup, but failed due to some param passing.

See: https://github.com/feathersjs/feathers/blob/96dd4d1e0b3c0b63028436a769d1179e0a4b92dd/packages/authentication-local/src/strategy.ts#L84

I am also curious of the actual point of getEntity after we have already done the findEntity. I am assuming that is just kinda how Grant works to be able to be flexible.

Edit I understand a bit more about the purpose of getEntity. It is used for returning the entity in the shape desired for returning to the client. And that is why provider is passed to it...so the protect hook works.

It seems like I just need to make my own Auth Strategy. Please feel free to close this. Like many things Feathers, the OOTB solutions are kind of guides and are easily extensible when needed.

KidkArolis commented 5 years ago

As you've discovered, in V4, the user entity is being returned to client after a succesful authentication, and that's what getEntity is used for. You can indeed override this behaviour by overriding the getEntitity method if you never want to return the user to the client.

I wonder if in development mode, we could catch errors in getEntity and if they are access denied errors, log a warning to developers about this edge case..

DaddyWarbucks commented 5 years ago

Understood. I am just going to extend the LocalStrategy with my own getEntity and findEntity methods. The main problem that I have run into is that params are passed to both of those method calls, and that is really one set of params being used for three different service calls: the create: /authentication, find: /users, and get: /users. This just simply breaks my app because of some extensive hooks on those services, specifically around the provider param.

On another note, I feel like there is an actual bug on this line: https://github.com/feathersjs/feathers/blob/96dd4d1e0b3c0b63028436a769d1179e0a4b92dd/packages/authentication-local/src/strategy.ts#L55

That should probably debug findParams.query or just query (both of which would be the same, but different from the original params.query).

KidkArolis commented 5 years ago

I think it should be as simple as this (should work the same for both Local and JWTStrategies):

module.exports = class MyLocalStrategy extends LocalStrategy {
  async getEntity (result, params) {
    const res = await super.getEntity(result, omit(params, 'provider'))

    // internal auth
    if (!params.provider) {
      return res
    }

    // external auth doesn't get the user obj
    return {}
  }
}

DaddyWarbucks commented 5 years ago

@KidkArolis Thanks for the help!

I landed on this

class Local extends LocalStrategy {
  async findEntity(username) {
    const { entityUsernameField, service, errorMessage } = this.configuration;
    const query = await this.getEntityQuery({
      [entityUsernameField]: username
    });

    const entityService = this.app.service(service);

    const result = await entityService.find({
      query,
      skipHooks: ['protect.password']
    });
    const list = Array.isArray(result) ? result : result.data;

    if (!Array.isArray(list) || list.length === 0) {
      throw new NotAuthenticated(errorMessage);
    }

    const [entity] = list;

    return entity;
  }

  async getEntity(result) {
    const { entityService } = this;
    const { entityId = entityService.id } = this.configuration;

    if (!entityId || result[entityId] === undefined) {
      throw new NotAuthenticated('Could not get local entity');
    }

    return entityService.get(result[entityId]);
  }
}

It is basically an exact copy of the OOTB LocalStrategy except I removed the ability to pass params from the /authentication service to those two methods. I instead set those params explicitly (in findEntity) or not at all (in getEntity). I could probably simplify them a bit, particularly in findEntity and use the findOne method that all of my services have via a mixin and get rid of the part that handles arrays. But, I am inclined to just leave it as close to OOTB as possible and leave a comment about why I overwrote them.

KidkArolis commented 5 years ago

Just make sure you're ok with the fact that the result of entityService.get() is being passed back to the UI after successful /authentication call.

DaddyWarbucks commented 5 years ago

I am closing this issue because it is not a bug. The developer can use the LocalStrategy without problems, but should be aware that the params sent to the /authentication service are "reused" for the two underlying calls to the entityService (generally /users). This may clash with that entityService's hook chain. If/When your app grows this may become a problem and if/when that happens it is advisable to extend or create your own LocalStrategy.

feathersjs / feathers