Combine RestApiPrivilegesEvaluator and RestApiAdminPrivilegesEvaluator to RestApiAuthorizationEvaluator

[cwperks]

Description

This PR contains a refactoring to simplify authz for security APIs.

Currently, authorization is split into 2 files:

1. RestApiPrivilegesEvaluator - For when plugins.security.restapi.admin.enabled is set to true which authorizes security APIs based on whether the user has explicitly been granted the requisite restapi:* permission
2. RestApiAdminPrivilegesEvaluator - For when plugins.security.restapi.roles_enabled is set which authorizes security APIs based on the user's roles

• Category

Refactoring

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.

[codecov]

Codecov Report

❌ Patch coverage is 67.97386% with 49 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.79%. Comparing base (4d6f34f) to head (69d1fa3).

Files with missing lines	Patch %	Lines
...y/dlic/rest/api/RestApiAuthorizationEvaluator.java	69.74%	22 Missing and 14 partials ⚠️
.../security/dlic/rest/api/PermissionsInfoAction.java	33.33%	2 Missing ⚠️
...i/migrate/MigrateResourceSharingInfoApiAction.java	0.00%	2 Missing ⚠️
...arch/security/dlic/rest/api/AbstractApiAction.java	75.00%	1 Missing ⚠️
...rch/security/dlic/rest/api/AllowlistApiAction.java	0.00%	1 Missing ⚠️
...security/dlic/rest/api/ConfigUpgradeApiAction.java	0.00%	1 Missing ⚠️
...ity/dlic/rest/api/MultiTenancyConfigApiAction.java	0.00%	1 Missing ⚠️
...earch/security/dlic/rest/api/NodesDnApiAction.java	0.00%	1 Missing ⚠️
.../security/dlic/rest/api/RateLimitersApiAction.java	0.00%	1 Missing ⚠️
...curity/dlic/rest/api/RollbackVersionApiAction.java	0.00%	1 Missing ⚠️
... and 2 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6072      +/-   ##
==========================================
- Coverage   74.83%   74.79%   -0.05%     
==========================================
  Files         447      446       -1     
  Lines       28470    28456      -14     
  Branches     4327     4326       -1     
==========================================
- Hits        21306    21283      -23     
- Misses       5172     5180       +8     
- Partials     1992     1993       +1     

Files with missing lines	Coverage Δ
.../opensearch/security/OpenSearchSecurityPlugin.java	85.01% <ø> (ø)
...earch/security/dlic/rest/api/AccountApiAction.java	98.68% <100.00%> (ø)
.../security/dlic/rest/api/ActionGroupsApiAction.java	98.27% <100.00%> (ø)
...nsearch/security/dlic/rest/api/AuditApiAction.java	91.04% <100.00%> (-2.99%)	⬇️
.../security/dlic/rest/api/CertificatesApiAction.java	80.00% <100.00%> (ø)
...security/dlic/rest/api/InternalUsersApiAction.java	93.54% <100.00%> (ø)
...nsearch/security/dlic/rest/api/RolesApiAction.java	96.07% <100.00%> (ø)
.../security/dlic/rest/api/RolesMappingApiAction.java	97.29% <100.00%> (ø)
...ecurity/dlic/rest/api/SecurityApiDependencies.java	93.33% <100.00%> (-0.79%)	⬇️
...security/dlic/rest/api/SecurityRestApiActions.java	88.88% <100.00%> (-1.12%)	⬇️
... and 15 more

... and 6 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 69d1fa3)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Merge RestApiPrivilegesEvaluator and RestApiAdminPrivilegesEvaluator into RestApiAuthorizationEvaluator Relevant files: src/main/java/org/opensearch/security/dlic/rest/api/RestApiAuthorizationEvaluator.java src/main/java/org/opensearch/security/dlic/rest/api/SecurityApiDependencies.java src/main/java/org/opensearch/security/dlic/rest/api/SecurityRestApiActions.java src/test/java/org/opensearch/security/dlic/rest/api/RestApiAuthorizationEvaluatorTest.java Sub-PR theme: Update all API action classes to use RestApiAuthorizationEvaluator Relevant files: src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java src/main/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiAction.java src/main/java/org/opensearch/security/dlic/rest/api/AllowlistApiAction.java src/main/java/org/opensearch/security/dlic/rest/api/AuditApiAction.java src/main/java/org/opensearch/security/dlic/rest/api/CertificatesApiAction.java src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java src/main/java/org/opensearch/security/dlic/rest/api/MultiTenancyConfigApiAction.java src/main/java/org/opensearch/security/dlic/rest/api/NodesDnApiAction.java src/main/java/org/opensearch/security/dlic/rest/api/RateLimitersApiAction.java src/main/java/org/opensearch/security/dlic/rest/api/RolesApiAction.java src/main/java/org/opensearch/security/dlic/rest/api/RolesMappingApiAction.java src/main/java/org/opensearch/security/dlic/rest/api/RollbackVersionApiAction.java src/main/java/org/opensearch/security/dlic/rest/api/SecurityConfigApiAction.java src/main/java/org/opensearch/security/dlic/rest/api/SecuritySSLCertsApiAction.java src/main/java/org/opensearch/security/dlic/rest/api/TenantsApiAction.java src/main/java/org/opensearch/security/dlic/rest/api/ViewVersionApiAction.java src/main/java/org/opensearch/security/dlic/rest/validation/EndpointValidator.java src/main/java/org/opensearch/security/resources/api/migrate/MigrateResourceSharingInfoApiAction.java

