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: Improve authentication parameter handling #1333

Merged
merged 3 commits into from
May 8, 2019
Merged

fix: Improve authentication parameter handling #1333

merged 3 commits into from
May 8, 2019

Conversation

daffl
Copy link
Member

@daffl daffl commented May 7, 2019

For #1324 4), 5), 6) and 7) and should close #1320

@@ -42,7 +42,6 @@ export class JWTStrategy extends AuthenticationBaseStrategy {
throw new NotAuthenticated(`Could not find entity service`);
}

// @ts-ignore
return entityService.get(id, params);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if I'm wrong, but I think there's another edge case hiding here.

Say the client calls app.authenticate({ strategy: 'jwt', accessToken }) - which, for example, my app does on initial render (I think that's mandatory to authenticate the socket?). That calls strategy.authenticate(), which calls this.getEntity(entityId, params) passing all of the params through. But entity can't be resolved from an external call if it's unauthenticated – authenticate() hook is skipped, but restrictToOwner or any custom logic looking at the user object is not skipped. This is why I had to resort to differentiating between internal and external calls in my "patched" jwt strategy:

class CustomJWTStrategy extends JWTStrategy {
  async getEntity(id, params) {
    const { entity } = this.configuration
    const entityService = this.entityService

    if (entityService === null) {
      throw new errors.NotAuthenticated(`Could not find entity service`)
    }

    const result = await entityService.get(id, omit(params, ['provider', 'query']))

    if (params.provider) {
      return entityService.get(id, { ...omit(params, 'query'), [entity]: result })
    } else {
      return result
    }
  }
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, that makes sense, will add it to this PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allright, it should be updated to work the same for both, the local and JWT strategies.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing. This all looks good to go to me.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks daffi but not only authentication service suffers from this (NotFound Error) on service entities (while app.services is returning all of them intact and in place!!) still exists on feathers via socketio (express is fine) I have had to handle it via socket common hook (like it's authorization!!) which is not the reason i rushed toward feathers in the first place , which was integration.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not exactly sure what you mean. A separate issue with a small example to reproduce would be helpful.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, sure . just wanted to say thanks , great job and the rest I'll issue.

@daffl daffl merged commit 6e77204 into master May 8, 2019
@daffl daffl deleted the auth-params branch May 8, 2019 01:13
EliSadaka pushed a commit to yusernetwork/authentication-oauth that referenced this pull request Oct 20, 2020
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.

3 participants