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 🐛: get the user from jwt token when its undefined #3132

Closed
wants to merge 1 commit into from

Conversation

hasan-dot
Copy link

Background:

This PR fixed the issue described here for OAuth2 callback

Testing strategy:

  1. Try building the project with the following env:
N8N_BASIC_AUTH_ACTIVE=true
N8N_BASIC_AUTH_USER=user
N8N_BASIC_AUTH_PASSWORD=pass
N8N_AUTH_EXCLUDE_ENDPOINTS="rest/oauth2-credential/callback"
  1. Try to add OAuth2 credentials for any node (I tried zendesk)
  2. The callback should work and not through an exception

PoF:

Screenshot 2022-04-13 at 13 02 45

@CLAassistant
Copy link

CLAassistant commented Apr 13, 2022

CLA assistant check
All committers have signed the CLA.

@Joffcom Joffcom added core Enhancement outside /nodes-base and /editor-ui community Authored by a community member labels Apr 13, 2022
@hasan-dot
Copy link
Author

@Joffcom

Replying to a message here:

It looks like it was reviewed internally but the PR only covers one endpoint for one case rather than removing the requirement fully for authentication on that end point

Yes the reason behind my solution is the following:

  • getCredentialForUser which is also using whereClause
  • both requires a User injected to the request
  • My hunch is there is a middleware somewhere doing this automatically if the request URL requires authentication
  • Since the url was whitelisted, User was not attached to the request

I didn't find where the user is being injected to I went with this approach

@csuermann
Copy link
Contributor

Thanks for the PR that kicked off fruitful internal discussions. #3168 should be addressing this issue.

@hasan-dot
Copy link
Author

@csuermann I'm really more than happy! Thanks for appreciation 🙏

@janober
Copy link
Member

janober commented Apr 25, 2022

Thanks a lot for your contribution @hasan-dot ! We merged PR #3168 which should fix your problem.

@janober janober closed this Apr 25, 2022
@hasan-dot hasan-dot deleted the fix_oauth2_callback branch April 25, 2022 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Authored by a community member core Enhancement outside /nodes-base and /editor-ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants