Add pagination support for Collection APIs

[itsmevichu]

Description

• Category: New feature
• Why these changes are required?
  Large deployments can have thousands of security configuration entities (users, roles, mappings, etc.). Returning everything in one response creates unbounded payloads. This PR adds opt-in cursor-based pagination to the six Security configuration collection APIs, using the same surface contract (size, sort, next_token) as OpenSearch core's _list APIs.
• What is the old behavior before changes and new behavior after changes?
  Without pagination parameters, all six collection endpoints behave identically to before — fully backward compatible.
  With the new opt-in parameters, responses use a paginated envelope:

GET /_plugins/_security/api/roles?size=4&sort=asc
{
  "next_token": "<cursor or null>",
  "roles": {
    "role_a": { ... },
    "role_b": { ... },
    ...
  }
}

Affected endpoints: internalusers, roles, rolesmapping, actiongroups, tenants, nodesdn.

• Key guarantees:
  Pagination applies after authorization and redaction — hidden entities cannot leak through page contents or cursor values.
  Cursors are bound to endpoint and sort direction; misuse returns 400.
  Pagination params on single-entity GETs return 400.
  Lexicographic cursor continuation - safe across additions and deletions between page requests.

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

Unit tests, Integration tests 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 2a631dd.

⛔ 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/PaginationCursor.java	63	medium	Pagination cursors are Base64-encoded JSON with no cryptographic signature or HMAC. A client can forge arbitrary cursors (e.g., crafting any `last_key` value) while still passing the ctype/sort validation checks. Since authorization is applied before pagination runs, this does not grant access to unauthorized entities, but it does allow a caller to skip or re-visit arbitrary portions of their authorized result set, bypassing pagination ordering guarantees and potentially revealing collection membership information through timing or trial-and-error.

The table above displays the top 10 most important findings.

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

