-
Notifications
You must be signed in to change notification settings - Fork 15
Automatically register the authenticate hook with 'local' #4
Comments
Nope we cannot do that because it's not always For example we have: app.configure(authentication(app.get('auth')))
.configure(local())
.configure(local({ name: 'local-device', Verifier }))
.configure(jwt())
.configure(jwt({ name: 'jwt-device' }));
app.service('authentication').hooks({
before: {
create: [
authentication.hooks.authenticate(['local-device', 'local', 'jwt', 'jwt-device'])
],
remove: authentication.hooks.authenticate('jwt')
}
}); to support both device authentication using |
Ok. I made this to help others with my same mindset in the future: feathersjs-ecosystem/authentication#368 I think it would be nice to remove this step, if we can figure out a way to do so. I'm thinking we could add a top-level auth option of Anyway, this won't matter much with clearer docs. I got the rare opportunity to see this through the eyes of a noob, even though we reviewed this stuff a few weeks ago. :) |
I still don't get it. Why can't app.configure(authentication(app.get('auth')))
.configure(local())
.configure(jwt()) And I kind of can authenticate but not really. |
@daffl no you can not authenticate. Not without calling You also want to be able to set custom options (like on a per hook or per route basis). Trust me dudes, it's not a good call to auto set it up. That's exactly one of the reasons we ended up having to change auth so drastically in the first place, because options were buried and there was too much magic happening or things that were set up automatically got in the way of trying to do something custom. I don't see what the big deal is here. It's a couple lines and gives you much more flexibility. If it was confusing to migrate and you read through the migration guide then let's talk about how the guide and docs can be improved to make this easier before we make even more code changes. 😄 |
As per our discussion, I think we can register those hooks automatically if we make the |
I gave this more thought and I think this is simply something we should be doing in the generator. We should not bury it and automatically set it up. There are many times where you may not want that hook added by default and there are also custom options that can be passed to that Kind of putting my foot down on this one. I think it's a docs and generator issue and not something we should change. |
Not too happy with it but the generator could take care of it. Maybe we can add a comment and make the strategies configurable like app.service('authentication').hooks({
before: {
create: [
authentication.hooks.authenticate(app.get('auth').strategies)
],
remove: authentication.hooks.authenticate('jwt')
}
}); So that you can add them dynamically (and hopefully make it more clear). |
Instead of having to setup your auth config then manually register the authenticate hook, like this:
Can we move the process of adding the hook into the
local
module so the step of registering a hook on the authentication service isn't required?And we could do similar changes to the other strategies, even oAuth2, since we require a strategy
name
. Is it possible to make multiple calls toauth.hooks.authenticate()
?/cc @ekryski @daffl
The text was updated successfully, but these errors were encountered: