IAM | User Inline Policy Implementation #9270
Conversation
WalkthroughImplements IAM inline user-policy CRUD in AccountSpaceNB, adds account_util policy helpers and pre-deletion resource checks, changes ListUserPolicies response shape to objects with Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AccountSpaceNB
participant AccountUtil as account_util
participant SystemStore as system_store
participant DB as Database
Note over Client,AccountSpaceNB: User inline policy CRUD (high level)
Client->>AccountSpaceNB: put_user_policy(username, name, document)
AccountSpaceNB->>AccountUtil: validate_and_return_requested_account(...)
AccountSpaceNB->>AccountUtil: _check_total_policy_size(...)/_check_user_policy_exists(...)
alt update existing
AccountSpaceNB->>AccountSpaceNB: update iam_user_policies entry
else new
AccountSpaceNB->>AccountSpaceNB: append new policy entry
end
AccountSpaceNB->>SystemStore: make_changes(updated iam_user_policies)
SystemStore->>DB: persist
AccountSpaceNB-->>Client: success
Client->>AccountSpaceNB: list_user_policies(username)
AccountSpaceNB->>AccountSpaceNB: read iam_user_policies
AccountSpaceNB-->>Client: PolicyNames: [{ member: name }, ...], IsTruncated
sequenceDiagram
participant Client
participant AccountSpaceNB
participant AccountUtil as account_util
participant SystemStore as system_store
participant DB as Database
Note over Client,AccountSpaceNB: Pre-deletion resource checks for user/account deletion
Client->>AccountSpaceNB: delete_account(account)
AccountSpaceNB->>AccountUtil: _check_if_user_does_not_have_resources_before_deletion(...)
AccountUtil->>AccountUtil: check access keys, user policies
alt conflict found
AccountUtil-->>AccountSpaceNB: DeleteConflict Error
AccountSpaceNB-->>Client: error
else
AccountSpaceNB->>SystemStore: make_changes(delete)
SystemStore->>DB: persist deletion
AccountSpaceNB-->>Client: success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/endpoint/iam/ops/iam_list_user_policies.js(1 hunks)src/sdk/accountspace_nb.js(3 hunks)src/test/unit_tests/internal/test_accountspace_nb.test.js(0 hunks)src/util/account_util.js(6 hunks)
💤 Files with no reviewable changes (1)
- src/test/unit_tests/internal/test_accountspace_nb.test.js
🧰 Additional context used
🧬 Code graph analysis (3)
src/endpoint/iam/ops/iam_list_user_policies.js (1)
src/endpoint/iam/iam_rest.js (2)
reply(188-188)reply(230-230)
src/sdk/accountspace_nb.js (3)
src/server/system_services/account_server.js (4)
account_util(25-25)account_util(41-41)system_store(17-17)_(5-5)src/endpoint/iam/iam_constants.js (1)
IAM_ACTIONS(5-23)src/util/account_util.js (5)
system_store(12-12)iam_user_policies(616-616)iam_user_policy_index(587-587)iam_user_policy_index(595-596)_(4-4)
src/util/account_util.js (3)
src/sdk/accountspace_nb.js (1)
IamError(9-9)src/server/system_services/account_server.js (1)
account_to_delete(403-403)src/endpoint/iam/iam_constants.js (1)
IAM_ACTIONS(5-23)
⏰ 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-package-lock-validation
- GitHub Check: run-jest-unit-tests
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/util/account_util.js (1)
606-613: Past review concern has been addressed.The undefined
access_keysissue flagged in the past review has been properly fixed at line 608 with the fallback to an empty array.src/sdk/accountspace_nb.js (2)
443-468: Past review concern has been addressed.The implementation now correctly uses
$setat line 464 to update only theiam_user_policiesfield, avoiding full-document replacement.
484-501: Past review concern has been addressed.The implementation now correctly mutates the
iam_user_policiesarray in place at line 491 and persists it with the valid$setoperator at line 497.
🧹 Nitpick comments (1)
src/util/account_util.js (1)
468-468: Refactor string matching to explicit action checks.Using
action.includes('policy')is fragile; future actions with "policy" in the name (e.g.,revoke_policy_version) could unintentionally match. Consider replacing with explicit action checks or a Set-based lookup for maintainability.-if (action === IAM_ACTIONS.LIST_ACCESS_KEYS || action.includes('policy') || action.includes('policies')) { +const POLICY_ACTIONS = new Set([ + IAM_ACTIONS.PUT_USER_POLICY, + IAM_ACTIONS.GET_USER_POLICY, + IAM_ACTIONS.DELETE_USER_POLICY, + IAM_ACTIONS.LIST_USER_POLICIES +]); +if (action === IAM_ACTIONS.LIST_ACCESS_KEYS || POLICY_ACTIONS.has(action)) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/sdk/accountspace_fs.js(1 hunks)src/sdk/accountspace_nb.js(3 hunks)src/util/account_util.js(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/sdk/accountspace_nb.js (2)
src/endpoint/iam/iam_constants.js (1)
IAM_ACTIONS(5-23)src/util/account_util.js (5)
system_store(12-12)iam_user_policies(617-617)iam_user_policy_index(587-587)iam_user_policy_index(595-596)_(4-4)
src/sdk/accountspace_fs.js (2)
src/util/account_util.js (3)
access_keys(283-283)access_keys(608-608)is_access_keys_removed(609-609)src/server/system_services/account_server.js (1)
account_to_delete(403-403)
src/util/account_util.js (2)
src/server/system_services/account_server.js (2)
account_to_delete(403-403)access_keys(157-157)src/endpoint/iam/iam_constants.js (1)
IAM_ACTIONS(5-23)
⏰ 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 (9)
src/util/account_util.js (5)
363-363: LGTM - improved logging practice.Logging only the ID instead of the entire account object reduces log verbosity and avoids potentially leaking sensitive account details.
451-456: LGTM!The new error helper follows established patterns and provides clear, actionable error messages for missing user policies.
482-488: LGTM!The deletion-conflict error helper provides clear feedback about blocking resources and follows the established error-handling pattern.
586-598: LGTM!The policy validation helpers mirror the access-key validation pattern and correctly use
findIndexwith early-exit error handling.
615-622: LGTM!The user-policy pre-deletion check correctly handles undefined
iam_user_policiesand follows the same defensive pattern as the access-keys check.src/sdk/accountspace_fs.js (1)
869-870: LGTM - defensive handling for optional field.The fallback to an empty array prevents runtime errors when
access_keysis undefined while preserving correct behavior when keys exist.src/sdk/accountspace_nb.js (3)
165-165: LGTM - proper pre-deletion validation.Adding the resource check prevents orphaned resources and enforces IAM deletion ordering constraints.
503-515: LGTM!The implementation correctly extracts and sorts policy names, with appropriate handling for undefined
iam_user_policies.
527-527: LGTM - enforces proper authorization.Adding the root-account check here ensures that only root accounts can operate on other users, consistent with IAM access-control requirements.
shirady
force-pushed
the
iam-user-policy-implementations
branch
from
7c36a7d to
98e2cd4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/sdk/accountspace_fs.js(1 hunks)src/sdk/accountspace_nb.js(3 hunks)src/util/account_util.js(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/sdk/accountspace_fs.js
🧰 Additional context used
🧬 Code graph analysis (2)
src/sdk/accountspace_nb.js (2)
src/endpoint/iam/iam_constants.js (1)
IAM_ACTIONS(5-23)src/util/account_util.js (5)
system_store(12-12)iam_user_policies(617-617)iam_user_policy_index(587-587)iam_user_policy_index(595-596)_(4-4)
src/util/account_util.js (2)
src/server/system_services/account_server.js (2)
account_to_delete(403-403)access_keys(157-157)src/endpoint/iam/iam_constants.js (1)
IAM_ACTIONS(5-23)
⏰ 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-package-lock-validation
- GitHub Check: run-jest-unit-tests
Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>
shirady
force-pushed
the
iam-user-policy-implementations
branch
from
60a28d2 to
914f545
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/endpoint/iam/iam_constants.js(3 hunks)src/endpoint/iam/ops/iam_list_user_policies.js(1 hunks)src/sdk/accountspace_fs.js(1 hunks)src/sdk/accountspace_nb.js(3 hunks)src/test/unit_tests/internal/test_accountspace_nb.test.js(0 hunks)src/util/account_util.js(7 hunks)
💤 Files with no reviewable changes (1)
- src/test/unit_tests/internal/test_accountspace_nb.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/endpoint/iam/ops/iam_list_user_policies.js
- src/endpoint/iam/iam_constants.js
🧰 Additional context used
🧬 Code graph analysis (3)
src/sdk/accountspace_fs.js (3)
src/util/account_util.js (3)
access_keys(284-284)access_keys(629-629)is_access_keys_empty(630-630)src/cmd/manage_nsfs.js (1)
access_keys(790-790)src/server/system_services/account_server.js (1)
account_to_delete(403-403)
src/sdk/accountspace_nb.js (1)
src/util/account_util.js (6)
system_store(12-12)iam_user_policies(638-638)iam_user_policy_index(589-589)iam_user_policy_index(597-598)members(570-570)_(4-4)
src/util/account_util.js (1)
src/endpoint/iam/iam_constants.js (3)
IAM_ACTIONS(5-23)IAM_ACTIONS_USER_INLINE_POLICY(25-30)AWS_LIMIT_CHARS_USER_INlINE_POLICY(72-72)
⏰ 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
2 participants
Describe the Problem
Add the implementation for the user inline policy.
Note: At this point, we don't use this policy; it is just stored in the DB.
Explain the Changes
put_user_policy,get_user_policy,delete_user_policy,list_user_policies.iam_list_user_policiesso it will show the members by user policy name.delete_useradd the step_check_if_user_does_not_have_resources_before_deletion- partial copy from accountspace_fs, only the part on IAM users (without the case of root accounts deletion), add there access key and user policy as resources that should be checked.test_accountspace_nb.test.jsas the plan it to add a layer in design.Issues:
list of GAPS:
idwas sent anObjectIdinstead of a string, which would need to track all those cases, as it defects the detailed error message (prints Object object).Testing Instructions:
Note:
nbis an alias that runs the local operator frombuild/_output/bin(alias created bydevenv).kubectl wait --for=condition=available backingstore/noobaa-default-backing-store --timeout=6m -n test1kubectl port-forward -n test1 service/iam 12443:443nb status --show-secrets -n test1and thenalias user-1-iam='AWS_ACCESS_KEY=<access-key> AWS_SECRET_ACCESS_KEY=<secret-key> aws --no-verify-ssl --endpoint-url https://localhost:12443'user-1-iam iam list-users; echo $?user-1-iam iam create-user --user-name RobertNote: To validate user creation, you can rerun
user-1-iam iam list-usersand expect 1 user in the listuser-1-iam iam put-user-policy --user-name Robert --policy-name mypolicy --policy-document file://policy_01.jsonAn example of a policy that was used:
{ "Version": "2012-10-17", "Statement": [ { "Effect": "Allow", "Action": [ "s3:GetBucketLocation" ], "Resource": "*" } ] }user-1-iam iam get-user-policy --user-name Robert --policy-name mypolicyuser-1-iam iam list-user-policies --user-name Robertuser-1-iam iam delete-user-policy --user-name Robert --policy-name mypolicyDoc added/updated
Tests added
Summary by CodeRabbit
New Features
Bug Fixes
Tests