Fix NPE on null user in RP code path

[DarshitChanpura]

Description

Resolves NPE thrown when user is null and RP code path is executed:

[2025-11-12T20:39:52,966][WARN ][r.suppressed             ][15e5649158172924e763e324589e0541] path: /_plugins/_ml/models/_register, params: {}
java.lang.NullPointerException: Cannot invoke "org.opensearch.commons.authuser.User.getName()" because "user" is null
    at org.opensearch.ml.helper.ModelAccessControlHelper.lambda$validateModelGroupAccess$4(ModelAccessControlHelper.java:184)
    at org.opensearch.core.action.ActionListener$1.onResponse(ActionListener.java:82)
    at org.opensearch.security.resources.ResourceAccessHandler.lambda$hasPermission$0(ResourceAccessHandler.java:185)
    at org.opensearch.core.action.ActionListener$1.onResponse(ActionListener.java:82)
    at org.opensearch.security.resources.ResourceSharingIndexHandler.lambda$fetchSharingInfo$24(ResourceSharingIndexHandler.java:579)
    at org.opensearch.core.action.ActionListener$1.onResponse(ActionListener.java:82)
    at org.opensearch.action.support.TransportAction$1.onResponse(TransportAction.java:115)
    at org.opensearch.action.support.TransportAction$1.onResponse(TransportAction.java:109)
    at org.opensearch.action.support.single.shard.TransportSingleShardAction$AsyncSingleAction$2.handleResponse(TransportSingleShardAction.java:298)
    at org.opensearch.action.support.single.shard.TransportSingleShardAction$AsyncSingleAction$2.handleResponse(TransportSingleShardAction.java:284)
    at org.opensearch.security.transport.SecurityInterceptor$RestoringTransportResponseHandler.handleResponse(SecurityInterceptor.java:430)
    at org.opensearch.transport.TransportService$ContextRestoreResponseHandler.handleResponse(TransportService.java:1587)
    at org.opensearch.transport.TransportService$DirectResponseChannel.processResponse(TransportService.java:1680)
    at org.opensearch.transport.TransportService$DirectResponseChannel.sendResponse(TransportService.java:1660)
    at org.opensearch.transport.TaskTransportChannel.sendResponse(TaskTransportChannel.java:72)
    at org.opensearch.action.support.ChannelActionListener.onResponse(ChannelActionListener.java:62)
    at org.opensearch.action.support.ChannelActionListener.onResponse(ChannelActionListener.java:45)
    at org.opensearch.action.ActionRunnable.lambda$supply$0(ActionRunnable.java:74)
    at org.opensearch.action.ActionRunnable$2.doRun(ActionRunnable.java:89)
    at org.opensearch.common.util.concurrent.ThreadContext$ContextPreservingAbstractRunnable.doRun(ThreadContext.java:984)
    at org.opensearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:52)
    at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
    at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
    at java.base/java.lang.Thread.run(Thread.java:1583)

