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

verifyToken hook option to error #200

Closed
tryy3 opened this Issue May 8, 2016 · 4 comments

Comments

Projects
None yet
3 participants
@tryy3
Copy link
Contributor

tryy3 commented May 8, 2016

I believe that the verifyToken() should have an option to error or not to error when for example the token is invalid or undefined.

Currently, if its undefined or invalid it will throw an error, which makes sense.
But currently I have the need of verifying the token and if its set, it should verify it, but if its not set, it should just continue.
The idea is that I have a secure parameter in my database and if its set, then if your logged in with certain permissions, then it should be sent, if they aren't logged in, the data should be filtered and only certain data should be sent.

Currently what I have done is grab the hook from feathers-auth, and remove the erroring so it just returns the function.
But I believe other people might encounter this issue, so I thought I would make an issue about this.

I am not sure which aproach would be best, to either make a seperate hook that wont error or make an option, to either error or not to error.
Or possibly move the verifying part out into some method, so people can make their own verifyToken, like they can call the verify token, if the token isn't set, return a certain thing, if it is set but its not a valid token, return something else, not sure if this follows feathersjs aproach, but its an option.

I didn't want to make a PR because I am new to featherjs and I am not sure which design for this problem would be the best.
So let me know what you think about this.
For now I am just gonna copy the hook and make it not error, but it would be nice to have a inbuilt hook that does this for me.

@daffl

This comment has been minimized.

Copy link
Member

daffl commented May 8, 2016

We could definitely add that as an option. An easy way to work around your issue would be to wrap the existing hook like this:

const hooks = require('feathers-authentication').hooks;

app.service('messages').before(function(hook) {
  if(hook.params.token) {
    return hooks.verifyToken(hook);
  }
});
@tryy3

This comment has been minimized.

Copy link
Contributor Author

tryy3 commented May 8, 2016

Hmm... interesting, didn't think of that aproach.
But that wouldn't really do it either, considering if the token is invalid, I still want to go on just not set a payload.
I guess I could catch the error, if its invalid token or token isn't set, then just throw the error as normal.

But it would be nice with an option, to either throw the error or return it.

Edit: Actually I could just add some checks in the if statement to check if the user data is set in the params.
But like I said, it would be nice to have an option, I have seen some more cases with the inbuilt hooks where similiar cases has happend, in most cases I have just come up with a different aproach, but if you decide to add an option, it would be nice to go through some more hooks and find similiar cases.
I don't remember which hooks it was, but I can always take a second look at the inbuilt hooks and see if I can find more cases where situations like this would be nice to have an option to return the error instead of throwing it.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented May 21, 2016

#210 should help because you'll be able to add a callback for when to conditionally run the hook. In addition you'll be able to modify the hook object so that you can tack on whatever params you need.

@ekryski ekryski added this to the 1.0 milestone May 21, 2016

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Jun 30, 2016

We actually came up with a better solution. You can simply wrap any existing hooks in your own and deal with the error how you would like to. For example,

import { hooks } from 'feathers-authentication';

exports.hashPassword = function(options) {
  // Add any custom options

  function(hook) {
    return new Promise((resolve, reject) => {
      if (myCondition !== true) {
        return resolve(hook);
      }

      // call the original hook
      hooks.hashPassword(options)(hook)
        .then(hook => {
          // do something custom
          resolve(hook);
        })
        .catch(error => {
          // do any custom error handling
          error.message = 'my custom message';
          reject(error);
        });
    });
  });
}

This provides the ultimate flexibility and doesn't require a change to the core hooks.

@ekryski ekryski closed this Jun 30, 2016

@ekryski ekryski removed the Backlog label Jun 30, 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.