-
-
Notifications
You must be signed in to change notification settings - Fork 794
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: Support websockets
authorizers
#1350
feat: Support websockets
authorizers
#1350
Conversation
d063557
to
e20ae2f
Compare
Hi @pgrzesik, now should pass tests on NodeJS 12 |
e20ae2f
to
3bb0d00
Compare
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.
I have one minor question, overall the implementation looks good 👍 Thanks @DocLM
endpoint.authorizer.type && | ||
endpoint.authorizer.type.toUpperCase() === 'TOKEN' | ||
) { | ||
if (this.log) { |
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.
Do you think we should just log here instead of erroring out? What's the reasoning behind it?
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.
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.
@pgrzesik I took inspiration from HTTP implementation.
From what I can see in the other server the current authorizers behaviour is to log and ignore the authorizer if not supported.
In fact this check is only because I want to maintain a common baseline on what is not supported with the HTTP API variant using authFunctionNameExtractor
but with Websockets I have to restrict the scope since at the moment only REQUEST
authorizers are available.
EDIT: I removed the duplicate check endpoint.authorizer.type
in the condition
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 clarification, it makes sense 👍
3bb0d00
to
d940f1f
Compare
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 a lot @DocLM 🙇
websockets
authorizers
Description
Extend current API Gateway implementation to support Authorizers with Websockets.
More information on AWS official docs: https://docs.aws.amazon.com/apigateway/latest/developerguide/apigateway-websocket-api-lambda-auth.html
Related:
Motivation and Context
This change is required since serverless-offline currently support authorizers only in HTTP routes.
This functionality now allow to test websocket authorizers without using real infrastructure.
How Has This Been Tested?
This change is tested using integration tests in
tests/integration/websocket-authorizer
.Tested functionalities includes:
Also manually tested against real authorizers used in real applications