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

Handling expired tokens #25

Closed
marshallswain opened this Issue Jan 18, 2016 · 9 comments

Comments

Projects
None yet
2 participants
@marshallswain
Copy link
Member

marshallswain commented Jan 18, 2016

In the middleware that checks the Authorization header, if the token is expired we are currently sending back a 401 Unauthorized response. I don't think that should be handled here. If it's an unprotected resource that doesn't require auth, shouldn't we treat it the same as if it were some anonymous user? I think that a requireAuth hook should probably handle whether or not that request is rejected. Any thoughts otherwise?

@marshallswain

This comment has been minimized.

Copy link
Member Author

marshallswain commented Jan 25, 2016

I'm going to fix this so that all authorization is handled through hooks. When an expired token is provided it will allow the request to go through as it would for an anonymous user instead of punishing those who have expired tokens.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Feb 2, 2016

At first I wasn't sure but I think I know what you are getting at here. In the decoupling branch I've moved this to a hook called verifyToken that can be registered as opposed to blanket middleware. It is currently registered as a before hook on the /auth/token service.

My question though now that I think about is, we probably want to always check to see if there is a token (like you were doing) and attempt to validate it only if it is there. Otherwise just move on to the next hook.

The reason being is that the end user would use req.user in middleware or params.user in hooks to do any authorization but because we aren't using sessions you need to "authenticate" the user on every request.

So my proposal is actually that we have middleware that fires before any other, that attempts to authenticate the user if a token was passed in, otherwise it just moves on through and we keep the verifyToken hook for the /auth/token service that returns a 401.

@ekryski ekryski modified the milestone: 1.0 release Feb 2, 2016

@marshallswain

This comment has been minimized.

Copy link
Member Author

marshallswain commented Feb 2, 2016

That makes sense to me. We can use a decodeToken hook that literally only decodes tokens and sets them up on hook.params.user, then use the requireAuth- or requireAuthForPrivate-type hooks to control access based on whether it's present.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Feb 2, 2016

Ya that what I was thinking but decodeToken would be middleware that is applied to any route. Basically global middleware, otherwise we'd have to explicitly add a decodeToken hook to each service the developer wants to protect.

Now that I type that out, maybe that's not such a big deal???

@marshallswain

This comment has been minimized.

Copy link
Member Author

marshallswain commented Feb 2, 2016

Yeah. It's not a big deal to do

service.before({
  all: [
    hooks.auth.decodeToken(), 
    hooks.auth.requireAuth()
  ]
});

I think having it be explicit, and not automatic, will be more helpful in the long run.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Feb 2, 2016

k 👍. That's makes it super easy then. If that's the case then we could just use:

service.before({
  all: [authHooks.verifyToken()]

You wouldn't even need the requireAuth call.

@marshallswain

This comment has been minimized.

Copy link
Member Author

marshallswain commented Feb 2, 2016

That works. Then decodeToken would only be needed if you want to implement some kind of custom access control logic.

@ekryski

This comment has been minimized.

Copy link
Member

ekryski commented Feb 5, 2016

Alrighty, so this is how this works now...

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

We can't rely on just using the req.feathers.user middleware hack because we aren't using sessions. So we need to decode the token, populate the user from the user id stored in the token payload and then check hook.params.user. This should work well for when we do introduce sessions because then it won't matter which way the user authenticates.

@marshallswain

This comment has been minimized.

Copy link
Member Author

marshallswain commented Feb 5, 2016

Looks really good!

@ekryski ekryski referenced this issue Feb 12, 2016

Merged

Decoupling #49

@ekryski ekryski closed this Feb 12, 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.
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.