Skip to content

fix APIGW UpdateMethod authorizer validation#7860

Merged
alexrashed merged 2 commits intomasterfrom
fix-update-method-authorizer
Mar 14, 2023
Merged

fix APIGW UpdateMethod authorizer validation#7860
alexrashed merged 2 commits intomasterfrom
fix-update-method-authorizer

Conversation

@bentsku
Copy link
Contributor

@bentsku bentsku commented Mar 13, 2023

Follow up from #7749, it seems the validation was lacking and a bit too restrictive while updating Authorizers-related fields. I've added more AWS tests, this should solve our Pro pipeline (tested locally and it works).

@bentsku bentsku requested a review from calvernaz as a code owner March 13, 2023 22:43
@bentsku bentsku temporarily deployed to localstack-ext-tests March 13, 2023 22:43 — with GitHub Actions Inactive
Copy link
Contributor

@calvernaz calvernaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍

@@ -492,11 +494,10 @@ def update_method(
continue

if path == "/authorizationType" and value in ("CUSTOM", "COGNITO_USER_POOLS"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so it either update the type or the id that points to, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you can:

  • Update the type, but if the type is either CUSTOM or COGNITO_USER_POOLS, you also need to provide a valid AuthorizerId in the call (so you'd need to provide both).
  • Update only the ID, it will be taken into account only if the existing authorisation type is CUSTOM or COGNITO_USER_POOLS

I can see there's one scenario missing here, if the type is already CUSTOM and we change it to COGNITO_USER_POOLS, technically I think we'd still need to update the AuthorizerId, but this hasn't been tested against AWS.

The validation logic is these Update* operation is pretty convoluted, and it takes a lot of testing to get all the possible combinations.

@github-actions
Copy link

github-actions bot commented Mar 14, 2023

LocalStack integration with Pro

       3 files  +    1         3 suites  +1   1h 42m 7s ⏱️ + 5m 26s
1 798 tests ±    0  1 415 ✔️ +    2  383 💤  -     2  0 ±0 
2 524 runs  +371  1 789 ✔️ +201  735 💤 +170  0 ±0 

Results for commit e107d94. ± Comparison against base commit 9165763.

♻️ This comment has been updated with latest results.

@bentsku bentsku temporarily deployed to localstack-ext-tests March 14, 2023 00:38 — with GitHub Actions Inactive
@alexrashed alexrashed merged commit 714b9e4 into master Mar 14, 2023
@alexrashed alexrashed deleted the fix-update-method-authorizer branch March 14, 2023 08:34
@coveralls
Copy link

Coverage Status

Coverage: 85.162% (+0.04%) from 85.12% when pulling e107d94 on fix-update-method-authorizer into 9165763 on master.

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.

4 participants