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

feat: Allow CORS policy to be configured #484

Merged
merged 5 commits into from
Nov 17, 2023

Conversation

korylprince
Copy link
Contributor

This allows the CORS policy to be configured or disabled on an op.Provider via WithCORSOptions or on on.RegisterServer via WithServerCORSOptions. The reason for this feature is library users may want a stricter CORS policy than the default policy currently forced by the library.

This takes care not to break backwards compatibility with any interfaces, functions, or default usage (e.g. the default policy is still applied by default).

Also, this removes the CORS middleware from the intercept function as every upstream use of it already had the CORS middleware applied.

@rdiers
Copy link

rdiers commented Nov 14, 2023

@korylprince ...Requesting if this can also be back-ported to the v2 implementation as well.

korylprince added a commit to korylprince/oidc that referenced this pull request Nov 15, 2023
@korylprince
Copy link
Contributor Author

@radsec PR #485 is the backported version of this.

muhlemmer
muhlemmer previously approved these changes Nov 16, 2023
Copy link
Collaborator

@muhlemmer muhlemmer left a comment

Choose a reason for hiding this comment

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

Thanks! looks great

@muhlemmer muhlemmer enabled auto-merge (squash) November 16, 2023 08:57
pkg/op/server_http.go Outdated Show resolved Hide resolved
auto-merge was automatically disabled November 16, 2023 23:50

Head branch was pushed to by a user without write access

Copy link

codecov bot commented Nov 16, 2023

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (f014796) 60.29% compared to head (5d7cb2c) 60.20%.

Files Patch % Lines
pkg/op/op.go 50.00% 8 Missing ⚠️
pkg/op/server_http.go 60.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #484      +/-   ##
==========================================
- Coverage   60.29%   60.20%   -0.10%     
==========================================
  Files          78       78              
  Lines        6753     6775      +22     
==========================================
+ Hits         4072     4079       +7     
- Misses       2385     2399      +14     
- Partials      296      297       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@korylprince
Copy link
Contributor Author

@muhlemmer thanks for taking a look at this! I pushed a new commit that I think works around this issue nicely. Instead of wrapping the *webServer itself for CORS, keep an internal handler that can either be the router or the CORS-wrapped router, and use that in ServeHTTP.

Copy link
Collaborator

@muhlemmer muhlemmer left a comment

Choose a reason for hiding this comment

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

Perfect, thanks!

@muhlemmer muhlemmer merged commit 7b64687 into zitadel:main Nov 17, 2023
11 of 13 checks passed
Copy link

🎉 This PR is included in version 3.3.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@muhlemmer
Copy link
Collaborator

hey @korylprince thanks for the contribution.

If you'd like to have a small gift in return, please send us a mail to hi@zitadel.com. We will send you a form with questions about your address and shirt size.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants