CR changes

Signed-off-by: shirady <[email protected]>

Author: shirady

src/endpoint/s3/s3_bucket_policy_utils.js

@@ -149,21 +149,28 @@ async function _is_object_version_fit(req, predicate, value) {
     return res;
 }
 
-async function has_bucket_policy_permission(policy, account, method, arn_path, req, disallow_public_access = false,
-        should_pass_principal = true) {
+async function has_bucket_policy_permission(policy, account, method, arn_path, req,
+    { disallow_public_access = false, should_pass_principal = true } = {}) {
     const [allow_statements, deny_statements] = _.partition(policy.Statement, statement => statement.Effect === 'Allow');
 
     // the case where the permission is an array started in op get_object_attributes
     const method_arr = Array.isArray(method) ? method : [method];
 
     // look for explicit denies
     const res_arr_deny = await is_statement_fit_of_method_array(
-        deny_statements, account, method_arr, arn_path, req, undefined, should_pass_principal); // No need to disallow in "DENY"
+        deny_statements, account, method_arr, arn_path, req, {
+            disallow_public_access: false, // No need to disallow in "DENY"
+            should_pass_principal
+        }
+    );
     if (res_arr_deny.every(item => item)) return 'DENY';
 
     // look for explicit allows
     const res_arr_allow = await is_statement_fit_of_method_array(
-        allow_statements, account, method_arr, arn_path, req, disallow_public_access, should_pass_principal);
+        allow_statements, account, method_arr, arn_path, req, {
+            disallow_public_access,
+            should_pass_principal
+        });
     if (res_arr_allow.every(item => item)) return 'ALLOW';
 
     // implicit deny
@@ -219,13 +226,13 @@ function _is_resource_fit(arn_path, statement) {
 }
 
 async function is_statement_fit_of_method_array(statements, account, method_arr, arn_path, req,
-    disallow_public_access = false, should_pass_principal = true) {
+    { disallow_public_access = false, should_pass_principal = true } = {}) {
     return Promise.all(method_arr.map(method_permission =>
-        _is_statements_fit(statements, account, method_permission, arn_path, req, disallow_public_access, should_pass_principal)));
+        _is_statements_fit(statements, account, method_permission, arn_path, req, { disallow_public_access, should_pass_principal })));
 }
 
-async function _is_statements_fit(statements, account, method, arn_path, req, disallow_public_access = false,
-        should_pass_principal = true) {
+async function _is_statements_fit(statements, account, method, arn_path, req,
+    { disallow_public_access = false, should_pass_principal = true} = {}) {
     for (const statement of statements) {
         const action_fit = _is_action_fit(method, statement);
         const principal_fit = should_pass_principal ? _is_principal_fit(account, statement, disallow_public_access) : true;

src/endpoint/s3/s3_rest.js

@@ -301,14 +301,17 @@ async function authorize_request_policy(req) {
     // we start the permission check on account identifier intentionally
     if (account_identifier_id) {
         permission_by_id = await s3_bucket_policy_utils.has_bucket_policy_permission(
-            s3_policy, account_identifier_id, method, arn_path, req, public_access_block?.restrict_public_buckets);
+            s3_policy, account_identifier_id, method, arn_path, req,
+            { disallow_public_access: public_access_block?.restrict_public_buckets }
+        );
         dbg.log3('authorize_request_policy: permission_by_id', permission_by_id);
     }
     if (permission_by_id === "DENY") throw new S3Error(S3Error.AccessDenied);
 
     if ((!account_identifier_id || permission_by_id !== "DENY") && account.owner === undefined) {
         permission_by_name = await s3_bucket_policy_utils.has_bucket_policy_permission(
-            s3_policy, account_identifier_name, method, arn_path, req, public_access_block?.restrict_public_buckets
+            s3_policy, account_identifier_name, method, arn_path, req,
+            { disallow_public_access: public_access_block?.restrict_public_buckets }
         );
         dbg.log3('authorize_request_policy: permission_by_name', permission_by_name);
     }
@@ -320,12 +323,12 @@ async function authorize_request_policy(req) {
 
 async function authorize_request_iam_policy(req) {
     const auth_token = req.object_sdk.get_auth_token();
-    const is_anon = !(auth_token && auth_token.access_key);
-    if (is_anon) return;
+    const is_anonymous = !(auth_token && auth_token.access_key);
+    if (is_anonymous) return;
 
     const account = req.object_sdk.requesting_account;
     const is_iam_user = account.owner !== undefined;
-    if (!is_iam_user) return;
+    if (!is_iam_user) return; // IAM policy is only on IAM users (account root user is authorized here)
 
     const resource_arn = _get_arn_from_req_path(req);
     const method = _get_method_from_req(req);
@@ -337,7 +340,8 @@ async function authorize_request_iam_policy(req) {
     for (const iam_policy of iam_policies) {
         // We reusing the bucket policy util function as it is checks the policy document
         const promise = s3_bucket_policy_utils.has_bucket_policy_permission(
-            iam_policy.policy_document, undefined, method, resource_arn, req, undefined, false
+            iam_policy.policy_document, undefined, method, resource_arn, req,
+            { should_pass_principal: false }
         );
         promises.push(promise);
     }
@@ -358,7 +362,9 @@ async function authorize_anonymous_access(s3_policy, method, arn_path, req, publ
     if (!s3_policy) throw new S3Error(S3Error.AccessDenied);
 
     const permission = await s3_bucket_policy_utils.has_bucket_policy_permission(
-        s3_policy, undefined, method, arn_path, req, public_access_block?.restrict_public_buckets);
+        s3_policy, undefined, method, arn_path, req,
+        { disallow_public_access: public_access_block?.restrict_public_buckets }
+    );
     if (permission === "ALLOW") return;
 
     throw new S3Error(S3Error.AccessDenied);

src/sdk/bucketspace_fs.js

@@ -951,7 +951,7 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
             action,
             `arn:aws:s3:::${bucket.name.unwrap()}${bucket_path}`,
             undefined,
-            bucket.public_access_block?.restrict_public_buckets,
+            { disallow_public_access: bucket.public_access_block?.restrict_public_buckets }
         );
         if (permission_by_id === "DENY") return false;
         // we (currently) allow account identified to be both id and name,
@@ -963,7 +963,7 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
                 action,
                 `arn:aws:s3:::${bucket.name.unwrap()}${bucket_path}`,
                 undefined,
-                bucket.public_access_block?.restrict_public_buckets,
+                { disallow_public_access: bucket.public_access_block?.restrict_public_buckets }
             );
         }
         if (permission_by_name === 'DENY') return false;