Fix role creation failure for restapi:admin/* permissions

[itsmevichu]

Description

This PR allows superadmin user to create roles with restapi:admin/* permissions.

• Category: Bug fix
• Why these changes are required?
  Creating a role with restapi:admin/* permissions was failing with an `Access denied error, even when the request was made by a superadmin user;
• What is the old behavior before changes and new behavior after changes?
  • Before:
    • Creating a role containing restapi:admin/* permissions would fail with Access denied, even when the request was made by a superadmin user.
  • After:
    • Superadmin users can successfully create roles containing restapi:admin/* permissions.

Issues Resolved

#6178

Testing

Unit testing and manual testing.

Check List

• New functionality includes testing
• New functionality has been documented
• New Roles/Permissions have a corresponding security dashboards plugin PR
• API changes companion pull request created
• Commits are signed per the DCO using --signoff

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.

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit b85cfd9.

Path	Line	Severity	Description
src/main/java/org/opensearch/security/dlic/rest/validation/EndpointValidator.java	163	low	Super admin bypass added to isAllowedToChangeEntityWithRestAdminPermissions: any user recognized as a super admin via adminDNs can skip the REST admin permission checks entirely. The check itself uses the established adminDNs certificate mechanism (high trust), so this is likely intentional, but it creates a broader privilege escalation path if admin certificates are ever compromised. No signs of malicious intent — the logic is consistent, tests cover both branches, and no external calls or obfuscation are present.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 0 | Low: 1

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 39e7667)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ No major issues detected

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 39e7667

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Prevent NPE when retrieving current user Guard against a null userAndRemoteAddress pair to avoid a potential NullPointerException if Utils.userAndRemoteAddressFrom returns null. This is consistent with defensive null-handling used elsewhere when retrieving the current user from the thread context. src/main/java/org/opensearch/security/dlic/rest/api/RestApiAuthorizationEvaluator.java [258-261] public boolean isCurrentUserSuperAdmin() { final Pair<User, TransportAddress> userAndRemoteAddress = Utils.userAndRemoteAddressFrom(threadContext); - return userAndRemoteAddress.getLeft() != null && adminDNs.isAdmin(userAndRemoteAddress.getLeft()); + return userAndRemoteAddress != null + && userAndRemoteAddress.getLeft() != null + && adminDNs.isAdmin(userAndRemoteAddress.getLeft()); } Suggestion importance[1-10]: 3 __ Why: Utils.userAndRemoteAddressFrom typically returns a non-null Pair, and existing usages in the same file (e.g., isCurrentUserAdminFor) don't null-check the pair itself. The defensive check is minor and likely unnecessary.	Low

---

Previous suggestions

Suggestions up to commit f719d6e

Category	Suggestion	Impact
General	Add null check for safety Guard against a potential NullPointerException when userAndRemoteAddressFrom returns null. While unlikely, defensively checking the pair itself prevents an unexpected NPE that could break the role creation flow for super admins. src/main/java/org/opensearch/security/dlic/rest/api/RestApiAuthorizationEvaluator.java [258-261] public boolean isCurrentUserSuperAdmin() { final Pair<User, TransportAddress> userAndRemoteAddress = Utils.userAndRemoteAddressFrom(threadContext); - return userAndRemoteAddress.getLeft() != null && adminDNs.isAdmin(userAndRemoteAddress.getLeft()); + return userAndRemoteAddress != null + && userAndRemoteAddress.getLeft() != null + && adminDNs.isAdmin(userAndRemoteAddress.getLeft()); } Suggestion importance[1-10]: 2 __ Why: Utils.userAndRemoteAddressFrom is used elsewhere in the codebase without null checks (as shown by the same pattern usage in this PR's context), suggesting it doesn't return null. The defensive check offers minimal value.	Low

Suggestions up to commit d4caeab

Category	Suggestion	Impact
Possible issue	Add null check for adminDNs Add null safety check for adminDNs before calling isAdmin(). If adminDNs is null, the method will throw a NullPointerException, causing the authorization check to fail unexpectedly. src/main/java/org/opensearch/security/dlic/rest/api/RestApiAuthorizationEvaluator.java [258-261] public boolean isCurrentUserSuperAdmin() { final Pair<User, TransportAddress> userAndRemoteAddress = Utils.userAndRemoteAddressFrom(threadContext); - return userAndRemoteAddress.getLeft() != null && adminDNs.isAdmin(userAndRemoteAddress.getLeft()); + return userAndRemoteAddress.getLeft() != null && adminDNs != null && adminDNs.isAdmin(userAndRemoteAddress.getLeft()); } Suggestion importance[1-10]: 7 __ Why: Adding a null check for adminDNs prevents potential NullPointerException in the authorization flow. However, this is a defensive programming practice rather than a critical bug fix, as adminDNs is likely initialized during object construction in the existing codebase.	Medium

Suggestions up to commit b85cfd9

Category	Suggestion	Impact
Possible issue	Add null check for adminDNs Add null safety check for adminDNs before calling isAdmin(). If adminDNs is null, the method will throw a NullPointerException when attempting to verify admin status. src/main/java/org/opensearch/security/dlic/rest/api/RestApiAuthorizationEvaluator.java [258-261] public boolean isCurrentUserSuperAdmin() { final Pair<User, TransportAddress> userAndRemoteAddress = Utils.userAndRemoteAddressFrom(threadContext); - return userAndRemoteAddress.getLeft() != null && adminDNs.isAdmin(userAndRemoteAddress.getLeft()); + return userAndRemoteAddress.getLeft() != null && adminDNs != null && adminDNs.isAdmin(userAndRemoteAddress.getLeft()); } Suggestion importance[1-10]: 7 __ Why: Adding a null check for adminDNs prevents potential NullPointerException. However, if adminDNs is a required dependency that should never be null in normal operation, this might mask initialization issues rather than fix them.	Medium
General	Replace real method calls in mocks The mock setup calls thenCallRealMethod() which may invoke actual implementation code during testing. This could lead to unexpected behavior or test failures if the real methods have dependencies. Consider using thenReturn() with appropriate values instead. src/test/java/org/opensearch/security/dlic/rest/api/RolesApiActionValidationTest.java [65-70] @Test public void nonSuperAdminIsNotAllowedToCreateRoleWithRestAdminPermissions() throws Exception { when(restApiAuthorizationEvaluator.isCurrentUserSuperAdmin()).thenReturn(false); Mockito.doReturn(CType.ROLES).when(configuration).getCType(); - when(configuration.getImplementingClass()).thenCallRealMethod(); - when(restApiAuthorizationEvaluator.containsRestApiAdminPermissions(any(Object.class))).thenCallRealMethod(); + when(configuration.getImplementingClass()).thenReturn(RoleV7.class); + when(restApiAuthorizationEvaluator.containsRestApiAdminPermissions(any(Object.class))).thenReturn(true); ... } Suggestion importance[1-10]: 4 __ Why: While using thenCallRealMethod() can introduce dependencies in tests, the suggestion to replace with hardcoded return values may not accurately test the actual behavior. The current approach might be intentional to test integration between components.	Low

[github-actions]

Persistent review updated to latest commit d4caeab

[DarshitChanpura]

Changes LGTM. Will approve once CI is green

[github-actions]

Persistent review updated to latest commit f719d6e

[github-actions]

Persistent review updated to latest commit 39e7667