[CORS] Allow Access-Control-Allow-Headers customization #36473
Conversation
Closes: keycloak#12682 Signed-off-by: Dmitry Telegin <dmitryt@backbase.com>
70df3cf to
7a29e65
Compare
There was a problem hiding this comment.
@dteleguin Thanks for the PR! I think the enhancement is valid, but it should be also reviewed by @keycloak/core team.
Review:
- I've added a small comment for String vs. Set
- Please add some basic tests for CORS
- In order to pass the HelpCommandDistTest, execute:
KEYCLOAK_REPLACE_EXPECTED=true mvn clean install -Dtest=HelpCommandDistTest -f quarkus/tests/integration/pom.xml
| private Set<String> exposedHeaders; | ||
|
|
||
| private boolean preflight; | ||
| private boolean auth; | ||
|
|
||
| DefaultCors(KeycloakSession session) { | ||
| DefaultCors(KeycloakSession session, String defaultAllowHeaders) { |
There was a problem hiding this comment.
| DefaultCors(KeycloakSession session, String defaultAllowHeaders) { | |
| DefaultCors(KeycloakSession session, Set<String> defaultAllowHeaders) { |
Is it possible to accept a Set of headers? In order to execute String.format("%s, %s", only at one place -> DefaultCors. The manageability would be better.
There was a problem hiding this comment.
This should not be a top-level server config option, but rather if we want to do this just an SPI provider option on the default cors provider.
The option should probably also be additional headers, rather than headers; as otherwise you'd have to include all headers KC needs already.
|
@stianst there is an SPI provider option already, namely I have introduced an abbreviated version ( So, you are suggesting that we remove |
I was thinking of the same - using sets will also allow to sort / deduplicate header names, which would not be possible with preformatted strings.
Could you please suggest particular tests for this PR?
Thanks for the hint. The top-level config option is currently being discussed with Stian. |
There was a problem hiding this comment.
I like the extension to the CORS SPI to set the headers, it is an obvious boon for extension developers that should be controllable. I don't think we should be adding any options to overwrite these options through configuration at this point in time (see #12682 (comment)).
|
Okay, so there seems to be a consensus that we should not be exposing this as a top-level user-visible config option (at least for now). I will close this PR and create another one covering just the API part. The user-visible config option will be implemented as a 3rd-party extension. |
|
Superseded by #40011 |
This PR allows simple customization of HTTP headers that should be allowed in cross-origin requests, in addition to the built-in list. A new CLI option is introduced:
Corresponding environment variable:
This is a global (server-level) setting. The main use case is supporting headers that are added by various tracing systems and web frameworks.
Closes: #12682