diff --git a/DEVNOTES.md b/DEVNOTES.md index 1a21c7305..4ee588e80 100644 --- a/DEVNOTES.md +++ b/DEVNOTES.md @@ -72,15 +72,19 @@ and the defaults should be correct. ### Developing with a local Elastic Search instance: #### Running with X-Pack Security enabled -Security is **on by default**, which is what the security work (`GET /api/elasticSearch/capabilities` -and native DLS/FLS) needs. Two things make that work, and they have to agree: +`config/` is not version-controlled — it is rendered per developer (see "Render Configs" above) — so +none of this arrives with a `git pull`; it has to go into your own `config/docker-compose.yaml`. The +example stanza below turns security **on**, which is what the security work +(`GET /api/elasticSearch/capabilities` and native DLS/FLS) needs, and keeps a +`${ES_SECURITY_ENABLED:-true}` gate so you can drop back to an unauthenticated cluster for a single +run. Two things make it work, and they have to agree: * `ELASTIC_PASSWORD` in the compose `elastic` service bootstraps the `elastic` superuser. * `authUser` / `authPassword` in `config/consent.yaml` are the credentials Consent sends. They are harmless when security is off — the ES client only sends credentials in response to a 401 challenge, which a security-disabled cluster never issues. -DLS and FLS are Platinum features, so the compose file self-generates a 30-day **trial** license via +DLS and FLS are Platinum features, so the stanza self-generates a 30-day **trial** license via `xpack.license.self_generated.type`. That setting only applies the first time a cluster forms. If your `elastic` data volume predates it, the cluster keeps its `basic` license and DLS/FLS come back `LICENSE_BLOCKED`. Activating the trial is a separate, deliberate step — never something a @@ -105,7 +109,7 @@ curl -s -u elastic:devpassword localhost:9200/_license # expect "type": "trial Note that transport SSL stays disabled. ES logs a bootstrap warning about it, which is expected and correct here — transport SSL is only required for multi-node clusters. -To get the old security-disabled cluster back for a run, without editing the committed file: +To get an unauthenticated cluster back for a run, without editing your compose file again: ```bash ES_SECURITY_ENABLED=false docker-compose -p consent -f config/docker-compose.yaml up @@ -126,20 +130,21 @@ An example docker-compose stanza for elastic: memory: 4gb environment: - "ES_JAVA_OPTS=-Xms2g -Xmx2g" - # X-Pack Security is OFF by default so the default `docker compose up` behaves exactly as - # before. Epics A-C and E (application-layer fallback) need no security. To work on Epic D - # (native DLS/FLS), start the stack with security on: + # X-Pack Security is ON here: the capability endpoint and Epic D (native DLS/FLS) need it. + # Epics A-C and E (application-layer fallback) do not, so to run one session unauthenticated: # - # ES_SECURITY_ENABLED=true docker-compose -p consent -f config/docker-compose.yaml up + # ES_SECURITY_ENABLED=false docker-compose -p consent -f config/docker-compose.yaml up # - # DLS/FLS is a Platinum feature, so also activate the 30-day trial license once per major version per cluster -- - # either through Consent's admin endpoint or straight at the cluster: + # See DEVNOTES.md ("Developing with a local Elastic Search instance") for the full workflow. + - xpack.security.enabled=${ES_SECURITY_ENABLED:-true} + # DLS/FLS is a Platinum feature, so self-generate a trial rather than the default basic + # license. Only applies the first time a cluster forms — on an existing `elastic` volume, + # activate it by hand instead, once per major version per cluster, either through Consent's + # admin endpoint or straight at the cluster: # # curl -X POST 'localhost:8000/api/elasticSearch/license/trial?acknowledge=true' # curl -u elastic:devpassword -XPOST 'localhost:9200/_license/start_trial?acknowledge=true' - # - # See DEVNOTES.md ("Developing with a local Elastic Search instance") for the full workflow. - - xpack.security.enabled=${ES_SECURITY_ENABLED:-true} + - xpack.license.self_generated.type=${ES_LICENSE_TYPE:-trial} # Bootstraps the `elastic` superuser password when security is on; ignored when it is off. # Must match authUser/authPassword in consent.yaml. - ELASTIC_PASSWORD=${ELASTIC_PASSWORD:-devpassword} @@ -151,13 +156,11 @@ An example docker-compose stanza for elastic: I also suggest changing the default bucket location so uploaded ontology files do not interfere with other dev environments. -#### Running local Elastic Search with security enabled - -By default the local cluster runs with `xpack.security.enabled=true`: requests are -now authenticated, which is what most work needs. Only work on native Elasticsearch document- and -field-level security (DLS/FLS) requires security to be on. Application-layer authorization work -does not — it never touches Elasticsearch security. +#### Which work actually needs security enabled +Only the capability endpoint and native Elasticsearch document- and field-level security (DLS/FLS) +require it. Application-layer authorization work does not — it never touches Elasticsearch security — +so if you are not working on those, running with `ES_SECURITY_ENABLED=false` is fine. #### Enabling DLS/FLS locally (trial license required) diff --git a/docs/plans/elasticsearch-service-duos-ui-usage.md b/docs/plans/elasticsearch-service-duos-ui-usage.md index 71a1ad8c3..df6902434 100644 --- a/docs/plans/elasticsearch-service-duos-ui-usage.md +++ b/docs/plans/elasticsearch-service-duos-ui-usage.md @@ -397,12 +397,21 @@ The dataset index stores `DatasetTerm` documents with nested objects used by sea - `dataTypes` - `assets` (`Map`; includes study asset collections) - `data` (`Map`) +- `externalIdentifier` +- `externalIdentifierType` ### UserTerm and DacTerm nested fields - `submitter` / `updateUser` (`UserTerm`): `userId`, `displayName`, `institution` + (`InstitutionTerm`: `id`, `name`) - `dac` (`DacTerm`): `dacId`, `dacName`, `dacEmail` +> **Access classification lives elsewhere.** Every path above is classified SEARCH-VISIBLE or +> INTERNAL in [`es-access-contract.md`](es-access-contract.md) §B, which is enumerated from the model +> classes and is the authoritative list. Adding a field to `DatasetTerm`, `StudyTerm`, `UserTerm`, +> `DacTerm`, or `InstitutionTerm` requires classifying it there in the same change — an unclassified +> field is dropped by the E-3 allowlist and omitted from the D-3 field grant. + ## Test, Config, and Docs Touchpoints In duos-ui Test files with stubs/intercepts tied to these paths: @@ -447,9 +456,9 @@ Size key: **S** ≈ 1 day, **M** ≈ 2–3 days, **L** ≈ 4–5 days, **XL** | Epic | Name | Owner | Blocked by | Blocks | | --- | --- | --- | --- | --- | | A | Discovery & Contract | Infra + Backend | — | B, C, D, E | -| B | Index Schema & Indexing Pipeline | Backend | A | D, E, F | -| C | Auth Context Service | Backend | A | D, E | -| D | Native DLS/FLS Path | Backend + Infra | A, B, C | F | +| B | Index Schema & Indexing Pipeline | Backend | A-2, A-3 (not A-1) | D, E, F | +| C | Auth Context Service | Backend | A-2 (not A-1) | D, E | +| D | Native DLS/FLS Path | Backend + Infra | A-1, B, C | F | | E | Compatibility Fallback | Backend | B, C | F | | F | API Hardening | Backend | D or E | G | | G | Frontend Alignment | Frontend | F | — | @@ -461,7 +470,19 @@ Size key: **S** ≈ 1 day, **M** ≈ 2–3 days, **L** ≈ 4–5 days, **XL** **Goal**: Establish facts about the Elasticsearch cluster's security capabilities, evaluate the local developer configuration changes needed, and define the formal access contract that all later -epics are built on. All other epics are blocked on A-1. +epics are built on. + +**Status**: A-0 closed. A-1 has local, control-cluster, and production measurements recorded in +[`es-security-capability-record.md`](es-security-capability-record.md); dev and staging remain, and +they decide Epic D vs. E. A-2 is delivered and complete as +[`es-access-contract.md`](es-access-contract.md) — every dimension and field is decided, and its +remaining OPEN items are proposed *changes* to current behavior, each with a preserve-today default, +so none of them blocks Epics B or C. + +**Note on the blocking relationships**: only the *enforcement mechanism* (Epic D vs. E) is blocked on +A-1. The access contract is not, and was deliberately written to be mechanism-neutral — the rules +must be identical under native DLS/FLS and under the mediated fallback, or the fallback becomes a +hole. Epics B and C are blocked on A-2, not A-1. --- @@ -522,8 +543,12 @@ layer and requires no Elasticsearch configuration changes. ##### A-0 Outcome -**Decision: Option B** — security is env-var gated in `config/docker-compose.yaml`, default off. -Findings below were verified empirically against `elasticsearch:9.4.4` using throwaway containers +**Decision: Option B** — security is env-var gated in `config/docker-compose.yaml` rather than +unconditionally on. The recommended default *inside* that gate later moved from off to on, once the +capability endpoint made a secured local cluster useful beyond Epic D (DEVNOTES.md); since `/config/` +is not version-controlled (see the note at the end of this section), that default is per-developer +either way, and the gate is the part of the decision that carries. Findings below were verified +empirically against `elasticsearch:9.4.4` using throwaway containers running the exact env block from `config/docker-compose.yaml`. (Originally established on 9.3.3 and re-verified on 9.4.4 when the pin moved; every finding reproduced unchanged, including the exact error strings. The only observed delta was the bundled Lucene version, 10.3.2 → 10.4.0.) @@ -576,9 +601,10 @@ Consequences: **Verified compose behavior (both modes)** -The `elastic` service now uses `xpack.security.enabled=${ES_SECURITY_ENABLED:-false}`, +The env block verified here was `xpack.security.enabled=${ES_SECURITY_ENABLED:-false}`, `ELASTIC_PASSWORD=${ELASTIC_PASSWORD:-devpassword}`, and explicit -`xpack.security.http.ssl.enabled=false`. Confirmed with the exact env block: +`xpack.security.http.ssl.enabled=false`. Both modes were exercised, so the table holds whichever way +the gate defaults — and the recommended default has since moved to `:-true` (DEVNOTES.md): | Mode | Unauthenticated | Authenticated | HTTPS on 9200 | | --- | --- | --- | --- | @@ -707,25 +733,53 @@ per-request credential work can begin, we need the cluster edition and security in the Elasticsearch index. This ticket defines the contract that shapes both the `accessPolicy` nested object (Ticket B-1) and the auth context resolver (Ticket C-1). -**Acceptance criteria**: -- Completed matrix: for each dimension (`publicVisibility`, ADMIN bypass, creator, custodian, DAC - member/chair, institution allowlist, policy tags) record: data source, enforcement level (DLS - filter vs. FLS field bundle), and whether persistent backing currently exists. -- Field-level security groupings decided: e.g. `"public"` profile grants `datasetName`, - `datasetId`, `study.studyName`; `"privileged"` additionally grants `study.dataCustodianEmail`, - `study.dataSubmitterEmail`. -- `publicVisibility` DLS semantics decided: invisible to non-privileged callers, or visible with - field redaction? +**Deliverable**: [`es-access-contract.md`](es-access-contract.md) — **written**. The sections below +record what it settled and what it deliberately did not. + +**Acceptance criteria** (all met except where noted): +- ✅ Completed matrix: for each dimension (`publicVisibility`, ADMIN bypass, dataset creator, study + creator, custodian, DAC member/chair, institution allowlist, policy tags) record data source, + enforcement level, persistent backing, **and whether the contract preserves current behavior or + expands it** — contract §A. Fourteen dimensions; eight PRESERVE, six DEFER. +- ✅ Field-level security groupings decided — contract §B, classifying **every** indexed path from + the model classes into SEARCH-VISIBLE or INTERNAL, with dynamic maps (`data`, `assets`) INTERNAL + and no wildcard grants permitted — including for admins, who bypass document filtering but not + field filtering. +- ✅ `publicVisibility` DLS semantics decided — contract Decision 1: restricted documents are + **invisible**, not redacted. Redaction is also not expressible in native FLS; see below. +- ✅ `publicVisibility = NULL` resolved — contract §A.1. It first looked like an undecidable policy + question because the two code paths read a null differently, but `study.public_visibility` is + `NOT NULL` and the nulls that actually occur come from the summary query's `LEFT JOIN` — i.e. they + are the "dataset has no study" case, which both paths already allow. Nothing was left for an owner + to decide. + +**Findings that change other tickets**: +- **A per-document `fieldAccessProfile` cannot drive native FLS.** Field grants live in the + credential's index privileges and apply uniformly to every document a search request matches; + nothing re-selects a grant per hit. B-2 is invalidated as written and B-1 must drop the field — + contract Decision 2. +- **Four dimensions in B-1's `AccessPolicyTerm` grant no access today** (DAC membership/chair, + institution, policy tags, principal allowlists). Populating them into a DLS filter would be a + silent authorization expansion, so they are DEFERred pending OPEN-3/OPEN-5. +- **Dataset creator and study creator are distinct** (different columns, both privileged paths); + B-1/B-3 must index both document-side IDs, and D-3/E-2 must compare the caller's one user ID + against both. +- **Custodian matching is case-sensitive and trims only the stored side**, so `Alice@x.org` fails + against a stored `alice@x.org` — contract §A.2, OPEN-6. **Implementation notes**: -- Start from the `StudyTerm` fields listed in the Indexed Elements section of this document. Flag - every field containing PII or internal-only data. -- `dataCustodianEmail` is currently parsed from the study property bag in - `DatasetService.isCreatorOrCustodian` (L224–238), not a dedicated DB column — note this as a - storage gap candidate. - -**Dependencies**: A-1. -**Size**: S +- `dataCustodianEmail` is parsed from the study property bag in `DatasetService.isCreatorOrCustodian` + (L220–236), not a dedicated DB column. Contract §C records what that does and does not cost, and + corrects A-3's "no new storage needed" framing. +- The Indexed Elements section of this document is incomplete (it omits `study.externalIdentifier`, + `study.externalIdentifierType`, and `UserTerm.institution` sub-fields). Contract §B is enumerated + from the model classes and supersedes it. + +**Dependencies**: A-1 — **satisfied for this ticket's purposes.** A-1's outstanding dev/staging rows +choose the enforcement *mechanism* (Epic D vs. E); the contract is stated in mechanism-neutral terms +because it must be identical either way, so it was not held for them. +**Size**: S — **actual: L.** The exhaustive field classification and the behavior-preservation audit +were the bulk of it. --- @@ -745,10 +799,23 @@ source for every field. Dimensions such as `allowedInstitutionIds`, `allowedPrin separate dataset-to-institution mapping table. **Implementation notes**: -- `dacId` already exists on `Dataset`; `dataCustodianEmail` already exists in the study property - bag — no new storage needed for those dimensions. +- `dacId` already exists as a column on `Dataset` — genuinely no new storage. +- `dataCustodianEmail` is **not** the same case, and the earlier "no new storage needed" framing was + wrong. Persistent backing exists, but as an unstructured JSON array inside the `study_property` + bag: no referential integrity (access is granted to a string, not a principal), no normalization, + no index, and no defined behavior for malformed values. [`es-access-contract.md`](es-access-contract.md) + §C records this in full. It does **not** block Epics B–E, because B-3 denormalizes custodian emails + into `accessPolicy` at index time — but it does make reindex-on-custodian-change a correctness + requirement (B-4), it leaves the non-search endpoints parsing the bag, and OPEN-6 (case + normalization) has to be answered either way. Whether custodianship should become a first-class + relation is this ticket's call to make. - `allowedPrincipalIds` (explicit user allowlists) and `policyTags` (consent-code-based access - tags) are the most likely to require new storage. + tags) are the most likely to require new storage — but answer contract **OPEN-5** first: none of + them corresponds to a current requirement, and if the answer is "not now," the storage question + does not arise and B-1 should drop the fields. +- Institution allowlists cannot be derived from `User.institutionId` alone: there is **no + dataset-to-institution mapping** of any kind today, so this dimension needs a storage decision + before it can mean anything at all. **Dependencies**: A-2. **Size**: M @@ -775,17 +842,30 @@ Currently it carries no structured access metadata; all visibility logic lives i This ticket adds the schema without populating it yet (population is B-3). **Acceptance criteria**: -- New `AccessPolicyTerm` class with fields: - - `publicVisibility: boolean` - - `creatorUserId: Integer` - - `creatorEmail: String` - - `custodianEmails: List` - - `dacId: Integer` - - `dacApproval: Boolean` - - `allowedInstitutionIds: List` - - `allowedPrincipalIds: List` - - `policyTags: List` - - `fieldAccessProfile: String` — `"public"` or `"privileged"` +- New `AccessPolicyTerm` class. **Field set revised by the A-2 contract** — see + [`es-access-contract.md`](es-access-contract.md) §D: + - `publicVisibility: Boolean` + - `hasStudy: Boolean` — carries contract §A row 5 (a dataset with no study is readable by + everyone today). Required because the filter treats a null `publicVisibility` on a study-bearing + document as *not* public, so "no study" cannot be expressed as an absent visibility. + - `datasetCreatorUserId: Integer` — the dataset's creator (`dataset.create_user_id`) + - `studyCreatorUserId: Integer` — the study's creator (`study.create_user_id`), a *different* + privileged path; contract §A rows 6 and 7 + - `custodianEmails: List` — trim surrounding whitespace on each stored value, matching + today's `custodian.trim()` behavior, but preserve case (contract §A.2). Lowercasing or otherwise + normalizing here would authorize case-mismatched custodians through search while the dataset + endpoints still rejected them; OPEN-6 proposes fixing both paths together. + - `dacId: Integer` — indexed for display/filtering parity, **not** consulted for authorization + while contract rows 9–10 are DEFERred + - ~~`creatorEmail`~~ — dropped; creator matching is by user ID (contract §A.2) + - ~~`dacApproval`~~ — dropped; it is a display attribute, not authorization (contract row 11) + - ~~`allowedInstitutionIds`, `allowedPrincipalIds`, `policyTags`~~ — **do not add** until OPEN-5 + establishes that they are requirements. None has storage or current behavior; shipping them + unpopulated invites a later reader to treat them as enforcement. + - ~~`fieldAccessProfile`~~ — **removed.** Native FLS cannot select a field grant per document + (contract Decision 2). +- Class-level comment recording that **every** `accessPolicy` path is INTERNAL and must never appear + in a field grant (contract §B.4). - `DatasetTerm` gains an `accessPolicy: AccessPolicyTerm` field. - Elasticsearch index mapping updated with `accessPolicy` as a `nested` (or `object`) type — confirm with A-1 outcome which is required for the DLS query approach. @@ -804,26 +884,26 @@ This ticket adds the schema without populating it yet (population is B-3). --- -#### Ticket B-2 — Add `fieldAccessProfile` marker to `DatasetTerm` +#### Ticket B-2 — ~~Add `fieldAccessProfile` marker to `DatasetTerm`~~ — **CANCELLED by A-2** -**Summary**: Add a `fieldAccessProfile` string field (inside `AccessPolicyTerm`) to signal which -FLS field bundle applies to the document. +**Do not implement.** The mechanism this ticket specifies does not exist in Elasticsearch. -**Context**: FLS in Elasticsearch requires knowing which fields each document grants to which -caller profile. A profile marker on the document allows the auth context (C-1) to request a -field-grant list matched to the document's declared profile, without enumerating fields per -document in the query path. +It assumed the auth context could read a profile marker off each document and request a matching +field grant. Field grants are declared in the credential's index privileges and are evaluated when +the request is authorized, then applied uniformly to every document that privilege matches; there is +no stage at which the cluster inspects a hit and re-selects a grant for it. +([Elastic: controlling access at document and field level](https://www.elastic.co/docs/deploy-manage/users-roles/cluster-or-deployment-auth/controlling-access-at-document-field-level)) -**Acceptance criteria**: -- `AccessPolicyTerm.fieldAccessProfile` present (this may overlap with B-1; keep as a separate - deliverable to track separately). -- Valid values: `"public"` (`publicVisibility=true`), `"privileged"` (`publicVisibility=false` or - restricted fields present). -- Unit test: `fieldAccessProfile` is `"public"` for a dataset whose study has - `publicVisibility=true`, and `"privileged"` for `publicVisibility=false`. - -**Dependencies**: B-1. -**Size**: S +The obvious replacement — a request-wide privileged bundle selected from the caller — is also +unsafe. Creator and custodian privilege is document-scoped, while the same DLS request also returns +unrelated public datasets. A privileged request-wide grant would therefore expose privileged fields +from those unrelated documents. See [`es-access-contract.md`](es-access-contract.md) Decision 2. + +**Replacement work**: D-3 and E-3 apply the single SEARCH-VISIBLE allowlist to every caller, +including ADMIN. C-1 derives document-visibility context only; it derives no field bundle. No +separate ticket is needed. + +**Size**: — (removed from the plan) --- @@ -837,16 +917,27 @@ and `institutionDAO` (L58–66). All data needed for `accessPolicy` is reachable the mapping. **Acceptance criteria**: -- `accessPolicy.publicVisibility` ← `dataset.getStudy().getPublicVisibility()` (null-safe). -- `accessPolicy.creatorUserId` ← `dataset.getCreateUserId()`. -- `accessPolicy.creatorEmail` ← `userDAO.getUserById(createUserId).getEmail()`. -- `accessPolicy.custodianEmails` ← parsed from study property bag using the same logic as - `DatasetService.isCreatorOrCustodian` (L224–238). +- `accessPolicy.publicVisibility` ← `dataset.getStudy().getPublicVisibility()` (null-safe). The + column is `NOT NULL`, so a study-bearing dataset always has a real value; the filter treats an + unexpected null as *not* public (contract §A.1). +- `accessPolicy.hasStudy` ← `dataset.getStudyId() != null`. This is what carries contract §A row 5 + — a dataset with no study is readable by everyone today, and that must not be expressed as a null + `publicVisibility`. +- `accessPolicy.datasetCreatorUserId` ← `dataset.getCreateUserId()`. +- `accessPolicy.studyCreatorUserId` ← `dataset.getStudy().getCreateUserId()` — a separate privileged + path from the dataset creator (contract §A rows 6/7), not a duplicate of it. +- `accessPolicy.custodianEmails` ← parsed from the study property bag as in + `DatasetService.isCreatorOrCustodian` (L220–236), preserving that method's exact matching + semantics — apply the same `trim()` on the stored side and **no** case normalization (contract + §A.2). Malformed or non-array property values must not throw out of indexing. - `accessPolicy.dacId` ← `dataset.getDacId()`. -- `accessPolicy.dacApproval` ← `dataset.getDacApproval()`. -- `accessPolicy.fieldAccessProfile` ← `"public"` if `publicVisibility=true`, else `"privileged"`. -- `allowedInstitutionIds` ← per outcome of A-3; empty list if not yet implemented. -- Unit test: null study → `accessPolicy.publicVisibility` defaults to `false`, no NPE. +- ~~`creatorEmail`, `dacApproval`, `fieldAccessProfile`, `allowedInstitutionIds`~~ — not populated; + see the revised B-1 field set. +- **A dataset with no study must remain readable by everyone** (contract §A row 5 — that is current + behavior). Do not default a null study to `publicVisibility=false`; that would hide datasets that + are visible today. Represent "no study" explicitly so the DLS filter can match it. +- Unit test: null study → no NPE, and the resulting document is readable by a caller with no + relationship to it. **Implementation notes**: - `isCreatorOrCustodian` in `DatasetService` (L224–238) parses custodian email from @@ -856,7 +947,8 @@ the mapping. - Guard all `study` accesses — datasets created outside the registration flow may have a null study reference. -**Dependencies**: B-1, B-2, A-3. +**Dependencies**: B-1. A-3 is not required for the current field set; its speculative dimensions are +deferred pending OPEN-5. **Size**: M --- @@ -931,16 +1023,21 @@ cannot add nested types to a live index. The active index is identified by and `toStudyTerm` before any auth enforcement code is written against them. **Acceptance criteria**: -- `publicVisibility=true` → `fieldAccessProfile == "public"`. -- `publicVisibility=false` → `fieldAccessProfile == "privileged"`. +- `publicVisibility` flows through for `true` / `false`. +- A dataset with no study indexes `hasStudy=false` and remains readable by an unrelated caller + (contract §A row 5) — the case a null `publicVisibility` would otherwise have to carry. - `custodianEmails` populated when study has `dataCustodianEmail` property. - `custodianEmails` is empty (not null) when study has no custodian property. -- Null study → `publicVisibility` defaults to `false`, no NPE. -- `dacId`, `dacApproval`, `creatorUserId`, `creatorEmail` flow through from dataset and user DAO. +- `custodianEmails` preserves case: `" Alice@X.org "` in the property bag indexes as `Alice@X.org` + (trimmed, not lowercased), so search authorizes exactly the callers `DatasetService` does today. +- Malformed `dataCustodianEmail` (not a JSON array, unparseable) does not throw out of indexing. +- Null study → no NPE, and the document remains readable by an unrelated caller (contract row 5). +- `dacId`, `datasetCreatorUserId`, `studyCreatorUserId` flow through — with a case where the dataset + creator and study creator are **different users**, since conflating them is the likely bug. +- No `accessPolicy` field is present in the SEARCH-VISIBLE projection (contract §B.4). **Implementation notes**: - Mirror the existing mock-heavy pattern in `ElasticSearchServiceTest` — mock all DAO calls. -- Use `@ParameterizedTest` for the `fieldAccessProfile` cases. **Dependencies**: B-3. **Size**: M @@ -969,24 +1066,32 @@ not once per document. **Acceptance criteria**: - `DatasetSearchAuthContext` (record or immutable class) with: - - `Integer userId` - - `String userEmail` - - `Integer institutionId` + - `Integer userId` — matches **both** the dataset creator and the study creator dimensions, which + are distinct columns and distinct privileged paths ([`es-access-contract.md`](es-access-contract.md) + §A rows 6 and 7); the filter must test both, not one + - `String userEmail` — passed through **unnormalized**, for the exact custodian matching the + dataset endpoints do today (contract §A.2) - `boolean isAdmin` - - `Set dacMemberships` — all DAC IDs the user belongs to - - `Set dacChairScopes` — DAC IDs where the user is chair - - `List policyTagGrants` — initially empty, placeholder for future policy-tag grants + - **No field-bundle field.** Search serves one bundle to every caller (contract Decision 2), so + there is nothing per-caller to derive. A per-caller `privileged` bundle was considered and + rejected: creator/custodian privilege is document-scoped, and a caller privileged on one dataset + also receives every public dataset through the same DLS filter, so a request-wide privileged + grant would project privileged fields out of unrelated documents. + - **No institution, DAC-membership/chair, principal-allowlist, or policy-tag fields.** None is + consumed by current read authorization; resolving speculative context adds queries and invites a + later implementation to feed it into DLS accidentally. Add a field only with the signed-off + requirement that consumes it (contract rows 9–14, OPEN-3/OPEN-5). - `DatasetSearchAuthContextResolver` service: accepts a `DuosUser`, returns a `DatasetSearchAuthContext`. - `isAdmin` is `true` when user has `UserRoles.ADMIN` (L13 in `UserRoles.java`). -- `dacMemberships` loaded from `DacDAO` — do not pull in `DacService` as a dependency to keep the - graph flat. +- No DAC lookup is performed; DAC membership and chair status grant no search read access today. **Implementation notes**: - `DuosUser.getRoles()` returns the role set; check for `UserRoles.ADMIN`. -- Keep the resolver stateless; all DB calls happen eagerly in the constructor/factory, not lazily. -- `DacService` (L51) already resolves DAC memberships — use `dacDAO` directly to avoid - introducing a circular dependency through `DacService`. +- Keep the resolver stateless and derive the context entirely from the supplied `DuosUser`; the + current contract requires no DAO lookup. +- Do not inject `DacDAO` or `DacService` until OPEN-3 is approved. The current resolver needs no + DAC dependency. **Dependencies**: A-2. **Size**: M @@ -1030,17 +1135,19 @@ the two code paths from diverging. `DatasetSearchAuthContext` and `DatasetAccessPolicy`. **Acceptance criteria**: -- Test matrix: `ADMIN`, public reader (`RESEARCHER`/`MEMBER`/`SIGNINGOFFICIAL`), dataset creator, - study custodian, DAC chair for the dataset's DAC, DAC member (not chair), - institution-restricted user, user with no matching institution. -- Each combination tested for `canRead`, `isCreator`, `isCustodian`. +- Positive matrix: `ADMIN`, public reader (`RESEARCHER`/`MEMBER`/`SIGNINGOFFICIAL`), dataset + creator, study creator, and study custodian. +- Negative matrix: `CHAIRPERSON` or `MEMBER` with a DAC relationship but no creator/custodian + relationship, and users sharing an institution with the submitter. These remain ordinary public + readers; DAC and institution do not feed the auth context or policy. +- Each applicable combination tested for `canRead`, `isCreator`, and `isCustodian`. - Edge cases: null study, dataset with no DAC, custodian email list empty, user with multiple roles. **Implementation notes**: - Use `@ParameterizedTest` with a method source building `DatasetSearchAuthContext` + `AccessPolicyTerm` pairs with expected `canRead` outcomes. -- Mock `DacDAO` in `DatasetSearchAuthContextResolver` tests to control DAC membership data. +- Assert that `DatasetSearchAuthContextResolver` has no DAC or institution DAO dependency. **Dependencies**: C-1, C-2. **Size**: M @@ -1061,8 +1168,8 @@ context available). #### Ticket D-1 — Extend `ElasticSearchConfiguration` with security-mode settings -**Summary**: Add `securityMode`, privileged service-account credential fields, and -`fieldAccessProfiles` to `ElasticSearchConfiguration`. +**Summary**: Add `securityMode`, privileged service-account credential fields, and the single search +field allowlist to `ElasticSearchConfiguration`. **Context**: `ElasticSearchConfiguration` currently holds a single shared `authUser`/`authPassword` (L22–24). The native path requires either a privileged account for API-key generation or a @@ -1073,16 +1180,18 @@ context available). - `String securityMode` — `"none"`, `"fallback"`, `"shadow"`, or `"native-dls"`. - `String serviceAccountUser` / `String serviceAccountPassword` — may reuse `authUser`/ `authPassword` if the same account has sufficient privilege. - - `Map> fieldAccessProfiles` — maps profile name to list of allowed field - glob patterns (e.g. `"public"` → `["datasetId", "datasetName", "study.studyName", ...]`). + - `List searchVisibleFields` — the one bundle's **literal** allowed paths, transcribed from + [`es-access-contract.md`](es-access-contract.md) §B. Not glob patterns: contract §B.5 forbids + wildcards, because `data`, `study.data`, and `study.assets` are dynamic maps and a + `study.*`-style grant would publish whatever a future registration schema puts in them. - Application starts cleanly with `securityMode: none` (legacy behavior unchanged). - Startup validation: if `securityMode` is `"native-dls"` and `serviceAccountUser` is blank, throw with a descriptive error. **Implementation notes**: - Use Dropwizard `@JsonProperty` / `@NotNull` pattern consistent with existing fields. -- `fieldAccessProfiles` default should include at minimum `"public"` and `"privileged"` entries - reflecting the field lists decided in A-2. +- `searchVisibleFields` defaults to the complete SEARCH-VISIBLE list from contract §B. There is no + public/privileged split and no caller-specific override. - Document new keys in the config YAML schema or `docs/`. **Dependencies**: A-1, A-2. @@ -1108,7 +1217,8 @@ using the existing low-level `RestClient` — no new client instance is needed p DLS query from D-3 and FLS field list; embed the resulting key as `Authorization: ApiKey `. - `run_as` approach (simpler fallback within D): set header `es-security-runas-user: ` on a service-account-authenticated request. -- Unit test: given `isAdmin=true`, generated credential grants unrestricted access. +- Unit test: given `isAdmin=true`, generated credential grants unrestricted **document** access but + uses the same SEARCH-VISIBLE FLS grant as every other caller. - Unit test: given non-admin context, credential includes DLS query and FLS field list. **Implementation notes**: @@ -1127,31 +1237,47 @@ using the existing low-level `RestClient` — no new client instance is needed p **Summary**: Build `DlsQueryBuilder` and `FlsGrantBuilder` that translate `DatasetSearchAuthContext` into an Elasticsearch DLS query string and an FLS field-grant list. -**Context**: The DLS query must express: return documents where `accessPolicy.publicVisibility=true` -OR `accessPolicy.creatorUserId = ` OR `accessPolicy.custodianEmails` contains `` -OR `accessPolicy.dacId` is in ``. ADMIN bypasses all filters. +**Context**: The DLS query expresses the contract's document-visibility rules +([`es-access-contract.md`](es-access-contract.md) §A rows 1–3, 5–8). The FLS grant is **constant** — +one bundle for every caller including admins (contract Decision 2), so `FlsGrantBuilder` takes no +caller input at all beyond validating that it was asked for the one bundle that exists. **Acceptance criteria**: - `DlsQueryBuilder.buildForContext(DatasetSearchAuthContext ctx)` → JSON string. - ADMIN → `{"match_all": {}}`. - - Non-admin → `bool` with `should` clauses for `publicVisibility`, creator, custodian, DAC - membership; `minimum_should_match: 1`. -- `FlsGrantBuilder.buildForContext(DatasetSearchAuthContext ctx, - Map> profiles)` → `List` field patterns. - - ADMIN → `["*"]`. - - Non-admin → field list from the caller's applicable profile. -- Unit tests for each access dimension and combinations. + - Non-admin → `bool` with `minimum_should_match: 1` over exactly these clauses: + `publicVisibility` true; **dataset has no study**; dataset creator; study creator; custodian. + - **No DAC clause, no institution clause, no policy-tag clause.** Contract rows 9–14 are DEFERred: + none of them grants dataset read access today, and adding them here is an authorization + expansion pending OPEN-3/OPEN-5. +- `FlsGrantBuilder.build()` → `List`: the SEARCH-VISIBLE literal paths from contract §B. + - **Same list for admins** — no `["*"]`. An admin wildcard would serve `accessPolicy.*` and the + dynamic property maps, contradicting contract §B.4/§B.5/§B.7. ADMIN is a document-visibility + bypass, not a projection bypass. + - No wildcard or `except` form anywhere in the grant (contract §B.5). +- Unit tests per dimension, plus negative tests: a DAC member who is not creator/custodian does + **not** match a non-public dataset; an admin grant contains no `accessPolicy` path. **Implementation notes**: -- Example DLS query for non-admin: +- DLS query for a non-admin caller: ```json {"bool": {"should": [ {"term": {"accessPolicy.publicVisibility": true}}, - {"term": {"accessPolicy.creatorUserId": 42}}, - {"terms": {"accessPolicy.custodianEmails": ["user@example.com"]}}, - {"terms": {"accessPolicy.dacId": [1, 3]}} + {"term": {"accessPolicy.hasStudy": false}}, + {"term": {"accessPolicy.datasetCreatorUserId": 42}}, + {"term": {"accessPolicy.studyCreatorUserId": 42}}, + {"terms": {"accessPolicy.custodianEmails": ["user@example.com"]}} ], "minimum_should_match": 1}} ``` +- `accessPolicy.hasStudy: false` is what keeps the currently-public "dataset with no study" case + readable (contract §A row 5). Without it that case is silently denied — it cannot be carried by a + null `publicVisibility`, because the filter treats a null on a study-bearing document as *not* + public (contract §A.1). +- Both creator clauses are required and they are different columns; matching only one denies + legitimate access to the other kind of creator. +- Custodian matching is **exact** — `keyword` term match, no lowercase normalizer on the field, and + the caller email passed through unnormalized (contract §A.2). This preserves today's + case-sensitive behavior; changing it is OPEN-6 and must move both paths at once. - If `accessPolicy` is mapped as `nested`, terms must be wrapped in a `nested` query — confirm with B-1 mapping decision. @@ -1198,8 +1324,10 @@ document filtering and field omission. - Test: non-admin search → `publicVisibility=false` document absent from results. - Test: dataset creator search → sees own `publicVisibility=false` document. - Test: ADMIN → sees all documents. -- Test: FLS — non-privileged caller's response does not contain `study.dataCustodianEmail`. -- Test: ADMIN response contains all fields. +- Test: FLS — every caller receives the same SEARCH-VISIBLE fields, including + `study.dataCustodianEmail`, which is published by search today. +- Test: ADMIN sees all documents but receives the same SEARCH-VISIBLE fields; `accessPolicy`, + `data`, `study.data`, and `study.assets` are absent. **Implementation notes**: - Use `testcontainers` with `docker.elastic.co/elasticsearch/elasticsearch:9.x` and @@ -1267,7 +1395,9 @@ alongside a server-built access policy filter derived from `DatasetSearchAuthCon - Non-admin → access policy filter equivalent to DLS query from D-3 (same logic, different execution path — reuse `DlsQueryBuilder.buildForContext` if D-3 has shipped, otherwise implement a standalone `AccessFilterBuilder` and unify later). -- Unit tests for each access dimension: public reader, creator, custodian, DAC member. +- Unit tests for each enforced access dimension: public reader, dataset creator, study creator, and + custodian. Add a negative DAC-member/chair case proving that DAC relationship alone does not add a + clause or grant a restricted document. - Negative test: crafted client query attempting to retrieve `publicVisibility=false` documents is blocked by the injected filter. @@ -1284,28 +1414,38 @@ alongside a server-built access policy filter derived from `DatasetSearchAuthCon #### Ticket E-3 — Server-managed field allowlist applied to search responses -**Summary**: Strip fields from Elasticsearch response documents that the caller is not permitted -to see, based on their `fieldAccessProfile`. +**Summary**: Strip every field outside the single SEARCH-VISIBLE allowlist from Elasticsearch +response documents, for every caller. **Context**: The fallback path cannot rely on native FLS. The server must remove restricted fields from hit `_source` objects before returning the response. **Acceptance criteria**: -- `ResponseFieldFilter.applyProfile(String responseJson, String callerProfile, - Map> profileDefs)` → `String`: - - For each hit in `hits.hits[*]._source`, removes fields not in the caller's profile grant list. - - ADMIN profile → no fields removed. - - `"public"` profile → only fields in the `"public"` grant list retained. -- Unit test: response with `study.dataCustodianEmail` has that field stripped for `"public"` - profile caller. -- Unit test: ADMIN caller receives the full document. +- `ResponseFieldFilter.apply(String responseJson, List searchVisibleFields)` → `String`: + - For each hit in `hits.hits[*]._source`, retains only the SEARCH-VISIBLE paths from contract §B + and drops everything else, including unrecognized paths. + - **ADMIN is filtered identically** — no bypass. Admins see every *document* (DLS `match_all`), + not every *field*; contract §B.7. +- Unit test: `accessPolicy` is absent from the response for **every** caller, admin included + (contract §B.4). +- Unit test: `data`, `study.data`, and `study.assets` are absent for every caller (contract §B.5). +- Unit test: a path not present in §B — simulating a newly added model field — is dropped rather + than passed through. **Implementation notes**: -- Walk `hits.hits[*]._source` as `Map` and apply a recursive filter against the - profile's allowed field glob patterns (e.g. `"study.*"` allows all `study` sub-fields). -- The caller's profile derives from `DatasetSearchAuthContext.isAdmin` → `"admin"`, otherwise - use the document's `accessPolicy.fieldAccessProfile` or a per-caller override from config. -- The `profileDefs` map comes from `ElasticSearchConfiguration.fieldAccessProfiles` (D-1). +- Walk `hits.hits[*]._source` as `Map` and apply a recursive **allowlist** filter: + retain only paths present in the bundle, drop everything else — including paths the filter does not + recognize. A denylist, or a glob like `"study.*"`, fails open on every field added later; contract + §B.5 forbids both, because `study.data` / `study.assets` / `data` are dynamic maps whose keys are + populated wholesale from property bags. +- There is one bundle for all callers (contract Decision 2) — it comes neither from the document + (cancelled B-2) nor from the caller. Admins included; see contract §B.7. +- `accessPolicy.*` must be stripped from every response regardless of bundle (contract §B.4). The + fallback path retrieves whole `_source` objects, so this is the ticket where an enforcement-input + field would otherwise be handed back to the caller. +- Bundle definitions must be generated from, or checked against, contract §B — the same source D-3 + builds its FLS grant from. Two hand-maintained lists will diverge, and the divergence will only be + visible in whichever environment runs the fallback. **Dependencies**: E-2, D-1. **Size**: M @@ -1320,7 +1460,7 @@ from hit `_source` objects before returning the response. **Acceptance criteria**: - When `securityMode == "fallback"`: - Client DSL processed by `SearchQueryMediator.mediate(clientDsl, ctx)` before ES submission. - - Response processed by `ResponseFieldFilter.applyProfile(...)` before returning to caller. + - Response processed by `ResponseFieldFilter.apply(...)` before returning to caller. - When `securityMode == "none"`: existing behavior unchanged. - Both `searchDatasets` (L212) and `searchDatasetsStream` (L230) updated. - `DatasetResource` callers at L425 and L439 updated to pass `duosUser` → resolved @@ -1345,14 +1485,15 @@ mediator and response filter. **Acceptance criteria**: - `SearchQueryMediator` tests: DSL passthrough for ADMIN, correct `bool.must` injection for each non-admin role/dimension. -- `ResponseFieldFilter` tests: field stripping per profile, ADMIN bypass, nested field handling - (`study.dataCustodianEmail`), null/missing source fields are not errors. +- `ResponseFieldFilter` tests: the same allowlist is applied to every caller including ADMIN; + nested SEARCH-VISIBLE fields such as `study.dataCustodianEmail` are retained; INTERNAL fields and + unknown fields are removed; null/missing source fields are not errors. - Negative test: client DSL with injected `_source` override does not expose restricted fields after full mediation pipeline. **Implementation notes**: - Use `JSONAssert` or Jackson-based assertions for comparing query structure. -- Test the full pipeline end-to-end: `mediate(...)` → mock response JSON → `applyProfile(...)` → +- Test the full pipeline end-to-end: `mediate(...)` → mock response JSON → `apply(...)` → assert final field set. **Dependencies**: E-4. @@ -1490,8 +1631,8 @@ access-policy grounds should be cleaned up. **Implementation notes**: - `BucketUtils.ts:L337` — inspect the query for any visibility-related terms. -- `DACDatasets.jsx:L52` — verify it does not filter by DAC membership client-side (the server - handles this via auth context for CHAIRPERSON callers after F-1). +- `DACDatasets.jsx:L52` — document any DAC filtering it performs. The server does **not** grant read + access from DAC membership or chair status; contract rows 9–10 defer that expansion. **Dependencies**: G-1, F-1. **Size**: M @@ -1649,7 +1790,7 @@ it affects users. ``` A-1 → A-2 → A-3 ↓ ↓ - B-1→B-2→B-3→B-4→B-5→B-6 C-1→C-2→C-3 + B-1→B-3→B-4→B-5→B-6 C-1→C-2→C-3 ↓ ↓ D-1→D-2→D-3→D-4→D-5 (or E-1→E-2→E-3→E-4→E-5) ↓ @@ -1669,17 +1810,17 @@ supports DLS/FLS. | Task | Size | Owner | | --- | --- | --- | | 0.1 Confirm target Elasticsearch edition, DLS/FLS availability, API-key/run-as support, and operational model for per-request credentials | S | Infra + Backend | -| 0.2 Define formal access contract: enumerate all dimensions (publicVisibility, ADMIN, creator, custodian, DAC, institution, allowlist, policy tags) and decide which are document-level vs. field-level | S | Backend (policy lead) | +| 0.2 Define formal access contract — **done and unblocked**: [`es-access-contract.md`](es-access-contract.md). Remaining OPEN items are proposed behavior changes, each defaulting to preserve-today, so Phase 1 can start | S → L | Backend (policy lead) | | 0.3 Inventory storage gaps: determine which new dimensions (institution allowlists, explicit user/group lists, policy tags) lack persistent backing and decide whether they go in existing Study/Dataset properties, new DB tables, or external config | M | Backend + DB | ### Phase 1 — Index Schema and Indexing Pipeline -*~Parallel with Phase 0 once contract is defined. Steps 1.1→1.2→1.3 are sequential; 1.4–1.6 parallel after 1.3.* +*~Parallel with Phase 0 once contract is defined. Steps 1.1→1.3 are sequential; 1.4–1.6 run in parallel after 1.3. Cancelled step 1.2 is retained below only as a decision record.* | Task | Size | Owner | | --- | --- | --- | -| 1.1 Add `accessPolicy` nested object to `DatasetTerm` carrying all DLS-needed fields: `publicVisibility`, `creatorUserId`, `creatorEmail`, `custodianEmails`, `datasetCreatorUserId`, `dacId`, `dacApproval`, `allowedInstitutionIds`, `allowedPrincipalIds`, `policyTags` | M | Backend | -| 1.2 Add field-access profile marker to `DatasetTerm` to control FLS (e.g. `fieldAccessProfile: "public" \| "privileged"`) | S | Backend (depends on 1.1) | +| 1.1 Add `accessPolicy` nested object to `DatasetTerm` carrying the DLS-needed fields the contract authorizes: `publicVisibility`, `hasStudy`, `datasetCreatorUserId`, `studyCreatorUserId`, `custodianEmails`, `dacId`. The speculative allowlist/tag fields are held back pending OPEN-5 | M | Backend | +| ~~1.2 Add field-access profile marker to `DatasetTerm` to control FLS~~ — **cancelled**: ES cannot select a field grant per document, and a request-wide caller-specific privileged bundle leaks fields from unrelated public documents. Search uses one allowlist for every caller (contract Decision 2) | — | — | | 1.3 Update `ElasticSearchService.toDatasetTerm` and `toStudyTerm` to populate all new `accessPolicy` fields from Dataset/Study/User data | M | Backend (depends on 1.1 and 0.3) | | 1.4 Update all reindex trigger paths (dataset registration, dataset update, study update, DAC externalization, explicit reindex endpoint) to ensure `accessPolicy` is always current | M | Backend (depends on 1.3) | | 1.5 Design versioned index migration: new index name + Elasticsearch alias cutover + full background reindex strategy; write the reindex script/job | M | Backend + Infra (depends on 1.3) | @@ -1691,9 +1832,9 @@ supports DLS/FLS. | Task | Size | Owner | | --- | --- | --- | -| 2.1 Create `DatasetSearchAuthContext` (or similar) that resolves an authenticated `DuosUser` into: userId, email, institutionId, global roles, DAC memberships by dacId, DAC chair scopes, and any policy-tag grants | M | Backend | +| 2.1 Create `DatasetSearchAuthContext` (or similar) with only the currently enforced inputs: userId, unnormalized email, and global roles. Do not resolve DAC, institution, allowlist, or policy-tag context until a signed-off requirement consumes it | M | Backend | | 2.2 Normalize existing Consent read rules from `DatasetService.verifyPublicVisibilityAccess` / `canReadStudy` / `isCreatorOrCustodian` into a shared policy evaluator usable by both search mediation and native DLS role generation; avoid duplicating the logic | S | Backend (depends on 2.1) | -| 2.3 Unit-test auth context for each role/dimension combination: ADMIN, public reader, creator, custodian, DAC chair, institution-restricted, explicit allowlist | M | Backend (depends on 2.1, 2.2) | +| 2.3 Unit-test auth context and policy evaluation for ADMIN, public reader, dataset creator, study creator, and custodian; add negative cases proving DAC membership/chair status and institution alone grant no access | M | Backend (depends on 2.1, 2.2) | ### Phase 3A — Native Elasticsearch DLS/FLS Path @@ -1701,11 +1842,11 @@ supports DLS/FLS. | Task | Size | Owner | | --- | --- | --- | -| 3A.1 Extend `ElasticSearchConfiguration` with security-mode flag, impersonation/API-key settings, and field-security profile definitions | S | Backend + Infra | +| 3A.1 Extend `ElasticSearchConfiguration` with security-mode flag, impersonation/API-key settings, and the single SEARCH-VISIBLE field allowlist | S | Backend + Infra | | 3A.2 Update `ElasticSearchSupport` to support per-request credential construction: either generate API keys with inline role descriptors or set run-as headers from a privileged service account | L | Backend (depends on 2.1, 3A.1) | | 3A.3 Build role/query descriptor generator that translates `DatasetSearchAuthContext` into Elasticsearch DLS query (wrapping index's `accessPolicy` fields) and FLS field-grant list | L | Backend (depends on 2.2, 3A.2) | | 3A.4 Wire per-request credentials into `ElasticSearchService.searchDatasets` and `searchDatasetsStream` so they use the secured client rather than the shared service credential | M | Backend (depends on 3A.3) | -| 3A.5 Add integration tests against a security-enabled Elasticsearch instance to validate DLS and FLS enforcement: document filtering, field omission, and admin bypass | L | Backend + QA (depends on 3A.4) | +| 3A.5 Add integration tests against a security-enabled Elasticsearch instance to validate DLS and FLS enforcement: document filtering, field omission, and admin document-bypass — asserting that the admin bypass is document-scoped only and that no caller receives `accessPolicy` or the dynamic maps | L | Backend + QA (depends on 3A.4) | ### Phase 3B — Compatibility Fallback @@ -1714,10 +1855,10 @@ supports DLS/FLS. | Task | Size | Owner | | --- | --- | --- | | 3B.1 Build `SearchQueryMediator` that accepts client DSL, strips unsafe response-shaping surfaces (`_source`, `docvalue_fields`, `script_fields`, `explain`, `profile`), and wraps the client query inside a server-built `bool` filter | M | Backend | -| 3B.2 Add mandatory authorization filter injection to `SearchQueryMediator` using `DatasetSearchAuthContext`: emit a `must` bool clause enforcing publicVisibility/creator/custodian/DAC/institution constraints as Elasticsearch terms/bool queries against indexed `accessPolicy` fields | L | Backend (depends on 2.1, 3B.1) | -| 3B.3 Add server-managed field allowlist per caller profile applied to search responses; strip sensitive fields server-side, not in the client | M | Backend (depends on 3B.2) | +| 3B.2 Add mandatory authorization filter injection to `SearchQueryMediator` using `DatasetSearchAuthContext`: emit a `must` bool clause enforcing publicVisibility / no-study / dataset-creator / study-creator / custodian as terms queries against indexed `accessPolicy` fields. **No DAC or institution clause** — contract rows 9–12 are DEFERred and adding them expands authorization | L | Backend (depends on 2.1, 3B.1) | +| 3B.3 Add the server-managed field allowlist (contract §B, one bundle for all callers) to search responses; strip internal fields server-side, not in the client, and apply it to admins too | M | Backend (depends on 3B.2) | | 3B.4 Wire `SearchQueryMediator` into both `searchDatasets` and `searchDatasetsStream` in `ElasticSearchService` | S | Backend (depends on 3B.2, 3B.3) | -| 3B.5 Unit-test `SearchQueryMediator` for each access dimension and confirm sensitive fields are absent from responses, not blanked on the client side | M | Backend (depends on 3B.4) | +| 3B.5 Unit-test `SearchQueryMediator` for each enforced dimension, add negative DAC/institution cases, and confirm INTERNAL fields are absent from responses rather than blanked on the client side | M | Backend (depends on 3B.4) | ### Phase 4 — API Hardening and Long-term Contract @@ -1764,9 +1905,10 @@ support. Phases 3B and 4.2/4.3 can be deferred if the cluster unambiguously supp `accessPolicy` metadata and backend-generated per-request Elasticsearch auth context. - **Required fallback**: server-owned query mediation and field allowlisting if native cluster capabilities or rollout timing block immediate DLS/FLS adoption. -- **Included scope**: `publicVisibility` enforcement on the server side, explicit allow lists, - institution restrictions, DAC-scoped access, policy tags/system-defined criteria, streaming - endpoint behavior, reindex strategy, testing, and duos-ui alignment. +- **Included scope**: `publicVisibility`, creator, and custodian enforcement on the server side; + explicit inventory and deferral of institution, DAC, principal-allowlist, and policy-tag access; + streaming endpoint behavior, reindex strategy, testing, and duos-ui alignment. Deferred dimensions + become enforcement scope only after their OPEN decision is approved. - **Excluded scope**: redesign of non-search Consent endpoints, unrelated UI behavior changes, and implementation of arbitrary policy-authoring UX unless policy storage gaps force a minimal admin/data-model addition. @@ -1779,10 +1921,11 @@ support. Phases 3B and 4.2/4.3 can be deferred if the cluster unambiguously supp 1. **Search API direction**: Option A — harden existing raw-DSL endpoints first for compatibility. Option B — add a server-owned search API in parallel and migrate clients over time. Recommendation: do both, but treat the server-owned API as the long-term destination. -2. **Field-level policy granularity**: Option A — role/profile-based field bundles (e.g. - `public-reader` vs `privileged-reader`). Option B — fully policy-tag-driven per-document field - exposure. Recommendation: start with profile-based bundles to control complexity, then evolve to - tag-driven rules if required. +2. **Field-level policy granularity**: the current contract deliberately has one SEARCH-VISIBLE + bundle for every caller. Role/profile-based or policy-tag-driven field exposure would be + document-scoped for creators and custodians and therefore cannot be implemented safely by native + FLS in a single search. If that requirement appears, reopen Decision 2 and use application-owned + per-document projection rather than adding another FLS profile. 3. **Institution restrictions source of truth**: Option A — user institution alone. Option B — institution plus library-card or other status-derived qualifiers. Recommendation: separate identity context from eligibility state so document policy remains stable even if login checks diff --git a/docs/plans/es-access-contract.md b/docs/plans/es-access-contract.md new file mode 100644 index 000000000..d7683daee --- /dev/null +++ b/docs/plans/es-access-contract.md @@ -0,0 +1,512 @@ +# Elasticsearch Access Contract — Ticket A-2 + +The formal access contract for dataset search: every access dimension, what it is allowed to do, +and which fields each class of caller may see. Companion to +[`elasticsearch-service-duos-ui-usage.md`](elasticsearch-service-duos-ui-usage.md) (the ticket plan) +and [`es-security-capability-record.md`](es-security-capability-record.md) (Ticket A-1, what the +clusters can enforce). + +This is the document B-1 (`accessPolicy` schema), C-1 (auth context resolver), D-3 (DLS/FLS +generator), and E-2/E-3 (fallback filter and allowlist) are implemented against. Where it says +**DECISION**, the question is settled and downstream tickets may rely on it. Where it says **OPEN**, +it needs a named owner's sign-off and is listed in §E — those are policy choices, not engineering +ones, and this document deliberately does not invent them. + +## Why this is not blocked on the rest of A-1 + +A-1 has production measured and dev/staging outstanding. Those rows choose the *enforcement +mechanism* — native DLS/FLS (Epic D) where licensed, mediated queries and response filtering +(Epic E) where not. They do not change the contract. The whole point of writing this separately is +that the answer to "who may see what" must be identical under either mechanism, or the two +enforcement paths will drift apart and the fallback will become a hole. So this contract is stated +in mechanism-neutral terms and each rule is annotated with how it lands in both. + +## DECISION 1 — Restricted documents are invisible, not redacted + +A caller who is not authorized for a dataset does not see a redacted version of it. The document is +absent from their results, and absent from the total hit count. + +The alternative — return every document and blank the restricted fields — was rejected for three +reasons: + +1. **It leaks by counting.** A result total, a facet count, or an aggregation bucket that includes + documents the caller may not read tells them those datasets exist and how many there are. +2. **It cannot be expressed in native FLS.** See Decision 2: field grants do not vary per document, + so "visible but redacted for restricted documents only" is not a thing a single search can do. +3. **It changes current behavior.** Today `DatasetService.verifyPublicVisibilityAccess` drops + unauthorized datasets from the list entirely (`DatasetService.java:147-171`). Invisibility + preserves that; redaction would be a new, more permissive posture adopted by accident. + +Landing in each mechanism: **Epic D** — a DLS `query` in the role descriptor, so the cluster filters +before scoring and counting. **Epic E** — a mandatory `filter` clause injected into the query's +boolean context, which must be non-removable by the caller-supplied DSL (E-2's sanitization is what +makes that true). + +## DECISION 2 — Search serves one field bundle to every caller + +Two designs are ruled out first, because both were in the plan and both are unsound. + +### A per-document FLS marker cannot work + +`fieldAccessProfile` as specified in B-1/B-2 — a per-document marker naming the FLS bundle that +applies to that document — **cannot work, and must be removed from the design.** + +Elasticsearch field-level security is declared in a role descriptor's index privileges: + +```json +{"indices": [{"names": ["dataset"], "privileges": ["read"], + "field_security": {"grant": ["datasetName", "study.studyName"]}}]} +``` + +The grant is bound to the *index privilege*, evaluated when the request is authorized, and applied +uniformly to every document that privilege matches. There is no point in the search lifecycle at +which the cluster reads a field off a hit and re-selects a field grant for it. A per-document marker +therefore has nothing to bind to. +([Elastic: controlling access at document and field level](https://www.elastic.co/docs/deploy-manage/users-roles/cluster-or-deployment-auth/controlling-access-at-document-field-level)) + +### Why a per-*caller* privileged bundle does not rescue it either + +The obvious repair — keep one bundle per request, but pick `privileged` when the caller is a creator +or custodian of *something* — is also wrong, and it fails in a way worth spelling out because it is +easy to talk yourself into. + +Creator and custodian privilege is **document-scoped**. The DLS filter is a disjunction: a custodian +of restricted dataset X receives X *and* every public dataset Y, because `publicVisibility = true` is +one of the `should` clauses. A request-wide `privileged` grant would then project privileged fields +out of all those unrelated public Y documents — datasets the caller has no relationship to +whatsoever. Being privileged on one document would buy privileged fields on every document in the +result set. + +So the constraint is real and has no clever workaround: **native FLS cannot express document-scoped +field access.** Anything that needs it must be built somewhere else: + +| Approach | Verdict | +| --- | --- | +| Separate indices per profile | Does not even apply. The profile here is caller-*relative* ("am I the custodian of this one?"), and an index split can only encode document-absolute properties. | +| Two searches — privileged-scoped and public-scoped — merged server-side | Technically possible; breaks scoring, paging, and aggregations across the merged set. | +| Application-owned projection per document after retrieval | Works, and is genuinely document-scoped. It is what Epic E does, and it is the upgrade path if the requirement ever appears. Doing it in the native path too would forfeit Epic D's reason to exist. | + +**What we do instead: DECISION 2 — search serves one field bundle to every caller.** + +The field grant does not vary at all — not by document, not by caller, not for admins. DLS varies +(who sees which documents); FLS does not. That is exactly the shape native FLS can enforce +correctly, so there is nothing left to get wrong. + +This is only a sound decision if no field genuinely needs document-scoped exposure through search, +so that was checked against the consumers rather than assumed (§B.0). It holds: every field the +catalog publishes through search is published to *all* authenticated callers today — duos-ui's +dataset table renders PI name, custodian emails, and data location unconditionally, and its +client-side filter matches on submitter and DAC emails. There is no existing privileged-in-search +tier to preserve. The fields that are genuinely internal are consumed by nobody through search and +simply leave the projection. + +Consequently the classification in §B has **two** tiers, not three: served by search, or not served +by search. Privileged, document-scoped data continues to reach privileged callers through the +per-dataset endpoints, which already do document-scoped checks in `DatasetService` and are not +affected by this contract. + +**If a future requirement does need a privileged field in search results**, this decision has to be +reopened, not worked around: the answer is application projection (Epic E's mechanism, applied to +the native path as well), and the cost is that Epic D stops being sufficient on its own. + +## DECISION 3 — The contract preserves today's authorization; every expansion is explicit + +The default for every dimension is **PRESERVE**: reproduce what the application does today, exactly, +including the parts that look accidental. Dimensions that would *grant new access* (DAC membership, +institution, policy tags) are **DEFER**red — schema may be reserved for them, but no enforcement +path may consult them until they are signed off in §E. + +This is the rule that keeps a security refactor from becoming a silent authorization change. The +matrix below states it per dimension so no implementer has to infer it. + +--- + +## §A — Access dimension matrix + +"Current effect" is what the application does **today**, from +`DatasetService.verifyPublicVisibilityAccess` / `canReadStudy` / `isCreatorOrCustodian` +(`DatasetService.java:147-237`). "Level" is document (who sees the dataset at all) or field (which +bundle they get). Under Decision 2 the field bundle is constant, so **every row's Level is +Document** — no dimension varies field access. The column is kept to make that explicit rather than +implied. + +| # | Dimension | Source of truth | Current effect on read | Level | Persistent backing | Contract | +| --- | --- | --- | --- | --- | --- | --- | +| 1 | ADMIN role | `user_role` → `UserRoles.ADMIN` | Full bypass: every dataset, no filtering | Document **only** | Yes — `user_role` table | **PRESERVE.** DLS filter is `match_all`. Field grant is the same as everyone else's — admin is not a projection bypass (§B.7). | +| 2 | Study `publicVisibility = TRUE` | `study.public_visibility` | Readable by any authenticated caller | Document | Yes — column | **PRESERVE.** | +| 3 | Study `publicVisibility = FALSE` | `study.public_visibility` | Readable only via #1, #6, #7, or #8 | Document | Yes — column | **PRESERVE.** | +| 4 | Study `publicVisibility = NULL` | `study.public_visibility` | **Unreachable** — the column is `NOT NULL`; the NULL seen in summary rows is the LEFT JOIN for #5 (§A.1) | Document | n/a | **RESOLVED.** Filter treats a null on a study-bearing document as not-public (fails closed). | +| 5 | Dataset has no study (`studyId IS NULL`) | `dataset.study_id` | Readable by everyone — "can't verify visibility, so return the dataset" | Document | Yes — column | **PRESERVE**, and see OPEN-2: this is fail-open. | +| 6 | Dataset creator | `dataset.create_user_id` = `user.user_id` | Readable | Document | Yes — column | **PRESERVE.** Distinct from #7. | +| 7 | Study creator | `study.create_user_id` = `user.user_id` | Readable | Document | Yes — column | **PRESERVE.** Distinct from #6. | +| 8 | Study data custodian | `study_property['dataCustodianEmail']` (JSON array) contains `user.email` | Readable | Document | **Partial — see §C.** Unstructured JSON in a property bag. | **PRESERVE**, with the matching rules in §A.2 made explicit. | +| 9 | DAC membership | `dac_user` via `DacDAO` | **No dataset read access today** | — | Yes — table | **DEFER.** Reserve `accessPolicy.dacId`; no enforcement path reads it. OPEN-3. | +| 10 | DAC chair | `dac_user` role | **No dataset read access today** | — | Yes — table | **DEFER.** OPEN-3. | +| 11 | `dacApproval` | `dataset.dac_approval` | **No read effect today** — a display/filter attribute the UI applies client-side (G-1) | — | Yes — column | **DEFER.** Indexing it as an `accessPolicy` field invites it to be read as authorization; it is not. OPEN-4. | +| 12 | Institution | `user.institution_id`; no dataset-side counterpart | **No dataset read access today** | — | User side only; **no dataset-to-institution mapping exists** | **DEFER.** Needs A-3 storage decision before it can mean anything. OPEN-5. | +| 13 | Policy tags | — | **Does not exist** | — | **None** | **DEFER.** OPEN-5. | +| 14 | Explicit principal allowlist | — | **Does not exist** | — | **None** | **DEFER.** OPEN-5. | + +Rows 9–14 are the ones the review flagged: B-1's `AccessPolicyTerm` lists them as fields alongside +the dimensions that do carry authorization today, with nothing recording that four of them grant +nothing. Reserving schema for them is fine. Wiring them into a DLS filter without sign-off would +grant DAC members, institution peers, and tag holders read access to non-public datasets that they +do not have today. + +### §A.1 — `publicVisibility = NULL` — RESOLVED by the schema, not by policy + +This looked like an unresolvable policy question and was briefly recorded as one. It is not: the +database settles it, and the apparent disagreement between the two code paths is unreachable. + +**`study.public_visibility` is `NOT NULL`** (`changelog-consent-2023-04-20-create-study.xml:22-24`, +never relaxed by a later changeset). A persisted study cannot have a null visibility, so +`canReadStudy` — which only ever runs against a loaded `Study` — cannot observe one. + +The NULL that *does* occur is in the summary list, and it comes from a join, not from a study: + +```sql +FROM dataset d LEFT JOIN study s ON s.study_id = d.study_id -- DatasetDAO.java:119-129 +``` + +`summary.public_visibility()` is NULL exactly when the dataset has **no study** — which is §A row 5, +a case both paths already agree on. Trace it: `Boolean.TRUE.equals(null)` is false, the creator +checks fail for a stranger, `summary.study_id() != null` is false, and control reaches the final +`else`, which adds the dataset. Readable. The single-dataset path returns the dataset for the same +reason (`studyId == null` → return). **The two paths agree.** + +**DECISION**: the contract has one filter, and it is the two-clause form already implied by rows 2–5: +`publicVisibility` is true, *or* the dataset has no study, *or* the caller is privileged on it. A +null or missing `publicVisibility` on a document that *does* have a study is unreachable, and the +filter must therefore treat it as **not public** — the unreachable branch fails closed rather than +open. B-3's "no study" marker (§D) is what keeps row 5 working under that rule, rather than leaning +on a null to mean "visible." + +This unblocks B-3, D-3, E-2, and G-1, which were waiting on a decision that did not need to be made. + +
+The divergence that prompted the question, kept for the record + +The two paths do read a null differently, and if the `NOT NULL` constraint were ever dropped they +would diverge in production. C-2 should normalize them while it is consolidating the predicates.
+ +**Single-dataset path** — `canReadStudy` (`DatasetService.java:193-205`): + +```java +if (!Boolean.FALSE.equals(study.getPublicVisibility())) { + return true; // NULL is readable by anyone +} +``` + +**Summary-list path** — `verifyPublicVisibilityAccess(List, User)` +(`DatasetService.java:147-171`): + +```java +if (Boolean.TRUE.equals(summary.public_visibility())) { // NULL is NOT public here + ... +} else if (...creator checks...) { +} else if (summary.study_id() != null) { + // falls through to a creator/custodian check, which a stranger fails +} +``` + +Were a null ever to reach these, a caller who is neither creator nor custodian **could** read the +study through `findMinimalDatasetByIdentifier` and **could not** see it in a dataset summary list. +The `NOT NULL` constraint is the only thing preventing that, which is a thin guarantee to leave +implicit — hence the normalization ask above. + +### §A.2 — Identity matching rules + +These are the details that decide whether a re-implementation actually reproduces current behavior. +All four are **PRESERVE**, meaning the DLS filter and the fallback must match today's semantics +exactly — but three of them are also latent defects, flagged as OPEN-6. + +| Rule | Current behavior | Note | +| --- | --- | --- | +| Creator matching (#6, #7) | Integer user ID equality | Sound. | +| Custodian matching (#8) | String equality on email: `user.getEmail().equals(custodian.trim())` | **Case-sensitive**, and only the *stored* value is trimmed — the user's email is not. `Alice@x.org` does not match a stored `alice@x.org`. | +| Custodian property parse | `Gson.fromJson(prop.getValue().toString(), List)` | Throws on malformed JSON; no defined behavior for a non-array value. | +| Missing custodian property | `Optional.empty()` → not a custodian | Correct and safe. | + +**DECISION: the index preserves today's exact matching.** B-3 stores custodian emails with only the +`trim()` the current code applies to the stored side, and C-1 compares the caller's email with +`equals`, case-sensitively. A case-mismatched email that does not authorize today must not authorize +through search either. + +This is deliberate, and it means shipping a known defect on purpose. Normalizing here would newly +authorize case-mismatched custodians — a real authorization expansion, arrived at as a side effect of +an indexing decision, and applying to the search path only while the non-search endpoints kept +rejecting the same user. That asymmetry is worse than the defect. **OPEN-6** proposes fixing it +properly: normalize in `DatasetService` and in the index together, under C-2's shared predicates, as +one reviewed change with parity across both paths. + +Note also that ES `keyword` fields match exactly by default, so exact matching is what the index does +without any special handling; normalization would be the thing requiring extra work, not the reverse. + +--- + +## §B — Field classification + +**Two tiers**, per Decision 2. Every indexed path gets exactly one; there is no unclassified residue, +because a field that nobody classified is a field that leaks by default. + +- **SEARCH-VISIBLE** — in the one bundle search serves. Every authenticated caller receives it, + including admins, and no caller receives more. (All search callers are authenticated: + `DatasetResource.java:442-462` is `@PermitAll` with `@Auth DuosUser`, so this is "any logged-in + user," not "anonymous.") +- **INTERNAL** — never served through search, to anyone, admins included. Present in the index only + because enforcement needs it, or because it is indexed and nothing consumes it. + +There is no third, privileged tier: Decision 2 establishes that native FLS cannot scope fields per +document, and §B.0 establishes that nothing needs it. + +### §B.0 — How these were classified, and what it means to move one + +The tier is **not** a judgment about how sensitive a field looks. It is a record of what the product +publishes today, checked against the consumers: + +- `duos-ui/src/components/data_search/DatasetSearchTableConstants.tsx` — the catalog table renders + `study.piName`, `study.dataCustodianEmail`, `dataset.dataLocation`, and `dataset.url` in + unconditional columns, for every caller. +- `duos-ui/src/libs/utils.ts:498-522` — the client-side search filter additionally matches on + `study.dataSubmitterEmail`, `dac.dacEmail`, and `createUserDisplayName`. + +So PI names, custodian emails, submitter emails, and DAC emails are **already public to every +authenticated user** in the DUOS catalog. Classifying them as privileged would not have protected +them; it would have broken the product — `dataCustodianEmail.join(', ')` in the table is not +null-guarded and throws outright if the field is stripped. This is the check that turned a plausible +three-tier design into a wrong one, and it is why the two tiers below are grounded in consumers +rather than in intuition about PII. + +Two directions of movement, with different costs: + +- **INTERNAL → SEARCH-VISIBLE** is an exposure change. It needs review here first. +- **SEARCH-VISIBLE → INTERNAL** is a tightening, and therefore a *product* change plus a duos-ui + change: something on screen today would disappear, and an unguarded consumer may throw. Do not do + it as part of the enforcement work. **OPEN-7** is where that conversation belongs. + +Every path below is a **literal Elasticsearch field path**, because that is what an FLS `grant` and +the E-3 allowlist consume. Object fields are enumerated to their leaves — `dataUse.primary` is an +array of objects, so `dataUse.primary.code` and `dataUse.primary.description` are the grantable +paths and `dataUse.primary[]` is not a usable path at all. + +### §B.1 — `DatasetTerm` (root) + +| Path | Tier | Note | +| --- | --- | --- | +| `datasetId` | SEARCH-VISIBLE | | +| `datasetIdentifier` | SEARCH-VISIBLE | | +| `datasetName` | SEARCH-VISIBLE | | +| `participantCount` | SEARCH-VISIBLE | Rendered and summed in the table. | +| `dataUse.primary.code` | SEARCH-VISIBLE | Filtered on client-side (`utils.ts:500`). | +| `dataUse.primary.description` | SEARCH-VISIBLE | | +| `dataUse.secondary.code` | SEARCH-VISIBLE | | +| `dataUse.secondary.description` | SEARCH-VISIBLE | | +| `accessManagement` | SEARCH-VISIBLE | Drives selectability and the access-type column. | +| `dacId` | SEARCH-VISIBLE | | +| `dacApproval` | SEARCH-VISIBLE | Display attribute, not authorization (§A row 11). | +| `hasInstitutionCertification` | SEARCH-VISIBLE | | +| `dataLocation` | SEARCH-VISIBLE | Rendered in the data-location column. | +| `url` | SEARCH-VISIBLE | Rendered as the data-location link; client-side filter matches it. | +| `createUserDisplayName` (deprecated) | SEARCH-VISIBLE | Client-side filter matches it. Deprecated in favour of `submitter.displayName`; if it is removed from the index, remove the consumer in the same change. | +| `requestLocation` | **INTERNAL** | Indexed, no consumer found in duos-ui. | +| `deletable` | **INTERNAL** | Lifecycle state; discloses whether a dataset is in use. No consumer. | +| `createUserId` | **INTERNAL** | Internal user ID. No consumer. | +| `data.*` | **INTERNAL** | Dynamic map — §B.5. | + +### §B.2 — `study` (`StudyTerm`) + +| Path | Tier | Note | +| --- | --- | --- | +| `study.studyId` | SEARCH-VISIBLE | Used as a row key. | +| `study.studyName` | SEARCH-VISIBLE | | +| `study.description` | SEARCH-VISIBLE | Client-side filter matches it. | +| `study.phsId` | SEARCH-VISIBLE | dbGaP accession. | +| `study.phenotype` | SEARCH-VISIBLE | | +| `study.species` | SEARCH-VISIBLE | Client-side filter matches it. | +| `study.dataTypes` | SEARCH-VISIBLE | Array of keywords; the path is grantable as-is. | +| `study.piName` | SEARCH-VISIBLE | **Rendered in an unconditional "PI" column.** Published today. | +| `study.dataCustodianEmail` | SEARCH-VISIBLE | **Rendered in an unconditional "Data Custodian" column**, and the consumer is not null-guarded. Also an authorization input — that does not make it secret, but it does mean B-4 must reindex on change. | +| `study.dataSubmitterEmail` | SEARCH-VISIBLE | Client-side filter matches it (`utils.ts:505`). | +| `study.externalIdentifier` | SEARCH-VISIBLE | | +| `study.externalIdentifierType` | SEARCH-VISIBLE | | +| `study.dataSubmitterId` | **INTERNAL** | Internal user ID. No consumer. | +| `study.throughBioId` | **INTERNAL** | Internal cross-system identifier. No consumer. | +| `study.publicVisibility` | **INTERNAL** | Under Decision 1 a caller only receives documents they may read, so it carries no information they need. G-1 removes the client-side filtering that reads it — sequence accordingly. | +| `study.assets.*` | **INTERNAL** | Dynamic map — §B.5. | +| `study.data.*` | **INTERNAL** | Dynamic map — §B.5. | + +### §B.3 — `submitter` / `updateUser` (`UserTerm`) and `dac` (`DacTerm`) + +| Path | Tier | Note | +| --- | --- | --- | +| `dac.dacId` | SEARCH-VISIBLE | | +| `dac.dacName` | SEARCH-VISIBLE | Client-side filter matches it. | +| `dac.dacEmail` | SEARCH-VISIBLE | Client-side filter matches it. Usually a shared mailbox. | +| `submitter.userId` | **INTERNAL** | Internal user ID. | +| `submitter.displayName` | **INTERNAL** | No consumer — the table uses the deprecated `createUserDisplayName` instead. Promote if a consumer appears; do not grant it speculatively. | +| `submitter.institution.id` | **INTERNAL** | | +| `submitter.institution.name` | **INTERNAL** | | +| `updateUser.userId` | **INTERNAL** | | +| `updateUser.displayName` | **INTERNAL** | | +| `updateUser.institution.id` | **INTERNAL** | | +| `updateUser.institution.name` | **INTERNAL** | | + +`updateUser` has no consumer at all and discloses who last edited a dataset. B-1 should consider +dropping it from the index rather than indexing it and then filtering it out of every response. + +### §B.4 — `accessPolicy` (added by B-1) + +**Every path under `accessPolicy.*` is INTERNAL, without exception, for every caller including +admins.** It is enforcement input: creator IDs, custodian emails, and whatever allowlists §E +eventually authorizes. Granting any of it would hand back identity data, and an allowlist field would +additionally disclose *who else* has access. + +This is stated as a rule rather than a list because B-1's field set is expected to grow. In the +fallback path (E-3) it matters most: that path retrieves whole `_source` objects, so `accessPolicy` +is returned by Elasticsearch and must be stripped by the server. + +### §B.5 — Dynamic maps are INTERNAL, and grants may not contain wildcards + +`DatasetTerm.data`, `StudyTerm.data`, and `StudyTerm.assets` are `Map` populated +wholesale from property-bag values (`ElasticSearchService.java:294-297, 514-517`). Their key sets are +not fixed, not validated, and not reviewable at design time — a new registration schema field lands +in them with no code change here. + +**Rule: dynamic maps are INTERNAL, and no field grant may contain a wildcard.** A grant of `data.*` +publishes whatever a future schema puts there, which is the precise failure this classification +exists to prevent. Individual keys may be promoted only by being enumerated explicitly — +`data.someKnownKey` — after review of what populates them. + +Two consequences for implementation: + +- **The grant list is an allowlist of literal paths.** ES FLS supports `grant` with `except` + patterns; do not use `{"grant": ["*"], "except": [...]}`, which fails open on every field added + later. Enumerate. This applies to the admin grant too — see §B.7. +- **Epic E must apply the same rule** in `ResponseFieldFilter` (E-3): filter by allowlist, dropping + unrecognized paths, rather than by denylist. + +### §B.6 — Keep this list in step with the model classes + +§B is enumerated from `DatasetTerm.java`, `StudyTerm.java`, `UserTerm.java`, `DacTerm.java`, +`InstitutionTerm.java`, `DataUseSummary.java`, and `DataUseTerm.java`. The "Indexed Elements" section +of the plan document was incomplete (it omitted `study.externalIdentifier`, +`study.externalIdentifierType`, and the `UserTerm.institution` sub-fields); it has been corrected, but +§B supersedes it either way. + +**Whoever adds a field to any of those classes must classify it here in the same change.** An +unclassified field is dropped by the E-3 allowlist and omitted from the D-3 grant — it will appear to +"not work" rather than to leak, which is the safe failure but an confusing one if this rule is not +known. A test asserting that every serialized path appears in §B's list would enforce it mechanically +and is worth adding in B-6. + +### §B.7 — Admins get the same bundle, not a wildcard + +Admins bypass **document** filtering (§A row 1: DLS is `match_all`). They do not bypass field +filtering. The D-3 FLS grant and the E-3 response filter must use the same SEARCH-VISIBLE list for +admins as for everyone else. + +A wildcard admin grant would serve `accessPolicy.*` and both dynamic maps to admin callers, directly +contradicting §B.4 and §B.5 — and it would do so through the same UI, which has no use for them. +Admin tooling that genuinely needs internal fields should use the admin endpoints, which are outside +this contract. "Admin" is a document-visibility bypass, not a projection bypass. + +--- + +## §C — Storage gaps, and a correction to A-3 + +A-3 currently states that `dataCustodianEmail` needs "no new storage." That is true in the narrow +sense and misleading in the way that matters, and A-2 flagged it as a storage-gap candidate for a +reason. Both are recorded here as the contract's position: + +**Persistent backing exists, but not in a queryable, constrained form.** The value lives in +`study_property` under the key `dataCustodianEmail`, typed `PropertyType.Json`, holding a JSON array +of strings (`DatasetService.java:213-237`; written at `DatasetService.java:573-582`). That means: + +- **No referential integrity.** A custodian email need not correspond to any DUOS user. Access is + granted to a string, not to a principal. +- **No constraint or normalization.** Case, whitespace, and duplicates are whatever was written; + §A.2 records the asymmetric trim and case-sensitive comparison that result. +- **No index.** Answering "which studies is this user custodian of" means parsing every property + bag, which is why the current code can only ask it one study at a time. +- **No defined behavior for malformed content.** A non-array or unparseable value throws from Gson + inside an authorization check. + +For Epics B–E this is workable, because B-3 denormalizes custodian emails into `accessPolicy` at +index time and the search path reads the index rather than the property bag. **The gap does not +block this work.** It stays a gap for the non-search endpoints, which keep parsing the bag, and it +makes reindex-on-custodian-change a correctness requirement rather than a nicety (B-4). + +Decisions that remain open on it are OPEN-6 (normalization) and, separately from this contract, +whether custodianship should become a first-class relation. The latter is A-3's call, not A-2's; +what A-2 requires is that A-3 stop describing it as a solved case. + +### Other dimensions' storage, restated + +| Dimension | Storage today | Needed for contract | +| --- | --- | --- | +| `publicVisibility` | `study.public_visibility` column | Nothing new. | +| Dataset creator | `dataset.create_user_id` column | Nothing new. | +| Study creator | `study.create_user_id` column | Nothing new. | +| `dacId` | `dataset.dac_id` column | Nothing new. | +| DAC membership / chair | `dac_user` table | Nothing new — but DEFERred (row 9/10), so nothing consumes it yet. | +| Institution allowlist | **User side only.** No dataset-to-institution mapping exists. | Blocked: A-3 must decide mapping table vs. derivation before OPEN-5 can be answered. | +| Policy tags | **None.** | Blocked on A-3 and OPEN-5. | +| Explicit principal allowlist | **None.** | Blocked on A-3 and OPEN-5. | + +--- + +## §D — What downstream tickets must change + +| Ticket | Required change | +| --- | --- | +| **B-1** | Drop `fieldAccessProfile` (Decision 2). Omit `allowedInstitutionIds`, `allowedPrincipalIds`, `policyTags` until OPEN-5 says they are requirements. Comment on the class that every field is INTERNAL (§B.4). Custodian emails are trimmed on the stored side but preserve case (§A.2); do not lowercase or otherwise normalize them. | +| **B-2** | **Cancelled.** It specifies a per-document FLS marker, which Elasticsearch cannot honour, and the per-caller repair leaks across documents. Deriving a bundle is no longer needed at all: there is one bundle (Decision 2). | +| **B-3** | Populate rows 1–3 and 5–8. Write an explicit **no-study marker** (`accessPolicy.hasStudy`, §A.1) so row 5 does not depend on a null meaning "visible." Do not normalize custodian emails. | +| **B-4** | Custodian changes must trigger reindex — see §C; the index is now the authorization source for search. | +| **C-1** | Carry only caller-side inputs: the caller's user ID, email **unnormalized** for exact matching, and global roles. Dataset/study creator IDs remain distinct document-side `accessPolicy` fields (rows 6/7); the filter compares the one caller ID with both. Do **not** resolve DAC, institution, allowlist, or policy-tag context while rows 9–14 are DEFERred. **No field-bundle field**: the bundle is constant (Decision 2). | +| **C-2** | Normalize the two `verifyPublicVisibilityAccess` overloads so their null handling cannot diverge if the `NOT NULL` constraint is ever relaxed (§A.1). | +| **D-3** | DLS filter from rows 1–3, 5–8 — plus the no-study clause. **No DAC clause** (row 9 is DEFERred). FLS grant = the single SEARCH-VISIBLE list from §B, literal paths, no wildcards, **including for admins** (§B.7). | +| **E-2 / E-3** | Same filter, same allowlist, same source document. E-3 must strip `accessPolicy.*` and both dynamic maps from every response including admin responses, and must filter by allowlist rather than denylist. | +| **G-1** | Client-side `publicVisibility`/`dacApproval` filtering is removed once the server filter implements rows 2–5 identically. `study.publicVisibility` is INTERNAL (§B.2), so the client stops receiving it — sequence the two changes together. | +| **G-2 / G-4** | Verify no duos-ui consumer reads an INTERNAL field. §B.0 records the audit done here; G-2 is where it is confirmed against the whole app rather than the search components. | + +--- + +## §E — Open decisions requiring sign-off + +These are policy, not engineering. **None of them blocks implementation** — every one has a stated +default that preserves current behavior, so Epics B and C can proceed while they are answered. What +they block is *changing* behavior in the direction each describes. + +| ID | Decision | Blocks | Default if unanswered | +| --- | --- | --- | --- | +| **OPEN-2** | Should a dataset with no study remain readable by everyone (row 5)? It is fail-open today. | Nothing — PRESERVE is implementable now | PRESERVE (readable), as the contract states — but worth a conscious confirmation rather than inheritance. | +| **OPEN-3** | Should DAC members and chairs gain read access to non-public datasets in their DAC? They have none today. | Rows 9–10 | DEFER. | +| **OPEN-4** | Is `dacApproval` ever authorization, or purely display? | Row 11, G-1 | Display only. | +| **OPEN-5** | Do institution allowlists, policy tags, and explicit principal allowlists exist as requirements at all? All three are speculative, none has storage, and B-1 reserved fields for them. | Rows 12–14, A-3 | DEFER — and if the answer is "not now," drop them from `AccessPolicyTerm` rather than shipping unpopulated fields that read as enforcement. | +| **OPEN-6** | Fix the custodian email case-sensitivity defect (§A.2)? The contract preserves today's exact matching, so this is a proposal, not a blocker. If approved it must land in `DatasetService` and the index **together**, via C-2 — normalizing only the search path would authorize a user through search whom the dataset endpoints still reject. | Nothing | Keep exact matching; fix as a separate reviewed change. | +| **OPEN-7** | Should the catalog **stop** publishing `study.piName`, `study.dataCustodianEmail`, `study.dataSubmitterEmail`, and `dac.dacEmail` to all authenticated callers? They are on screen today (§B.0), so this contract keeps them SEARCH-VISIBLE. Restricting them is a product decision, and needs a duos-ui change in the same release — the table's `dataCustodianEmail.join(', ')` throws if the field is absent. | Nothing — this is a proposed tightening, not a gap | Keep publishing. Changing it is product scope, not enforcement scope. | + +--- + +## Status + +**Complete and unblocked.** + +Every dimension in §A and every path in §B is decided, and no OPEN item stands between this contract +and B-1/B-3/C-1. That is a change from the first draft, which recorded `publicVisibility = NULL` as +an unanswerable policy question: the schema answers it (§A.1), and checking rather than escalating +turned the last blocker into a fact. + +What the OPEN items in §E now represent is proposed *changes* — expanding access (OPEN-3, OPEN-5), +tightening it (OPEN-7), or fixing a defect (OPEN-6) — each with a default that preserves what the +application does today. They can be answered on their own schedule. + +Two things this contract does **not** cover, deliberately: + +- **Document-scoped field access.** Decision 2 establishes that native FLS cannot do it and that + nothing currently needs it. If that changes, this document is where the change starts, and the + answer is application projection rather than a cleverer FLS grant. +- **The non-search endpoints.** `DatasetService` keeps its own document-scoped checks and its own + richer projections. This contract governs the search index only; C-2 is what keeps the two from + drifting apart. diff --git a/docs/plans/es-security-capability-record.md b/docs/plans/es-security-capability-record.md index 7252409e0..b07b28db9 100644 --- a/docs/plans/es-security-capability-record.md +++ b/docs/plans/es-security-capability-record.md @@ -137,13 +137,22 @@ reversible. Treat a production activation as an infra change, not a diagnostic. ## Environment inventory -### Local (`config/docker-compose.yaml`) — measured 2026-07-29 with the write probes - -Ticket A-0 is closed: the compose file now sets `xpack.security.enabled` to **true** by default -(overridable per-run with `ES_SECURITY_ENABLED=false`) and self-generates a **trial** license, so the -security features are exercisable locally as shipped. The endpoint has now been run against the -running local cluster in write-probe mode, so every row below is an observation rather than an -inference — this is the first environment where all five capabilities came back `SUPPORTED`: +### Local (rendered `config/docker-compose.yaml`) — measured 2026-07-29 with the write probes + +Ticket A-0 is closed. Be precise about what that does and does not mean for anyone else's machine: +`/config/` is git-ignored (`.gitignore` L149) and rendered per developer by the Broad-internal +`firecloud-develop`, so **nothing in this repository sets any Elasticsearch default** — there is no +committed compose file for a change to land in. The cluster measured below is a local rendered copy, +edited to set `xpack.security.enabled` to **true** (overridable per-run with +`ES_SECURITY_ENABLED=false`) and to self-generate a **trial** license. The durable form of that +change is the `firecloud-develop` compose template, which is outside this repo and still needs an +owner — see the A-0 outcome in +[`elasticsearch-service-duos-ui-usage.md`](elasticsearch-service-duos-ui-usage.md). Until it lands, +each developer applies these settings themselves; see the notice below. + +The endpoint has now been run against that local cluster in write-probe mode, so every row below is +an observation rather than an inference — this is the first environment where all five capabilities +came back `SUPPORTED`: | Capability | Verdict | Evidence | | --- | --- | --- | @@ -178,12 +187,12 @@ work, not as a preview of the deployed rows. #### Notice: developers must update their own local configuration -A local cluster does **not** pick these settings up on its own. `config/docker-compose.yaml` is -committed, but most people carry local edits to it (the bucket location, ports, memory limits) or run -a copy of their own, so a pull of this branch will not necessarily put these settings into the file -you actually start ES with. Each developer has to enable them in their own compose file before the -endpoint will report anything like the table above — and a local cluster that lags behind produces -`UNAVAILABLE` / `LICENSE_BLOCKED` verdicts that read like findings when they are only local drift. +A local cluster does **not** pick these settings up on its own, and pulling this branch will not put +them anywhere: `config/docker-compose.yaml` is git-ignored and rendered per developer, so the file you +actually start ES with is not in this repository at all. Each developer has to enable these settings +in their own rendered copy before the endpoint will report anything like the table above — and a local +cluster that lags behind produces `UNAVAILABLE` / `LICENSE_BLOCKED` verdicts that read like findings +when they are only local drift. What has to be true in your `config/docker-compose.yaml` (and any personal copy or override file you run instead of it): @@ -290,9 +299,11 @@ falls back to the license reading and says which of the two you are looking at. ### `dev` — not yet measured > Call `POST /api/elasticSearch/capabilities` against dev with an Admin token and -> paste the findings here. Dev is the right place to run the write probes first in a *deployed* -> environment — teardown has been confirmed on the control clusters and locally, so what dev adds is -> confirmation under a real shared credential rather than a superuser one. +> summarise the verdicts here — verdicts only, with the raw report attached to the ticket (see the +> note under `production` below). Production has since been measured and came back clean, so the +> write probes are no longer unproven in a *deployed* environment; what dev and staging now add is +> the other two thirds of the decision rule, and — if either runs a narrower credential than +> production's — the first reading of what a real least-privilege shared credential can do. | Capability | Verdict | Evidence | | --- | --- | --- | @@ -318,29 +329,93 @@ falls back to the license reading and says which of the two you are looking at. | API keys | | | | `run_as` | | | -### `production` — not yet measured +### `production` — measured 2026-08-05 with the write probes -Call the endpoint read-only first — in that mode it creates nothing, so it cannot leave anything -behind on the production cluster. Only `POST` it after the same call has been run in dev -and staging and the teardown has been confirmed there; the probes are designed to be safe in -production (namespaced, 10-minute expiry, torn down in a `finally`, failures reported in `notes`), -but production is not the place to find out. +Measured out of the order this document recommends: the write probes were run against production +before dev or staging. The run came back clean — five `SUPPORTED` verdicts, `write_probes_run: true`, +and no teardown failure in `notes` — so nothing was harmed by taking it first, but the sequencing +advice above stands for the environments still to be measured. -If production's shared credential turns out to lack `manage_security`, the write probes there will be -inconclusive by construction. That is a finding to record rather than a problem to work around: it -means Epic D's per-request key minting needs a privilege grant before it can work in production at -all. +Production is an **Elastic Cloud** deployment on an **enterprise** license, and every capability was +observed rather than inferred: | Capability | Verdict | Evidence | | --- | --- | --- | -| Elasticsearch version | | | -| Distribution | | | -| Edition / license | | | -| X-Pack Security enabled | | | -| DLS | | | -| FLS | | | -| API keys | | | -| `run_as` | | | +| Elasticsearch version | 9.x — one minor behind the pinned client, same major | `GET /` → `version.number` | +| Distribution | elasticsearch | `GET /` → `version.distribution` | +| Edition / license | Elastic Cloud, `enterprise`, `status: active` | `elastic_cloud: true`; `GET /_license` | +| X-Pack Security enabled | **`SUPPORTED`** | `GET /_xpack` 200; `GET /_security/_authenticate` 200 | +| DLS | **`SUPPORTED` — enforced, not merely accepted** | a `match_none` DLS key returned **none** of the documents the shared credential can see from `GET /dataset/_search` | +| FLS | **`SUPPORTED` — enforced** | a key granting only `datasetIdentifier` returned documents carrying only that field | +| API keys | **`SUPPORTED`** | key created, authenticated, invalidated | +| `run_as` | **`SUPPORTED`** (self-impersonation only — see below) | the `es-security-runas-user` header was honoured and the request resolved to the named principal | +| Credential privileges | the deployment's credential holds the key-minting privilege Epic D needs; the full block is on the ticket, not here | `POST /_security/user/_has_privileges` | +| `dataset` index | non-empty | so the `match_none` DLS result means enforcement rather than an empty index | +| `elasticsearch-rest-client` (POM) | 9.4.4 against a cluster one minor older — same major | `rest_client_compatibility` could not read the client version in the deployed jar at the time of this run; since fixed (see below) | +| Recommendation | Epic D viable here, **observed** | probe role and keys carrying DLS/FLS descriptors accepted *and* enforced | + +Teardown behaved as documented: three short-lived keys and one probe role were created under the +`duos-capability-probe` / `duos_dlsfls_probe` names and removed again, with no teardown failure +reported in `notes`. + +> **Environment specifics are deliberately not in this file, because this repository is public.** +> The full report — the principal the deployment authenticates as, its roles, the complete +> `cluster_privileges` block, and the `security_settings` dump including the cluster's audit setting +> — is attached to **DT-3826**, where access is already scoped. What is kept here is what the Epic D +> / Epic E decision rule actually consumes: whether security is on, whether DLS and FLS are licensed +> *and enforced*, and whether the deployment can mint keys. When filling in the rows for dev and +> staging below, summarise the verdicts the same way and attach the raw report to the ticket rather +> than pasting it here. + +Four things in this run are worth carrying forward as their own findings. + +**Epic D's per-request key minting is not privilege-blocked in production.** The note further down +predicted the opposite — that the shared `authUser` almost certainly does not hold a key-minting +grant — and production contradicts it in the permissive direction, so there is no privilege to request +from infra before Epic D can proceed there. Two caveats travel with that. The run says nothing about +what a *least-privilege* credential could do, exactly as the local superuser run said nothing about +the deployed ones; and the breadth of what that credential does hold is worth reviewing on its own +terms, independent of this work — raised on DT-3826. Epic D itself needs only `manage_own_api_key`, +which is the grant to ask for if that credential is ever narrowed. + +**The `run_as` evidence is self-impersonation.** The probe resolved the credential's own principal to +itself, which shows the cluster accepts and honours the header but not that it will resolve a +*different* principal. If Epic D's design leans on `run_as` rather than on per-request keys, re-run +with `?runAsUser=` to settle it; the DLS/FLS verdicts do not depend on this. + +**Do not assume cluster-side audit logging.** Under Epic D the per-request API key *is* the +access-control decision, so on a cluster with auditing off nothing on the cluster side records which +key read what. Whatever audit trail the access contract needs must therefore come from the Consent +side unless auditing is known to be enabled on that deployment — read +`xpack.security.audit.enabled` out of the report per environment rather than assuming either way, and +treat enabling it as an infra ask. + +**Elastic Cloud reserves some cluster settings to Elastic's operators.** That gates none of DLS, FLS, +API keys, or `run_as` — all four were observed working — but it does mean any future infra ask +(enabling audit, for instance) is a support request rather than a settings change. + +#### Why `rest_client_compatibility` came back indeterminate — since fixed + +The field reported that it "could not determine the client or cluster version at runtime," which reads +like a gap but was not a finding about the cluster. The cluster version was known (it is in the +report's own `version` field); the *client* version was not, because +`ElasticSearchCapabilityService` read it from +`RestClient.class.getPackage().getImplementationVersion()`, and the shade plugin strips dependency +manifests (`META-INF/MANIFEST*`, `**/pom.properties`) when assembling the deployable uber jar. Locally, +where the client keeps its own jar, the same code reported the 9.4.4 match — so the field would have +stayed indeterminate in *every* deployed environment while working fine everywhere it did not matter. + +Fixed: the version now falls back to `elasticsearch.rest.client.version` in `mvn.properties`, the +build-time property file `properties-maven-plugin` already generates from the pom (the same mechanism +`SwaggerResource` uses), with the dependency version promoted to a pom property so it lands there. +The package lookup is still tried first, since it reports the jar actually loaded rather than the one +the build pinned. Verified against the built `consent.jar`: the package version is null there, +reproducing the production symptom exactly, and the fallback resolves 9.4.4. **A re-run against +production should now name the client version rather than declining to.** + +The substantive question it would have answered is settled anyway: the pinned 9.4.4 client ran against +a cluster one minor older, a gap within the same major and the same shape as the 9.3.3 control-cluster +run recorded below, and every security call in this run succeeded. ## REST client compatibility — resolved @@ -372,7 +447,7 @@ itself evidence that the transport reaches `/_security` there. The report says a ## Decision -**Pending** — blocked on the `dev`, `staging`, and `production` rows above. The decision rule, +**Pending** — blocked on the `dev` and `staging` rows above; `production` is measured. The decision rule, fixed in advance so the measurement determines the outcome: | Measured state of the deployed clusters | Decision | @@ -386,14 +461,29 @@ fixed in advance so the measurement determines the outcome: | DLS/FLS accepted, enforcement **not checked** (`INFERRED_SUPPORTED` from a write-probe run) | **Not a decision.** Acceptance is not enforcement. Fix what the notes say stopped the check — usually an empty or unreadable dataset index — and re-run the write probes before recording anything. | | DLS/FLS licensed but `xpack.security.dls_fls.enabled=false` | **Epic E** until the setting is enabled. The license entitles the cluster to the feature; the setting switches it off cluster-wide, so it is an infra change, not a license one. | -Known so far: the local environment now falls in the **first** row — security enabled, DLS and FLS -licensed *and* observed enforced — which closes Ticket A-0 but decides nothing on its own. The rule -above is about the deployed clusters, and local's superuser credential is not the shared credential -any of them use. +Known so far: **local and production both fall in the first row** — security enabled, DLS and FLS +licensed *and* observed enforced. Production is the one that counts, since it is a deployed cluster +measured through its own deployment's credential; local closed Ticket A-0 but decides nothing on its +own. + +That leaves the decision genuinely pending rather than merely unrecorded. The rule turns on all three +deployed clusters, and two of them are unmeasured — if dev or staging is on a lower license tier, or +has `dls_fls.enabled` off, the outcome is the "environments disagree" row (**both** epics, Epic E as +the portable path) rather than a clean Epic D. Production being on Elastic Cloud enterprise makes +that a real possibility rather than a formality: it is the environment most likely to carry the +strongest license, so the others cannot be assumed to match it. + +One caveat that no further measurement will remove: production's credential is broadly privileged, so +its write probes have the same limitation local's did. They prove what the *cluster* licenses and +enforces — which is what the decision rule asks — but not what a least-privilege service credential +could do there. If that credential is ever narrowed, Epic D's minimum grant is `manage_own_api_key`. ## Notes for whoever runs this against the deployed clusters -- The shared `authUser` almost certainly does **not** hold `manage_security` or `manage_api_key`. +- The shared `authUser` may or may not hold `manage_security` or `manage_api_key` — this was expected + to be the binding constraint, and in production it turned out not to be. Do not carry that forward + as an assumption about dev or staging; measure each, and keep the per-environment specifics on the + ticket rather than in this file. The report's `cluster_privileges` block says exactly which it has, via `POST /_security/user/_has_privileges`. If it lacks them, that is itself a finding: Epic D's per-request key minting goes through `POST /_security/api_key`, which needs at minimum diff --git a/pom.xml b/pom.xml index b4d2578ae..312078093 100644 --- a/pom.xml +++ b/pom.xml @@ -28,6 +28,11 @@ 4.2.16.Final 3.8.0 12.1.11 + + 9.4.4 UTF-8 UTF-8 @@ -903,7 +908,7 @@ org.elasticsearch.client elasticsearch-rest-client - 9.4.4 + ${elasticsearch.rest.client.version} diff --git a/src/main/java/org/broadinstitute/consent/http/ConsentModule.java b/src/main/java/org/broadinstitute/consent/http/ConsentModule.java index 4fa63523a..18d6ca6d9 100644 --- a/src/main/java/org/broadinstitute/consent/http/ConsentModule.java +++ b/src/main/java/org/broadinstitute/consent/http/ConsentModule.java @@ -373,6 +373,17 @@ private DataUseMatcherV4 providesDataUseMatcherV4(DataUseUtil dataUseUtil) { * ElasticSearchHealthCheck} — takes this instance by injection rather than calling {@code * ElasticSearchSupport.createRestClient} itself. Closed on shutdown, since nothing else releases * those connections. + * + *

