fix(bug): Check if Identity.ReauthorizeEvery equals zero #2105

jfuss · 2021-07-26T20:47:10Z

Issue #, if available:
#1508

Description of changes:
Was originally introduced #1577 but reverted #2038 due to service impact. Specifically, the originally implementation did not consider intrinsics as the value (a dict) and therefore threw a TypeError and caused SAM to unexpectedly crash.

This PR reverts the revert (re-introducing the originally changes) but updated the implementation to support handling intrinsics. Also adding more unit tests to cover a wider range of inputs and new functional tests to test a "live" template with intrinsics in it.

Description of how you validated changes:
Ran both the functional tests and unit tests on develop and this branch. Which pass.

Here is the output from the unit tests:

(venv-python38) ➜  serverless-application-model git:(develop) ✗ pytest -vv tests/ -k TestApiGatewayAuthorizer
=========================================================================================================================================================================== test session starts ============================================================================================================================================================================
platform darwin -- Python 3.8.0, pytest-6.1.2, py-1.9.0, pluggy-0.13.1 -- /Users/jfuss/sam-github/serverless-application-model/venv-python38/bin/python3.8
cachedir: .pytest_cache
rootdir: /Users/jfuss/sam-github/serverless-application-model, configfile: pytest.ini
plugins: cov-2.10.1
collected 1937 items / 1926 deselected / 11 selected

tests/model/test_api.py::TestApiGatewayAuthorizer::test_create_authorizer_fails_with_empty_identity PASSED                                                                                                                                                                                                                                                           [  9%]
tests/model/test_api.py::TestApiGatewayAuthorizer::test_create_authorizer_fails_with_missing_identity_values_and_not_cached PASSED                                                                                                                                                                                                                                   [ 18%]
tests/model/test_api.py::TestApiGatewayAuthorizer::test_create_authorizer_fails_with_string_authorization_scopes PASSED                                                                                                                                                                                                                                              [ 27%]
tests/model/test_api.py::TestApiGatewayAuthorizer::test_create_authorizer_with_identity_ReauthorizeEvery_asNone_valid_with_query_strings PASSED                                                                                                                                                                                                                      [ 36%]
tests/model/test_api.py::TestApiGatewayAuthorizer::test_create_authorizer_with_identity_intrinsic_is_invalid_if_no_querystring_stagevariables_context_headers PASSED                                                                                                                                                                                                 [ 45%]
tests/model/test_api.py::TestApiGatewayAuthorizer::test_create_authorizer_with_identity_intrinsic_is_valid_with_context PASSED                                                                                                                                                                                                                                       [ 54%]
tests/model/test_api.py::TestApiGatewayAuthorizer::test_create_authorizer_with_identity_intrinsic_is_valid_with_headers PASSED                                                                                                                                                                                                                                       [ 63%]
tests/model/test_api.py::TestApiGatewayAuthorizer::test_create_authorizer_with_identity_intrinsic_is_valid_with_query_strings PASSED                                                                                                                                                                                                                                 [ 72%]
tests/model/test_api.py::TestApiGatewayAuthorizer::test_create_authorizer_with_identity_intrinsic_is_valid_with_stage_variables PASSED                                                                                                                                                                                                                               [ 81%]
tests/model/test_api.py::TestApiGatewayAuthorizer::test_create_authorizer_with_non_integer_identity PASSED                                                                                                                                                                                                                                                           [ 90%]
tests/model/test_api.py::TestApiGatewayAuthorizer::test_create_oauth2_auth PASSED

Checklist:

Write/update tests
make pr passes
Update documentation
Verify transformed template deploys and application functions as expected

Examples?

Please reach out in the comments, if you want to add an example. Examples will be
added to sam init through https://github.com/awslabs/aws-sam-cli-app-templates/

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

)" (aws#2038)" This reverts commit ed3c283.

…te changes

codecov-commenter · 2021-07-26T21:30:00Z

Codecov Report

Merging #2105 (87a0805) into develop (e7a1496) will increase coverage by 0.56%.
The diff coverage is 98.46%.

