Fix anonymous user access with public_access defined and fix public_access tests #9007
Conversation
| @@ -46,7 +46,7 @@ async function run_on_random_bucket(s3, bucket_prefix, cb) { | |||
| try { | |||
| await cb(bucket); | |||
| } finally { | |||
| await s3.deleteBucket({ Bucket: bucket }); | |||
| await s3.deleteBucket({ Bucket: bucket }).catch(() => { /* nothing */ }); | |||
There was a problem hiding this comment.
why would delete bucket fail? shouldn't we handle such failure?
There was a problem hiding this comment.
Because sometimes the cb code might forget to delete all the objects (and their versions) from the bucket but that doesn't effect the correctness.
Failing to delete should be OK if the cb deems so. We can obviously write code to handle those cases but I think that would be more fragile without adding anything to the correctness of the respective tests. WDYT?
| @@ -110,29 +110,33 @@ mocha.describe('noobaa public access block test', function() { | |||
|
|
|||
| mocha.it('put_public_access_block must throw when acls are used', async function() { | |||
| await run_on_random_bucket(s3_owner, bucket_prefix, async bucket => { | |||
| try { | |||
There was a problem hiding this comment.
what is the difference between the old tests and the new? can you add a short explanation in the PR of why did the test not work, and what you changed
There was a problem hiding this comment.
Right, actually assert.fail throws but the finally runs anyway so basically its like .catch(_.noop). Which gives us false positives.
Signed-off-by: Utkarsh Srivastava <[email protected]>
tangledbytes
force-pushed
the
utkarsh/fix/public-access-block-anon
branch
from
b453b17 to
02bfc94
Compare
Describe the Problem
The problem was noticed by the QE while verifying PublicAccessBlock (PAB) feature for ODF. It was noticed that although the feature worked fine for the noobaa users but failed to work when anonymous user was used. That is, when a public policy was used and the
RestrictPublicAccesswas applied to it, the anonymous user could still access the data which is undesirable.Explain the Changes
There was missing
RestrictPublicAccesscheck in the anonymous auth flow which this PR adds. Also, the tests were badly written by me or else our CI would have caught the issue. I have fixed those tests now.Issues: Fixed #xxx / Gap #xxx
Feature Epic: https://issues.redhat.com/browse/RHSTOR-6623
Reported Issue: https://issues.redhat.com/browse/DFBUGS-2382
Testing Instructions:
./node_modules/.bin/jest src/test/unit_tests/jest_tests/test_s3_utils.test.js./node_modules/.bin/mocha src/test/unit_tests/test_public_access_block.jsNC_CORETEST=true NC_NSFS_NO_DB_ENV=true ./node_modules/.bin/mocha src/test/unit_tests/test_public_access_block.js