Ordering dependency: the shutdown hook is registered here, inside the provider, so it + * is registered only when this client is first provisioned. That is safe today because every + * consumer is reached from {@code ConsentApplication.run}, which resolves the resources and + * health checks before the server starts — so provisioning always happens while the lifecycle is + * still accepting registrations. It stops being safe if this client ever becomes reachable only + * from a path that first runs after startup: Dropwizard will not call {@code start()} on a {@link + * Managed} added after the lifecycle has started, and depending on when it was added it may not + * be stopped either, which would leak the connection pool and its threads. If that day comes, + * register the hook eagerly from {@code configure()} rather than moving the client's construction + * around. */ @Provides @Singleton diff --git a/src/main/java/org/broadinstitute/consent/http/service/ElasticSearchCapabilityService.java b/src/main/java/org/broadinstitute/consent/http/service/ElasticSearchCapabilityService.java index 7fb91eb2b..05865c476 100644 --- a/src/main/java/org/broadinstitute/consent/http/service/ElasticSearchCapabilityService.java +++ b/src/main/java/org/broadinstitute/consent/http/service/ElasticSearchCapabilityService.java @@ -1,11 +1,13 @@ package org.broadinstitute.consent.http.service; +import com.google.common.annotations.VisibleForTesting; import com.google.gson.JsonElement; import com.google.gson.JsonObject; import com.google.gson.JsonParser; import com.google.inject.Inject; import jakarta.ws.rs.HttpMethod; import java.io.IOException; +import java.io.InputStream; import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Arrays; @@ -14,6 +16,7 @@ import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Properties; import java.util.Set; import java.util.TreeMap; import java.util.UUID; @@ -112,6 +115,12 @@ public class ElasticSearchCapabilityService implements ConsentLogger { */ private static final String DLS_FLS_ENABLED_SETTING = "xpack.security.dls_fls.enabled"; + /** Build-time properties written from the pom by {@code properties-maven-plugin}. */ + private static final String BUILD_PROPERTIES_RESOURCE = "/mvn.properties"; + + /** The pom property carrying the pinned {@code elasticsearch-rest-client} version. */ + private static final String REST_CLIENT_VERSION_PROPERTY = "elasticsearch.rest.client.version"; + private static final String VERSION_FIELD = "version"; private static final String LICENSE_FIELD = "license"; private static final String USERNAME_FIELD = "username"; @@ -1315,7 +1324,11 @@ private EnforcementAttempt attemptEnforcement( return EnforcementAttempt.inconclusive(); } - String searchPath = "/%s/_search?size=1".formatted(request.index()); + // track_total_hits because the default caps hits.total.value at 10000: without it, a DLS filter + // that was accepted and then ignored on a larger index reports "10000 of N" rather than the + // number actually visible. The verdict is a 0-vs-non-0 test either way, but the evidence string + // is what gets read and quoted, so it has to be the real count. + String searchPath = "/%s/_search?size=1&track_total_hits=true".formatted(request.index()); ProbeResult search = probeAsApiKey(key.encoded(), HttpMethod.GET, searchPath); String evidence = "GET %s through %s -> %d".formatted(searchPath, request.keyLabel(), search.status()); @@ -1563,7 +1576,7 @@ private String edition( * real compatibility axis is major-version skew. */ private String restClientCompatibility(String clusterVersion) { - String clientVersion = RestClient.class.getPackage().getImplementationVersion(); + String clientVersion = clientVersion(); if (clientVersion == null || clusterVersion == null) { return "Could not determine the client or cluster version at runtime. This report was itself " + "produced through RestClient.performRequest, so the transport reaches /_security."; @@ -1580,6 +1593,49 @@ private String restClientCompatibility(String clusterVersion) { + "over HTTP, but confirm the skew is within the supported range."; } + /** + * The version of the bundled {@code elasticsearch-rest-client}, from whichever source survives + * the build in hand. + * + *

The client's own {@code Package} is the more truthful of the two, since it reports the jar + * actually loaded rather than the version the build pinned — but it is only populated when that + * jar keeps its manifest. The shade plugin strips dependency manifests when assembling the + * deployable uber jar, so in every deployed environment this returns null and the build-time + * property is all there is. Falling back keeps the report from going indeterminate exactly where + * it is being used to make a decision. + */ + private String clientVersion() { + String packageVersion = RestClient.class.getPackage().getImplementationVersion(); + if (packageVersion != null) { + return packageVersion; + } + return buildProperty(REST_CLIENT_VERSION_PROPERTY); + } + + /** + * Reads a value from {@code mvn.properties}, which {@code properties-maven-plugin} writes from + * the pom at build time into both {@code target/classes} and {@code target/test-classes}. Returns + * null rather than throwing: an unreadable build property costs the report one field, and is not + * a reason to fail an inventory whose other verdicts were measured against the live cluster. + */ + @VisibleForTesting + String buildProperty(String name) { + try (InputStream is = getClass().getResourceAsStream(BUILD_PROPERTIES_RESOURCE)) { + if (is == null) { + return null; + } + Properties properties = new Properties(); + properties.load(is); + String value = properties.getProperty(name); + return (value == null || value.isBlank()) ? null : value; + } catch (IOException e) { + logWarn( + "Could not read %s from %s: %s" + .formatted(name, BUILD_PROPERTIES_RESOURCE, e.getMessage())); + return null; + } + } + private String recommendation( boolean securityApiPresent, String licenseType, diff --git a/src/test/java/org/broadinstitute/consent/http/service/ElasticSearchCapabilityServiceTest.java b/src/test/java/org/broadinstitute/consent/http/service/ElasticSearchCapabilityServiceTest.java index 0eece3aa6..dacb69322 100644 --- a/src/test/java/org/broadinstitute/consent/http/service/ElasticSearchCapabilityServiceTest.java +++ b/src/test/java/org/broadinstitute/consent/http/service/ElasticSearchCapabilityServiceTest.java @@ -35,6 +35,8 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; @@ -61,8 +63,9 @@ class ElasticSearchCapabilityServiceTest { private static final String INVALIDATE_KEY = "DELETE /_security/api_key"; private static final String CREATE_ROLE = "PUT /_security/role"; private static final String DELETE_ROLE = "DELETE /_security/role"; + private static final String REST_CLIENT_VERSION_PROPERTY = "elasticsearch.rest.client.version"; private static final String COUNT = "/dataset/_count"; - private static final String SEARCH = "/dataset/_search?size=1"; + private static final String SEARCH = "/dataset/_search?size=1&track_total_hits=true"; private static final String ROOT_BODY = """ @@ -107,6 +110,11 @@ private ElasticSearchCapabilityService service() throws IOException { return new ElasticSearchCapabilityService(esClient, config, this::apiKeyClient); } + /** For the checks that never reach the cluster, so the client stub would go unused. */ + private ElasticSearchCapabilityService serviceWithoutClientStubs() { + return new ElasticSearchCapabilityService(esClient, config, this::apiKeyClient); + } + /** * Answers from {@link #stubs}, turning a stubbed non-2xx into the ResponseException ES throws. */ @@ -199,12 +207,16 @@ private List requestsTo(String method, String endpointPrefix) { * Only the error path builds a {@link ResponseException}, which is what reads the request line; * the 2xx path reads just the status and entity. Stubbing the request line only for non-2xx * responses keeps every stub read by the test it's created for, so no stub needs to be lenient. + * + *