We are updating migrate API such that all resources with no owners will now have a "default_owner" to be supplied upon calling migrate API.
(opensearch-project/security#5789)

These resources will not have public access by default. Instead will have to be shared publicly.

Related Issues

Check List

• New functionality includes testing.
• New functionality has been documented.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Summary by CodeRabbit

Release Notes

• Bug Fixes

  • Improved stability of access control error messages by implementing safe handling for scenarios where user information may be unavailable, preventing potential application issues.

• Tests

  • Added comprehensive test coverage for model group access denial scenarios, including edge cases with missing or present user information.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[brianf-aws]

Thank you for providing the fix! Do we need any backport labels?

Lets also monitor the CI.

[ylwu-amzn]

If user is null and model group is public, does the user have access or not ?

[DarshitChanpura]

no.. null users are not granted access..

if you are talking from anonymous user login perspective then yes since anonymous users can access resource in two ways:

1. Resource is shared with anonymous user or backend role (https://github.com/opensearch-project/security/blob/9e6047f99da4df3404e2d52f3afe4e49e508c3a5/src/main/java/org/opensearch/security/auth/BackendRegistry.java#L480)
2. Resource is marked as public by following the new convention (https://github.com/opensearch-project/security/blob/main/RESOURCE_SHARING_AND_ACCESS_CONTROL.md#example-publicly-shared-resource)

[coderabbitai]

Walkthrough

This pull request adds null-safety checks when constructing error messages for denied model group access. The change prevents potential NullPointerException when a user object is null by wrapping user.getName() calls with a null check. Two corresponding test methods verify correct behavior for both null and non-null user scenarios.

Changes

Cohort / File(s)	Change Summary
Null-safety in access denial handling plugin/src/main/java/org/opensearch/ml/helper/ModelAccessControlHelper.java	Modified two validateModelGroupAccess code paths to safely handle null user objects when constructing error messages by wrapping user.getName() with null checks to prevent NullPointerException
Access authorization tests plugin/src/test/java/org/opensearch/ml/helper/ModelAccessControlHelperTests.java	Added two test methods: test_ResourceAuthz_NotAuthorized_UserNull_UsesUnknownName (verifies behavior when user is null) and test_ResourceAuthz_NotAuthorized_UserPresent_UsesUserName (verifies behavior when user is present) to validate access denial message construction

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

• Straightforward null-safety pattern applied consistently across two locations
• Two new test methods follow established testing patterns with mock setup and assertion verification
• No complex logic or control flow changes

Poem

🐰✨ A null that once would crash and burn,
Now safely caught at every turn—
With gentle checks, we guard the way,
No NPE shall win the day!
Tests confirm our rabbit's care. 🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.
Description check	❓ Inconclusive	The PR description includes details about the NPE issue, affected code location, and context about the resource migration behavior change. However, the 'Related Issues' section is empty without an issue reference, and most checklist items remain unchecked (tests, documentation, API changes, public docs).	Add the specific issue number being resolved to the 'Related Issues' section (e.g., 'Resolves #[issue-number]') to complete the description template.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Fix NPE on null user in RP code path' directly and clearly describes the main fix: preventing a NullPointerException when a null user occurs in the Resource Permissions code path, which matches the primary code change.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[DarshitChanpura]

@ylwu-amzn @dhrubo-os Can we get this merged into 3.4?

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

plugin/src/main/java/org/opensearch/ml/helper/ModelAccessControlHelper.java (2)

109-109: Null-safety fix prevents NPE in error message construction.

The ternary operator correctly guards against NPE when user is null. This fix addresses the issue described in the PR where User.getName() was being invoked on a null user object in the resource authorization code path.

However, the error message will now contain "User null is not authorized" which may not be the most user-friendly message for operators debugging access issues.

Consider using a more descriptive identifier for null users:

-                                    + (user == null ? null : user.getName())
+                                    + (user == null ? "<anonymous>" : user.getName())

This would produce "User <anonymous> is not authorized" instead of "User null is not authorized", making the error message clearer for troubleshooting.

Also applies to: 185-185

---

176-176: Remove unnecessary blank line.

This blank line doesn't add meaningful separation and can be removed to maintain consistency with the rest of the codebase.

-
 
         if (shouldUseResourceAuthz(ML_MODEL_GROUP_RESOURCE_TYPE)) {

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6de6889 and 9c5d9c1.

📒 Files selected for processing (2)

• plugin/src/main/java/org/opensearch/ml/helper/ModelAccessControlHelper.java (3 hunks)
• plugin/src/test/java/org/opensearch/ml/helper/ModelAccessControlHelperTests.java (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

plugin/src/test/java/org/opensearch/ml/helper/ModelAccessControlHelperTests.java (2)

common/src/main/java/org/opensearch/ml/common/CommonValue.java (1)

• CommonValue (14-150)

common/src/main/java/org/opensearch/ml/common/ResourceSharingClientAccessor.java (1)

• ResourceSharingClientAccessor (13-42)

🔇 Additional comments (1)

plugin/src/test/java/org/opensearch/ml/helper/ModelAccessControlHelperTests.java (1)

490-552: Comprehensive test coverage for null-safety fix.

These two test methods properly validate the null-safety fix in both scenarios:

1. When user is null, the error message correctly contains "User null is not authorized"
2. When user is present, the error message correctly contains the actual username

The tests follow the existing patterns in the test class and properly clean up by resetting the ResourceSharingClient to avoid side effects on other tests.

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 80.25%. Comparing base (6de6889) to head (9c5d9c1).

Files with missing lines	Patch %	Lines
...opensearch/ml/helper/ModelAccessControlHelper.java	50.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #4421      +/-   ##
============================================
+ Coverage     80.22%   80.25%   +0.03%     
- Complexity    10245    10259      +14     
============================================
  Files           858      858              
  Lines         44552    44552              
  Branches       5158     5160       +2     
============================================
+ Hits          35742    35756      +14     
+ Misses         6639     6624      -15     
- Partials       2171     2172       +1     

Flag	Coverage Δ
ml-commons	80.25% <50.00%> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.