@@             Coverage Diff             @@
##           develop    #2105      +/-   ##
===========================================
+ Coverage    93.58%   94.15%   +0.56%     
===========================================
  Files           90       92       +2     
  Lines         6124     6292     +168     
  Branches      1260     1284      +24     
===========================================
+ Hits          5731     5924     +193     
+ Misses         183      169      -14     
+ Partials       210      199      -11

Impacted Files	Coverage Δ
...lator/plugins/application/serverless_app_plugin.py	`82.09% <ø> (-0.11%)`	⬇️
samtranslator/swagger/swagger.py	`93.65% <88.88%> (+0.28%)`	⬆️
samtranslator/__init__.py	`100.00% <100.00%> (ø)`
samtranslator/feature_toggle/dialup.py	`100.00% <100.00%> (ø)`
samtranslator/feature_toggle/feature_toggle.py	`100.00% <100.00%> (+12.16%)`	⬆️
samtranslator/metrics/metrics.py	`100.00% <100.00%> (ø)`
samtranslator/model/api/api_generator.py	`94.39% <100.00%> (+0.03%)`	⬆️
samtranslator/model/apigateway.py	`97.73% <100.00%> (+0.58%)`	⬆️
samtranslator/model/eventsources/pull.py	`84.73% <100.00%> (+6.04%)`	⬆️
samtranslator/model/eventsources/push.py	`92.10% <100.00%> (+1.17%)`	⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f313094...87a0805. Read the comment docs.

samtranslator/model/apigateway.py

mndeveci · 2021-07-27T11:42:08Z

samtranslator/model/apigateway.py

+            return False
+
+        # If we can resolve ttl, attempt to see if things are valid
+        if (ttl is None or int(ttl) > 0) and required_properties_missing:


We already checked that ttl has an int value, checking None is unnecessary here. I think if we go with @mgrandis's suggestion, it will simplify some of the things here.

See above comment about the above suggestion.

mndeveci · 2021-07-27T11:43:51Z

tests/model/test_api.py

+    def test_create_authorizer_fails_with_missing_identity_values_and_not_cached(self):
+        with pytest.raises(InvalidResourceException):
+            ApiGatewayAuthorizer(
+                api_logical_id="logicalId",
+                name="authName",
+                identity={"ReauthorizeEvery": 10},
+                function_payload_type="REQUEST",
+            )


This test is same as the one on line 19, right?

Correct. This was removed in the original PR, so looks like I messed something up when adding them back. I remove this.

mndeveci · 2021-07-27T11:44:31Z

tests/model/test_api.py

+    def test_create_authorizer_fails_with_empty_identity(self):
+        with pytest.raises(InvalidResourceException):
+            ApiGatewayAuthorizer(
+                api_logical_id="logicalId", name="authName", identity={}, function_payload_type="REQUEST"
+            )


And this is same as the one on line 28?

Correct. This was removed in the original PR, so looks like I messed something up when adding them back. I remove this.

mgrandis · 2021-07-27T16:13:40Z

tests/model/test_api.py

