add $client hook #65

eddyystop · 2016-12-07T19:00:26Z

It will help people feel safe and warm inside,
instead of wondering if they’re doing something that’s a hack.
Will put docs in GitBook once this is merged.

- It will help people feel safe and warm inside, instead of wondering if they’re doing something that’s a hack. - Will put docs in GitBook once this is merged.

marshallswain · 2016-12-07T23:34:28Z

src/new.js

@@ -225,3 +225,49 @@ export const isNot = (predicate) => {
    return result.then(result1 => !result1);
  };
 };
+
+/**
+ * Move params from client to hook.params.


marshallswain · 2016-12-07T23:36:34Z

src/new.js

+  };
+};
+
+const reservedParamProps = ['app', 'authenticated', '__authenticated',


Should sequelize and mongoose be in this list?

Do any other adapters have special props? Knex? Stripe?

Not sure. I don't think so but I'd have to look.

permitted and __permitted have now been removed. The permissions hooks no longer use those fields based on this discussion feathersjs-ecosystem/feathers-permissions#4 (comment).

Its hook.app not hook.params.app. So app should be removed.

ekryski · 2016-12-08T20:05:13Z

test/client.test.js

+      assert.throws(() => {
+        $client(
+          'app', 'authenticated', '__authenticated',
+          'permitted', '__permitted', 'provider', 'query'


Just forgot to remove permitted and __permitted.

Thanks for that catch!

daffl · 2016-12-09T14:04:16Z

I'm not sure about reservedParamProps since the hook shouldn't really know about any other plugin that could potentially have restricted properties.

We might be able to define things like authenticated as read-only properties so it would throw an error if you are overriding it. That won't take care of when they are not defined but I'd go as far as saying that if you put sequelize or something like that explicitly into the whitelist it's your own fault.

eddyystop · 2016-12-09T14:21:04Z

@ekryski and @marshall were proponents of the whitelist so we need their input.

ekryski · 2016-12-09T16:46:41Z

We might be able to define things like authenticated as read-only properties so it would throw an error if you are overriding it.

Yeah that's the plan but until I get that sorted the whitelist for that piece is a good prevention measure. So we can either hold up this hook or ship it and pull that stuff out in the future.

That won't take care of when they are not defined but I'd go as far as saying that if you put sequelize or something like that explicitly into the whitelist it's your own fault.

I agree but then we should probably rename those in the adapters to $sequelize and $mongoose or something to denote that they are special. It's still pretty easy to inadvertently overwrite them. I think we have a few cases where it is a little too easy to accidentally do the wrong thing cough .remove(null) cough.

marshall · 2016-12-09T16:59:36Z

you have the wrong Marshall :)

ekryski · 2016-12-09T17:21:37Z

Poor @marshall, pretty sure that has happened a couple times on some of our issues/PRs. Sorry about that mate! 😄

@eddyystop I think you meant @marshallswain

daffl · 2016-12-09T17:31:26Z

It's not implicit thought right? You have to explicitly whitelist a property. So if you explicitly whitelist something potentially dangerous there isn't much we can do about it so why try and do it with (some of) the stuff we know about right now?

Oh also, we probably want a reverse of this hook for the client side (so something that puts a whitelist of params into query.$client).

eddyystop · 2016-12-24T18:25:08Z

@daffl, your first comment is valid. I think another point of the whitelist was for the server to limit what the client may specify. It prevents what someone hacking from the browser console can do.

How will we package client side hooks? We'd also need a copy of them in this repo so the server can do what the client can.

Merry Christmas eve.

daffl · 2016-12-26T18:13:34Z

Probably fairly simple like:

function client(... whitelist) {
  return function(hook) {
    const client = hook.params.query.$client = {};

    whitelist.forEach(key => client[key] = hook.params[key]);
  }
}

eddyystop · 2016-12-27T13:59:56Z

I add the client-side hook as a separate PR

daffl · 2016-12-27T18:25:10Z

I still don't think that the reservedParamProps is necessary when people have to explicitly provide a whitelist (as I said, if someone is explicitly doing something wrong we can't help them) but if it's still controversial then

ekryski · 2017-01-05T20:07:00Z

ok 2 comments:

I think we should name the hook function client instead of $client. That's me being a bit pedantic but it will keep it consistent with the rest of the hooks. It looks a bit out of place being prefixed with $.
After reviewing again and thinking about it, I think I'm with @daffl on this now. We don't really need the reservedParamProps if someone whitelists them themselves, then it's their own fault and they are whitelisting some pretty buried props. I'll work to quickly prefix them with __ to have a bit more obscurity.

After that is done

- Also changed tests

ekryski · 2017-01-06T00:16:15Z

Looks good. @eddyystop!

eddyystop added 2 commits December 7, 2016 13:55

add $client hook

8081d85

- It will help people feel safe and warm inside, instead of wondering if they’re doing something that’s a hack. - Will put docs in GitBook once this is merged.

Fixed lint issue

0e7831e

marshallswain reviewed Dec 7, 2016

View reviewed changes

eddyystop added 2 commits December 8, 2016 10:55

Changes made as per comments

a9fc33b

Fixed docs

3881781

ekryski reviewed Dec 8, 2016

View reviewed changes

Fixed ekryski's issue

499010a

Merge branch 'master' into client

15bbc7c

This was referenced Dec 18, 2016

oops, where can I find the docs? feathersjs-ecosystem/feathers-authentication-management#7

Closed

Created support for event filter and permission hooks. Reorganized service hooks because of this. #75

Merged

eddyystop and others added 2 commits January 5, 2017 16:39

Merge branch 'master' into client

3a74b43

Changed hook name to client & removed reserved prop name list

cfda9cb

- Also changed tests

eddyystop merged commit 818474f into master Jan 6, 2017

eddyystop deleted the client branch January 6, 2017 02:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add $client hook #65

add $client hook #65

eddyystop commented Dec 7, 2016

marshallswain Dec 7, 2016

marshallswain Dec 7, 2016

ekryski Dec 7, 2016

marshallswain Dec 7, 2016

ekryski Dec 8, 2016

ekryski Dec 8, 2016

eddyystop Dec 8, 2016

ekryski Dec 8, 2016

eddyystop Dec 8, 2016

daffl commented Dec 9, 2016

eddyystop commented Dec 9, 2016

ekryski commented Dec 9, 2016 •

edited

Loading

marshall commented Dec 9, 2016

ekryski commented Dec 9, 2016

daffl commented Dec 9, 2016 •

edited

Loading

eddyystop commented Dec 24, 2016

daffl commented Dec 26, 2016

eddyystop commented Dec 27, 2016

daffl commented Dec 27, 2016

ekryski commented Jan 5, 2017 •

edited

Loading

ekryski commented Jan 6, 2017

add $client hook #65

add $client hook #65

Conversation

eddyystop commented Dec 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daffl commented Dec 9, 2016

eddyystop commented Dec 9, 2016

ekryski commented Dec 9, 2016 • edited Loading

marshall commented Dec 9, 2016

ekryski commented Dec 9, 2016

daffl commented Dec 9, 2016 • edited Loading

eddyystop commented Dec 24, 2016

daffl commented Dec 26, 2016

eddyystop commented Dec 27, 2016

daffl commented Dec 27, 2016

ekryski commented Jan 5, 2017 • edited Loading

ekryski commented Jan 6, 2017

ekryski commented Dec 9, 2016 •

edited

Loading

daffl commented Dec 9, 2016 •

edited

Loading

ekryski commented Jan 5, 2017 •

edited

Loading