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

[Discussion] Migrating to 1.0 - hook changes #396

Closed
petermikitsh opened this Issue Jan 14, 2017 · 5 comments

Comments

Projects
None yet
2 participants
@petermikitsh
Copy link
Contributor

petermikitsh commented Jan 14, 2017

As I have been migrating to v1.0, I have encountered some concerns with the hook changes that merit some discussion.

As detailed here, the migration docs instruct us to re-write hooks from this:

[
    auth.verifyToken(),
    auth.populateUser()
]

to this:

[
  auth.hooks.authenticate('jwt')
]

I appreciate the decision to make hooks more generic with respect to authentication strategy. 1.x came with solid design changes that make auth more flexible.

However, the implementation of the v1 authenticate hook (in the case of a JWT auth) performs the dual functionality of both verifyToken -and- populateUser. A slight mistake in the docs, but they suggest we replace populateUser with populate from feathers-hooks-commons, which performs entirely different function.

In my particular use case, I had service endpoints, such as find, that used only the populateUser hook. In my application logic -- if there was a user present in the hook -- then I would check their privileges and return more entities (whereas unauthenticated or unprivileged users would only get a subset of entities). To be more concrete, think of this as a CMS inside of my application, and only administrators can see unpublished posts.

But if I put the authenticate in place of populateUser, my service will return 401 errors when no JWT is present-- which is an undesired change in behavior for my service endpoint. So, in the meantime, I've made of my own general purpose populateUser hook for my service (which I adapted from the 1.x authenticate hook):

(function (strategy, opts = {}) {
  return function (hook) {
    const request = {
      query: hook.data,
      body: hook.data,
      params: hook.params,
      headers: hook.params.headers || {},
      cookies: hook.params.cookies || {},
      session: {}
    };
    return hook.app.authenticate(strategy, opts)(request).then((result = {}) => {
      if (result.success) {
        hook.params = Object.assign({authenticated: true}, hook.params, result.data);
      }
      return Promise.resolve(hook);
    });
  };
}('jwt'))

This implementation is a bit more forgiving. If a JWT is present, it will try to add user data to the hook's parameters, but if it can't, it will continue along anyways-- providing analogous behavior to 0.x populateUser.

tl,dr: I think the concept of a general purpose populateUser hook is worth keeping around in 1.0. While there were big changes in 1.x around generalizing strategies, deprecating a valuable hook seems misaligned with the rest of the changes. Generalized, small, lego-sized-piece-of-functionality hooks are great for developers to achieve a desired functionality. That's what I liked about 0.x and I want to continue seeing that in 1.x.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Jan 14, 2017

If a JWT is present, it will try to add user data to the hook's parameters, but if it can't, it will continue along anyways.

I must be missing something because this is exactly what feathers-authentication-jwt does right here.

You can do both anonymous auth or have a user attached. It's totally up to you. You'll only be getting a 401 if your token is invalid, not if the user doesn't exist.

In my particular use case, I had service endpoints, such as find, that used only the populateUser hook. In my application logic -- if there was a user present in the hook -- then I would check their privileges and return more entities (whereas unauthenticated or unprivileged users would only get a subset of entities). To be more concrete, think of this as a CMS inside of my application, and only administrators can see unpublished posts.

You can do all of this with auth v1. You should just do the same thing. If the user does not exist then obviously you can't check if they have permissions so they should have anonymous user status. feathers-permissions makes this even easier.

There are a few different ways to customize things if the default is not the way you want it:

  1. Customize the Verifier for the auth plugin
  2. Write your own custom passport auth strategy
  3. Write some custom permissions logic
  4. Wrap your auth.hooks.authenticate call in a conditional hook.

I think really what you want is option 4. Where you check that a token exists and if it doesn't you just proceed. This is effectively making the endpoint unauthenticated but being able to handle a request with a token to get additional user info.

Hope that helps 😄

@petermikitsh

This comment has been minimized.

Copy link
Contributor Author

petermikitsh commented Jan 14, 2017

If a JWT is present, it will try to add user data to the hook's parameters, but if it can't, it will continue along anyways.

I must be missing something because this is exactly what feathers-authentication-jwt does right here.

I was referring to the authentication hook logic. That's where the user object gets attached to the hook, and if no JWT is present, a 401 error is returned.

If the user does not exist then obviously you can't check if they have permissions so they should have anonymous user status.

To rehash -- in 0.x, we just don't pass a JWT to signify unauthenticated. In 1.x, we pass in an anonymous JWT when calling the service endpoint to avoid returning a 401. I'll give that a try, that sounds like that remove my need for my custom populateUser replacement hook.

Hope that helps 😄

It does help! I think the confusing thing is that there's a good number of moving parts in the upgrade. It seems the new replacement hooks won't function as expected unless you're utilizing new 1.x constructs, like anonymous JWT's.

I'll give this another go, and if I run into anymore issues, I will re-open this. Thank you, @ekryski!

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Jan 16, 2017

Sounds good @petermikitsh! I agree there are a lot of moving parts. Also, having the docs out of sync isn't helping but we're working feverishly on that. 😄

@petermikitsh

This comment has been minimized.

Copy link
Contributor Author

petermikitsh commented Jan 17, 2017

@ekryski Just wanted to follow up and say that adding logic to get an anonymous JWT in my client made consuming the API work as expected.

Would you accept a PR for a small edit to the migration docs? I'd be happy to clarify the nuances of 1.x hook usage.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Jan 17, 2017

@petermikitsh great to hear! 💯 a PR would be great. 😄

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.