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

v0.6 - Bugs fixes, new hooks, and hook tests #109

Merged
merged 27 commits into from Mar 24, 2016

Conversation

Projects
None yet
2 participants
@ekryski
Copy link
Member

ekryski commented Mar 21, 2016

I'm splitting my local work into two PRs to make it easier to review and migrate to. This one has a bunch of bug fixes, some breaking changes to hooks, and adds a bunch of hook tests.

Changes

  • Fixes for #107, #103, #102, #105
  • Adds a bunch of tests (#9, #59)
  • All hooks now pull from auth config (#93)
  • Added ability to disable local and OAuth2 redirects independently (#89)
  • Removed toLowerCase hook. It already lives in feathers-hooks
  • Renamed requireAuth hook to restrictToAuth
  • Renamed queryWithUserId hook to queryWithCurrentUser
  • Renamed setUserId hook to associateCurrentUser
  • Renamed restrictToSelf hook to restrictToOwner as it could be used on other resources other than users
  • Added a restrictToRoles hook

@ekryski ekryski force-pushed the v0.6 branch from bf376bb to d86f002 Mar 24, 2016

if (Array.isArray(hook.data)) {
hook.data.forEach(item => {
setId(item);
});

This comment has been minimized.

@daffl

daffl Mar 24, 2016

Member

Nit: hook.data.forEach(setId);

This comment has been minimized.

@ekryski

ekryski Mar 24, 2016

Author Member

👍

@@ -1,23 +1,29 @@
import bcrypt from 'bcrypt';

This comment has been minimized.

@daffl

daffl Mar 24, 2016

Member

After merging this I'll update #113 if necessary

This comment has been minimized.

@ekryski

ekryski Mar 24, 2016

Author Member

👍

if (!hook.params.user) {
// TODO (EK): Add a debugger call to remind the dev to check their hook chain
// as they probably don't have the right hooks in the right order.
throw new errors.NotAuthenticated();

This comment has been minimized.

@daffl

daffl Mar 24, 2016

Member

No error message?


const defaults = {
idField: '_id',
ownerField: 'userId'

This comment has been minimized.

@daffl

daffl Mar 24, 2016

Member

👍


export default function(options = {}){
return function(hook) {
if (hook.type !== 'before') {

This comment has been minimized.

@daffl

daffl Mar 24, 2016

Member

I would think this hook could also be an after hook for get where it just checks the ownerField of result

This comment has been minimized.

@daffl

daffl Mar 24, 2016

Member

Maybe then we even want to make this the same as the restrict-to-self hook from the guide.

This comment has been minimized.

@ekryski

ekryski Mar 24, 2016

Author Member

You thinking that it removes any documents that you don't own? That's pretty easy to do.

This comment has been minimized.

@ekryski

ekryski Mar 24, 2016

Author Member

Let's create a new issue for this and do it in a new PR. This one is already way too massive, and it won't be a breaking change.

This comment has been minimized.

@daffl

daffl Mar 24, 2016

Member

Totally. This one is good to go.

if (options.roles.indexOf(role) !== -1) {
authorized = true;
}
});

This comment has been minimized.

@daffl

daffl Mar 24, 2016

Member

Imho easier to read: const authorized = roles.some(role => options.roles.indexOf(role) !== -1);

This comment has been minimized.

@ekryski

ekryski Mar 24, 2016

Author Member

👍

@@ -1,3 +1,5 @@
if (!global._babelPolyfill) { require('babel-polyfill'); }

This comment has been minimized.

@daffl

daffl Mar 24, 2016

Member

I know you have an issue with this but do we need this here?

This comment has been minimized.

@ekryski

ekryski Mar 24, 2016

Author Member

Not really anymore. It was more so that I could run tests that don't require feathers individually without having to run the whole suite.

This comment has been minimized.

@daffl

daffl Mar 24, 2016

Member

We can probably move it into the tests then.

// If we should redirect on success and the redirect route is the same as the
// default then we'll set up a route. Otherwise we'll leave it to the developer
// to set up their own custom route handler.
if (authOptions.shouldRedirectOnSuccess && authOptions.successRedirect === defaults.successRedirect) {

This comment has been minimized.

@daffl

daffl Mar 24, 2016

Member

Does it need shouldRedirectOnSuccess or could you just use successRedirect = false?

This comment has been minimized.

@ekryski

ekryski Mar 24, 2016

Author Member

Ya good call. It was late when I was working on that.

@daffl

This comment has been minimized.

Copy link
Member

daffl commented Mar 24, 2016

Awesome! Just a couple of notes. Then :shipit:

ekryski added some commits Mar 24, 2016

@ekryski ekryski merged commit fca25f2 into master Mar 24, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@ekryski ekryski deleted the v0.6 branch Mar 24, 2016

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.