⚡ Recommended focus areas for review Null PrivilegesConfiguration The constructor accepts privilegesConfiguration as a parameter and stores it, but isCurrentUserAdminFor calls privilegesConfiguration.privilegesEvaluator() without null-checking. In the test setup (RestApiAuthorizationEvaluatorTest), null is passed for privilegesConfiguration, which would cause a NullPointerException if isCurrentUserAdminFor is called with restapiAdminEnabled=true. final PrivilegesEvaluationContext context = privilegesConfiguration.privilegesEvaluator() .createContext(userAndRemoteAddress.getLeft(), permission); final boolean hasAccess = context.getActionPrivileges().hasExplicitClusterPrivilege(context, permission).isAllowed(); Missing continue on parse error In parseDisabledEndpoints, when value.getValue() is not a Collection, the code logs an error but does not continue to the next entry. It then falls through to iterate over (Collection) value.getValue(), which will throw a ClassCastException at runtime. if (value.getValue() instanceof Collection == false) { logger.error( "Disabled HTTP methods of endpoint '{}' must be an array, actually is '{}', skipping.", endpointString, (value.getValue().toString()) ); } final List<Method> disabledMethods = new LinkedList<>(); for (Object disabledMethodObj : (Collection) value.getValue()) { Thread-safety concern disabledEndpointsForUsers is a HashMap that is written to lazily in getDisabledEndpointsForCurrentUser without synchronization. This map is shared across threads and could lead to race conditions or data corruption under concurrent access. if (disabledEndpointsForUsers.containsKey(userPrincipal)) { return disabledEndpointsForUsers.get(userPrincipal); } if (!currentUserHasRestApiAccess(userRoles)) { return this.allEndpoints; } final Map<Endpoint, List<Method>> finalEndpoints = new HashMap<>(); final List<Endpoint> remainingEndpoints = new LinkedList<>(Arrays.asList(Endpoint.values())); boolean hasDisabledEndpoints = false; for (String userRole : userRoles) { final Map<Endpoint, List<Method>> endpointsForRole = disabledEndpointsForRoles.get(userRole); if (endpointsForRole == null || endpointsForRole.isEmpty()) { continue; } remainingEndpoints.retainAll(endpointsForRole.keySet()); hasDisabledEndpoints = true; } if (isDebugEnabled) { logger.debug("Remaining endpoints for user {} after retaining all : {}", userPrincipal, remainingEndpoints); } if (hasDisabledEndpoints == false) { if (isDebugEnabled) { logger.debug( "No disabled endpoints for user {} at all, only globally disabledendpoints apply.", userPrincipal, remainingEndpoints ); } disabledEndpointsForUsers.put(userPrincipal, addGloballyDisabledEndpoints(finalEndpoints)); return finalEndpoints; } for (Endpoint endpoint : remainingEndpoints) { final List<Method> remainingMethodsForEndpoint = new LinkedList<>(Arrays.asList(Method.values())); for (String userRole : userRoles) { final Map<Endpoint, List<Method>> endpoints = disabledEndpointsForRoles.get(userRole); if (endpoints != null && endpoints.isEmpty() == false) { remainingMethodsForEndpoint.retainAll(endpoints.get(endpoint)); } } finalEndpoints.put(endpoint, remainingMethodsForEndpoint); } if (isDebugEnabled) { logger.debug("Disabled endpoints for user {} after retaining all : {}", userPrincipal, finalEndpoints); } addGloballyDisabledEndpoints(finalEndpoints); disabledEndpointsForUsers.put(userPrincipal, finalEndpoints); if (isDebugEnabled) { logger.debug( "Disabled endpoints for user {} after retaining all : {}", userPrincipal, disabledEndpointsForUsers.get(userPrincipal) ); } return disabledEndpointsForUsers.get(userPrincipal); Duplicate instantiation PermissionsInfoAction creates its own RestApiAuthorizationEvaluator instance in its constructor, while SecurityRestApiActions.getHandler already creates one and passes it via SecurityApiDependencies. This results in two separate instances with potentially different state (e.g., separate disabledEndpointsForUsers caches). this.restApiAuthorizationEvaluator = new RestApiAuthorizationEvaluator( settings, adminDNs, roleMapper, principalExtractor, configPath, threadPool, privilegesConfiguration );

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 69d1fa3
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add missing continue to prevent ClassCastException When value.getValue() is not a Collection, the code logs an error but then falls through and attempts to cast it to Collection, which will throw a ClassCastException. A continue statement should be added after the error log to skip processing this entry. src/main/java/org/opensearch/security/dlic/rest/api/RestApiAuthorizationEvaluator.java [368-377] if (value.getValue() instanceof Collection == false) { logger.error( "Disabled HTTP methods of endpoint '{}' must be an array, actually is '{}', skipping.", endpointString, (value.getValue().toString()) ); + continue; } final List<Method> disabledMethods = new LinkedList<>(); for (Object disabledMethodObj : (Collection) value.getValue()) { Suggestion importance[1-10]: 8 __ Why: The missing continue after the error log for non-Collection values causes a ClassCastException at runtime. This is a real bug that existed in the original RestApiPrivilegesEvaluator code and was carried over to the merged class without being fixed.	Medium
	Replace null with mock to prevent NullPointerException Passing null for privilegesConfiguration will cause a NullPointerException at runtime when isCurrentUserAdminFor is called, since the merged class now calls privilegesConfiguration.privilegesEvaluator(). A mock of PrivilegesConfiguration should be provided instead. src/test/java/org/opensearch/security/dlic/rest/api/RestApiAuthorizationEvaluatorTest.java [36-44] this.privilegesEvaluator = new RestApiAuthorizationEvaluator( Settings.EMPTY, mock(AdminDNs.class), (user, caller) -> user.getSecurityRoles(), mock(PrincipalExtractor.class), mock(Path.class), mock(ThreadPool.class), - null + mock(PrivilegesConfiguration.class) ); Suggestion importance[1-10]: 6 __ Why: Passing null for privilegesConfiguration could cause a NullPointerException if isCurrentUserAdminFor is called in tests, since the merged class calls privilegesConfiguration.privilegesEvaluator(). However, the test class may only test role/cert-based access methods that don't invoke privilegesConfiguration, so the actual risk depends on which test methods exist.	Low

---

Previous suggestions

Suggestions up to commit 109df00

Category	Suggestion	Impact
Possible issue	Add missing continue to prevent ClassCastException When value.getValue() is not a Collection, the code logs an error but does not continue to skip the entry. The subsequent unchecked cast (Collection) value.getValue() will throw a ClassCastException at runtime. A continue statement should be added after the error log. src/main/java/org/opensearch/security/dlic/rest/api/RestApiAuthorizationEvaluator.java [368-377] if (value.getValue() instanceof Collection == false) { logger.error( "Disabled HTTP methods of endpoint '{}' must be an array, actually is '{}', skipping.", endpointString, (value.getValue().toString()) ); + continue; } final List<Method> disabledMethods = new LinkedList<>(); for (Object disabledMethodObj : (Collection) value.getValue()) { Suggestion importance[1-10]: 8 __ Why: This is a real bug carried over from the original RestApiPrivilegesEvaluator code - when value.getValue() is not a Collection, the error is logged but execution falls through to an unchecked (Collection) cast that will throw a ClassCastException. Adding continue is necessary to match the intended "skipping" behavior described in the error message.	Medium
	Replace null with a mock for required dependency Passing null for privilegesConfiguration in the test constructor will cause a NullPointerException if any test exercises isCurrentUserAdminFor, since the merged class now calls privilegesConfiguration.privilegesEvaluator(). A mock of PrivilegesConfiguration should be provided instead. src/test/java/org/opensearch/security/dlic/rest/api/RestApiAuthorizationEvaluatorTest.java [36-44] +final PrivilegesConfiguration privilegesConfiguration = mock(PrivilegesConfiguration.class); this.privilegesEvaluator = new RestApiAuthorizationEvaluator( Settings.EMPTY, mock(AdminDNs.class), (user, caller) -> user.getSecurityRoles(), mock(PrincipalExtractor.class), mock(Path.class), mock(ThreadPool.class), - null + privilegesConfiguration ); Suggestion importance[1-10]: 6 __ Why: Passing null for privilegesConfiguration could cause a NullPointerException if tests exercise isCurrentUserAdminFor. However, the existing tests in this file may only test role-based access methods that don't call privilegesConfiguration, so the actual impact depends on test coverage. The suggestion is valid but may only matter for specific test scenarios.	Low

[github-actions]

Persistent review updated to latest commit 69d1fa3