[Feathers 4.0.0-pre] Authentication issues

[KidkArolis]

I'm creating a little umbrella issue to keep track of the issues I've found with the auth in the 4.0.0. I will update this as things get addressed one way or another.

1) [FIXED] Backwards incompatible AUTHENTICATE symbol ✅

Previously, it was possible to pass authenticated: true param, for example in tests, to skip authentication hook and pass the user manually. in pre.0, this has been replaced with AUTHENTICATE: false sumbol. The change has been reverted and is now merged.

PR: #1317

2) [FIXED] Clientside app.logout() doesn't work ✅

Clientside app.logout() throws.

PR: #1319
Update: was only an issue if calling logout if already logged out.

3) [FIXED] Strategy errors get swallowed ✅

Each auth strategy is always being tried in sequence until a succesful match is found or they all fail. The issue with this is that the request might have been for strategy jwt but the first strategy in the list was local. In this case, we won't respond to the client with the real auth error. The issue is discussed in more depth here and a proposed fix has been suggested:

Discussion: #1323
PR: #1327

4) [FIXED] Local strategy fails to resolve user object in case hooks rely on user ✅

The new local strategy fetches user twice. Once as an internal call to retrieve the full user object and compare the password. And once more to get a "clean" user object to pass back to the clientside. When resolving the second time, the authenticated user object is not being passed to the find call, which means the call could fail if any hooks rely on the authenticated user object, e.g. to scope the call to an organisation with restrictToOwner.

PR: #1320

5) [FIXED] Local strategy makes new assumptions about user find() ✅

Previously, the /authenticate endpoint would only return the jwt access token. Now it also returns the resolved user, which is usually what you want. There might be one subtle issue with this approach in that this strategy now makes an assumption about whether the find() by username will work for an external call. Note: this is particularly dangerous if the user service strips out query params, e.g. if "email" or "username" gets stripped out, the local strategy selects all users (with limit: 1), essentially returning the wrong user (first match).

Suggestion: use get(id) to resolve the user object for external use.

6) [FIXED] JWT strategy fails because it forwards all params from authenticate hook ✅

When using authenticate('jwt') hook, the hook calls jwt strategie's authenticate method with the full params of the original service call. The issue is that that call might include some query that's not compatible with the user service. The fix could be either to omit query, or omit params entirely.

PR: #1320

7) [FIXED] JWT strategy fails to findEntity since it makes an external call ✅

JWT strategy is not doing the double user fetch like the local strategy does. Instead, it simply calls entityService.get() without omitting provider. This doesn't work if the user service relies on the call to be authenticated. Suggested fix is to make an internal call (omitting all params, see point 7 above) to get the user. In case this authentication is being performed for the authenticate hook - return this result, we're done. In case this authentication is being done for /authentication endpoint, make a second get() call to resolve the external representation of the user object, just like the local strategy does.

PR: #1320

8) [FIXED] Overriding OAuthStrategy is not possible ✅

Use of the default OAuthStrategy is currently hardcoded inside the express middleware service.register(key, new OAuthStrategy());. Suggestion: allow registed OAuthStrategy like the other strategies: authentication.register('oauth', new OAuthStrategy()), and grab that from within the middleware.

Update: this was not an issue, it is possible to register custom per provider strategies, e.g.:

authentication.register('google', new ExtendedOAuthStrategy())

9) [FIXED] Default OAuth express middleware is using in memory session store ✅

This means that a cookie is used (is it possible to avoid this? That was one of the goals of new auth? I'm not sure you can do oauth without server side storage though). More importantly, there is no way to customise the session storage being used, the in memory one is not always suitable for production use. Suggestion: accept express session options as options to the oauth() middleware.

10) [FIXED] OAuth Strategy with Google fails due to missing profile.id ✅

It seems that profile.id is not defined when authenticating against google with scopes openid email profile. It only has profile.sub. I don't know if this is provider specific, or a grant thing. There aren't many docs on that. I think it's sub according to the OpenID Connect spec: https://developers.google.com/+/web/api/rest/openidconnect/getOpenIdConnect. Although not sure if this would differ per each grant supported provider. Hm.

11) [FIXED] Errors in options.getRedirect() are uncaught ✅

Given that that's a user provided function, in case it throws unexpected error, I think that should be handled by feathers and passed to next(err).

12) [FIXED] Difficult to override OAuth findEntity query ✅

Previously, it was possible to pass makeQuery option, e.g.:

makeQuery: () => ({ $or: [{ [`${this.name}Id`]: profile.id }, { email: profile.email }] })

Now, you have to override the entire findEntity() method just to customise this query. Suggestion: add makeFindQuery or similar method to the OAuthStrategy.

[daffl]

Thanks for the thorough beta testing and summary! 🚀 Here's where things are at:

1. Backwards incompatible AUTHENTICATE symbol

• Merged with fix: Bring back params.authenticated #1317

2. Clientside app.logout() doesn't work

• Merged with Fix authentication client logout function to guard against null #1319 (added a question in the PR)

3. Strategy errors get swallowed

• Merged with fix: Always require strategy parameter in authentication #1327

4. Local strategy fails to resolve user object in case hooks rely on user

• Will review Fix local auth strategy, pass authed entity for hooks to use #1320
• Looks like the tests are failing. I think I might add a getParams to the authentication service so we can use that for all cases

5. Local strategy makes new assumptions about user find()

• Good call, I will add a PR

6. JWT strategy fails because it forwards all params from authenticate hook:
7. JWT strategy fails to findEntity since it makes an external call

• Strategies are usually just run when calling authenticationService.authenticate which always passes authenticated: true (skipping authentication hooks for all subsequent calls). Were you running into problems with this?

8. Overriding OAuthStrategy is not possible

• There is a test that makes sure that existing strategies are not overwritten and you should also be able to register custom ones over the default oAuth strategies afterwards. Did this not work?

9. Default OAuth express middleware is using in memory session store

• At least for oAuth1 there is unfortunately no way around a session
• This session store is only used for the Grant sub-app and the oAuth flow
• I was just about to argue that the values stored there a pretty transient and probably shouldn't be persisted anyway but you're right, when load balancing an app this will be necessary
• The goal for getting rid of the cookie was to not use it to pass the generated JWT to the frontend. This is now done with a redirect in the hash or query string - definitely still needs to be documented better

10. OAuth Strategy with Google fails due to missing profile.id

• Should we just use const profileId = profile.sub || profile.id;? grant-profile is literally just using what is coming back from the providers API so it's a good call to first go with the spec and then what most are doing.
• It's not unlikely that some oAuth providers might need additional customization if none of those values are coming back (will have to see if it comes up)

[KidkArolis]

Regarding 7, the user service can have a bunch of hooks that rely on params.user:

authenticate(‘jwt’)
filterDataBasedOnRole()
restrictToOwner()
etc

Authenticate hook works fine, because its skipped due to authenticated: true.

But other hooks fail! Because we are passing authenticated: true, but no user.

In general, we can’t compute what user object to return externally without knowing who is making the request. That is why i introduced the double call in that PR, just like its done for local strategy.

[daffl]

Yep, that's totally a valid issue. I will work from your PR #1320 to fix this. It just looked like you were also having a problem with the auth hooks running.

[KidkArolis]

Regarding 8, I reread the code mode carefully, indeed I should be able to register a custom per provider (e.g. 'google') strategy. I'll try that right now. Update: yup, works like a charm.

[KidkArolis]

Adding 11) Catch errors in options.getRedirect(). Given that that's a user provided function, in case it throws unexpected error, I think that should be handled by feathers and passed to next(err).

[KidkArolis]

All fixed! 🎉