+        auth = ApiGatewayAuthorizer(
+            api_logical_id="logicalId",
+            name="authName",
+            identity={"ReauthorizeEvery": {"FN:If": ["isProd", 10, 0]}, "Headers": ["Accept"]},


nit: the intrinsic key is actually Fn::If

* Revert "Revert "Issue 1508 remove check requiring identity ... (aws#1577)" (aws#2038)" This reverts commit ed3c283. * Update implementation to support intrinsics/ add more tests to validate changes * Fixing hashes for py2 * Run make black * Handle pr feedback * Add another unit tests to cover the original issue Co-authored-by: Jacob Fuss <[email protected]>

* chore: don't install integration tests (#1964) * Remove unnecessary use of comprehension (#1805) * fix: Grammatical error in README.md (#1965) * fix: Added SAR Support Check (#1972) * Added SAR Support Check * Added docstring and Removed Instance Initialization for Class Method * update pyyaml version to get the security update (#1974) * fix the request parameter parsing, the value can contain dots (#1975) * fix the request parameter parsing, the value can contain dots * fix the unit test for python 2.7 * use built in split, instead of concatenating the string * Documentation: fix incorrect header (#1979) Fixed the incorrectly formatted header for HTTP API section * fix: mutable default values in method definitions (#1997) * fix: remove explicit logging level set in single module (#1998) * fix: Fail when Intrinsics are in SourceVPC list instead of IntrinsicsSourceVPC (#1999) * feat: Adding support for metric publishing. (#2062) * Adding support for metric publishing, manually tested and tests pending. * Adding make action for running test with html coverage. * Adding unit tests for metrics. * Writing integration tests for metrics publishing. * Black reformatting * Fixing unit test. * Addressing PR comments. * Clearing __init__py from test module. * Updating documentation for publish method. Co-authored-by: Tarun Mall <[email protected]> * Update AppConfig boto3 client config to shorten timeout (#2070) * Update AppConfig boto3 client config to shorten timeout * Update timeout config of AppConfig boto3 client * feat: Add ValidateBody/ValidateParameters attribute to API models (#2026) * Added validators to the swagger definitions when a model is required * Remove approach of trying an extra field for validators * WIP - tests deps not working * Finished and fixed all tests to make pr command was complaining. Fixed case where a validator could have a / only and changed it to translate as 'root' * py2.7 complaint * Fix py2.7 order of validators on output folder * WIP - adding validator fields * Set another strategy to set the validators without affect existent clients with model required * Set another strategy to set the validators without affect existent clients with model required * Remove mistaken file pushed * Remove mistaken file pushed * Partial coments fixes * Implemented case where we can set a only limited combinations on the specs * Fix case test when multiple path are using same validator * Added openapi3 specific tests * Added openapi3 specific tests * Fix requested changes from review: Renamed from ValidateRequest -> ValidateParameters also renamed everything from validator_request. Moved validators names according with the comum use. * new approach to not cause KeyErrors * Setting validator to the only given method not all methods on the path * removed extra space on comment - lint * Normalized method name path that comes from the template Co-authored-by: Rondineli Gomes de Araujo <[email protected]> Co-authored-by: Rondineli <[email protected]> Co-authored-by: Rondineli <[email protected]> * fix: fail with invalid resource, when RetentionPolicy has unresolved intrinsic value (#2074) * fix: fail with invalid resource, when RetentionPolicy has unresolved intrinsic value * make black * remove extra formatting * add test case for !ref * test: Migration of combination integration tests (#1970) * fix: Compare integration tests results using hash of file content instead of image compare (#2086) * change yaml.load to yaml.safe_load for the security best practice * use yaml_parse for consistant style * remove pillow library for image comparing, use hash instead * make it compatible with py2 * Remove logging of SAR service call response (#2064) * Test CloudWatchEvent Properties With Intrinsic Functions (#2090) * Add headers whenever cors is set * Add cw event success cases * Replace GetAtt with condition * Cleanup test yaml * Cleanup test yaml * Remove files * Add intrinsic to pattern field * Fix wrong files committed * test: Added intrinsic tests for Function SNS Event (#2101) * fix: Validate Trigger field and test Cognito properties with intrinsic functions (#2092) * Add headers whenever cors is set * Fix Cognito trigger validation * Make templates deployable * Removed trigger value validation * Fix Python2 string matching test issue * Use PropertyType mechanism to exclude intrinsics * Remove unused import * feat(Intrinsic Tests): Added Tests for Kinesis Event Source with Intrinsics (#2103) * Added Tests for Kinesis EventSource with Intrinsics * Fixed Formatting with Black * fix: Don't attempt to refetch a swagger method object by its name as we already have it (#2031) * Fix a bad logic where a hash key is modified then refetched which results in KeyError * add unit tests and update other instance of same usage on line 534 * fix: Fix the same code in open_api.py Co-authored-by: Mehmet Nuri Deveci <[email protected]> Co-authored-by: Mathieu Grandis <[email protected]> * test: Adding intrinsic tests for Function S3 Event (#2100) * fix(bug): Check if Identity.ReauthorizeEvery equals zero (#2105) * Revert "Revert "Issue 1508 remove check requiring identity ... (#1577)" (#2038)" This reverts commit ed3c283. * Update implementation to support intrinsics/ add more tests to validate changes * Fixing hashes for py2 * Run make black * Handle pr feedback * Add another unit tests to cover the original issue Co-authored-by: Jacob Fuss <[email protected]> * chore: Update PR Checklist to include writing intrinsic tests (#2106) * Update PR Checklist to include writing intrinsic tests * Handle pr feedback Co-authored-by: Jacob Fuss <[email protected]> * Delete .travis.yml (#2102) * fix: Correct grammar in the waiting for changeset message (#2027) * chore: bump version to 1.39.0 (#2128) Co-authored-by: Chih-Hsuan Yen <[email protected]> Co-authored-by: Harsh Mishra <[email protected]> Co-authored-by: Pranav <[email protected]> Co-authored-by: Cosh_ <[email protected]> Co-authored-by: Mohamed Elasmar <[email protected]> Co-authored-by: Ben <[email protected]> Co-authored-by: Mehmet Nuri Deveci <[email protected]> Co-authored-by: Jacob Fuss <[email protected]> Co-authored-by: Tarun <[email protected]> Co-authored-by: Tarun Mall <[email protected]> Co-authored-by: Wing Fung Lau <[email protected]> Co-authored-by: Rondineli <[email protected]> Co-authored-by: Rondineli Gomes de Araujo <[email protected]> Co-authored-by: Rondineli <[email protected]> Co-authored-by: Rondineli <[email protected]> Co-authored-by: mingkun2020 <[email protected]> Co-authored-by: Daniel Mil <[email protected]> Co-authored-by: Ahmed Elbayaa <[email protected]> Co-authored-by: Jacob Fuss <[email protected]> Co-authored-by: Ichinose Shogo <[email protected]> Co-authored-by: Ryan Parman <[email protected]>

* Release/v1.39.0 (#2141) * chore: don't install integration tests (#1964) * Remove unnecessary use of comprehension (#1805) * fix: Grammatical error in README.md (#1965) * fix: Added SAR Support Check (#1972) * Added SAR Support Check * Added docstring and Removed Instance Initialization for Class Method * update pyyaml version to get the security update (#1974) * fix the request parameter parsing, the value can contain dots (#1975) * fix the request parameter parsing, the value can contain dots * fix the unit test for python 2.7 * use built in split, instead of concatenating the string * Documentation: fix incorrect header (#1979) Fixed the incorrectly formatted header for HTTP API section * fix: mutable default values in method definitions (#1997) * fix: remove explicit logging level set in single module (#1998) * fix: Fail when Intrinsics are in SourceVPC list instead of IntrinsicsSourceVPC (#1999) * feat: Adding support for metric publishing. (#2062) * Adding support for metric publishing, manually tested and tests pending. * Adding make action for running test with html coverage. * Adding unit tests for metrics. * Writing integration tests for metrics publishing. * Black reformatting * Fixing unit test. * Addressing PR comments. * Clearing __init__py from test module. * Updating documentation for publish method. Co-authored-by: Tarun Mall <[email protected]> * Update AppConfig boto3 client config to shorten timeout (#2070) * Update AppConfig boto3 client config to shorten timeout * Update timeout config of AppConfig boto3 client * feat: Add ValidateBody/ValidateParameters attribute to API models (#2026) * Added validators to the swagger definitions when a model is required * Remove approach of trying an extra field for validators * WIP - tests deps not working * Finished and fixed all tests to make pr command was complaining. Fixed case where a validator could have a / only and changed it to translate as 'root' * py2.7 complaint * Fix py2.7 order of validators on output folder * WIP - adding validator fields * Set another strategy to set the validators without affect existent clients with model required * Set another strategy to set the validators without affect existent clients with model required * Remove mistaken file pushed * Remove mistaken file pushed * Partial coments fixes * Implemented case where we can set a only limited combinations on the specs * Fix case test when multiple path are using same validator * Added openapi3 specific tests * Added openapi3 specific tests * Fix requested changes from review: Renamed from ValidateRequest -> ValidateParameters also renamed everything from validator_request. Moved validators names according with the comum use. * new approach to not cause KeyErrors * Setting validator to the only given method not all methods on the path * removed extra space on comment - lint * Normalized method name path that comes from the template Co-authored-by: Rondineli Gomes de Araujo <[email protected]> Co-authored-by: Rondineli <[email protected]> Co-authored-by: Rondineli <[email protected]> * fix: fail with invalid resource, when RetentionPolicy has unresolved intrinsic value (#2074) * fix: fail with invalid resource, when RetentionPolicy has unresolved intrinsic value * make black * remove extra formatting * add test case for !ref * test: Migration of combination integration tests (#1970) * fix: Compare integration tests results using hash of file content instead of image compare (#2086) * change yaml.load to yaml.safe_load for the security best practice * use yaml_parse for consistant style * remove pillow library for image comparing, use hash instead * make it compatible with py2 * Remove logging of SAR service call response (#2064) * Test CloudWatchEvent Properties With Intrinsic Functions (#2090) * Add headers whenever cors is set * Add cw event success cases * Replace GetAtt with condition * Cleanup test yaml * Cleanup test yaml * Remove files * Add intrinsic to pattern field * Fix wrong files committed * test: Added intrinsic tests for Function SNS Event (#2101) * fix: Validate Trigger field and test Cognito properties with intrinsic functions (#2092) * Add headers whenever cors is set * Fix Cognito trigger validation * Make templates deployable * Removed trigger value validation * Fix Python2 string matching test issue * Use PropertyType mechanism to exclude intrinsics * Remove unused import * feat(Intrinsic Tests): Added Tests for Kinesis Event Source with Intrinsics (#2103) * Added Tests for Kinesis EventSource with Intrinsics * Fixed Formatting with Black * fix: Don't attempt to refetch a swagger method object by its name as we already have it (#2031) * Fix a bad logic where a hash key is modified then refetched which results in KeyError * add unit tests and update other instance of same usage on line 534 * fix: Fix the same code in open_api.py Co-authored-by: Mehmet Nuri Deveci <[email protected]> Co-authored-by: Mathieu Grandis <[email protected]> * test: Adding intrinsic tests for Function S3 Event (#2100) * fix(bug): Check if Identity.ReauthorizeEvery equals zero (#2105) * Revert "Revert "Issue 1508 remove check requiring identity ... (#1577)" (#2038)" This reverts commit ed3c283. * Update implementation to support intrinsics/ add more tests to validate changes * Fixing hashes for py2 * Run make black * Handle pr feedback * Add another unit tests to cover the original issue Co-authored-by: Jacob Fuss <[email protected]> * chore: Update PR Checklist to include writing intrinsic tests (#2106) * Update PR Checklist to include writing intrinsic tests * Handle pr feedback Co-authored-by: Jacob Fuss <[email protected]> * Delete .travis.yml (#2102) * fix: Correct grammar in the waiting for changeset message (#2027) * chore: bump version to 1.39.0 (#2128) Co-authored-by: Chih-Hsuan Yen <[email protected]> Co-authored-by: Harsh Mishra <[email protected]> Co-authored-by: Pranav <[email protected]> Co-authored-by: Cosh_ <[email protected]> Co-authored-by: Mohamed Elasmar <[email protected]> Co-authored-by: Ben <[email protected]> Co-authored-by: Mehmet Nuri Deveci <[email protected]> Co-authored-by: Jacob Fuss <[email protected]> Co-authored-by: Tarun <[email protected]> Co-authored-by: Tarun Mall <[email protected]> Co-authored-by: Wing Fung Lau <[email protected]> Co-authored-by: Rondineli <[email protected]> Co-authored-by: Rondineli Gomes de Araujo <[email protected]> Co-authored-by: Rondineli <[email protected]> Co-authored-by: Rondineli <[email protected]> Co-authored-by: mingkun2020 <[email protected]> Co-authored-by: Daniel Mil <[email protected]> Co-authored-by: Ahmed Elbayaa <[email protected]> Co-authored-by: Jacob Fuss <[email protected]> Co-authored-by: Ichinose Shogo <[email protected]> Co-authored-by: Ryan Parman <[email protected]> * feat: ARM support (#2163) * feat: ARM architecture support for Function and Layer * Add docs/tests for Globals Architectures (#85) Co-authored-by: Renato Valenzuela <[email protected]> Co-authored-by: Chih-Hsuan Yen <[email protected]> Co-authored-by: Harsh Mishra <[email protected]> Co-authored-by: Pranav <[email protected]> Co-authored-by: Cosh_ <[email protected]> Co-authored-by: Mohamed Elasmar <[email protected]> Co-authored-by: Ben <[email protected]> Co-authored-by: Mehmet Nuri Deveci <[email protected]> Co-authored-by: Jacob Fuss <[email protected]> Co-authored-by: Tarun <[email protected]> Co-authored-by: Tarun Mall <[email protected]> Co-authored-by: Wing Fung Lau <[email protected]> Co-authored-by: Rondineli <[email protected]> Co-authored-by: Rondineli Gomes de Araujo <[email protected]> Co-authored-by: Rondineli <[email protected]> Co-authored-by: Rondineli <[email protected]> Co-authored-by: mingkun2020 <[email protected]> Co-authored-by: Daniel Mil <[email protected]> Co-authored-by: Ahmed Elbayaa <[email protected]> Co-authored-by: Jacob Fuss <[email protected]> Co-authored-by: Ichinose Shogo <[email protected]> Co-authored-by: Ryan Parman <[email protected]> Co-authored-by: Renato Valenzuela <[email protected]>

jfuss added 4 commits July 22, 2021 09:42

Revert "Revert "Issue 1508 remove check requiring identity ... (aws#1577

5d2896b

)" (aws#2038)" This reverts commit ed3c283.

Update implementation to support intrinsics/ add more tests to valida…

201e4a5

…te changes

Fixing hashes for py2

3aa2014

Run make black

87a0805

mgrandis added the pr/internal label Jul 26, 2021

mgrandis suggested changes Jul 26, 2021

View reviewed changes

samtranslator/model/apigateway.py Show resolved Hide resolved

mndeveci suggested changes Jul 27, 2021

View reviewed changes

jfuss added 2 commits July 27, 2021 10:08

Handle pr feedback

5f29f93

Add another unit tests to cover the original issue

d04a588

mndeveci approved these changes Jul 27, 2021

View reviewed changes

mgrandis reviewed Jul 27, 2021

View reviewed changes

mgrandis approved these changes Jul 27, 2021

View reviewed changes

jfuss merged commit cf7b935 into aws:develop Jul 27, 2021

tunesmith mentioned this pull request Oct 25, 2022

Api Authorizer must specify at least one identity even authorization caching is disabled #1508

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(bug): Check if Identity.ReauthorizeEvery equals zero #2105

fix(bug): Check if Identity.ReauthorizeEvery equals zero #2105

Uh oh!

jfuss commented Jul 26, 2021

Uh oh!

codecov-commenter commented Jul 26, 2021 •

edited

Loading

Uh oh!

Uh oh!

mndeveci Jul 27, 2021

Uh oh!

jfuss Jul 27, 2021

Uh oh!

mndeveci Jul 27, 2021

Uh oh!

jfuss Jul 27, 2021

Uh oh!

mndeveci Jul 27, 2021

Uh oh!

jfuss Jul 27, 2021

Uh oh!

mgrandis Jul 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fix(bug): Check if Identity.ReauthorizeEvery equals zero #2105

fix(bug): Check if Identity.ReauthorizeEvery equals zero #2105

Uh oh!

Conversation

jfuss commented Jul 26, 2021

Uh oh!

codecov-commenter commented Jul 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

mndeveci Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

jfuss Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

mndeveci Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

jfuss Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

mndeveci Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

jfuss Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

mgrandis Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Jul 26, 2021 •

edited

Loading