feathersjs-ecosystem / feathers-hooks-common

Useful hooks for use with FeathersJS services.
https://hooks-common.feathersjs.com
MIT License
193 stars 90 forks source link

New Hooks: variableMulti and variableWhitelist #659

Open DaddyWarbucks opened 2 years ago

DaddyWarbucks commented 2 years ago

Database adapters offer us options to control multi and whitelist, but they do not provide us a way to change these options per some variable. For example, we may want to allow multi on the server but not on REST and Socket.

You can see some related issues here: https://github.com/feathersjs/feathers/issues/2550 https://github.com/feathersjs-ecosystem/feathers-hooks-common/issues/651

My first thought was something similar to disallow for controlling these per provider. Something like

  const multiHook = multi({
    server: true,
    // ... all others false be default?
    // or the user could define them I guess
  });

  const whitelistHook = whiteList({
    rest: [...],
    socketio: [...]
    // if left undefined, use the basic whitelist
    // so server would get the full whitelist here
  });

Note that to use these, you would need to set multi: true and whitelist: [...all possible operators] in the service options, then this hook would regulate them per provider.

I don't use the iff hooks much, but it may be better to create more generic multi/whitelist hooks and use those instead. See: https://github.com/feathersjs-ecosystem/feathers-hooks-common/issues/651

I am open to create a PR for these two hooks. But, which style would you all prefer? Hooks like described above or hooks that would be used in iff?

fratzinger commented 2 years ago

Thanks for this PR! I also felt the lack of conditional multi as in #651. What do you think of this approach for multi?:

import { isProvider } from 'feathers-hooks-common';

const multiHook = multi(isProvider('server'));

or more generally a cb:

import { isProvider } from 'feathers-hooks-common';

const multiHook = multi(context => context.params.isAuthenticated);
DaddyWarbucks commented 2 years ago

Yep. That makes total sense.

I suppose whitelist is more difficult because we would have to parse the query and check the operators. Its also a bit different because you would be returning the whitelist and not the result of a predicate.

For example

const whitelistHook = whitelist(context => {
  if (context.params.isAuthenticated) {
    return [...]
  } else {
    return [...]
  }
});
fratzinger commented 2 years ago

Yes, I think that would be the most flexible approach because it also allows more complex situations with if else/switch statements. 👍

I see another problem for whitelist. I don't know of a wildcard. What's the equivalent for native multi: true for whitelist? I think we need a native whitelist: ['*'] or whitelist: '*' before we can work on a context based feathers-hooks-common version.

DaddyWarbucks commented 2 years ago

You are right, there is no native support for "all operators" in whitelist, so I was thinking you would have to add ALL operators to the actual service options whitelist. Then the hook would regulate them down from there.

fratzinger commented 2 years ago

I thought about this recently because of sequelize' $dollar.notation$ which is a flaw by sequelize. I think we should support wildcard/regex/predicate for whitelist first.