Feathers: Hook "all: [ protect('password') ]" breaks others hook's behaviour

[m0dch3n]

Steps to reproduce

In my user hooks, (as from the init template), I have the following :

const {keep, iff, isProvider} = require('feathers-hooks-common');
const {
  hashPassword, protect
} = require('@feathersjs/authentication-local').hooks;

```javascript
after: {
all: [
// Make sure the password field is never sent to the client
// Always must be the last hook
protect('password')
],
find: [],
get: [ iff(isProvider('external'), keep('id', 'email', 'firstName', 'lastName')) ],
create: [],
update: [],
patch: [],
remove: []
},

### Actual behavior
In this situation, my keep hook is ignored, and I receive all the fields from the database.

### Expected behavior
It should not return all the fields from my database.

Btw, the following is working

```javascript
  after: {
    all: [],
    find: [protect('password')],
    get: [ iff(isProvider('external'), keep('id', 'email', 'firstName', 'lastName')), protect('password') ],
    create: [protect('password')],
    update: [protect('password')],
    patch: [protect('password')],
    remove: [protect('password')]
  },

System configuration

Module versions

    "@feathersjs/feathers": "version": "3.1.0"
    "@feathersjs/authentication-local": "version": "1.1.0"
    "feathers-hooks-common": "version": "4.10.0"

NodeJS version: v9.5.0

Operating System: macOSX

[daffl]

all hooks get added before the method specific hooks so it is not the last hook. In this case you'll have to add it at the right spot to each method:

  after: {
    all: [
    ],
    find: [ protect('password') ],
    get: [ iff(isProvider('external'), keep('id', 'email', 'firstName', 'lastName')), protect('password') ],
    create: [ protect('password') ],
    update: [ protect('password') ],
    patch: [ protect('password') ],
    remove: [ protect('password') ]
  },

[m0dch3n]

ok no problem, I did this already to fix temporarly, because I thought, it's a bug... I found now the corresponding text in the feathers documentation

Pro Tip: When using the full object, all is a special keyword meaning this hook will run for all methods. all hooks will be registered before other method specific hooks.

As the template suggests to put protect('password') in the all section, maybe, we could update the helping text to something like

  // Make sure the password field is never sent to the client
  // Always must be the last hook
 // Attention: all is called before the specific hooks

[m0dch3n]

Or another idea, would be to make all register before in the before section, and in the after section, to register after the specific hooks ?

However, this would break some peoples hook flow, so maybe this needs a new MINOR release...

[daffl]

That's totally how it should work (and already the order in which application level hooks are executed) but it will be a breaking change. It's definitely not entirely clear without reading through the docs in what order it would run.

Another thing you also could do is add

service.hooks(hooks).hooks({
  before: protect('password')
});

To your service.js file.

[daffl]

Maybe what @alexisabril suggested of having additional first and last fields would be a good solution to clarify this.

[m0dch3n]

I had the same idea, having first and last would be great!

[leob]

I just came across this discussion, and I was wondering if in this case "discard('password')" could simply have been used instead of "protect('password')" ... but I assume there are good reasons why 'protect' was introduced (in Buzzard) - is it related to this discussion?

https://github.com/feathersjs/authentication/issues/434

So is it recommended to always use "protect('password')" rather than "discard('password')" ?

[daffl]

The difference is that protect will not change the actual result but still guarantee that all clients only see the safe version of the data (including real-time events).

The problem with just modifying (discard) the result conditionally is that if you e.g. patch a user in an internal call, clients will still get the patched event and it will contain all the data unless you modify the dispatch data in a filter/publisher (which I am fairly certain nobody is actually doing).

[leob]

Thanks for the detailed explanation! Understand it now.

Feathers is easy to get started with, but subtle and powerful, and my understanding of what it can do is still growing every day in "baby step" fashion by following the discussions on Slack and Github.

Extremely well designed stuff, and I'm still convinced it is (within the JS/node.js ecosystem) by far the best solution for the (communications oriented) app I'm trying to build.

[m0dch3n]

@daffl @alexisabril

I investigated a little bit in the behaviour of all:

If you register twice your hooks, by doing the following i.e.

  service.hooks(
    {
      before:
        {
          all: [hook1],
          create: [hook2]
        }
    });

  service.hooks(
    {
      before:
        {
          all: [hook3],
          create: [hook4]
        }
    });

Currently the order in create would be hook1, hook2, hook3, hook4

So, all is simply appended in the 2nd registration, and hook3 is not before hook2...

This is because in

https://github.com/feathersjs/commons/blob/master/lib/hooks.js#L228

push is used, instead of myHooks.unshift.apply(myHooks, hooks.all);

Which whould result in hook3, hook1, hook2, hook4

Which IMO is more correct, if the docs say:

Pro Tip: When using the full object, all is a special keyword meaning this hook will run for all methods. all hooks will be registered before other method specific hooks.

What do you think about the following commit

https://github.com/m0dch3n/commons/commit/2cc3167b2d166a493d285b5641aa7af9a1eccf48

The problem and idea, with all, first, last is then solved.

However, that if you do 2 registrations, you still have the problem with last now:

  service.hooks(
    {
      before:
        {
          last: [hook2],
          create: [hook1]
        }
    });

  service.hooks(
    {
      before:
        {
          last: [hook4],
          create: [hook3]
        }
    });

will result in hook1, hook2, hook3, hook4, so hook2 will not be after hook3...

[m0dch3n]

When I think further, hook2, shouldn't know anything about hook3 and hook4, so in that point of view, my solution is correct...

[daffl]

@leob Thank you! The goal with improvements (like hook.dispatch which the protect hook is using) is to hopefully get rid of as many of the subtleties as possible.

@m0dch3n Yep, that's exactly how it should work. We'd have to add tests and update the generator and documentation but I think that'd be a good change to make.

[m0dch3n]

@daffl

I think, we can't simply modify push to unshift for all, because this will affect projects, where there is more than 1 registration of hooks...

so maybe, we should abandon all, and do this in a new major upgrade only...

about the tests, so in a single registration

• first should garuantee that they are always before every other hook
• last should garuantee that they are always after every other hook

in a second registration

• first should be before all the other hooks from the registration before
• specific hooks should be appended after all the others hooks from the registration before (even if there was a last hook in the first round), but they should be before new last hooks
• last should be after the all the current hooks

for a nth registration same rules then in 2nd