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

Validate if provider #39

Merged
merged 5 commits into from Feb 9, 2016

Conversation

Projects
None yet
4 participants
@mastertinner
Copy link
Contributor

mastertinner commented Feb 5, 2016

fix some hooks to not fire if the request is internal

@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Feb 6, 2016

For queryWithUserId, it would probably be better to remove the else and error throwing, which would remove the need for a provider check, I think. That way, if a hook.params.user is in place, it will automatically add it. Then it isn't duplicating functionality from the requireAuth hook, but works with it. Here's an example that uses some of the new hooks in the upcoming OAuth release:

messageService.before({
  find: [
    authHooks.verifyToken({secret: 'feathers-rocks'}),
    authHooks.populateUser(),
    authHooks.requireAuth(),
    authHooks.queryWithUserId()
  ]
});
@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Feb 6, 2016

I think the same could be said for restrictToSelf and setUserId. Thanks for your help!

@mastertinner

This comment has been minimized.

Copy link
Contributor Author

mastertinner commented Feb 6, 2016

thanks for your feedback, @marshallswain! i adapted the PR accordingly.

Tobias Fuhrimann added some commits Feb 6, 2016

Tobias Fuhrimann
Tobias Fuhrimann
@daffl

This comment has been minimized.

Copy link
Member

daffl commented Feb 9, 2016

@marshallswain Good to go? We can merge it. There may be some conflicts with the decoupling branch by @ekryski but I don't think the hooks changed too much.

@mastertinner Thank you for contributing!

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Feb 9, 2016

👍 Ya we can merge this stuff. I did make a couple changes to hooks but I'll sort out the conflicts.

Mad props to @mastertinner for his help! 💥

ekryski added a commit that referenced this pull request Feb 9, 2016

@ekryski ekryski merged commit 144095c into feathersjs:master Feb 9, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@marshallswain

This comment has been minimized.

Copy link
Member

marshallswain commented Feb 9, 2016

Thanks for taking care of this, y'all. I'm a little spread thin right now. :)
Thanks @mastertinner for your help.

@mastertinner

This comment has been minimized.

Copy link
Contributor Author

mastertinner commented Feb 9, 2016

thanks for the support, guys!

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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.