Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CORS: add an optional custom function to validate the origin #1651

Merged
merged 2 commits into from Nov 25, 2020

Conversation

dahu33
Copy link
Contributor

@dahu33 dahu33 commented Oct 9, 2020

The CORS middleware is great and works very well if the list of allowed origins is static. However, in many use cases, this list need to be dynamic (for example, if coming from the database). This is a non-breaking change that makes the CORS middleware accepting an optional custom function to validate the origin.

PS: the diff looks weird for some unknown reason but the code change is minimal (mostly indentation).

Thanks!

@codecov
Copy link

codecov bot commented Oct 9, 2020

Codecov Report

Merging #1651 (e6f24aa) into master (17a5fca) will increase coverage by 0.15%.
The diff coverage is 90.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1651      +/-   ##
==========================================
+ Coverage   84.72%   84.88%   +0.15%     
==========================================
  Files          29       29              
  Lines        1938     1945       +7     
==========================================
+ Hits         1642     1651       +9     
+ Misses        188      187       -1     
+ Partials      108      107       -1     
Impacted Files Coverage Δ
middleware/cors.go 89.15% <90.90%> (+3.63%) ⬆️

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 17a5fca...e6f24aa. Read the comment docs.

@shoenseiwaso
Copy link

Codecov appears broken, it's saying coverage was reduced by 284 lines in files completely unrelated to the change. See also this 3 line README.md change with a similar code coverage result. Echo maintainers, let us know if we've missed something here!

Copy link
Contributor

@pafuent pafuent left a comment

Choose a reason for hiding this comment

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

LGTM

@dahu33
Copy link
Contributor Author

dahu33 commented Nov 6, 2020

@pafuent thank you very much for the review! @lammel, @vishr, any chance this could merged (I cannot merge myself) and land in the next release?

middleware/cors.go Outdated Show resolved Hide resolved
Copy link
Contributor

@lammel lammel left a comment

Choose a reason for hiding this comment

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

The use case looks valid to me.
I also do not see any breaking changes. Tests look ok, if you can think of further useful tests.

Would you mind providing a PR with a code example in https://github.com/labstack/echox too.
That would complete the change and help others with an example.

@dahu33
Copy link
Contributor Author

dahu33 commented Nov 16, 2020

@lammel thanks the review. I have implemented your feedback and opened a PR here labstack/echox#164 to document the feature. Could you please have a look?

@lammel
Copy link
Contributor

lammel commented Nov 20, 2020

Seems I was too eager merging another CORS related issue first.
Could you rebase please, will merge it than.

@dahu33
Copy link
Contributor Author

dahu33 commented Nov 24, 2020

Seems I was too eager merging another CORS related issue first.
Could you rebase please, will merge it than.

Done! The diff looks weird for some reason but the code change is the same as before.

@lammel lammel merged commit 502cce2 into labstack:master Nov 25, 2020
@lammel
Copy link
Contributor

lammel commented Nov 25, 2020

Merged. Thanks @dahu33 !

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.

None yet

4 participants