---

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 3b6474f)

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 Possible Issue withPaginatedGetRequest captures the currently-registered GET handler as legacyHandler and then overwrites the GET slot. If a caller invokes onGetRequest after withPaginatedGetRequest (or calls withPaginatedGetRequest twice), the captured legacy handler becomes stale/self-referential and the fallback path either bypasses the intended handler or recurses. The ordering contract is implicit and easy to violate; consider asserting a prior onGetRequest registration and preventing later overwrites, or restructuring so both handlers coexist without capture. 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); add(RestRequest.Method.GET, (channel, request, client) -> { final ValidationResult<ToXContent> result = mapper.apply(request); if (result != null) { result.valid(toXContent -> Responses.ok(channel, toXContent)) .error((status, toXContent) -> response(channel, status, toXContent)); } else { legacyHandler.handle(channel, request, client); } }); return this; } Performance / Correctness toXContent serializes the entries map to a JSON string with DefaultObjectMapper.writeValueAsString, then re-parses it into a Map just to hand it to XContentBuilder.field(...). This doubles memory/CPU per page and, more importantly, converts entry values to generic Map/List/String via Jackson defaults, which can silently differ from the legacy non-paginated response formatting (e.g., custom serializers, field ordering, or omitDefaults-style flags applied elsewhere). Prefer writing entries directly with the XContent builder using the same serialization path as the legacy handler. public XContentBuilder toXContent(final XContentBuilder builder, final Params params) throws IOException { builder.startObject(); if (nextToken == null) { builder.nullField(FIELD_NEXT_TOKEN); } else { builder.field(FIELD_NEXT_TOKEN, nextToken); } @SuppressWarnings("unchecked") final Map<String, ?> serialisable = DefaultObjectMapper.readValue( DefaultObjectMapper.writeValueAsString(entries, false), Map.class ); builder.field(resourceKey, serialisable); builder.endObject(); return builder; } Control Flow routeGetRequest returns null to signal "not paginated, fall through to legacy handler". Returning null from a method typed ValidationResult<ToXContent> is a fragile contract — any future refactor that dereferences the result (e.g., adds .map(...) or logging) will NPE. Consider modeling fall-through explicitly (e.g., an Optional<ValidationResult<...>> or a dedicated sentinel) and documenting it on the withPaginatedGetRequest API. protected ValidationResult<ToXContent> routeGetRequest(final RestRequest request) throws IOException { if (PaginationRequestParser.isPaginationRequested(request)) { return processPaginatedGetRequest(request); } return null; } Cursor Encoding The cursor is base64-encoded JSON containing ctype, sort, and last_key but is not signed or integrity-protected. Clients can forge/modify cursors (e.g., set last_key to an arbitrary value) to skip entries or probe existence of names. Since pagination applies after authorization/redaction the impact is limited to enumeration through valid names the caller can already see, but note that last_key is echoed only inside the token — still, treating the cursor as opaque-but-tamper-evident (HMAC with a node-local secret) would prevent misuse and future coupling issues. At minimum, document that cursors are unauthenticated and client-mutable. 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); try { final String json = MAPPER.writeValueAsString(node); final String encoded = Base64.getEncoder().encodeToString(json.getBytes(StandardCharsets.UTF_8)); return new PaginationCursor(encoded, lastKey); } catch (Exception e) { throw new IllegalStateException("Failed to encode pagination cursor", e); } }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 3b6474f

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Guard against null legacy GET handler legacyHandler may be null if withPaginatedGetRequest is invoked before onGetRequest, causing a NullPointerException on non-paginated GETs. Guard against a null legacy handler (or enforce registration order) to avoid a runtime crash when the builder methods are called in a different order. src/main/java/org/opensearch/security/dlic/rest/api/RequestHandler.java [186-195] 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 onGetRequest to be registered first"); add(RestRequest.Method.GET, (channel, request, client) -> { final ValidationResult<ToXContent> result = mapper.apply(request); if (result != null) { result.valid(toXContent -> Responses.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 concern about registration order dependency causing potential NPE. Adding an explicit Objects.requireNonNull check improves robustness and provides clearer error messaging, though current callers appear to register handlers in the correct order.	Low
General	Avoid double JSON serialization of entries Serializing entries to a JSON string and re-parsing it back to a Map on every page response is expensive (double serialization) and can silently drop/alter types (e.g. numbers, dates). Prefer writing the map directly via builder.field(resourceKey) + builder.map(...) or convert via DefaultObjectMapper.convertValue(entries, Map.class) to avoid the string round-trip. 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 -); +final Map<String, ?> serialisable = DefaultObjectMapper.convertValue(entries, Map.class); builder.field(resourceKey, serialisable); Suggestion importance[1-10]: 6 __ Why: Valid performance improvement — the current implementation does an unnecessary JSON string round-trip on every response. Using convertValue avoids the overhead and potential type issues.	Low
	Ensure params consumed on all paths Calling request.param(...) here consumes those params. If parse returns an error before consumeParameters runs, or if isPaginationRequested is false and pagination params are absent while the parser is still invoked elsewhere, the sequence is fine — but note that consumeParameters in AbstractApiAction also consumes them, which is redundant. More importantly, consuming inside a validator that may bail out early is a common source of "unconsumed parameter" errors on error paths; ensure all three params are consumed on the bad-request paths too (they are, via request.param, so verify no branch skips them). src/main/java/org/opensearch/security/dlic/rest/api/pagination/PaginationRequestParser.java [46-48] final String sizeParam = request.param(PARAM_SIZE); final String sortParam = request.param(PARAM_SORT); final String tokenParam = request.param(PARAM_NEXT_TOKEN); +// (all three consumed unconditionally above — OK) Suggestion importance[1-10]: 2 __ Why: The suggestion only asks to verify existing behavior and the improved_code is identical to the existing_code except for a comment. Low impact.	Low
	Verify cursor filter comparison semantics The filter uses String.compareTo (UTF-16 code-unit order) while sorting later also uses the natural order — which is consistent. However, entity names may contain characters where users expect case-insensitive or locale-aware ordering; more importantly, if the map ever contains a key equal to lastKey that was NOT on the previous page (e.g. re-created with same name), the strict >/< filter will skip it. Confirm this is intended semantics, otherwise consider >=/<= combined with excluding the exact cursor key only when it was actually the last emitted. src/main/java/org/opensearch/security/dlic/rest/api/pagination/Paginator.java [62-68] -// Filter first 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; }); } Suggestion importance[1-10]: 2 __ Why: The suggestion only asks to verify semantics and provides identical improved_code to existing_code. Strict >/< is actually correct for cursor-based pagination to avoid duplicates.	Low

