Skip to content

crud operations + assume role - #9865

Merged
sakshimunjal merged 1 commit into
noobaa:masterfrom
sakshimunjal:nc-iam-roles-phase1
Aug 13, 2026
Merged

crud operations + assume role#9865
sakshimunjal merged 1 commit into
noobaa:masterfrom
sakshimunjal:nc-iam-roles-phase1

Conversation

@sakshimunjal

@sakshimunjal sakshimunjal commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Describe the Problem

NC lacked standalone IAM role support: role CRUD was NotImplemented, and STS/S3 assumed-role flows only worked via containerized system_store.

Explain the Changes

  1. Add NC IAM role CRUD (Create/Get/Update/Delete/ListRole) + FS layout in config_fs / accountspace_fs
  2. Wire STS AssumeRole for NC (role resolve + trust policy) and S3 auth resolve via resolve_iam_role_by_arn + NC empty-policy allow until Phase 2

Issues: Fixed #xxx / Gap #xxx

  1. Gap: NC IAM Roles Phase 1 (CRUD + AssumeRole) — design iam_nc.md

Testing Instructions:

  1. Create role → AssumeRole → set temp creds → aws s3 ls (unset session token before IAM CRUD)
  2. Verify Get/Update/Delete/ListRole; confirm containerized path still uses system_store
  • Doc added/updated
  • Tests added

Summary by CodeRabbit

  • New Features
    • Added filesystem-backed IAM role management, including creation, viewing, updating, deletion, listing, path filtering, session durations, and trust policies.
    • Enhanced NC assume-role support with improved role resolution and trust-policy retrieval.
    • Expanded account configuration to support descriptions and role session settings.
  • Bug Fixes
    • Improved authorization decisions for assumed-role sessions without identity policies.
  • Documentation
    • Updated role and account schema documentation.
  • Tests
    • Added coverage for role lifecycle, filtering, authorization, and cleanup scenarios.

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

IAM role support is added to ConfigFS and account-space APIs. NC-specific role resolution is wired into STS, IAM authorization, and assume-role policy retrieval. Account schema fields and SDK request wiring are updated.

Changes

IAM roles and NC integration

Layer / File(s) Summary
Role schema contract
src/server/system_services/schemas/nsfs_account_schema.js, src/manage_nsfs/nsfs_schema_utils.js, src/test/unit_tests/nc/configuration/test_nc_nsfs_account_schema_validation.test.js
The account schema accepts role description, session duration, and trust-policy fields. Several account requirements and their validation tests are removed.
Role storage and account APIs
src/sdk/config_fs.js, src/sdk/accountspace_fs.js, src/test/unit_tests/nsfs/test_accountspace_fs.test.js
ConfigFS persists roles through identity files and symlinks. Account-space APIs implement role lifecycle operations, ARN lookup, trust-policy updates, and role tests.
Accountspace and STS wiring
src/sdk/object_sdk.js, src/sdk/nsfs_object_sdk.js, src/sdk/sts_sdk.js, src/cmd/nsfs.js
Accountspace is initialized in object SDKs and passed into STS request initialization.
NC STS role resolution
src/sdk/sts_sdk.js, src/endpoint/sts/sts_rest.js
STS selects environment-specific role resolution and retrieves NC role trust policies from accountspace.
NC IAM policy authorization
src/endpoint/iam/iam_utils.js
Assumed-role policy resolution uses accountspace. The NSFS no-inline-policy path includes assumed-role sessions.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant StsSDK
  participant AccountSpaceFS
  participant IAMAuthorization

  Client->>StsSDK: Assume role
  StsSDK->>AccountSpaceFS: Resolve role ARN
  AccountSpaceFS-->>StsSDK: Role, account, and access-key data
  StsSDK-->>Client: Assumed-role credentials and trust policy
  IAMAuthorization->>AccountSpaceFS: Resolve assumed-role identity policies
  AccountSpaceFS-->>IAMAuthorization: Role policy data
Loading

Possibly related PRs

Suggested reviewers: jackyalbo, naveenpaul1, aayushchouhan09

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main changes: role CRUD operations and AssumeRole support.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
⚔️ Resolve merge conflicts 💡
  • Resolve merge conflict in branch nc-iam-roles-phase1
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (3)
src/sdk/accountspace_fs.js (1)

718-720: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider sorting list_roles members for stable output.

