Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[Feathers 4.0.0-pre] Authentication issues #1324

Copy link
Copy link
@KidkArolis

Description

@KidkArolis
Issue body actions

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.

Reactions are currently unavailable

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions

      Morty Proxy This is a proxified and sanitized view of the page, visit original site.