IAM | User Inline Policy Document Additional Validation #9274
Conversation
WalkthroughPolicy-name and policy-document validators were moved from Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant IAMController as IAM Controller
participant IAMUtils as iam_utils
Note over Client,IAMController: User policy operations (put/get/delete)
Client->>IAMController: putUserPolicy(request)
IAMController->>IAMUtils: validate_policy_name(policyName)
alt name invalid
IAMUtils-->>IAMController: throw ValidationError (invalid name)
IAMController-->>Client: error
else name valid
IAMController->>IAMUtils: validate_policy_document(policyDocument)
alt document malformed or contains Principal/NotPrincipal
IAMUtils-->>IAMController: throw MalformedPolicyDocument
IAMController-->>Client: error
else document valid
IAMController->>IAMController: persist policy
IAMController-->>Client: success
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (1)src/endpoint/iam/iam_utils.js (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (9)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/endpoint/iam/iam_utils.js (1)
684-693: Add defensive check for Statement field.The function assumes
policy_document.Statementexists and is iterable. If it's missing or not an array, the function will throw an unhelpfulTypeErrorinstead ofMalformedPolicyDocument.Consider adding a check:
function _validate_policy_document_iam_structure(policy_document) { + if (!policy_document.Statement || !Array.isArray(policy_document.Statement)) { + const { code, http_code, type } = IamError.MalformedPolicyDocument; + const message_with_details = 'Policy document must contain a Statement.'; + throw new IamError({ code, message: message_with_details, http_code, type }); + } for (const statement of policy_document.Statement) { const statement_principal = statement.Principal || statement.NotPrincipal; if (statement_principal) { const { code, http_code, type } = IamError.MalformedPolicyDocument; const message_with_details = 'Policy document should not specify a principal.'; throw new IamError({ code, message: message_with_details, http_code, type }); } } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/endpoint/iam/iam_utils.js(6 hunks)src/test/unit_tests/api/iam/test_iam_utils.test.js(5 hunks)src/test/unit_tests/api/iam/test_validation_utils.test.js(0 hunks)src/util/validation_utils.js(1 hunks)
💤 Files with no reviewable changes (1)
- src/test/unit_tests/api/iam/test_validation_utils.test.js
🧰 Additional context used
📓 Path-based instructions (1)
src/test/**/*.*
⚙️ CodeRabbit configuration file
src/test/**/*.*: Ensure that the PR includes tests for the changes.
Files:
src/test/unit_tests/api/iam/test_iam_utils.test.js
🧬 Code graph analysis (3)
src/util/validation_utils.js (1)
src/endpoint/iam/iam_utils.js (4)
_(4-4)require(6-6)require(7-8)require(10-10)
src/test/unit_tests/api/iam/test_iam_utils.test.js (1)
src/endpoint/iam/iam_utils.js (19)
max_length(470-470)max_length(562-562)max_length(614-614)max_length(648-648)iam_constants(9-9)IamError(78-78)IamError(251-251)IamError(477-477)IamError(506-506)IamError(534-534)IamError(541-541)IamError(569-569)IamError(591-591)IamError(620-620)IamError(655-655)IamError(672-672)IamError(688-688)IamError(701-701)IamError(705-705)
src/endpoint/iam/iam_utils.js (3)
src/util/validation_utils.js (15)
require(7-7)min_length(92-92)max_length(93-93)input_length(54-54)input_length(69-69)message_with_details(25-26)message_with_details(56-57)message_with_details(71-73)message_with_details(98-99)message_with_details(105-106)message_with_details(110-111)message_with_details(126-127)message_with_details(161-162)message_with_details(213-214)_(6-6)src/util/string_utils.js (2)
AWS_POLICY_DOCUMENT_REGEXP(20-20)AWS_POLICY_NAME_REGEXP(19-19)src/endpoint/iam/ops/iam_put_user_policy.js (3)
IamError(8-8)IamError(30-30)message_with_details(29-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run-jest-unit-tests
- GitHub Check: Build Noobaa Image
- GitHub Check: run-package-lock-validation
🔇 Additional comments (3)
src/util/validation_utils.js (1)
4-5: LGTM! Helpful clarification.The comment clearly documents the intended scope of this file, which aligns with the PR objective to separate IAM-specific validations from shared utility validations.
src/endpoint/iam/iam_utils.js (2)
641-662: LGTM!The policy name validation logic is correct and follows the established validation pattern used elsewhere in the codebase.
668-676: LGTM!The JSON validation helper is straightforward and handles parse errors appropriately.
shirady
force-pushed
the
iam-user-policy-document-validations
branch
from
758c863 to
8c7df6f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/endpoint/iam/iam_utils.js (1)
597-631: Fix critical logic error in length validation.Line 616 uses OR (
||) instead of AND (&&) in the length check. This condition will always evaluate to true for any number, effectively disabling length validation.Apply this diff:
const min_length = 1; const max_length = 131072; const input_length = input_policy_document.length; - const is_valid_policy_document_length = input_length >= min_length || input_length <= max_length; + const is_valid_policy_document_length = input_length >= min_length && input_length <= max_length; // regex check const is_valid_policy_document = AWS_POLICY_DOCUMENT_REGEXP.test(input_policy_document);src/test/unit_tests/api/iam/test_iam_utils.test.js (2)
705-722: Remove unreachable code from test.Lines 717-718 are unreachable because
validate_policy_documentthrows an error when a Principal field is present. The variableresis never assigned, and these lines will never execute.Apply this diff:
const dummy_policy_document = JSON.stringify({ Version: '2012-10-17', Statement: [{ Effect: 'Allow', Action: 's3:GetBucketLocation', Resource: '*', Principal: '*' // in IAM user inline policies we don't have the principal (user policy is embed to user) }] }); - const res = iam_utils.validate_policy_document(dummy_policy_document, iam_constants.IAM_PARAMETER_NAME.POLICY_DOCUMENT); - expect(res).toBe(true); - throw new NoErrorThrownError(); + iam_utils.validate_policy_document(dummy_policy_document, iam_constants.IAM_PARAMETER_NAME.POLICY_DOCUMENT); + throw new NoErrorThrownError();
724-742: Remove unreachable code from test.Lines 736-737 are unreachable because
validate_policy_documentthrows an error when a NotPrincipal field is present. The variableresis never assigned, and these lines will never execute.Apply this diff:
const dummy_policy_document = JSON.stringify({ Version: '2012-10-17', Statement: [{ Effect: 'Allow', Action: 's3:GetBucketLocation', Resource: '*', NotPrincipal: '*' // in IAM user inline policies we don't have the principal (user policy is embed to user) }] }); - const res = iam_utils.validate_policy_document(dummy_policy_document, iam_constants.IAM_PARAMETER_NAME.POLICY_DOCUMENT); - expect(res).toBe(true); - throw new NoErrorThrownError(); + iam_utils.validate_policy_document(dummy_policy_document, iam_constants.IAM_PARAMETER_NAME.POLICY_DOCUMENT); + throw new NoErrorThrownError();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/endpoint/iam/iam_utils.js(6 hunks)src/test/unit_tests/api/iam/test_iam_utils.test.js(5 hunks)src/test/unit_tests/api/iam/test_validation_utils.test.js(0 hunks)src/util/validation_utils.js(1 hunks)
💤 Files with no reviewable changes (1)
- src/test/unit_tests/api/iam/test_validation_utils.test.js
🧰 Additional context used
📓 Path-based instructions (1)
src/test/**/*.*
⚙️ CodeRabbit configuration file
src/test/**/*.*: Ensure that the PR includes tests for the changes.
Files:
src/test/unit_tests/api/iam/test_iam_utils.test.js
🧬 Code graph analysis (3)
src/test/unit_tests/api/iam/test_iam_utils.test.js (1)
src/endpoint/iam/iam_utils.js (24)
min_length(469-469)min_length(500-500)min_length(561-561)min_length(613-613)min_length(647-647)max_length(470-470)max_length(562-562)max_length(614-614)max_length(648-648)iam_constants(9-9)IamError(78-78)IamError(251-251)IamError(477-477)IamError(506-506)IamError(534-534)IamError(541-541)IamError(569-569)IamError(591-591)IamError(620-620)IamError(655-655)IamError(672-672)IamError(688-688)IamError(701-701)IamError(705-705)
src/util/validation_utils.js (1)
src/endpoint/iam/iam_utils.js (4)
_(4-4)require(6-6)require(7-8)require(10-10)
src/endpoint/iam/iam_utils.js (3)
src/util/validation_utils.js (12)
min_length(92-92)max_length(93-93)message_with_details(25-26)message_with_details(56-57)message_with_details(71-73)message_with_details(98-99)message_with_details(105-106)message_with_details(110-111)message_with_details(126-127)message_with_details(161-162)message_with_details(213-214)_(6-6)src/endpoint/iam/ops/iam_put_user_policy.js (3)
IamError(8-8)IamError(30-30)message_with_details(29-29)src/util/string_utils.js (2)
AWS_POLICY_DOCUMENT_REGEXP(20-20)AWS_POLICY_NAME_REGEXP(19-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
- GitHub Check: run-package-lock-validation
🔇 Additional comments (15)
src/endpoint/iam/iam_utils.js (7)
7-8: LGTM!The import expansion to include
AWS_POLICY_NAME_REGEXPandAWS_POLICY_DOCUMENT_REGEXPis correct and supports the new policy validation functions added below.
402-404: LGTM!The update to use the locally defined
validate_policy_nameandvalidate_policy_documentfunctions is correct and aligns with the PR objective of moving policy validation fromvalidation_utils.jstoiam_utils.js.
419-419: LGTM!The function call correctly uses the relocated
validate_policy_namevalidator.
434-434: LGTM!The function call correctly uses the relocated
validate_policy_namevalidator.
633-662: LGTM!The
validate_policy_namefunction correctly validates type, length, and regex pattern. The error handling is consistent with other validators in the file.
664-676: LGTM!The JSON parsing helper correctly catches parse errors and throws
MalformedPolicyDocumentwith an appropriate message.
821-822: LGTM!The new exports correctly expose the policy validation functions, completing the migration from
validation_utils.js.src/test/unit_tests/api/iam/test_iam_utils.test.js (6)
185-189: LGTM!The test correctly verifies that
validate_iam_pathreturnsundefinedwhen the input is undefined.
316-320: LGTM!The test correctly verifies that
validate_markerreturnsundefinedwhen the input is undefined.
410-414: LGTM!The test correctly verifies that
validate_access_key_idreturnsundefinedwhen the input is undefined.
514-518: LGTM!The test correctly verifies that
validate_statusreturnsundefinedwhen the input is undefined.
550-661: LGTM!The test suite for
validate_policy_nameprovides comprehensive coverage including boundary conditions, valid formats (CamelCase and SnakeCase), invalid characters, length constraints, and type validation.
663-703: LGTM!The test cases for undefined input, valid JSON, invalid JSON, and empty string correctly validate the policy document parsing and validation behavior.
src/util/validation_utils.js (2)
4-5: LGTM!The clarifying comment correctly documents that this file is for shared validation functions between IAM API and NSFS (NC CLI), which aligns with the PR objectives.
7-7: LGTM!The removal of
AWS_POLICY_NAME_REGEXPandAWS_POLICY_DOCUMENT_REGEXPfrom the imports is correct, as these constants are no longer used in this file after migrating policy validation toiam_utils.js.
shirady
changed the title
Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>
shirady
force-pushed
the
iam-user-policy-document-validations
branch
from
ccb5345 to
860a611
Compare
2 participants
Describe the Problem
To have a specific error message for a user inline policy that has a
Principal/NotPrincipalfield in the statement.Explain the Changes
_validate_policy_document_iam_structure.MalformedPolicyDocumentinstead ofValidationError.validation_utils.jstoiam_utils.jsas the intention ofvalidation_utilsis for functions that are mutual to IAM API and manage nsfs (NC CLI), and also added a comment about this.Principal/NotPrincipalfield in the IAM policy statement.Issues:
Testing Instructions:
Unit tests:
Please run:
npx jest test_iam_utils.testnpx jest test_validation_utils.test.jsSummary by CodeRabbit
New Features
Tests