Improvement/cldsrv 427 permissions check #5421
Conversation
Hello benzekrimaha,My role is to assist you with the merge of this Status report is not available. |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
| return false; | ||
| } | ||
| return (aclPermission || (bucketPolicyPermission === 'allow')); | ||
| if (method === 'isBucketAuthorized') { |
There was a problem hiding this comment.
I'm of the personal opinion that if such conditions are necessary for such large parts of code, it's better practice to keep this separate into smaller, different functions.
There was a problem hiding this comment.
Actually this check was only added for the bucket specific checks/ object ones in the else , as the functions are nearly identical besides for those checks I used the method param passed by the functions 'isBucketAUthorized', 'isObjectAuthorized' and 'evaluateBucketPolicyWithIAM' to separate them and keep all of the changes related to authorizations in only one function (can ease modifications in the future as well)
| requestTypes.forEach(requestType => { | ||
| if (actionImplicitDenies[requestType] === undefined) { | ||
| // eslint-disable-next-line no-param-reassign | ||
| actionImplicitDenies[requestType] = false; | ||
| } | ||
| }); |
There was a problem hiding this comment.
No need to initialize undefined values of actionImplicitDenies.
Inside the second forEach you can directly test if it's false with !!actionImplicitDenies[requestType].
| return true; | ||
| const results = {}; | ||
| const mainApiCall = requestTypes[0]; | ||
| requestTypes.forEach(_requestType => { |
There was a problem hiding this comment.
You can directly return requestTypes.every(_requestType => {...}) instead of storing and checking all the results.
This will also avoid looping over everything when you encounter a first error.
benzekrimaha
changed the base branch from
development/7.10
to
improvement/CLDSRV-426-acl-impl-deny
change variable names for clarity edit: update arsenal package
Update lib/api/api.js Co-authored-by: Jonathan Gramain <[email protected]>
CLDSRV-426:fixups on ACL permission checks for implicitDeny logic CLDSRV-426:better readability on ACL permission
CLDSRV-426: additionnal test for ACL permission
…denies (cherry picked from commit c01898f)
benzekrimaha
force-pushed
the
improvement/CLDSRV-427-permissions-check
branch
from
bb3da5a to
e572f97
Compare
benzekrimaha
changed the base branch from
improvement/CLDSRV-426-acl-impl-deny
to
development/7.10
benzekrimaha
force-pushed
the
improvement/CLDSRV-427-permissions-check
branch
from
e572f97 to
6919ca7
Compare
- In this commit , I added a helper (processBucketPolicy) function for the bycket policies checks that are shared between the isbucketAuthorized, isObjAuthorized and evaluateBucketPolicyWithIAM for a better code readability and to avoid long functions.
benzekrimaha
force-pushed
the
improvement/CLDSRV-427-permissions-check
branch
from
15ef59c to
33d7c99
Compare
PR opened after closing : #5323
Bucket policies are not correctly interpreted, this is part of the following epic to fix that: scality/Arsenal#2181
This PR is aiming enable authorization logics from IAM policies, bucket policies and ACLs to be interpreted in aggregate in CloudServer's 'isBucketAuthorized' and 'isObjectAuthorized' functions. , ticket linked to this issue here : https://scality.atlassian.net/browse/CLDSRV-427
PRs providing implicit Deny logic to CS for processing in this PR
scality/Arsenal#2181
https://github.com/scality/Vault/pull/2135
#5322
#5420
I'm not bumping a new CLDSRV version since a new version has been created in this merged PR : #5322 , Please let me know if it needs to be done anyways.