Feathers: auth v4 - is publicly accessible entity.get now required?

[jnardone]

Steps to reproduce

One of our services has a global disallow hook on the users service. This has worked fine in the past - we allow other services to call it, but nothing external should need to directly interact with the users service.

In v4, this gets an error because of a disallowed GET:

'Provider 'rest' can not call 'get'. (disallow)' stack='MethodNotAllowed: Provider 'rest' can not call 'get'. (disallow)

Looking at the jwt.ts code, I see:

    const result = await entityService.get(id, omit(params, 'provider'));

    if (!params.provider) {
      return result;
    }

    return entityService.get(id, { ...params, [entity]: result });  // <-- ??

Why does this code now pass the params to the (last) get call if a provider is present on the authentication call? This is somewhat antithetical to the whole idea of an auth service - if I'm authenticated the service is supposed to return a (filtered) user context back to the caller. (It also seems inefficient to be calling this twice for every external authentication)

As noted in the final comments on the PR that added this: https://github.com/feathersjs/feathers/pull/1320

Why isn't the entityService.get just calling with NO params to get the user details?

Expected behavior

I should not have to have a publicly available users GET for authentication to succeed

Actual behavior

Request fails :(

System configuration

4.0.0-pre.4 for all components
node 10.16
macos mojave

[daffl]

The payload now automatically includes the external version of the entity so it doesn't have to be retrieves separately again. Strategies can be customized to implement their own getEntity functionality.

[KidkArolis]

It's part of the new feature where feathers.authenticate() returns the user (the auth entity).

daffl will probably know better what you can do here (perhaps a config option is needed..), but in the meantime you can customise this to your needs by overriding getEntity method of the jwt strategy:

class CustomJWTStrategy extends JWTStrategy {
  async getEntity(id, params) {
    // perform the internal entity call for auth purposes
    // but return {} to the client
    return params.provider ? {} : super.getEntity(id, params)
  }
}

You might need to do this in your local and oauth strategies as well.

[jnardone]

I guess the question I really have is: what is this protecting? Why are the params being passed at all to the entity.get call? Wasn't the user already being returned as part of authenticate in earlier versions of feathers auth?

[KidkArolis]

No, it wasn't being returned previously. So by treating this as external call you're protecting whatever the app needs to protect (e.g. hashed password, among other app specific things).

[jnardone]

The current feathers auth client definitely returns a user as part of the result of a successful auth, so I'm not sure why you say it's not currently returned.

[daffl]

It does not unless you added something to do that yourself. That's why there is a full example in the current docs that shows encoding the token and getting the user (which is now not necessary anymore).

[jnardone]

sigh. you're right (though in our case we add it to hook.result in an after hook, it was buried.)

Sorry for my confusion. Requiring this (in the default state) to be externally accessible is not a good thing though. It provides an authenticated endpoint that a user could use to get info about ANY other user in the system.

I know I can override getEntity, but the default implementation feels wrong and leaky to me. It seems like it'd make more sense to be able to sanitize the user record in an after hook vs. passing in the params. I'll do this as a custom for now but something feels off.

[daffl]

How would that happen? It will only return the user _after_ their credentials have been successfully authenticated the same way as you did by adding it to hook.result (except more secure because it is treated as an external request which will remove protected fields like the password automatically).

[jnardone]

Forcing a developer to make it external is making assumptions about access controls and other hooks on the users service. Just because I authenticate to the users service doesn't mean the results are magically filtered to just me. I don't see how, with a valid token, I can't just do GET /users/:randomID over and over and get other users' info. But I think it's reasonable to assume that I should be able to get info about myself as part of an authentication.

How does auth magically know which fields to remove for an external request? If you mean the protect('password') hook on users, that's going to happen regardless of internal/external.

If there's another mechanism that's removing protected fields automatically, can't those be run inside auth after the user is retrieved to be sent back to the client?

[KidkArolis]

In my experience, Feathers is vey open by default, once you create a service everyone can perform all operations on that service. I usually have to add the following restrictions:

• restrict query
• validate body schema
• scope to user/organisation
• filter result
• filter event payload

Ideally, I'd also like Feathers to be whitelisting rather than blacklisting based, but right now this doesn't strike me as standing out, you're supposed to implement a secure users service, just like you should implement any service securely.

[jnardone]

"you're supposed to implement a secure users service, just like you should implement any service securely." - agree! Which is why I'm a little surprised that the default user lookup implementation assumes that the service can be reached externally (since it passes the params provider down as part of the request).

Like I said, we'll just deal with this with a custom provider.

[daffl]

The v4 generator will restrict the users service accordingly. A user will only be able to access their own id publicly by default (with secure fields like the password removed). It is however much more user friendly to return the user object upon authentication. There were a _lot_ of issues with having people do that manually.