-
Notifications
You must be signed in to change notification settings - Fork 236
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
Integrate with passport-jwt #27
Conversation
Thanks for the PR. Can you add some tests? |
@sandrinodimattia tests added. Getting the error messages to bubble up to validate expectations is a little strange with passport + express, but it gets the job done. |
@sandrinodimattia is there anything I can do to move this towards a merge? |
Hey guys, are there any plans to merge this PR? |
I sort of feel like maybe I shouldn't have spent my time to write the tests? |
@gconnolly thanks for your contribution. I am planing to use your PR along with @nestjs/passport |
"So when is this getting merged?" |
fix passport export
# Conflicts: # index.d.ts
# Conflicts: # index.d.ts
@gconnolly I send you PR with latest upstream merged. Also now exposing |
Almost a year later and still waiting... |
updated to 1.3.0 upstream
@xmlking Somehow I missed the notification of your PR 🤷♂️ . Merged! thank you. I noticed several version changes to the package.json. I am having difficulty tracing them back to the corresponding upstream commit, and they are not currently in master. CC @SeanLMcCullough thanks for the ping. Hopefully we get a merge before its first birthday 🍰 |
Any plans on merging this? What's the hold up? Anything I can do to help? |
reverted some unintentional dependency changes. Will continue to maintain this PR, while there are still people inquiring about it. |
Please merge this. I really need this. |
This has been waiting for over a year... any chance this is going to happen? |
Please merge! |
@gconnolly any plans to merge this soon? |
please merge 🙏 |
This worked well for me. Thanks for the work on the PR! Looking forward to seeing it merged. |
@sandrinodimattia Plz merge. |
If anyone wants to use this NOW, I've made a fork of this fork (jajaperson/node-rwks-rsa) with the |
This would be really useful. Waiting for it. |
please merge this |
@Mutmatt @sandrinodimattia @lbalmaceda @gconnolly Not sure who is the owner/maintainer of this repo, but is there any work to be done on this ticket, and who could merge it when it's finished. If there are some updates needed I can spare some time to work on it, as it would be nicer to stay with the official package, without using forked repos 😄 |
@lbalmaceda @damieng @Mutmatt @cocojoe @sandrinodimattia Could you or any of your colleagues finally merge this? |
What's going on with this ticket? Someone? Anyone? Hellooooo? What are the next steps to make this bad boy merge? |
Oye! I can check on this on Monday. Not trying to pass the buck, however, this repo isn't owned or maintained by my team. We just used it. I'll see if I can get the group some traction |
@Mutmatt any progress? |
We are actively discussing this PR internally and certainly value the effort, time, and energy everyone has contributed here. I don’t yet have an update to report but will provide one when I am able to. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the follow up @Mutmatt. Someone from our side has reviewed the PR. I'll proceed to merge this and make a new release.
😎 |
Now that auth0/node-jwks-rsa#27 is merged, we no longer have to use a fork of the package.
I created an integration with passport/passport-jwt, so I thought I would share it. It includes the integration as well as an example.