---

Previous suggestions

Suggestions up to commit 93769ed

Category	Suggestion	Impact
Possible issue	Guard against missing legacy GET handler If withPaginatedGetRequest is called before onGetRequest (or without one being registered), legacyHandler will be null, causing a NullPointerException on non-paginated GETs. Add a null check and either throw a clear configuration error at registration time or provide a sensible fallback response so misordered builder usage fails fast rather than at runtime. src/main/java/org/opensearch/security/dlic/rest/api/RequestHandler.java [186-195] final RequestHandler legacyHandler = requestHandlers.get(RestRequest.Method.GET); +Objects.requireNonNull(legacyHandler, "withPaginatedGetRequest requires onGetRequest 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); } }); Suggestion importance[1-10]: 6 __ Why: Valid defensive check: if withPaginatedGetRequest is called without a prior onGetRequest, legacyHandler would be null causing NPE at request time. Adding a fail-fast check improves builder robustness, though current usage patterns register them together.	Low
Security	Bound next_token size before decoding Base64.getDecoder().decode(null) throws NullPointerException which is caught, but callers may pass unexpected inputs; more importantly, an oversized token could cause excessive memory allocation. Enforce a maximum token length before decoding to prevent malicious clients from sending very large next_token values that force large allocations. src/main/java/org/opensearch/security/dlic/rest/api/pagination/PaginationCursor.java [88-89] +if (encoded == null || encoded.length() > 4096) { + return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage("Invalid next_token.")); +} final byte[] bytes = Base64.getDecoder().decode(encoded); final JsonNode node = MAPPER.readTree(new String(bytes, StandardCharsets.UTF_8)); Suggestion importance[1-10]: 5 __ Why: A reasonable hardening suggestion to prevent large next_token allocations from malicious clients. Impact is moderate as the token is user-controlled input, though OpenSearch typically has upstream request size limits.	Low
General	Avoid JSON round-trip during serialization Serializing entries via JSON string round-trip on every response is inefficient and can lose type fidelity for values already supporting ToXContent. Write the map directly to the XContentBuilder (e.g., builder.field(resourceKey).map(entries)) or leverage the existing serializer to avoid the double conversion and the intermediate String allocation. 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 +builder.field(resourceKey); +builder.map( + DefaultObjectMapper.readValue(DefaultObjectMapper.writeValueAsString(entries, false), Map.class) ); -builder.field(resourceKey, serialisable); Suggestion importance[1-10]: 4 __ Why: The observation about inefficient JSON round-trip is valid, but the improved_code shown is essentially identical to the existing code and doesn't actually eliminate the round-trip, weakening the suggestion's usefulness.	Low
	Document null fall-through contract explicitly Returning null to signal "fall through to legacy handler" is fragile and violates the CheckedFunction contract used elsewhere; a NPE risk exists if callers ever unwrap the result. Consider using an explicit sentinel (e.g., Optional) or documenting the null contract with a @Nullable annotation to make the fall-through behavior explicit and prevent accidental misuse. src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java [227-232] +/** Returns {@code null} to indicate the request is not paginated and should be handled by the legacy GET handler. */ +@Nullable protected ValidationResult<ToXContent> routeGetRequest(final RestRequest request) throws IOException { if (PaginationRequestParser.isPaginationRequested(request)) { return processPaginatedGetRequest(request); } return null; } Suggestion importance[1-10]: 3 __ Why: Documentation-only improvement suggesting a @Nullable annotation and comment. The current code works correctly; this is a minor readability/maintainability enhancement.	Low

Suggestions up to commit 8f9ede6

Category	Suggestion	Impact
Possible issue	Guard against missing legacy GET handler If withPaginatedGetRequest is called before onGetRequest, legacyHandler will be null and pagination-less GETs will NPE at runtime. Guard against a null legacy handler or enforce ordering explicitly (e.g., throw an IllegalStateException at registration time) to fail fast and prevent hard-to-debug NPEs in production. 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); + if (legacyHandler == null) { + throw new IllegalStateException("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 defensive check — if withPaginatedGetRequest is called before onGetRequest, the captured legacyHandler would be null and cause an NPE at runtime. Adding an explicit fail-fast check improves robustness, though current callers appear to follow correct ordering.	Low
General	Avoid double JSON serialization of entries Serializing entries by round-tripping through a JSON string and re-parsing into a Map is expensive per request and can misrepresent complex types (e.g., dates, custom serializers). Consider writing raw JSON directly into the XContentBuilder (e.g., builder.rawField) or converting once via objectMapper.convertValue(entries, Map.class) to avoid the double serialization cost on every page response. 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 -); -builder.field(resourceKey, serialisable); +final String json = DefaultObjectMapper.writeValueAsString(entries, false); +try (final java.io.InputStream is = new java.io.ByteArrayInputStream(json.getBytes(java.nio.charset.StandardCharsets.UTF_8))) { + builder.rawField(resourceKey, is, org.opensearch.common.xcontent.XContentType.JSON); +} Suggestion importance[1-10]: 6 __ Why: Legitimate performance concern — the current code serializes entries to a JSON string then parses back into a Map, which is wasteful. Using rawField or convertValue would be more efficient and preserve type fidelity for complex objects.	Low
	Validate cursor before loading configuration The paginated GET handler skips endpointValidator.onConfigLoad's single-entity name extraction path but does not apply the same removeOthers/entity-name filtering as processGetRequest. More importantly, validate the cursor BEFORE loading the (potentially large) configuration to avoid unnecessary I/O for requests with malformed tokens, and to return the 400 faster. src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java [174-193] protected ValidationResult<ToXContent> processPaginatedGetRequest(final RestRequest request) throws IOException { return PaginationRequestParser.parse(request).map(params -> { if (nameParam(request) != null) { return ValidationResult.error( RestStatus.BAD_REQUEST, badRequestMessage("Pagination parameters are not supported for single-entity GET requests.") ); } - 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); - }); + final ValidationResult<PaginationCursor> cursorResult = params.hasCursor() + ? PaginationCursor.decode(params.nextToken, getConfigType(), params.sort) + : ValidationResult.success(null); + return cursorResult.map(cursor -> + loadConfiguration(getConfigType(), true, true) + .map(configuration -> ValidationResult.success(SecurityConfiguration.of(null, configuration))) + .map(endpointValidator::onConfigLoad) + .map(securityConfiguration -> buildPaginatedPage(securityConfiguration.configuration(), params, cursor)) + ); }); } Suggestion importance[1-10]: 5 __ Why: Reasonable optimization to validate the cursor before loading potentially large configuration. This avoids unnecessary I/O for malformed tokens and slightly improves error response latency, but is a moderate performance improvement rather than a correctness fix.	Low
	Document codepoint ordering guarantees String.compareTo returns locale-independent Unicode codepoint order, but the integration test asserts ordering using String.compareTo while the wire format has no explicit collation guarantee. To ensure a stable, well-defined total order regardless of JDK locale settings and to match the ordering used when generating the cursor's lastKey on a different node, explicitly use Comparator.naturalOrder() / String::compareTo in both the filter and the sort, and document that ordering is codepoint-based. src/main/java/org/opensearch/security/dlic/rest/api/pagination/Paginator.java [61-68] -// Filter first +// Filter first (codepoint order — same order used to produce cursor) 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; }); } Suggestion importance[1-10]: 2 __ Why: The improved_code is essentially identical to the existing_code (only a comment is added). String.compareTo is already locale-independent, so this is a documentation-only suggestion with minimal impact.	Low

Suggestions up to commit 94a2be6

Category	Suggestion	Impact
Possible issue	URL-encode query parameter values Token values include Base64 characters (e.g. +, /, =) which are not URL-safe and will be corrupted when placed in a query string without encoding. This will cause the crossEndpointTokenReturnsBadRequest/sortMismatchTokenReturnsBadRequest/continuation tests to fail intermittently. URL-encode the value part before appending. src/integrationTest/java/org/opensearch/security/api/PaginationRestApiIntegrationTest.java [45-52] /** Appends ?key=val&… to an already-built API path. */ private static String withParams(final String path, final String... kvPairs) { final var sb = new StringBuilder(path).append('?'); for (int i = 0; i < kvPairs.length; i += 2) { if (i > 0) sb.append('&'); - sb.append(kvPairs[i]).append('=').append(kvPairs[i + 1]); + sb.append(kvPairs[i]).append('=') + .append(java.net.URLEncoder.encode(kvPairs[i + 1], java.nio.charset.StandardCharsets.UTF_8)); } return sb.toString(); } Suggestion importance[1-10]: 7 __ Why: Correct concern: Base64 tokens include +, /, = which are not URL-safe and can be corrupted in query strings. This could cause flaky tests when passing next_token values.	Medium
	Guard against missing legacy GET handler If withPaginatedGetRequest is called before onGetRequest, legacyHandler will be null and any non-pagination GET will throw a NullPointerException. Guard against null (or enforce ordering) so callers cannot accidentally register the paginated wrapper without a legacy fallback. src/main/java/org/opensearch/security/dlic/rest/api/RequestHandler.java [156-165] final RequestHandler legacyHandler = requestHandlers.get(RestRequest.Method.GET); +Objects.requireNonNull(legacyHandler, "onGetRequest handler must be registered before withPaginatedGetRequest"); 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]: 5 __ Why: Valid defensive programming point — if withPaginatedGetRequest is called without a prior onGetRequest, legacyHandler would be null and cause NPE. Adding an explicit null check would improve developer feedback.	Low
General	Avoid costly JSON round-trip when serializing entries Serializing entries via a JSON string round-trip (writeValueAsString + readValue) is expensive and can silently drop or reorder fields. Since entries are typically ToXContent or plain maps, prefer using builder.field(resourceKey, entries) directly, or convert via XContentHelper/DefaultObjectMapper.convertValue to avoid the double-parse cost and potential precision loss for numeric/date fields. src/main/java/org/opensearch/security/dlic/rest/api/pagination/PaginationResult.java [63-68] @Override public XContentBuilder toXContent(final XContentBuilder builder, final Params params) throws IOException { builder.startObject(); if (nextToken == null) { builder.nullField(FIELD_NEXT_TOKEN); } else { builder.field(FIELD_NEXT_TOKEN, nextToken); } - @SuppressWarnings("unchecked") - final Map<String, ?> serialisable = DefaultObjectMapper.readValue( - DefaultObjectMapper.writeValueAsString(entries, false), - Map.class - ); - builder.field(resourceKey, serialisable); + builder.field(resourceKey, entries); builder.endObject(); return builder; } Suggestion importance[1-10]: 6 __ Why: Valid observation: the JSON round-trip is unnecessary overhead and could cause subtle issues. However, the entries may not be directly serializable via builder.field, so the simplification needs verification. Moderate impact on performance/correctness.	Low
	Avoid re-sorting entire collection per page The comment says "high-performance" but this sorts the entire filtered set on every page request, which is O(n log n) per page and O(n² log n) for a full traversal. Consider extracting sorted keys once (or using a TreeMap/NavigableMap view) and using tailMap/headMap from the cursor position to reduce per-page work to O(page size). src/main/java/org/opensearch/security/dlic/rest/api/pagination/Paginator.java [62-76] -// Filter first -Stream<Map.Entry<String, T>> entryStream = allEntries.entrySet().stream(); +// Sort keys once, then slice from cursor position +final java.util.List<String> sortedKeys = allEntries.keySet().stream() + .sorted(isDesc ? Comparator.reverseOrder() : Comparator.naturalOrder()) + .collect(Collectors.toList()); +int startIdx = 0; if (lastKey != null && !lastKey.isEmpty()) { - entryStream = entryStream.filter(entry -> { - final int cmp = entry.getKey().compareTo(lastKey); - return isDesc ? cmp < 0 : cmp > 0; - }); + // binary search for insertion point strictly after lastKey + int lo = 0, hi = sortedKeys.size(); + while (lo < hi) { + int mid = (lo + hi) >>> 1; + int cmp = sortedKeys.get(mid).compareTo(lastKey); + if (isDesc ? cmp < 0 : cmp > 0) hi = mid; else lo = mid + 1; + } + startIdx = lo; +} +final int targetSize = params.size; +final int endIdx = Math.min(startIdx + targetSize + 1, sortedKeys.size()); +final List<Map.Entry<String, T>> candidatePage = new java.util.ArrayList<>(endIdx - startIdx); +for (int i = startIdx; i < endIdx; i++) { + final String k = sortedKeys.get(i); + candidatePage.add(Map.entry(k, allEntries.get(k))); } -// 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()); - Suggestion importance[1-10]: 4 __ Why: Valid performance observation, though for typical security config sizes (roles, users) the impact is minor. The suggestion improves algorithmic complexity but adds complexity to the code.	Low

