feat(event_handler): allow multiple CORS origins by rubenfonseca · Pull Request #2279 · aws-powertools/powertools-lambda-python

rubenfonseca · 2023-05-16T14:51:39Z

Issue number: #1006

Summary

Changes

Please provide a summary of what's being changed

This PR adds support for additional origins for CORS. It also changes the behavior to never return Origin information outside of a CORS request.

User experience

Please share what the user experience looks like before and after this change

NOTE: this PR changes the default behavior when a non-CORS request is sent. Previously, we would return the configured Origin as part of the response. Now we only do it if there's an Origin in the request and it matches one of the allowed origins. Should we do something about this?

Checklist

If your change doesn't seem to apply, please leave them unchecked.

Meet tenets criteria
I have performed a self-review of this change
Changes have been tested
Changes are documented
PR title follows conventional commit semantics

Is this a breaking change?

RFC issue number:

Checklist:

Migration process documented
Implement warnings (if it can live side by side)

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

docs/core/event_handler/api_gateway.md

codecov-commenter · 2023-05-16T14:58:06Z

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (eb3e795) 97.46% compared to head (3ca9aaf) 97.46%.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #2279   +/-   ##
========================================
  Coverage    97.46%   97.46%           
========================================
  Files          149      149           
  Lines         6895     6901    +6     
  Branches       506      509    +3     
========================================
+ Hits          6720     6726    +6     
  Misses         137      137           
  Partials        38       38

Impacted Files	Coverage Δ
aws_lambda_powertools/event_handler/api_gateway.py	`100.00% <100.00%> (ø)`
...lambda_powertools/utilities/data_classes/common.py	`100.00% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

leandrodamascena

I tested this with several scenarios, resolvers and origins and in all of them it worked perfectly! You are truly amazing, Ruben!

I made some comments and after you answer them I think we are ready to merge!!

leandrodamascena · 2023-05-16T22:37:54Z

docs/core/event_handler/api_gateway.md

+???+ tip "Multiple allowed origins?"
+    If you require multiple allowed origins, pass the additional origins using the `extra_origins` key.
+


Ruben, please add the extra_origins field in the table below.

How about adding a new tab with an example to show how to use this new field? What do you think?

Maybe I'm dizzy in the taxi but this reads odd, perhaps what you meant was:

Multiple origins?

If you need to allow multiple origins ...

As in, Allow-Origins is an explicit CORS terminology, but here we can be more flexible in wording

docs/core/event_handler/api_gateway.md

aws_lambda_powertools/event_handler/api_gateway.py

heitorlessa

Some tiny suggestions but looks great!

aws_lambda_powertools/event_handler/api_gateway.py

heitorlessa · 2023-05-17T09:04:00Z

aws_lambda_powertools/event_handler/api_gateway.py

        self.allow_credentials = allow_credentials

-    def to_dict(self) -> Dict[str, str]:
+    def to_dict(self, origin: Optional[str]) -> Dict[str, str]:


origin: str = ""?

Annotation says it's optional but we are not setting a default value.

You can drop the Optional (None), and simply set to an empty str

Does this mean I would have to compare if origin == "" later on instead of not origin? Doesn't it sound wrong?

TIL not will eventually translate to __bool__, so it works for empty strings too

I've changed my mind again and I believe it's better to keep the Optional. Reason is, the caller will try to fetch an Origin from the headers. The Origin is not always present, so mypy would complain that I can't pass an Optional to the to_dict. So I think in this case the Optional makes sense.

aws_lambda_powertools/event_handler/api_gateway.py

docs/core/event_handler/api_gateway.md

heitorlessa · 2023-05-17T09:11:38Z

docs/core/event_handler/api_gateway.md

+???+ tip "Multiple allowed origins?"
+    If you require multiple allowed origins, pass the additional origins using the `extra_origins` key.
+


Maybe I'm dizzy in the taxi but this reads odd, perhaps what you meant was:

Multiple origins?

If you need to allow multiple origins ...

As in, Allow-Origins is an explicit CORS terminology, but here we can be more flexible in wording

tests/functional/event_handler/test_api_gateway.py

rubenfonseca · 2023-05-17T18:53:37Z

As promised, added E2E tests.

leandrodamascena

I ran the end2end test on this branch and everything is working as expected, Ruben! There are some random bugs at specific runtimes, but they are known bugs and nothing related to this change. - https://github.com/awslabs/aws-lambda-powertools-python/actions/runs/5006899451/jobs/8972788300

One more amazing work, dude!!!

feat(event_source): allow multiple CORS origins

1f59251

boring-cyborg bot added documentation Improvements or additions to documentation event_handlers tests labels May 16, 2023

pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 16, 2023

github-actions bot added the feature New feature or functionality label May 16, 2023

rubenfonseca commented May 16, 2023

View reviewed changes

docs/core/event_handler/api_gateway.md Show resolved Hide resolved

rubenfonseca marked this pull request as ready for review May 16, 2023 14:53

rubenfonseca requested a review from a team as a code owner May 16, 2023 14:53

rubenfonseca requested review from heitorlessa and removed request for a team May 16, 2023 14:53

rubenfonseca linked an issue May 16, 2023 that may be closed by this pull request

Support multiple Allowed Origins in CORS #1006

Closed

leandrodamascena requested changes May 16, 2023

View reviewed changes

heitorlessa reviewed May 17, 2023

View reviewed changes

rubenfonseca and others added 2 commits May 17, 2023 14:32

fix: addressed comments

be743c3

docs: adding example

61a6581

pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 17, 2023

chore: add e2e tests

3ca9aaf

rubenfonseca requested review from heitorlessa and leandrodamascena May 17, 2023 18:53

leandrodamascena approved these changes May 17, 2023

View reviewed changes

Merge branch 'develop' into rf/multiple-origins

0acb944

leandrodamascena merged commit 042e83a into develop May 18, 2023

leandrodamascena deleted the rf/multiple-origins branch May 18, 2023 15:57

leandrodamascena changed the title ~~feat(event_source): allow multiple CORS origins~~ feat(event_handler): allow multiple CORS origins May 31, 2023

		???+ tip "Multiple allowed origins?"
		If you require multiple allowed origins, pass the additional origins using the `extra_origins` key.

Conversation

rubenfonseca commented May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

User experience

Checklist

Acknowledgment

Uh oh!

Uh oh!

codecov-commenter commented May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

leandrodamascena left a comment

Choose a reason for hiding this comment

Uh oh!

leandrodamascena May 16, 2023

Choose a reason for hiding this comment

Uh oh!

heitorlessa May 17, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

heitorlessa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

heitorlessa May 17, 2023

Choose a reason for hiding this comment

Uh oh!

rubenfonseca May 17, 2023

Choose a reason for hiding this comment

Uh oh!

rubenfonseca May 17, 2023

Choose a reason for hiding this comment

Uh oh!

rubenfonseca May 17, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

heitorlessa May 17, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rubenfonseca commented May 17, 2023

Uh oh!

leandrodamascena left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rubenfonseca commented May 16, 2023 •

edited

Loading

codecov-commenter commented May 16, 2023 •

edited

Loading