[DT-3825] Move tests to resource/service so they can be exercised to confirm re… - #2992
[DT-3825] Move tests to resource/service so they can be exercised to confirm re…#2992otchet-broad wants to merge 10 commits into
Conversation
…quirements in the running ES cluster.
There was a problem hiding this comment.
Pull request overview
Adds an admin-only Elasticsearch “capability inventory” endpoint to let Consent deployments self-report which ES security features (X-Pack Security, API keys, DLS/FLS, run_as) are available/enforced in each environment, with optional write probes for end-to-end enforcement validation. This fits into the existing Dropwizard + Guice architecture by introducing a new Resource -> Service pair, updating DI to provide a shared ES RestClient, and documenting/typing the response via OpenAPI.
Changes:
- Introduces
GET /api/elasticSearch/capabilities(@RolesAllowed(ADMIN)) andElasticSearchCapabilityServiceto probe cluster security capabilities (read-only by default; optionalwriteProbes=true). - Updates Guice wiring to provide a singleton
RestClientwith lifecycle shutdown management, and injects it into ES services. - Adds comprehensive unit tests for capability verdict logic + resource wiring, and adds OpenAPI path/schema plus supporting docs/DEVNOTES updates.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/org/broadinstitute/consent/http/service/ElasticSearchCapabilityService.java | Implements the capability probing logic (read-only + optional write probes) and report assembly |
| src/main/java/org/broadinstitute/consent/http/resources/ElasticSearchCapabilityResource.java | Adds the admin-only API endpoint exposing the capability report |
| src/main/java/org/broadinstitute/consent/http/models/elastic_search/ElasticSearchCapabilityReport.java | Defines the JSON response model for the capability report |
| src/main/java/org/broadinstitute/consent/http/models/elastic_search/ElasticSearchCapability.java | Defines a single capability line item in the report |
| src/main/java/org/broadinstitute/consent/http/models/elastic_search/CapabilityVerdict.java | Adds verdict enum values distinguishing observed vs inferred vs blocked vs unknown outcomes |
| src/main/java/org/broadinstitute/consent/http/ConsentModule.java | Provides a singleton ES RestClient, registers shutdown, and wires the new capability service |
| src/main/java/org/broadinstitute/consent/http/ConsentApplication.java | Registers the new capability resource |
| src/main/resources/assets/api-docs.yaml | Adds the new endpoint to the top-level OpenAPI doc |
| src/main/resources/assets/paths/elasticSearchCapabilities.yaml | Defines the OpenAPI path contract for the new endpoint |
| src/main/resources/assets/schemas/ElasticSearchCapabilityReport.yaml | Defines the OpenAPI schema for the report payload |
| src/test/java/org/broadinstitute/consent/http/service/ElasticSearchCapabilityServiceTest.java | Adds detailed unit tests for probing/verdict behavior, including write-probe teardown assertions |
| src/test/java/org/broadinstitute/consent/http/resources/ElasticSearchCapabilityResourceTest.java | Adds resource-level tests for parameter passing and error handling |
| src/test/java/org/broadinstitute/consent/http/ConsentModuleTest.java | Adds DI/lifecycle tests ensuring the ES client is shared and shut down |
| docs/plans/es-security-capability-record.md | Documents how to produce and record per-environment findings and the D/E decision rule |
| DEVNOTES.md | Updates local ES development instructions to match the current compose-based setup |
Comments suppressed due to low confidence (1)
src/test/java/org/broadinstitute/consent/http/service/ElasticSearchCapabilityServiceTest.java:179
- This helper uses Mockito
lenient()stubbing, which this repo’s guidelines explicitly discourage (see.github/copilot-instructions.md). You can avoid lenient stubs here by only stubbinggetRequestLine()when you’re constructing an error response (status >= 300), since it’s only needed forResponseException.
private Response response(int status, String body) {
Response response = mock(Response.class);
…y removing refs to it, improve coverage.
rushtong
left a comment
There was a problem hiding this comment.
This is great work, thank you 👍🏽
I had one suggestion (inline) and Claude had two minor, non-blocking observations:
- No multi-node failover during write probes — defaultApiKeyClientFactory (ElasticSearchCapabilityService.java:1018-1024) builds its API-key client from only esClient.getNodes().get(0).getHost(), unlike the primary client which resolves the full host list (or cloud ID) via ElasticSearchSupport.createRestClient. I verified createRestClient adds no custom TLS trust store or timeout config beyond JVM defaults, so this doesn't introduce a new SSL/timeout failure mode — the only real gap is that a probe could fail to fail over if node 0 happens to be down during a multi-node cluster's write-probe pass. Low impact for a diagnostic endpoint.
- Write side effects behind a GET — writeProbes=true creates and deletes real API keys/roles on the cluster, but is triggered via GET with a query parameter rather than POST. This breaks HTTP's "GET is safe" convention; since auth is bearer-token (not cookie-based per the documented curl -H "Authorization: Bearer ..." usage), CSRF risk is minimal, but a URL like .../capabilities?writeProbes=true could be accidentally re-triggered by link prefetching, monitoring crawlers, or anything that follows GET links if ever bookmarked/shared. Worth a passing comment, not a blocker — this is an admin-only, intentionally temporary diagnostic tool per the PR description.
kevinmarete
left a comment
There was a problem hiding this comment.
Looking good, just some comments from agent review:
It found two blocking correctness issues and one lower-priority privilege-handling issue that do not appear to be covered by the existing review comments.
[P1] Respect xpack.security.dls_fls.enabled
dlsFlsCapability() reads xpack.security.dls_fls.enabled, but does not use its value when determining the verdict. A cluster with a qualifying license and xpack.security.dls_fls.enabled=false will be reported as INFERRED_SUPPORTED, and the recommendation will incorrectly say Epic D is viable.
Please return UNAVAILABLE or INFERRED_UNAVAILABLE when this setting is explicitly disabled, and incorporate the setting into the recommendation logic.
[P1] Do not report inconclusive enforcement as supported
When the dataset index is empty or unreadable, no usable probe key exists, or the FLS response has no inspectable fields, enforcement is not tested. However, the code retains the role-acceptance verdict of SUPPORTED. The recommendation then states that the filters were “accepted and enforced.”
Role acceptance does not prove enforcement. These paths should remain UNKNOWN or INFERRED_SUPPORTED, and the recommendation should only claim enforcement after a completed end-to-end enforcement probe.
[P2] Align API-key privilege detection with the endpoint being called
The probe creates keys through POST /_security/api_key, but canMint checks manage_api_key || grant_api_key.
The create-key endpoint uses manage_own_api_key or manage_api_key; grant_api_key applies to the separate /api_key/grant endpoint. As written:
- A credential with only
manage_own_api_keyis incorrectly reported as unable to create keys. - A credential with only
grant_api_keyis predicted to work, but this probe’s create-key request will be rejected.
Please align the privilege check and API documentation with the endpoint the probe actually uses.
kevinmarete
left a comment
There was a problem hiding this comment.
The substantive review findings have been resolved—thank you. One stale instruction remains after moving write probes to POST: deploymentNotes() still tells callers to “Re-run with writeProbes=true,” and the PR description still documents the old GET query parameter.
Please update both to instruct callers to:
POST /api/elasticSearch/capabilities
|



Addresses
https://broadworkbench.atlassian.net/browse/DT-3825
Summary
Ticket A-1 needs a written record of which Elasticsearch security features (X-Pack Security,
DLS, FLS, API keys,
run_as) are available in each Consent environment, because that recorddecides between Epic D (native DLS/FLS enforcement in the cluster) and Epic E
(application-layer filtering).
Getting that record previously meant either cluster network access or a copy of each
environment's credential. Neither is available, and neither should be needed: every environment
already runs a Consent deployment holding its own cluster credential. This adds an admin-only
endpoint that reports the inventory from inside the application, so an Admin token per
environment is the only thing required.
What this adds
GET /api/elasticSearch/capabilities(@RolesAllowed(ADMIN)), backed byElasticSearchCapabilityService. It reports cluster name, version, distribution(Elasticsearch vs. OpenSearch), edition/license, X-Pack Security state, the authenticated
principal and its roles, the credential's cluster privileges, the security-relevant cluster
settings, a capability verdict per feature, REST-client compatibility, a recommendation, and
interpretive notes.
Two modes:
support cannot be proven without writing, so they come back
INFERRED_SUPPORTED/LICENSE_BLOCKED, reasoned from the license tier.run_asand X-Pack Security are observed.?writeProbes=true— mints a short-lived API key and authenticates as it, creates a rolecarrying a DLS query and an FLS grant, then searches the real dataset index through keys
carrying those filters to check the cluster enforces them (a
match_noneDLS key mustreturn zero documents; a key granting one field must return only that field). This matters
because a Basic-licensed cluster accepts a DLS role descriptor at creation and fails only
later at search time — a failure mode no license inference can predict.
?runAsUser=<name>optionally targets therun_asprobe at a specific user instead of thecredential's own principal.
Verdicts distinguish the things that are easy to conflate
CapabilityVerdictseparatesLICENSE_BLOCKED(the cluster's answer) fromNOT_PERMITTED(this credential's answer) from
UNAVAILABLE(absent, or accepted-and-silently-ignored) fromthe
INFERRED_*values. Only a license verdict is a fact about the cluster, so only it shouldcarry weight in the Epic D/E decision. When the deployment's credential lacks
manage_security/manage_api_key, the write probes report themselves as inconclusive andfall back to the license reading rather than recording a privilege refusal as a verdict against
the native path.
Probe safety
Everything created is namespaced
duos-capability-probe-*/duos_dlsfls_probe_*, carries a10-minute expiry regardless of what happens next, is torn down in a
finallyblock, and anyteardown failure is surfaced in the response
notesrather than left in the server log. Everyprobe key carries an explicit
role_descriptorsblock — including the plain round-trip key,whose descriptor grants nothing — so no key inherits a snapshot of the deployment credential's
permissions.
Wiring change: one shared
RestClientConsentModulenow providesRestClientas a singleton and registers a namedElasticSearchClientShutdownManagedto close it.ElasticSearchServicetakes the injectedclient rather than building its own. Each
RestClientowns a connection pool and backgroundthreads, so a client per consumer multiplied pools against the same cluster and left them
unclosed at shutdown.
Removed
scripts/es-security-audit.sh— reimplemented the whole verdict matrix in untested bash.ElasticSearchSecurityProbeTest— drove the security APIs through the productionElasticSearchSupport.createRestClientpath but was inert unlessES_PROBE_URLwas set, soit never ran in CI.
The service supersedes both: same calls, unit-tested verdict logic, and a successful response
from any environment is itself the REST-client compatibility evidence the probe test existed to
supply. Two implementations that can disagree about "is DLS enforced here" are worse than one
that is tested.
Docs
docs/plans/es-security-capability-record.md— the record itself: how it's produced, themeasured local and control-cluster results, empty tables awaiting
dev/staging/production, the REST-client compatibility conclusion, and the decision rule fixed in advanceso the measurement determines the outcome.
DEVNOTES.md— the local ES section rewritten; it described a 5.5.0 image and manual composeedits that no longer match the shipped
config/docker-compose.yaml. Now covers running withsecurity on, the trial-license-on-an-existing-volume trap, and
ES_SECURITY_ENABLED=falsefor a security-disabled run.paths/elasticSearchCapabilities.yamlandschemas/ElasticSearchCapabilityReport.yaml, referenced fromapi-docs.yaml.Findings so far
Measured against a local trial-licensed 9.4.4 cluster in write-probe mode, all five
capabilities came back
SUPPORTED, with DLS observed enforced (0 of 1158 documents through amatch_nonekey) and FLS observed projecting. Control clusters on 9.3.3 and 9.4.4 establishthe tier baseline: API keys and
run_asare Basic-tier features; DLS and FLS are not.Epic D therefore carries a license dependency Epic E does not.
The decision stays pending — local runs as the
elasticsuperuser, which is not the sharedcredential any deployed environment uses. The
dev,staging, andproductionrows are theremaining work, and dev is the right place to run write probes first in a deployed environment.
Testing
ElasticSearchCapabilityServiceTest— 32 tests over a mockedRestClient, keyed bymethod + endpoint (and by role descriptor where three key creations share both), covering
each verdict path in both modes and both license tiers.
ElasticSearchCapabilityResourceTest— 4 tests.ConsentModuleTest— 3 new tests: the client is a singleton, both ES services share it(exactly one shutdown registration), and it is actually closed on
stop().To exercise it against a live cluster:
Have you read CONTRIBUTING.md lately? If not, do that first.