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:
  { "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 94a2be6)

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 Nested ValidationResult flattening processPaginatedGetRequest returns a ValidationResult<ValidationResult<ValidationResult<...>>> because .map(...) lambdas return ValidationResult values instead of using .flatMap. Specifically, PaginationRequestParser.parse(request).map(params -> { ... return loadConfiguration(...).map(...).map(...); }) will wrap the inner ValidationResult in another ValidationResult, and the same pattern with PaginationCursor.decode(...).map(cursor -> buildPaginatedPage(...)) compounds this. Depending on how map is implemented, error propagation (e.g. from cursor decode or config load) may not surface as a proper 400 to the client, and successful results may not be a ToXContent as declared. Verify with a test that a malformed next_token still yields the expected 400 body — the integration test only asserts status code, not content path. 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); }); }); } Wasteful JSON round-trip serialization toXContent serializes each page by writing the entries to a JSON string via DefaultObjectMapper.writeValueAsString and then re-parsing back into a Map just to pass it to builder.field(...). For large page sizes (up to MAX_SIZE=1000 entries with potentially complex role/user configs), this doubles serialization cost and allocates significant transient memory per response. Consider using builder.field(resourceKey) followed by directly serializing the entries via the existing XContent-aware writer, or builder.rawField with a pre-serialized stream, to avoid the parse-and-rebuild cycle. 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; } Fragile handler ordering assumption withPaginatedGetRequest captures the previously-registered GET handler via requestHandlers.get(RestRequest.Method.GET) and then re-adds a wrapper for GET. This only works if onGetRequest (or another GET handler) was registered before withPaginatedGetRequest. If a caller reverses the order (as could easily happen given fluent builder chains in InternalUsersApiAction and NodesDnApiAction), legacyHandler will be null and any non-paginated GET will NPE at runtime. Add a null check with a clear error, or enforce ordering at build time. 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 -> ok(channel, toXContent)) .error((status, toXContent) -> response(channel, status, toXContent)); } else { legacyHandler.handle(channel, request, client); } }); return this; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 94a2be6

Explore these optional code suggestions:

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

---

Previous suggestions

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

Suggestions up to commit 3585239

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

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.76%. Comparing base (d1b600b) to head (94a2be6).
⚠️ Report is 4 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.51%   75.76%   +0.25%     
==========================================
  Files         456      461       +5     
  Lines       30282    30555     +273     
  Branches     4574     4626      +52     
==========================================
+ Hits        22866    23150     +284     
+ Misses       5284     5279       -5     
+ Partials     2132     2126       -6     

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 15 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