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

feat: Add support for request authorizers to have a querystring identity source #1610

Merged
merged 1 commit into from
Nov 17, 2022

Conversation

kohanian
Copy link
Contributor

@kohanian kohanian commented Nov 11, 2022

Description

This PR adds support for request authorizers to look at a querystring for the identity source. With Serverless only supporting header, this adds one of the other identity sources that AWS supports.

Motivation and Context

This PR https://github.com/dherault/serverless-offline/pull/1600 was a fantastic addition, eliminating the need for any custom local authorizer workarounds. I'm using serverless on a current project, and it's great to remove the workaround. I have a use case where I have authorizers using the header, but I also have one that requires querystring. AWS supports 4 different identity sources per their documentation. Ideally, all are supported, but this can be iterated on.

How Has This Been Tested?

I worked with the integration tests in request-authorizers section. I ran serverless offline locally against the integration test template and tested first by pinging through postman to ensure it was doing as expected. Then I added integration tests on top of the current ones. I had to change a little bit of helper functions outside of that because the tests were all testing them as Bearer tokens, however in my querystring use cases, there's no "Bearer" in the payload. I also tested this against my project and it works as expected.

Screenshots (if appropriate):

@kohanian kohanian changed the title Adds support for request authorizers to have a querystring identity source Add support for request authorizers to have a querystring identity source Nov 11, 2022
@dnalborczyk
Copy link
Collaborator

thank you @kohanian !! this looks great. give me a couple days to look over your PR.

@dnalborczyk dnalborczyk changed the title Add support for request authorizers to have a querystring identity source feat: Add support for request authorizers to have a querystring identity source Nov 17, 2022
@dnalborczyk dnalborczyk merged commit 8d83e44 into dherault:master Nov 17, 2022
@kohanian
Copy link
Contributor Author

thank you @dnalborczyk !

@bwp91
Copy link

bwp91 commented Nov 20, 2022

Hey first of all let me say that I really appreciate the work that goes into this plugin and the PRs too.

Since this PR has been implemented (or at least since v11.4.0) I have been receiving a 500 response code instead of the 401 that I would expect in the following scenario.

I have a project which uses cookie auth and developing with serverless-offline has been successful. For my protected endpoints for the purposes of this comment I will split into two circumstances for invalid auth:

(1) cookie in header does not exist at all
(2) cookie exists in header but token is incorrect

Prior to v11.4.0 I would receive this response in both scenarios:

{
  error: 'Unauthorized',
  message: 'Unauthorized',
  statusCode: 401,
}

Since v11.4.0 I (correctly) receive this response in situation (2). However in situation (1) when no header is provided at all I now receive (with no log entries)

{
  "error": "Internal Server Error",
  "message": "An internal server error occurred",
  "statusCode": 500
}

My question is, is this change in HTTP response code expected?

@kohanian
Copy link
Contributor Author

kohanian commented Nov 21, 2022

Hey @bwp91 , thanks for your message. The response status code should not have changed because of this addition. I'll try to find some time in my day to debug this. The situations you've laid out sound correct. In my experience, AWS returns 401 with no value exists at all in the headers. Once I find the issue, i'll make another pull request and add integration tests for the error codes. @dnalborczyk , should @bwp91 create an issue for tracking purposes?

@bwp91
Copy link

bwp91 commented Nov 26, 2022

Hi @kohanian

In fact this seems to now (at least for my use case) be fixed by this PR #1610 and version 11.6.0

Thanks everyone!

@kohanian
Copy link
Contributor Author

kohanian commented Nov 28, 2022

@bwp91 credit to @rion18 for putting in the fix to this. 👍 #1618

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants