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

Be careful with discard('password') in user #434

Closed
MHerszak opened this Issue Mar 6, 2017 · 7 comments

Comments

Projects
None yet
5 participants
@MHerszak
Copy link

MHerszak commented Mar 6, 2017

I just realized I was adding discard('password') to after find hook for my users model. I got the Error: 'user' record in the database is missing a 'password'. Maybe just add a little reminder to be careful with discard('password') when it is actually necessary for checkCredentials for local authentication.

@daffl

This comment has been minimized.

Copy link
Member

daffl commented Mar 10, 2017

@eddyystop I know we talked about that it is pretty easy to make this conditional using commonHooks.when but I'm still wondering if it should only discard when a provider is set. This is the third or fourth time I've seen this problem come up.

@eddyystop

This comment has been minimized.

Copy link
Member

eddyystop commented Mar 10, 2017

There is a reason we wanted to remove the "magic" from inside some hooks. For example, more than once we've had people on Slack surprised that remove did not remove anything when called from on the server.

A name like discard should just discard, while it looks reasonable for a name like discardForExternal to be conditional.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Mar 20, 2017

@MHerszak makes a fair point, it is easy but it's also pretty apparent as to what the error is but it does warrant a bit more insight/pointers somewhere.

I don't think we should do anything other than maybe have a warning or log inside the hook. It should be consistent with the rest of the hooks and not assume the existence of a provider, otherwise we'd be repeating the mistake of remove (which in my opinion we should have just updated as part of v3.0.0 instead of adding yet another hook that appears to do the same thing).

@MHerszak

This comment has been minimized.

Copy link
Author

MHerszak commented Mar 22, 2017

Yeah, this is not simple. A quick fix would be to set this.options.passwordField to null but that would probably add other problems.

@snewell92

This comment has been minimized.

Copy link

snewell92 commented Aug 7, 2017

I created a symbol that I check for now.

export const AuthMode = Symbol('AUTH');

I use it as a property on the hook params, set it to a boolean. Then I can disable it after the authentication hook if I need default behavior for whatever reason.


const setAuthMode = hook => hook.params[AuthMode] = true;
const disableAuthMode = hook => hook.params[AuthMode] = false;
/* ... */

before: {
  create: [
    setAuthMode,
    authentication.hook.authenticate(config.strategies),
    disableAuthMode,
    populateUserPayload
  ], /* ...*/
}

There could be an internal symbol feathers uses, and then common hooks (like discard) could switch behavior based on the needs of other internal services based on either the presence of the symbol property, or the symbol property's value. That wouldn't interfere with those hooks' general behaviors, while still working seamlessly all together.

And the above authentication service hook example could be the output of the CLI, to be more explicit, or the authentication hook could set that param before it calls whatever is set as the user service.

@daffl

This comment has been minimized.

Copy link
Member

daffl commented Aug 8, 2017

I like the idea using Symbols. The next version of Feathers will also allow hooks to set the dispatch data which is what providers will use so hopefully some of those issues should be preventable.

@daffl

This comment has been minimized.

Copy link
Member

daffl commented Dec 18, 2017

This has been fixed via the protect hook.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.