-
Notifications
You must be signed in to change notification settings - Fork 2k
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
feat(pubsub): authenticated push requests #1256
feat(pubsub): authenticated push requests #1256
Conversation
I can't add reviewers here 😢 /cc @anguillanneuf @bcoe |
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.
- Confirm there's no need to check
iss
in JWT. - Keep old push endpoint code as it is. Wrap new auth push endpoint code in a new region tag.
- Please update
README.md
as well. a). Include instructions to set up authenticated push subscription. b). Include expected behavior when testing locally.
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.
LGTM 👍
If you get a chance, perhaps just double check that those certificate is invalid ...
Just an idea, if Node's crypto library supports it, perhaps before the:
-----BEGIN RSA PRIVATE KEY-----
Add
This is a fake certificate used for testing.
-----BEGIN RSA PRIVATE KEY-----
☝️ then when this is pulled around in the future, this question won't come up in review.
@bcoe I pulled them from the Python samples, so I think they should be fine, but @anguillanneuf would probably know for certain. |
@anguillanneuf thank you for confirming 👍 |
No description provided.