PublicAccessBlock Support #8571
Conversation
tangledbytes
force-pushed
the
utkarsh/feat/public-access-block
branch
from
f9f367e to
f26335e
Compare
tangledbytes
force-pushed
the
utkarsh/feat/public-access-block
branch
from
f26335e to
0144b3b
Compare
|
@tangledbytes Can we add tests here? I can also see ceph s3-tests have some tests regarding this feature - maybe also we can add those. |
| @@ -168,14 +173,19 @@ function _is_action_fit(method, statement) { | |||
| return statement.Action ? action_fit : !action_fit; | |||
| } | |||
|
|
|||
| function _is_principal_fit(account, statement) { | |||
| function _is_principal_fit(account, statement, ignore_anon_principal = false) { | |||
There was a problem hiding this comment.
why not call it "disallow_public_access" as in the earlier parts of the flow? I think this name is more clear; we know why we should ignore the fit.
There was a problem hiding this comment.
Actually it seems semantically different. _is_principal_fit shouldn't really be concerned disallow_public_access as it seems a higher level construct. That's why even in invocation I used the keyword disallow_public_access however in definition I used ignore_anon_principal.
I can change it though if it seems confusing to follow (easily readable code is I think more important than my "semantically better" code).
There was a problem hiding this comment.
I'm just not sure I fully get the flow. If we want to disallow public access. Shouldn't we have it only for Allow statement and not for Deny? It seems to me that we should not ignore a fit in case of a Deny statement.
Also, If the issue is with semantics, we need a better name for this as * is not just for anonymous principals it is for any principal.
Maybe some more comments in this area will be good to make me and future code readers better understand its use.
There was a problem hiding this comment.
@tangledbytes is this relevant also for deny? no response to my previous comment
There was a problem hiding this comment.
Ah! I am actually sorry. I actually thought that I responded here but turns out I forgot. Yes, you are right, I should not ignore it in case of DENY.
Also yes, it sounds right that anon isn't the right word. I will replace it with public_principla? How's that?
Again. apologies, missed this 😅
tangledbytes
force-pushed
the
utkarsh/feat/public-access-block
branch
from
0144b3b to
b1e0df2
Compare
tangledbytes
force-pushed
the
utkarsh/feat/public-access-block
branch
from
b1e0df2 to
882f83e
Compare
tangledbytes
force-pushed
the
utkarsh/feat/public-access-block
branch
2 times, most recently
from
df1bf16 to
6f9f7ef
Compare
Signed-off-by: Utkarsh Srivastava <[email protected]> Complete BlockPublicAccess integration Signed-off-by: Utkarsh Srivastava <[email protected]> add NC test for PublicAccessBlock Signed-off-by: Utkarsh Srivastava <[email protected]> dont check for disallow_public_access in DENY statements Signed-off-by: Utkarsh Srivastava <[email protected]>
tangledbytes
force-pushed
the
utkarsh/feat/public-access-block
branch
from
6f9f7ef to
645c917
Compare
Explain the changes
This PR attempts to add basic support for PublicAccessBlock.
Issues: Fixed #xxx / Gap #xxx
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