feathersjs / feathers

The API and real-time application framework
https://feathersjs.com
MIT License
15.06k stars 752 forks source link

When using custom authentication service paths, express+rest doesn't parse authorization header by default #1415

Closed nborko closed 5 years ago

nborko commented 5 years ago

For 4.0.0 (crow):

I have a case where I'm using express and rest services only. I have multiple authentication service paths (though you only need to set up any one custom service path) with their own configuration keys, and therefore no default authentication service. For example,

const app = express(feathers())
    .configure(configuration())
    .use(express.json({limit: '1mb'}))
    .use(express.urlencoded({ extended: true }))
    .configure(express.rest())

const authService = new AuthenticationService(app, 'custom')
authService.register('jwt', new JWTStrategy())
authService.revister('local', new LocalStrategy())
app.use('/custom/authentication', authService)

(also all my services under this authentication service are subpaths of /custom)

Anything using the authenticate hook, e.g.

authenticate({ service: 'custom/authentication', strategies: ['jwt'])

gets a 401, because at no point does the Authorization header get parsed.

After tracing the source, I resolved this by adding the following line:

app.use('/custom', express.parseAuthentication({ service: 'custom/authentication' }))

Since custom service points and configurations is prominently highlighted in the documentation, at the very least this needs to be documented, since it's a pretty huge gotcha (and took me hours to track down). It would be even better to take care of this when app.use is called to add the authentication service, but that would probably require extra configuration information.

jnardone commented 5 years ago

Interesting - I'm using custom services and not seeing this behavior.

daffl commented 5 years ago

If the configuration key and service path are the same this will work automatically. So in @nborko's case, app.use('/custom', authService) would have worked without additional configuration. The problem is that the service path is only set during setup (after the application is started). Another probably easier option would be to set defaultAuthentication manually:

app.use('/custom/authentication', authService)
app.set('defaultAuthentication', '/custom/authenticaton');
jnardone commented 5 years ago

Why do we need to have a defaultAuthentication? I don't know how I feel about an auth service being called implicitly that I don't know about.

So, if I have 2 auth services, and I'm using explicit configurations like the OP is doing, I either need to name my app.set config setting the same as the service endpoint or set a defaultAuthentication?

e.g.

  app.set('portal-auth', config);
  const service = new AuthenticationService(app, 'portal-auth');
  service.register('portal-jwt', new JWTStrategy());
  service.register('portal-local', new LocalStrategy());
  app.use('/portal-auth', service);
  app.service('portal-auth').hooks(hooks);

if I had called my config portalAuthConfig then this would have failed?

nborko commented 5 years ago

In my case, the actual config key doesn't match the service name. My example did for the sake of simplicity in explaining the issue.

In actuality my configurations are generated programmatically because my services are also generated dynamically from a database. The generated service names are something like 'foo/service' and 'bar/service' for the same service for two different databases and sites, both using the same models. They don't share a common authentication configuration: the entity services are different, as are the secrets etc. So although I recognize that in a simple case you can explicitly set the defaultAuthentication service, that doesn't work for me.

So I'm not really lamenting the complexity of what I need to do to support such a scheme, but I wanted to point out that there's an extra, undocumented step that's needed if you don't want/use a default authentication service and/or configuration.

daffl commented 5 years ago

Allright. I added a fix for this in https://github.com/feathersjs/feathers/pull/1470 because that restriction was indeed weird. With those changes it should always reliable find the default service we want.