feat(AWS HTTP API): Add support for custom Lambda authorizers #9192
Conversation
pgrzesik
force-pushed
the
add-support-for-custom-http-api-authorizers
branch
from
f2f567f
to
10b42b1
Compare
Codecov Report
@@ Coverage Diff @@
## master #9192 +/- ##
==========================================
+ Coverage 87.21% 87.24% +0.02%
==========================================
Files 310 310
Lines 11454 11485 +31
==========================================
+ Hits 9990 10020 +30
- Misses 1464 1465 +1
Continue to review full report at Codecov.
|
| @@ -88,6 +89,7 @@ class HttpApiEvents { | |||
| }, | |||
| name: { type: 'string' }, | |||
| scopes: { type: 'array', items: { type: 'string' } }, | |||
| type: { type: 'string', regexp: new RegExp('^(REQUEST|JWT)$', 'i').toString() }, | |||
There was a problem hiding this comment.
I would stick to single case support, and define it here with enum.
It think there's no clear benefit in supporting case insensitivity here, while it's definitely confusing and implementation wise error-prone.
There was a problem hiding this comment.
I was thinking about it for a moment and I've went with this approach to be consistent with authorizers for http event, where we support case insensitvity and even examples in documentation use both notations. Do you think it would be better to introduce that change which will make httpApi implementation a bit more consistent while sacrificing consistency between http and httpApi ?
There was a problem hiding this comment.
I think that case insensitivity in case of http was not great decision. It also predated config based validation.
I would not look back here, same as when deciding on other parts of the config we didn't look back (e.g. in HTTP API it's provider.httpApi.name while with Rest API it's provider.apiName)
Also if user will migrate from http and start to use upper case with httpApi it'll get very meaningful error message, so I don't think it should be a problem.
pgrzesik
force-pushed
the
add-support-for-custom-http-api-authorizers
branch
from
10b42b1
to
0dd28b9
Compare
| })), | ||
| })), | ||
| functions: Object.values(serviceConfig.functions).map((functionConfig) => { | ||
| const functionEvents = functionConfig.events || []; |
There was a problem hiding this comment.
Why this change is necessary? Normally functionConfig.events is guaranteed internally
There was a problem hiding this comment.
It was due to a quirky test setup of
In this case, the existence of
events as an array is not ensured on functions. I've decided to patch it in a way that will ensure that generatePayload will default to an empty list in such edge cases.
There was a problem hiding this comment.
hmm.. but it was more an issue in test than in tested module.
Technically serverless instance guarantees that that every function has events array, and in such case we should not introduce a noise with such checks (same as we assume that all values match configuration schema)
There was a problem hiding this comment.
However, as I think of it. In future we may loose that guarantee. Especially when we will pass a configuration input directly (by schema, events is not a required property).
So after all I think it's fine to add such check here
pgrzesik
force-pushed
the
add-support-for-custom-http-api-authorizers
branch
from
0dd28b9
to
3753393
Compare
pgrzesik
force-pushed
the
add-support-for-custom-http-api-authorizers
branch
from
3753393
to
45755fd
Compare
Adds support for custom Lambda authorizers for HTTP API. There is a slight change to original proposal, instead of
nameandarnI usedfunctionNameandfunctionArnbecausenamewas already reserved for overriding authorizer name.Additionally it includes a small fix to
generatePayloadto a bug that was exposed after adding authorizer functions tohttpApifixture.Addresses: #8210