A null body is an entity-less response, which is a shape a real cluster returns and a + * distinct one from an empty JSON body: it is the null the parser has to survive. */ private Response response(int status, String body) { Response response = mock(Response.class); when(response.getStatusLine()) .thenReturn(new BasicStatusLine(HttpVersion.HTTP_1_1, status, "reason")); - when(response.getEntity()).thenReturn(new StringEntity(body, ContentType.APPLICATION_JSON)); + when(response.getEntity()) + .thenReturn(body == null ? null : new StringEntity(body, ContentType.APPLICATION_JSON)); if (status >= 300) { when(response.getRequestLine()) .thenReturn(new BasicRequestLine("GET", "/", HttpVersion.HTTP_1_1)); @@ -216,6 +228,11 @@ private void stub(String key, int status, String body) { stubs.put(key, new StubResponse(status, body)); } + /** A response that carries no entity at all, rather than an empty JSON one. */ + private void stubWithNoBody(String key, int status) { + stubs.put(key, new StubResponse(status, null)); + } + private void onceServed(String key, Runnable sideEffect) { sideEffects.put(key, sideEffect); } @@ -1058,29 +1075,6 @@ void testUnparseableSearchResponseIsUnknownRatherThanNotEnforced() throws IOExce assertTrue(dls.detail().contains("no hit total"), dls.detail()); } - @Test - void testFlsWithNoInspectableFieldsFallsBackAndSaysWhy() throws IOException { - stubSecurityEnabledCluster("trial"); - stubWorkingWriteProbes(); - // A hit with no _source at all: nothing to check the projection against. - stub( - "Zmxz|" + SEARCH, - 200, - """ - {"hits":{"total":{"value":2},"hits":[{"_id":"1"}]}}"""); - - ElasticSearchCapabilityReport report = service().getCapabilityReport(null, true); - - // Role acceptance stands, but the report must not let that read as proven enforcement. - assertEquals( - CapabilityVerdict.INFERRED_SUPPORTED, - capability(report, "Field-level security (FLS)").verdict()); - assertFalse(capability(report, "Field-level security (FLS)").detail().contains("Proven")); - assertTrue( - report.notes().stream().anyMatch(n -> n.contains("no document fields to inspect")), - "the reader must be told the projection check was inconclusive: " + report.notes()); - } - @Test void testMissingProbeKeyIsSaidToLimitTheDlsAndFlsVerdicts() throws IOException { stubSecurityEnabledCluster("trial"); @@ -1514,40 +1508,42 @@ void testHitCountReadsAPlainNumericTotalAsWellAsAnObjectShapedOne() throws IOExc assertEquals(CapabilityVerdict.SUPPORTED, dls.verdict()); } - @Test - void testFlsSearchWithNoHitsIsReportedAsInconclusiveRatherThanUnprojected() throws IOException { - stubSecurityEnabledCluster("trial"); - stubWorkingWriteProbes(); - // No "hits" array under "hits" at all: firstHitSourceFields() must fall back rather than throw. - stub( - "Zmxz|" + SEARCH, - 200, - """ - {"hits":{"total":{"value":0}}}"""); - - ElasticSearchCapabilityReport report = service().getCapabilityReport(null, true); - - assertTrue( - report.notes().stream() - .anyMatch(n -> n.contains("FLS projection check returned no document fields"))); - } - - @Test - void testFlsSearchWithAnEmptyHitsArrayIsReportedAsInconclusive() throws IOException { + /** + * Every search-response shape that leaves no document field to inspect. None of them is evidence + * about the projection either way, so each has to fall back to role acceptance and say so — and + * none may reach {@code firstHitSourceFields} as an exception or as a claim of enforcement. + */ + @ParameterizedTest(name = "{0}") + @ValueSource( + strings = { + // No "hits" object at all. + "{\"took\":1,\"timed_out\":false}", + // A "hits" object with no inner "hits" array. + "{\"hits\":{\"total\":{\"value\":0}}}", + // An inner "hits" that is present but not an array. + "{\"hits\":{\"total\":{\"value\":2},\"hits\":{\"unexpected\":\"shape\"}}}", + // An array with no documents in it. + "{\"hits\":{\"total\":{\"value\":0},\"hits\":[]}}", + // A first hit that is not an object. + "{\"hits\":{\"total\":{\"value\":2},\"hits\":[\"unexpected\"]}}", + // A hit with no _source, so nothing to check the projection against. + "{\"hits\":{\"total\":{\"value\":2},\"hits\":[{\"_id\":\"1\"}]}}" + }) + void testFlsSearchShapesWithNoInspectableFieldsFallBackAndSayWhy(String searchBody) + throws IOException { stubSecurityEnabledCluster("trial"); stubWorkingWriteProbes(); - // "hits" is present as an array, but empty: no document to read fields from. - stub( - "Zmxz|" + SEARCH, - 200, - """ - {"hits":{"total":{"value":0},"hits":[]}}"""); + stub("Zmxz|" + SEARCH, 200, searchBody); ElasticSearchCapabilityReport report = service().getCapabilityReport(null, true); + ElasticSearchCapability fls = capability(report, "Field-level security (FLS)"); + assertEquals(CapabilityVerdict.INFERRED_SUPPORTED, fls.verdict(), searchBody); + assertFalse(fls.detail().contains("Proven"), fls.detail()); assertTrue( report.notes().stream() - .anyMatch(n -> n.contains("FLS projection check returned no document fields"))); + .anyMatch(n -> n.contains("FLS projection check returned no document fields")), + "the reader must be told the projection check was inconclusive: " + report.notes()); } @Test @@ -1669,6 +1665,49 @@ void testMajorVersionSkewIsCalledOutRatherThanAssumedCompatible() throws IOExcep assertTrue(report.restClientCompatibility().contains("Major-version skew")); } + /** + * Guards the pom wiring rather than the Java. The client version is read from the shaded jar's + * missing manifest in every deployed environment, so {@code mvn.properties} is the only source + * that survives packaging — and it only carries the version because the pom declares it as a + * property instead of inline on the dependency. Inlining it again would silently return the + * report to "Could not determine" in exactly the environments it is used to make a decision in, + * and nothing else in the suite would notice. + */ + @Test + void testRestClientVersionSurvivesPackagingAsABuildProperty() { + String version = serviceWithoutClientStubs().buildProperty(REST_CLIENT_VERSION_PROPERTY); + + assertNotNull( + version, + "elasticsearch.rest.client.version is missing from mvn.properties; it must be declared as a " + + "pom property (not inline on the dependency) and listed in src/test/resources/" + + "mvn.properties, which shadows the generated file on the test classpath"); + assertTrue(version.matches("\\d+\\.\\d+.*"), "expected a version number, got: " + version); + } + + @Test + void testUnknownBuildPropertyIsReportedAsAbsentRatherThanThrowing() { + assertNull(serviceWithoutClientStubs().buildProperty("no.such.property")); + } + + @Test + void testMatchingMajorVersionNamesTheResolvedClientVersion() throws IOException { + stubSecurityDisabledCluster(); + String clientMajor = + serviceWithoutClientStubs().buildProperty(REST_CLIENT_VERSION_PROPERTY).split("\\.")[0]; + stub( + ROOT, + 200, + """ + {"cluster_name":"duos-cluster","version":{"number":"%s.0.0","build_flavor":"default"}}""" + .formatted(clientMajor)); + + ElasticSearchCapabilityReport report = service().getCapabilityReport(null, false); + + assertTrue(report.restClientCompatibility().startsWith("Compatible:")); + assertFalse(report.restClientCompatibility().contains("Could not determine")); + } + // --------------------------------------------------------------------------- // License state and trial activation // --------------------------------------------------------------------------- @@ -1956,5 +1995,650 @@ void testCapabilityReportNeverActivatesTheTrial() throws IOException { assertTrue(activationRequests().isEmpty(), "the capability report started a trial license"); } + // --------------------------------------------------------------------------- + // The default API-key client + // --------------------------------------------------------------------------- + + /** + * The injecting constructor's own factory, which every deployment uses and which the other write + * probe tests replace with a stub. Exercised against a closed port so no cluster is needed: what + * is under test is that the factory builds a usable client from the injected client's nodes at + * all, and that a client which cannot connect degrades to an unproven verdict rather than an + * exception escaping the report. + */ + @Test + void testTheDefaultApiKeyClientFactoryBuildsAClientFromTheInjectedClientsNodes() + throws IOException { + stubSecurityEnabledCluster("trial"); + stubWorkingWriteProbes(); + // The discard port: a connection here is refused immediately rather than hanging. + when(esClient.getNodes()).thenReturn(List.of(new Node(new HttpHost("127.0.0.1", 9, "http")))); + stubClient(esClient, this::keyFor); + + ElasticSearchCapabilityReport report = + new ElasticSearchCapabilityService(esClient, config).getCapabilityReport(null, true); + + ElasticSearchCapability apiKeys = capability(report, "API keys"); + assertEquals(CapabilityVerdict.UNKNOWN, apiKeys.verdict()); + assertTrue(apiKeys.detail().contains("could not authenticate"), apiKeys.detail()); + // The key was still minted, so it must still be invalidated. + assertEquals(1, requestsTo("DELETE", "/_security/api_key").size()); + } + + // --------------------------------------------------------------------------- + // License readings the cluster answers only partly + // --------------------------------------------------------------------------- + + /** + * A tier with no status is not the same as a tier that is inactive. Reading it as LICENSE_BLOCKED + * would state a finding about the cluster from a field the cluster never sent. + */ + @Test + void testMissingLicenseStatusLeavesDlsFlsUnknownRatherThanBlocked() throws IOException { + stubSecurityEnabledCluster("trial"); + stub( + LICENSE, + 200, + """ + {"license":{"type":"trial"}}"""); + + ElasticSearchCapabilityReport report = service().getCapabilityReport(null, false); + + for (String name : List.of("Document-level security (DLS)", "Field-level security (FLS)")) { + ElasticSearchCapability capability = capability(report, name); + assertEquals(CapabilityVerdict.UNKNOWN, capability.verdict(), name); + assertTrue(capability.detail().contains("license status could not be read"), name); + } + assertTrue(report.recommendation().contains("Inconclusive"), report.recommendation()); + assertTrue( + report.recommendation().contains("license status could not be read"), + report.recommendation()); + } + + /** A status of whitespace carries no more information than an absent one. */ + @Test + void testBlankLicenseStatusIsTreatedAsUnreadableRatherThanInactive() throws IOException { + stubSecurityEnabledCluster("trial"); + stub( + LICENSE, + 200, + """ + {"license":{"type":"trial","status":" "}}"""); + + ElasticSearchCapabilityReport report = service().getCapabilityReport(null, false); + + assertEquals( + CapabilityVerdict.UNKNOWN, capability(report, "Document-level security (DLS)").verdict()); + assertTrue(report.recommendation().contains("Inconclusive"), report.recommendation()); + } + + /** A status without a tier maps to no entitlement, and must not be guessed at either. */ + @Test + void testActiveLicenseWithNoTierReportedIsUnknownRatherThanBlocked() throws IOException { + stubSecurityEnabledCluster("trial"); + stub( + LICENSE, + 200, + """ + {"license":{"status":"active"}}"""); + + ElasticSearchCapabilityReport report = service().getCapabilityReport(null, false); + + assertNull(report.licenseType()); + ElasticSearchCapability dls = capability(report, "Document-level security (DLS)"); + assertEquals(CapabilityVerdict.UNKNOWN, dls.verdict()); + assertTrue(dls.detail().contains("could not be mapped"), dls.detail()); + } + + /** + * The edition is what a reader scans first, so a cluster whose license could not be read has to + * say "unknown" there rather than inherit a tier from somewhere else. + */ + @Test + void testUnreadableLicenseLeavesTheEditionUnknown() throws IOException { + stubSecurityEnabledCluster("trial"); + stub( + LICENSE, + 500, + """ + {"error":{"reason":"internal server error"}}"""); + + ElasticSearchCapabilityReport report = service().getCapabilityReport(null, false); + + assertEquals("unknown", report.edition()); + assertNull(report.licenseType()); + assertEquals( + CapabilityVerdict.UNKNOWN, capability(report, "Document-level security (DLS)").verdict()); + assertTrue(report.recommendation().contains("Inconclusive"), report.recommendation()); + } + + @Test + void testTrialStatusWithoutTheEligibilityFieldIsReportedAsUnknown() throws IOException { + stubLicense("basic", "active", null); + stub(TRIAL_STATUS, 200, "{}"); + + ElasticSearchLicenseStatus license = service().getLicenseStatus(); + + assertNull(license.trialAvailable(), "a missing flag must not read as an ineligible cluster"); + assertTrue(license.notes().stream().anyMatch(note -> note.contains("trial_status"))); + } + + @Test + void testANonPrimitiveEligibilityFlagIsReportedAsUnknown() throws IOException { + stubLicense("basic", "active", null); + stub( + TRIAL_STATUS, + 200, + """ + {"eligible_to_start_trial":{"unexpected":"shape"}}"""); + + ElasticSearchLicenseStatus license = service().getLicenseStatus(); + + assertNull(license.trialAvailable()); + } + + /** + * A 200 is not an activation. Only the cluster's own {@code trial_was_started} flag says a trial + * was started, so every 200 that does not carry it as a true boolean has to be reported as a + * non-activation — recording ACTIVATED from any of these would put an irreversible change that + * never happened into the per-environment record. + */ + @ParameterizedTest(name = "{0}") + @ValueSource( + strings = { + // The flag absent entirely. + "{\"acknowledged\":true}", + // The flag present and explicitly false. + "{\"acknowledged\":true,\"trial_was_started\":false}", + // The flag present but not a boolean. + "{\"trial_was_started\":{\"unexpected\":\"shape\"}}" + }) + void testA200WithoutATrueStartedFlagIsNotReportedAsActivated(String activationBody) + throws IOException { + stubLicense("basic", "active", true); + stub(START_TRIAL, 200, activationBody); + + ElasticSearchLicenseActivation activation = service().activateTrialLicense(); + + assertEquals(LicenseActivationOutcome.REFUSED, activation.outcome(), activationBody); + assertEquals("basic", activation.licenseAfter().licenseType()); + } + + // --------------------------------------------------------------------------- + // Edition and deployment shape + // --------------------------------------------------------------------------- + + /** + * {@code build_flavor} says the distribution directly, so an OSS build is reported as one even on + * a cluster whose {@code /_xpack} endpoint answers — the flavor is the more specific signal. + */ + @Test + void testAnOssBuildFlavorIsReportedAsOssEvenWhenTheXPackEndpointAnswers() throws IOException { + stubSecurityEnabledCluster("trial"); + stub( + ROOT, + 200, + """ + {"cluster_name":"duos-cluster","version":{"number":"9.3.3","build_flavor":"oss"}}"""); + + ElasticSearchCapabilityReport report = service().getCapabilityReport(null, false); + + assertEquals("OSS (no X-Pack endpoint)", report.edition()); + } + + /** Older clusters answer 400 rather than 404 for an endpoint they do not have. */ + @Test + void testAnXPack400IsReadAsAnOssBuildJustLikeA404() throws IOException { + stubSecurityDisabledCluster(); + stub( + XPACK, + 400, + """ + {"error":{"reason":"no handler found for uri [/_xpack]"}}"""); + + ElasticSearchCapabilityReport report = service().getCapabilityReport(null, false); + + assertEquals("OSS (no X-Pack endpoint)", report.edition()); + } + + /** + * A cloud ID present but empty is an unset configuration value, and reading it as Elastic Cloud + * would put a note in the report asserting a deployment shape that was never configured. + */ + @Test + void testABlankCloudIdIsNotReadAsAnElasticCloudDeployment() throws IOException { + config.setCloudId(" "); + stubSecurityEnabledCluster("trial"); + + ElasticSearchCapabilityReport report = service().getCapabilityReport(null, false); + + assertFalse(report.elasticCloud()); + assertEquals("trial", report.edition()); + assertTrue(report.notes().stream().noneMatch(n -> n.contains("cloud ID"))); + } + + /** + * A missing version cannot be compared, but nor may it be read as a cluster whose {@code version} + * block is simply shaped differently than expected. + */ + @Test + void testAPrimitiveWhereTheClusterShouldHaveSentAnObjectIsReadAsAbsent() throws IOException { + stubSecurityEnabledCluster("trial"); + stub( + ROOT, + 200, + """ + {"cluster_name":"duos-cluster","version":"9.3.3"}"""); + + ElasticSearchCapabilityReport report = service().getCapabilityReport(null, false); + + assertNull(report.version()); + assertTrue(report.restClientCompatibility().contains("Could not determine")); + } + + @Test + void testAnObjectWhereTheClusterShouldHaveSentAStringIsReadAsAbsent() throws IOException { + stubSecurityEnabledCluster("trial"); + stub( + ROOT, + 200, + """ + {"cluster_name":{"unexpected":"shape"},"version":{"number":"9.3.3"}}"""); + + ElasticSearchCapabilityReport report = service().getCapabilityReport(null, false); + + assertNull(report.clusterName()); + assertEquals("9.3.3", report.version()); + } + + @Test + void testRolesThatAreNotAnArrayAreReadAsNoRoles() throws IOException { + stubSecurityEnabledCluster("trial"); + stub( + AUTHENTICATE, + 200, + """ + {"username":"consent","roles":"superuser"}"""); + + ElasticSearchCapabilityReport report = service().getCapabilityReport(null, false); + + assertEquals("consent", report.authenticatedUser()); + assertTrue(report.authenticatedUserRoles().isEmpty()); + } + + // --------------------------------------------------------------------------- + // Probe scope and the security API's own answers + // --------------------------------------------------------------------------- + + /** + * {@code datasetIndexName} is {@code @NotNull} in the configuration, so this is only reachable + * from a hand-built one — and the fallback has to be a name no real index uses, because a probe + * that widened its own scope would run a DLS role and a search against a live index it was never + * pointed at. + */ + @Test + void testAnUnconfiguredIndexFallsBackToANamespacedNameRatherThanWideningScope() + throws IOException { + config.setDatasetIndexName(null); + stubSecurityEnabledCluster("trial"); + + service().getCapabilityReport(null, false); + + String body = bodyOf(requestsTo("POST", "/_security/user/_has_privileges").getFirst()); + assertTrue(body.contains("\"names\":[\"duos-capability-probe-index\"]"), body); + } + + @Test + void testABlankConfiguredIndexFallsBackTheSameWay() throws IOException { + config.setDatasetIndexName(" "); + stubSecurityEnabledCluster("trial"); + + service().getCapabilityReport(null, false); + + String body = bodyOf(requestsTo("POST", "/_security/user/_has_privileges").getFirst()); + assertTrue(body.contains("\"names\":[\"duos-capability-probe-index\"]"), body); + } + + /** + * A 401 is security answering, not security absent: an unauthenticated call to a security-enabled + * cluster is exactly what produces one. Reading it as an absent API would report every security + * verdict as UNAVAILABLE on a cluster that has security switched on. + */ + @Test + void testA401FromTheSecurityApiMeansSecurityIsPresentNotAbsent() throws IOException { + stubSecurityEnabledCluster("trial"); + stub( + AUTHENTICATE, + 401, + """ + {"error":{"reason":"missing authentication credentials"}}"""); + + ElasticSearchCapabilityReport report = service().getCapabilityReport(null, false); + + assertEquals(CapabilityVerdict.SUPPORTED, capability(report, "X-Pack Security").verdict()); + assertEquals( + CapabilityVerdict.INFERRED_SUPPORTED, + capability(report, "Document-level security (DLS)").verdict()); + assertTrue( + report.notes().stream().noneMatch(n -> n.contains("/_security API is not available")), + report.notes().toString()); + } + + @Test + void testA403FromTheSecurityApiMeansSecurityIsPresentNotAbsent() throws IOException { + stubSecurityEnabledCluster("trial"); + stub( + AUTHENTICATE, + 403, + """ + {"error":{"reason":"action is unauthorized for user [consent]"}}"""); + + ElasticSearchCapabilityReport report = service().getCapabilityReport(null, false); + + assertEquals(CapabilityVerdict.SUPPORTED, capability(report, "X-Pack Security").verdict()); + assertFalse(report.clusterPrivileges().isEmpty(), "the privilege probe should still be tried"); + } + + @Test + void testXPackAnswerWithoutASecurityFeatureBlockFallsBackToTheClusterSetting() + throws IOException { + stubSecurityEnabledCluster("trial"); + stub( + XPACK, + 200, + """ + {"features":{}}"""); + + ElasticSearchCapabilityReport report = service().getCapabilityReport(null, false); + + assertEquals(Boolean.TRUE, report.securityEnabled()); + } + + /** + * Neither source answering leaves the question genuinely open. A null is not a false: reported as + * one it would state that security is off on a cluster that said nothing either way. + */ + @Test + void testSecurityEnabledIsUnknownWhenNeitherXPackNorTheSettingsSayAnything() throws IOException { + stubSecurityEnabledCluster("trial"); + stub( + XPACK, + 200, + """ + {"features":{"security":{"available":true}}}"""); + stub( + SETTINGS, + 200, + """ + {"defaults":{"xpack.security.authc.api_key.enabled":"true"},"persistent":{}, + "transient":{}}"""); + + ElasticSearchCapabilityReport report = service().getCapabilityReport(null, false); + + assertNull(report.securityEnabled()); + // The /_security API still answered, so the feature verdicts stand on that rather than on a + // security-enabled flag nobody reported. + assertEquals(CapabilityVerdict.UNAVAILABLE, capability(report, "X-Pack Security").verdict()); + assertEquals( + CapabilityVerdict.INFERRED_SUPPORTED, + capability(report, "Document-level security (DLS)").verdict()); + } + + // --------------------------------------------------------------------------- + // Settings filtering + // --------------------------------------------------------------------------- + + /** A DLS/FLS setting outside the {@code xpack.security} namespace still gates the capability. */ + @Test + void testADlsFlsSettingOutsideTheXPackNamespaceIsStillReported() throws IOException { + stubSecurityEnabledCluster("trial"); + stub( + SETTINGS, + 200, + """ + {"defaults":{"xpack.security.enabled":"true"}, + "persistent":{"indices.dls_fls.enabled":"false"},"transient":{}}"""); + + ElasticSearchCapabilityReport report = service().getCapabilityReport(null, false); + + assertEquals("false", report.securitySettings().get("indices.dls_fls.enabled")); + } + + /** + * Audit-logfile settings are filtered even when explicitly configured: they describe log + * formatting rather than capability, and there are enough of them to bury the settings that do. + */ + @Test + void testExplicitlyConfiguredAuditLogfileSettingsAreStillFilteredOut() throws IOException { + stubSecurityEnabledCluster("trial"); + stub( + SETTINGS, + 200, + """ + {"defaults":{"xpack.security.enabled":"true"}, + "persistent":{"xpack.security.audit.logfile.events.include":"access_granted", + "xpack.security.audit.enabled":"true"},"transient":{}}"""); + + ElasticSearchCapabilityReport report = service().getCapabilityReport(null, false); + + assertFalse( + report.securitySettings().containsKey("xpack.security.audit.logfile.events.include"), + report.securitySettings().toString()); + assertEquals("true", report.securitySettings().get("xpack.security.audit.enabled")); + } + + /** Structured settings values are skipped rather than stringified into the report. */ + @Test + void testNonPrimitiveSettingValuesAreSkippedRatherThanStringified() throws IOException { + stubSecurityEnabledCluster("trial"); + stub( + SETTINGS, + 200, + """ + {"defaults":{"xpack.security.enabled":"true", + "xpack.security.authc.realms":{"native":{"native1":{"order":"0"}}}}, + "persistent":{},"transient":{}}"""); + + ElasticSearchCapabilityReport report = service().getCapabilityReport(null, false); + + assertEquals("true", report.securitySettings().get("xpack.security.enabled")); + assertFalse(report.securitySettings().containsKey("xpack.security.authc.realms")); + } + + // --------------------------------------------------------------------------- + // run_as target selection + // --------------------------------------------------------------------------- + + @Test + void testABlankRunAsUserFallsBackToTheAuthenticatedUser() throws IOException { + stubSecurityEnabledCluster("trial"); + + ElasticSearchCapabilityReport report = service().getCapabilityReport(" ", false); + + assertEquals(CapabilityVerdict.SUPPORTED, capability(report, "run_as impersonation").verdict()); + assertTrue( + requests.stream() + .flatMap(r -> r.getOptions().getHeaders().stream()) + .filter(h -> h.getName().equals("es-security-runas-user")) + .allMatch(h -> h.getValue().equals("consent")), + "a blank request should impersonate the authenticated user, not blank"); + } + + @Test + void testABlankAuthenticatedUserLeavesRunAsWithNoTarget() throws IOException { + stubSecurityEnabledCluster("trial"); + stub( + AUTHENTICATE, + 200, + """ + {"username":" ","roles":[]}"""); + + ElasticSearchCapabilityReport report = service().getCapabilityReport(" ", false); + + ElasticSearchCapability runAs = capability(report, "run_as impersonation"); + assertEquals(CapabilityVerdict.UNKNOWN, runAs.verdict()); + assertTrue(runAs.detail().contains("No target user was available"), runAs.detail()); + } + + // --------------------------------------------------------------------------- + // Write-probe refusals and response shapes + // --------------------------------------------------------------------------- + + /** PUT role answers 201 as readily as 200, and both mean the cluster took the filters. */ + @Test + void testARoleAcceptedWith201IsTreatedAsCreatedAndTornDown() throws IOException { + stubSecurityEnabledCluster("trial"); + stubWorkingWriteProbes(); + stub( + CREATE_ROLE, + 201, + """ + {"role":{"created":true}}"""); + + ElasticSearchCapabilityReport report = service().getCapabilityReport(null, true); + + assertEquals( + CapabilityVerdict.SUPPORTED, capability(report, "Document-level security (DLS)").verdict()); + assertEquals(1, requestsTo("DELETE", "/_security/role/").size()); + } + + @Test + void testA401OnKeyCreationIsReadAsAPrivilegeRefusal() throws IOException { + stubSecurityEnabledCluster("trial"); + stubWorkingWriteProbes(); + stub( + CREATE_KEY, + 401, + """ + {"error":{"reason":"missing authentication credentials"}}"""); + + ElasticSearchCapabilityReport report = service().getCapabilityReport(null, true); + + assertEquals(CapabilityVerdict.NOT_PERMITTED, capability(report, "API keys").verdict()); + } + + @Test + void testA401OnTheEnforcementSearchIsReadAsAPrivilegeRefusal() throws IOException { + stubSecurityEnabledCluster("trial"); + stubWorkingWriteProbes(); + stub( + "ZGxz|" + SEARCH, + 401, + """ + {"error":{"reason":"missing authentication credentials"}}"""); + + ElasticSearchCapabilityReport report = service().getCapabilityReport(null, true); + + ElasticSearchCapability dls = capability(report, "Document-level security (DLS)"); + assertEquals(CapabilityVerdict.NOT_PERMITTED, dls.verdict()); + assertTrue(dls.detail().contains("privilege rather than licensing"), dls.detail()); + } + + /** + * The licence/privilege split is what the whole report turns on, and a cluster does not always + * use the word "non-compliant" when it refuses on licensing grounds. Missing this refusal would + * report a licensing limit as a privilege one and send someone to fix the wrong thing. + */ + @Test + void testARefusalNamingTheLicenseWithoutTheWordNonCompliantIsStillALicenseBlock() + throws IOException { + stubSecurityEnabledCluster("basic"); + stub( + CREATE_KEY, + 500, + """ + {"error":{"reason":"internal server error"}}"""); + stub( + CREATE_ROLE, + 403, + """ + {"error":{"reason":"field and document level security requires a platinum license"}}"""); + + ElasticSearchCapabilityReport report = service().getCapabilityReport(null, true); + + ElasticSearchCapability dls = capability(report, "Document-level security (DLS)"); + assertEquals(CapabilityVerdict.LICENSE_BLOCKED, dls.verdict()); + assertTrue(dls.detail().contains("The license does not permit it"), dls.detail()); + } + + @Test + void testACountResponseWithoutACountFieldIsTreatedAsUnreadable() throws IOException { + stubSecurityEnabledCluster("trial"); + stubWorkingWriteProbes(); + stub(COUNT, 200, "{}"); + + ElasticSearchCapabilityReport report = service().getCapabilityReport(null, true); + + assertTrue( + report.notes().stream().anyMatch(n -> n.contains("not readable")), + report.notes().toString()); + assertEquals( + CapabilityVerdict.INFERRED_SUPPORTED, + capability(report, "Document-level security (DLS)").verdict()); + assertTrue(requestsTo("GET", "/dataset/_search").isEmpty()); + } + + /** A hit total whose shape is unrecognised must never become the "not enforced" verdict. */ + @Test + void testATotalObjectWithoutAValueIsUnknownRatherThanNotEnforced() throws IOException { + stubSecurityEnabledCluster("trial"); + stubWorkingWriteProbes(); + stub( + "ZGxz|" + SEARCH, + 200, + """ + {"hits":{"total":{}}}"""); + + ElasticSearchCapabilityReport report = service().getCapabilityReport(null, true); + + ElasticSearchCapability dls = capability(report, "Document-level security (DLS)"); + assertEquals(CapabilityVerdict.UNKNOWN, dls.verdict()); + assertTrue(dls.detail().contains("no hit total"), dls.detail()); + } + + @Test + void testATotalOfAnUnexpectedJsonTypeIsUnknownRatherThanNotEnforced() throws IOException { + stubSecurityEnabledCluster("trial"); + stubWorkingWriteProbes(); + stub( + "ZGxz|" + SEARCH, + 200, + """ + {"hits":{"total":["unexpected"]}}"""); + + ElasticSearchCapabilityReport report = service().getCapabilityReport(null, true); + + assertEquals( + CapabilityVerdict.UNKNOWN, capability(report, "Document-level security (DLS)").verdict()); + } + + // --------------------------------------------------------------------------- + // Transport-level response shapes + // --------------------------------------------------------------------------- + + /** A response with no entity at all, which is not the same as one with an empty JSON body. */ + @Test + void testAResponseCarryingNoEntityIsHandledAsAnEmptyBody() throws IOException { + stubSecurityEnabledCluster("trial"); + stubWithNoBody(HAS_PRIVILEGES, 200); + + ElasticSearchCapabilityReport report = service().getCapabilityReport(null, false); + + assertTrue(report.clusterPrivileges().isEmpty()); + // A credential whose privileges could not be read is not a credential known to lack them. + assertEquals(CapabilityVerdict.INFERRED_SUPPORTED, capability(report, "API keys").verdict()); + } + + /** Valid JSON that is not an object is as unusable as invalid JSON, and must not throw either. */ + @Test + void testAJsonBodyThatIsNotAnObjectIsHandledAsAnEmptyBody() throws IOException { + stubSecurityEnabledCluster("trial"); + stub(HAS_PRIVILEGES, 200, "[\"unexpected\"]"); + + ElasticSearchCapabilityReport report = service().getCapabilityReport(null, false); + + assertTrue(report.clusterPrivileges().isEmpty()); + } + private record StubResponse(int status, String body) {} } diff --git a/src/test/resources/mvn.properties b/src/test/resources/mvn.properties index 642f12b91..06f8b5c43 100644 --- a/src/test/resources/mvn.properties +++ b/src/test/resources/mvn.properties @@ -1,2 +1,8 @@ #Properties +# +# This file shadows the mvn.properties that properties-maven-plugin generates into +# target/test-classes, because test resources are copied after generate-resources. The deployed jar +# gets the plugin's full property set; tests get only what is listed here, so a property read at +# runtime has to be added in both places. swagger.ui.path=${swagger.ui.path} +elasticsearch.rest.client.version=${elasticsearch.rest.client.version}