feathers-hooks-common icon indicating copy to clipboard operation
feathers-hooks-common copied to clipboard

New Hooks: variableMulti and variableWhitelist

Open DaddyWarbucks opened this issue 3 years ago • 6 comments
trafficstars

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?

DaddyWarbucks avatar Apr 06 '22 21:04 DaddyWarbucks

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);

fratzinger avatar May 22 '22 16:05 fratzinger

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 [...]
  }
});

DaddyWarbucks avatar May 22 '22 23:05 DaddyWarbucks

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.

fratzinger avatar May 23 '22 09:05 fratzinger

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.

DaddyWarbucks avatar May 23 '22 16:05 DaddyWarbucks

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.

fratzinger avatar May 23 '22 17:05 fratzinger