list_users sorts members by username before returning; list_roles returns filesystem order. Sorting by role_name keeps listing deterministic and consistent with the users path.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/sdk/accountspace_fs.js` around lines 718 - 720, Update list_roles to sort
the members returned by _list_config_files_for_roles by role_name before
constructing the result. Preserve the existing is_truncated value and return
shape while ensuring deterministic ordering consistent with list_users.
src/endpoint/sts/sts_rest.js (1)

242-248: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Prefer reusing parse_role_arn over ad-hoc ARN slicing.

The NC branch re-parses the ARN by hand (slice/split(':')[4]) while the non-NC branch below relies on resolve_iam_role_by_arn (which uses parse_role_arn). Reusing the shared parser avoids divergent handling of paths in the ARN and malformed inputs.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/endpoint/sts/sts_rest.js` around lines 242 - 248, Update the NC branch
around role_name and account_id to reuse parse_role_arn instead of manually
slicing role_arn and splitting its account segment. Use the parser’s extracted
role name and account ID when calling config_fs.get_role_by_name, preserving the
existing assume_role_policy_document return behavior.
src/server/system_services/schemas/nsfs_account_schema.js (1)

10-15: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚖️ Poor tradeoff

Loosening required on the shared account schema weakens validation for real accounts.

Since this schema is reused for roles, dropping email, access_keys, and master_key_id from required lets role data validate — but it also stops enforcing these on genuine account configs written via create_account_config_file/_prepare_for_account_schema. Consider a dedicated role_schema (or a oneOf) so account invariants stay enforced while roles validate against role-appropriate requirements.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/server/system_services/schemas/nsfs_account_schema.js` around lines 10 -
15, Preserve required validation for genuine account configurations while
allowing role data to validate: update the shared account-schema usage around
nsfs_account_config and the
create_account_config_file/_prepare_for_account_schema flow to distinguish
account and role validation, using a dedicated role_schema or an appropriate
oneOf. Keep email, access_keys, and master_key_id required for account configs,
while defining only role-appropriate requirements for roles.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/sdk/accountspace_fs.js`:
- Around line 679-681: Remove the unused role_data_updated assignment and ARN
computation from update_role, while preserving the await of
update_role_config_file and its existing empty-object return value. Do not alter
the update operation’s behavior.

In `@src/sdk/sts_sdk.js`:
- Around line 104-109: The _get_role_containerized return path must guard
against missing owner access keys and support both string and SensitiveString
values. Mirror _get_role_nc by checking access_keys?.length before indexing,
then normalize the first access_key with a string check or unwrap; preserve the
existing ACCESS_DENIED/NO_SUCH_ACCOUNT error behavior for malformed owners.

In `@src/server/system_services/schemas/nsfs_account_schema.js`:
- Around line 120-122: Update the role default handling associated with
assume_role_policy_document and _new_role_defaults so an omitted policy is not
defaulted to an empty object. Use undefined or validate the supplied parameter
before nsfs_schema_utils.validate_account_schema, allowing CreateRole’s IAM
policy-document validation error instead of INVALID_SCHEMA from the NC schema.

---

Nitpick comments:
In `@src/endpoint/sts/sts_rest.js`:
- Around line 242-248: Update the NC branch around role_name and account_id to
reuse parse_role_arn instead of manually slicing role_arn and splitting its
account segment. Use the parser’s extracted role name and account ID when
calling config_fs.get_role_by_name, preserving the existing
assume_role_policy_document return behavior.

In `@src/sdk/accountspace_fs.js`:
- Around line 718-720: Update list_roles to sort the members returned by
_list_config_files_for_roles by role_name before constructing the result.
Preserve the existing is_truncated value and return shape while ensuring
deterministic ordering consistent with list_users.

In `@src/server/system_services/schemas/nsfs_account_schema.js`:
- Around line 10-15: Preserve required validation for genuine account
configurations while allowing role data to validate: update the shared
account-schema usage around nsfs_account_config and the
create_account_config_file/_prepare_for_account_schema flow to distinguish
account and role validation, using a dedicated role_schema or an appropriate
oneOf. Keep email, access_keys, and master_key_id required for account configs,
while defining only role-appropriate requirements for roles.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 582b01cf-9c68-4593-bec5-d1ad18d8c2be

📥 Commits

