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

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

Closed
nborko opened this issue Jun 25, 2019 · 5 comments · Fixed by #1470

Comments

@nborko
Copy link

nborko commented Jun 25, 2019

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
Copy link
Contributor

jnardone commented Jul 2, 2019

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

@daffl
Copy link
Member

daffl commented Jul 2, 2019

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
Copy link
Contributor

jnardone commented Jul 2, 2019

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
Copy link
Author

nborko commented Jul 2, 2019

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
Copy link
Member

daffl commented Jul 19, 2019

Allright. I added a fix for this in #1470 because that restriction was indeed weird. With those changes it should always reliable find the default service we want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants