Add pagination support to Security configuration collection APIs (#6339)

[nishthm]

Description

Introduces opt-in cursor-based pagination on the six caller-visible Security configuration collection GETs — internalusers, roles, rolesmapping, actiongroups, tenants, and nodesdn — using the same size / next_token / sort contract as the OpenSearch _list/* APIs introduced in opensearch-project/OpenSearch#14641 and #14718. This gives operators one consistent pagination surface across core and plugin configuration APIs, closing the Security-plugin half of that consistency goal.

Backward compatible. Requests without any of size, next_token, or sort retain the exact pre-existing response shape byte-for-byte. Pagination is only active when a caller explicitly opts in

Contract

Param	Range / values	Default	On invalid
size	integer 1..1000	100	HTTP 400
next_token	opaque URL-safe Base64	absent	HTTP 400
sort	asc | desc	asc	HTTP 400

Paginated response:

GET /_plugins/_security/api/roles?size=2&sort=asc
{
  "next_token": "<opaque token>",
  "roles": {
    "role_a": { ... },
    "role_b": { ... }
  }
}

Terminal page has "next_token": null. The wrapper key is the endpoint's CType.toLCString() (matches the URL segment).

Cursor design

• URL-safe Base64 encoding of a small versioned JSON payload: {"v":1,"c":"<ctype>","s":"<sort>","n":"<lastName>"}.
• Bound to {format version, CType, sort direction, last returned entity name}. Any mismatch — malformed / non-JSON / non-object / missing field / wrong field type / cross-endpoint / cross-direction / stale version — returns HTTP 400.
• Lexical (name-based) continuation. The entity referenced by a cursor does not need to still exist; insertions or deletions strictly before the cursor do not shift later pages. Matches the invariant OpenSearch _list/* APIs use.
• Single-entity GETs (/roles/{name}) reject pagination parameters with HTTP 400.

Diff shape

• New: PaginationHelper.java (~400 LOC) — the whole feature lives here (cursor codec, param validation, page assembly, response wrapper).
• RequestHandler.java: one new builder method onCollectionGetRequest(CType, mapper) — same mapper signature as the existing onGetRequest, so endpoint pipelines (permission checks, filterBy/show_all composition) are preserved verbatim.
• AbstractApiAction.java: consumes size / next_token / sort centrally in prepareRequest, matching the pattern established by wait_for_completion in Support asynchronous Security configuration APIs with wait_for_completion #6337 — so singleton endpoints ignore the params silently rather than 400-ing on them.
• Six endpoints, one line each:
  • InternalUsersApiAction, NodesDnApiAction: swap .onGetRequest(mapper) for .onCollectionGetRequest(getConfigType(), mapper) — their filterBy / show_all composition inside the mapper is untouched.
  • RolesApiAction, ActionGroupsApiAction, RolesMappingApiAction, TenantsApiAction: add .onCollectionGetRequest(getConfigType(), this::processGetRequest).

Known trade-offs (documented in the design doc, flagged here for reviewers)

1. Nodesdn has no live-cluster integration test. The LocalCluster framework does not seed the nodesdn document in the security index, so a live GET returns 403 "Security index need to be updated to support 'nodesdn'" — that failure fires before pagination ever runs. The nodesdn pagination wiring is covered by (a) PaginationHelperTest which exercises the shared code path nodesdn now uses, (b) the one-call swap in NodesDnApiAction verifiable by code review, and (c) the existing NodesDnApiTest unit test. If reviewers prefer a live integration test, closing the gap is ~30–50 lines of test setup: obtain an admin-cert client via localCluster.getAdminCertRestClient() and seed the nodesdn document in @BeforeClass. Happy to add in a follow-up.

2. Per-request rendering cost is O(N), not O(size). PaginatedConfigurationResponse.toXContent serializes the full configuration once and then filters to the page. Chosen for byte-identical per-entity shape parity with the non-paginated path (no divergent serializer codepath to maintain). At N = 10,000 with size = 100 this is ~100× more work than strictly necessary. Not measurable at typical scales; if profiling on a large real deployment flags it, the fix is to serialize per-entity for names in the page (O(N) → O(size)), with a shape-diff regression test to keep both paths equivalent.

Follow-ups intentionally out of scope

• Search-style endpoints wrapping the security index (/_search-style pattern used by e.g. alerting). Larger architectural change — the plugin currently reads from an in-memory SecurityDynamicConfiguration snapshot, not directly from .opendistro_security, and a search endpoint would need to reconcile with the DLS/FLS layer that already sits on top of that index. Trackable separately.
• Tenant / workspace awareness across plugins (Implementing pagination for _cat/indices API (#14718) OpenSearch#16209). This PR applies the pagination contract to the caller-visible set exactly as loadConfiguration(omitSensitiveData=true) already exposes it; tenant behavior is unchanged.

Rollout notes

• Zero-risk for existing clients. No request that worked before this change behaves differently after it.
• Cursor is forward-compatible: the v field lets us bump the on-wire structure later and return a clean 400 on stale cursors rather than silently misbehaving.
• No new dependencies — standard-library Base64.getUrlEncoder(), existing DefaultObjectMapper, existing SecurityDynamicConfiguration.
• Singleton APIs (audit, allowlist, securityconfig) accept the three new query params silently (consumed centrally) but never act on them.

Issues Resolved

Closes #6339

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

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
N/A

Testing

Unit — PaginationHelperTest, 25 cases, all pass. Covers:

• isRequested / consumeParameters semantics.
• 12 parse-and-validate rejection cases: bad sort, non-integer / out-of-range size, malformed Base64, non-JSON, valid-JSON-but-not-object, missing required field (each of v/c/s/n), wrong field types, cross-endpoint cursor, cross-direction cursor, version-mismatched cursor.
• Cursor round-trip with a Unicode name (BMP + supplementary chars).
• Assertion that emitted cursors are URL-safe (no +, /, =).
• Pagination happy path: asc / desc traversal across multiple pages, terminal null token, empty collection, single-entity GET rejection, response-shape wrapper containment.
• Lexical continuation: deletion of the cursor-named entity mid-traversal, plus insertion before the cursor — verifies later pages don't shift.

Integration — PaginationRestApiIntegrationTest, 16 cases, all pass (real cluster via LocalCluster, SECURITY_RESTAPI_ADMIN_ENABLED=true). Covers:

• Backward compatibility: no params → response has no next_token; single-entity GET unchanged.
• Ascending / descending traversal across multiple pages of /roles.
• Last page → next_token: null.
• Bad size / sort / next_token → HTTP 400 (five separate inputs).
• Cursor issued for /roles replayed on /actiongroups → HTTP 400.
• Cursor issued for sort=asc replayed with sort=desc → HTTP 400.
• Pagination params on /roles/{name} → HTTP 400.
• Deletion of the cursor-named entity between page requests does not shift later pages.
• Smoke test across roles, rolesmapping, actiongroups, internalusers, tenants (nodesdn deferred — see below).
• internalusers + filterBy=service composes with pagination.
• Hidden entities remain invisible to non-admin callers under pagination.
• Response shape wrapper key matches endpoint CType.
• Empty next_token= treated as absent.

Regression coverage. Full org.opensearch.security.dlic.rest.api.* unit suite: 166 tests, 0 failures attributable to this change (one pre-existing flake on RollbackVersionApiTest.testRollbackToPreviousVersion_success that reproduces on the base commit before this change is applied).

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 Reviewer Guide 🔍

(Review updated until commit a8a755d)

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 Performance On every paginated request, PaginatedConfigurationResponse.toXContent serializes the entire configuration to JSON and then reparses it into a Map<String, Object> just to render a slice of pageNames. For large collections (e.g., internal users with many entries) with small size values, this makes pagination O(N) work per page instead of O(size), defeating a primary benefit of pagination and increasing GC pressure. Consider rendering only the entries in pageNames directly from the configuration. @Override public XContentBuilder toXContent(final XContentBuilder builder, final Params params) throws IOException { // Serialize the full configuration to a Map exactly as SecurityDynamicConfiguration does, // so page entries render identically to the non-paginated response shape. We then keep // only the entries in `pageNames` (in the ordering requested by the caller). final boolean omitDefaults = params != null && params.paramAsBoolean("omit_defaults", false); final Map<String, Object> full = DefaultObjectMapper.readValue( DefaultObjectMapper.writeValueAsString(configuration, omitDefaults), TYPE_REF_MSO ); builder.startObject(); if (nextToken == null) { builder.nullField(PARAM_NEXT_TOKEN); } else { builder.field(PARAM_NEXT_TOKEN, nextToken); } builder.startObject(ctype.toLCString()); for (final String name : pageNames) { final Object entry = full.get(name); if (entry == null) { continue; } builder.field(name, entry); } builder.endObject(); builder.endObject(); return builder; } Possible Issue The lexical continuation loop uses the sort comparator to skip entries: while (comparator.compare(sortedNames.get(startIdx), params.lastName) <= 0). For descending sort, this skips items where name.compareTo(lastName) >= 0 in reverse (i.e., ≤ 0 by reverse comparator means the natural comparison is ≥ 0), which correctly skips names lexicographically ≥ lastName. However, this depends on the sortedNames being sorted with the same comparator. Verify the loop terminates correctly when duplicates or edge cases (e.g., empty strings) exist; a name equal to lastName returns 0 and is correctly skipped, but any future change to the comparator (e.g., locale-sensitive) could silently break continuation semantics. Consider making the skip logic explicit and comparator-independent. int startIdx = 0; if (params.lastName != null) { while (startIdx < sortedNames.size() && comparator.compare(sortedNames.get(startIdx), params.lastName) <= 0) { startIdx++; } }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to a8a755d
Explore these optional code suggestions:

Category	Suggestion	Impact
General	Use binary search for cursor position lookup Use binary search instead of a linear scan to locate the cursor start position. On large configurations with a high lastName, the current O(n) skip can dominate cost per page, degrading paginated traversal to O(n^2) overall. src/main/java/org/opensearch/security/dlic/rest/api/pagination/PaginationHelper.java [265-294] static PaginatedConfigurationResponse paginate( final SecurityDynamicConfiguration<?> configuration, final PaginationParams params, final CType<?> ctype ) { final Map<String, ?> entries = configuration.getCEntries(); final List<String> sortedNames = new ArrayList<>(entries.keySet()); final Comparator<String> comparator = SORT_ASC.equals(params.sort) ? Comparator.naturalOrder() : Comparator.reverseOrder(); Collections.sort(sortedNames, comparator); - // Lexical continuation from the cursor's last name: consume names strictly beyond it. int startIdx = 0; if (params.lastName != null) { - while (startIdx < sortedNames.size() && comparator.compare(sortedNames.get(startIdx), params.lastName) <= 0) { - startIdx++; - } + int idx = Collections.binarySearch(sortedNames, params.lastName, comparator); + startIdx = (idx >= 0) ? idx + 1 : -(idx + 1); } final int endIdx = Math.min(startIdx + params.size, sortedNames.size()); final List<String> pageNames = sortedNames.subList(startIdx, endIdx); final String nextToken; if (endIdx < sortedNames.size() && !pageNames.isEmpty()) { nextToken = encodeCursor(ctype, params.sort, pageNames.get(pageNames.size() - 1)); } else { nextToken = null; } return new PaginatedConfigurationResponse(pageNames, configuration, ctype, nextToken); } Suggestion importance[1-10]: 6 __ Why: The linear scan to find startIdx is O(n) per page, making full traversal O(n^2) for large configurations. Using Collections.binarySearch with the same comparator reduces this to O(log n). The improved code is correct and directly applicable.	Low
	Avoid serializing entire config for one page Serializing the entire configuration to JSON and then re-parsing it into a Map just to emit a small page is expensive — a size=1 page on a large config still materializes all entries. Fetch only the entries needed for the current page directly from configuration.getCEntries() to render. src/main/java/org/opensearch/security/dlic/rest/api/pagination/PaginationHelper.java [357-384] @Override public XContentBuilder toXContent(final XContentBuilder builder, final Params params) throws IOException { - // Serialize the full configuration to a Map exactly as SecurityDynamicConfiguration does, - // so page entries render identically to the non-paginated response shape. We then keep - // only the entries in `pageNames` (in the ordering requested by the caller). final boolean omitDefaults = params != null && params.paramAsBoolean("omit_defaults", false); - final Map<String, Object> full = DefaultObjectMapper.readValue( - DefaultObjectMapper.writeValueAsString(configuration, omitDefaults), - TYPE_REF_MSO - ); - builder.startObject(); if (nextToken == null) { builder.nullField(PARAM_NEXT_TOKEN); } else { builder.field(PARAM_NEXT_TOKEN, nextToken); } builder.startObject(ctype.toLCString()); + final Map<String, ?> entries = configuration.getCEntries(); for (final String name : pageNames) { - final Object entry = full.get(name); + final Object entry = entries.get(name); if (entry == null) { continue; } - builder.field(name, entry); + final Map<String, Object> rendered = DefaultObjectMapper.readValue( + DefaultObjectMapper.writeValueAsString(entry, omitDefaults), + TYPE_REF_MSO + ); + builder.field(name, rendered); } builder.endObject(); builder.endObject(); return builder; } Suggestion importance[1-10]: 5 __ Why: Serializing the full configuration to JSON and re-parsing it into a Map just to render a small page is wasteful for large configurations. The improved approach serializes only the page entries, reducing memory and CPU overhead per request. However, the improved code may not preserve the exact same serialization behavior (e.g., omitDefaults handling) as the original for complex nested objects.	Low
	URL-encode query parameter values in tests Query parameter values are appended without URL encoding. Since next_token uses URL-safe Base64 this is usually fine, but tests pass values like @@@not-base64@@@ and other special characters — those should be encoded to avoid ambiguous request lines that could be interpreted differently by the HTTP client vs. the server. src/integrationTest/java/org/opensearch/security/api/PaginationRestApiIntegrationTest.java [97-108] private static String pageQuery(final Map<String, String> params) { final StringBuilder sb = new StringBuilder("?"); boolean first = true; for (final var entry : params.entrySet()) { if (!first) { sb.append('&'); } - sb.append(entry.getKey()).append('=').append(entry.getValue()); + sb.append(java.net.URLEncoder.encode(entry.getKey(), StandardCharsets.UTF_8)) + .append('=') + .append(java.net.URLEncoder.encode(entry.getValue(), StandardCharsets.UTF_8)); first = false; } return sb.toString(); } Suggestion importance[1-10]: 3 __ Why: The pageQuery helper doesn't URL-encode values, but in practice the integration test only passes URL-safe Base64 tokens and simple strings as values. The @@@not-base64@@@ value is only used in invalidInputs_badSizeSortAndTokenReturn400 which constructs the query string directly, not via pageQuery, so the practical risk is low.	Low

---

Previous suggestions

Suggestions up to commit 3e7a96e

Category	Suggestion	Impact
General	Avoid full-config serialization per page Serializing the entire configuration to a string and re-parsing it into a Map on every page request is expensive for large collections (e.g. thousands of roles) and defeats much of the benefit of pagination. Consider iterating only over pageNames and serializing each entry individually via configuration.getCEntry(name) (or an equivalent per-entry render path) so the cost scales with page size rather than total collection size. src/main/java/org/opensearch/security/dlic/rest/api/pagination/PaginationHelper.java [362-365] +// Render only the entries on the current page instead of serializing the whole +// configuration on every request. final Map<String, Object> full = DefaultObjectMapper.readValue( DefaultObjectMapper.writeValueAsString(configuration, omitDefaults), TYPE_REF_MSO ); Suggestion importance[1-10]: 6 __ Why: Valid performance concern: serializing the entire configuration on every page request scales with total collection size rather than page size, undermining pagination benefits. However, improved_code is identical to existing_code, providing no actual implementation of the suggested optimization.	Low
	Ensure cursor matches rendered entries The next_token is derived from the last entry name on the current page, but paginate() serializes the response by looking up entries from a Jackson-serialized map keyed by names from configuration.getCEntries(). If the sorted name contains characters that get transformed during JSON serialization (or if entries are filtered out post-sort, e.g. hidden users), full.get(name) may return null and the emitted cursor may point to a name not present in the rendered page, breaking continuation. Consider deriving the cursor from the actually-rendered last entry, or filter sortedNames to only names present in the serialized map before slicing. src/main/java/org/opensearch/security/dlic/rest/api/pagination/PaginationHelper.java [283-291] +// Ensure the page only contains names that will actually render, so the emitted cursor +// matches what the client sees. final int endIdx = Math.min(startIdx + params.size, sortedNames.size()); final List<String> pageNames = sortedNames.subList(startIdx, endIdx); final String nextToken; if (endIdx < sortedNames.size() && !pageNames.isEmpty()) { nextToken = encodeCursor(ctype, params.sort, pageNames.get(pageNames.size() - 1)); } else { nextToken = null; } Suggestion importance[1-10]: 5 __ Why: The concern about cursor pointing to a name not present in the rendered page is valid in principle, but the improved_code is identical to the existing_code, offering no concrete fix. The actual filtering (hidden entities) happens upstream before paginate is called, mitigating the risk.	Low
	Reserved param names may mask conflicts Consuming size, sort, and next_token centrally in prepareRequest means these names are now silently swallowed by every Security REST API endpoint, including ones that don't opt into pagination. If any existing endpoint uses size or sort as a domain parameter (or does so in the future), it will no longer be flagged as unrecognized. Consider consuming these only when the endpoint has opted into pagination, or documenting that these query parameter names are reserved plugin-wide. src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java [700] +// Only consume pagination params centrally if the endpoint opted in; otherwise these +// remain unrecognized and the standard 400 path applies. PaginationHelper.consumeParameters(request); Suggestion importance[1-10]: 4 __ Why: A reasonable observation about reserving size/sort/next_token plugin-wide, but the improved_code is identical to existing_code and offers no concrete alternative. The PR author explicitly documents this design choice in the surrounding comment.	Low

[github-actions]

PR Code Analyzer ❗

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

⛔ 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/PaginationHelper.java	236	medium	Pagination cursors are Base64URL-encoded JSON with no cryptographic signature or HMAC. Any authenticated caller can decode, modify the 'n' (last-name) field, and re-encode a cursor to start pagination from an arbitrary entity name. While callers already have full read access to the collection, unsigned cursors allow probing whether a specific entity name exists by binary-searching cursor positions — a potential information-disclosure channel that bypasses normal traversal ordering. Adding an HMAC over the payload using a server-side secret would close this.
src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java	696	low	PaginationHelper.consumeParameters() is called unconditionally for every endpoint, silently discarding size/next_token/sort parameters on endpoints that do not opt into pagination. Callers supplying pagination parameters to a non-paginating endpoint receive a normal (non-paginated) 200 response with no indication the parameters were ignored, which could mask client-side misconfiguration and lead users to believe pagination is working when it is not.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 0 | Medium: 1 | 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]

Persistent review updated to latest commit a8a755d

[itsmevichu]

There is already an existing PR #6378 raised by me that addresses the same issue. Maybe give it a look when you get a chance.