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

fix(authentication-oauth): Allow req.feathers to be used in oAuth authentication requests #1886

Merged
merged 1 commit into from
Mar 24, 2020

Conversation

daffl
Copy link
Member

@daffl daffl commented Mar 23, 2020

@mikekolganov
Copy link

Thank you! I'll close my PRs now.

@daffl
Copy link
Member Author

daffl commented Mar 23, 2020

Thanks! Wanted to make sure that you agree it indeed addresses the same issue. Also, thank you for the report and pull request! Will go out with the next release shortly.

@daffl daffl merged commit 854c9ca into master Mar 24, 2020
@daffl daffl deleted the oauth-params branch March 24, 2020 04:15
@KidkArolis
Copy link
Contributor

Re posting from Slack:

This means params.provider = ‘rest’ is set now, and before it wasn’t, this can affect logic such as looking up entities via findEntity. In my case, findEntity (to find user) stopped working, because the request appeared to be external (due to provider being set), and you’re not allowed to get users if you’re not authenticated.

I need to update my logic to exclude provider in the oauth flow. But wondering if this could affect other peeps.

Perhaps Feathers oauth strategy should be updated to exclude provider somewhere in the flow of authenticate -> findEntity? (since this isn’t an external request, this is oauth internally trying to resolve an entity).

@daffl
Copy link
Member Author

daffl commented Apr 22, 2020

Ah darn, I didn't think of that but since there is a way to access the oAuth strategy directly through an external request (authenticate with an existing oAuth access token) it's probably a good thing to get the entities with protections in place.

@KidkArolis
Copy link
Contributor

Hm.. thanks for pointing that out, I might have to restrict that edge case further.

Thing is, the "protections" often depend on the user accessing the data, but in this case - there is no user.. unless we find the entity and then re-find it again with the user passed in, the way it's done in the jwt strategy, basically.

@daffl
Copy link
Member Author

daffl commented Apr 22, 2020

Yeah, I'm starting to wonder if the non-JWT strategies should only ever create an access token and not return the user. Maybe a longer lived refresh token that can only create new access JWTs which also returns the user information.

@sebastien-prudhomme
Copy link

Got also a side effect with feathers-permission because of these lines:

    if (!entity) {
      debug(`hook.params.${entityName} does not exist. If you were expecting it to be defined check your hook order and your idField options in your auth config.`);

      if (params.provider) {
        throw new Forbidden('You do not have the correct permissions (invalid permission entity).');
      }

      return context;
    }

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 this pull request may close these issues.

4 participants