Reviewing files that changed from the base of the PR and between ae08790 and ac66287.

📒 Files selected for processing (7)
  • src/endpoint/iam/iam_utils.js
  • src/endpoint/sts/sts_rest.js
  • src/manage_nsfs/nsfs_schema_utils.js
  • src/sdk/accountspace_fs.js
  • src/sdk/config_fs.js
  • src/sdk/sts_sdk.js
  • src/server/system_services/schemas/nsfs_account_schema.js

Comment thread src/sdk/accountspace_fs.js Outdated
Comment thread src/sdk/sts_sdk.js Outdated
Comment thread src/server/system_services/schemas/nsfs_account_schema.js

@alphaprinz alphaprinz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to have sts_rest route logic to accountspace_nb/fs, rather than using is_nc_env() function.
Also didn't understand why we removed account fields from the schema. We still use the same schema for account json, right?

Comment thread src/endpoint/iam/iam_utils.js Outdated

const { account_id, role_name } = parsed;

if (is_nc_environment()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think delegating this function to namesapce/accountspace_nb/fs would be nicer than this if.

Comment thread src/endpoint/iam/iam_utils.js
Comment thread src/endpoint/iam/iam_utils.js
Comment thread src/endpoint/sts/sts_rest.js Outdated
const role_arn = req.body.role_arn;
const role_name = role_arn.slice(role_arn.lastIndexOf('/') + 1);

if (is_nc_environment()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move to namespace/accountspace impl, take 3 :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done for NC via sts_sdk → accountspace; containerized path unchanged.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point is not to have is_nc_env() when we have AccountSpace_NB and AccountSpace_FS.
iam_utils.resolve_iam_role_by_arn() should be moved to AccountSpace_NB.
sts_rest.get_assume_role_policy() should use a virtual req.accountsdk.accountspace.resolve_role_by_arn().

@shirady again, WDYT?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread src/server/system_services/schemas/nsfs_account_schema.js

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/sdk/nsfs_object_sdk.js (1)

9-32: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Resolve fs_root into AccountSpaceFS’ config root for simple-mode NSFS.

In the else branch, new AccountSpaceFS({ fs_root }) ignores fs_root; AccountSpaceFS only uses config_root to construct ConfigFS, so IAM/STS role and access-key lookups read from the default NC config location (NSFS_NC_DEFAULT_CONF_DIR) instead of this simple-mode NSFS endpoint’s fs_root. Pass fs_root as the config root, or route accounts over fs_root through a simple-mode accountspace.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/sdk/nsfs_object_sdk.js` around lines 9 - 32, Update the simple-mode
branch of NsfsObjectSDK’s constructor to initialize AccountSpaceFS with fs_root
as its config_root, ensuring IAM, STS role, and access-key lookups use the
endpoint’s configured root instead of the default NC configuration directory.
Leave the config_root branch unchanged.
🧹 Nitpick comments (1)
src/endpoint/iam/iam_utils.js (1)

1343-1356: 🔒 Security & Privacy | 🔵 Trivial

NC empty-policy allow now also covers assumed-role sessions.

The fast-path bypass for zero inline policies is widened to is_iam_user || is_assumed_role_session under NC (req.object_sdk.nsfs_config_root). This matches the PR's stated Phase-1 scope (empty-policy allow pending PutRolePolicy/Phase 2) and is already tracked via the inline TODO, so flagging only for visibility — happy to help wire this up once role inline policies (Phase 2) land, if useful.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/endpoint/iam/iam_utils.js` around lines 1343 - 1356, The empty
inline-policy fast path in the IAM authorization flow now intentionally allows
assumed-role sessions under NC. Preserve the condition in
authorize_request_iam_policy using req.object_sdk.nsfs_config_root and the
is_iam_user || is_assumed_role_session check, along with its existing Phase-2
TODO; no code change is required.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/sdk/nsfs_object_sdk.js`:
- Around line 9-32: Update the simple-mode branch of NsfsObjectSDK’s constructor
to initialize AccountSpaceFS with fs_root as its config_root, ensuring IAM, STS
role, and access-key lookups use the endpoint’s configured root instead of the
default NC configuration directory. Leave the config_root branch unchanged.

---

Nitpick comments:
In `@src/endpoint/iam/iam_utils.js`:
- Around line 1343-1356: The empty inline-policy fast path in the IAM
authorization flow now intentionally allows assumed-role sessions under NC.
Preserve the condition in authorize_request_iam_policy using
req.object_sdk.nsfs_config_root and the is_iam_user || is_assumed_role_session
check, along with its existing Phase-2 TODO; no code change is required.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 4a6f6390-db94-45d1-9346-b04f79d0679f

📥 Commits

Reviewing files that changed from the base of the PR and between ac66287 and f4194b6.

📒 Files selected for processing (11)
  • src/cmd/nsfs.js
  • src/endpoint/iam/iam_utils.js
  • src/endpoint/sts/sts_rest.js
  • src/manage_nsfs/nsfs_schema_utils.js
  • src/sdk/accountspace_fs.js
  • src/sdk/config_fs.js
  • src/sdk/nsfs_object_sdk.js
  • src/sdk/object_sdk.js
  • src/sdk/sts_sdk.js
  • src/server/system_services/schemas/nsfs_account_schema.js
  • src/test/unit_tests/nsfs/test_accountspace_fs.test.js
🚧 Files skipped from review as they are similar to previous changes (4)
  • src/server/system_services/schemas/nsfs_account_schema.js
  • src/manage_nsfs/nsfs_schema_utils.js
  • src/sdk/accountspace_fs.js
  • src/sdk/config_fs.js

@sakshimunjal
sakshimunjal force-pushed the nc-iam-roles-phase1 branch 2 times, most recently from 45ca4a4 to 00d8255 Compare August 1, 2026 13:54

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/sdk/accountspace_fs.js`:
- Around line 733-738: Update get_role_by_arn to validate that the parsed ARN
represents the IAM service and has a role/ resource type before calling
config_fs.get_role_by_name. Return the parser’s existing error response for
non-role ARN values, while preserving the current filesystem lookup for valid
role ARNs.
- Around line 673-680: Prevent lost updates between update_role and
update_assume_role_policy by changing the role metadata update flow around
update_role_config_file to use compare-and-swap, per-role locking, or
field-level merge semantics. Ensure concurrent updates preserve
assume_role_policy_document when UpdateRole changes description or
max_session_duration, and preserve those metadata fields when
UpdateAssumeRolePolicy changes the policy. Apply the corresponding fix at
src/sdk/accountspace_fs.js lines 673-680 and 791-797.
- Around line 604-615: Update create_role around is_role_exists_by_name,
list_roles_under_account, and create_role_config_file to reserve the role name
atomically while writing the role configuration. Move name-conflict and
per-account quota enforcement into the same exclusive write/creation operation,
preserving EntityAlreadyExists and LimitExceeded errors, so concurrent requests
cannot bypass either check.

In
`@src/test/unit_tests/nc/configuration/test_nc_nsfs_account_schema_validation.test.js`:
- Around line 180-214: Replace the commented-out required-field tests in the
account schema validation suite with active coverage for the optional-field
contract. Add a role-shaped fixture that omits email, access_keys,
allow_bucket_creation, and master_key_id, then assert validate_account_schema()
accepts it; apply the same coverage to the related commented sections without
preserving obsolete missing-property expectations.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 2e8b9ccb-0639-4bc6-948f-4c3109d14027

📥 Commits

Reviewing files that changed from the base of the PR and between 45ca4a4 and 00d8255.

📒 Files selected for processing (12)
  • src/cmd/nsfs.js
  • src/endpoint/iam/iam_utils.js
  • src/endpoint/sts/sts_rest.js
  • src/manage_nsfs/nsfs_schema_utils.js
  • src/sdk/accountspace_fs.js
  • src/sdk/config_fs.js
  • src/sdk/nsfs_object_sdk.js
  • src/sdk/object_sdk.js
  • src/sdk/sts_sdk.js
  • src/server/system_services/schemas/nsfs_account_schema.js
  • src/test/unit_tests/nc/configuration/test_nc_nsfs_account_schema_validation.test.js
  • src/test/unit_tests/nsfs/test_accountspace_fs.test.js
🚧 Files skipped from review as they are similar to previous changes (10)
  • src/cmd/nsfs.js
  • src/manage_nsfs/nsfs_schema_utils.js
  • src/endpoint/sts/sts_rest.js
  • src/sdk/object_sdk.js
  • src/sdk/nsfs_object_sdk.js
  • src/endpoint/iam/iam_utils.js
  • src/server/system_services/schemas/nsfs_account_schema.js
  • src/sdk/sts_sdk.js
  • src/test/unit_tests/nsfs/test_accountspace_fs.test.js
  • src/sdk/config_fs.js

Comment thread src/sdk/accountspace_fs.js
Comment thread src/sdk/accountspace_fs.js Outdated
Comment thread src/sdk/accountspace_fs.js Outdated
Comment on lines +733 to +738
async get_role_by_arn(params) {
const parsed = parse_role_arn(params.role_arn);
if (parsed.error) return { error: parsed.error };
const { account_id, role_name } = parsed;
const iam_role = await this.config_fs.get_role_by_name(role_name, account_id, { silent_if_missing: true });
if (!iam_role) return { error: 'NO_SUCH_ROLE', account_id, role_name };

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Reject ARN values that do not identify an IAM role.

parse_role_arn() extracts only the account ID and final path segment. A value such as arn:aws:iam::ACCOUNT_ID:user/ROLE_NAME can resolve as role ROLE_NAME when that role exists. Validate the service and role/ resource type before the filesystem lookup.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/sdk/accountspace_fs.js` around lines 733 - 738, Update get_role_by_arn to
validate that the parsed ARN represents the IAM service and has a role/ resource
type before calling config_fs.get_role_by_name. Return the parser’s existing
error response for non-role ARN values, while preserving the current filesystem
lookup for valid role ARNs.

Comment on lines +180 to +214
// it('account without email', () => {
// const account_data = get_account_data();
// delete account_data.email;
// const reason = 'Test should have failed because of missing required property ' +
// 'email';
// const message = "must have required property 'email'";
// assert_validation(account_data, reason, message);
// });

// it('account with undefined email', () => {
// const account_data = get_account_data();
// account_data.email = undefined;
// const reason = 'Test should have failed because of missing required property ' +
// 'email';
// const message = "must have required property 'email'";
// assert_validation(account_data, reason, message);
// });

// it('account without access_keys', () => {
// const account_data = get_account_data();
// delete account_data.access_keys;
// const reason = 'Test should have failed because of missing required property ' +
// 'access_keys';
// const message = "must have required property 'access_keys'";
// assert_validation(account_data, reason, message);
// });

// it('account with undefined access_keys', () => {
// const account_data = get_account_data();
// account_data.access_keys = undefined;
// const reason = 'Test should have failed because of missing required property ' +
// 'access_keys';
// const message = "must have required property 'access_keys'";
// assert_validation(account_data, reason, message);
// });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Add active tests for the new optional-field contract.

These comments remove validation coverage for email, access_keys, allow_bucket_creation, and master_key_id. Add a role-shaped fixture that omits these fields and assert that validate_account_schema() accepts it. This protects the role schema contract without retaining obsolete account-only requirements.

As per path instructions, src/test/**/*.* must include tests for the changes.

Also applies to: 308-324, 344-360

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/test/unit_tests/nc/configuration/test_nc_nsfs_account_schema_validation.test.js`
around lines 180 - 214, Replace the commented-out required-field tests in the
account schema validation suite with active coverage for the optional-field
contract. Add a role-shaped fixture that omits email, access_keys,
allow_bucket_creation, and master_key_id, then assert validate_account_schema()
accepts it; apply the same coverage to the related commented sections without
preserving obsolete missing-property expectations.

Source: Path instructions

Comment thread src/sdk/accountspace_fs.js Outdated
// 2 - verify role_name is unique under the account
// 3 - enforce per-account role limit
// 4 - write role identity.json + symlink
async create_role(params, account_sdk) {

This comment was marked as off-topic.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LDAP changes will be there in next PR, this PR is only for role APIs and basic STS assume role

@alphaprinz alphaprinz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think we can remove the is_nc_env() if we move the resolve_role_by_arn to accountspace. I'll ask Shira to also weigh in on this. Please let me know if you think it's problematic. I think it will give us cleaner code.

Comment thread src/endpoint/iam/iam_utils.js Outdated
* @returns {Promise<object[]|null>} policies, or null if assumed role could not be resolved
*/
async function _get_identity_policies(account, is_iam_user, assumed_role_arn, bucketspace) {
async function _get_identity_policies(account, is_iam_user, assumed_role_arn, bucketspace, object_sdk) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets move iam_utils.resolve_iam_role_by_arn() to accountspace_nb.
That way we have the nb-only iam_utils.resolve_iam_role_by_arn() in an nb-only file.
And then we can skip the 'object_sdk?.accountspace ?' test.
It does mean you need to pass account_sdk to _get_identity_policies(), possibly from req.
@shirady WDYT?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alphaprinz I'm fine with what you're suggesting.
I think it is more relevant to you @naveenpaul1 @VershaAgrawal @aayushchouhan09.

Comment thread src/endpoint/iam/iam_utils.js
Comment thread src/endpoint/sts/sts_rest.js Outdated
const role_arn = req.body.role_arn;
const role_name = role_arn.slice(role_arn.lastIndexOf('/') + 1);

if (is_nc_environment()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point is not to have is_nc_env() when we have AccountSpace_NB and AccountSpace_FS.
iam_utils.resolve_iam_role_by_arn() should be moved to AccountSpace_NB.
sts_rest.get_assume_role_policy() should use a virtual req.accountsdk.accountspace.resolve_role_by_arn().

@shirady again, WDYT?

Comment thread src/sdk/sts_sdk.js Outdated
* @returns {Promise<Object>}
*/
async _assume_role(role_arn) {
const role_response = await (is_nc_environment() ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here for is_nc_env()

Comment thread src/endpoint/iam/iam_utils.js Outdated
* @returns {Promise<object[]|null>} policies, or null if assumed role could not be resolved
*/
async function _get_identity_policies(account, is_iam_user, assumed_role_arn, bucketspace) {
async function _get_identity_policies(account, is_iam_user, assumed_role_arn, bucketspace, object_sdk) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alphaprinz I'm fine with what you're suggesting.
I think it is more relevant to you @naveenpaul1 @VershaAgrawal @aayushchouhan09.

Comment thread src/endpoint/sts/sts_rest.js Outdated
const role_arn = req.body.role_arn;
const role_name = role_arn.slice(role_arn.lastIndexOf('/') + 1);

if (is_nc_environment()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread src/manage_nsfs/manage_nsfs_cli_errors.js
Comment thread src/manage_nsfs/manage_nsfs_validations.js Outdated
Comment thread src/manage_nsfs/manage_nsfs_validations.js Outdated
Comment thread src/manage_nsfs/nsfs_schema_utils.js Outdated
Comment thread src/server/system_services/schemas/nsfs_account_schema.js
Comment thread src/server/system_services/schemas/nsfs_account_schema.js Outdated
Comment thread src/sdk/accountspace_fs.js
Comment thread src/sdk/accountspace_fs.js Outdated

@alphaprinz alphaprinz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found some things that can be removed.
Otherwise LGTM :)

const message = "must have required property 'access_keys'";
assert_validation(account_data, reason, message);
});
// it('account without access_keys', () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a point in keeping the commented-out code.
We can add a comment explaining why these fields are not mandatory for account.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, just wanted to get the review from @shirady if these required fields can be removed. If yes, then will be removing these

Comment thread src/sdk/object_sdk.js Outdated
this.object_io = object_io;
this.stats = stats;
this.bucketspace = bucketspace || new BucketSpaceNB({ rpc_client, internal_rpc_client });
// NC only: used for assumed-role identity policy resolve via AccountSpaceFS

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still true?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be removed

Comment thread src/manage_nsfs/manage_nsfs_cli_errors.js
Comment thread src/server/system_services/schemas/nsfs_account_schema.js

@aayushchouhan09 aayushchouhan09 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please check the usage of accountspace in object sdk and sts sdk, if it is not relevant then we can remove it.

@sakshimunjal
sakshimunjal force-pushed the nc-iam-roles-phase1 branch 4 times, most recently from f9a7513 to 7ab5e3b Compare August 13, 2026 10:18
Signed-off-by: Sakshi Munjal <sakshimunjal@Sakshis-MacBook-Pro.local>
@sakshimunjal
sakshimunjal merged commit b7bd233 into noobaa:master Aug 13, 2026
19 of 20 checks passed
Comment on lines +129 to +132
type: {
// to make consistent with containerized
type: 'string',
enum: ['account', 'user', 'role'],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aayushchouhan09 in you PR can you define it in common_api so it can be reused here and in your changes?

@coderabbitai coderabbitai Bot mentioned this pull request Aug 14, 2026
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants