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

kohanian commented 1 year ago

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):

dnalborczyk commented 1 year ago

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

kohanian commented 1 year ago

thank you @dnalborczyk !

bwp91 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Hi @kohanian

In fact this seems to now (at least for my use case) be fixed by this PR https://github.com/dherault/serverless-offline/pull/1610 and version 11.6.0

Thanks everyone!

kohanian commented 1 year ago

@bwp91 credit to @rion18 for putting in the fix to this. 👍 https://github.com/dherault/serverless-offline/pull/1618

dherault / serverless-offline