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

ConfigurationUtil change broke CorsConfigUtil #3

Open
ehasoo opened this issue Feb 22, 2023 · 2 comments · May be fixed by #4
Open

ConfigurationUtil change broke CorsConfigUtil #3

ehasoo opened this issue Feb 22, 2023 · 2 comments · May be fixed by #4

Comments

@ehasoo
Copy link

ehasoo commented Feb 22, 2023

Hey sorry for posting bad issue but its critical.

I'm not 100% sure but it seems that ConfigurationUtil change from kumuluz/kumuluzee#215
broke com.kumuluz.ee.cors.utils.CorsConfigUtil line 34

ConfigurationUtil cfg = ConfigurationUtil.getInstance();
String configKey = "kumuluzee.cors-filter.annotations." + key;
Optional corsFilterOpt = cfg.get(configKey);
if (corsFilterOpt.isPresent()) {

@Jamsek-m Jamsek-m linked a pull request Feb 22, 2023 that will close this issue
@bannmann
Copy link

bannmann commented Jan 5, 2024

Hey team, could you look at this? I can confirm the bug with 4.1.0 (I didn't use CORS with earlier versions).

The following config was entirely ignored and the CORS extension only used the annotation value:

kumuluzee:
  cors-filter:
    annotations:
      my-resource:
        allow-origin: "https://kumuluz.com"

Debugging and fiddling around, I found the workaround below. However, given that the Kumuluz CORS docs suggest using the nested YAML syntax, this is neither obvious nor really feasible.

kumuluzee:
  cors-filter.annotations.my-resource: arbitrary
  cors-filter.annotations.my-resource.allow-origin: "https://kumuluz.com"

Also, speaking of the docs: reading the source, I was surprised to find that setting any CORS config property for a resource makes Kumuluz ignore all annotation values, not just the one corresponding to the property. While that behavior makes sense, it should definitely be documented explicitly!

@MBJuric
Copy link
Member

MBJuric commented Jan 8, 2024

We will have a look and fix the issue. Thanks for the feedback.

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 a pull request may close this issue.

3 participants