Added support for tagging for IAM users #9260
Conversation
WalkthroughAdds IAM user tagging: new TagUser/UntagUser/ListUserTags actions and MAX_TAGS constant; request parsing and validators; REST handlers and ops for tag/un-tag/list; AccountSDK and AccountSpace (NB and FS) methods; tag regex and validation utilities; tests updated for length-message expectations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant REST as IAM REST
participant Val as iam_utils
participant SDK as AccountSDK
participant Space as AccountSpaceNB
participant Store as SystemStore
Note over Client,REST: TagUser / UntagUser flow (form-encoded)
Client->>REST: POST /?Action=TagUser (form body)
REST->>Val: parse body -> parse_tags_from_request_body / parse_tag_keys_from_request_body
REST->>Val: validate_tagging_params(Action, params)
alt valid
Val-->>REST: ok
REST->>SDK: tag_user(params)
SDK->>Space: tag_user(params, account_sdk)
Space->>Space: auth & ownership checks
Space->>Store: persist tag changes
Store-->>Space: persisted
Space-->>SDK: success
SDK-->>REST: success
REST-->>Client: XML TagUserResponse
else invalid
Val-->>REST: IamError.InvalidInput
REST-->>Client: XML error response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ 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 |
aayushchouhan09
force-pushed
the
tag-usr
branch
3 times, most recently
from
acfb97a to
64c0ab8
Compare
![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/ops/iam_list_user_tags.js (1)
17-17: Consider extracting the magic number 50 to a named constant.While the AWS default for ListUserTags is indeed 50, hardcoding it here creates a potential maintenance issue. Consider adding a constant like
DEFAULT_MAX_ITEMS_LIST_USER_TAGSiniam_constants.jsfor better maintainability and consistency with other IAM list operations.Apply this diff to improve maintainability:
In
src/endpoint/iam/iam_constants.js:const DEFAULT_MAX_ITEMS = 100; +const DEFAULT_MAX_ITEMS_LIST_USER_TAGS = 50;Then in this file:
- max_items: iam_utils.parse_max_items(req.body.max_items) ?? 50, // AWS default for ListUserTags + max_items: iam_utils.parse_max_items(req.body.max_items) ?? iam_constants.DEFAULT_MAX_ITEMS_LIST_USER_TAGS,
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/endpoint/iam/iam_constants.js(2 hunks)src/endpoint/iam/iam_rest.js(2 hunks)src/endpoint/iam/iam_utils.js(3 hunks)src/endpoint/iam/ops/iam_list_user_tags.js(1 hunks)src/endpoint/iam/ops/iam_tag_user.js(1 hunks)src/endpoint/iam/ops/iam_untag_user.js(1 hunks)src/sdk/account_sdk.js(1 hunks)src/sdk/accountspace_nb.js(1 hunks)src/sdk/nb.d.ts(1 hunks)src/util/string_utils.js(2 hunks)src/util/validation_utils.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
src/endpoint/iam/ops/iam_list_user_tags.js (3)
src/endpoint/iam/ops/iam_list_account_aliases.js (3)
iam_utils(5-5)params(14-17)iam_constants(6-6)src/endpoint/iam/ops/iam_list_attached_user_policies.js (3)
iam_utils(5-5)params(15-20)iam_constants(6-6)src/endpoint/iam/iam_utils.js (1)
iam_constants(8-8)
src/sdk/account_sdk.js (3)
src/endpoint/iam/ops/iam_list_user_tags.js (1)
params(14-18)src/endpoint/iam/ops/iam_tag_user.js (1)
params(23-26)src/endpoint/iam/ops/iam_untag_user.js (1)
params(20-23)
src/endpoint/iam/iam_utils.js (4)
src/endpoint/iam/ops/iam_list_user_tags.js (2)
iam_constants(6-6)params(14-18)src/endpoint/iam/ops/iam_tag_user.js (2)
iam_constants(6-6)params(23-26)src/endpoint/iam/ops/iam_untag_user.js (2)
iam_constants(6-6)params(20-23)src/util/validation_utils.js (1)
iam_constants(7-7)
src/util/validation_utils.js (1)
src/util/string_utils.js (6)
i(32-32)i(73-73)i(81-81)i(124-124)i(128-128)AWS_IAM_TAG_KEY_AND_VALUE_REGEXP(18-18)
src/sdk/accountspace_nb.js (6)
src/endpoint/iam/ops/iam_list_user_tags.js (1)
params(14-18)src/endpoint/iam/ops/iam_tag_user.js (2)
params(23-26)tags(14-14)src/endpoint/iam/ops/iam_untag_user.js (1)
params(20-23)src/server/system_services/account_server.js (6)
params(315-315)params(678-678)params(942-942)system_store(17-17)account_util(25-25)account_util(41-41)src/endpoint/iam/iam_constants.js (1)
IAM_ACTIONS(5-19)src/util/account_util.js (4)
system_store(12-12)username(355-355)username(385-386)username(419-420)
src/endpoint/iam/ops/iam_tag_user.js (3)
src/endpoint/iam/ops/iam_list_user_tags.js (2)
iam_utils(5-5)params(14-18)src/endpoint/iam/ops/iam_untag_user.js (3)
iam_utils(5-5)tag_index(15-15)params(20-23)src/util/http_utils.js (1)
CONTENT_TYPE_APP_FORM_URLENCODED(39-39)
src/endpoint/iam/iam_rest.js (2)
src/endpoint/iam/iam_utils.js (3)
require(6-6)require(7-7)require(9-9)src/sdk/accountspace_fs.js (5)
require(9-9)require(11-11)require(12-13)require(17-17)require(19-19)
src/endpoint/iam/ops/iam_untag_user.js (4)
src/endpoint/iam/ops/iam_list_user_tags.js (5)
dbg(4-4)require(7-7)iam_utils(5-5)iam_constants(6-6)params(14-18)src/endpoint/iam/ops/iam_tag_user.js (6)
dbg(4-4)require(7-7)iam_utils(5-5)iam_constants(6-6)tag_index(15-15)params(23-26)src/endpoint/iam/iam_utils.js (4)
require(6-6)require(7-7)require(9-9)iam_constants(8-8)src/util/http_utils.js (1)
CONTENT_TYPE_APP_FORM_URLENCODED(39-39)
⏰ 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). (2)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (13)
src/util/string_utils.js (2)
18-18: LGTM! Unicode regex pattern for IAM tag validation.The regex correctly uses Unicode property escapes (\p{L}, \p{Z}, \p{N}) and the 'u' flag to support international characters in IAM tags, which aligns with AWS IAM tag requirements.
169-169: LGTM! Export added correctly.The new regex constant is properly exported for use in validation utilities.
src/endpoint/iam/ops/iam_list_user_tags.js (1)
20-32: LGTM! Correct implementation of ListUserTags response.The validation, SDK call, and response structure correctly implement the AWS ListUserTags API, including conditional Marker inclusion for pagination.
src/sdk/account_sdk.js (1)
120-137: LGTM! Tag methods follow established patterns.The three new tag-related methods (tag_user, untag_user, list_user_tags) correctly delegate to the accountspace layer and follow the same pattern as existing user methods.
src/endpoint/iam/iam_constants.js (2)
15-18: LGTM! IAM action constants added correctly.The new tagging actions (TAG_USER, UNTAG_USER, LIST_USER_TAGS) follow the existing naming conventions and are properly added to the frozen IAM_ACTIONS object.
35-37: LGTM! Action title mappings added correctly.The mappings from snake_case action names to AWS CamelCase style titles are consistent with existing patterns.
src/endpoint/iam/iam_rest.js (2)
66-67: LGTM! Action mappings for TagUser and UntagUser.The new actions are correctly mapped from AWS CamelCase to internal snake_case format.
84-87: LGTM! User tagging handlers registered correctly.All three tagging operation handlers (tag_user, untag_user, list_user_tags) are properly wired into the IAM_OPS mapping.
src/endpoint/iam/ops/iam_untag_user.js (2)
14-23: LGTM! Tag keys parsing and parameter construction.The implementation correctly parses multiple tag keys from the form-encoded request body and constructs the params object for the untag operation.
25-35: LGTM! Validation and response handling.The validation and account SDK call are correctly implemented, and the response structure follows IAM conventions.
src/sdk/nb.d.ts (1)
904-907: LGTM! TypeScript interface updated correctly.The three new tag-related methods are properly added to the AccountSpace interface with signatures consistent with existing methods.
src/endpoint/iam/ops/iam_tag_user.js (2)
14-26: LGTM! Tag parsing and parameter construction.The implementation correctly parses tag key-value pairs from the form-encoded body, appropriately defaults missing values to empty strings (which AWS allows), and constructs the params object.
28-38: LGTM! Validation and response handling.The validation, account SDK call, and response structure are correctly implemented following IAM conventions.
aayushchouhan09
force-pushed
the
tag-usr
branch
from
64c0ab8 to
4a0e4c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/sdk/accountspace_nb.js (1)
338-355: Avoid mutating cached tag arrays while listing
all_tags.sort(...)mutatesrequested_account.taggingin place becauseall_tagsis a direct reference. That turns this read path into a stateful operation on the cached system-store document, which can surprise concurrent readers/writers or break assumptions about insertion order. Clone before sorting so the listing stays side-effect free.Apply this diff:
- const sorted_tags = all_tags.sort((a, b) => a.key.localeCompare(b.key)); + const sorted_tags = [...all_tags].sort((a, b) => a.key.localeCompare(b.key));
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/endpoint/iam/iam_constants.js(4 hunks)src/endpoint/iam/iam_rest.js(2 hunks)src/endpoint/iam/iam_utils.js(3 hunks)src/endpoint/iam/ops/iam_list_user_tags.js(1 hunks)src/endpoint/iam/ops/iam_tag_user.js(1 hunks)src/endpoint/iam/ops/iam_untag_user.js(1 hunks)src/sdk/account_sdk.js(1 hunks)src/sdk/accountspace_nb.js(2 hunks)src/sdk/nb.d.ts(1 hunks)src/util/string_utils.js(2 hunks)src/util/validation_utils.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/endpoint/iam/ops/iam_tag_user.js
- src/util/validation_utils.js
- src/endpoint/iam/iam_utils.js
- src/sdk/account_sdk.js
- src/sdk/nb.d.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/endpoint/iam/ops/iam_list_user_tags.js (4)
src/endpoint/iam/ops/iam_list_instance_profile_tags.js (3)
iam_utils(5-5)iam_constants(6-6)params(15-19)src/endpoint/iam/ops/iam_list_mfa_device_tags.js (3)
iam_utils(5-5)iam_constants(6-6)params(15-19)src/endpoint/iam/iam_utils.js (1)
iam_constants(8-8)src/endpoint/iam/iam_rest.js (2)
reply(179-179)reply(221-221)
src/sdk/accountspace_nb.js (3)
src/util/account_util.js (7)
require(8-8)require(14-14)require(17-17)require(18-18)username(355-355)username(385-386)username(419-420)src/endpoint/iam/ops/iam_list_user_tags.js (1)
params(14-18)src/endpoint/iam/iam_constants.js (2)
IAM_ACTIONS(5-19)DEFAULT_MAX_TAGS(52-52)
src/endpoint/iam/ops/iam_untag_user.js (3)
src/endpoint/iam/ops/iam_list_user_tags.js (3)
iam_utils(5-5)iam_constants(6-6)params(14-18)src/endpoint/iam/ops/iam_tag_user.js (4)
iam_utils(5-5)iam_constants(6-6)tag_index(15-15)params(23-26)src/util/http_utils.js (1)
CONTENT_TYPE_APP_FORM_URLENCODED(39-39)
⏰ 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
aayushchouhan09
force-pushed
the
tag-usr
branch
from
4a0e4c8 to
b4576f5
Compare
aayushchouhan09
force-pushed
the
tag-usr
branch
from
b4576f5 to
3ec1994
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/sdk/accountspace_nb.js (1)
326-355: Acknowledge TODO: Pagination parameters accepted but not implemented.The method accepts
markerandmax_itemsparameters (passed from the endpoint handler) but does not implement pagination logic. While the TODO comment on line 338 acknowledges this, callers might expect these parameters to work. This is acceptable for a phased rollout, but consider:
- Adding a debug log when pagination params are provided but ignored
- Ensuring the API documentation clearly states pagination is not yet supported
Apply this diff if you want to log when pagination parameters are ignored:
async list_user_tags(params, account_sdk) { const action = IAM_ACTIONS.LIST_USER_TAGS; const requesting_account = system_store.get_account_by_email(account_sdk.requesting_account.email); const username = account_util.populate_username(params.username, requesting_account.name.unwrap()); account_util._check_if_requesting_account_is_root_account(action, requesting_account, { username: params.username }); account_util._check_if_account_exists(action, username); const requested_account = system_store.get_account_by_email(username); account_util._check_if_requested_account_is_root_account_or_IAM_user(action, requesting_account, requested_account); account_util._check_if_requested_is_owned_by_root_account(action, requesting_account, requested_account); + if (params.marker || params.max_items) { + dbg.log1('AccountSpaceNB.list_user_tags: pagination parameters provided but not yet implemented, returning all tags'); + } + // TODO: Pagination not supported - currently returns all tags, ignoring marker and max_items params const all_tags = requested_account.tagging || [];
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/endpoint/iam/iam_constants.js(4 hunks)src/endpoint/iam/iam_rest.js(2 hunks)src/endpoint/iam/iam_utils.js(4 hunks)src/endpoint/iam/ops/iam_list_user_tags.js(1 hunks)src/endpoint/iam/ops/iam_tag_user.js(1 hunks)src/endpoint/iam/ops/iam_untag_user.js(1 hunks)src/sdk/account_sdk.js(1 hunks)src/sdk/accountspace_nb.js(2 hunks)src/sdk/nb.d.ts(1 hunks)src/util/string_utils.js(2 hunks)src/util/validation_utils.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/sdk/nb.d.ts
- src/endpoint/iam/ops/iam_tag_user.js
- src/endpoint/iam/ops/iam_untag_user.js
- src/sdk/account_sdk.js
- src/util/validation_utils.js
- src/endpoint/iam/ops/iam_list_user_tags.js
- src/endpoint/iam/iam_constants.js
🧰 Additional context used
🧬 Code graph analysis (2)
src/sdk/accountspace_nb.js (5)
src/util/account_util.js (14)
require(8-8)require(14-14)require(17-17)require(18-18)system_store(12-12)username(355-355)username(385-386)username(419-420)IamError(15-15)IamError(320-320)IamError(360-360)IamError(389-389)IamError(426-426)IamError(450-450)src/endpoint/iam/ops/iam_list_user_tags.js (1)
params(14-18)src/endpoint/iam/ops/iam_tag_user.js (1)
params(14-17)src/endpoint/iam/ops/iam_untag_user.js (1)
params(14-17)src/endpoint/iam/iam_constants.js (2)
IAM_ACTIONS(5-23)DEFAULT_MAX_TAGS(60-60)
src/endpoint/iam/iam_utils.js (3)
src/endpoint/iam/ops/iam_list_user_tags.js (2)
params(14-18)iam_constants(6-6)src/endpoint/iam/ops/iam_tag_user.js (2)
params(14-17)iam_constants(6-6)src/endpoint/iam/ops/iam_untag_user.js (2)
params(14-17)iam_constants(6-6)
⏰ 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 (12)
src/util/string_utils.js (2)
18-18: LGTM: Regex pattern correctly implements AWS IAM tag validation.The Unicode flag (
/u) is required for the\p{}property escapes to function correctly, enabling proper validation of international characters in tag keys and values per AWS specifications.
171-171: LGTM: Export follows existing conventions.The constant is properly exported alongside other regex patterns in the module.
src/sdk/accountspace_nb.js (3)
12-12: LGTM: Import is necessary and correctly structured.The
DEFAULT_MAX_TAGSconstant is properly imported and used for enforcing the AWS 50-tag limit.
254-295: LGTM: Tag merging and limit enforcement correctly implemented.The method properly:
- Validates requester permissions and target account
- Merges existing and new tags using a Map (ensuring key uniqueness)
- Enforces the AWS 50-tag limit after merging (addresses previous review feedback)
- Persists changes and logs the operation
The implementation correctly handles the edge case where merging could exceed the limit, preventing invalid states.
297-324: LGTM: Efficient tag removal implementation.The method efficiently removes tags using a Set for O(1) lookup performance. The implementation correctly handles non-existent tag keys by silently ignoring them, matching AWS behavior.
src/endpoint/iam/iam_rest.js (2)
38-40: LGTM: Action mappings follow existing conventions.The three new IAM tagging actions are correctly registered with appropriate snake_case mappings.
88-91: LGTM: Operation handlers properly registered and well-organized.The handlers are appropriately placed after access key operations with a clear comment, improving code organization as suggested in previous reviews.
src/endpoint/iam/iam_utils.js (5)
84-101: LGTM: Routing logic correctly handles overlapping action names.The order-dependent routing (checking 'tag' before 'user') correctly handles actions like 'tag_user' that contain multiple keywords. The comment on line 85 clearly documents this requirement, which helps future maintainers.
While explicit action matching would be more robust, the current approach is acceptable given IAM's consistent naming conventions.
157-176: LGTM: Tagging validation router follows established patterns.The
validate_tagging_paramsfunction consistently follows the same switch-based routing pattern used byvalidate_user_paramsandvalidate_access_keys_params, maintaining code consistency.
608-649: LGTM: Well-designed parsing utilities for AWS-style indexed parameters.The
parse_indexed_membersfunction is elegantly generic and reusable. The specific tag parsers correctly handle:
- Key-value pairs with optional values (defaulting to empty string per AWS semantics)
- Tag key arrays for untag operations
The implementation is clean and maintainable.
651-692: LGTM: Tag validators correctly implement requested validation order.All three validators properly implement the requested validation sequence:
- Require username (addressing previous review feedback)
- Validate username format
- Validate operation-specific parameters (tags, tag_keys, or pagination)
The consistent pattern across all three validators improves maintainability.
Based on previous review comments.
706-711: LGTM: New utilities and validators properly exported.All new parsing and validation functions are correctly exported, making them available to the IAM operation handlers.
aayushchouhan09
force-pushed
the
tag-usr
branch
from
3ec1994 to
3fe58b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/sdk/nb.d.ts (1)
910-913: Consider more specific TypeScript types for better type safety.While the current generic types are consistent with the rest of the interface, you could improve type safety with more specific signatures:
tag_user(params: { username: string; tags: Array<{ key: string; value: string }> }, account_sdk: AccountSDK): Promise<void>; untag_user(params: { username: string; tag_keys: string[] }, account_sdk: AccountSDK): Promise<void>; list_user_tags(params: { username: string; marker?: string; max_items?: number }, account_sdk: AccountSDK): Promise<{ tags: Array<{ key: string; value: string }>; is_truncated: boolean }>;This would provide better IDE autocomplete and catch type errors at compile time.
src/sdk/accountspace_nb.js (1)
419-419: Clarify the TODO comment.The TODO mentions pagination is not supported, but the method signature accepts
markerandmax_itemsparameters that are silently ignored. Consider making the TODO more explicit about this:- // TODO: Pagination not supported - currently returns all tags, ignoring marker and max_items params + // TODO: Pagination not implemented - marker and max_items params are currently ignored; all tags are returnedThis helps future maintainers understand that the parameters exist but are deliberately not used yet.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/endpoint/iam/iam_constants.js(4 hunks)src/endpoint/iam/iam_rest.js(2 hunks)src/endpoint/iam/iam_utils.js(4 hunks)src/endpoint/iam/ops/iam_list_user_tags.js(1 hunks)src/endpoint/iam/ops/iam_tag_user.js(1 hunks)src/endpoint/iam/ops/iam_untag_user.js(1 hunks)src/sdk/account_sdk.js(1 hunks)src/sdk/accountspace_nb.js(2 hunks)src/sdk/nb.d.ts(1 hunks)src/util/string_utils.js(2 hunks)src/util/validation_utils.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/util/validation_utils.js
🧰 Additional context used
🧬 Code graph analysis (8)
src/sdk/nb.d.ts (3)
src/endpoint/iam/ops/iam_list_user_tags.js (1)
params(14-18)src/endpoint/iam/ops/iam_tag_user.js (1)
params(17-20)src/endpoint/iam/ops/iam_untag_user.js (1)
params(17-20)
src/endpoint/iam/ops/iam_tag_user.js (5)
src/endpoint/iam/ops/iam_list_user_tags.js (5)
dbg(4-4)require(7-7)iam_utils(5-5)iam_constants(6-6)params(14-18)src/endpoint/iam/ops/iam_untag_user.js (5)
dbg(4-4)require(7-7)iam_utils(5-5)iam_constants(6-6)params(17-20)src/sdk/accountspace_nb.js (3)
dbg(8-8)require(11-12)iam_utils(7-7)src/endpoint/iam/iam_utils.js (4)
require(6-6)require(7-7)require(9-9)iam_constants(8-8)src/util/http_utils.js (1)
CONTENT_TYPE_APP_FORM_URLENCODED(39-39)
src/sdk/accountspace_nb.js (5)
src/util/account_util.js (7)
require(8-8)require(14-14)require(16-16)require(17-18)username(370-370)username(400-401)username(434-435)src/endpoint/iam/ops/iam_list_user_tags.js (1)
params(14-18)src/endpoint/iam/ops/iam_tag_user.js (1)
params(17-20)src/endpoint/iam/ops/iam_untag_user.js (1)
params(17-20)src/endpoint/iam/iam_constants.js (2)
IAM_ACTIONS(5-23)DEFAULT_MAX_TAGS(60-60)
src/endpoint/iam/ops/iam_untag_user.js (1)
src/util/http_utils.js (1)
CONTENT_TYPE_APP_FORM_URLENCODED(39-39)
src/endpoint/iam/iam_utils.js (3)
src/endpoint/iam/ops/iam_list_user_tags.js (2)
params(14-18)iam_constants(6-6)src/endpoint/iam/ops/iam_tag_user.js (2)
params(17-20)iam_constants(6-6)src/endpoint/iam/ops/iam_untag_user.js (2)
params(17-20)iam_constants(6-6)
src/endpoint/iam/ops/iam_list_user_tags.js (3)
src/endpoint/iam/ops/iam_list_user_policies.js (4)
iam_utils(5-5)iam_constants(6-6)params(14-18)reply(22-22)src/endpoint/iam/ops/iam_list_instance_profile_tags.js (3)
iam_utils(5-5)iam_constants(6-6)params(15-19)src/endpoint/iam/ops/iam_list_mfa_device_tags.js (3)
iam_utils(5-5)iam_constants(6-6)params(15-19)
src/sdk/account_sdk.js (3)
src/endpoint/iam/ops/iam_list_user_tags.js (1)
params(14-18)src/endpoint/iam/ops/iam_tag_user.js (1)
params(17-20)src/endpoint/iam/ops/iam_untag_user.js (1)
params(17-20)
src/endpoint/iam/iam_rest.js (1)
src/sdk/accountspace_fs.js (5)
require(9-9)require(11-11)require(12-13)require(17-17)require(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-package-lock-validation
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (19)
src/endpoint/iam/ops/iam_tag_user.js (2)
1-11: LGTM!The imports and API documentation reference are properly structured.
26-43: LGTM!The response structure follows IAM conventions and the module exports are correctly configured for form-encoded requests with XML replies.
src/sdk/account_sdk.js (1)
120-137: LGTM!The three new tagging methods follow the established delegation pattern used throughout this class. The implementation is consistent and correct.
src/endpoint/iam/ops/iam_list_user_tags.js (2)
14-23: LGTM!The refactoring from
get_usertolist_user_tagsis correct, and usingDEFAULT_MAX_TAGS(50) instead ofDEFAULT_MAX_ITEMS(100) is appropriate for tag-specific operations. The debug logging is also helpful.
25-36: LGTM!The response now correctly uses
reply.tagsandreply.is_truncateddirectly without defaulting values, which addresses the previous review feedback.src/endpoint/iam/ops/iam_untag_user.js (2)
1-24: LGTM!The implementation is consistent with
tag_user.jsand follows the established pattern. The comments clearly explain the AWS encoding transformation for tag keys.
26-43: LGTM!The response structure and module exports are correctly configured.
src/endpoint/iam/iam_constants.js (1)
16-18: LGTM!The new constants are correctly defined and follow the established naming conventions. The
DEFAULT_MAX_TAGSvalue of 50 aligns with AWS IAM's actual limit of 50 tags per user resource.Also applies to: 39-41, 60-60, 86-86
src/endpoint/iam/iam_rest.js (1)
38-40: LGTM!The tagging actions and handlers are correctly registered and positioned after access keys as suggested in the previous review. The action mappings follow the established naming conventions.
Also applies to: 88-91
src/util/string_utils.js (1)
18-18: Regex pattern correctly matches AWS IAM tag character requirements.The pattern aligns with AWS documentation: allowed characters are letters (A–Z, a–z), numbers (0–9), space, and _ . : / = + - @. The Unicode property escapes (
\p{L},\p{Z},\p{N}) correctly represent letters, spaces, and numbers respectively, and the/uflag is necessary for this syntax.Note: The pattern requires at least one character; verify this is intentional if the regex applies to tag values (which AWS allows to be empty).
src/sdk/accountspace_nb.js (2)
335-376: LGTM! Past review concern has been addressed.The 50-tag limit is now correctly enforced after merging existing and new tags (lines 357-362), preventing users from exceeding AWS limits. The Map-based deduplication approach is efficient and matches AWS behavior where new tag values overwrite existing ones for the same key.
378-405: LGTM! Efficient implementation.The use of Set for tag key lookup (line 392) provides O(1) removal checks, and the silent handling of non-existent keys aligns with AWS IAM behavior.
src/endpoint/iam/iam_utils.js (7)
84-101: LGTM! Proper routing for tag actions.The updated comment (lines 84-85) clearly explains why order matters, and checking for 'tag' before 'user' (line 90) correctly prevents
tag_userfrom being misrouted to user validation. This ensures tagging actions are dispatched to the newvalidate_tagging_paramsfunction.
157-176: LGTM! Consistent dispatcher pattern.The
validate_tagging_paramsfunction follows the same switch-based dispatch pattern used by other validators in this file, providing consistent error handling and routing for all three tagging actions.
608-626: LGTM! Reusable generic parser.The
parse_indexed_membersfunction is well-designed with optional mapper support, making it reusable for various AWS-style indexed parameters. Starting the index at 1 (line 617) correctly matches AWS conventions.
628-645: LGTM! Correct tag parsing.The function properly handles AWS's indexed tag format and defaults missing values to empty strings (line 642), which aligns with AWS IAM behavior where tag values are optional. The JSDoc examples are helpful for understanding the transformation.
647-657: LGTM! Straightforward key parsing.Clean implementation leveraging
parse_indexed_membersfor the simpler case of extracting just tag keys without values.
659-700: LGTM! Validation order addresses past review feedback.All three tag validators now follow the requested order from past review comments:
- Required username check first (lines 665, 679, 693)
- Username validation second (lines 666, 680, 694)
- Tag-specific validation last (lines 667, 681, 695-696)
This ensures proper error messages and aligns with AWS validation semantics.
714-719: LGTM! All new functions properly exported.All six new parsing and validation functions are correctly exported, making them available to the IAM operation handlers.
aayushchouhan09
force-pushed
the
tag-usr
branch
from
3fe58b7 to
d3a0e24
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/util/validation_utils.js (1)
194-208: Pattern validation works but error message is slightly misleading for empty values.The function displays different regex patterns for keys (with
+) vs values (with*) in error messages (line 201), but always validates usingAWS_IAM_TAG_KEY_AND_VALUE_REGEXPwhich has+(line 202). This works in practice because empty values skip pattern validation at line 255 invalidate_iam_tags, but the error message could confuse developers.Consider this clarification:
function _validate_tag_pattern(value, position, is_value = false) { - const pattern = is_value ? '[\\p{L}\\p{Z}\\p{N}_.:/=+\\-@]*' : '[\\p{L}\\p{Z}\\p{N}_.:/=+\\-@]+'; + // Note: Both keys and non-empty values use the same regex (with +) + // Empty values are not validated by this function + const pattern = '[\\p{L}\\p{Z}\\p{N}_.:/=+\\-@]+'; const is_valid = AWS_IAM_TAG_KEY_AND_VALUE_REGEXP.test(value);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/endpoint/iam/iam_constants.js(4 hunks)src/endpoint/iam/iam_rest.js(2 hunks)src/endpoint/iam/iam_utils.js(4 hunks)src/endpoint/iam/ops/iam_list_user_tags.js(1 hunks)src/endpoint/iam/ops/iam_tag_user.js(1 hunks)src/endpoint/iam/ops/iam_untag_user.js(1 hunks)src/sdk/account_sdk.js(1 hunks)src/sdk/accountspace_nb.js(2 hunks)src/sdk/nb.d.ts(1 hunks)src/util/string_utils.js(2 hunks)src/util/validation_utils.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/endpoint/iam/ops/iam_tag_user.js
- src/endpoint/iam/iam_rest.js
🧰 Additional context used
🧬 Code graph analysis (7)
src/endpoint/iam/ops/iam_untag_user.js (5)
src/endpoint/iam/ops/iam_list_user_tags.js (4)
require(7-7)iam_utils(5-5)iam_constants(6-6)params(14-18)src/endpoint/iam/ops/iam_tag_user.js (4)
require(7-7)iam_utils(5-5)iam_constants(6-6)params(17-20)src/sdk/accountspace_nb.js (2)
require(11-12)iam_utils(7-7)src/endpoint/iam/iam_utils.js (4)
require(6-6)require(7-7)require(9-9)iam_constants(8-8)src/util/http_utils.js (1)
CONTENT_TYPE_APP_FORM_URLENCODED(39-39)
src/sdk/account_sdk.js (3)
src/endpoint/iam/ops/iam_list_user_tags.js (1)
params(14-18)src/endpoint/iam/ops/iam_tag_user.js (1)
params(17-20)src/endpoint/iam/ops/iam_untag_user.js (1)
params(17-20)
src/endpoint/iam/iam_utils.js (3)
src/endpoint/iam/ops/iam_list_user_tags.js (2)
params(14-18)iam_constants(6-6)src/endpoint/iam/ops/iam_tag_user.js (2)
params(17-20)iam_constants(6-6)src/endpoint/iam/ops/iam_untag_user.js (2)
params(17-20)iam_constants(6-6)
src/util/validation_utils.js (1)
src/util/string_utils.js (6)
AWS_IAM_TAG_KEY_AND_VALUE_REGEXP(18-18)i(34-34)i(75-75)i(83-83)i(126-126)i(130-130)
src/sdk/nb.d.ts (3)
src/endpoint/iam/ops/iam_list_user_tags.js (1)
params(14-18)src/endpoint/iam/ops/iam_tag_user.js (1)
params(17-20)src/endpoint/iam/ops/iam_untag_user.js (1)
params(17-20)
src/sdk/accountspace_nb.js (4)
src/endpoint/iam/ops/iam_list_user_tags.js (1)
params(14-18)src/endpoint/iam/ops/iam_tag_user.js (1)
params(17-20)src/endpoint/iam/ops/iam_untag_user.js (1)
params(17-20)src/endpoint/iam/iam_constants.js (2)
IAM_ACTIONS(5-23)DEFAULT_MAX_TAGS(60-60)
src/endpoint/iam/ops/iam_list_user_tags.js (2)
src/endpoint/iam/ops/iam_list_user_policies.js (4)
iam_utils(5-5)iam_constants(6-6)params(14-18)reply(22-22)src/endpoint/iam/ops/iam_list_instance_profile_tags.js (3)
iam_utils(5-5)iam_constants(6-6)params(15-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: run-package-lock-validation
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (16)
src/endpoint/iam/iam_constants.js (1)
16-18: LGTM! Clean addition of IAM tagging constants.The new IAM actions, message title mappings, and DEFAULT_MAX_TAGS constant align with AWS IAM tagging conventions and are properly exported.
Also applies to: 39-41, 60-60, 86-86
src/endpoint/iam/ops/iam_untag_user.js (1)
1-43: LGTM! Follows established IAM operation patterns.The implementation is consistent with other IAM operations in structure and validation approach.
src/sdk/nb.d.ts (1)
910-913: LGTM! Type definitions properly extended.The new AccountSpace methods are correctly positioned after access key methods and follow the existing interface patterns.
src/sdk/account_sdk.js (1)
120-137: LGTM! Clean delegation pattern.The new tag-related methods properly delegate to the accountspace layer, maintaining consistency with the existing SDK structure.
src/endpoint/iam/ops/iam_list_user_tags.js (1)
17-17: LGTM! Correctly delegates to backend list_user_tags.The handler now properly calls the dedicated list_user_tags method and uses DEFAULT_MAX_TAGS for the tagging context, with response fields correctly mapped from the backend reply.
Also applies to: 20-23, 28-29
src/util/string_utils.js (1)
18-18: LGTM! Regex pattern correctly implements AWS IAM tag validation.The Unicode flag
/uis necessary for the\p{L},\p{Z}, and\p{N}property escapes to work correctly, and the character set matches AWS IAM tag key/value requirements.Also applies to: 171-171
src/util/validation_utils.js (3)
173-192: LGTM! Helper function with clear AWS-style error messages.The length constraint validation provides informative error messages that match AWS IAM API conventions.
219-261: Good validation logic with proper null/array guard.The null and array guard at lines 241-245 correctly addresses the concern from previous reviews about preventing raw TypeError on property access.
272-292: LGTM! Tag keys validation is correctly implemented.The validation properly allows empty arrays (per AWS behavior) and enforces all constraints on non-empty tag key arrays.
src/sdk/accountspace_nb.js (3)
335-376: LGTM! Tag limit enforcement correctly addresses previous review concern.The 50-tag limit check at lines 358-362 properly validates the merged tag count, preventing the system from storing more tags than AWS allows.
378-405: LGTM! Untag implementation is clean and correct.The filtering logic using a Set for efficient lookups is appropriate, and the validation follows established patterns.
407-436: Verify the response structure's 'member' wrapper.The response at lines 423-428 wraps each tag in a
memberobject. Typically, AWS IAM API responses have tags as{ Key, Value }objects directly, with thememberwrapper added during XML serialization by the framework. Verify that the XML serializer isn't adding a second level ofmemberwrapping.Expected structure sent to XML serializer:
tags: [ { Key: 'env', Value: 'prod' }, { Key: 'team', Value: 'platform' } ]Current structure:
tags: [ { member: { Key: 'env', Value: 'prod' } }, { member: { Key: 'team', Value: 'platform' } } ]If the XML serializer automatically wraps array elements with
member, remove the explicit wrapper:- const tags = sorted_tags.length > 0 ? sorted_tags.map(tag => ({ - member: { - Key: tag.key, - Value: tag.value - } - })) : []; + const tags = sorted_tags.map(tag => ({ + Key: tag.key, + Value: tag.value + }));src/endpoint/iam/iam_utils.js (4)
84-92: LGTM: Correct routing order for tagging actions.The updated routing logic correctly checks for 'tag' before 'user' to avoid misrouting actions like
TAG_USERthat contain both substrings. The inline comment (line 85) clearly documents this critical ordering requirement.
157-176: LGTM: Clean dispatcher for tagging validations.The
validate_tagging_paramsfunction follows the established pattern of other validation dispatchers in this file (e.g.,validate_user_params,validate_policy_params), maintaining consistency across the codebase.
608-657: LGTM: Well-designed parsing utilities for AWS-style indexed parameters.The three parsing functions correctly implement AWS conventions:
parse_indexed_membersuses 1-based indexing and stops at the first missing index, which matches AWS behavior for consecutive parameter arrays.parse_tags_from_request_bodyappropriately defaults missing tag values to empty strings (line 642), which is valid per AWS IAM tag semantics.- Clear JSDoc examples document the expected input/output formats.
659-700: LGTM: Validation functions correctly address past review feedback.All three validation functions (
validate_tag_user_params,validate_untag_user_params,validate_list_user_tags_params) now follow the correct order requested in past review comments:
check_required_usernameenforces the required parametervalidate_usernamevalidates format/constraints- Tag-specific validations complete the checks
This matches AWS semantics where
UserNameis required for all three tag operations, and aligns with the validation patterns used throughout this file.
aayushchouhan09
force-pushed
the
tag-usr
branch
from
d3a0e24 to
5a4de3a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/endpoint/iam/iam_constants.js(4 hunks)src/endpoint/iam/iam_rest.js(3 hunks)src/endpoint/iam/iam_utils.js(4 hunks)src/endpoint/iam/ops/iam_list_user_tags.js(1 hunks)src/endpoint/iam/ops/iam_tag_user.js(1 hunks)src/endpoint/iam/ops/iam_untag_user.js(1 hunks)src/sdk/account_sdk.js(1 hunks)src/sdk/accountspace_nb.js(2 hunks)src/sdk/nb.d.ts(1 hunks)src/util/string_utils.js(2 hunks)src/util/validation_utils.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/util/string_utils.js
- src/endpoint/iam/iam_rest.js
- src/endpoint/iam/ops/iam_tag_user.js
- src/endpoint/iam/ops/iam_untag_user.js
🧰 Additional context used
🧬 Code graph analysis (6)
src/endpoint/iam/ops/iam_list_user_tags.js (5)
src/endpoint/iam/ops/iam_tag_user.js (3)
iam_utils(5-5)iam_constants(6-6)params(17-20)src/endpoint/iam/ops/iam_untag_user.js (3)
iam_utils(5-5)iam_constants(6-6)params(17-20)src/endpoint/iam/ops/iam_list_user_policies.js (4)
iam_utils(5-5)iam_constants(6-6)params(14-18)reply(22-22)src/endpoint/iam/ops/iam_list_instance_profile_tags.js (3)
iam_utils(5-5)iam_constants(6-6)params(15-19)src/endpoint/iam/iam_rest.js (2)
reply(188-188)reply(230-230)
src/sdk/account_sdk.js (3)
src/endpoint/iam/ops/iam_list_user_tags.js (1)
params(14-18)src/endpoint/iam/ops/iam_tag_user.js (1)
params(17-20)src/endpoint/iam/ops/iam_untag_user.js (1)
params(17-20)
src/endpoint/iam/iam_utils.js (3)
src/endpoint/iam/ops/iam_list_user_tags.js (2)
params(14-18)iam_constants(6-6)src/endpoint/iam/ops/iam_tag_user.js (2)
params(17-20)iam_constants(6-6)src/endpoint/iam/ops/iam_untag_user.js (2)
params(17-20)iam_constants(6-6)
src/sdk/accountspace_nb.js (4)
src/endpoint/iam/ops/iam_list_user_tags.js (1)
params(14-18)src/endpoint/iam/ops/iam_tag_user.js (1)
params(17-20)src/endpoint/iam/ops/iam_untag_user.js (1)
params(17-20)src/endpoint/iam/iam_constants.js (2)
IAM_ACTIONS(5-23)DEFAULT_MAX_TAGS(60-60)
src/util/validation_utils.js (1)
src/util/string_utils.js (6)
AWS_IAM_TAG_KEY_AND_VALUE_REGEXP(18-18)i(34-34)i(75-75)i(83-83)i(126-126)i(130-130)
src/sdk/nb.d.ts (3)
src/endpoint/iam/ops/iam_list_user_tags.js (1)
params(14-18)src/endpoint/iam/ops/iam_tag_user.js (1)
params(17-20)src/endpoint/iam/ops/iam_untag_user.js (1)
params(17-20)
⏰ 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
aayushchouhan09
force-pushed
the
tag-usr
branch
from
5a4de3a to
65c9694
Compare
aayushchouhan09
force-pushed
the
tag-usr
branch
from
65c9694 to
6d3e2fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/sdk/accountspace_nb.js (1)
419-435: Honor MaxItems/Marker in ListUserTagsListUserTags has to respect the caller’s
max_itemsandmarker. Right now we ignore both, always return every tag, and never setis_truncated, which breaks AWS pagination semantics (e.g., the CLI’s--max-itemsflag). Please slice the sorted list according to the requested limit (capped atDEFAULT_MAX_TAGS), advance from the marker when provided, computeis_truncated, and surface the next marker when more data remains.- // TODO: Pagination not supported - currently returns all tags, ignoring marker and max_items params - const all_tags = requested_account.tagging || []; - const sorted_tags = all_tags.sort((a, b) => a.key.localeCompare(b.key)); - - const tags = sorted_tags.length > 0 ? sorted_tags.map(tag => ({ + const all_tags = requested_account.tagging || []; + const sorted_tags = all_tags.slice().sort((a, b) => a.key.localeCompare(b.key)); + + const limit = Math.min(params.max_items ?? DEFAULT_MAX_TAGS, DEFAULT_MAX_TAGS); + let start_index = 0; + if (params.marker) { + const marker_index = sorted_tags.findIndex(tag => tag.key === params.marker); + if (marker_index >= 0) start_index = marker_index + 1; + } + + const page = sorted_tags.slice(start_index, start_index + limit); + const is_truncated = start_index + limit < sorted_tags.length; + const last_tag = page[page.length - 1]; + const next_marker = is_truncated && last_tag ? last_tag.key : undefined; + + const tags = page.length > 0 ? page.map(tag => ({ member: { Key: tag.key, Value: tag.value } })) : []; - dbg.log1('AccountSpaceNB.list_user_tags: returning', tags.length, 'tags for user', params.username); - - return { - tags: tags, - is_truncated: false - }; + dbg.log1('AccountSpaceNB.list_user_tags: returning', tags.length, 'tags for user', params.username, + 'requested marker', params.marker, 'is_truncated', is_truncated); + + return { + tags, + is_truncated, + marker: next_marker + };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/endpoint/iam/iam_constants.js(4 hunks)src/endpoint/iam/iam_rest.js(3 hunks)src/endpoint/iam/iam_utils.js(4 hunks)src/endpoint/iam/ops/iam_list_user_tags.js(1 hunks)src/endpoint/iam/ops/iam_tag_user.js(1 hunks)src/endpoint/iam/ops/iam_untag_user.js(1 hunks)src/sdk/account_sdk.js(1 hunks)src/sdk/accountspace_nb.js(2 hunks)src/sdk/nb.d.ts(1 hunks)src/util/string_utils.js(2 hunks)src/util/validation_utils.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/endpoint/iam/iam_constants.js
- src/endpoint/iam/ops/iam_list_user_tags.js
- src/util/string_utils.js
🧰 Additional context used
🧬 Code graph analysis (8)
src/sdk/account_sdk.js (3)
src/endpoint/iam/ops/iam_list_user_tags.js (1)
params(14-18)src/endpoint/iam/ops/iam_tag_user.js (1)
params(17-20)src/endpoint/iam/ops/iam_untag_user.js (1)
params(17-20)
src/util/validation_utils.js (1)
src/util/string_utils.js (6)
AWS_IAM_TAG_KEY_AND_VALUE_REGEXP(18-18)i(34-34)i(75-75)i(83-83)i(126-126)i(130-130)
src/sdk/nb.d.ts (3)
src/endpoint/iam/ops/iam_list_user_tags.js (1)
params(14-18)src/endpoint/iam/ops/iam_tag_user.js (1)
params(17-20)src/endpoint/iam/ops/iam_untag_user.js (1)
params(17-20)
src/sdk/accountspace_nb.js (5)
src/util/account_util.js (23)
require(8-8)require(14-14)require(16-16)require(17-18)system_store(12-12)username(370-370)username(400-401)username(434-435)message_with_details(334-334)message_with_details(374-374)message_with_details(403-403)message_with_details(440-440)message_with_details(447-447)message_with_details(459-459)message_with_details(482-482)IamError(15-15)IamError(335-335)IamError(375-375)IamError(404-404)IamError(441-441)IamError(448-448)IamError(472-472)IamError(483-483)src/endpoint/iam/ops/iam_list_user_tags.js (1)
params(14-18)src/endpoint/iam/ops/iam_tag_user.js (1)
params(17-20)src/endpoint/iam/ops/iam_untag_user.js (1)
params(17-20)src/endpoint/iam/iam_constants.js (2)
IAM_ACTIONS(5-23)DEFAULT_MAX_TAGS(60-60)
src/endpoint/iam/iam_rest.js (1)
src/endpoint/iam/iam_utils.js (13)
IamError(77-77)IamError(250-250)IamError(476-476)IamError(505-505)IamError(533-533)IamError(540-540)IamError(568-568)IamError(590-590)IamError(602-602)IamError(606-606)require(6-6)require(7-7)require(9-9)
src/endpoint/iam/iam_utils.js (3)
src/endpoint/iam/ops/iam_list_user_tags.js (2)
params(14-18)iam_constants(6-6)src/endpoint/iam/ops/iam_tag_user.js (2)
params(17-20)iam_constants(6-6)src/endpoint/iam/ops/iam_untag_user.js (2)
params(17-20)iam_constants(6-6)
src/endpoint/iam/ops/iam_tag_user.js (2)
src/endpoint/iam/iam_utils.js (1)
iam_constants(8-8)src/util/http_utils.js (1)
CONTENT_TYPE_APP_FORM_URLENCODED(39-39)
src/endpoint/iam/ops/iam_untag_user.js (4)
src/endpoint/iam/ops/iam_list_user_tags.js (5)
dbg(4-4)require(7-7)iam_utils(5-5)iam_constants(6-6)params(14-18)src/endpoint/iam/ops/iam_tag_user.js (5)
dbg(4-4)require(7-7)iam_utils(5-5)iam_constants(6-6)params(17-20)src/endpoint/iam/iam_utils.js (4)
require(6-6)require(7-7)require(9-9)iam_constants(8-8)src/util/http_utils.js (1)
CONTENT_TYPE_APP_FORM_URLENCODED(39-39)
⏰ 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). (2)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (1)
src/endpoint/iam/iam_utils.js (1)
612-661: Helper abstraction keeps tag parsing consistent.Centralizing the indexed-member parsing lets all tagging callers share the same logic, reducing divergence risks.
aayushchouhan09
force-pushed
the
tag-usr
branch
from
6d3e2fe to
fc1f111
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/endpoint/iam/ops/iam_list_user_tags.js (1)
26-35: ReturnMarkerwhen pagination is truncatedIf the backend signals additional pages (
is_truncatedtrue) it also hands backreply.marker, but we drop it here. Clients then have no way to request the next page. Please thread the marker through the XML payload.- return { - ListUserTagsResponse: { - ListUserTagsResult: { - Tags: reply.tags, - IsTruncated: reply.is_truncated - }, + const list_user_tags_result = { + Tags: reply.tags, + IsTruncated: reply.is_truncated, + }; + if (reply.marker) { + list_user_tags_result.Marker = reply.marker; + } + + return { + ListUserTagsResponse: { + ListUserTagsResult: list_user_tags_result, ResponseMetadata: { RequestId: req.request_id, }
♻️ Duplicate comments (2)
src/util/validation_utils.js (1)
255-299: Reject reservedaws:tag prefixWe’re still accepting tag keys that start with the reserved
aws:prefix in both TagUser and UntagUser flows. AWS hard-fails those requests, so allowing them here creates a mismatch where clients see “success” from us but would be rejected by AWS. Please add the prefix guard in both validators so we surface the same validation error._type_check_input('string', tag.key, `${tag_position}.key`); _validate_length_constraint(tag.key, `${tag_position}.key`, 1, MAX_TAG_KEY_LENGTH); _validate_tag_pattern(tag.key, `${tag_position}.key`, false); + if (tag.key.toLowerCase().startsWith('aws:')) { + throw new RpcError('VALIDATION_ERROR', + `1 validation error detected: Value at '${tag_position}.key' failed to satisfy constraint: Member must not start with reserved prefix 'aws:'`); + } @@ - for (let i = 0; i < tag_keys.length; i++) { - _type_check_input('string', tag_keys[i], 'tagKeys'); - - if (_has_tag_key_violation(tag_keys[i])) { + for (let i = 0; i < tag_keys.length; i++) { + const tag_key = tag_keys[i]; + _type_check_input('string', tag_key, 'tagKeys'); + + if (_has_tag_key_violation(tag_key)) { throw new RpcError('VALIDATION_ERROR', `1 validation error detected: Value at 'tagKeys' failed to satisfy constraint: Member must satisfy constraint: [${TAG_KEY_CONSTRAINTS.join(', ')}]`); } + if (tag_key.toLowerCase().startsWith('aws:')) { + throw new RpcError('VALIDATION_ERROR', + `1 validation error detected: Value at 'tagKeys' failed to satisfy constraint: Member must not start with reserved prefix 'aws:'`); + } }src/sdk/accountspace_nb.js (1)
421-421: Avoid mutating the original array.The
sort()method mutates the originalall_tagsarray fromrequested_account.tagging. If this array is referenced elsewhere, this could cause unexpected behavior.Apply this diff to sort a copy instead:
- const sorted_tags = all_tags.sort((a, b) => a.key.localeCompare(b.key)); + const sorted_tags = all_tags.slice().sort((a, b) => a.key.localeCompare(b.key));
🧹 Nitpick comments (1)
src/endpoint/iam/iam_utils.js (1)
84-101: Routing logic updated correctly, consider making it more explicit.The change to check for 'tag' before 'user' is necessary to prevent misrouting actions like 'tag_user'. The current substring-based approach works for the existing action set.
For future robustness, consider explicitly checking for tagging actions instead of substring matching:
function validate_params(action, params) { - if (action.includes('tag')) { + const tagging_actions = [ + iam_constants.IAM_ACTIONS.TAG_USER, + iam_constants.IAM_ACTIONS.UNTAG_USER, + iam_constants.IAM_ACTIONS.LIST_USER_TAGS + ]; + if (tagging_actions.includes(action)) { validate_tagging_params(action, params);This prevents potential misrouting if future actions contain 'tag' but aren't tagging operations.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/endpoint/iam/iam_constants.js(4 hunks)src/endpoint/iam/iam_rest.js(3 hunks)src/endpoint/iam/iam_utils.js(4 hunks)src/endpoint/iam/ops/iam_list_user_tags.js(1 hunks)src/endpoint/iam/ops/iam_tag_user.js(1 hunks)src/endpoint/iam/ops/iam_untag_user.js(1 hunks)src/sdk/account_sdk.js(1 hunks)src/sdk/accountspace_fs.js(1 hunks)src/sdk/accountspace_nb.js(2 hunks)src/sdk/nb.d.ts(1 hunks)src/util/string_utils.js(2 hunks)src/util/validation_utils.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/sdk/account_sdk.js
- src/sdk/nb.d.ts
- src/endpoint/iam/ops/iam_untag_user.js
- src/endpoint/iam/ops/iam_tag_user.js
- src/util/string_utils.js
🧰 Additional context used
🧬 Code graph analysis (6)
src/endpoint/iam/iam_utils.js (3)
src/endpoint/iam/ops/iam_list_user_tags.js (2)
params(14-18)iam_constants(6-6)src/endpoint/iam/ops/iam_tag_user.js (2)
params(17-20)iam_constants(6-6)src/endpoint/iam/ops/iam_untag_user.js (2)
params(17-20)iam_constants(6-6)
src/util/validation_utils.js (1)
src/util/string_utils.js (6)
AWS_IAM_TAG_KEY_AND_VALUE_REGEXP(18-18)i(34-34)i(75-75)i(83-83)i(126-126)i(130-130)
src/endpoint/iam/ops/iam_list_user_tags.js (5)
src/endpoint/iam/ops/iam_list_user_policies.js (4)
iam_utils(5-5)iam_constants(6-6)params(14-18)reply(22-22)src/endpoint/iam/ops/iam_list_instance_profile_tags.js (3)
iam_utils(5-5)iam_constants(6-6)params(15-19)src/endpoint/iam/ops/iam_list_mfa_device_tags.js (3)
iam_utils(5-5)iam_constants(6-6)params(15-19)src/endpoint/iam/iam_utils.js (1)
iam_constants(8-8)src/endpoint/iam/iam_rest.js (2)
reply(188-188)reply(230-230)
src/sdk/accountspace_nb.js (5)
src/util/account_util.js (23)
require(8-8)require(14-14)require(16-16)require(17-18)system_store(12-12)username(370-370)username(400-401)username(434-435)message_with_details(334-334)message_with_details(374-374)message_with_details(403-403)message_with_details(440-440)message_with_details(447-447)message_with_details(459-459)message_with_details(482-482)IamError(15-15)IamError(335-335)IamError(375-375)IamError(404-404)IamError(441-441)IamError(448-448)IamError(472-472)IamError(483-483)src/endpoint/iam/ops/iam_list_user_tags.js (1)
params(14-18)src/endpoint/iam/ops/iam_tag_user.js (1)
params(17-20)src/endpoint/iam/ops/iam_untag_user.js (1)
params(17-20)src/endpoint/iam/iam_constants.js (2)
IAM_ACTIONS(5-23)DEFAULT_MAX_TAGS(60-60)
src/endpoint/iam/iam_rest.js (2)
src/endpoint/iam/iam_utils.js (13)
IamError(77-77)IamError(250-250)IamError(476-476)IamError(505-505)IamError(533-533)IamError(540-540)IamError(568-568)IamError(590-590)IamError(602-602)IamError(606-606)require(6-6)require(7-7)require(9-9)src/sdk/accountspace_fs.js (6)
IamError(14-14)require(9-9)require(11-11)require(12-13)require(17-17)require(19-19)
src/sdk/accountspace_fs.js (5)
src/endpoint/iam/ops/iam_list_user_tags.js (1)
params(14-18)src/endpoint/iam/ops/iam_tag_user.js (1)
params(17-20)src/endpoint/iam/ops/iam_untag_user.js (1)
params(17-20)src/endpoint/iam/ops/iam_get_user.js (1)
params(14-16)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). (2)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (11)
src/sdk/accountspace_nb.js (4)
12-12: LGTM!The DEFAULT_MAX_TAGS import is correctly added to support the 50-tag limit enforcement in the tag_user method.
335-376: LGTM!The tag_user implementation correctly:
- Validates all required conditions
- Merges existing and new tags using a Map (handles duplicate keys properly)
- Enforces the 50-tag AWS limit after merging (addresses previous review feedback)
- Persists changes atomically
378-405: LGTM!The untag_user implementation correctly:
- Validates all required conditions
- Uses a Set for efficient key lookups
- Filters out the specified tag keys
- Persists changes atomically
407-436: Confirm API contract approach for pagination across all list operations.Verified: The REST layer accepts
markerandmax_itemsparameters and passes them to the SDK, butlist_user_tags(and other list operations likelist_usersandlist_access_keys) ignore them and returnis_truncated: false. Explicit "GAP - no pagination at this point" comments indicate this is known systemic technical debt.This creates an inconsistent API contract. Choose one approach:
- Remove pagination parameters from REST handlers if pagination won't be implemented
- Implement basic pagination to honor
max_itemsacross all list operations- Document clearly in API responses/specs that pagination is not supported
The inconsistency affects multiple operations and should be resolved consistently across all list methods.
src/endpoint/iam/iam_utils.js (7)
157-176: LGTM!The validate_tagging_params function follows the established pattern for action routing and correctly dispatches to the appropriate tag-specific validators.
605-608: LGTM!The INVALID_INPUT error mapping correctly translates RPC errors to IAM errors, supporting the tag validation flow.
612-630: LGTM!The parse_indexed_members utility correctly implements AWS-style indexed parameter parsing. The logic properly:
- Uses 1-based indexing per AWS conventions
- Stops at the first gap (correct behavior)
- Supports optional transformation via mapper function
632-649: LGTM!The parse_tags_from_request_body function correctly parses AWS-encoded indexed tag members. Defaulting the value to an empty string (line 646) is appropriate since AWS IAM permits empty tag values.
651-661: LGTM!The parse_tag_keys_from_request_body function correctly parses tag key arrays using the generic parser.
663-704: LGTM!All three tagging validators correctly:
- Check for required username first (addressing previous review feedback)
- Validate username format
- Validate tag-specific parameters
- Handle errors appropriately via translate_rpc_error
718-723: LGTM!The exports correctly expose the new parsing utilities and validators for use by the IAM tagging REST handlers.
src/util/validation_utils.js
Outdated
| * @param {number} min_length - minimum length (optional) | ||
| * @param {number} max_length - maximum length (optional) | ||
| */ | ||
| function _validate_length_constraint(value, position, min_length, max_length) { |
There was a problem hiding this comment.
Why create a new function when we already have _length_check_input?
There was a problem hiding this comment.
We can remove it after some minimal modification to _length_check_input()
|
|
||
| if (_has_tag_key_violation(tag_keys[i])) { | ||
| throw new RpcError('VALIDATION_ERROR', | ||
| `1 validation error detected: Value at 'tagKeys' failed to satisfy constraint: Member must satisfy constraint: [${TAG_KEY_CONSTRAINTS.join(', ')}]`); |
There was a problem hiding this comment.
Did you check this error against AWS?
It seems like it prints the array on constraints.
There was a problem hiding this comment.
yes, it should look like (verified in aws):
An error occurred (ValidationError) when calling the UntagUser operation: 1 validation error detected: Value at 'tagKeys' failed to satisfy constraint: Member must satisfy constraint: [Member must have length less than or equal to 128, Member must have length greater than or equal to 1, Member must satisfy regular expression pattern: [\p{L}\p{Z}\p{N}_.:/=+\-@]+, Member must not be null]
| // output: [{ key: 'env', value: 'prod' }, ...] | ||
| const params = { | ||
| username: req.body.user_name, | ||
| tags: iam_utils.parse_tags_from_request_body(req.body), |
There was a problem hiding this comment.
Did you test that you get the expected error when parsing fails?
There was a problem hiding this comment.
Added error handling, Thanks
aayushchouhan09
force-pushed
the
tag-usr
branch
from
fc1f111 to
2d9323f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/util/validation_utils.js (2)
283-289: Add validation for reserved 'aws:' prefix on tag keys in UntagUser.Similar to TagUser, UntagUser should reject tag keys starting with the reserved
aws:prefix to maintain consistency with AWS behavior.Add this check after line 284:
for (let i = 0; i < tag_keys.length; i++) { _type_check_input('string', tag_keys[i], 'tagKeys'); + if (tag_keys[i].toLowerCase().startsWith('aws:')) { + throw new RpcError('VALIDATION_ERROR', + `1 validation error detected: Value at 'tagKeys' failed to satisfy constraint: Member must not start with reserved prefix 'aws:'`); + } + if (_has_tag_key_violation(tag_keys[i])) {
231-251: Add validation for reserved 'aws:' prefix on tag keys.AWS rejects tag keys starting with the reserved
aws:prefix. According to past review comments, this validation should be added after line 243 to ensure consistency with AWS behavior.Add this check after validating the tag key pattern:
_type_check_input('string', tag.key, `${tag_position}.key`); _length_check_input(1, MAX_TAG_KEY_LENGTH, tag.key, `${tag_position}.key`); _validate_tag_pattern(tag.key, `${tag_position}.key`, false); + if (tag.key.toLowerCase().startsWith('aws:')) { + throw new RpcError('VALIDATION_ERROR', + `1 validation error detected: Value at '${tag_position}.key' failed to satisfy constraint: Member must not start with reserved prefix 'aws:'`); + } _type_check_input('string', tag.value, `${tag_position}.value`);
🧹 Nitpick comments (1)
src/sdk/accountspace_nb.js (1)
407-436: Document pagination limitation as technical debt.The TODO comment notes that pagination is not currently supported. While this is acceptable for an initial implementation, ensure this is tracked as technical debt since AWS IAM clients may expect pagination support.
The current implementation always returns
is_truncated: falseand ignoresmarkerandmax_itemsparameters. Consider tracking this as a follow-up issue if not already documented.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/endpoint/iam/iam_constants.js(4 hunks)src/endpoint/iam/iam_rest.js(3 hunks)src/endpoint/iam/iam_utils.js(4 hunks)src/endpoint/iam/ops/iam_list_user_tags.js(1 hunks)src/endpoint/iam/ops/iam_tag_user.js(1 hunks)src/endpoint/iam/ops/iam_untag_user.js(1 hunks)src/sdk/account_sdk.js(1 hunks)src/sdk/accountspace_fs.js(1 hunks)src/sdk/accountspace_nb.js(2 hunks)src/sdk/nb.d.ts(1 hunks)src/util/string_utils.js(2 hunks)src/util/validation_utils.js(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/endpoint/iam/ops/iam_untag_user.js
- src/endpoint/iam/iam_constants.js
- src/sdk/accountspace_fs.js
- src/endpoint/iam/ops/iam_tag_user.js
🧰 Additional context used
🧬 Code graph analysis (6)
src/sdk/account_sdk.js (3)
src/endpoint/iam/ops/iam_list_user_tags.js (1)
params(14-18)src/endpoint/iam/ops/iam_tag_user.js (1)
params(17-20)src/endpoint/iam/ops/iam_untag_user.js (1)
params(17-20)
src/endpoint/iam/iam_rest.js (3)
src/endpoint/iam/iam_utils.js (13)
IamError(77-77)IamError(250-250)IamError(476-476)IamError(505-505)IamError(533-533)IamError(540-540)IamError(568-568)IamError(590-590)IamError(602-602)IamError(606-606)require(6-6)require(7-7)require(9-9)src/sdk/accountspace_fs.js (6)
IamError(14-14)require(9-9)require(11-11)require(12-13)require(17-17)require(19-19)src/sdk/accountspace_nb.js (1)
IamError(9-9)
src/util/validation_utils.js (1)
src/util/string_utils.js (6)
AWS_IAM_TAG_KEY_AND_VALUE_REGEXP(18-18)i(34-34)i(75-75)i(83-83)i(126-126)i(130-130)
src/sdk/nb.d.ts (3)
src/endpoint/iam/ops/iam_list_user_tags.js (1)
params(14-18)src/endpoint/iam/ops/iam_tag_user.js (1)
params(17-20)src/endpoint/iam/ops/iam_untag_user.js (1)
params(17-20)
src/endpoint/iam/iam_utils.js (4)
src/endpoint/iam/ops/iam_list_user_tags.js (2)
params(14-18)iam_constants(6-6)src/endpoint/iam/ops/iam_tag_user.js (2)
params(17-20)iam_constants(6-6)src/endpoint/iam/ops/iam_untag_user.js (2)
params(17-20)iam_constants(6-6)src/util/validation_utils.js (1)
iam_constants(7-7)
src/endpoint/iam/ops/iam_list_user_tags.js (3)
src/endpoint/iam/ops/iam_tag_user.js (3)
params(17-20)iam_utils(5-5)iam_constants(6-6)src/endpoint/iam/ops/iam_untag_user.js (3)
params(17-20)iam_utils(5-5)iam_constants(6-6)src/endpoint/iam/ops/iam_list_user_policies.js (4)
params(14-18)iam_utils(5-5)iam_constants(6-6)reply(22-22)
⏰ 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: run-jest-unit-tests
- GitHub Check: Build Noobaa Image
🔇 Additional comments (16)
src/util/string_utils.js (1)
18-18: LGTM!The regex pattern correctly uses Unicode property escapes to support international characters in IAM tag keys and values, and the
/uflag is necessary for these escapes to work properly.Also applies to: 171-171
src/util/validation_utils.js (3)
9-13: LGTM!Tag validation constants align with AWS IAM limits for tag keys (128 chars) and values (256 chars).
36-42: LGTM!The optional parameter support correctly enables validation of tag values that can be empty (no minimum length requirement).
188-195: LGTM!The tag pattern validator and helper functions correctly implement AWS IAM tag validation logic, and the exports are properly added.
Also applies to: 201-205, 297-298
src/sdk/account_sdk.js (1)
124-137: LGTM!The new tagging methods follow the established delegation pattern and are correctly implemented.
src/sdk/nb.d.ts (1)
910-913: LGTM!The TypeScript interface declarations correctly define the new tagging methods with appropriate signatures.
src/endpoint/iam/ops/iam_list_user_tags.js (1)
20-29: LGTM!The handler correctly validates input, delegates to the account SDK, and returns the backend reply for tags and truncation status.
src/endpoint/iam/iam_rest.js (2)
24-24: LGTM!The INVALID_INPUT error mapping is correctly added to support tag validation errors.
39-41: LGTM!The tagging action mappings and operation handlers are correctly registered following the established pattern.
Also applies to: 89-92
src/sdk/accountspace_nb.js (2)
335-376: LGTM!The
tag_userimplementation correctly merges existing and new tags, enforces the AWS 50-tag limit after merging (as required), and includes proper authorization checks.
378-405: LGTM!The
untag_userimplementation efficiently filters out specified tag keys and includes proper authorization checks.src/endpoint/iam/iam_utils.js (5)
84-101: LGTM!The routing logic correctly prioritizes 'tag' actions before 'user' actions to prevent misrouting, and the comment clearly documents this requirement.
157-176: LGTM!The tagging parameter validator correctly routes to the appropriate validation function for each tagging action.
612-665: LGTM!The parsing utilities correctly handle AWS-style indexed request parameters and provide appropriate error handling.
671-708: LGTM!The tag validation functions correctly enforce required parameters and validate inputs in the proper order: required checks, username validation, then tag-specific validation.
605-608: LGTM!The INVALID_INPUT error translation is correctly added, and all new parsing and validation functions are properly exported.
Also applies to: 722-727
Signed-off-by: Aayush Chouhan <achouhan@redhat.com>
aayushchouhan09
force-pushed
the
tag-usr
branch
from
2d9323f to
c2a54f9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/util/validation_utils.js (1)
216-254: Add validation to reject tag keys with reservedaws:prefix.AWS reserves the
aws:prefix for tag keys—these tags cannot be created or edited by users. The currentvalidate_iam_tagsfunction does not validate against this reserved prefix, allowing invalid tags to pass through. The regex pattern allows colons, so keys likeaws:departmentwould incorrectly validate.Add this check after line 243 (following the
_validate_tag_patterncall):_type_check_input('string', tag.key, `${tag_position}.key`); _length_check_input(1, MAX_TAG_KEY_LENGTH, tag.key, `${tag_position}.key`); _validate_tag_pattern(tag.key, `${tag_position}.key`, false); + if (tag.key.toLowerCase().startsWith('aws:')) { + throw new RpcError('VALIDATION_ERROR', + `1 validation error detected: Value at '${tag_position}.key' failed to satisfy constraint: Member must not start with reserved prefix 'aws:'`); + }
🧹 Nitpick comments (1)
src/endpoint/iam/iam_constants.js (1)
60-60: Verify MAX_TAGS limit aligns with AWS IAM.The
MAX_TAGS = 50constant matches the AWS IAM limit for tags per user. Consider adding a comment referencing the AWS documentation to help future maintainers understand this constraint.Apply this diff to add documentation:
// miscellaneous const DEFAULT_MAX_ITEMS = 100; -const MAX_TAGS = 50; +const MAX_TAGS = 50; // AWS IAM limit: https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_iam-quotas.html const MAX_NUMBER_OF_ACCESS_KEYS = 2;Also applies to: 86-86
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/endpoint/iam/iam_constants.js(4 hunks)src/endpoint/iam/iam_rest.js(3 hunks)src/endpoint/iam/iam_utils.js(4 hunks)src/endpoint/iam/ops/iam_list_user_tags.js(1 hunks)src/endpoint/iam/ops/iam_tag_user.js(1 hunks)src/endpoint/iam/ops/iam_untag_user.js(1 hunks)src/sdk/account_sdk.js(1 hunks)src/sdk/accountspace_fs.js(1 hunks)src/sdk/accountspace_nb.js(2 hunks)src/sdk/nb.d.ts(1 hunks)src/test/unit_tests/api/iam/test_iam_ops_input_validation.test.js(4 hunks)src/util/string_utils.js(2 hunks)src/util/validation_utils.js(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/endpoint/iam/ops/iam_untag_user.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_ops_input_validation.test.js
🧬 Code graph analysis (9)
src/sdk/account_sdk.js (3)
src/endpoint/iam/ops/iam_list_user_tags.js (1)
params(14-18)src/endpoint/iam/ops/iam_tag_user.js (1)
params(17-20)src/endpoint/iam/ops/iam_untag_user.js (1)
params(17-20)
src/endpoint/iam/ops/iam_tag_user.js (4)
src/endpoint/iam/ops/iam_list_user_tags.js (3)
iam_utils(5-5)iam_constants(6-6)params(14-18)src/endpoint/iam/ops/iam_untag_user.js (3)
iam_utils(5-5)iam_constants(6-6)params(17-20)src/endpoint/iam/iam_utils.js (1)
iam_constants(8-8)src/util/http_utils.js (1)
CONTENT_TYPE_APP_FORM_URLENCODED(39-39)
src/sdk/accountspace_nb.js (5)
src/util/account_util.js (16)
require(8-8)require(14-14)require(16-16)require(17-18)system_store(12-12)username(370-370)username(400-401)username(434-435)IamError(15-15)IamError(335-335)IamError(375-375)IamError(404-404)IamError(441-441)IamError(448-448)IamError(472-472)IamError(483-483)src/endpoint/iam/ops/iam_list_user_tags.js (1)
params(14-18)src/endpoint/iam/ops/iam_tag_user.js (1)
params(17-20)src/endpoint/iam/ops/iam_untag_user.js (1)
params(17-20)src/endpoint/iam/iam_constants.js (2)
IAM_ACTIONS(5-23)MAX_TAGS(60-60)
src/sdk/accountspace_fs.js (5)
src/endpoint/iam/ops/iam_list_user_tags.js (1)
params(14-18)src/endpoint/iam/ops/iam_tag_user.js (1)
params(17-20)src/endpoint/iam/ops/iam_untag_user.js (1)
params(17-20)src/endpoint/iam/ops/iam_get_user.js (1)
params(14-16)src/endpoint/iam/iam_constants.js (1)
IAM_ACTIONS(5-23)
src/endpoint/iam/iam_rest.js (1)
src/endpoint/iam/iam_utils.js (13)
IamError(77-77)IamError(250-250)IamError(476-476)IamError(505-505)IamError(533-533)IamError(540-540)IamError(568-568)IamError(590-590)IamError(602-602)IamError(606-606)require(6-6)require(7-7)require(9-9)
src/sdk/nb.d.ts (3)
src/endpoint/iam/ops/iam_list_user_tags.js (1)
params(14-18)src/endpoint/iam/ops/iam_tag_user.js (1)
params(17-20)src/endpoint/iam/ops/iam_untag_user.js (1)
params(17-20)
src/endpoint/iam/iam_utils.js (3)
src/endpoint/iam/ops/iam_list_user_tags.js (2)
params(14-18)iam_constants(6-6)src/endpoint/iam/ops/iam_tag_user.js (2)
params(17-20)iam_constants(6-6)src/endpoint/iam/ops/iam_untag_user.js (2)
params(17-20)iam_constants(6-6)
src/util/validation_utils.js (1)
src/util/string_utils.js (6)
AWS_IAM_TAG_KEY_AND_VALUE_REGEXP(18-18)i(34-34)i(75-75)i(83-83)i(126-126)i(130-130)
src/endpoint/iam/ops/iam_list_user_tags.js (2)
src/endpoint/iam/ops/iam_tag_user.js (3)
params(17-20)iam_utils(5-5)iam_constants(6-6)src/endpoint/iam/ops/iam_list_user_policies.js (4)
params(14-18)iam_utils(5-5)iam_constants(6-6)reply(22-22)
⏰ 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 (14)
src/sdk/accountspace_fs.js (1)
561-584: LGTM! Appropriate stub implementation for filesystem backend.The tagging methods follow the established pattern for unimplemented features in
AccountSpaceFS:
tag_useranduntag_usercorrectly throwNotImplementederrorslist_user_tagsvalidates user existence viaget_userbefore returning an empty tag list- Return structure matches the expected format consumed by the IAM endpoint
This aligns with other unimplemented features like
put_user_policyand provides a consistent API surface.src/test/unit_tests/api/iam/test_iam_ops_input_validation.test.js (1)
258-258: Test assertion updated to match more specific validation error.The error message expectation was tightened from
/invalid/ito/length greater than/i, aligning with the improved validation messages that now explicitly state length constraints.src/endpoint/iam/iam_constants.js (1)
16-18: LGTM! IAM actions and mappings correctly defined.The new tagging actions follow the established naming conventions:
IAM_ACTIONSuses snake_case:tag_user,untag_user,list_user_tagsACTION_MESSAGE_TITLE_MAPuses AWS-style CamelCase:TagUser,UntagUser,ListUserTagsAlso applies to: 39-41
src/util/string_utils.js (1)
18-18: LGTM! Unicode-aware tag validation regex.The regex correctly uses the
/uflag to enable Unicode property escapes:
\p{L}matches any letter\p{Z}matches any whitespace/separator\p{N}matches any numberThis aligns with AWS IAM tag requirements that support Unicode characters. Based on learnings.
Also applies to: 171-171
src/endpoint/iam/ops/iam_tag_user.js (1)
12-33: LGTM! Clean implementation following IAM endpoint patterns.The handler correctly:
- Parses AWS form-encoded tags from request body (handles
tags_member_1_key,tags_member_1_value, etc.)- Validates params using
iam_utils.validate_params- Delegates to
account_sdk.tag_user- Returns properly formatted XML response
Past review comments about edge case handling (non-sequential indices, duplicate keys, null guards) have been addressed in the parsing and validation utilities.
src/sdk/account_sdk.js (1)
120-137: LGTM! SDK methods correctly delegate to accountspace.The new tagging methods follow the established pattern:
- Retrieve accountspace via
_get_accountspace()- Delegate to corresponding accountspace method
- Pass params and AccountSDK instance (
this) for contextThis maintains consistency with existing USER and ACCESS KEY operations.
src/util/validation_utils.js (4)
9-13: Tag validation constants properly defined.The constants align with AWS IAM requirements:
- Key: 1-128 characters
- Value: 0-256 characters
- Unicode pattern support via
\p{L},\p{Z},\p{N}
36-42: Improved validation with more specific error messages.The refactored
_length_check_inputnow supports optional min/max parameters, and error messages explicitly state "length greater than or equal to" rather than generic "invalid". This addresses the test expectation changes intest_iam_ops_input_validation.test.js.Also applies to: 54-57
231-239: Good defense against malformed tag objects.The null and array checks prevent runtime TypeErrors when accessing
tag.keyortag.valueon malformed payloads, ensuring validation errors are returned instead of 500 errors.
265-293: Tag key validation is thorough.The
validate_iam_tag_keysfunction properly validates:
- Array type
- Length constraints (≤50 keys per request)
- Individual key constraints (type, length, pattern)
The error messages match AWS format by listing all constraints together. However, the same
aws:prefix check mentioned above should also be applied here for consistency.src/endpoint/iam/ops/iam_list_user_tags.js (1)
12-36: LGTM! List operation correctly returns dynamic tag data.The handler properly:
- Constructs params with username, optional marker, and max_items (defaults to 100)
- Validates params against
LIST_USER_TAGSaction- Returns actual
reply.tagsandreply.is_truncatedfrom the accountspace layerThis addresses past review feedback to return dynamic data rather than hardcoded empty lists, aligning with the pattern used in
list_user_policies.src/sdk/accountspace_nb.js (1)
419-435: Pagination deliberately not implemented (known limitation).The TODO comment on line 419 clearly documents that pagination is not supported in this PR. The implementation correctly returns all tags sorted by key but ignores
params.markerandparams.max_items. Based on past review discussion, this was a deliberate decision for this PR.If pagination support is planned for a future PR, consider extracting the pagination logic into a reusable utility function that can be shared across list operations (list_users, list_access_keys, list_user_tags, etc.) to maintain consistency.
src/endpoint/iam/iam_utils.js (2)
90-91: Well-designed routing logic prevents misclassification.The comment on line 85 and the implementation correctly handle the overlapping action names. Checking for 'tag' before 'user' ensures that
tag_user,untag_user, andlist_user_tagsactions are routed tovalidate_tagging_paramsrather than being incorrectly classified as user actions.
612-634: Solid implementation of AWS-style indexed parameter parsing.The generic
parse_indexed_membersfunction correctly handles AWS's 1-based indexed arrays and provides a flexible mapper option. The error handling appropriately wraps any parsing failures in anINVALID_INPUTerror.
3 participants
Describe the Problem
Support tagging for IAM users.
Explain the Changes
TagUser,UntagUser, andListUserTagsAPIs.Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
Summary by CodeRabbit
New Features
Validation
Configuration Updates
Behavior
Tests