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.

Sign up for GitHub

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

Jump to bottom

WebSocket Authorization requires policy to contain a context #1471

Closed
aardvarkk opened this issue Jun 7, 2022 · 0 comments · Fixed by #1472
Closed

WebSocket Authorization requires policy to contain a context #1471

aardvarkk opened this issue Jun 7, 2022 · 0 comments · Fixed by #1472

Comments

@aardvarkk
Copy link
Contributor

aardvarkk commented Jun 7, 2022 •
edited
Loading

Bug Report

When using a custom authorizer for a websocket, I was returning a policy that didn't contain a context value. It is marked as optional here. Serverless offline would return a 500 error code despite claiming that authorization was successful.

Current Behavior

Logs show offline: Authorization function returned a successful response: (λ: websocketAuthorize), but the caller receives back a 500 and the websocket does not connect.

Expected behavior/code

The server should successfully connect without the optional auth response containing a context.

Possible Solution

I will submit a PR to fix this.

Workaround

Provide an empty object as a context instead of leaving it undefined.
aardvarkk added a commit to aardvarkk/serverless-offline that referenced this issue Jun 7, 2022
@aardvarkk
fix: don't require context (dherault#1471)
ea37827 
- Fixes dherault#1471
- The policy's context should be *optional*, but the code crashed if the context wasn't provided
- Followed a model similar to createAuthScheme where the context is properly checked for existence before being validated and used
- Add a test to verify the websocket authorizer works when the policy doesn't contain a context. This test fails with the existing code.
- Un-skipped the tests so they're actually run and verified the test now passes.
@aardvarkk aardvarkk mentioned this issue Jun 7, 2022
Merged
dnalborczyk pushed a commit that referenced this issue Jun 14, 2022
@aardvarkk
fix: don't require context (#1471) (#1472)
aaccf96
This was referenced Jul 31, 2022
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: don't require context (#1471) aardvarkk/serverless-offline
1 participant
@aardvarkk