IAM | Block OBC Accounts From IAM API #9308
Conversation
WalkthroughAdded a runtime guard in IAM REST Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant IAM_Rest as iam_rest.authorize_request
participant AccountSDK as account_sdk
participant Authz as authorization_logic
Client->>IAM_Rest: API request (user-management)
IAM_Rest->>AccountSDK: load requesting_account
AccountSDK-->>IAM_Rest: requesting_account (includes bucket_claim_owner?)
alt bucket_claim_owner present
rect rgb(255,230,230)
IAM_Rest->>IAM_Rest: log error (OBC detected)
IAM_Rest-->>Client: AccessDeniedException (deny)
end
else not present
IAM_Rest->>Authz: proceed with existing authorization
Authz-->>IAM_Rest: authorized / denied
IAM_Rest-->>Client: respond accordingly
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-11-12T04:55:42.193ZApplied to files:
⏰ 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)
🔇 Additional comments (1)
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 |
shirady
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
src/endpoint/iam/iam_rest.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-12T04:55:42.193Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
Applied to files:
src/endpoint/iam/iam_rest.js
⏰ 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-package-lock-validation
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (2)
src/endpoint/iam/iam_rest.js (2)
226-231: Well-placed authorization check for OBC accounts.The placement of this check is correct—it executes after the account is loaded and authorized, ensuring
requesting_accountis available. UsingAccessDeniedExceptionis appropriate for blocking unauthorized operations.
226-231: The current implementation is correct and consistent with codebase patterns.The verification shows that
bucket_claim_owneris never explicitly set tonullanywhere in the codebase. The field either remainsundefined(for regular accounts) or is assigned an ObjectId (for OBC accounts). The current check using!== undefinedis the standard pattern used throughout the codebase for optional account properties (e.g.,requesting_account.owner !== undefinedin account_util.js:517). Additionally, the same field is checked with truthiness in s3_rest.js:258, confirming the field design assumes only undefined or truthy values.The suggestion to use
!= nullis unnecessary and would deviate from established codebase conventions.Likely an incorrect or invalid review comment.
shirady
force-pushed
the
iam-do-not-allow-obc-accounts-create-users
branch
from
0d63d65 to
2e0a8c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/endpoint/iam/iam_rest.js (1)
226-231: Use the idiomatic truthy check for consistency with codebase patterns.The suggestion to replace
bucket_claim_owner !== undefinedwithbucket_claim_owneris well-founded. The codebase consistently uses truthy checks for this field across multiple locations (account_server.js:1027, auth_server.js:545, s3_rest.js:258, and s3_rest.js:271), making the proposed change more consistent and idiomatic.Since
bucket_claim_owneris an ObjectID field that is either undefined (non-OBC accounts) or an ObjectID reference (OBC accounts), a truthy check is both functionally correct and matches the established pattern:- if (req.account_sdk.requesting_account.bucket_claim_owner !== undefined) { + if (req.account_sdk.requesting_account.bucket_claim_owner) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/endpoint/iam/iam_rest.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-12T04:55:42.193Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
Applied to files:
src/endpoint/iam/iam_rest.js
🧬 Code graph analysis (1)
src/endpoint/iam/iam_rest.js (1)
src/endpoint/iam/iam_utils.js (15)
IamError(82-82)IamError(255-255)IamError(481-481)IamError(510-510)IamError(538-538)IamError(545-545)IamError(573-573)IamError(595-595)IamError(624-624)IamError(659-659)IamError(676-676)IamError(691-691)IamError(698-698)IamError(711-711)IamError(715-715)
⏰ 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-package-lock-validation
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
shirady
force-pushed
the
iam-do-not-allow-obc-accounts-create-users
branch
from
2e0a8c5 to
e210292
Compare
Signed-off-by: shirady <[email protected]>
shirady
force-pushed
the
iam-do-not-allow-obc-accounts-create-users
branch
from
e210292 to
eb6e8e3
Compare
2 participants
Describe the Problem
Currently all accounts in the system can perform IAM API, we decided at this point that accounts that are related to the OBC cannot perform IAM API operations (have the property
bucket_claim_ownerin the DB is it the bucket ID of the OBC).Explain the Changes
authorize_request.Issues:
List of GAPs:
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/s3 12443:443kubectl port-forward -n test1 service/iam 14443:443nb obc create shira-obc1 -n test1 --show-secretsalias account-obc-s3='AWS_ACCESS_KEY=<access-key> AWS_SECRET_ACCESS_KEY=<secret-key> aws --no-verify-ssl --endpoint-url https://localhost:12443'alias account-obc-iam='AWS_ACCESS_KEY=<access-key> AWS_SECRET_ACCESS_KEY=<secret-key> aws --no-verify-ssl --endpoint-url https://localhost:14443'account-obc-s3 s3 ls; echo $?(should see the OBC bucket).account-obc-iam iam list-users; echo $?In the logs:
Code changes for testing:
src/sdk/object_sdk.jsuses cache expiry of 1 millisecond.const account_cache = new LRUCache({ name: 'AccountCache', - expiry_ms: config.OBJECT_SDK_ACCOUNT_CACHE_EXPIRY_MS, + expiry_ms: 1, //SDSDNotes:
In step 1 - deploying the system, I used
--use-standalone-dbfor simplicity (fewer steps for the system in Ready status).Doc added/updated
Tests added
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.