Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Hooks: variableMulti and variableWhitelist #659

Open
DaddyWarbucks opened this issue Apr 6, 2022 · 5 comments
Open

New Hooks: variableMulti and variableWhitelist #659

DaddyWarbucks opened this issue Apr 6, 2022 · 5 comments

Comments

@DaddyWarbucks
Copy link
Contributor

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:
feathersjs/feathers#2550
#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: #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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants