Corsfilter fix#12151
Conversation
ErykKul
left a comment
There was a problem hiding this comment.
Looks good to me. Thanks! Just a release note and it's ready.
|
@ErykKul Should I just add |
|
@beepsoft Please feel free to create an issue and link this PR to it. Can be brief. Also, I see Jenkins is not happy with one IT: Can you please look into that? Thx! Also, CorsIT must be added to https://github.com/IQSS/dataverse/blob/develop/tests/integration-tests.txt |
|
Do we need an issue at this point? I only raised the issue question because the release note authoring README.md mentioned, but I don't know if and how that applies here. Regarding SwordIT: I could not reproduce it locally. I added CorsIT to integration-tests.txt. |
|
I see in https://jenkins.dataverse.org/blue/organizations/jenkins/IQSS-Dataverse-Develop-PR/detail/PR-12151/7/tests that the tests now run, but CorsIT fails - the headers are all not present. I have a hunch that we might need @donsizemore to enable CORS during testing. Unfortunately, we currently have no way to change JVM options at runtime (there may exist an issue about this already). @pdurbin WDYT should we disable these tests until we figure out the remote JVM option part? Can we just enable CORS in Jenkins without affecting other tests? |
|
FWIW: I've recently started making non-IT tests with direct (mocked) calls to API methods to be able to test non-default JVM options, e.g. in LDNInboxTest.java and LocalContextsTest.java. Is that a useful approach here? |
|
There seem to be some open questions and some failing tests and the PR is still in draft so I moved it from "ready for QA" to "ready for review". |
@beepsoft |
|
https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-12151/16/testReport/ CorsIT tests pass, pipeline back to normal |
|
Thank you all for the work you put into this. Sorry I couldn’t be involved over the past few weeks. Is there anything else I need to do on this, or can we go ahead and close the issue? |
Hi. Thank you for your work on this too! From my side, I would very strongly recommend you to rewrite the assertions for CorsIT as I mentioned in my earlier review. Installations like to run the test suites again, and if they set their CORS allowance policies to "*" then, CorsIT would fail and they wouldn't be able to figure out why. I know this because it is the exact same thing that happened with me ;) Once again though, amazing work from everyone. |
|
@beepsoft is CORS on or off by default as of this PR? Please see my confusion over the docs: #12151 (comment) |
|
@beepsoft p.s. @srmanda-cs and I are talking here if you want to join: https://dataverse.zulipchat.com/#narrow/channel/379673-dev/topic/CorsFilter.20Servlet.20Filter.20not.20discovered/with/584443987 |
…elocate Big Data guide IQSS#12151 IQSS#12161
pdurbin
left a comment
There was a problem hiding this comment.
I zoomed with @poikilotherm and he resolved my remaining issues with the docs in 2cac1fe (thanks!). API tests are passing. (Heads up to @srmanda-cs that, like Jenkins, we'll need to configure containers to turn on CORS for the CORS IT tests to pass.) I'm approving this. Thanks, all!
|
@pdurbin Why was this assigned back to us all? I'm not sure what's missing. I'm a bit puzzled what the goal is with Jenkins tests and all... |
@poikilotherm please see #12151 (comment) and #dev > CorsFilter Servlet Filter not discovered @ 💬 |
The same applies to me. If there are any pending failures for @srmanda-cs or others in their environments, a fix is welcome, but I cannot commit to checking all possible test environments. This issue was supposed to be just about five lines of changes in CorsFilter.java 🙂 |
Due to our test infrastructure, it's not easy to do assertions on the content of the headers. Also, for the issue fixed in the CORS filter, this test doesn't need to check the content, just make sure they are present.
|
@beepsoft the conversation is happening here: https://dataverse.zulipchat.com/#narrow/channel/379673-dev/topic/CorsFilter.20Servlet.20Filter.20not.20discovered/near/585851791 @poikilotherm seems to have pushed a fix at ed07f96 so I'm unassigning you, @beepsoft. |
|
@pdurbin Jenkins is happy now, the CI env is no longer interfering. Took Ash and myself off. IMHO this is ready for QA unless you see something else. |
@srmanda-cs said I could dismiss his review at https://dataverse.zulipchat.com/#narrow/channel/379673-dev/topic/CorsFilter.20Servlet.20Filter.20not.20discovered/near/585903599 (all the fixes he wanted are now in).
What this PR does / why we need it:
Fixes
CorsFilterinvocation inconsistency.When accessing /api/... endpoints the
CorsFilterwas not always hit and so these endpoints could not be used from webapps even if CORS was properly configured.The problem seemed to be with
ApiRouter's forwarding mechanism from /api/... to /api/v1/...Adding FORWARD dispatcher types to
@WebFiltersolves this.Fix is based on this zulip thread:
https://dataverse.zulipchat.com/#narrow/channel/379673-dev/topic/CorsFilter.20Servlet.20Filter.20not.20discovered/near/572358009
Also fixes #12161 with docs changes made by @poikilotherm originally as PR #12162 . Preview doc changes here: https://dataverse-guide--12151.org.readthedocs.build/en/12151/installation/config.html#cross-origin-resource-sharing-cors
Suggestions on how to test this:
CorsIT.javais added to test whether CORS headers are present for both /api/... and /api/v1/... invocations. For it to pass CORS must be enabled in Dataverse, eg. by settingDATAVERSE_CORS_ORIGIN: "*"env for
dev_dataverseindocker-compose-dev.yml