Suggestions up to commit 6fe9022

Category	Suggestion	Impact
Possible issue	Reuse legacy filtering before paginating The paginated GET path calls loadConfiguration(getConfigType(), true, true) but does not perform authorization/redaction steps that legacy processGetRequest inherits via endpointValidator.onConfigLoad/removeOthers, and there is no filtering hook (e.g., filterUsers for internal users). As a result, paginated responses may leak entries that the legacy handler would hide/redact (e.g., filtered users, reserved/hidden entries not stripped for the caller). Ensure the same post-load filtering that the legacy GET applies is invoked before paginating. src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java [174-193] protected ValidationResult<ToXContent> processPaginatedGetRequest(final RestRequest request) throws IOException { return PaginationRequestParser.parse(request).map(params -> { if (nameParam(request) != null) { return ValidationResult.error( RestStatus.BAD_REQUEST, badRequestMessage("Pagination parameters are not supported for single-entity GET requests.") ); } - return loadConfiguration(getConfigType(), true, true).map( - configuration -> ValidationResult.success(SecurityConfiguration.of(null, configuration)) - ).map(endpointValidator::onConfigLoad).map(securityConfiguration -> { + return processGetRequest(request).map(securityConfiguration -> { Suggestion importance[1-10]: 7 __ Why: Valid concern: paginated GET path does not apply the same filtering (e.g., filterUsers in InternalUsersApiAction) as the legacy GET path, potentially leaking entries. However, the proposed improved_code is incomplete and doesn't fully address subclass overrides.	Medium
	Guard against missing legacy GET handler If withPaginatedGetRequest is called before onGetRequest, legacyHandler will be null and any non-paginated GET will throw a NullPointerException. Add a null-check with a clear error (or require ordering explicitly) to fail fast on misconfiguration and avoid an obscure runtime NPE. 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 a prior onGetRequest registration"); 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]: 5 __ Why: Adding an explicit null-check for legacyHandler would fail fast on misconfiguration rather than producing an obscure NPE at runtime. Minor defensive improvement.	Low
General	Avoid JSON round-trip that can reorder keys Serializing entries by writing to a JSON string and re-parsing into a Map is expensive per page and can reorder keys, defeating the deterministic ordering guaranteed by Paginator (which uses LinkedHashMap). Prefer writing entries directly to the XContentBuilder (e.g., builder.field(resourceKey); builder.map(entries); or iterating and serializing each value) to preserve order and avoid double-serialization overhead. 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 -); -builder.field(resourceKey, serialisable); +builder.field(resourceKey); +builder.map(entries); Suggestion importance[1-10]: 6 __ Why: The JSON round-trip is inefficient and could theoretically reorder keys, though most JSON parsers preserve insertion order. Direct serialization is cleaner and more performant.	Low
	Share one comparator for filter and sort compareTo on String uses UTF-16 code-unit ordering, but the entries are sorted using Map.Entry.comparingByKey() which also uses natural (UTF-16) order — however, these must remain in lock-step forever. If a future change swaps the sort comparator (e.g., to a locale-aware or case-insensitive comparator), the filter predicate will silently drop or duplicate entries across pages. Extract a single Comparator constant and use it for both the filter predicate and the sort to keep them consistent. src/main/java/org/opensearch/security/dlic/rest/api/pagination/Paginator.java [62-74] -// Filter first +final Comparator<String> keyComparator = Comparator.naturalOrder(); 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); + final int cmp = keyComparator.compare(entry.getKey(), lastKey); return isDesc ? cmp < 0 : cmp > 0; }); } Suggestion importance[1-10]: 3 __ Why: Minor maintainability suggestion. Current code is correct and using a shared comparator only guards against hypothetical future changes rather than a real issue.	Low

Suggestions up to commit 6fe9022

Category	Suggestion	Impact
Security	Apply same redaction/filter to paginated entries The paginated result is built from configuration.getCEntries() but the response is written by PaginationResult.toXContent which re-serializes entries via DefaultObjectMapper without honoring the current REST API filtering (e.g., hidden/reserved entities, static entries, filterBy, hashes redaction). The non-paginated path applies these through ok(channel, securityConfiguration.configuration()) and endpoint-specific onGetRequest mappers (see InternalUsersApiAction filterUsers, NodesDnApiAction show_all, hash removal). This can leak sensitive/hidden fields in paginated responses. Apply the same redaction/filtering to the entries map before paginating. src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java [174-193] -protected ValidationResult<ToXContent> processPaginatedGetRequest(final RestRequest request) throws IOException { - return PaginationRequestParser.parse(request).map(params -> { - if (nameParam(request) != null) { - return ValidationResult.error( - RestStatus.BAD_REQUEST, - badRequestMessage("Pagination parameters are not supported for single-entity GET requests.") - ); - } - 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); - }); - }); -} +return loadConfiguration(getConfigType(), true, true).map( + configuration -> ValidationResult.success(SecurityConfiguration.of(null, configuration)) +).map(endpointValidator::onConfigLoad).map(securityConfiguration -> { + final SecurityDynamicConfiguration<?> configuration = securityConfiguration.configuration(); + // TODO: apply the same filtering/redaction used by the non-paginated GET path + // (hidden/reserved handling, password/hash removal, filterBy, etc.) before paginating. + 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: Valid concern: the paginated path bypasses endpoint-specific filtering (hidden/reserved handling, password/hash redaction, filterBy) applied by onGetRequest mappers in subclasses like InternalUsersApiAction and NodesDnApiAction, which could leak sensitive data.	Medium
Possible issue	Guard against missing legacy GET handler withPaginatedGetRequest requires onGetRequest to have been called first, otherwise legacyHandler will be null and calling withPaginatedGetRequest on any endpoint that forgets the ordering will produce a NullPointerException when a non-paginated GET arrives. Add a null-check with a clear error, or fall back to methodNotImplementedHandler, so the failure mode is explicit at registration/handling time. 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 onGetRequest 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]: 5 __ Why: Adding an explicit null check for legacyHandler makes the failure mode clearer at registration time, though the current ordering in buildDefaultRequestHandlers ensures it is set. Minor defensive improvement.	Low
General	Avoid double JSON serialization of entries Serializing entries to a JSON string and re-parsing to a Map on every paginated response is unnecessarily expensive and loses type fidelity. Prefer writing the map directly via the XContentBuilder, or use a streaming approach; this avoids double serialization and any subtle differences from the standard config serializer used elsewhere. 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 -); -builder.field(resourceKey, serialisable); +builder.field(resourceKey, entries); Suggestion importance[1-10]: 5 __ Why: The double serialization is genuinely inefficient, but the round-trip may be intentional to leverage the config serializer's shape. The suggestion is a reasonable performance improvement but needs verification that direct serialization produces equivalent output.	Low
	Use one comparator for filter and sort String.compareTo uses UTF-16 code-unit ordering, but Map.Entry.comparingByKey() (used just below) uses Comparable.compareTo — the two agree here, however, if entity names contain surrogate/international characters, ordering may be surprising and, more importantly, must be stable and consistent everywhere the cursor is compared. Consider extracting the comparator once (Comparator keyCmp = isDesc ? Comparator.reverseOrder() : Comparator.naturalOrder();) and using it both for the filter (keyCmp.compare(entry.getKey(), lastKey) > 0) and for sorting to guarantee they can never diverge. src/main/java/org/opensearch/security/dlic/rest/api/pagination/Paginator.java [62-68] -// Filter first +final Comparator<String> keyCmp = isDesc ? Comparator.<String>naturalOrder().reversed() : Comparator.<String>naturalOrder(); 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; - }); + entryStream = entryStream.filter(entry -> keyCmp.compare(entry.getKey(), lastKey) > 0); } +Comparator<Map.Entry<String, T>> comparator = Map.Entry.comparingByKey(keyCmp); Suggestion importance[1-10]: 3 __ Why: Minor code consistency improvement. Both filter and sort currently use String.compareTo/natural ordering so they already agree; the refactor is stylistic rather than fixing a real bug.	Low

[codecov]

Codecov Report

❌ Patch coverage is 95.13889% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.07%. Comparing base (e44d36f) to head (3b6474f).

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.98%   76.07%   +0.08%     
==========================================
  Files         466      471       +5     
  Lines       31128    31269     +141     
  Branches     4694     4715      +21     
==========================================
+ Hits        23654    23789     +135     
- Misses       5301     5306       +5     
- Partials     2173     2174       +1     

Files with missing lines	Coverage Δ
...arch/security/dlic/rest/api/AbstractApiAction.java	88.49% <100.00%> (+0.82%)	⬆️
...security/dlic/rest/api/InternalUsersApiAction.java	93.65% <100.00%> (+0.05%)	⬆️
...earch/security/dlic/rest/api/NodesDnApiAction.java	93.18% <100.00%> (ø)
...nsearch/security/dlic/rest/api/RequestHandler.java	99.11% <100.00%> (+0.08%)	⬆️
...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 7 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

[github-actions]

Persistent review updated to latest commit 6fe9022

[github-actions]

Persistent review updated to latest commit 6fe9022

[github-actions]

Persistent review updated to latest commit 94a2be6

[github-actions]

Persistent review updated to latest commit 8f9ede6

[github-actions]

Persistent review updated to latest commit 93769ed

[itsmevichu]

@cwperks @DarshitChanpura Could you please review this PR when you get a chance? It’s been inactive for a while, and I’d really appreciate your feedback. Thanks!

[github-actions]

Persistent review updated to latest commit 3b6474f

[cwperks]

@cwperks @DarshitChanpura Could you please review this PR when you get a chance? It’s been inactive for a while, and I’d really appreciate your feedback. Thanks!

I will take a look this week. Sorry for delay. Ty for taking this on, its certainly something that I think can be done more broadly for any category of configuration.

[cwperks]

@itsmevichu one high-level comment:

How would pagination work for APIs that may support a filter?

I think that internalusers may have a param called filterBy to distinguish internal from service account users (note: service was created in context of extensions and not currently used so in effect the internalusers API has no filter mechanism), but what happens if we pass a filter in like /_plugins/_security/api/logs* that matches 10_ users. Do the changes in this PR work?

Same on nodes_dn where I see a param called show_all. I'm not familiar with this param, but let's make sure the logic honors the existing params.