feat: Collect Lambda Authorizers found in Cloudformation resources - #4668
Merged
Conversation
…nto collect_swagger
…nto collect_swagger
github-actions
Bot
added
pr/internal
area/local/invoke
lucashuy
commented
Feb 8, 2023
Comment on lines
+171
to
+175
| if identity_source_template is not None and not isinstance(identity_source_template, str): | ||
| raise InvalidSamTemplateException( | ||
| f"Lambda Authorizer '{logical_id}' contains an invalid '{CfnApiProvider._AUTHORIZER_IDENTITY_SOURCE}', " | ||
| "it must be a comma-separated string." | ||
| ) |
Contributor
Author
There was a problem hiding this comment.
For authorizers of type REQUEST, identity sources are optional in API Gateway V1 depending on if caching is enabled or not (if caching is enabled, must provide identity sources). I've made the choice to not validate this and only validate the type if it was provided.
Contributor
There was a problem hiding this comment.
It is great to read these feedbacks, but I wonder if we should write some of them in the code so that we can understand the decision later? 🤔
lucashuy
commented
Feb 8, 2023
Comment on lines
+103
to
+105
| payload_version = authorizer_object.get( | ||
| SwaggerParser._AUTHORIZER_PAYLOAD_VERSION, LambdaAuthorizer.PAYLOAD_V1 | ||
| ) |
Contributor
Author
There was a problem hiding this comment.
Moving to constant (not really related to the actual contents of this PR)
Comment on lines
+28
to
+30
| AWS_APIGATEWAY_V2_BASE_PATH_MAPPING, | ||
| AWS_APIGATEWAY_V2_DOMAIN_NAME, | ||
| AWS_APIGATWAY_DOMAIN_NAME, | ||
| AWS_APIGATEWAY_DOMAIN_NAME, |
Contributor
Author
There was a problem hiding this comment.
Fixing spelling/formatting mistake (not related to the contents of this PR)
sriram-mv
reviewed
Feb 9, 2023
| if resource_type == AWS_APIGATEWAY_METHOD: | ||
| self._extract_cloud_formation_method(stack.stack_path, resources, logical_id, resource, collector) | ||
|
|
||
| if resource_type == AWS_APIGATEWAY_AUTHORIZER: |
Contributor
There was a problem hiding this comment.
Nothing specific to this PR, but I would want to us to move to a dispatch pattern whenever there are multiple if branches.
Something like:
for resource_extractor in resource_extractors:
resource_extract.extract(*args, **kwargs) # pass in the args deemed necessary.might be easier to read and the behavior is apparent.
| # split and parse out identity sources | ||
| identity_source_list = [] | ||
|
|
||
| if identity_source_template: |
Contributor
There was a problem hiding this comment.
should there be validation on it being a comma separated list?
Contributor
Author
There was a problem hiding this comment.
Thats a good idea. I think we can further extend this to validating the actual identity source itself, likely using a set of regular expressions.
I can make that change in a different PR since that also applies to the other methods of collecting authorizers, and also address the empty string comparison that is done when linking.
mndeveci
reviewed
Feb 10, 2023
| ApiCollector to save Authorizers into | ||
| """ | ||
| properties = resource.get("Properties", {}) | ||
| authorizer_type = properties.get(CfnApiProvider._AUTHORIZER_TYPE, "").lower() |
Contributor
There was a problem hiding this comment.
Just curious, why we are calling lower here?
Contributor
Author
There was a problem hiding this comment.
When defining the type in Properties, the expected values are TOKEN and REQUEST, but if defined in swagger, then its token or request. To make things consistent, I elected to make things lowercase once it makes its way into SAM CLI.
Contributor
Author
There was a problem hiding this comment.
Oh, a good point here would be to also consider validating this behaviour.
Comment on lines
+121
to
+152
| if not authorizer_type: | ||
| raise InvalidSamTemplateException( | ||
| f"Authorizer '{logical_id}' is missing the '{CfnApiProvider._AUTHORIZER_TYPE}' " | ||
| "property, an Authorizer type must be defined." | ||
| ) | ||
|
|
||
| if not rest_api_id: | ||
| raise InvalidSamTemplateException( | ||
| f"Authorizer '{logical_id}' is missing the '{CfnApiProvider._AUTHORIZER_REST_API}' " | ||
| "property, this must be defined." | ||
| ) | ||
|
|
||
| if not name: | ||
| raise InvalidSamTemplateException( | ||
| f"Authorizer '{logical_id}' is missing the '{CfnApiProvider._AUTHORIZER_NAME}' " | ||
| "property, the Name must be defined." | ||
| ) | ||
|
|
||
| if authorizer_type not in LambdaAuthorizer.VALID_TYPES: | ||
| LOG.warning( | ||
| "Authorizer '%s' with type '%s' is currently not supported. " | ||
| "Only Lambda Authorizers of type TOKEN and REQUEST are supported.", | ||
| logical_id, | ||
| authorizer_type, | ||
| ) | ||
| return | ||
|
|
||
| if not authorizer_uri: | ||
| raise InvalidSamTemplateException( | ||
| f"Authorizer '{logical_id}' is missing the '{CfnApiProvider._AUTHORIZER_AUTHORIZER_URI}' " | ||
| "property, a valid Lambda ARN must be provided." | ||
| ) |
Contributor
There was a problem hiding this comment.
I feel like this part can be extracted as validation method to to improve the readability. Which can also be combined what @sriram-mv suggested above (like different types might have their own implementation for a base interface or an abstract class).
Comment on lines
+171
to
+175
| if identity_source_template is not None and not isinstance(identity_source_template, str): | ||
| raise InvalidSamTemplateException( | ||
| f"Lambda Authorizer '{logical_id}' contains an invalid '{CfnApiProvider._AUTHORIZER_IDENTITY_SOURCE}', " | ||
| "it must be a comma-separated string." | ||
| ) |
Contributor
There was a problem hiding this comment.
It is great to read these feedbacks, but I wonder if we should write some of them in the code so that we can understand the decision later? 🤔
sriram-mv
approved these changes
Feb 14, 2023
mndeveci
approved these changes
Feb 14, 2023
lucashuy
added a commit
that referenced
this pull request
Apr 13, 2023
* chore: Merge from develop into feat/apigw-lambda-auth (#4642) * feat: List Command (#4587) * feat: Add table view and rich table (#3851) * Add table view and rich table * Black reformat * Update reproducible reqs * Comment out table comparison assertions * Make rich table class members protected * feat: Adding base commands and help messages for sam list (#3912) * Added the base commands of sam list and their corresponding help messages * Added no-args handling to base commands and added files for integration test suite * Made additions to resources integration tests * Made additions to the sam list integration test suite * Added to sam list integration and unit tests * Added integration tests to test help messages of sam list commands * Reformatted files * Cleaned up unfinished tests * adding check to see what the appveyor test will produce. Trying to resolve test failure * Fixed test to check help messages * modified pattern matching for eliminating newlines when matching help message * Changed the way whitespaces are handled in matching help messages * Addressed PR comments, moved items into common classes * Made modifications based on comments, removed relative import paths, added method annotations, fixed text formatting * Reformatted files * removed folder deletion * removed uneccessary folder creation and deletion * fixed errors with cwd of integration tests Co-authored-by: Andrew Zhan <zhandr@amazon.com> * Added the stack-outputs command implementation and tests (#3947) Co-authored-by: Andrew Zhan <zhandr@amazon.com> * Refactored stack outputs command to the producer mapper consumer design pattern (#3980) * Added the base commands of sam list and their corresponding help messages * Added no-args handling to base commands and added files for integration test suite * Made additions to resources integration tests * Made additions to the sam list integration test suite * Added to sam list integration and unit tests * Added integration tests to test help messages of sam list commands * Reformatted files * Cleaned up unfinished tests * adding check to see what the appveyor test will produce. Trying to resolve test failure * Fixed test to check help messages * modified pattern matching for eliminating newlines when matching help message * Changed the way whitespaces are handled in matching help messages * Addressed PR comments, moved items into common classes * Made modifications based on comments, removed relative import paths, added method annotations, fixed text formatting * Reformatted files * removed folder deletion * removed uneccessary folder creation and deletion * fixed errors with cwd of integration tests * Added implementation and tests for the stack-outputs command * Added test skips for integration tests, added unit tests, removed redundant init_client call * commit to retrigger appveyor tests * Commmit to trigger appveyor * Modified client source, made fixes based on comments * Made fixes based on comments * Combined get_stack_info and stack_exists, and modified unit tests * Empty-Commit * Empty-Commit * Empty-Commit * fixed tests based on comments * reformatted file * Refactored stack outputs command to the producer mapper consumer design pattern * Fixed formatting * Moved interfaces, made changes based on comments * Made fixes based on comments * Made fixes based on comments * Empty commit * Made changes based on comments, added new exceptions * Fixed format * Fixed return type declaration * Fixed return type declaration * Changed return type to list * Fixed error Co-authored-by: Andrew Zhan <zhandr@amazon.com> * Local transform and resource collection (#4020) Co-authored-by: Andrew Zhan <zhandr@amazon.com> * feat: Adding cloud resources to sam list resources output (#4056) * Added the base commands of sam list and their corresponding help messages * Added no-args handling to base commands and added files for integration test suite * Made additions to resources integration tests * Made additions to the sam list integration test suite * Added to sam list integration and unit tests * Added integration tests to test help messages of sam list commands * Reformatted files * Cleaned up unfinished tests * adding check to see what the appveyor test will produce. Trying to resolve test failure * Fixed test to check help messages * modified pattern matching for eliminating newlines when matching help message * Changed the way whitespaces are handled in matching help messages * Addressed PR comments, moved items into common classes * Made modifications based on comments, removed relative import paths, added method annotations, fixed text formatting * Reformatted files * removed folder deletion * removed uneccessary folder creation and deletion * fixed errors with cwd of integration tests * Added implementation and tests for the stack-outputs command * Added test skips for integration tests, added unit tests, removed redundant init_client call * commit to retrigger appveyor tests * Commmit to trigger appveyor * Modified client source, made fixes based on comments * Made fixes based on comments * Combined get_stack_info and stack_exists, and modified unit tests * Empty-Commit * Empty-Commit * Empty-Commit * fixed tests based on comments * reformatted file * Refactored stack outputs command to the producer mapper consumer design pattern * Fixed formatting * Moved interfaces, made changes based on comments * Made fixes based on comments * Made fixes based on comments * Empty commit * Made changes based on comments, added new exceptions * Fixed format * Fixed return type declaration * Fixed return type declaration * Changed return type to list * Fixed error * Implementation of the local transform and resource collection * Empty-Commit * Added section to avoid unused variable * Refactored common code * Added tests, modified PR * Fixed formatting * Made fixes based on PR comments * Fixed formatting * Fixed typing errors * Reverted typing * Fixed error with typing * Made changes to handling optional params * Fixes to typing errors * Made edits based on comments * Fixed error * Changed return type * Reverted return type due to make pr error * Added change to fix make pr error * Removed translate_utils.py file * Added cloud resources to sam list resources output * Empty commit * modified test format * Modified tests * Modified test Co-authored-by: Andrew Zhan <zhandr@amazon.com> * feat: Adding the sam list testable resources command, tests, and table output format support for all sam list commands (#4081) * Added the base commands of sam list and their corresponding help messages * Added no-args handling to base commands and added files for integration test suite * Made additions to resources integration tests * Made additions to the sam list integration test suite * Added to sam list integration and unit tests * Added integration tests to test help messages of sam list commands * Reformatted files * Cleaned up unfinished tests * adding check to see what the appveyor test will produce. Trying to resolve test failure * Fixed test to check help messages * modified pattern matching for eliminating newlines when matching help message * Changed the way whitespaces are handled in matching help messages * Addressed PR comments, moved items into common classes * Made modifications based on comments, removed relative import paths, added method annotations, fixed text formatting * Reformatted files * removed folder deletion * removed uneccessary folder creation and deletion * fixed errors with cwd of integration tests * Added implementation and tests for the stack-outputs command * Added test skips for integration tests, added unit tests, removed redundant init_client call * commit to retrigger appveyor tests * Commmit to trigger appveyor * Modified client source, made fixes based on comments * Made fixes based on comments * Combined get_stack_info and stack_exists, and modified unit tests * Empty-Commit * Empty-Commit * Empty-Commit * fixed tests based on comments * reformatted file * Refactored stack outputs command to the producer mapper consumer design pattern * Fixed formatting * Moved interfaces, made changes based on comments * Made fixes based on comments * Made fixes based on comments * Empty commit * Made changes based on comments, added new exceptions * Fixed format * Fixed return type declaration * Fixed return type declaration * Changed return type to list * Fixed error * Implementation of the local transform and resource collection * Empty-Commit * Added section to avoid unused variable * Refactored common code * Added tests, modified PR * Fixed formatting * Made fixes based on PR comments * Fixed formatting * Fixed typing errors * Reverted typing * Fixed error with typing * Made changes to handling optional params * Fixes to typing errors * Made edits based on comments * Fixed error * Changed return type * Reverted return type due to make pr error * Added change to fix make pr error * Removed translate_utils.py file * Added cloud resources to sam list resources output * Empty commit * modified test format * Modified tests * Modified test * Adding the sam list testable resources command, tests, and table output format support for all sam list commands * Changed table and made changes based on pr comments * Fixed integration test expected outputs * Fixed table heading * Added docstring and re-arranged the testable resources producer to reduce if-elses within a single function * Fixed format Co-authored-by: Andrew Zhan <zhandr@amazon.com> * Renaming the 'testable-resources' command to 'endpoints' (#4116) * Added the base commands of sam list and their corresponding help messages * Added no-args handling to base commands and added files for integration test suite * Made additions to resources integration tests * Made additions to the sam list integration test suite * Added to sam list integration and unit tests * Added integration tests to test help messages of sam list commands * Reformatted files * Cleaned up unfinished tests * adding check to see what the appveyor test will produce. Trying to resolve test failure * Fixed test to check help messages * modified pattern matching for eliminating newlines when matching help message * Changed the way whitespaces are handled in matching help messages * Addressed PR comments, moved items into common classes * Made modifications based on comments, removed relative import paths, added method annotations, fixed text formatting * Reformatted files * removed folder deletion * removed uneccessary folder creation and deletion * fixed errors with cwd of integration tests * Added implementation and tests for the stack-outputs command * Added test skips for integration tests, added unit tests, removed redundant init_client call * commit to retrigger appveyor tests * Commmit to trigger appveyor * Modified client source, made fixes based on comments * Made fixes based on comments * Combined get_stack_info and stack_exists, and modified unit tests * Empty-Commit * Empty-Commit * Empty-Commit * fixed tests based on comments * reformatted file * Refactored stack outputs command to the producer mapper consumer design pattern * Fixed formatting * Moved interfaces, made changes based on comments * Made fixes based on comments * Made fixes based on comments * Empty commit * Made changes based on comments, added new exceptions * Fixed format * Fixed return type declaration * Fixed return type declaration * Changed return type to list * Fixed error * Implementation of the local transform and resource collection * Empty-Commit * Added section to avoid unused variable * Refactored common code * Added tests, modified PR * Fixed formatting * Made fixes based on PR comments * Fixed formatting * Fixed typing errors * Reverted typing * Fixed error with typing * Made changes to handling optional params * Fixes to typing errors * Made edits based on comments * Fixed error * Changed return type * Reverted return type due to make pr error * Added change to fix make pr error * Removed translate_utils.py file * Added cloud resources to sam list resources output * Empty commit * modified test format * Modified tests * Modified test * Adding the sam list testable resources command, tests, and table output format support for all sam list commands * Changed table and made changes based on pr comments * Fixed integration test expected outputs * Fixed table heading * Added docstring and re-arranged the testable resources producer to reduce if-elses within a single function * Fixed format * Renamed command from testable resources to endpoints Co-authored-by: Andrew Zhan <zhandr@amazon.com> * Cleanup integration tests * Cleanup tests, address comments * Move boto3 imports, update unit tests * Add comments, use constants for resources * Update unit test mocking type * Add missing parameters * Add additional comments * Fix spelling, minor updates Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com> Co-authored-by: andrewzhan <andrewzhan8@gmail.com> Co-authored-by: Andrew Zhan <zhandr@amazon.com> * feat: Add warning about not providing stack name option (#4624) * fix: Fix failing list tests on Windows (#4623) * fix: Fix failing list tests on Windows * Black reformat * Add event tracking for sam validate --lint metrics (#4612) * Update lint helpand output message * Add event tracking for sam validate --lint metrics * Add unit test for tracking --------- Co-authored-by: Sriram Madapusi Vasudevan <3770774+sriram-mv@users.noreply.github.com> Co-authored-by: Qingchuan Ma <69653965+qingchm@users.noreply.github.com> Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com> * Use safe yaml parse in list producer (#4632) * Revert an integration test change related to permission change revert (#4633) --------- Co-authored-by: Daniel Mil <84205762+mildaniel@users.noreply.github.com> Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com> Co-authored-by: andrewzhan <andrewzhan8@gmail.com> Co-authored-by: Andrew Zhan <zhandr@amazon.com> Co-authored-by: David <114027923+cdavidxu-hub@users.noreply.github.com> Co-authored-by: Sriram Madapusi Vasudevan <3770774+sriram-mv@users.noreply.github.com> Co-authored-by: Qingchuan Ma <69653965+qingchm@users.noreply.github.com> * feat: Collect Lambda authorizers in swagger definition (#4641) * Initial suppport to gather lambda authorizers in swagger * Added more unit tests * Made it clear that tests are Lambda auth related * Added function docstring * Added missed case where identity sources differ depending on API Gateway version * Changed some values to constants * Addressed comments * Added empty check to other security definition check * Updated log messages to change some info to warnings, and no auth info to debug * feat: Collect Lambda Authorizers found in Cloudformation resources (#4668) * Initial suppport to gather lambda authorizers in swagger * Added more unit tests * Made it clear that tests are Lambda auth related * Added function docstring * Added missed case where identity sources differ depending on API Gateway version * Changed some values to constants * Addressed comments * Added empty check to other security definition check * Added collection of Lambda authorizers for CFN resources * Addressed comments by moving validation logic to its own methods * feat: Collect Lambda Authorizers under the Auth property for Serverless resources (#4654) * Initial suppport to gather lambda authorizers in swagger * Added more unit tests * Made it clear that tests are Lambda auth related * Added function docstring * Added missed case where identity sources differ depending on API Gateway version * Added parsing Authorizers inside of Auth properties for Serverless resources * Added unit tests * Added test for the HTTP API extraction method * Changed some values to constants * Changed some variables to constants * Addressed comments * Added empty check to other security definition check * Correctly name identity sources * Addressed some comments * Changed LOGs to debugs to reduce spam --------- Co-authored-by: Sriram Madapusi Vasudevan <3770774+sriram-mv@users.noreply.github.com> * feat: Added identity source validation and removed empty string state (#4683) * Added identity source validation and removed empty string state * make black reformat * Moved identity source validator into validators folder * Fixed linting errors * Run make black * Compile regular expressions * feat: Added identity source validation in request handling (#4762) * Added identity source validation in request handling * Removed call to create flask app * Added context to id validator * Add route type check for operation_name * Convert to dictionary to avoid typing issue * Addressed comments and moved Route class to it's own module * feat: Event construction refactor (#4798) * Refactored LocalApigwService by moving some event generation logic out * Cleaned operation name generation * Addressed comments * feat: Event constructors for authorizers (#4807) * Refactored LocalApigwService by moving some event generation logic out * Cleaned operation name generation * Addressed comments * Added tests * Fixed typing for identity getter functions * feat: Invoke Lambda authorizer (#4840) * Added initial invocation logic for Lambda authorizer * Added response validation * Removed unused method * Added tests * Updated principalId get logic * Changed typing to be correct * Addressed comments * Format * Moved lambda auth invocation to its own method * Addressed comments * feat: Integration testing of local Lambda authorizers in serverless properties (#4872) * Added initial invocation logic for Lambda authorizer * Added response validation * Removed unused method * Added tests * Updated principalId get logic * Changed typing to be correct * Addressed comments * Format * Moved lambda auth invocation to its own method * Addressed comments * Initial integration testing setup and work * Change test class name and add comment to make it more clear that this is a bad test case * make black * feat: Validate headers against validation expression property (#4910) * Added identity validation expression check * Added additional test cases * fix: Fix APIGW V2 context passing (#4916) * Updated context passing logic to consider V2 payloads * Addressed comments by adding checking for API event * feat: Integration testing of Lambda authorizers defined as CFN resources (#4917) * Added identity validation expression check * Updated context passing logic to consider V2 payloads * Initial CFN authorizer resource testing * Added tests to validate template validation * fix: Added missing checks for Swagger parsing of Lambda authorizers and fixed some existing ones (#4938) * Added missing check for simple responses and fixed validation string check * Added missing check for simple responses * Added checks for payload version * make format * Addressed comments * feat: Integration testing of Lambda authorizers defined under the Swagger document (#4939) * Added identity validation expression check * Updated context passing logic to consider V2 payloads * Added missing check for simple responses and fixed validation string check * Initial swagger parsing integration testing * Added missing check for simple responses * Added checks for payload version * make format * Added template validation for swagger * make format * fix: Only print console message if enableSimpleResponses is defined (#4994) * feat: Add usage disclaimer when starting API with authorizers (#4968) * Added disclaimer for authorizer usage and updated log message for undefined authorizers * Populated message * Updated message * Updated message * feat: Add metrics for using the Lambda authorizer feature (#4942) * Added event tracking for using Lambda authorizers * Added and update unit tests * Moved event tracker to after invocation logic and added session ID passing * Updated event tracker to accept exceptions * Added tests * Updated doc string to include exception message * Addressed comments and removed old test * Added exception to __repr__ and __eq__ * chore: Removed old test file (#5006) --------- Co-authored-by: Daniel Mil <84205762+mildaniel@users.noreply.github.com> Co-authored-by: Mehmet Nuri Deveci <5735811+mndeveci@users.noreply.github.com> Co-authored-by: andrewzhan <andrewzhan8@gmail.com> Co-authored-by: Andrew Zhan <zhandr@amazon.com> Co-authored-by: David <114027923+cdavidxu-hub@users.noreply.github.com> Co-authored-by: Sriram Madapusi Vasudevan <3770774+sriram-mv@users.noreply.github.com> Co-authored-by: Qingchuan Ma <69653965+qingchm@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why is this change necessary?
Adds the ability to parse
AWS::ApiGateway::AuthorizerandAWS::ApiGatewayV2::Authorizerresources to collect Lambda authorizers.Updates the parsing of
AWS::ApiGateway::MethodandAWS::ApiGatewayV2::Routeresources to save their reference to an authorizer.How does it address the issue?
Updates the main
cfn_api_provider.pyby adding two new static methods for the authorizers, and modifies the existing methods forMethodandRoute.What side effects does this change have?
Mandatory Checklist
PRs will only be reviewed after checklist is complete
make prpassesmake update-reproducible-reqsif dependencies were changedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.