feathersjs / feathers

The API and real-time application framework
https://feathersjs.com
MIT License
15.06k stars 752 forks source link

Omit provider field for getEntity #1903

Closed mkovel closed 4 years ago

mkovel commented 4 years ago

Hi, @marshallswain @daffl

I am using a feather-permissions plugin and figured out some weird behavior. When I cover users service with permissions, I have problems with authentication. I can not pass authentication for user who doesn't have permissions for users:get service. I made investigation and discovered next code in authentication-local strategy here.

    const password = data[passwordField];
    const result = await this.findEntity(username, omit(params, 'provider'));

    await this.comparePassword(result, password);

    return {
      authentication: { strategy: this.name },
      [entity]: await this.getEntity(result, params)
    };

And I do not understand, why findEntity is called with omitted field 'provider', but getEntity not? I think getEntity also internal call of users service and should repeat the behavior of findEntity, i mean also omits field 'provider'. This behavior will allow solve my case and doesn't conflict with goal of LocalStrategy.authenticate(). IMHO.

PS If I am right, let me know and i will prepare PR for this.

daffl commented 4 years ago

findEntity needs the password information to actually compare it. getEntity, as the documentation states

returns the external representation for entity that will be sent back to the client.

You can easily customize this behaviour. Usually a user always needs permission to the get on the users service and then limit access to the users id.

mkovel commented 4 years ago

@daffl Thanks for the quick answer, i did it in way that you described for customize this behavior. But in my case this is not suitable, because i am going to use users behalf of administrator and this role should be able to communicate with users without any limitation. Anyway it only my case and it can not be a reason for changing default behavior for LocalStrategy. But my suggestion for changing current behavior of LocalStrategy was based on excpectation of similar interaction LocalStrategy with findEntity and getEntity. But this is not so and i try understand a reason of that. Why do we need call getEntity in external way, because actually that is internal call from authentication service. Is'nt that, what i am missing?

Sorry if i am asking stupid questions.

daffl commented 4 years ago

Again, like the documentation for getEntity says:

returns the external representation for entity that will be sent back to the client.

This is usually the user field sent back to the client after successful authentication. It includes the provider (and all other information from the external call) because it can not include the password (or anything else that shouldn't be accessible externally).

mkovel commented 4 years ago

@daffl Do you mean that is safely way for getting user info for passing it as a part of authentication.create response ?

PS Ouch, I almost screwed up. I did not notice, that password hash exists in authentication.create response. Thanks a lot man.