Fix favorites to consult allowlist #3526
Conversation
|
Need to take a look a the |
| `mkdir -p #{project_path}` | ||
| `mkdir -p #{project_path2}` | ||
| # regular directory now though? | ||
| #`mkdir -p #{s3_path}` |
There was a problem hiding this comment.
Can you expand more on what you mean by this? The next assertion passes when I uncomment this line, and it's the URLs that are wrong - and the problem there:
Which is what the old assertion was (but then if I change to the old assertion I still get S3 weirdness, so I want to investigate your thoughts behind this being commented out).
| p.remote? || p.path.directory? && p.path.readable? && p.path.executable? | ||
| Configuration.allowlist_paths.include?(p.path) && (p.remote? || p.path.directory?) && p.path.readable? && p.path.executable? |
There was a problem hiding this comment.
I think this is the source of the issues you're having with the tests.
This would evaluate to false. I.e., it does not account for child paths.
['/a'].include?('/a/b')
I think you need to use Allowlist.default.permitted?(p.path)
There was a problem hiding this comment.
What is Allowlist here?
There was a problem hiding this comment.
I replaced Configuration.allowlist_paths.include?(p.path) with AllowlistPolicy.default.permitted?(p.path) and there is still wonkiness with the way the S3 path is expanded in the test.
There was a problem hiding this comment.
Yep you found it - I pulled that from memory, should have been AllowlistPolicy
| test 'should create Files dropdown' do | ||
| scratch_path = File.expand_path 'test/fixtures/dummy_fs/scratch' |
There was a problem hiding this comment.
I don't think we need to edit this test case. I just checked out the master version of this file and ran the tests and it passes without modification.
Which is to say, I think we should generally add tests and try not to modify tests. Tests are our safeguards and I think this one ensures that remote filesystems will always appear in the dropdown.
There was a problem hiding this comment.
Coming back around to this.
This test will fail on the branch with the change, because the paths scratch_path and project_path are not allowed, so they don't display which caused the previous test failures.
The test case was only altered when the failure showed up after the change. I can change it back, but the tests will fail like on the previous commit:
https://github.com/OSC/ondemand/actions/runs/9004647134/job/24738112032
There was a problem hiding this comment.
This test will fail on the branch with the change, because the paths scratch_path and project_path are not allowed, so they don't display which caused the previous test failures.
There's no allowlist setting in the original test. What I'm saying is that we should revert it back to what it is in the master branch.
The original test case tests the behavior when no OOD_ALLOWLIST_PATH has been set and I think we should keep that case.
There was a problem hiding this comment.
Ok. I can revert it, what do we do when it fails though?
There was a problem hiding this comment.
Ok. I can revert it, what do we do when it fails though?
I just tested and it passes. But if it fails - then it's doing what it's supposed to: signaling to us that we did something wrong. That's the whole point to test cases - they're guard rails to say that this is the behavior that we want to preserve. If they fail, then we're not preserving that behavior correctly and we need to fix something in the code.
There was a problem hiding this comment.
Oh my lord, i know what a test is man lol.
I just don't get how this passes with the change, but that's fine. Mark as done and we can finally be done with this infernal code.
Oglopf
force-pushed
the
allowlist-favs-fix
branch
from
ce4f833 to
a7bb946
Compare
Fixes #3193