Add pagination support for Collection APIs

[itsmevichu]

[Drafted for now - working on tests]

Description

[Describe what this change achieves]

• Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)
• Why these changes are required?
• What is the old behavior before changes and new behavior after changes?

Issues Resolved

#6339

Is this a backport? If so, please add backport PR # and/or commits #, and remove backport-failed label from the original PR.

Do these changes introduce new permission(s) to be displayed in the static dropdown on the front-end? If so, please open a draft PR in the security dashboards plugin and link the draft PR here

Testing

[Please provide details of testing done: unit testing, integration 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 decd858.

⛔ Hard block: Issues at High severity or above will block this PR from merging.

Path	Line	Severity	Description
src/main/java/org/opensearch/security/dlic/rest/api/pagination/PaginationRequestParser.java	65	medium	No upper bound is enforced on the page size parameter. A caller can supply size=Integer.MAX_VALUE (or any arbitrarily large value), causing Paginator to allocate a correspondingly large list and sort the entire entry set, enabling resource exhaustion / memory-pressure DoS against the security configuration endpoint.
src/main/java/org/opensearch/security/dlic/rest/api/pagination/PaginationCursor.java	57	medium	The pagination cursor is plain Base64url-encoded JSON with no HMAC or cryptographic signature. Any authenticated caller can forge an arbitrary cursor (e.g., set last_key to a known prefix) to enumerate configuration entries in a controlled order, bypassing the intended opaque-token contract and enabling efficient key enumeration of sensitive security configuration objects.
src/main/java/org/opensearch/security/dlic/rest/api/pagination/PaginationCursor.java	104	low	The catch-all exception handler propagates e.getMessage() verbatim to the HTTP 400 response body. Depending on the underlying Jackson parse error, this can expose internal class names, stack details, or input content in the error response visible to the caller.

The table above displays the top 10 most important findings.

Total: 3 | Critical: 0 | High: 0 | Medium: 2 | 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 3585239)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review NPE on Invalid Cursor In processPaginatedGetRequest, when params.hasCursor() is true and PaginationCursor.decode(...) returns a ValidationResult in error state, the outer .map(cursor -> buildPaginatedPage(...)) invocation on an error result would not invoke the mapper, but the return type is a nested ValidationResult<ValidationResult<ToXContent>>. This nested wrapping may not be flattened properly by the outer chain, potentially causing the error response to be lost or the successful path to leak the inner ValidationResult as ToXContent. Verify the type flow produces a valid ValidationResult<ToXContent> in both success and error cases. if (params.hasCursor()) { return PaginationCursor.decode(params.nextToken, getConfigType(), params.sort) .map(cursor -> buildPaginatedPage(configuration, params, cursor)); } return buildPaginatedPage(configuration, params, null); }); Performance/Correctness toXContent serializes entries by first writing them to a JSON string via DefaultObjectMapper.writeValueAsString, then reading back into a Map, and finally passing to builder.field. This double serialization is inefficient for large pages and may lose type fidelity (e.g., non-string keys, custom serializers configured on entries). Consider writing entries directly via the builder or using the entries' native toXContent if available. @SuppressWarnings("unchecked") final Map<String, ?> serialisable = DefaultObjectMapper.readValue( DefaultObjectMapper.writeValueAsString(entries, false), Map.class ); builder.field(resourceKey, serialisable); Ordering Dependency withPaginatedGetRequest captures the currently registered GET handler as the "legacy" fallback. This creates a hidden ordering requirement: onGetRequest must be called before withPaginatedGetRequest, otherwise legacyHandler will be null and invoking a non-paginated GET will NPE. If callers in the future reorder these calls (or forget to register onGetRequest), the failure mode is a runtime NPE rather than a clear error. Consider validating that legacyHandler != null and failing fast at builder time. // fall through to it when the override returns null. final RequestHandler legacyHandler = requestHandlers.get(RestRequest.Method.GET); add(RestRequest.Method.GET, (channel, request, client) -> { final ValidationResult<ToXContent> result = mapper.apply(request); if (result != null) { result.valid(toXContent -> ok(channel, toXContent)) .error((status, toXContent) -> response(channel, status, toXContent)); } else { legacyHandler.handle(channel, request, client); } }); return this; Sort Semantics Pagination sorts and filters keys using String.compareTo, which is locale-independent codepoint ordering. If entity names contain non-ASCII characters or mixed case, the ordering may surprise clients and, more importantly, may not match ordering used elsewhere in the API. Additionally, since the cursor stores last_key as a plain string but the resume comparison also uses compareTo, any inconsistency in how keys are compared between requests (or after config changes) could cause entries to be skipped or duplicated. Document the ordering guarantee explicitly. Stream<Map.Entry<String, T>> entryStream = allEntries.entrySet().stream(); if (lastKey != null && !lastKey.isEmpty()) { entryStream = entryStream.filter(entry -> { final int cmp = entry.getKey().compareTo(lastKey); return isDesc ? cmp < 0 : cmp > 0; }); } // Sort the filtered items Comparator<Map.Entry<String, T>> comparator = Map.Entry.comparingByKey(); if (isDesc) { comparator = comparator.reversed(); } final int targetSize = params.size; final List<Map.Entry<String, T>> candidatePage = entryStream.sorted(comparator).limit(targetSize + 1L).collect(Collectors.toList());

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 3585239
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Preserve entry ordering in serialization Serializing entries via a JSON round-trip (writeValueAsString → readValue into a HashMap) both loses the ordering guaranteed by the upstream LinkedHashMap from Paginator and is unnecessarily expensive. Since Jackson's HashMap deserialization does not preserve insertion order, page results may be emitted in arbitrary order, breaking the ascending/descending contract. Serialize directly (or into a LinkedHashMap) to preserve ordering. src/main/java/org/opensearch/security/dlic/rest/api/pagination/PaginationResult.java [63-68] -@SuppressWarnings("unchecked") final Map<String, ?> serialisable = DefaultObjectMapper.readValue( DefaultObjectMapper.writeValueAsString(entries, false), - Map.class + new com.fasterxml.jackson.core.type.TypeReference<java.util.LinkedHashMap<String, Object>>() {} ); builder.field(resourceKey, serialisable); Suggestion importance[1-10]: 8 __ Why: Legitimate correctness concern: deserializing into a raw Map.class (HashMap) loses the insertion order that pagination depends on, potentially breaking the ascending/descending ordering contract in the serialized response.	Medium
	Guard against missing legacy GET handler If withPaginatedGetRequest is called before onGetRequest (as happens in InternalUsersApiAction where it is chained after other handlers), legacyHandler may be null, leading to a NullPointerException on non-paginated GETs. Guard against a missing legacy handler or document/enforce ordering, and consider throwing a clear error at registration time if no legacy GET handler exists. src/main/java/org/opensearch/security/dlic/rest/api/RequestHandler.java [150-167] public RequestHandlersBuilder withPaginatedGetRequest( final CheckedFunction<RestRequest, ValidationResult<ToXContent>, IOException> mapper ) { Objects.requireNonNull(mapper, "withPaginatedGetRequest handler can't be null"); - // Capture the legacy handler that was registered by onGetRequest so we can - // fall through to it when the override returns null. final RequestHandler legacyHandler = requestHandlers.get(RestRequest.Method.GET); + Objects.requireNonNull(legacyHandler, "withPaginatedGetRequest requires an onGetRequest handler to be registered first"); add(RestRequest.Method.GET, (channel, request, client) -> { final ValidationResult<ToXContent> result = mapper.apply(request); if (result != null) { result.valid(toXContent -> ok(channel, toXContent)) .error((status, toXContent) -> response(channel, status, toXContent)); } else { legacyHandler.handle(channel, request, client); } }); return this; } Suggestion importance[1-10]: 6 __ Why: Valid observation: if withPaginatedGetRequest is called before onGetRequest, legacyHandler will be null, causing NPE on non-paginated GETs. Adding a Objects.requireNonNull improves fail-fast behavior, though current usages appear to register handlers in the correct order.	Low
General	Avoid null return for fall-through Returning null from a method declared to return ValidationResult is fragile and relies on the caller (withPaginatedGetRequest) treating null as "fall through". This is easy to misuse and produces a NullPointerException if the contract changes. Consider a more explicit signaling mechanism (e.g., Optional, or a dedicated sentinel/enum) so the fall-through path is not tied to nullability of a ValidationResult. src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java [142-147] -protected ValidationResult<ToXContent> routeGetRequest(final RestRequest request) throws IOException { +protected Optional<ValidationResult<ToXContent>> routeGetRequest(final RestRequest request) throws IOException { if (PaginationRequestParser.isPaginationRequested(request)) { - return processPaginatedGetRequest(request); + return Optional.of(processPaginatedGetRequest(request)); } - return null; + return Optional.empty(); } Suggestion importance[1-10]: 3 __ Why: A stylistic improvement for API clarity; using Optional would be cleaner but current null-return pattern is functional and internally consistent.	Low
	Relax strict-inequality ordering assertion Using compareTo(...) < 0 (strictly less) will fail if any two consecutive keys are equal, but more importantly this local ordering check does not guarantee that pages themselves were returned in order — only within the accumulated list. Consider verifying page boundaries too or at least allow <= 0 to avoid false negatives if duplicates ever appear. src/integrationTest/java/org/opensearch/security/api/PaginationRestApiIntegrationTest.java [106-112] for (int i = 1; i < allSeen.size(); i++) { assertThat( allSeen.get(i - 1) + " must sort before " + allSeen.get(i), - allSeen.get(i - 1).compareTo(allSeen.get(i)) < 0, + allSeen.get(i - 1).compareTo(allSeen.get(i)) <= 0, is(true) ); } Suggestion importance[1-10]: 2 __ Why: Keys are unique entity names, so strict inequality is actually the correct assertion. Relaxing to <= would weaken the test rather than improve it.	Low

---

Previous suggestions

Suggestions up to commit decd858

Category	Suggestion	Impact
Security	Apply config-load validation before paginating The paginated GET path bypasses endpointValidator.onConfigLoad and the entity-filtering logic used by processGetRequest, meaning results may not be redacted/authorized consistently with the single-entity path. Apply onConfigLoad (or an equivalent hook) to the loaded configuration before paginating. src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java [182-188] -return loadConfiguration(getConfigType(), true, true).map(configuration -> { - if (params.hasCursor()) { - return PaginationCursor.decode(params.nextToken, getConfigType(), params.sort) - .map(cursor -> buildPaginatedPage(configuration, params, cursor)); - } - return buildPaginatedPage(configuration, params, null); -}); +return loadConfiguration(getConfigType(), true, true) + .map(configuration -> ValidationResult.success(SecurityConfiguration.of(null, configuration))) + .map(endpointValidator::onConfigLoad) + .map(securityConfiguration -> { + final SecurityDynamicConfiguration<?> configuration = securityConfiguration.configuration(); + if (params.hasCursor()) { + return PaginationCursor.decode(params.nextToken, getConfigType(), params.sort) + .map(cursor -> buildPaginatedPage(configuration, params, cursor)); + } + return buildPaginatedPage(configuration, params, null); + }); Suggestion importance[1-10]: 8 __ Why: Important correctness/security concern: the paginated GET path skips endpointValidator.onConfigLoad, which could lead to inconsistent authorization/redaction compared to the single-entity path.	Medium
	Enforce maximum page size limit There is no upper bound on size, which allows a caller to request an arbitrarily large page (e.g. size=Integer.MAX_VALUE), potentially causing excessive memory allocation in Paginator.paginate via limit(targetSize + 1L) and the LinkedHashMap sizing. Enforce a reasonable maximum page size. src/main/java/org/opensearch/security/dlic/rest/api/pagination/PaginationRequestParser.java [76-81] -if (size <= 0) { +if (size <= 0 || size > 1000) { return ValidationResult.error( RestStatus.BAD_REQUEST, - badRequestMessage("Invalid size parameter '" + size + "'. Must be a positive integer.") + badRequestMessage("Invalid size parameter '" + size + "'. Must be between 1 and 1000.") ); } Suggestion importance[1-10]: 7 __ Why: Valid security/resource concern. Without an upper bound, callers can request very large pages causing memory pressure. Adding a max page size is a reasonable safeguard.	Medium
Possible issue	Guard against missing legacy GET handler legacyHandler may be null if withPaginatedGetRequest is called before onGetRequest, causing a NullPointerException at runtime when pagination is not requested. Add a null check with a clear failure message, or enforce the ordering by throwing during builder configuration. src/main/java/org/opensearch/security/dlic/rest/api/RequestHandler.java [156-166] final RequestHandler legacyHandler = requestHandlers.get(RestRequest.Method.GET); +if (legacyHandler == null) { + throw new IllegalStateException("withPaginatedGetRequest must be registered after onGetRequest"); +} add(RestRequest.Method.GET, (channel, request, client) -> { final ValidationResult<ToXContent> result = mapper.apply(request); if (result != null) { result.valid(toXContent -> ok(channel, toXContent)) .error((status, toXContent) -> response(channel, status, toXContent)); } else { legacyHandler.handle(channel, request, client); } }); Suggestion importance[1-10]: 6 __ Why: Valid concern: if withPaginatedGetRequest is invoked before onGetRequest, legacyHandler will be null and cause a NullPointerException. Adding an explicit check improves robustness and developer experience.	Low
General	Handle serialization errors and null input MAPPER.writeValueAsString can throw a checked/unchecked exception depending on Jackson version, but more importantly the method has no declared IOException handling. Wrap in try/catch to convert failures into a clear runtime error, and also guard against null lastKey which will cause a NullPointerException in ObjectNode.put. src/main/java/org/opensearch/security/dlic/rest/api/pagination/PaginationCursor.java [58-66] public static PaginationCursor encode(final CType<?> ctype, final String sort, final String lastKey) { + Objects.requireNonNull(lastKey, "lastKey must not be null"); final ObjectNode node = MAPPER.createObjectNode(); node.put(FIELD_CTYPE, ctype.toLCString()); node.put(FIELD_SORT, sort); node.put(FIELD_LAST_KEY, lastKey); - final String json = MAPPER.writeValueAsString(node); - final String encoded = Base64.getUrlEncoder().withoutPadding().encodeToString(json.getBytes(StandardCharsets.UTF_8)); - return new PaginationCursor(encoded, lastKey); + try { + final String json = MAPPER.writeValueAsString(node); + final String encoded = Base64.getUrlEncoder().withoutPadding().encodeToString(json.getBytes(StandardCharsets.UTF_8)); + return new PaginationCursor(encoded, lastKey); + } catch (Exception e) { + throw new IllegalStateException("Failed to encode pagination cursor", e); + } } Suggestion importance[1-10]: 4 __ Why: The encode method is only called internally with a non-null lastKey derived from a map entry, so null guards are of limited value. Adding exception handling is a minor defensive improvement.	Low

[codecov]

Codecov Report

❌ Patch coverage is 95.13889% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.47%. Comparing base (5e8e5f1) to head (3585239).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
...ity/dlic/rest/api/pagination/PaginationCursor.java	83.78%	3 Missing and 3 partials ⚠️
...h/security/dlic/rest/api/pagination/Paginator.java	96.42%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6378      +/-   ##
==========================================
+ Coverage   75.33%   75.47%   +0.14%     
==========================================
  Files         456      461       +5     
  Lines       30075    30281     +206     
  Branches     4564     4591      +27     
==========================================
+ Hits        22657    22856     +199     
+ Misses       5297     5296       -1     
- Partials     2121     2129       +8     

Files with missing lines	Coverage Δ
...arch/security/dlic/rest/api/AbstractApiAction.java	89.51% <100.00%> (+0.83%)	⬆️
...security/dlic/rest/api/InternalUsersApiAction.java	93.60% <100.00%> (+0.05%)	⬆️
...earch/security/dlic/rest/api/NodesDnApiAction.java	90.90% <100.00%> (ø)
...nsearch/security/dlic/rest/api/RequestHandler.java	98.96% <100.00%> (+0.11%)	⬆️
...ity/dlic/rest/api/pagination/PaginationParams.java	100.00% <100.00%> (ø)
...c/rest/api/pagination/PaginationRequestParser.java	100.00% <100.00%> (ø)
...ity/dlic/rest/api/pagination/PaginationResult.java	100.00% <100.00%> (ø)
...h/security/dlic/rest/api/pagination/Paginator.java	96.42% <96.42%> (ø)
...ity/dlic/rest/api/pagination/PaginationCursor.java	83.78% <83.78%> (ø)

... and 8 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]

Persistent review updated to latest commit 3585239