Change Tomcat Valve to servlet Filter #11
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11 +/- ##
============================================
+ Coverage 86.11% 89.47% +3.35%
+ Complexity 116 112 -4
============================================
Files 6 9 +3
Lines 353 323 -30
Branches 65 53 -12
============================================
- Hits 304 289 -15
+ Misses 24 12 -12
+ Partials 25 22 -3
Continue to review full report at Codecov.
|
|
Pinging @acoburn and @ajs6f help to reveal my mistake. I setup the Filter to attempt to load a relative location of the settings file by using I had to ignore the test it as I am at a loss why this is not working. Alternatively, I'll just required the path to be absolute. |
|
@whikloj this will sound silly, but, I think, maven puts test resources at the root of your classpath (right?) by default but maybe gradle needs to be told where to look at?, It defines two different resource locations, main v/s tests.. Maybe you need in your build.gradle, for src/test/resources/exampleSettings.yaml to be found or something like ? I'm guessing really here 😢 , you know java is like lava for me, can't swim there but looks tempting and glowing... I feel it all depends on the output of running SynFilter.class.getClassLoader().getResource(".") and see what that points to when running the tests. As always dismiss if all this is just me confused and you tried that. good night |
|
I'm going to defer to @acoburn here, who knows more about Gradle in his little finger than I do in my whole body. I will offer that loading editable config from the web app itself is generally not a good idea for a Java webapp. Better to have a config location outside the web app. Ideally, the web app itself (on the filesystem) never changes at all and can be deployed packed and operated packed. |
There was a problem hiding this comment.
Looks basically good to me, code-wise. I ran out of time but generally there are a lot of things called testXml that aren't XML and there are a good number of Autocloseable things that aren't being used in try-with-resource and there are a number of for loops that ought to be Iterable::forEachcalls, but these are all niceties that won't affect the operation of the code significantly.
| assertEquals(username, requestCaptor.getValue().getUserPrincipal().getName()); | ||
|
|
||
| for (final String role : finalRoles) { | ||
| assertTrue(requestCaptor.getValue().isUserInRole(role)); |
There was a problem hiding this comment.
Unless order is important, prefer finalRoles.forEach(role -> assertTrue(requestCaptor.getValue().isUserInRole(role)))
|
|
||
| final InputStream stream = new ByteArrayInputStream(testXml.getBytes()); | ||
| final Config settings = SettingsParser.getSitesObject(stream); | ||
| final String testXml = String.join("\n", |
|
|
||
| final InputStream stream = new ByteArrayInputStream(testXml.getBytes()); | ||
| final Config settings = SettingsParser.getSitesObject(stream); | ||
| final String testXml = String.join("\n", |
|
|
||
| final InputStream stream = new ByteArrayInputStream(testXml.getBytes()); | ||
| final Config settings = SettingsParser.getSitesObject(stream); | ||
| final String testXml = String.join("\n", |
|
|
||
| final InputStream stream = new ByteArrayInputStream(testXml.getBytes()); | ||
| final Config settings = SettingsParser.getSitesObject(stream); | ||
| final String testXml = String.join("\n", |
| final InputStream stream = new ByteArrayInputStream(testXml.getBytes()); | ||
| final Config settings = SettingsParser.getSitesObject(stream); | ||
| assertEquals(-1, settings.getVersion()); | ||
| final String testXml = String.join("\n", |
| " multiline", | ||
| " key"); | ||
|
|
||
| final StringReader stream = new StringReader(testXml); |
There was a problem hiding this comment.
StringReader is Autocloseable; ideally this would be in try-with-resource.
| final InputStream stream = new ByteArrayInputStream(testXml.getBytes()); | ||
| final Config settings = SettingsParser.getSitesObject(stream); | ||
| assertEquals(12, settings.getVersion()); | ||
| final String testXml = String.join("\n", |
|
@whikloj from what I can tell, this is the issue:
(That's why the test results show this message: "No key found for site: ...") |
|
@whikloj I have some meetings this AM, but I can do a proper review later today. |
|
@whikloj one more thing (before I forget this) -- I agree with @ajs6f's comment about not allowing an editable config file in the web application. By allowing/encouraging folks to bake a default username/password into an application just opens you up to quite a few security-related issues. I think it's much better to remove that feature altogether and simply require users to have an external configuration file. |
|
@ajs6f @acoburn @DiegoPino thanks for having a look. My java experience always lacks a certain flair so I appreciate the help. I'll work on the code review from @ajs6f and it seems the consensus is to drop the relative file locating. It was a re-framing of the previous finding the resource relative to $CATALINA_HOME which no longer made sense. Instead I'll just require an absolute location to the configuration file. |
|
@whikloj Let me know if this is ready for testing? |
|
@Natkeeran this is ready when ever you are. |
|
While trying to do the setup to test this, on Step 8, I get the following: |
|
@Natkeeran sorry I was cleaning up for Islandora 7.x-1.x stuff and I deleted my repository. We should probably stop making repos with the name of the whole stack. Anyways I recreated it, so you should be good to go. |
|
I followed the test instructions, still able to PUT without authorization. Since this PR is dependent on two others, I assume those two have to go in first before this can be tested. |
|
I followed your steps. But, I am still able to put without the token. Are there specific things I can check to make sure I have your changes? |
|
I'll try this again and see if I can see what is happening. |
|
@Natkeeran ok I had steps 3 and 4 above reversed, I have corrected them. So you can try again. |
|
@Natkeeran Can you give this another shot? If you don't think you'll have the time, let me know and I'll see if someone else can hop on it. This has popped back up in Islandora/documentation#966. |
|
@dannylamb @Natkeeran I'll try to get this rebased quickly |
|
@whikloj I can test when this is ready. |
|
I just use the button, I haven't had a problem yet. |
|
Crayfish doesn't seem to survive the script in the testing instructions, which is back from when we didn't use features. I'm going to be smart next time and make a snapshot before running the script 🤦♂️ I should just be able to strip out most of it and get it working. |
|
@whikloj Success! Using this in lieu of your gist: cd /var/www/html/drupal/web/modules/contrib/islandora
git remote add whikloj https://github.com/whikloj/islandora-1.git
git fetch whikloj
git checkout issue-736
/var/www/html/drupal/vendor/bin/drupal router:rebuild
/var/www/html/drupal/vendor/bin/drupal cache:rebuild all
MODULES="Gemini Milliner Houdini Hypercube"
for i in $MODULES; do
cd /var/www/html/Crayfish/$i/vendor/islandora/crayfish-commons
sudo git remote add whikloj https://github.com/whikloj/Crayfish-commons.git
sudo git fetch whikloj
sudo git checkout issue-736
doneMade an image and it kicked off derivatives. Everything wound up in Gemini. Then I confirmed I can't write as anonymous But can when authenticated |
This reverts commit 9482e95.
GitHub Issue: Islandora/documentation#736
Depends on: Islandora/documentation#84
Depends on: Islandora/Crayfish-Commons#15
What does this Pull Request do?
What's new?
The servletfilter will fail to initialize if it can't access it's settings file or the settings file is invalid.
In theory this filter should work on non-Tomcat container, but I haven't tried that.
The claim changes are
id->webidname->suburl->issDoes this change require documentation to be updated? Documentation has been updated
Does this change add any new dependencies? no
Does this change require any other modifications to be made to the repository (ie. Regeneration activity, etc.)?
Could this change impact execution of existing code?
How should this be tested?
There is no easy way to test this, it will require uninstalling Islandora. But this should hopefully work.
roles/externaldirectory if it exists.git patch <patchfile>.ansible-galaxy install --role-file=requirements.yml --roles-path=roles/externalroles/external/Islandora-Devops.fcrepo-syndefaults/main.ymlchangefcrepo_syn_version: 0.1.0tofcrepo_syn_version: 0.1.1tasks/install.ymlchangeurl: https://github.com/Islandora-CLAW/Syn/releases/download/v{{ fcrepo_syn_version }}/islandora-syn-{{ fcrepo_syn_version }}-all.jartourl: https://github.com/whikloj/Syn/releases/download/v{{ fcrepo_syn_version }}/islandora-syn-{{ fcrepo_syn_version }}-all.jarvagrant upvagrant sshYou should be able to create a collection or image in Drupal and have it sync to Fedora.
But you should not be able to PUT/POST to fedora without a valid JWT or token. Like
Additional Notes:
Any additional information that you think would be helpful when reviewing this PR.
Interested parties
Tag (@ mention) interested parties or, if unsure, @Islandora-CLAW/committers